Fix daemon startup on a no-IPv6 host #49525
Conversation
robmry
added
area/networking
|
|
||
| // Install the rules to isolate this network against each of the other networks | ||
| if n.driver.config.EnableIPTables { | ||
| if n.driver.config.EnableIPTables && (!enable || n.config.EnableIPv4) { |
There was a problem hiding this comment.
Should we use thisConfig here? (also in the other branch below)?
I must say that the 3 booleans combined make it hard to grasp what exactly the conditions are (perhaps a comment or breaking to a separate condition could help 🤔);
- We only manipulate if IP(6)tables is enabled
- And we want to disable, or (explicitly) enable
Is there a case where n.config.EnableIPv4 = false means we must disable things?
There was a problem hiding this comment.
I'll have a look ... but the intention is to only add the rules if iptables is enabled, and the network has IPv4 enabled.
But, in case the rule was created by an older version, it's unconditionally deleted if iptables is enabled (because it was unconditionally added before the EnableIPv4 check was added).
There was a problem hiding this comment.
I've split the condition and added a comment ... does that help?
There was a problem hiding this comment.
Should we use thisConfig here? (also in the other branch below)?
There are a few other code paths that locks the network before accessing its config, but it's not really needed because this field is never written after struct initialization:
Anyway, let's keep it as is now it's been changed. We should probably refactor all those code paths after dealing with 28.0.1.
robmry
force-pushed
the
startup_ip6_disabled
branch
from
de48643 to
a8c5cac
Compare
| // Only create the rules if the network has IPv4 enabled. But, always delete | ||
| // rules, in case they were set up by an older daemon that didn't check whether | ||
| // the network has IPv4. | ||
| if !enable || thisConfig.EnableIPv4 { |
There was a problem hiding this comment.
Does enable == true mean isolated and enabled == false not isolated, or the reverse?
I know this wasn't changed by this PR, but maybe we could rename it to make it a little bit more clear?
There was a problem hiding this comment.
enable == true means 'create the rules'.
enabled == false means 'delete the rules'. It's used to rollback something fails down the line, or when the network is deleted.
| // Only create the rules if the network has IPv4 enabled. But, always delete | ||
| // rules, in case they were set up by an older daemon that didn't check whether | ||
| // the network has IPv4. | ||
| if !enable || thisConfig.EnableIPv4 { |
There was a problem hiding this comment.
I find this version more confusing than the previous was, and now the comment right before if n.driver.config.EnableIPTables contradicts the inner comment as it talks about "installing" while the inner comment mentions the delete case.
But that's usual level of bikeshedding I guess, so whatever feels best for you seems okay to me.
There was a problem hiding this comment.
I've deleted the comment above, which mostly duplicated the function's comment - and updated the function's comment to say what "enable" does.
There was a problem hiding this comment.
(Also rebased - so I'll let the tests run, then merge.)
With ip6tables enabled (by default), the daemon should start on a host with IPv6 disabled - then error if IPv6 network creation is attempted. That regressed in commit 255fff4 - so, only try to set up network isolation rules for a network if it's IPv6-enabled. Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
startup_ip6_disabled
branch
from
a8c5cac to
8cc4d1d
Compare
4 participants
- What I did
With ip6tables enabled (by default), the daemon should start on a host with IPv6 disabled - then error if IPv6 network creation is attempted.
In 28.0.0, it fails with ...
- How I did it
In 27.x, the error wasn't propagated properly, a
nilwas returned following an attempt to roll-back the rule that wasn't created, so startup continued. Commit 255fff4 fixed the error propagation, breaking startup in this case.The underlying problem was a missing check that the network had IPv6 enabled for INC rule setup. So, added that check.
- How to verify it
Start the daemon on host with IPv6 disabled.
- Human readable description for the release notes