UDN LGW: ensure masq chain exists before adding rules #4697
Conversation
martinkennelly
added
the
feature/user-defined-network-segmentation
Prior to this PR, we may try to insert a rule to jump to a chain that doesn't exist. Signed-off-by: Martin Kennelly <mkennell@redhat.com>
martinkennelly
force-pushed
the
ensure-chain-udn
branch
from
2e9ab22
to
24b4749
Compare
| @@ -79,6 +79,18 @@ func deleteIptRules(rules []nodeipt.Rule) error { | |||
| return nodeipt.DelRules(rules) | |||
| } | |||
|
|
|||
| // ensureChain ensures that a chain exists within a table | |||
| func ensureChain(table, chain string) error { | |||
There was a problem hiding this comment.
I think we already have a util for this somewhere no?
I thought insertIpt does chain creation automatically... that was why I didn't create the chain explicitly how is this crash transient and goes away eventually?
There was a problem hiding this comment.
I couldnt find any.
If its a rule for a chain then itll auto create the chain, but if its a jump, it wont create the chain you jump to.
I assume it hit a rule during init startup that then created the chain but the error inserting the jump rule happened earlier and following reboot of the container, the new chain is there.
There was a problem hiding this comment.
could you use the IPTablesHelper function NewChain() similar to
ovn-kubernetes/go-controller/pkg/node/iptables/iptables.go
Lines 108 to 111 in 244efcf
There was a problem hiding this comment.
If its a rule for a chain then itll auto create the chain, but if its a jump, it wont create the chain you jump to. I assume it hit a rule during init startup that then created the chain but the error inserting the jump rule happened earlier and following reboot of the container, the new chain is there.
yea that's it. thanks martin for rca
There was a problem hiding this comment.
@JacobTanenbaum yea
func addChaintoTable(ipt util.IPTablesHelper, tableName, chain string) {
if err := ipt.NewChain(tableName, chain); err != nil {
klog.V(5).Infof("Chain: \"%s\" in table: \"%s\" already exists, skipping creation: %v", chain, tableName, err)
}
}
he is indirectly using that
| if err != nil { | ||
| return fmt.Errorf("failed to get IPTables helper to add UDN chain: %v", err) | ||
| } | ||
| addChaintoTable(ipt, table, chain) |
There was a problem hiding this comment.
ah! I was talking about addChainToTable...OK looks like there isn't a wrapper for that and hence this happens..
There was a problem hiding this comment.
I know it is unrelated to this change but addChaintoTable ignores all errors, not only the ones where the chain already exists. I feel like it would be best to introduce a new method to the helper, something like:
// EnsureChain creates a new chain if it doesn't already exist
func (ipt *IPTables) EnsureChain(table, chain string) error {
exists, err := ipt.ChainExists(table, chain)
if err != nil || exists {
return err
}
return ipt.NewChain(table, chain)
}
Another option is to actually compare the error value in addChaintoTable.
This doesn't have to happen in this PR.
|
/assign @kyrtapz who takes the credit for spotting this. |
| @@ -26,6 +26,11 @@ func newLocalGateway(nodeName string, hostSubnets []*net.IPNet, gwNextHops []net | |||
| klog.Info("Creating new local gateway") | |||
| gw := &gateway{} | |||
|
|
|||
| if util.IsNetworkSegmentationSupportEnabled() { | |||
| if err := ensureChain("nat", iptableUDNMasqueradeChain); err != nil { | |||
There was a problem hiding this comment.
NIT: Wouldn't it make more sense to move this to somewhere in
?There was a problem hiding this comment.
i think the purpose of that func was generate the rules and apply them, hence i put the creation of a chain outside it.
| if err != nil { | ||
| return fmt.Errorf("failed to get IPTables helper to add UDN chain: %v", err) | ||
| } | ||
| addChaintoTable(ipt, table, chain) |
There was a problem hiding this comment.
I know it is unrelated to this change but addChaintoTable ignores all errors, not only the ones where the chain already exists. I feel like it would be best to introduce a new method to the helper, something like:
// EnsureChain creates a new chain if it doesn't already exist
func (ipt *IPTables) EnsureChain(table, chain string) error {
exists, err := ipt.ChainExists(table, chain)
if err != nil || exists {
return err
}
return ipt.NewChain(table, chain)
}
Another option is to actually compare the error value in addChaintoTable.
This doesn't have to happen in this PR.
|
Ill follow up with another PR to address the issue pat brought up. |
cc @tssurya
Fixes #4695