In firewalld_conf, we validate the input. For example,

  if not value or value.lower() not in ["no", "false", "yes", "true"]:

However, no normalization happens. That means, the value can be set to
"No", "False" or "yes". That means, everybody down the road has to
perform the same validation of those strings. Which is worse, since this
is open coded here, so they have to duplicate the implementation of how
a boolean looks like.

Also, for example "IPv6_rpfilter" will default to "yes"/"no" due to

  self.set("IPv6_rpfilter", "yes" if config.FALLBACK_IPV6_RPFILTER else "no")

while "FlushAllOnReload" will default to "True"/"False" due to

  self.set("FlushAllOnReload", str(config.FALLBACK_FLUSH_ALL_ON_RELOAD))

Instead, normalize the values while validating them. Booleans get a
"yes"/"no" value. But we anyway should check booleans with
firewall.functions.str_to_bool() and not rely on this.

While at it, add helper functions to not repeat this over and over.

Also, for enums strip whitespace and allow values in upper case (but
they will be normalize to lower-case).

Also, booleans now use firewall.functions.str_to_bool(), which is the
new way for how a boolean string looks like. This also accepts values
like "1", "on" or "y", because, if a user configures such a value, their
intent is pretty clear and no point in being picky.

…lesFlowtable"

All supported keys should also be checked by _normalize().

This was never used. Drop it.

…irewalld_conf class

The next commit will add key metadata, which is static (depending not on
an instance of firewalld_conf). While could be nested inside
firewalld_conf class, it's better to have it at module level.

That meta data will need the _parse_reload_policy() function. It's
awkward, if those are inside the firewalld_conf class. Move it.

Instead of repeating the validation of each key over and over, add some
meta data (KeyType) for how to handle them generically.

Certain keys, like "ReloadPolicy",key_type=_parse_reload_policy are not
really re-used. However, those are the minority. We should instead aim
for simple properties (int, bool), which have a common behavior.

The default() and normalize() functions are not fully generic. Instead,
based on the key_type we switch on a hand full of behaviors.
Theoretically, we could go overboard and create various subclasses with
virtual functions. In practice, this approach is fine and pretty
extensible.

Note that firewalld_conf.set() always strips the string value. Normalize
does that too, and ensures that the value is always stripped.

Fixes: 0019371 ('feat(fw): add ReloadPolicy option in firewalld.conf')

These keys are API. No caller should try to have there bogus whitespace.
A lookup via "    LogDenied" is a bug, not something that the
get()/set() functions should try to workaround.

…irewalld_conf

Previously, the key meta data for firewalld.conf and D-Bus was separate.
While you might think that this is better, because then firewalld_conf
is independent from the D-Bus API, it is not.

It is worse, because the firewalld.conf entries are exposed on D-Bus.
And that is not some happenstance but something fundamental. Code
doesn't get simpler, by pretending those things would be separate.

The benefit is, when now you review the code to see what keys exist, you
also can see how this is exposed on D-Bus. It is not detached at some
remote place. Likewise, when adding a new key, it becomes immediately
clear, that you also must consider how this will be exposed on D-Bus.

And really, firewalld_conf still has no relevant logic about D-Bus. It
merely has a bit of D-Bus related meta data telling how this property
would look on D-Bus. There is now `import dbus` in "firewalld_conf.py",
but only because "dbus_type=dbus.String" is more useful than an
alternative type annotation like "dbus_type='s'" or "dbus_type=str".

…tly list deprecated keys

There should be one place, where we define whether a key is deprecated.
We have that information in valid_keys. Use it.

…n D-Bus setter

Don't duplicate how those values are handled. Instead, re-use
firewald_conf's normalize() code.

Aside dropping redundant code, this also fixes:

  $ busctl get-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload
  s "yes"
  $ busctl set-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload "s" fAlse
  $ busctl get-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload
  s "fAlse"

There really is no use in ever allowing un-normalized values or invalid
values.

Let firewalld_conf.set() always try to normalize the value. By default,
the function is strict, that means, on invalid value an exception will
be raised.

Note that normalize() always returns appropriately stripped values.
Hence it's wrong for set() trying to strip the value itself.

Internally, the values are all strings and normalize() ensures that.

Now that we know that the values are normalized, we also know that
a boolean key is strictly "yes" or "no".

Still, it's slightly inconvenient. Add "get_as()" helper, that can
convert those strings to booleans. Note that this only accepts
converting the value, if the keytype is also boolean.

