[fix] reconcile NodeBalancer firewall rules on port changes (#380)

* firewall: reconcile NodeBalancer firewall rules on port changes

Previously, updateNodeBalancerFirewallWithACL only considered ACL IP
annotation changes and never updated firewall rules when Service
ports were added or removed—leading to stale or unintended open ports.

This commit:
- Rebuilds the complete desired rule set (Service.Spec.Ports + ACL)
- Compares existing inbound rules vs. desired
- Extends ruleChanged to detect both IP and port modifications
- Adds unit tests

* add: e2e tests for port change in service

Author: jescarri

cloud/linode/firewall/firewalls.go

@@ -24,6 +24,7 @@ const (
 	maxRulesPerFirewall     = 25
 	accept                  = "ACCEPT"
 	drop                    = "DROP"
+	portRangeParts          = 2
 )
 
 var (
@@ -130,9 +131,88 @@ func ipsChanged(ips *linodego.NetworkAddresses, rules []linodego.FirewallRule) b
 	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 @@ func ruleChanged(old linodego.FirewallRuleSet, newACL aclConfig) bool {
 		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 @@ func (l *LinodeClient) updateNodeBalancerFirewallWithACL(
 				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