Skip to content

Commit

Permalink
Fix that when destroying ipset when updating NodeNetworkPolicy
Browse files Browse the repository at this point in the history 
Signed-off-by: Hongliang Liu <hongliang.liu@broadcom.com>
  • Loading branch information
@hongliangl
hongliangl committed Sep 30, 2024
1 parent a60f535 commit 8f58f0f
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 40 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
53 changes: 31 additions & 22 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,37 +578,38 @@ 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)
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)
},
Expand Down
18 changes: 9 additions & 9 deletions pkg/agent/util/ipset/ipset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ func NewClient() *Client {

func (c *Client) DestroyIPSet(name string) error {
cmd := exec.Command("ipset", "destroy", name)
if err := cmd.Run(); err != nil {
if output, err := cmd.CombinedOutput(); err != nil {
if strings.Contains(err.Error(), "The set with the given name does not exist") {
return nil
}
return fmt.Errorf("error destroying ipset %s: %v", name, err)
return fmt.Errorf("error destroying ipset %s, err: %w, output: %s", name, err, output)
}
return nil
}
Expand All @@ -75,26 +75,26 @@ func (c *Client) CreateIPSet(name string, setType SetType, isIPv6 bool) error {
// #nosec G204 -- inputs are not controlled by users
cmd = exec.Command("ipset", "create", name, string(setType), "-exist")
}
if err := cmd.Run(); err != nil {
return fmt.Errorf("error creating ipset %s: %v", name, err)
if output, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("error creating ipset %s, err: %w, output: %s", name, err, output)
}
return nil
}

// AddEntry adds a new entry to the set, it will ignore error when the entry already exists.
func (c *Client) AddEntry(name string, entry string) error {
cmd := exec.Command("ipset", "add", name, entry, "-exist")
if err := cmd.Run(); err != nil {
return fmt.Errorf("error adding entry %s to ipset %s: %v", entry, name, err)
if output, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("error adding entry %s to ipset %s, err: %w, output: %s", entry, name, err, output)
}
return nil
}

// DelEntry deletes the entry from the set, it will ignore error when the entry doesn't exist.
func (c *Client) DelEntry(name string, entry string) error {
cmd := exec.Command("ipset", "del", name, entry, "-exist")
if err := cmd.Run(); err != nil {
return fmt.Errorf("error deleting entry %s from ipset %s: %v", entry, name, err)
if output, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("error deleting entry %s from ipset %s, err: %w, output: %s", entry, name, err, output)
}
return nil
}
Expand All @@ -104,7 +104,7 @@ func (c *Client) ListEntries(name string) ([]string, error) {
cmd := exec.Command("ipset", "list", name)
output, err := cmd.CombinedOutput()
if err != nil {
return nil, fmt.Errorf("error listing ipset %s: %v", name, err)
return nil, fmt.Errorf("error listing ipset %s, err: %w, output: %s", name, err, output)
}
memberStr := memberPattern.ReplaceAllString(string(output), "")
lines := strings.Split(memberStr, "\n")
Expand Down

0 comments on commit 8f58f0f

Please sign in to comment.