…d.conf"

Don't re-implement the parsing of the values. Granted, now boolean
settings are normalized to "yes" / "no", but still, let's use `get_as()`
which internally also checks that the requested value is really a
boolean type.

Also, don't access the default values like
config.FALLBACK_CLEANUP_ON_EXIT. The key meta data in firewalld_conf
knows what the default value of a key is. Get it from there.

Note that previously there was code like:

        if self._firewalld_conf.get("IPv6_rpfilter"):
            value = self._firewalld_conf.get("IPv6_rpfilter")

as if the key could be unset. But it cannot. Currently, firewalld_conf
will always set all known keys (possibly to their default). Maybe it
would make sense to keep track whether a key is unset in the file or
whether a value was set explicitly. But that does not (currently) exist.
And having code that depends that it could be, is not useful.

set_log_denied() should not re-implement the validation of the valid
enum values. At least,

   if value not in config.LOG_DENIED_VALUE:

is not a suitable check, and does not support the nuances (like
stripping whitespace or accepting any letter case).

Instead, use normalize() on the value.

- don't access the redundant config.FALLBACK_* fields. Instead, these
  values are keys from firewalld_conf's valid_keys. Get the default
  value from there.

- don't check whether the setting is set before getting it. It is always
  set, because firewalld_conf will fill it with default values.

As input "value" for SetLogDenied() different variants are supported.
When we emit the LogDeniedChanged signal, we should use the normalized
value.

Also, in set_log_denied(), update self._log_denied. It doesn't really
matter, beccause set_log_denied() is collowed by fw.reload(). Still.

This was always empty and never had any effect.

…tribute

By calling the getter function get_log_denied(), we can easily see (during a
git-grep) that those callers only read the value (and don't modify/set
it). Use the setter, instead of the attribute directly.

…ort_key()

In most cases, the log_denied argument is a string like "off" or "unicast",
from firewalld.conf.

In these cases, it was a boolean.

Rename the "log_denied" argument to "do_log_denied", as it has a different
semantic than what we usually expect.

…an properties in "firewalld.conf"

When a user sets a boolean property to "", "default" or "-1", their
intent is pretty clear. In the file, that should have the same meaning
as if the value is not set and silently use the default value. Note that
previously, we would have also used the default value, but logged a
warning that the value was not accepted.

Note that this also affects the D-Bus API, where the property setters
end up calling the same code:

    $ busctl get-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload
    s "yes"
    $ busctl set-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload "s" false
    $ busctl get-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload
    s "no"
    $ busctl set-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload "s" default
    $ busctl get-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload
    s "yes"

Previously, we would get

    $ busctl set-property org.fedoraproject.FirewallD1 /org/fedoraproject/FirewallD1/config org.fedoraproject.FirewallD1.config FlushAllOnReload "s" default
    Failed to set property FlushAllOnReload on interface org.fedoraproject.FirewallD1.config: INVALID_VALUE: 'default' for FlushAllOnReload

… more exactly

We will next add a new option to firewalld.conf, named "LogDomainGroup".
Note that we already have "LogDomain", and some tests did

   sed -i 's/^LogDenied.*/LogDenied=all/' ./firewalld.conf

This will replace the wrong occurrences.

Instead, adjust the regular expressions and match more specifically to
the text we want to replace.

…enied value itself

We have many functions that take a log_denied argument. On the other hand,
we have many places that directly call self._fw.get_log_denied().

So you might think, it would be better to avoid accessing the global
get_log_denied() at many places, instead only fetch it at the highest
level and pass it down as arguments. The deeper down in the callstack
we are, the less we want to access the global variable.

However, if you fallow that through, you will see that a large amount
of functions will need to get a new log_denied argument. It's a lot
of churn. Instead, it's fine to call self._fw.get_log_denied(), because
that is just a getter (without side effects) of a variable that only changes
during reload().

Hence, let _pkttype_match_fragment() look up the value itself, when the
caller doesn't have a value at hand.

Also, rename the "pkttype" argument to "log_denied". The argument really
holds a value from LogDenied setting, and it needs special if-checks to
map it to the pkttype for the nft API.

This test is a copy of "src/tests/regression/gh940.at".

At this point, the test is still identical to "gh940.at" (except
changing the test name and keywords). In the next commit, it will be
adjusted. We add first the original LOG based test, so we can see
how the next commit will adjust the test for NFLOG.