Recipes/ENRT/ConfigMixins: Add FirewallMixin.py #364
Conversation
not really... this lnst repository doesn't really have "self tests" we run all of our recipes regularly and typically also use those to schedule a test run in beaker with the modified code from the PR to provide a proof of testing.
what exactly do you mean by dumped? do you want to print the information for "debug purposes" or just because it could be "interesting"? or is this information IMPORTANT in some way and should be relevant for the PASS/FAIL of a recipe? This also provides context for what to do with
as if these are eventually meant to do some form of MEASUREMENT which we want to regularly evaluate... we'd want to look into integrating these tests into the If these are "significantly" different, we probably want to create a new test set .... |
|
On Tue, Mar 12, 2024 at 03:14:52AM -0700, Ondrej Lichtner wrote:
> Is there a test suite I could integrate any unit tests for the config mixin?
not really... this lnst repository doesn't really have "self tests" we run all of our recipes regularly and typically also use those to schedule a test run in beaker with the modified code from the PR to provide a proof of testing.
> The applied ruleset should be dumped
what exactly do you mean by dumped?
Logged or stored for later.
do you want to print the information for "debug purposes" or just because it could be "interesting"? or is this information IMPORTANT in some way and should be relevant for the PASS/FAIL of a recipe?
Users may want to see stateful ruleset data (e.g., counter or quota
values) after the test run has finished, to verify correctness of the
test or qualify results. They may be used to check e.g. how many packets
were offloaded.
I didn't find other recipes/mixins collecting data after the fact (aside
from the obvious perf/ping results). Is this just off-topic or do tests
collect such data by themselves?
This also provides context for what to do with
> I wrote a few hello world recipes for testing, they're here: SirPhuttel@630fb76
as if these are eventually meant to do some form of MEASUREMENT which we want to regularly evaluate... we'd want to look into integrating these tests into the `lnst.Recipes.ENRT` test set
If these are "significantly" different, we probably want to create a new test set .... `lnst.Recipes.Firewalls` for example...
I wrote those for the purpose of testing the config mixin. I don't have
a clear picture of how it will be used eventually, aside from previous
standalone tests I had written.
Take e.g. hello_world_router.py: I created an env of three VMs, two
having a dedicated link, so I could implement this routing benchmark.
Maybe I missed it, but I haven't seen other Recipes setting up a
client-router-server env. With netfilter, exercising the whole routing
path is crucial, and that's not possible with just client-server setup.
Not sure if I answered your questions, but I hope you can more clearly
see the "loose ends" in my plan now. :)
Thanks, Phil
|
|
On Tue, Mar 12, 2024 at 04:49:25AM -0700, Ondrej Lichtner wrote:
@olichtne requested changes on this pull request.
> + The rulesets will be applied by a derived class's _apply_ruleset()
+ method, i.e. typically fed into 'nft -f' or 'iptables-restore'.
+ """
+ return {}
+
+ def _apply_ruleset(self, host, path):
+ raise NotImplementedError("Method _apply_ruleset must be defined by a child class.")
+
+ def _dump_ruleset(self, host, config):
+ raise NotImplementedError("Method _dump_ruleset must be defined by a child class.")
+
+ def _push_ruleset(self, host, ruleset):
+ """
+ Push `ruleset` into a temporary file on `host`, returning its path.
+ """
+ fd, name = mkstemp(prefix="FirewallMixin", text=True)
why use the lower level `mkstemp` function and manual call of `close` ? wouldn't the context manager way make this a bit simpler?
```python
with NamedTemporaryFile(prefix="FirewallMixin") as fd:
write(fd, ruleset.encode())
close(fd)
remote_path = host.copy_file_to_machine(fd.name)
```
You meant manual call of unlink(), right? Seems sensible, though I don't
see the benefit (but I see everything through procedural goggles
anyway).
> + Push `ruleset` into a temporary file on `host`, returning its path.
+ """
+ fd, name = mkstemp(prefix="FirewallMixin", text=True)
+ write(fd, ruleset.encode('utf-8'))
+ close(fd)
+ remote_path = host.copy_file_to_machine(name)
+ unlink(name)
+ return remote_path
+
+ def apply_sub_configuration(self, config):
+ super().apply_sub_configuration(config)
+
+ if not config.firewall_rulesets:
+ config.firewall_rulesets = self.firewall_rulesets
+
+ self.stored_rulesets = {}
`stored_rulesets` should also probably be part of the `config` object
ACK.
> + for host, ruleset in config.firewall_rulesets.items():
+ desc.append("Firewall: ruleset with %d lines on host %s" % (len(ruleset.split("\n")), host))
+
+ return desc
+
+ def remove_sub_configuration(self, config):
+ for host, ruleset in self.stored_rulesets.items():
+ # XXX: this may spam debug log, but counter values may be worth logging
+ logging.debug("Dropping Firewall ruleset on host %s:\n%s\n" % (host, self._dump_ruleset(host, config)))
+ name = self._push_ruleset(host, ruleset)
+ self._apply_ruleset(host, name)
+ host.run('rm -f "%s"' % name)
+
+ return super().remove_sub_configuration(config)
+
+class BulkFirewallMixin(FirewallMixin):
i feel like this class - the generator method and the subconfig generator should be part of the base `FirewallMixin` class... i mean, without this, the subclasses don't really do anything on their own, and with this addition, they would still by default do the default "nothing" operation.
But I'm not sure if you had some more specific reason to separate this into its own class?
Hmm. I started with just FirewallMixin and implemented the bulk variant
in a second step. The non-bulk variant just used self.firewall_rulesets,
and with the addition of the bulk variant I started adding the current
ruleset to config. Hence the conditional config.firewall_rulesets
population in apply_sub_configuration(). I guess unifying them and
making the generator by default just use self.firewall_rulesets would
indeed simplify things.
> +
+ def _dump_ruleset(self, host, config):
+ return host.run('nft list ruleset').stdout
+
+class Ip46tablesMixin(FirewallMixin):
+ """
+ A common base class for all the iptables-like FirewallMixin backends.
+ Do not inherit directly, use one of the *tablesMixin classes instead.
+ """
+
+ @Property
+ def iptables_command(self):
+ raise NotImplementedError("Property iptables_command must be defined by a child class.")
+
+ def _apply_ruleset(self, host, path):
+ host.run('%s-restore "%s"' % (self.iptables_command, path))
this would look nicer as an f-string:
```
host.run(f'{self.iptables_command}-restore "{path}"')
```
ACK.
> + new_config.firewall_rulesets = ruleset
+ yield new_config
+
+class NftablesMixin(FirewallMixin):
+ """
+ An nftables backend for FirewallMixin.
+ """
+
+ def _apply_ruleset(self, host, path):
+ host.run('nft flush ruleset')
+ host.run('nft -f "%s"' % path)
+
+ def _dump_ruleset(self, host, config):
+ return host.run('nft list ruleset').stdout
+
+class Ip46tablesMixin(FirewallMixin):
would it make more sense to name this as `IptablesBaseMixin` instead? seeing that there's also `ebtables` and `arptables` that inherit from this?
Good point, thanks! It's also consistent with other such base-classes.
> + def iptables_command(self):
+ raise NotImplementedError("Property iptables_command must be defined by a child class.")
+
+ def _apply_ruleset(self, host, path):
+ host.run('%s-restore "%s"' % (self.iptables_command, path))
+
+ def _extract_tables_n_chains(self, ruleset):
+ out = {}
+ curtable = None
+ for line in ruleset.split('\n'):
+ if line[0] == '*':
+ out[line] = []
+ elif line[0] == ':':
+ words = line.split(' ')
+ if not words[1] == '-': # ignore user-defined chains
+ out[curtable].append(words[0][1:])
`curtable` is always `None` here is that correct?
Oh, that's indeed totally broken.
[...]
+ def _append_missing_parts(self, dst_ruleset, src_ruleset):
+ for table, chains in self._extract_tables_n_chains(src_ruleset).items():
+ tline = '*' + table + '\n'
the `table` here is coming from https://github.com/LNST-project/lnst/pull/364/files#diff-c1ca3132235b422fab430b7645942020bf1635e314f37db433a3ca03575f4e2dR129 which i think means that it fits the format of `*....` with the `\n` cut off
It's more or less the same bug: I should have extracted the table name
into curtable and use that as key instead of the whole input line.
but here you assign `tline = '*' + "*...."` so you'll end up with double `*` characters is that correct?
Definitely not. Sorry for the buggy code, I surely tested this at some
point. Looks like some last minute rewrite in between. :(
> + unlink(name)
+ return remote_path
+
+ def apply_sub_configuration(self, config):
+ super().apply_sub_configuration(config)
+
+ if not config.firewall_rulesets:
+ config.firewall_rulesets = self.firewall_rulesets
+
+ self.stored_rulesets = {}
+ for host, ruleset in config.firewall_rulesets.items():
+ self.stored_rulesets[host] = self._dump_ruleset(host, config)
+
+ name = self._push_ruleset(host, ruleset)
+ self._apply_ruleset(host, name)
+ host.run('rm -f "%s"' % name)
this is what removes the pushed files - i think this answers your other comment in https://github.com/LNST-project/lnst/pull/364/files#r1519638084
I wasn't sure if Jan suggested to instead register all those temp files
for removal in a common step but wanted to implement the suggested
changes first for a more informed reply.
BTW: I'm doing this temp file thing merely because it's not possible to
feed data via stdin to processes spawned by host.run(). But given the
multiple layers and possible unwanted side-effects of feeding large data
this way I guess the temp file method is the better choice, anyway.
Thanks, Phil
|
yes, i mean the manual call of unlink() The benefit is that in case there's an unhandled exception raised somewhere for some reason, the When you're calling the
yeah, i think the non-bulk variant is simply a special case of the bulk variant which is why it could be merged together.
no worries, this is why we do reviews :)
yeah... at some point it is a bit more beneficial to write a module/class that runs directly on the agent instead of running code on the controller and sending data to the agent, i think this isn't very well documented but you can do this: class MyCustomCodeClass():
def method1():
logging.debug("this runs on the agent")
class MyRecipe():
def test():
host1_custom_class = self.host1.init_class(MyCustomCodeClass)
host1_custom_class.method()more complex example can be seen in: https://github.com/LNST-project/lnst/blob/master/lnst/Recipes/ENRT/BasePvPRecipe.py#L144 which uses this "code class": https://github.com/LNST-project/lnst/blob/master/lnst/RecipeCommon/LibvirtControl.py |
they currently don't... but if we want to do something with the data from the e.g. if a test result is OBVIOUSLY FAIL because "a zero was reported with |
Accept a string in exec_cmd() parameter 'stdin' to feed into the called process.
SirPhuttel
force-pushed
the
FirewallMixin
branch
from
43a57b0 to
571e4d5
Compare
The set of classes contained in this module enable users to push firewall rulesets onto the hosts used for test runs. There are five flavors to choose from for the actual firewall implementation, with descriptive names: * NftablesMixin * IptablesMixin * Ip6tablesMixin * EbtablesMixin * ArptablesMixin To use the provided functionality, inherit one of them and either provide a dictionary of "host: ruleset" entries in a property 'firewall_rulesets' or overwrite the 'firewall_rulesets_generator()' method with a generator yielding as many dictionaries a desired for performing a test run with. The latter provides a simple means of testing with gradually increasing or extending rulesets. In order to perform post-processing on rulesets after a test run, override the 'firewall_rulesets' setter method.
SirPhuttel
force-pushed
the
FirewallMixin
branch
from
571e4d5 to
f23034a
Compare
|
if there's no other comments from @jtluka tomorrow i can merge this in. |
The set of classes contained in this module enable users to push
firewall rulesets onto the hosts used for test runs.
There are five flavors to choose from for the actual firewall
implementation, with descriptive names:
To use the provided functionality, inherit one of them and either
provide a dictionary of "host: ruleset" entries in a property
'firewall_rulesets' or overwrite the 'firewall_rulesets_generator()'
method with a generator yielding as many dictionaries a desired for
performing a test run with. The latter provides a simple means of
testing with gradually increasing or extending rulesets.
In order to perform post-processing on rulesets after a test run,
override the 'firewall_rulesets' setter method.
Changes since v1:
skip empty ruleset lines
Tests
I wrote a few hello world recipes for testing, they're here:
SirPhuttel@ea21b0e
Bugs
Overriding a parent's class's property is not easy if a setter implementation
is also present. For instance, when inheriting NftablesMixin and only
overriding the getter, it has to be decorated using
'@NftablesMixin.firewall_rulesets.getter' instead of just '@Property'. When
overriding both getter and setter though, the expected '@Property' and
'@firewall_rulesets.setter' suffice.
Reviews
@jtluka
@enhaut
@olichtne
(Thanks in advance!)