Delete the NodeNetworkPolicy chains left behind in the datapath

syncIPTables only rewrites the chains the caches know about, so a chain
deleted while a sync was holding an older snapshot is recreated by that
sync, and nothing removes it afterwards: the rules of a deleted policy
stay in the datapath until the Agent restarts.

The periodic sync now lists the chains of the filter table and deletes
the ones which are not in the caches any more. The datapath is listed
before the caches are read, which is what makes it safe: the rules of a
chain are stored in the caches before the chain is written to the
datapath, so a chain which appears in the listing was already in the
caches when the listing was taken, and is not mistaken for a leftover.

Signed-off-by: Hongliang Liu <hongliang.liu@broadcom.com>

Author: hongliangl

pkg/agent/route/route_linux.go

@@ -61,6 +61,10 @@ const (
 	vxlanPort  = 4789
 	genevePort = 6081
 
+	// nodeNetworkPolicyChainPrefix is the prefix of the chains created for NodeNetworkPolicy, including
+	// the two chains holding the rules which jump to the per-policy ones.
+	nodeNetworkPolicyChainPrefix = "ANTREA-POL"
+
 	// syncDebounceDuration is how long a notification waits in the queue, so that the updates occurring
 	// in quick succession are coalesced into a single sync.
 	syncDebounceDuration = 100 * time.Millisecond
@@ -511,6 +515,7 @@ func (c *Client) syncNetworkConfig(ctx context.Context) {
 		return
 	}
 	c.triggerIPTablesSync()
+	c.cleanupOrphanNodeNetworkPolicyChains()
 	if c.nftables != nil {
 		if err := c.syncNFTables(ctx); err != nil {
 			klog.ErrorS(err, "Failed to sync nftables")
@@ -1031,6 +1036,45 @@ func (c *Client) removeUnexpectedAntreaJumpRule(protocol iptables.Protocol, jump
 
 // syncIPTables ensure that the iptables infrastructure we use is set up.
 // It's idempotent and can safely be called on every startup.
+// cleanupOrphanNodeNetworkPolicyChains deletes the NodeNetworkPolicy chains which are still in the datapath but not
+// in the cache any more. syncIPTables only rewrites the chains the cache knows about, so a chain deleted while a sync
+// was holding an older snapshot is recreated by that sync and nothing removes it afterwards.
+//
+// The datapath is listed before the cache is read, which is what makes this safe: the rules of a chain are stored in
+// the cache before the chain is written to the datapath, so a chain which appears in the listing was already in the
+// cache when the listing was taken, and is not mistaken for an orphan.
+func (c *Client) cleanupOrphanNodeNetworkPolicyChains() {
+	if !c.nodeNetworkPolicyEnabled {
+		return
+	}
+	chainsInDatapath, err := c.iptables.ListChains(c.getIPProtocol(), iptables.FilterTable)
+	if err != nil {
+		klog.ErrorS(err, "Failed to list the chains of the filter table")
+		return
+	}
+	for ipProtocol, chains := range chainsInDatapath {
+		cache := c.iptablesCache.ipv4[featureNodeNetworkPolicy]
+		if iptables.IsIPv6Protocol(ipProtocol) {
+			cache = c.iptablesCache.ipv6[featureNodeNetworkPolicy]
+		}
+		for _, chain := range chains {
+			if !strings.HasPrefix(chain, nodeNetworkPolicyChainPrefix) {
+				continue
+			}
+			if _, exists := cache.Load(chain); exists {
+				continue
+			}
+			// The chain may still be referenced by a jump rule which the next sync will remove, in
+			// which case the deletion fails and is retried on the next period.
+			if err := c.iptables.DeleteChain(ipProtocol, iptables.FilterTable, chain); err != nil {
+				klog.V(2).InfoS("Failed to delete an orphan NodeNetworkPolicy chain, will retry", "chain", chain, "err", err)
+				continue
+			}
+			klog.InfoS("Deleted an orphan NodeNetworkPolicy chain", "chain", chain)
+		}
+	}
+}
+
 func (c *Client) syncIPTables(cleanupStaleJumpRules bool) error {
 	ipProtocol := c.getIPProtocol()
 	jumpRules := []jumpRule{

pkg/agent/route/route_linux_test.go

@@ -381,6 +381,54 @@ func TestSyncIPSet(t *testing.T) {
 	}
 }
 
+func TestCleanupOrphanNodeNetworkPolicyChains(t *testing.T) {
+	const orphanChain = "ANTREA-POL-RULE-ORPHAN"
+
+	tests := []struct {
+		name                     string
+		nodeNetworkPolicyEnabled bool
+		chainsInDatapath         []string
+		expectedDeletes          []string
+	}{
+		{
+			name:                     "delete the chains which are not in the cache any more",
+			nodeNetworkPolicyEnabled: true,
+			// The first two are seeded in the cache by initNodeNetworkPolicy, the last one is not
+			// managed by Antrea at all.
+			chainsInDatapath: []string{config.NodeNetworkPolicyIngressRulesChain, orphanChain, "KUBE-SERVICES"},
+			expectedDeletes:  []string{orphanChain},
+		},
+		{
+			// Antrea does not own these chains when the feature is disabled, so it must not touch them.
+			name:                     "do nothing when NodeNetworkPolicy is disabled",
+			nodeNetworkPolicyEnabled: false,
+			chainsInDatapath:         []string{orphanChain},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			mockIPTables := iptablestest.NewMockInterface(ctrl)
+			c := &Client{
+				networkConfig:            &config.NetworkConfig{IPv4Enabled: true},
+				nodeNetworkPolicyEnabled: tt.nodeNetworkPolicyEnabled,
+				iptablesCache:            newIPTablesCache(),
+				iptables:                 mockIPTables,
+			}
+			if tt.nodeNetworkPolicyEnabled {
+				c.iptablesCache.ipv4[featureNodeNetworkPolicy].Store(config.NodeNetworkPolicyIngressRulesChain, []string{})
+				mockIPTables.EXPECT().ListChains(iptables.ProtocolIPv4, iptables.FilterTable).Return(
+					map[iptables.Protocol][]string{iptables.ProtocolIPv4: tt.chainsInDatapath}, nil)
+			}
+			for _, chain := range tt.expectedDeletes {
+				mockIPTables.EXPECT().DeleteChain(iptables.ProtocolIPv4, iptables.FilterTable, chain).Return(nil)
+			}
+
+			c.cleanupOrphanNodeNetworkPolicyChains()
+		})
+	}
+}
+
 func TestSyncIPTables(t *testing.T) {
 	mockIPTablesListRulesOfChains := func(mockIPTables *iptablestest.MockInterfaceMockRecorder,
 		protocol iptables.Protocol,

pkg/agent/util/iptables/iptables.go

@@ -115,6 +115,8 @@ type Interface interface {
 
 	ListRules(protocol Protocol, table string, chain string) (map[Protocol][]string, error)
 
+	ListChains(protocol Protocol, table string) (map[Protocol][]string, error)
+
 	Restore(data string, flush bool, useIPv6 bool) error
 
 	Save() ([]byte, error)
@@ -401,6 +403,23 @@ func (c *Client) Restore(data string, flush bool, useIPv6 bool) error {
 }
 
 // Save calls iptables-saves to dump chains and tables in iptables.
+// ListChains lists the names of the chains of the given table, for every matching protocol.
+func (c *Client) ListChains(protocol Protocol, table string) (map[Protocol][]string, error) {
+	allChains := make(map[Protocol][]string)
+	for p := range c.ipts {
+		ipt := c.ipts[p]
+		if !matchProtocol(ipt, protocol) {
+			continue
+		}
+		chains, err := ipt.ListChains(table)
+		if err != nil {
+			return nil, fmt.Errorf("error listing chains of table %s: %w", table, err)
+		}
+		allChains[p] = chains
+	}
+	return allChains, nil
+}
+
 func (c *Client) Save() ([]byte, error) {
 	var output []byte
 	for p := range c.ipts {