Repository: cloudnativelabs/kube-router (go)

Issue:
Bug in network policy ipsets when using dualStack
**What happened?**
A clear and concise description of what the bug is.

We are using two ipsetHandlers, one for each ip protocol:
* nsc.ipSetHandlers[v1.IPv4Protocol]
* nsc.ipSetHandlers[v1.IPv6Protocol]

These two ipSetHandlers are tracking ipsets of both ip families. I don't know if this is made by design or perhaps this is the bug. Anyway, as a consequence, when restoring the ipsets [here](https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/netpol/policy.go#L151), we are writing the same ipsets twice and the data that stays is the one created by `ipSetHandlers[v1.IPv6Protocol]` because it is the one created the last.

This logic should be functionally fine if both ipSetHandlers carried the same information, however, they are not. When setting verbosity level to 3 in order to see this log https://github.com/cloudnativelabs/kube-router/blob/master/pkg/utils/ipset.go#L569, I can observe that `ipSetHandlers[v1.IPv6Protocol]` uses wrong ipv4 addresses for the ipv4 ipsets

**What did you expect to happen?**
A clear and concise description of what you expected to happen.

ipsets being correct always

**How can we reproduce the behavior you experienced?**
Steps to reproduce the behavior:
**1. Step 1** Deploy client, server, service and policy yamls from here: https://github.com/jfmontanaro/k3s-netpol-issue-demo
**2. Step 2** Check this works: kubectl exec -it pod/client -- wget -O - http://whoami.default.svc.cluster.local/
**3. Step 3**  Check ipsets and you will see both ipv4 and ipv6 ipsets with the correct server's IPs
**4. Step 4**  Remove server
**5. Step 5** Deploy server again
**6. Step 6** `kubectl exec -it pod/client -- wget -O - http://whoami.default.svc.cluster.local` does not work
**7. Step 7**  Check ipsets and you will see that ipv6 ipset has the correct IP but ipv4 ipset has either no IP or has the old IP

**Screenshots / Architecture Diagrams / Network Topologies **
If applicable, add those here to help explain your problem.

** System Information (please complete the following information):**
 - Kube-Router Version (`kube-router --version`): [e.g. 1.0.1] 2.1.1
 - Kube-Router Parameters: [e.g. --run-router --run-service-proxy --enable-overlay --overlay-type=full etc.]
 - Kubernetes Version (`kubectl version`) : [e.g. 1.18.3] 1.29.3
 - Cloud Type: [e.g. AWS, GCP, Azure, on premise] on premise
 - Kubernetes Deployment Type: [e.g. EKS, GKE, Kops, Kubeadm, etc.] Via k3s
 - Kube-Router Deployment Type: [e.g. DaemonSet, System Service]
 - Cluster Size: [e.g. 200 Nodes]

