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

Author: Jesus Carrillo

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,154 @@
+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 {
+	// NetworkAddresses for the rule
+	ips := linodego.NetworkAddresses{IPv4: &ipList}
+	// Single inbound rule with these ports and addresses
+	rule := linodego.FirewallRule{
+		Protocol:  "TCP",
+		Ports:     ports,
+		Addresses: ips,
+	}
+	return linodego.FirewallRuleSet{
+		InboundPolicy: policy,
+		Inbound:       []linodego.FirewallRule{rule},
+	}
+}
+
+func TestRuleChanged_NoChange(t *testing.T) {
+	old := makeOldRuleSet(
+		[]string{"1.2.3.4/32"},
+		"80,8080",
+		drop,
+	)
+	newACL := aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}}
+	svc := &v1.Service{Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 80}, {Port: 8080}}}}
+
+	if changed := ruleChanged(old, newACL, svc); changed {
+		t.Errorf("expected no change, but ruleChanged returned true")
+	}
+}
+
+func TestRuleChanged_IPChange(t *testing.T) {
+	old := makeOldRuleSet(
+		[]string{"1.2.3.4/32"},
+		"80",
+		drop,
+	)
+	// Change in IP
+	newACL := aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"5.6.7.8/32"}}}
+	svc := &v1.Service{Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 80}}}}
+
+	if !ruleChanged(old, newACL, svc) {
+		t.Errorf("expected change due to IP modification, but ruleChanged returned false")
+	}
+}
+
+func TestRuleChanged_PortChange(t *testing.T) {
+	old := makeOldRuleSet(
+		[]string{"1.2.3.4/32"},
+		"80",
+		drop,
+	)
+	// Ports updated on the Service
+	newACL := aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}}
+	svc := &v1.Service{Spec: v1.ServiceSpec{Ports: []v1.ServicePort{{Port: 80}, {Port: 8080}}}}
+
+	if !ruleChanged(old, newACL, svc) {
+		t.Errorf("expected change due to port modification, but ruleChanged returned false")
+	}
+}
+
+func TestParsePorts_ValidSingle(t *testing.T) {
+	input := "80"
+	exp := []int32{80}
+	out, err := parsePorts(input)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(out, exp) {
+		t.Errorf("got %v, want %v", out, exp)
+	}
+}
+
+func TestParsePorts_ValidMultiple(t *testing.T) {
+	input := "80,443"
+	exp := []int32{80, 443}
+	out, err := parsePorts(input)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(out, exp) {
+		t.Errorf("got %v, want %v", out, exp)
+	}
+}
+
+func TestParsePorts_ValidRange(t *testing.T) {
+	input := "100-102"
+	exp := []int32{100, 101, 102}
+	out, err := parsePorts(input)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(out, exp) {
+		t.Errorf("got %v, want %v", out, exp)
+	}
+}
+
+func TestParsePorts_Combined(t *testing.T) {
+	input := "80,100-102,8080"
+	exp := []int32{80, 100, 101, 102, 8080}
+	out, err := parsePorts(input)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(out, exp) {
+		t.Errorf("got %v, want %v", out, exp)
+	}
+}
+
+func TestParsePorts_Spaces(t *testing.T) {
+	input := " 80 ,  443-445 "
+	exp := []int32{80, 443, 444, 445}
+	out, err := parsePorts(input)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !reflect.DeepEqual(out, exp) {
+		t.Errorf("got %v, want %v", out, exp)
+	}
+}
+
+func TestParsePorts_InvalidRangeFormat(t *testing.T) {
+	cases := []string{"1-2-3", "100-"}
+	for _, input := range cases {
+		if _, err := parsePorts(input); err == nil {
+			t.Errorf("expected error for invalid range '%s', got nil", input)
+		}
+	}
+}
+
+func TestParsePorts_NonNumeric(t *testing.T) {
+	cases := []string{"abc", "80,a", "a-100"}
+	for _, input := range cases {
+		if _, err := parsePorts(input); err == nil {
+			t.Errorf("expected error for non-numeric '%s', got nil", input)
+		}
+	}
+}
+
+func TestParsePorts_RangeStartGreaterThanEnd(t *testing.T) {
+	input := "200-100"
+	if _, err := parsePorts(input); err == nil {
+		t.Errorf("expected error for start greater than end, got nil")
+	}
+}