Skip to content

Commit

Permalink
Fix issue with ipset or iptables chain removal during NodeNetworkPoli…
Browse files Browse the repository at this point in the history 
…cy updates or deletions

This commit addresses an issue where stale ipset or iptables chain is
not deleted during NodeNetworkPolicy updates or deletions. The root cause
is that the ipset or iptables chain is still referenced by other iptables
rules during the deletion or update attempt. The fix ensures proper order
of ipset and iptables synchronization.

Signed-off-by: Hongliang Liu <[email protected]>
  • Loading branch information
@hongliangl
hongliangl committed Sep 30, 2024
1 parent 1dad03b commit 87da3fe
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 32 deletions.
34 changes: 25 additions & 9 deletions pkg/agent/controller/networkpolicy/node_reconciler_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,13 @@ func (r *nodeReconciler) Forget(ruleID string) error {
if err := r.deleteCoreIPTRule(ruleID, coreIPTChain, isIPv6); err != nil {
return err
}
if lastRealized.ipsets[ipProtocol] != "" {
if err := r.routeClient.DeleteNodeNetworkPolicyIPSet(lastRealized.ipsets[ipProtocol], isIPv6); err != nil {
if lastRealized.serviceIPTChain != "" {
if err := r.routeClient.DeleteNodeNetworkPolicyIPTables([]string{lastRealized.serviceIPTChain}, isIPv6); err != nil {
return err
}
}
if lastRealized.serviceIPTChain != "" {
if err := r.routeClient.DeleteNodeNetworkPolicyIPTables([]string{lastRealized.serviceIPTChain}, isIPv6); err != nil {
if lastRealized.ipsets[ipProtocol] != "" {
if err := r.routeClient.DeleteNodeNetworkPolicyIPSet(lastRealized.ipsets[ipProtocol], isIPv6); err != nil {
return err
}
}
Expand Down Expand Up @@ -442,19 +442,35 @@ func (r *nodeReconciler) update(lastRealized *nodePolicyLastRealized, newRule *C
ipnet := newLastRealized.ipnets[ipProtocol]
prevIPSet := lastRealized.ipsets[ipProtocol]
ipset := newLastRealized.ipsets[ipProtocol]

shouldUpdateCoreIPTRules := prevIPSet != ipset || prevIPNet != ipnet
// The name of ipset for a rule will not change during updates.
if ipset != "" {
// If the current rule uses an ipset, sync the ipset first, then sync the core iptables rule that
// references it.
if err := r.routeClient.AddOrUpdateNodeNetworkPolicyIPSet(iptRule.IPSet, iptRule.IPSetMembers, iptRule.IsIPv6); err != nil {
return err
}
if shouldUpdateCoreIPTRules {
if err := r.addOrUpdateCoreIPTRules(iptRule.CoreIPTChain, iptRule.IsIPv6, true, &coreIPTRule{ruleID, iptRule.Priority, iptRule.CoreIPTRule}); err != nil {
return err
}
}
} else if prevIPSet != "" {
// If the previous rule used an ipset, sync the core iptables rule first to remove its reference, then
// delete the unused ipset.
if shouldUpdateCoreIPTRules {
if err := r.addOrUpdateCoreIPTRules(iptRule.CoreIPTChain, iptRule.IsIPv6, true, &coreIPTRule{ruleID, iptRule.Priority, iptRule.CoreIPTRule}); err != nil {
return err
}
}
if err := r.routeClient.DeleteNodeNetworkPolicyIPSet(lastRealized.ipsets[ipProtocol], iptRule.IsIPv6); err != nil {
return err
}
}
if prevIPSet != ipset || prevIPNet != ipnet {
if err := r.addOrUpdateCoreIPTRules(iptRule.CoreIPTChain, iptRule.IsIPv6, true, &coreIPTRule{ruleID, iptRule.Priority, iptRule.CoreIPTRule}); err != nil {
return err
} else {
if shouldUpdateCoreIPTRules {
if err := r.addOrUpdateCoreIPTRules(iptRule.CoreIPTChain, iptRule.IsIPv6, true, &coreIPTRule{ruleID, iptRule.Priority, iptRule.CoreIPTRule}); err != nil {
return err
}
}
}
}
Expand Down
56 changes: 33 additions & 23 deletions pkg/agent/controller/networkpolicy/node_reconciler_linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,12 +267,17 @@ func TestNodeReconcilerReconcileAndForget(t *testing.T) {
`-A ANTREA-POL-INGRESS-RULES -m set --match-set ANTREA-POL-INGRESSRULE1-4 src -j ANTREA-POL-INGRESSRULE1 -m comment --comment "Antrea: for rule ingress-rule-01, policy AntreaClusterNetworkPolicy:name1"`,
},
}
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE1-4", sets.New[string]("1.1.1.1/32", "192.168.1.0/25"), false).Times(1)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESSRULE1"}, serviceRules, false).Times(1)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules, false).Times(1)
mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE1-4", false)
mockRouteClient.DeleteNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESSRULE1"}, false).Times(1)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, [][]string{nil}, false).Times(1)
s1p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE1-4", sets.New[string]("1.1.1.1/32", "192.168.1.0/25"), false).Times(1)
s1p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESSRULE1"}, serviceRules, false).Times(1)
s1p3 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules, false).Times(1)
s2p1 := mockRouteClient.DeleteNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESSRULE1"}, false).Times(1)
s2p2 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE1-4", false)
s2p3 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, [][]string{nil}, false).Times(1)
s1p2.After(s1p1)
s1p3.After(s1p2)
s2p1.After(s1p3)
s2p2.After(s2p1)
s2p3.After(s1p2)
},
rulesToAdd: []*CompletedRule{
ingressRule1,
Expand All @@ -297,10 +302,13 @@ func TestNodeReconcilerReconcileAndForget(t *testing.T) {
`-A ANTREA-POL-EGRESS-RULES -d 2002:1a23:fb44::1/128 -j ANTREA-POL-EGRESSRULE1 -m comment --comment "Antrea: for rule egress-rule-01, policy AntreaClusterNetworkPolicy:name1"`,
},
}
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESSRULE1"}, serviceRules, true).Times(1)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESS-RULES"}, coreRules, true).Times(1)
mockRouteClient.DeleteNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESSRULE1"}, true).Times(1)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESS-RULES"}, [][]string{nil}, true).Times(1)
s1p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESSRULE1"}, serviceRules, true).Times(1)
s1p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESS-RULES"}, coreRules, true).Times(1)
s2p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESS-RULES"}, [][]string{nil}, true).Times(1)
s2p2 := mockRouteClient.DeleteNodeNetworkPolicyIPTables([]string{"ANTREA-POL-EGRESSRULE1"}, true).Times(1)
s1p2.After(s1p1)
s2p1.After(s1p2)
s2p2.After(s2p1)
},
rulesToAdd: []*CompletedRule{
egressRule1,
Expand Down Expand Up @@ -570,39 +578,41 @@ func TestNodeReconcilerReconcileAndForget(t *testing.T) {
s4p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", sets.New[string]("1.1.1.1/32", "1.1.1.2/32"), false).Times(1)
s4p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules4, false).Times(1)
s5 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", sets.New[string]("1.1.1.2/32", "1.1.1.3/32"), false).Times(1)
s6p1 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s6p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules6, false).Times(1)
s6p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules6, false).Times(1)
s6p2 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s7 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules7, false).Times(1)
s8p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", sets.New[string]("1.1.1.1/32", "1.1.1.2/32"), false).Times(1)
s8p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules8, false).Times(1)
s9p1 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s9p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules9, false).Times(1)
s9p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules9, false).Times(1)
s9p2 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s10 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules10, false).Times(1)
s11p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", sets.New[string]("1.1.1.1/32", "1.1.1.2/32"), false).Times(1)
s11p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules11, false).Times(1)
s12p1 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s12p2 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules12, false).Times(1)
s12p1 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules12, false).Times(1)
s12p2 := mockRouteClient.DeleteNodeNetworkPolicyIPSet("ANTREA-POL-INGRESSRULE3-4", false).Times(1)
s13 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, coreRules13, false).Times(1)
s14 := mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, [][]string{nil}, false).Times(1)
s2.After(s1)
s3.After(s2)
s4p1.After(s3)
s4p2.After(s3)
s4p2.After(s4p1)
s5.After(s4p2)
s5.After(s4p2)
s6p1.After(s5)
s6p2.After(s5)
s6p2.After(s6p1)
s7.After(s6p2)
s8p1.After(s7)
s8p2.After(s7)
s8p2.After(s8p1)
s9p1.After(s8p2)
s9p2.After(s8p2)
s9p2.After(s9p1)
s10.After(s9p2)
s11p1.After(s10)
s11p2.After(s10)
s11p2.After(s11p1)
s12p2.After(s12p1)
s12p1.After(s11p2)
s12p2.After(s11p2)
s12p2.After(s12p1)
s13.After(s12p2)
mockRouteClient.AddOrUpdateNodeNetworkPolicyIPTables([]string{"ANTREA-POL-INGRESS-RULES"}, [][]string{nil}, false).Times(1)
s14.After(s13)
},
rulesToAdd: []*CompletedRule{
ingressRule3WithFromAnyAddress,
Expand Down

0 comments on commit 87da3fe

Please sign in to comment.