** Logs, other output, metrics **
Please provide logs, other kind of output or observed metrics here.
```
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: I0509 10:27:38.562250   11491 ipset.go:568] ipset restore looks like: create TMP-MOR3H7HU5JDLK6FI hash:ip family inet hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: add TMP-MOR3H7HU5JDLK6FI 10.42.1.7 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create KUBE-DST-2FAIIK2E4RIPMTGF hash:ip family inet hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: swap TMP-MOR3H7HU5JDLK6FI KUBE-DST-2FAIIK2E4RIPMTGF
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create TMP-NWERIXQHTHQDEF6A hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-NWERIXQHTHQDEF6A
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create inet6:KUBE-DST-I3PRO5XXEERITJZO hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: swap TMP-NWERIXQHTHQDEF6A inet6:KUBE-DST-I3PRO5XXEERITJZO
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-NWERIXQHTHQDEF6A
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: destroy TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: destroy TMP-NWERIXQHTHQDEF6A
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: I0509 10:27:38.595720   11491 policy.go:183] Restoring IPv4 ipset took 33.491478ms
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: I0509 10:27:38.595803   11491 ipset.go:568] MANU ipset restore looks like: create TMP-MOR3H7HU5JDLK6FI hash:ip family inet hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: add TMP-MOR3H7HU5JDLK6FI 10.42.1.6 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create KUBE-DST-2FAIIK2E4RIPMTGF hash:ip family inet hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: swap TMP-MOR3H7HU5JDLK6FI KUBE-DST-2FAIIK2E4RIPMTGF
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create TMP-NWERIXQHTHQDEF6A hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-NWERIXQHTHQDEF6A
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: add TMP-NWERIXQHTHQDEF6A 2001:cafe:42:1::7 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: create inet6:KUBE-DST-I3PRO5XXEERITJZO hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: swap TMP-NWERIXQHTHQDEF6A inet6:KUBE-DST-I3PRO5XXEERITJZO
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: flush TMP-NWERIXQHTHQDEF6A
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: destroy TMP-MOR3H7HU5JDLK6FI
May 09 10:27:38 terraform-mbuil-vm0 k3s[11491]: destroy TMP-NWERIXQHTHQDEF6A
```
As you can observe, the ipv6 handler is using an old ipv4 `10.42.1.6` for ipset  `KUBE-DST-2FAIIK2E4RIPMTGF`

**Additional context**
Add any other context about the problem here.



Pull request description:
Fix ipset Duplication Across Handlers
@mrueg / @manuelbuil

Attempts to separate IP families within the different ipset handlers. This has likely been an issue since the first dual-stack implementation was contributed, but hasn't been discovered until now. The problem was that ipset was being utilized the same way that iptables was where different handlers implicitly meant different families. However, the ipset command doesn't differentiate that way, it has one command that always contains both sets. This was likely missed during the port to dual stack that was contributed ~2 years ago.

Fixes #1665 

