From 1f644a078c01399ee0bc0b967c169ecf712acc36 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Mon, 27 Jan 2025 23:21:10 -0500 Subject: [PATCH] fix ACL update bug and add e2e test for it (#306) --- cloud/linode/firewall/firewalls.go | 6 + cloud/linode/loadbalancers_test.go | 123 ++++++++++++++++-- e2e/test/lb-fw-update-acl/chainsaw-test.yaml | 93 +++++++++++++ .../create-pods-services.yaml | 68 ++++++++++ e2e/test/lb-fw-update-acl/update-service.yaml | 25 ++++ 5 files changed, 305 insertions(+), 10 deletions(-) create mode 100644 e2e/test/lb-fw-update-acl/chainsaw-test.yaml create mode 100644 e2e/test/lb-fw-update-acl/create-pods-services.yaml create mode 100644 e2e/test/lb-fw-update-acl/update-service.yaml diff --git a/cloud/linode/firewall/firewalls.go b/cloud/linode/firewall/firewalls.go index 60c51a9a..fcfc10a9 100644 --- a/cloud/linode/firewall/firewalls.go +++ b/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 diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go index 2cce0575..6657b35f 100644 --- a/cloud/linode/loadbalancers_test.go +++ b/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,14 +413,15 @@ 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) } } @@ -426,14 +429,15 @@ func testCreateNodeBalancerWithDenyList(t *testing.T, client *linodego.Client, f 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) { diff --git a/e2e/test/lb-fw-update-acl/chainsaw-test.yaml b/e2e/test/lb-fw-update-acl/chainsaw-test.yaml new file mode 100644 index 00000000..15b05807 --- /dev/null +++ b/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 diff --git a/e2e/test/lb-fw-update-acl/create-pods-services.yaml b/e2e/test/lb-fw-update-acl/create-pods-services.yaml new file mode 100644 index 00000000..ddcab7f6 --- /dev/null +++ b/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 diff --git a/e2e/test/lb-fw-update-acl/update-service.yaml b/e2e/test/lb-fw-update-acl/update-service.yaml new file mode 100644 index 00000000..f05597e0 --- /dev/null +++ b/e2e/test/lb-fw-update-acl/update-service.yaml @@ -0,0 +1,25 @@ +--- +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"] + } + } + 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