Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload rules atomically and verify rules before deploy #10

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

traylenator
Copy link
Collaborator

@traylenator traylenator commented Nov 18, 2020

Background: The unit file for nftables on CentOS 8 contains:

ExecStart=/sbin/nft -f /etc/sysconfig/nftables.conf
ExecReload=/sbin/nft 'flush ruleset; include "/etc/sysconfig/nftables.conf";'
ExecStop=/sbin/nft flush ruleset

As things stood on config modification systemctl stop nftables ; systemctl start nftables was being
called resulting in:

nft flush ruleset
nft -f /etc/sysconfig/nftables.conf

as distinct commands so a non-atomic flush and load of ruleset.

With this change it is now.

/sbin/nft 'flush ruleset; include "/etc/sysconfig/nftables.conf";'

Also added is validation of the ruleset by puppet prior to updating the real configuration.

Configuration is deployed to /etc/nftables/puppet-preflight/ and /etc/nftables/puppet-preflight.nft

This is validate with nft -c and if and only if the configuration is valid in the nft sense the configuration
will be copied to the live location /etc/nftables/puppet/ and /etc/nftables/puppet.nft before the service
is reloaded.

@traylenator
Copy link
Collaborator Author

I'm not very proud of that restart line so happy to take suggestions.

@traylenator traylenator marked this pull request as draft November 18, 2020 09:25
@traylenator
Copy link
Collaborator Author

Still thinking about this so draft for now.

@duritong
Copy link
Collaborator

Yeah, this is generally an issue with Puppet, since Puppet forgets that it wasn't able to reload the firewall the run before and since nothing changes in the next run there is no need to re-run the reload it will never be done.

We also have this issue in the shorewall module and what we did there, was to periodically run a shorewall check as cron and notify whether it succeeded or not. I don't think this is something we should re-implement in the nftables module.

Also I would not like to reload the tables on each run.

But since nft supports a check run:

nft -h
[...]
-c, --check			Check commands validity without actually applying the changes.

I would suggest to trigger a nft check on any change before trying to restart the nftables service OR when the current running version does not match the current running version.

The latter could be done through concat putting things down into a temporary file (e.g. /etc/nftables/puppet.nft.stage), that is then checked, and once successfully copied (through file resource) to /etc/nftables/puppet.nft) and when this one is updated the service is restarted.

manifests/init.pp Outdated Show resolved Hide resolved
@duritong
Copy link
Collaborator

But I also like your idea, as it would change the file, so we would ensure to try to update it again...

@traylenator
Copy link
Collaborator Author

Also I would not like to reload the tables on each run.

Just to be clear this will not reload every time, only when the config is changed or broken as that always triggers a change.
Very possible that is exactly what you were saying.

Thanks for the other comments, will digest.

@traylenator
Copy link
Collaborator Author

Adding the intermediate file to do a validation test before service. I will add the #CONFIG BROKEN in that file so we do still get the multiple attempts every puppet run.

@traylenator traylenator changed the title Reload rules atomically Reload rules atomically and verify rules before deploy Nov 24, 2020
@traylenator
Copy link
Collaborator Author

traylenator commented Nov 24, 2020

So now.

  1. configuration is deployed to /etc/nftables/puppet-preflight.nft and /etc/nftables/puppet/preflight/
  2. /usr/sbin/nft -I /etc/nftables/puppet-preflight -c -f /etc/nftables/puppet-preflight.nft ....
  3. Copy same configuration to /etc/nftables/puppet.nft and /etc/nftables/puppet
  4. reload or start on that.

The result is okay. , if I add some rubbish config:

Error: /Stage[main]/Nftables/Exec[nft validate]: Failed to call refresh: '/usr/sbin/nft -I /etc/nftables/puppet-preflight -c -f /etc/nftables/puppet-preflight.nft || ( /usr
/bin/echo "#CONFIG BROKEN" >> /etc/nftables/puppet-preflight.nft && /bin/false)' returned 1 instead of one of [0]                                                           
Error: /Stage[main]/Nftables/Exec[nft validate]: '/usr/sbin/nft -I /etc/nftables/puppet-preflight -c -f /etc/nftables/puppet-preflight.nft || ( /usr/bin/echo "#CONFIG BROKE
N" >> /etc/nftables/puppet-preflight.nft && /bin/false)' returned 1 instead of one of [0]                                                                                   
Notice: /Stage[main]/Nftables/File[/etc/nftables/puppet.nft]: Dependency Exec[nft validate] has failures: true                   