Patch:
diff --git a/pkg/controllers/netpol/network_policy_controller.go b/pkg/controllers/netpol/network_policy_controller.go
index 92f960f4..e4cc19f8 100644
--- a/pkg/controllers/netpol/network_policy_controller.go
+++ b/pkg/controllers/netpol/network_policy_controller.go
@@ -226,6 +226,22 @@ func (npc *NetworkPolicyController) fullPolicySync() {
 	npc.mu.Lock()
 	defer npc.mu.Unlock()
 
+	for ipFamily := range npc.ipSetHandlers {
+		// Ensure that we start with clean handlers that don't contain previous save data
+		var err error
+		//nolint:exhaustive // we don't need a default condition here because we control this ourselves
+		switch ipFamily {
+		case v1core.IPv4Protocol:
+			npc.ipSetHandlers[ipFamily], err = utils.NewIPSet(false)
+		case v1core.IPv6Protocol:
+			npc.ipSetHandlers[ipFamily], err = utils.NewIPSet(true)
+		}
+		if err != nil {
+			klog.Errorf("failed to create ipset handler: %v", err)
+			return
+		}
+	}
+
 	healthcheck.SendHeartBeat(npc.healthChan, "NPC")
 	start := time.Now()
 	syncVersion := strconv.FormatInt(start.UnixNano(), syncVersionBase)
@@ -684,28 +700,16 @@ func (npc *NetworkPolicyController) cleanupStaleIPSets(activePolicyIPSets map[st
 		}()
 	}
 
-	for ipFamily, ipsets := range npc.ipSetHandlers {
+	for _, ipsets := range npc.ipSetHandlers {
 		cleanupPolicyIPSets := make([]*utils.Set, 0)
-
 		if err := ipsets.Save(); err != nil {
 			klog.Fatalf("failed to initialize ipsets command executor due to %s", err.Error())
 		}
-		if ipFamily == v1core.IPv6Protocol {
-			for _, set := range ipsets.Sets() {
-				if strings.HasPrefix(set.Name, fmt.Sprintf("%s:%s", utils.FamillyInet6, kubeSourceIPSetPrefix)) ||
-					strings.HasPrefix(set.Name, fmt.Sprintf("%s:%s", utils.FamillyInet6, kubeDestinationIPSetPrefix)) {
-					if _, ok := activePolicyIPSets[set.Name]; !ok {
-						cleanupPolicyIPSets = append(cleanupPolicyIPSets, set)
-					}
-				}
-			}
-		} else {
-			for _, set := range ipsets.Sets() {
-				if strings.HasPrefix(set.Name, kubeSourceIPSetPrefix) ||
-					strings.HasPrefix(set.Name, kubeDestinationIPSetPrefix) {
-					if _, ok := activePolicyIPSets[set.Name]; !ok {
-						cleanupPolicyIPSets = append(cleanupPolicyIPSets, set)
-					}
+		for _, set := range ipsets.Sets() {
+			if set.HasPrefix(kubeSourceIPSetPrefix) ||
+				set.HasPrefix(kubeDestinationIPSetPrefix) {
+				if _, ok := activePolicyIPSets[set.Name]; !ok {
+					cleanupPolicyIPSets = append(cleanupPolicyIPSets, set)
 				}
 			}
 		}
diff --git a/pkg/utils/ipset.go b/pkg/utils/ipset.go
index 0b8ee33f..3329ed95 100644
--- a/pkg/utils/ipset.go
+++ b/pkg/utils/ipset.go
@@ -439,6 +439,11 @@ func (ipset *IPSet) Name(setName string) string {
 	return IPSetName(setName, ipset.isIpv6)
 }
 
+func (set *Set) HasPrefix(prefix string) bool {
+	fullPrefix := IPSetName(prefix, set.Parent.isIpv6)
+	return strings.HasPrefix(set.name(), fullPrefix)
+}
+
 func (set *Set) name() string {
 	return set.Parent.Name(set.Name)
 }
@@ -449,17 +454,38 @@ func (set *Set) name() string {
 // add KUBE-DST-3YNVZWWGX3UQQ4VQ 100.96.1.6 timeout 0
 func parseIPSetSave(ipset *IPSet, result string) map[string]*Set {
 	sets := make(map[string]*Set)
+
+	// Checks that the parent IPSet family aligns with the ipset that we're saving. This makes ipset.Save() (which is
+	// IP family agnostic) behave a bit more like iptables.Save() which is IP family sensitive since different commands
+	// give access to different IP families.
+	createRelevant := func(pIPSet *IPSet, ipsetName string) bool {
+		if pIPSet.isIpv6 {
+			return strings.HasPrefix(ipsetName, IPv6SetPrefix)
+		}
+		return !strings.HasPrefix(ipsetName, IPv6SetPrefix)
+	}
+
 	// Save is always in order
 	lines := strings.Split(result, "\n")
+	skipTilNextCreate := false
 	for _, line := range lines {
 		content := strings.Split(line, " ")
-		if content[0] == "create" {
+		switch stmtType := content[0]; stmtType {
+		case "create":
+			if !createRelevant(ipset, content[1]) {
+				skipTilNextCreate = true
+				continue
+			}
+			skipTilNextCreate = false
 			sets[content[1]] = &Set{
 				Parent:  ipset,
 				Name:    content[1],
 				Options: scrubInitValFromOptions(content[2:]),
 			}
-		} else if content[0] == "add" {
+		case "add":
+			if skipTilNextCreate {
+				continue
+			}
 			set := sets[content[1]]
 			set.Entries = append(set.Entries, &Entry{
 				Set:     set,
@@ -566,7 +592,7 @@ func (ipset *IPSet) Save() error {
 // Send formatted ipset.sets into stdin of "ipset restore" command.
 func (ipset *IPSet) Restore() error {
 	restoreString := buildIPSetRestore(ipset)
-	klog.V(3).Infof("ipset restore looks like: %s", restoreString)
+	klog.V(3).Infof("ipset (ipv6? %t) restore looks like:\n%s", ipset.isIpv6, restoreString)
 	stdin := bytes.NewBufferString(restoreString)
 	err := ipset.runWithStdin(stdin, "restore", "-exist")
 	if err != nil {


Test patch:
diff --git a/pkg/utils/ipset_test.go b/pkg/utils/ipset_test.go
index f9b6fddd..454f53d8 100644
--- a/pkg/utils/ipset_test.go
+++ b/pkg/utils/ipset_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_buildIPSetRestore(t *testing.T) {
@@ -101,3 +102,71 @@ func Test_scrubInitValFromOptions(t *testing.T) {
 		assert.Equal(t, desired, scrubInitValFromOptions(noInitVal))
 	})
 }
+
+func Test_parseIPSetSave(t *testing.T) {
+	ipsetSaveText := `
+create inet6:kube-router-local-ips hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0 bucketsize 12 initval 0x6b804011
+create inet6:kube-router-svip hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0 bucketsize 12 initval 0x1f794545
+add inet6:kube-router-svip 2001:db8:42:1::423b timeout 0
+add inet6:kube-router-svip 2001:db8:42:1200:: timeout 0
+create inet6:kube-router-svip-prt hash:ip,port family inet6 hashsize 1024 maxelem 65536 timeout 0 bucketsize 12 initval 0xdf8a8ddd
+add inet6:kube-router-svip-prt 2001:db8:42:1200::,tcp:5000 timeout 0
+add inet6:kube-router-svip-prt 2001:db8:42:1::423b,tcp:5000 timeout 0
+create KUBE-DST-2FAIIK2E4RIPMTGF hash:ip family inet hashsize 1024 maxelem 65536 timeout 0 bucketsize 12 initval 0x331717b4
+add KUBE-DST-2FAIIK2E4RIPMTGF 10.242.1.11 timeout 0
+create inet6:KUBE-DST-I3PRO5XXEERITJZO hash:ip family inet6 hashsize 1024 maxelem 65536 timeout 0 bucketsize 12 initval 0xb7e095a4
+add inet6:KUBE-DST-I3PRO5XXEERITJZO 2001:db8:42:1001::b timeout 0
+`
+	tests := []struct {
+		name  string
+		ipset *IPSet
+		have  string
+		want  map[string][]string
+	}{
+		{
+			name:  "Ensure IPv4 parent only contains IPv4 sets",
+			ipset: &IPSet{isIpv6: false},
+			have:  ipsetSaveText,
+			want: map[string][]string{
+				"KUBE-DST-2FAIIK2E4RIPMTGF": {
+					"10.242.1.11",
+				},
+			},
+		},
+		{
+			name:  "Ensure IPv6 parent only contains IPv6 sets",
+			ipset: &IPSet{isIpv6: true},
+			have:  ipsetSaveText,
+			want: map[string][]string{
+				"inet6:kube-router-local-ips": {},
+				"inet6:kube-router-svip": {
+					"2001:db8:42:1::423b",
+					"2001:db8:42:1200::",
+				},
+				"inet6:kube-router-svip-prt": {
+					"2001:db8:42:1200::,tcp:5000",
+					"2001:db8:42:1::423b,tcp:5000",
+				},
+				"inet6:KUBE-DST-I3PRO5XXEERITJZO": {
+					"2001:db8:42:1001::b",
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			results := parseIPSetSave(tt.ipset, tt.have)
+			require.Len(t, results, len(tt.want), "want and have should be the same length")
+			idx := 0
+			for ipsetName, ipsetEntries := range tt.want {
+				require.Contains(t, results, ipsetName)
+				resEntries := results[ipsetName]
+				for idx2, entry := range ipsetEntries {
+					assert.Equal(t, entry, resEntries.Entries[idx2].Options[0])
+				}
+				idx++
+			}
+		})
+	}
+}