Repository: antrea-io/antrea
Pull request #2469 (2021-07-26 09:09:35)

[IPv6] Bug fix in adding iptable rules
* bug fix in EnsureRule()
* refactor iptables methods

Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

lzhecheng: /test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e
codecov-commenter: # [Codecov](https://codecov.io/gh/antrea-io/antrea/pull/2469?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#2469](https://codecov.io/gh/antrea-io/antrea/pull/2469?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (cac8084) into [main](https://codecov.io/gh/antrea-io/antrea/commit/cdc84533515b585b8835bb7135f5cfb5d94be916?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (cdc8453) will **decrease** coverage by `17.83%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/antrea-io/antrea/pull/2469/graphs/tree.svg?width=650&height=150&src=pr&token=TjWaLL2GoC&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/antrea-io/antrea/pull/2469?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

```diff
@@             Coverage Diff             @@
##             main    #2469       +/-   ##
===========================================
- Coverage   59.82%   41.99%   -17.84%     
===========================================
  Files         284      146      -138     
  Lines       22168    18118     -4050     
===========================================
- Hits        13263     7609     -5654     
- Misses       7483     9824     +2341     
+ Partials     1422      685      -737     
```

| Flag | Coverage Δ | |
|---|---|---|
| kind-e2e-tests | `?` | |
| unit-tests | `41.99% <0.00%> (-0.03%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/antrea-io/antrea/pull/2469?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [pkg/agent/util/iptables/iptables.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L3V0aWwvaXB0YWJsZXMvaXB0YWJsZXMuZ28=) | `0.00% <0.00%> (-37.40%)` | :arrow_down: |
| [pkg/agent/agent\_linux.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L2FnZW50X2xpbnV4Lmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/ovs/openflow/default.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL292cy9vcGVuZmxvdy9kZWZhdWx0Lmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/agent/config/node\_config.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L2NvbmZpZy9ub2RlX2NvbmZpZy5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/agent/cniserver/pod\_configuration\_linux.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L2NuaXNlcnZlci9wb2RfY29uZmlndXJhdGlvbl9saW51eC5nbw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/apis/controlplane/v1beta1/register.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FwaXMvY29udHJvbHBsYW5lL3YxYmV0YTEvcmVnaXN0ZXIuZ28=) | `0.00% <0.00%> (-92.86%)` | :arrow_down: |
| [pkg/ovs/openflow/logs.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL292cy9vcGVuZmxvdy9sb2dzLmdv) | `9.52% <0.00%> (-90.48%)` | :arrow_down: |
| [pkg/apis/controlplane/register.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FwaXMvY29udHJvbHBsYW5lL3JlZ2lzdGVyLmdv) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [pkg/agent/nodeportlocal/k8s/annotations.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L25vZGVwb3J0bG9jYWwvazhzL2Fubm90YXRpb25zLmdv) | `0.00% <0.00%> (-83.88%)` | :arrow_down: |
| [pkg/agent/client.go](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL2FnZW50L2NsaWVudC5nbw==) | `0.00% <0.00%> (-77.42%)` | :arrow_down: |
| ... and [232 more](https://codecov.io/gh/antrea-io/antrea/pull/2469/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | |

lzhecheng: /test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e /test-ipv6-only-e2e
lzhecheng: @antoninbas @tnqn I refactored some iptables methods. PTAL.
lzhecheng: /test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e /test-ipv6-only-e2e


lzhecheng: /test-all /test-ipv6-all /test-ipv6-only-all
lzhecheng: /test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e /test-ipv6-only-e2e
antoninbas: @tnqn PTAL

@lzhecheng please cherry-pick this as needed (at least v1.2, but potentially all the way back to v0.13 if appropriate)
lzhecheng: /test-e2e /test-conformance /test-networkpolicy /test-ipv6-all /test-ipv6-only-all
lzhecheng: /test-all /test-ipv6-all /test-ipv6-only-all
lzhecheng: /test-all /test-ipv6-all /test-ipv6-only-all

@antoninbas

tnqn: /test-all

Patch:
diff --git a/pkg/agent/nodeportlocal/portcache/port_table.go b/pkg/agent/nodeportlocal/portcache/port_table.go
index 644af03ce..19a776913 100644
--- a/pkg/agent/nodeportlocal/portcache/port_table.go
+++ b/pkg/agent/nodeportlocal/portcache/port_table.go
@@ -119,7 +119,7 @@ func (pt *PortTable) AddRule(podIP string, podPort int) (int, error) {
 	if err != nil {
 		return 0, err
 	}
-	if err := pt.PodPortRules.AddRule(nodePort, fmt.Sprintf("%s:%d", podIP, podPort)); err != nil {
+	if err := pt.PodPortRules.AddRule(nodePort, podIP, podPort); err != nil {
 		if err := socket.Close(); err != nil {
 			klog.ErrorS(err, "Unexpected error when closing socket")
 		}
@@ -138,7 +138,7 @@ func (pt *PortTable) DeleteRule(podIP string, podPort int) error {
 	pt.tableLock.Lock()
 	defer pt.tableLock.Unlock()
 	data := pt.getEntryByPodIPPort(podIP, podPort)
-	if err := pt.PodPortRules.DeleteRule(data.NodePort, fmt.Sprintf("%s:%d", podIP, podPort)); err != nil {
+	if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort); err != nil {
 		return err
 	}
 	if err := data.socket.Close(); err != nil {
diff --git a/pkg/agent/nodeportlocal/rules/iptable_rule.go b/pkg/agent/nodeportlocal/rules/iptable_rule.go
index 7cdb0cf9e..af4bb91e9 100644
--- a/pkg/agent/nodeportlocal/rules/iptable_rule.go
+++ b/pkg/agent/nodeportlocal/rules/iptable_rule.go
@@ -56,16 +56,16 @@ func (ipt *iptablesRules) Init() error {
 // traffic) and OUTPUT chain (for locally-generated traffic). All NPL DNAT rules
 // will be added to this chain.
 func (ipt *iptablesRules) initRules() error {
-	if err := ipt.table.EnsureChain(iptables.NATTable, NodePortLocalChain); err != nil {
+	if err := ipt.table.EnsureChain(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain); err != nil {
 		return err
 	}
 	ruleSpec := []string{
 		"-p", "tcp", "-m", "addrtype", "--dst-type", "LOCAL", "-j", NodePortLocalChain,
 	}
-	if err := ipt.table.EnsureRule(iptables.NATTable, iptables.PreRoutingChain, ruleSpec); err != nil {
+	if err := ipt.table.AppendRule(iptables.ProtocolIPv4, iptables.NATTable, iptables.PreRoutingChain, ruleSpec); err != nil {
 		return err
 	}
-	if err := ipt.table.EnsureRule(iptables.NATTable, iptables.OutputChain, ruleSpec); err != nil {
+	if err := ipt.table.AppendRule(iptables.ProtocolIPv4, iptables.NATTable, iptables.OutputChain, ruleSpec); err != nil {
 		return err
 	}
 	return nil
@@ -78,13 +78,14 @@ func buildRuleForPod(port int, podIP string) []string {
 	}
 }
 
-// AddRule appends a DNAT rule in NodePortLocalChain chain of NAT table
-func (ipt *iptablesRules) AddRule(port int, podIP string) error {
-	rule := buildRuleForPod(port, podIP)
-	if err := ipt.table.EnsureRule(iptables.NATTable, NodePortLocalChain, rule); err != nil {
+// AddRule appends a DNAT rule in NodePortLocalChain chain of NAT table.
+func (ipt *iptablesRules) AddRule(nodePort int, podIP string, podPort int) error {
+	podAddr := fmt.Sprintf("%s:%d", podIP, podPort)
+	rule := buildRuleForPod(nodePort, podAddr)
+	if err := ipt.table.AppendRule(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain, rule); err != nil {
 		return err
 	}
-	klog.Infof("Successfully added DNAT rule for Pod IP %s, port %d", podIP, port)
+	klog.InfoS("Successfully added DNAT rule", "podAddr", podAddr, "nodePort", nodePort)
 	return nil
 }
 
@@ -107,18 +108,19 @@ func (ipt *iptablesRules) AddAllRules(nplList []PodNodePort) error {
 }
 
 // DeleteRule deletes a specific NPL rule from NodePortLocalChain chain
-func (ipt *iptablesRules) DeleteRule(port int, podIP string) error {
-	klog.Infof("Deleting DNAT rule for Pod IP %s, port %d", podIP, port)
-	rule := buildRuleForPod(port, podIP)
-	if err := ipt.table.DeleteRule(iptables.NATTable, NodePortLocalChain, rule); err != nil {
+func (ipt *iptablesRules) DeleteRule(nodePort int, podIP string, podPort int) error {
+	podAddr := fmt.Sprintf("%s:%d", podIP, podPort)
+	rule := buildRuleForPod(nodePort, podAddr)
+	if err := ipt.table.DeleteRule(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain, rule); err != nil {
 		return err
 	}
+	klog.InfoS("Successfully deleted DNAT rule", "podAddr", podAddr, "nodePort", nodePort)
 	return nil
 }
 
 // DeleteAllRules deletes all NPL rules programmed in the node
 func (ipt *iptablesRules) DeleteAllRules() error {
-	exists, err := ipt.table.ChainExists(iptables.NATTable, NodePortLocalChain)
+	exists, err := ipt.table.ChainExists(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain)
 	if err != nil {
 		return fmt.Errorf("failed to check if NodePortLocal chain exists in NAT table: %v", err)
 	}
@@ -128,13 +130,13 @@ func (ipt *iptablesRules) DeleteAllRules() error {
 	ruleSpec := []string{
 		"-p", "tcp", "-m", "addrtype", "--dst-type", "LOCAL", "-j", NodePortLocalChain,
 	}
-	if err := ipt.table.DeleteRule(iptables.NATTable, iptables.PreRoutingChain, ruleSpec); err != nil {
+	if err := ipt.table.DeleteRule(iptables.ProtocolIPv4, iptables.NATTable, iptables.PreRoutingChain, ruleSpec); err != nil {
 		return err
 	}
-	if err := ipt.table.DeleteRule(iptables.NATTable, iptables.OutputChain, ruleSpec); err != nil {
+	if err := ipt.table.DeleteRule(iptables.ProtocolIPv4, iptables.NATTable, iptables.OutputChain, ruleSpec); err != nil {
 		return err
 	}
-	if err := ipt.table.DeleteChain(iptables.NATTable, NodePortLocalChain); err != nil {
+	if err := ipt.table.DeleteChain(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain); err != nil {
 		return err
 	}
 	return nil
diff --git a/pkg/agent/nodeportlocal/rules/rules.go b/pkg/agent/nodeportlocal/rules/rules.go
index a4a70b6d8..7c9bca1ab 100644
--- a/pkg/agent/nodeportlocal/rules/rules.go
+++ b/pkg/agent/nodeportlocal/rules/rules.go
@@ -19,8 +19,8 @@ package rules
 // PodPortRules is an interface to abstract operations on rules for Pods
 type PodPortRules interface {
 	Init() error
-	AddRule(port int, podip string) error
-	DeleteRule(port int, podip string) error
+	AddRule(nodePort int, podIP string, podPort int) error
+	DeleteRule(nodePort int, podIP string, podPort int) error
 	DeleteAllRules() error
 	AddAllRules(nplList []PodNodePort) error
 }
diff --git a/pkg/agent/route/route_linux.go b/pkg/agent/route/route_linux.go
index 06cfca5b3..68aae961d 100644
--- a/pkg/agent/route/route_linux.go
+++ b/pkg/agent/route/route_linux.go
@@ -258,6 +258,7 @@ func (c *Client) syncIPTables() error {
 	var err error
 	v4Enabled := config.IsIPv4Enabled(c.nodeConfig, c.networkConfig.TrafficEncapMode)
 	v6Enabled := config.IsIPv6Enabled(c.nodeConfig, c.networkConfig.TrafficEncapMode)
+
 	c.ipt, err = iptables.New(v4Enabled, v6Enabled)
 	if err != nil {
 		return fmt.Errorf("error creating IPTables instance: %v", err)
@@ -274,11 +275,11 @@ func (c *Client) syncIPTables() error {
 		{iptables.MangleTable, iptables.OutputChain, antreaOutputChain, "Antrea: jump to Antrea output rules"},
 	}
 	for _, rule := range jumpRules {
-		if err := c.ipt.EnsureChain(rule.table, rule.dstChain); err != nil {
+		if err := c.ipt.EnsureChain(iptables.ProtocolDual, rule.table, rule.dstChain); err != nil {
 			return err
 		}
 		ruleSpec := []string{"-j", rule.dstChain, "-m", "comment", "--comment", rule.comment}
-		if err := c.ipt.EnsureRule(rule.table, rule.srcChain, ruleSpec); err != nil {
+		if err := c.ipt.AppendRule(iptables.ProtocolDual, rule.table, rule.srcChain, ruleSpec); err != nil {
 			return err
 		}
 	}
@@ -771,5 +772,5 @@ func (c *Client) DeleteSNATRule(mark uint32) error {
 	}
 	c.markToSNATIP.Delete(mark)
 	snatIP := value.(net.IP)
-	return c.ipt.DeleteRule(iptables.NATTable, antreaPostRoutingChain, c.snatRuleSpec(snatIP, mark))
+	return c.ipt.DeleteRule(iptables.ProtocolDual, iptables.NATTable, antreaPostRoutingChain, c.snatRuleSpec(snatIP, mark))
 }
diff --git a/pkg/agent/util/iptables/iptables.go b/pkg/agent/util/iptables/iptables.go
index 2c6a3fb74..dfc14a090 100644
--- a/pkg/agent/util/iptables/iptables.go
+++ b/pkg/agent/util/iptables/iptables.go
@@ -52,6 +52,15 @@ const (
 
 type Protocol byte
 
+var protocolStrMap = map[Protocol]string{
+	ProtocolIPv4: "IPv4",
+	ProtocolIPv6: "IPv6",
+}
+
+func (p Protocol) String() string {
+	return protocolStrMap[p]
+}
+
 const (
 	ProtocolDual Protocol = iota
 	ProtocolIPv4
@@ -63,20 +72,20 @@ const (
 var restoreWaitSupportedMinVersion = semver.Version{Major: 1, Minor: 6, Patch: 2}
 
 type Client struct {
-	ipts []*iptables.IPTables
+	ipts map[Protocol]*iptables.IPTables
 	// restoreWaitSupported indicates whether iptables-restore (or ip6tables-restore) supports --wait flag.
 	restoreWaitSupported bool
 }
 
 func New(enableIPV4, enableIPV6 bool) (*Client, error) {
-	var ipts []*iptables.IPTables
+	ipts := make(map[Protocol]*iptables.IPTables)
 	var restoreWaitSupported bool
 	if enableIPV4 {
 		ipt, err := iptables.New()
 		if err != nil {
 			return nil, fmt.Errorf("error creating IPTables instance: %v", err)
 		}
-		ipts = append(ipts, ipt)
+		ipts[ProtocolIPv4] = ipt
 		restoreWaitSupported = isRestoreWaitSupported(ipt)
 	}
 	if enableIPV6 {
@@ -84,7 +93,7 @@ func New(enableIPV4, enableIPV6 bool) (*Client, error) {
 		if err != nil {
 			return nil, fmt.Errorf("error creating IPTables instance for IPv6: %v", err)
 		}
-		ipts = append(ipts, ip6t)
+		ipts[ProtocolIPv6] = ip6t
 		if !restoreWaitSupported {
 			restoreWaitSupported = isRestoreWaitSupported(ip6t)
 		}
@@ -99,9 +108,12 @@ func isRestoreWaitSupported(ipt *iptables.IPTables) bool {
 }
 
 // EnsureChain checks if target chain already exists, creates it if not.
-func (c *Client) EnsureChain(table string, chain string) error {
-	for idx := range c.ipts {
-		ipt := c.ipts[idx]
+func (c *Client) EnsureChain(protocol Protocol, table string, chain string) error {
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
 		exists, err := ipt.ChainExists(table, chain)
 		if err != nil {
 			return fmt.Errorf("error checking if chain %s exists in table %s: %v", chain, table, err)
@@ -112,48 +124,56 @@ func (c *Client) EnsureChain(table string, chain string) error {
 		if err := ipt.NewChain(table, chain); err != nil {
 			return fmt.Errorf("error creating chain %s in table %s: %v", chain, table, err)
 		}
-		klog.V(2).Infof("Created chain %s in table %s", chain, table)
+		klog.V(2).InfoS("Created a chain", "chain", chain, "table", table, "protocol", p)
 	}
 	return nil
 }
 
-// ChainExists checks if a chain already exists in a table
-func (c *Client) ChainExists(table string, chain string) (bool, error) {
-	for idx := range c.ipts {
-		exists, err := c.ipts[idx].ChainExists(table, chain)
+// ChainExists checks if target chain already exists in a table
+func (c *Client) ChainExists(protocol Protocol, table string, chain string) (bool, error) {
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
+		exists, err := ipt.ChainExists(table, chain)
 		if err != nil {
 			return false, fmt.Errorf("error checking if chain %s exists in table %s: %v", chain, table, err)
 		}
-		if exists {
-			return true, nil
+		if !exists {
+			return false, nil
 		}
+		klog.V(2).InfoS("A chain exists", "chain", chain, "table", table, "protocol", p)
 	}
-	return false, nil
+	return true, nil
 }
 
-// EnsureRule checks if target rule already exists, appends it if not.
-func (c *Client) EnsureRule(table string, chain string, ruleSpec []string) error {
-	for idx := range c.ipts {
-		ipt := c.ipts[idx]
+// AppendRule checks if target rule already exists with the protocol, appends it if not.
+func (c *Client) AppendRule(protocol Protocol, table string, chain string, ruleSpec []string) error {
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
 		exist, err := ipt.Exists(table, chain, ruleSpec...)
 		if err != nil {
 			return fmt.Errorf("error checking if rule %v exists in table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
 		if exist {
-			return nil
+			continue
 		}
 		if err := ipt.Append(table, chain, ruleSpec...); err != nil {
 			return fmt.Errorf("error appending rule %v to table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
+		klog.V(2).InfoS("Appended a rule", "rule", ruleSpec, "table", table, "chain", chain, "protocol", p)
 	}
-	klog.V(2).Infof("Appended rule %v to table %s chain %s", ruleSpec, table, chain)
 	return nil
 }
 
 // InsertRule checks if target rule already exists, inserts it if not.
 func (c *Client) InsertRule(protocol Protocol, table string, chain string, ruleSpec []string) error {
-	for idx := range c.ipts {
-		ipt := c.ipts[idx]
+	for p := range c.ipts {
+		ipt := c.ipts[p]
 		if !matchProtocol(ipt, protocol) {
 			continue
 		}
@@ -162,13 +182,13 @@ func (c *Client) InsertRule(protocol Protocol, table string, chain string, ruleS
 			return fmt.Errorf("error checking if rule %v exists in table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
 		if exist {
-			return nil
+			continue
 		}
 		if err := ipt.Insert(table, chain, 1, ruleSpec...); err != nil {
 			return fmt.Errorf("error inserting rule %v to table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
+		klog.V(2).InfoS("Inserted a rule", "rule", ruleSpec, "table", table, "chain", chain)
 	}
-	klog.V(2).Infof("Inserted rule %v to table %s chain %s", ruleSpec, table, chain)
 	return nil
 }
 
@@ -185,46 +205,59 @@ func matchProtocol(ipt *iptables.IPTables, protocol Protocol) bool {
 }
 
 // DeleteRule checks if target rule already exists, deletes the rule if found.
-func (c *Client) DeleteRule(table string, chain string, ruleSpec []string) error {
-	for idx := range c.ipts {
-		exist, err := c.ipts[idx].Exists(table, chain, ruleSpec...)
+func (c *Client) DeleteRule(protocol Protocol, table string, chain string, ruleSpec []string) error {
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
+		exist, err := ipt.Exists(table, chain, ruleSpec...)
 		if err != nil {
 			return fmt.Errorf("error checking if rule %v exists in table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
 		if !exist {
-			return nil
+			continue
 		}
-		if err := c.ipts[idx].Delete(table, chain, ruleSpec...); err != nil {
+		if err := ipt.Delete(table, chain, ruleSpec...); err != nil {
 			return fmt.Errorf("error deleting rule %v from table %s chain %s: %v", ruleSpec, table, chain, err)
 		}
-		klog.V(2).Infof("Deleted rule %v from table %s chain %s", ruleSpec, table, chain)
+		klog.V(2).InfoS("Deleted a rule", "rule", ruleSpec, "table", table, "chain", chain, "protocol", p)
 	}
 	return nil
 }
 
-// DeleteChain deletes all rules from a chain in a table and then delete the chain
-func (c *Client) DeleteChain(table string, chain string) error {
-	for idx := range c.ipts {
-		err := c.ipts[idx].ClearChain(table, chain)
+// DeleteChain deletes all rules from a chain in a table and then delete the chain.
+func (c *Client) DeleteChain(protocol Protocol, table string, chain string) error {
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
+		exists, err := ipt.ChainExists(table, chain)
 		if err != nil {
+			return fmt.Errorf("error checking if chain %s exists in table %s: %v", chain, table, err)
+		}
+		if !exists {
+			continue
+		}
+		if err = ipt.ClearChain(table, chain); err != nil {
 			return fmt.Errorf("error clearing rules from table %s chain %s: %v", table, chain, err)
 		}
-		err = c.ipts[idx].DeleteChain(table, chain)
-		if err != nil {
+		if err = ipt.DeleteChain(table, chain); err != nil {
 			return fmt.Errorf("error deleting chain %s from table %s: %v", chain, table, err)
 		}
-		klog.V(2).Infof("Deleted chain %s from table %s", chain, table)
+		klog.V(2).InfoS("Deleted a chain", "chain", chain, "table", table, "protocol", p)
 	}
 	return nil
 }
 
-// ListRules lists all rules from a chain in a table
+// ListRules lists all rules from a chain in a table.
 func (c *Client) ListRules(table string, chain string) ([]string, error) {
 	var allRules []string
-	for idx := range c.ipts {
-		rules, err := c.ipts[idx].List(table, chain)
+	for p := range c.ipts {
+		rules, err := c.ipts[p].List(table, chain)
 		if err != nil {
-			return rules, fmt.Errorf("error getting rules from table %s chain %s: %v", table, chain, err)
+			return rules, fmt.Errorf("error getting rules from table %s chain %s protocol %s: %v", table, chain, p, err)
 		}
 		allRules = append(allRules, rules...)
 	}
@@ -265,7 +298,7 @@ func (c *Client) Restore(data []byte, flush bool, useIPv6 bool) error {
 		defer unlockFunc()
 	}
 	if err := cmd.Run(); err != nil {
-		klog.Errorf("Failed to execute %s: %v\nstdin:\n%s\nstderr:\n%s", iptablesCmd, err, data, stderr)
+		klog.ErrorS(err, "Failed to execute iptables command", "iptablesCmd", iptablesCmd, "stdin", data, "stderr", stderr)
 		return fmt.Errorf("error executing %s: %v", iptablesCmd, err)
 	}
 	return nil
@@ -274,9 +307,9 @@ func (c *Client) Restore(data []byte, flush bool, useIPv6 bool) error {
 // Save calls iptables-saves to dump chains and tables in iptables.
 func (c *Client) Save() ([]byte, error) {
 	var output []byte
-	for idx := range c.ipts {
+	for p := range c.ipts {
 		var cmd string
-		ipt := c.ipts[idx]
+		ipt := c.ipts[p]
 		switch ipt.Proto() {
 		case iptables.ProtocolIPv6:
 			cmd = "ip6tables-save"


Test patch:
diff --git a/pkg/agent/nodeportlocal/npl_agent_test.go b/pkg/agent/nodeportlocal/npl_agent_test.go
index ea9aae035..40c8c7abf 100644
--- a/pkg/agent/nodeportlocal/npl_agent_test.go
+++ b/pkg/agent/nodeportlocal/npl_agent_test.go
@@ -191,8 +191,8 @@ func setUp(t *testing.T, tc *testConfig, objects ...runtime.Object) *testData {
 	mockCtrl := gomock.NewController(t)
 
 	mockIPTables := rulestesting.NewMockPodPortRules(mockCtrl)
-	mockIPTables.EXPECT().AddRule(gomock.Any(), gomock.Any()).AnyTimes()
-	mockIPTables.EXPECT().DeleteRule(gomock.Any(), gomock.Any()).AnyTimes()
+	mockIPTables.EXPECT().AddRule(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
+	mockIPTables.EXPECT().DeleteRule(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	mockIPTables.EXPECT().AddAllRules(gomock.Any()).AnyTimes()
 
 	mockPortOpener := portcachetesting.NewMockLocalPortOpener(mockCtrl)
diff --git a/pkg/agent/nodeportlocal/rules/testing/mock_rules.go b/pkg/agent/nodeportlocal/rules/testing/mock_rules.go
index b0ce5f010..da1d7a03e 100644
--- a/pkg/agent/nodeportlocal/rules/testing/mock_rules.go
+++ b/pkg/agent/nodeportlocal/rules/testing/mock_rules.go
@@ -63,17 +63,17 @@ func (mr *MockPodPortRulesMockRecorder) AddAllRules(arg0 interface{}) *gomock.Ca
 }
 
 // AddRule mocks base method
-func (m *MockPodPortRules) AddRule(arg0 int, arg1 string) error {
+func (m *MockPodPortRules) AddRule(arg0 int, arg1 string, arg2 int) error {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "AddRule", arg0, arg1)
+	ret := m.ctrl.Call(m, "AddRule", arg0, arg1, arg2)
 	ret0, _ := ret[0].(error)
 	return ret0
 }
 
 // AddRule indicates an expected call of AddRule
-func (mr *MockPodPortRulesMockRecorder) AddRule(arg0, arg1 interface{}) *gomock.Call {
+func (mr *MockPodPortRulesMockRecorder) AddRule(arg0, arg1, arg2 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddRule", reflect.TypeOf((*MockPodPortRules)(nil).AddRule), arg0, arg1)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddRule", reflect.TypeOf((*MockPodPortRules)(nil).AddRule), arg0, arg1, arg2)
 }
 
 // DeleteAllRules mocks base method
@@ -91,17 +91,17 @@ func (mr *MockPodPortRulesMockRecorder) DeleteAllRules() *gomock.Call {
 }
 
 // DeleteRule mocks base method
-func (m *MockPodPortRules) DeleteRule(arg0 int, arg1 string) error {
+func (m *MockPodPortRules) DeleteRule(arg0 int, arg1 string, arg2 int) error {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "DeleteRule", arg0, arg1)
+	ret := m.ctrl.Call(m, "DeleteRule", arg0, arg1, arg2)
 	ret0, _ := ret[0].(error)
 	return ret0
 }
 
 // DeleteRule indicates an expected call of DeleteRule
-func (mr *MockPodPortRulesMockRecorder) DeleteRule(arg0, arg1 interface{}) *gomock.Call {
+func (mr *MockPodPortRulesMockRecorder) DeleteRule(arg0, arg1, arg2 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteRule", reflect.TypeOf((*MockPodPortRules)(nil).DeleteRule), arg0, arg1)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteRule", reflect.TypeOf((*MockPodPortRules)(nil).DeleteRule), arg0, arg1, arg2)
 }
 
 // Init mocks base method