and 2nd puppet run is exactly the same till fixed.

The particular awkward ones are the includes called inside /etc/nftables/puppet

It's all a bit convoluted.

The only real advantage of the 3rd commit over the 2nd commit is it covers the reboot case with broken rules.

I'm sure there's a brilliant idea in there somewhere.

For info when using relative includes it is relative to the directory nft is running in. The -I adds an extra directory.

@duritong duritong requested a review from keachi November 24, 2020 20:32
@duritong
Copy link
Collaborator

Excellent, this looks great!

Copy link
Collaborator

@keachi keachi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@traylenator
Copy link
Collaborator Author

One more commit I think.
I don't like that if you run

nft -f /etc/nftables/puppet.nft

It does not fail but loads a completely wrong rule set.
Will make puppet.nft contain relative includes also so the above command fails and you have to specify the -I parameter.

@duritong
Copy link
Collaborator

It does not fail but loads a completely wrong rule set.
Will make puppet.nft contain relative includes also so the above command fails and you have to specify the -I parameter.

Yeah, this could be confusing (if not even dangerous), but if we can have a workaround: even better.

Background: The unit file for nftables on CentOS 8 contains:

```
ExecStart=/sbin/nft -f /etc/sysconfig/nftables.conf
ExecReload=/sbin/nft 'flush ruleset; include "/etc/sysconfig/nftables.conf";'
ExecStop=/sbin/nft flush ruleset
```

As things stood on config modication `systemctl stop nftables ; systemctl start nftables` was being
called resulting in:

```
nft flush ruleset
nft -f /etc/sysconfig/nftables.conf
```

as distinct commands so a non-atomic flush and load of ruleset.

With this change it is now.

```
/sbin/nft 'flush ruleset; include "/etc/sysconfig/nftables.conf";'
```

There is subsequently a redundant extra 'flush ruleset' in there to be followed
up in a seperate patch as I have desire to make that tunable.

To verify what happens when broken rules have been applied, e.g

```puppet
nftables::rule{'default_in-junk':
  content =>  'junk',
}
```

This results in puppet run of

```
Error: /Stage[main]/Nftables/Service[nftables]: Failed to call refresh: Systemd restart for nftables failed!
journalctl log for nftables:
```

and the existing rules are left live, previously the flush from the stop was occuring.

The reload attempt would only happen once however leaving a time bomb at reboot.
To resvole this the configuration is modified to force a reconfig and reload every puppet run.
@traylenator
Copy link
Collaborator Author

nft -c -f /etc/nftables/puppet.nft 

now fails hard.

/etc/nftables/puppet.nft:11:1-26: Error: File not found: inet-filter.nft
include "inet-filter.nft"
^^^^^^^^^^^^^^^^^^^^^^^^^^
/etc/nftables/puppet.nft:13:1-22: Error: File not found: ip6-nat.nft
include "ip6-nat.nft"
^^^^^^^^^^^^^^^^^^^^^^

and there are comments in the file about that you need to run:

nft -c -I /etc/nftables/puppet -f /etc/nftables/puppet.nft 

to get the desired result

All rebased.

This migrates better on puppet6 than on puppet5 as the systemctl daemon-reload is not called early enough for the service reload.
We are puppet 6 from yesterday.

@traylenator traylenator marked this pull request as ready for review November 26, 2020 10:37
@duritong duritong merged commit bd54947 into voxpupuli:master Nov 26, 2020
@duritong
Copy link
Collaborator

great stuff!

@traylenator traylenator added the enhancement New feature or request label Dec 10, 2020
@traylenator traylenator deleted the reload branch December 15, 2020 10:29
figless pushed a commit to figless/puppet-nftables that referenced this pull request Aug 25, 2021
7395300 Merge pull request voxpupuli#25 from cernops/no_nat
82d1065 Allow disabling default NAT tables and chains
bd54947 Merge pull request voxpupuli#10 from traylenator/reload
30462da Reload rules atomically

git-subtree-dir: code
git-subtree-split: 7395300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants