added firewall-reload command #1301
Conversation
Reviewer's GuideThis PR introduces a new Sequence diagram for systemd-triggered nftables firewall reloadsequenceDiagram
participant systemd as systemd
participant nftables as nftables.service
participant reload as netavark-nftables-reload.service
participant netavark as netavark (firewall-reload)
systemd->>nftables: Start or reload nftables.service
nftables-->>reload: Triggers netavark-nftables-reload.service
reload->>netavark: Execute 'netavark firewall-reload'
netavark->>netavark: Read /run/containers/netavark/
netavark->>netavark: Re-apply firewall rules for all networks
netavark-->>reload: Exit
reload-->>nftables: Complete
nftables-->>systemd: Complete
Class diagram for new firewall-reload command integrationclassDiagram
class Main {
+main()
}
class SubCommand {
+FirewallReload
}
class firewall_reload {
+firewall_reload(config_dir: Option<OsString>) NetavarkResult<()>
}
class firewalld_reload {
+listen(config_dir: Option<OsString>) NetavarkResult<()>
}
Main --> SubCommand
SubCommand --> firewall_reload : FirewallReload variant
Main --> firewall_reload : calls firewall_reload()
SubCommand --> firewalld_reload : FirewallDReload variant
Main --> firewalld_reload : calls listen()
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Rishikpulhani - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/commands/firewall_reload.rs:29` </location>
<code_context>
+
+ // Call the reload logic. We pass `None` for the D-Bus connection
+ // because we are on an nftables system and don't need it.
+ reload_rules(config_dir);
+
+ Ok(())
</code_context>
<issue_to_address>
The return value of reload_rules is ignored, which may hide errors.
If systemd relies on exit codes to detect failures, propagate errors from reload_rules to ensure systemd can respond appropriately.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Call the reload logic. We pass `None` for the D-Bus connection
// because we are on an nftables system and don't need it.
reload_rules(config_dir);
Ok(())
}
// This function is copied directly from firewalld_reload.rs.
// It's a simple wrapper to handle and log any errors.
fn reload_rules(config_dir: &Path) {
if let Err(e) = reload_rules_inner(config_dir) {
log::error!("failed to reload firewall rules: {e}");
}
}
=======
// Call the reload logic. We pass `None` for the D-Bus connection
// because we are on an nftables system and don't need it.
reload_rules(config_dir)?;
Ok(())
}
// This function is copied directly from firewalld_reload.rs.
// It's a simple wrapper to handle and log any errors.
fn reload_rules(config_dir: &Path) -> Result<(), Error> {
reload_rules_inner(config_dir).map_err(|e| {
log::error!("failed to reload firewall rules: {e}");
e
})
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/commands/firewall_reload.rs:43` </location>
<code_context>
+}
+
+// This is the core logic, also copied directly.
+// We keep the `conn` parameter for compatibility with the firewall driver trait,
+// but it will be `None` and won't be used by the nftables driver.
+fn reload_rules_inner(config_dir: &Path) -> NetavarkResult<()> {
+ // read_fw_config reads all the JSON files from `/run/containers/netavark/`
+ let conf = read_fw_config(config_dir).wrap("read firewall config")?;
</code_context>
<issue_to_address>
The comment about the unused `conn` parameter is outdated.
The function no longer has a `conn` parameter, so the comment should be updated to reflect the current signature.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/commands/firewall_reload.rs
Outdated
| // We keep the `conn` parameter for compatibility with the firewall driver trait, | ||
| // but it will be `None` and won't be used by the nftables driver. | ||
| fn reload_rules_inner(config_dir: &Path) -> NetavarkResult<()> { |
There was a problem hiding this comment.
nitpick: The comment about the unused conn parameter is outdated.
The function no longer has a conn parameter, so the comment should be updated to reflect the current signature.
Rishikpulhani
force-pushed
the
fix-nftables-reload
branch
2 times, most recently
from
7e16902 to
0ec3879
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
There was a problem hiding this comment.
Can you remove the extra merge commit and rebase instead to keep a clean git history please.
Also this is going to need a test under test/
For example in in test/250-bridge-nftables.bats you could do a normal setup like most other test, then run_in_host_netns nft flush ruleset (or how the command is called to clean all rules). Then call netavark firewall-reload and then check that the nft rules exist again.
src/commands/firewall_reload.rs
Outdated
| }; | ||
|
|
||
| // This is our new main entry point. It's a "oneshot" function. | ||
| // It will be called once by systemd, do its job, and then exit. |
There was a problem hiding this comment.
The fact that it is called by systemd should not matter here, any user can call this.
|
|
||
| [Install] | ||
| # This tells systemd that this service should be enabled when nftables is enabled. | ||
| WantedBy=nftables.service |
There was a problem hiding this comment.
Oh and missing newline here'
Did you test that the unit dependencies and such actually work correctly?
There was a problem hiding this comment.
@Luap99
Yes, I tested the unit dependencies and they are working correctly.
|
Thank you for the feedback, @Luap99. I will make the suggested changes and add the tests shortly. |
Rishikpulhani
force-pushed
the
fix-nftables-reload
branch
5 times, most recently
from
61caae2 to
4c78798
Compare
test/250-bridge-nftables.bats
Outdated
| # check that the firewall config files exist | ||
| run_helper ls -A "$NETAVARK_TMPDIR/config/firewall/networks/" | ||
| assert "$output" != "" "network config file should exist" |
There was a problem hiding this comment.
a positive match is almost always better, the file name should be based on the id so you should be able to lookup the full file path and ensure it exists instead of allowing for any path.
src/commands/firewall_reload.rs
Outdated
| // Get the appropriate firewall driver (it will auto-detect nftables). | ||
| let fw_driver = get_supported_firewall_driver(Some(conf.driver))?; | ||
|
|
||
| // Loop through each network configuration and restore its rules. | ||
| for net in conf.net_confs { | ||
| fw_driver.setup_network(net, &None)?; | ||
| } | ||
| // Loop through each container's port mappings and restore them. | ||
| for port in &conf.port_confs { | ||
| fw_driver.setup_port_forward(port.into(), &None)?; | ||
| } |
There was a problem hiding this comment.
it doesn't auto detect nftables so the comment is misleading, nftables is one driver it might use, but the code is generic so it will work for any driver.
And passing None is not right, we should call let conn = Connection::system().ok() and then pass this, that is needed to ensure it correctly adds the rule to the trusted zone in firewalld.
There was a problem hiding this comment.
ahh actually while making this PR I had in mind the case where this code would only be used when firewalld is absent from the system, so I thought it would be fine to just pass None directly.
There was a problem hiding this comment.
That is most likely correct for the majority of users but it cost us nothing to pass the real connection in case it is needed so doing that is fine.
Rishikpulhani
force-pushed
the
fix-nftables-reload
branch
2 times, most recently
from
05ecade to
5ab3192
Compare
On systems using nftables directly without firewalld, restarting the nftables.service would flush all of Netavark's rules, breaking container networking. This introduces a new "oneshot" command, `netavark firewall-reload`, which reads the container network state from /run/containers/netavark/ and re-applies all necessary firewall rules. To automate this, a new systemd service, `netavark-nftables-reload.service`, is added. This service is procedurally linked to `nftables.service` and triggers the reload command automatically whenever the main nftables service is started or reloaded. Fixes: containers#1258 Signed-off-by: Rishikpulhani <[email protected]>
Rishikpulhani
force-pushed
the
fix-nftables-reload
branch
from
5ab3192 to
3439753
Compare
|
@Luap99 I’ve made the requested changes. Could you please review it again ? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, Rishikpulhani, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
On systems using nftables directly without firewalld, restarting the nftables.service would flush all of Netavark's rules, breaking container networking.
This introduces a new "oneshot" command,
netavark firewall-reload, which reads the container network state from /run/containers/netavark/ and re-applies all necessary firewall rules.To automate this, a new systemd service,
netavark-nftables-reload.service, is added. This service is procedurally linked tonftables.serviceand triggers the reload command automatically whenever the main nftables service is started or reloaded.Fixes: #1258
Summary by Sourcery
Add a oneshot firewall-reload mechanism for nftables-only systems to restore container networking after nftables service restarts
New Features:
Enhancements: