[fix] reconcile NodeBalancer firewall rules on port changes

cloud/linode/firewall/firewalls.go

@@ -24,6 +24,7 @@
 	maxRulesPerFirewall     = 25
 	accept                  = "ACCEPT"
 	drop                    = "DROP"
+	portRangeParts          = 2
 )
 
 var (
@@ -130,9 +131,88 @@
 	return false
 }
 
+func parsePorts(ports string) ([]int32, error) {
+	var result []int32
+	portParts := strings.Split(ports, ",")
+	for _, part := range portParts {
+		part = strings.TrimSpace(part)
+		if strings.Contains(part, "-") {
+			rangeParts := strings.Split(part, "-")
+			if len(rangeParts) != portRangeParts {
+				return nil, fmt.Errorf("invalid range format: %s", part)
+			}
+
+			start, err1 := strconv.ParseInt(rangeParts[0], 10, 32)
+			end, err2 := strconv.ParseInt(rangeParts[1], 10, 32)
+			if err1 != nil || err2 != nil {
+				return nil, fmt.Errorf("invalid number in range: %s", part)
+			}
+			if start > end {
+				return nil, fmt.Errorf("start greater than end in range: %s", part)
+			}
+
+			for i := start; i <= end; i++ {
+				result = append(result, int32(i))
+			}
+		} else {
+			port, err := strconv.ParseInt(part, 10, 32)
+			if err != nil {
+				return nil, fmt.Errorf("invalid port: %s", part)
+			}
+			result = append(result, int32(port))
+		}
+	}
+
+	return result, nil
+}
+
+func isPortsChanged(rules []linodego.FirewallRule, service *v1.Service) bool {
+	// Service has at least one port, so we can check if there are any rules in firewall
+	// We only care about the first rule, as all rules should have same ports
+	if len(rules) == 0 {
+		return true
+	}
+	oldPorts := rules[0].Ports
+	if oldPorts == "" {
+		return true
+	}
+	// Split the old ports by comma and convert to a slice of strings
+	oldPortsSlice, err := parsePorts(oldPorts)
+	if err != nil {
+		klog.Errorf("Error parsing old ports: %v", err)
+		return true
+	}
+	// Create a map to store the old ports for easy lookup
+	oldPortsMap := make(map[int32]struct{}, len(oldPortsSlice))
+	for _, port := range oldPortsSlice {
+		oldPortsMap[port] = struct{}{}
+	}
+	svcPortsMap := make(map[int32]struct{}, len(service.Spec.Ports))
+	for _, port := range service.Spec.Ports {
+		svcPortsMap[port.Port] = struct{}{}
+	}
+
+	// Check if the ports in the service are different from the old ports
+	for _, port := range service.Spec.Ports {
+		if _, ok := oldPortsMap[port.Port]; !ok {
+			return true
+		}
+	}
+
+	// Check if there are any old ports that are not in the service
+	for _, port := range oldPortsSlice {
+		if _, ok := svcPortsMap[port]; !ok {
+			return true
+		}
+	}
+
+	// If all ports match, return false
+	return false
+}
+
 // ruleChanged takes an old FirewallRuleSet and new aclConfig and returns if
 // the IPs of the FirewallRuleSet would be changed with the new ACL Config
-func ruleChanged(old linodego.FirewallRuleSet, newACL aclConfig) bool {
+func ruleChanged(old linodego.FirewallRuleSet, newACL aclConfig, service *v1.Service) bool {
 	var ips *linodego.NetworkAddresses
 	if newACL.AllowList != nil {
 		// this is a allowList, this means that the rules should have `DROP` as inboundpolicy
@@ -156,7 +236,7 @@
 		ips = newACL.DenyList
 	}
 
-	return ipsChanged(ips, old.Inbound)
+	return ipsChanged(ips, old.Inbound) || isPortsChanged(old.Inbound, service)
 }
 
 func chunkIPs(ips []string) [][]string {
@@ -463,7 +543,7 @@
 				return err
 			}
 
-			changed := ruleChanged(firewalls[0].Rules, acl)
+			changed := ruleChanged(firewalls[0].Rules, acl, service)
 			if !changed {
 				return nil
 			}

cloud/linode/firewall/firewalls_test.go

@@ -0,0 +1,113 @@
+package firewall
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/linode/linodego"
+	v1 "k8s.io/api/core/v1"
+)
+
+// makeOldRuleSet constructs a FirewallRuleSet with the given IPs, ports string, and policy.
+func makeOldRuleSet(ipList []string, ports string, policy string) linodego.FirewallRuleSet {
+	ips := linodego.NetworkAddresses{IPv4: &ipList}
+	rule := linodego.FirewallRule{
+		Protocol:  "TCP",
+		Ports:     ports,
+		Addresses: ips,
+	}
+	return linodego.FirewallRuleSet{
+		InboundPolicy: policy,
+		Inbound:       []linodego.FirewallRule{rule},
+	}
+}
+
+func TestRuleChanged(t *testing.T) {
+	tests := []struct {
+		name       string
+		oldIPs     []string
+		oldPorts   string
+		policy     string
+		newACL     aclConfig
+		svcPorts   []int32
+		wantChange bool
+	}{
+		{
+			name:       "NoChange",
+			oldIPs:     []string{"1.2.3.4/32"},
+			oldPorts:   "80,8080",
+			policy:     drop,
+			newACL:     aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}},
+			svcPorts:   []int32{80, 8080},
+			wantChange: false,
+		},
+		{
+			name:       "IPChange",
+			oldIPs:     []string{"1.2.3.4/32"},
+			oldPorts:   "80",
+			policy:     drop,
+			newACL:     aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"5.6.7.8/32"}}},
+			svcPorts:   []int32{80},
+			wantChange: true,
+		},
+		{
+			name:       "PortChange",
+			oldIPs:     []string{"1.2.3.4/32"},
+			oldPorts:   "80",
+			policy:     drop,
+			newACL:     aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}},
+			svcPorts:   []int32{80, 8080},
+			wantChange: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			old := makeOldRuleSet(tc.oldIPs, tc.oldPorts, tc.policy)
+			svc := &v1.Service{Spec: v1.ServiceSpec{Ports: func() []v1.ServicePort {
+				ps := make([]v1.ServicePort, len(tc.svcPorts))
+				for i, p := range tc.svcPorts {
+					ps[i] = v1.ServicePort{Port: p}
+				}
+				return ps
+			}()}}
+			got := ruleChanged(old, tc.newACL, svc)
+			if got != tc.wantChange {
+				t.Errorf("ruleChanged() = %v, want %v", got, tc.wantChange)
+			}
+		})
+	}
+}
+
+func TestParsePorts(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		want    []int32
+		wantErr bool
+	}{
+		{"ValidSingle", "80", []int32{80}, false},
+		{"ValidMultiple", "80,443", []int32{80, 443}, false},
+		{"ValidRange", "100-102", []int32{100, 101, 102}, false},
+		{"Combined", "80,100-102,8080", []int32{80, 100, 101, 102, 8080}, false},
+		{"Spaces", " 80 ,  443-445 ", []int32{80, 443, 444, 445}, false},
+		{"InvalidRangeFormat", "1-2-3", nil, true},
+		{"InvalidRangeFormat2", "100-", nil, true},
+		{"NonNumeric", "abc", nil, true},
+		{"NonNumeric2", "80,a", nil, true},
+		{"NonNumeric3", "a-100", nil, true},
+		{"StartGreaterThanEnd", "200-100", nil, true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parsePorts(tc.input)
+			if (err != nil) != tc.wantErr {
+				t.Fatalf("parsePorts(%q) error = %v, wantErr %v", tc.input, err, tc.wantErr)
+			}
+			if !tc.wantErr && !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("parsePorts(%q) = %v, want %v", tc.input, got, tc.want)
+			}
+		})
+	}
+}

e2e/test/lb-update-port/chainsaw-test.yaml

@@ -0,0 +1,152 @@
+# 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-update-port
+  labels:
+    all:
+    lke:
+spec:
+  namespace: "lb-update-port"
+  steps:
+    - 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 loadbalancer ip and check both pods reachable
+      try:
+        - script:
+            content: |
+              set -e
+              sleep 30
+              IP=$(kubectl get svc svc-test -n $NAMESPACE -o json | jq -r .status.loadBalancer.ingress[0].ip)
+
+              podnames=()
+
+              for i in {1..10}; do
+                  if [[ ${#podnames[@]} -lt 2 ]]; then
+                      output=$(curl -s $IP:80 | jq -e .podName || true)
+
+                      if [[ "$output" == *"test-"* ]]; then
+                          unique=true
+                          for i in "${array[@]}"; do
+                              if [[ "$i" == "$output" ]]; then
+                                  unique=false
+                                  break
+                              fi
+                          done
+                          if [[ "$unique" == true ]]; then
+                              podnames+=($output)
+                          fi
+                      fi
+                  else
+                      break
+                  fi
+                  sleep 10
+              done
+
+              if [[ ${#podnames[@]} -lt 2 ]]; then
+                  echo "all pods failed to respond"
+              else
+                  echo "all pods responded"
+              fi
+            check:
+              ($error == null): true
+              (contains($stdout, 'all pods responded')): true
+    - name: Update service
+      try:
+        - apply:
+            file: update-port-service.yaml
+    - name: Check pods reachable on new port
+      try:
+        - script:
+            content: |
+              set -e
+              #wait for changes to propagate to the LB
+              sleep 60
+              IP=$(kubectl get svc svc-test -n $NAMESPACE -o json | jq -r .status.loadBalancer.ingress[0].ip)
+
+              podnames=()
+
+              for i in {1..10}; do
+                  if [[ ${#podnames[@]} -lt 2 ]]; then
+                      output=$(curl -s $IP:8080 | jq -e .podName || true)
+
+                      if [[ "$output" == *"test-"* ]]; then
+                          unique=true
+                          for i in "${array[@]}"; do
+                              if [[ "$i" == "$output" ]]; then
+                                  unique=false
+                                  break
+                              fi
+                          done
+                          if [[ "$unique" == true ]]; then
+                              podnames+=($output)
+                          fi
+                      fi
+                  else
+                      break
+                  fi
+                  sleep 10
+              done
+
+              if [[ ${#podnames[@]} -lt 2 ]]; then
+                  echo "all pods failed to respond"
+              else
+                  echo "all pods responded"
+              fi
+            check:
+              ($error == null): true
+              (contains($stdout, 'all pods responded')): true
+    - name: Fetch firewall ID and check ports 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)
+                  echo "$fw" | jq -r '.data[].rules.inbound[]'
+                  if echo "$fw" | jq -r '.data[].rules.inbound[].ports' | grep 8080 ; then
+                    echo "firewall rule updated with new port"
+                    break
+                  fi
+                  sleep 10
+              done
+            check:
+                ($error == null): true
+                (contains($stdout, 'firewall rule updated with new port')): true
+    - name: Delete Pods
+      try:
+        - delete:
+            ref:
+              apiVersion: v1
+              kind: Pod
+    - name: Delete Service
+      try:
+        - delete:
+            ref:
+              apiVersion: v1
+              kind: Service