fix ACL update bug and add e2e test for it (#306)

cloud/linode/firewall/firewalls.go

@@ -69,6 +69,9 @@ func ipsChanged(ips *linodego.NetworkAddresses, rules []linodego.FirewallRule) b
 	}
 
 	if ips.IPv4 != nil {
+		if len(*ips.IPv4) != len(ruleIPv4s) {
+			return true
+		}
 		for _, ipv4 := range *ips.IPv4 {
 			if !slices.Contains(ruleIPv4s, ipv4) {
 				return true
@@ -77,6 +80,9 @@ func ipsChanged(ips *linodego.NetworkAddresses, rules []linodego.FirewallRule) b
 	}
 
 	if ips.IPv6 != nil {
+		if len(*ips.IPv6) != len(ruleIPv6s) {
+			return true
+		}
 		for _, ipv6 := range *ips.IPv6 {
 			if !slices.Contains(ruleIPv6s, ipv6) {
 				return true

cloud/linode/loadbalancers_test.go

@@ -393,10 +393,12 @@ func testCreateNodeBalanceWithBothAllowOrDenyList(t *testing.T, client *linodego
 	annotations := map[string]string{
 		annotations.AnnLinodeCloudFirewallACL: `{
 			"allowList": {
-				"ipv4": ["2.2.2.2"]
+				"ipv4": ["2.2.2.2/32"],
+				"ipv6": ["2001:db8::/128"]
 			},
 			"denyList": {
-				"ipv4": ["2.2.2.2"]
+				"ipv4": ["2.2.2.2/32"],
+				"ipv6": ["2001:db8::/128"]
 			}
 		}`,
 	}
@@ -411,29 +413,31 @@ func testCreateNodeBalancerWithAllowList(t *testing.T, client *linodego.Client,
 	annotations := map[string]string{
 		annotations.AnnLinodeCloudFirewallACL: `{
 			"allowList": {
-				"ipv4": ["2.2.2.2"]
+				"ipv4": ["2.2.2.2/32"],
+				"ipv6": ["2001:db8::/128"]
 			}
 		}`,
 	}
 
 	err := testCreateNodeBalancer(t, client, f, annotations, nil)
 	if err != nil {
-		t.Fatalf("expected a non-nil error, got %v", err)
+		t.Fatalf("expected a nil error, got %v", err)
 	}
 }
 
 func testCreateNodeBalancerWithDenyList(t *testing.T, client *linodego.Client, f *fakeAPI) {
 	annotations := map[string]string{
 		annotations.AnnLinodeCloudFirewallACL: `{
 			"denyList": {
-				"ipv4": ["2.2.2.2"]
+				"ipv4": ["2.2.2.2/32"],
+				"ipv6": ["2001:db8::/128"]
 			}
 		}`,
 	}
 
 	err := testCreateNodeBalancer(t, client, f, annotations, nil)
 	if err != nil {
-		t.Fatalf("expected a non-nil error, got %v", err)
+		t.Fatalf("expected a nil error, got %v", err)
 	}
 }
 
@@ -1680,7 +1684,7 @@ func testUpdateLoadBalancerUpdateFirewallACL(t *testing.T, client *linodego.Clie
 			Annotations: map[string]string{
 				annotations.AnnLinodeCloudFirewallACL: `{
 					"allowList": {
-						"ipv4": ["2.2.2.2"]
+						"ipv4": ["2.2.2.2/32", "3.3.3.3/32"]
 					}
 				}`,
 			},
@@ -1744,16 +1748,17 @@ func testUpdateLoadBalancerUpdateFirewallACL(t *testing.T, client *linodego.Clie
 
 	fwIPs := firewalls[0].Rules.Inbound[0].Addresses.IPv4
 	if fwIPs == nil {
-		t.Errorf("expected 2.2.2.2, got %v", fwIPs)
+		t.Errorf("expected ips, got %v", fwIPs)
 	}
 
 	fmt.Printf("got %v", fwIPs)
 
+	// Add ipv6 ips in allowList
 	svc.ObjectMeta.SetAnnotations(map[string]string{
 		annotations.AnnLinodeCloudFirewallACL: `{
 			"allowList": {
-				"ipv4": ["2.2.2.2"],
-				"ipv6": ["dead:beef::/128"]
+				"ipv4": ["2.2.2.2/32", "3.3.3.3/32"],
+				"ipv6": ["dead:beef::/128", "dead:bee::/128"]
 			}
 		}`,
 	})
@@ -1782,6 +1787,98 @@ func testUpdateLoadBalancerUpdateFirewallACL(t *testing.T, client *linodego.Clie
 		t.Errorf("expected non nil IPv4, got %v", fwIPs)
 	}
 
+	if len(*fwIPs) != 2 {
+		t.Errorf("expected two IPv4 ips, got %v", fwIPs)
+	}
+
+	if firewallsNew[0].Rules.Inbound[0].Addresses.IPv6 == nil {
+		t.Errorf("expected non nil IPv6, got %v", firewallsNew[0].Rules.Inbound[0].Addresses.IPv6)
+	}
+
+	if len(*firewallsNew[0].Rules.Inbound[0].Addresses.IPv6) != 2 {
+		t.Errorf("expected two IPv6 ips, got %v", firewallsNew[0].Rules.Inbound[0].Addresses.IPv6)
+	}
+
+	// Update ips in allowList
+	svc.ObjectMeta.SetAnnotations(map[string]string{
+		annotations.AnnLinodeCloudFirewallACL: `{
+			"allowList": {
+				"ipv4": ["2.2.2.1/32", "3.3.3.3/32"],
+				"ipv6": ["dead::/128", "dead:bee::/128"]
+			}
+		}`,
+	})
+
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("UpdateLoadBalancer returned an error: %s", err)
+	}
+
+	nbUpdated, err = lb.getNodeBalancerByStatus(context.TODO(), svc)
+	if err != nil {
+		t.Fatalf("failed to get NodeBalancer via status: %s", err)
+	}
+
+	firewallsNew, err = lb.client.ListNodeBalancerFirewalls(context.TODO(), nbUpdated.ID, &linodego.ListOptions{})
+	if err != nil {
+		t.Fatalf("failed to List Firewalls %s", err)
+	}
+
+	if len(firewallsNew) == 0 {
+		t.Fatalf("No attached firewalls found")
+	}
+
+	fwIPs = firewallsNew[0].Rules.Inbound[0].Addresses.IPv4
+	if fwIPs == nil {
+		t.Errorf("expected non nil IPv4, got %v", fwIPs)
+	}
+
+	if len(*fwIPs) != 2 {
+		t.Errorf("expected two IPv4 ips, got %v", fwIPs)
+	}
+
+	if firewallsNew[0].Rules.Inbound[0].Addresses.IPv6 == nil {
+		t.Errorf("expected non nil IPv6, got %v", firewallsNew[0].Rules.Inbound[0].Addresses.IPv6)
+	}
+
+	if len(*firewallsNew[0].Rules.Inbound[0].Addresses.IPv6) != 2 {
+		t.Errorf("expected two IPv6 ips, got %v", firewallsNew[0].Rules.Inbound[0].Addresses.IPv6)
+	}
+
+	// remove one ipv4 and one ipv6 ip from allowList
+	svc.ObjectMeta.SetAnnotations(map[string]string{
+		annotations.AnnLinodeCloudFirewallACL: `{
+			"allowList": {
+				"ipv4": ["3.3.3.3/32"],
+				"ipv6": ["dead:beef::/128"]
+			}
+		}`,
+	})
+
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("UpdateLoadBalancer returned an error: %s", err)
+	}
+
+	nbUpdated, err = lb.getNodeBalancerByStatus(context.TODO(), svc)
+	if err != nil {
+		t.Fatalf("failed to get NodeBalancer via status: %s", err)
+	}
+
+	firewallsNew, err = lb.client.ListNodeBalancerFirewalls(context.TODO(), nbUpdated.ID, &linodego.ListOptions{})
+	if err != nil {
+		t.Fatalf("failed to List Firewalls %s", err)
+	}
+
+	if len(firewallsNew) == 0 {
+		t.Fatalf("No attached firewalls found")
+	}
+
+	fwIPs = firewallsNew[0].Rules.Inbound[0].Addresses.IPv4
+	if fwIPs == nil {
+		t.Errorf("expected non nil IPv4, got %v", fwIPs)
+	}
+
 	if len(*fwIPs) != 1 {
 		t.Errorf("expected one IPv4, got %v", fwIPs)
 	}
@@ -1793,6 +1890,12 @@ func testUpdateLoadBalancerUpdateFirewallACL(t *testing.T, client *linodego.Clie
 	if len(*firewallsNew[0].Rules.Inbound[0].Addresses.IPv6) != 1 {
 		t.Errorf("expected one IPv6, got %v", firewallsNew[0].Rules.Inbound[0].Addresses.IPv6)
 	}
+
+	// Run update with same ACL
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("UpdateLoadBalancer returned an error: %s", err)
+	}
 }
 
 func testUpdateLoadBalancerUpdateFirewall(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {

e2e/test/lb-fw-update-acl/chainsaw-test.yaml

@@ -0,0 +1,93 @@
+# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: lb-fw-update-acl
+spec:
+  namespace: "lb-fw-update-acl"
+  steps:
+    - name: Check if CCM is deployed
+      try:
+        - assert:
+            file: ../assert-ccm-resources.yaml
+    - name: Create pods and services
+      try:
+        - apply:
+            file: create-pods-services.yaml
+      catch:
+        - describe:
+            apiVersion: v1
+            kind: Pod
+        - describe:
+            apiVersion: v1
+            kind: Service
+    - name: Check that loadbalancer ip is assigned
+      try:
+      - assert:
+          resource:
+            apiVersion: v1
+            kind: Service
+            metadata:
+              name: svc-test
+            status:
+              (loadBalancer.ingress[0].ip != null): true
+    - name: Fetch Nodebalancer ID, make sure it has firewall attached
+      try:
+        - script:
+            content: |
+              set -e
+
+              for i in {1..10}; do
+                  nbid=$(KUBECONFIG=$KUBECONFIG NAMESPACE=$NAMESPACE LINODE_TOKEN=$LINODE_TOKEN ../scripts/get-nb-id.sh)
+
+                  fw=$(curl -s --request GET \
+                    -H "Authorization: Bearer $LINODE_TOKEN" \
+                    -H "Content-Type: application/json" \
+                    -H "accept: application/json" \
+                    "https://api.linode.com/v4/nodebalancers/${nbid}/firewalls" || true)
+                  
+                  fwCount=$(echo $fw | jq '.data | length')
+                  ips=$(echo $fw | jq '.data[].rules.inbound[].addresses.ipv4[]')
+                  if [[ $fwCount -eq 1 && -n $ips && $ips == *"7.7.7.7/32"* ]]; then
+                      echo "firewall attached and rule has specified ip"
+                      break
+                  fi
+                  sleep 10
+              done
+            check:
+                ($error == null): true
+                (contains($stdout, 'firewall attached and rule has specified ip')): true
+    - name: Update service with new ACL
+      try:
+        - apply:
+            file: update-service.yaml
+      catch:
+        - describe:
+            apiVersion: v1
+            kind: Service
+    - name: Fetch firewall ID and check rules are updated
+      try:
+        - script:
+            content: |
+              set -e
+
+              for i in {1..10}; do
+                  nbid=$(KUBECONFIG=$KUBECONFIG NAMESPACE=$NAMESPACE LINODE_TOKEN=$LINODE_TOKEN ../scripts/get-nb-id.sh)
+
+                  fw=$(curl -s --request GET \
+                    -H "Authorization: Bearer $LINODE_TOKEN" \
+                    -H "Content-Type: application/json" \
+                    -H "accept: application/json" \
+                    "https://api.linode.com/v4/nodebalancers/${nbid}/firewalls" || true)
+                  
+                  fwCount=$(echo $fw | jq -r '.data | length')
+                  ips=$(echo $fw | jq -r '.data[].rules.inbound[].addresses.ipv4[]')
+                  if [[ $fwCount -eq 1 && -n $ips && ! $ips == *"7.7.7.7/32"* ]]; then
+                      echo "firewall attached and rule updated"
+                      break
+                  fi
+                  sleep 10
+              done
+            check:
+                ($error == null): true
+                (contains($stdout, 'firewall attached and rule updated')): true

e2e/test/lb-fw-update-acl/create-pods-services.yaml

@@ -0,0 +1,68 @@
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: lb-fw-update-acl
+  name: test
+spec:
+  replicas: 2
+  selector:
+    matchLabels:
+      app: lb-fw-update-acl
+  template:
+    metadata:
+      labels:
+        app: lb-fw-update-acl
+    spec:
+      affinity:
+        podAntiAffinity:
+          preferredDuringSchedulingIgnoredDuringExecution:
+          - podAffinityTerm:
+              labelSelector:
+                matchExpressions:
+                - key: app
+                  operator: In
+                  values:
+                  - simple-lb
+              topologyKey: kubernetes.io/hostname
+            weight: 100
+      containers:
+      - image: appscode/test-server:2.3
+        name: test
+        ports:
+        - name: http-1
+          containerPort: 8080
+          protocol: TCP
+        env:
+        - name: POD_NAME
+          valueFrom:
+            fieldRef:
+              apiVersion: v1
+              fieldPath: metadata.name
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: svc-test
+  annotations:
+    service.beta.kubernetes.io/linode-loadbalancer-firewall-acl: |
+      {
+        "denyList": {
+            "ipv4": ["8.8.8.8/32",
+                    "9.9.9.9/32",
+                    "7.7.7.7/32"]
+        }
+      }
+  labels:
+    app: lb-fw-update-acl
+spec:
+  type: LoadBalancer
+  selector:
+    app: lb-fw-update-acl
+  ports:
+    - name: http-1
+      protocol: TCP
+      port: 80
+      targetPort: 8080
+  sessionAffinity: None