Skip to content

Commit cd5a75d

Browse files
jescarrijescarri
authored
[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
1 parent fccf21c commit cd5a75d

File tree

5 files changed

+445
-3
lines changed

5 files changed

+445
-3
lines changed

‎cloud/linode/firewall/firewalls.go‎

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ const (
2424
maxRulesPerFirewall = 25
2525
accept = "ACCEPT"
2626
drop = "DROP"
27+
portRangeParts = 2
2728
)
2829

2930
var (
@@ -130,9 +131,88 @@ func ipsChanged(ips *linodego.NetworkAddresses, rules []linodego.FirewallRule) b
130131
return false
131132
}
132133

134+
func parsePorts(ports string) ([]int32, error) {
135+
var result []int32
136+
portParts := strings.Split(ports, ",")
137+
for _, part := range portParts {
138+
part = strings.TrimSpace(part)
139+
if strings.Contains(part, "-") {
140+
rangeParts := strings.Split(part, "-")
141+
if len(rangeParts) != portRangeParts {
142+
return nil, fmt.Errorf("invalid range format: %s", part)
143+
}
144+
145+
start, err1 := strconv.ParseInt(rangeParts[0], 10, 32)
146+
end, err2 := strconv.ParseInt(rangeParts[1], 10, 32)
147+
if err1 != nil || err2 != nil {
148+
return nil, fmt.Errorf("invalid number in range: %s", part)
149+
}
150+
if start > end {
151+
return nil, fmt.Errorf("start greater than end in range: %s", part)
152+
}
153+
154+
for i := start; i <= end; i++ {
155+
result = append(result, int32(i))
156+
}
157+
} else {
158+
port, err := strconv.ParseInt(part, 10, 32)
159+
if err != nil {
160+
return nil, fmt.Errorf("invalid port: %s", part)
161+
}
162+
result = append(result, int32(port))
163+
}
164+
}
165+
166+
return result, nil
167+
}
168+
169+
func isPortsChanged(rules []linodego.FirewallRule, service *v1.Service) bool {
170+
// Service has at least one port, so we can check if there are any rules in firewall
171+
// We only care about the first rule, as all rules should have same ports
172+
if len(rules) == 0 {
173+
return true
174+
}
175+
oldPorts := rules[0].Ports
176+
if oldPorts == "" {
177+
return true
178+
}
179+
// Split the old ports by comma and convert to a slice of strings
180+
oldPortsSlice, err := parsePorts(oldPorts)
181+
if err != nil {
182+
klog.Errorf("Error parsing old ports: %v", err)
183+
return true
184+
}
185+
// Create a map to store the old ports for easy lookup
186+
oldPortsMap := make(map[int32]struct{}, len(oldPortsSlice))
187+
for _, port := range oldPortsSlice {
188+
oldPortsMap[port] = struct{}{}
189+
}
190+
svcPortsMap := make(map[int32]struct{}, len(service.Spec.Ports))
191+
for _, port := range service.Spec.Ports {
192+
svcPortsMap[port.Port] = struct{}{}
193+
}
194+
195+
// Check if the ports in the service are different from the old ports
196+
for _, port := range service.Spec.Ports {
197+
if _, ok := oldPortsMap[port.Port]; !ok {
198+
return true
199+
}
200+
}
201+
202+
// Check if there are any old ports that are not in the service
203+
for _, port := range oldPortsSlice {
204+
if _, ok := svcPortsMap[port]; !ok {
205+
return true
206+
}
207+
}
208+
209+
// If all ports match, return false
210+
return false
211+
}
212+
133213
// ruleChanged takes an old FirewallRuleSet and new aclConfig and returns if
134214
// the IPs of the FirewallRuleSet would be changed with the new ACL Config
135-
func ruleChanged(old linodego.FirewallRuleSet, newACL aclConfig) bool {
215+
func ruleChanged(old linodego.FirewallRuleSet, newACL aclConfig, service *v1.Service) bool {
136216
var ips *linodego.NetworkAddresses
137217
if newACL.AllowList != nil {
138218
// 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 {
156236
ips = newACL.DenyList
157237
}
158238

159-
return ipsChanged(ips, old.Inbound)
239+
return ipsChanged(ips, old.Inbound) || isPortsChanged(old.Inbound, service)
160240
}
161241

162242
func chunkIPs(ips []string) [][]string {
@@ -463,7 +543,7 @@ func (l *LinodeClient) updateNodeBalancerFirewallWithACL(
463543
return err
464544
}
465545

466-
changed := ruleChanged(firewalls[0].Rules, acl)
546+
changed := ruleChanged(firewalls[0].Rules, acl, service)
467547
if !changed {
468548
return nil
469549
}

‎cloud/linode/firewall/firewalls_test.go‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
package firewall
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"github.com/linode/linodego"
8+
v1 "k8s.io/api/core/v1"
9+
)
10+
11+
// makeOldRuleSet constructs a FirewallRuleSet with the given IPs, ports string, and policy.
12+
func makeOldRuleSet(ipList []string, ports string, policy string) linodego.FirewallRuleSet {
13+
ips := linodego.NetworkAddresses{IPv4: &ipList}
14+
rule := linodego.FirewallRule{
15+
Protocol: "TCP",
16+
Ports: ports,
17+
Addresses: ips,
18+
}
19+
return linodego.FirewallRuleSet{
20+
InboundPolicy: policy,
21+
Inbound: []linodego.FirewallRule{rule},
22+
}
23+
}
24+
25+
func TestRuleChanged(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
oldIPs []string
29+
oldPorts string
30+
policy string
31+
newACL aclConfig
32+
svcPorts []int32
33+
wantChange bool
34+
}{
35+
{
36+
name: "NoChange",
37+
oldIPs: []string{"1.2.3.4/32"},
38+
oldPorts: "80,8080",
39+
policy: drop,
40+
newACL: aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}},
41+
svcPorts: []int32{80, 8080},
42+
wantChange: false,
43+
},
44+
{
45+
name: "IPChange",
46+
oldIPs: []string{"1.2.3.4/32"},
47+
oldPorts: "80",
48+
policy: drop,
49+
newACL: aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"5.6.7.8/32"}}},
50+
svcPorts: []int32{80},
51+
wantChange: true,
52+
},
53+
{
54+
name: "PortChange",
55+
oldIPs: []string{"1.2.3.4/32"},
56+
oldPorts: "80",
57+
policy: drop,
58+
newACL: aclConfig{AllowList: &linodego.NetworkAddresses{IPv4: &[]string{"1.2.3.4/32"}}},
59+
svcPorts: []int32{80, 8080},
60+
wantChange: true,
61+
},
62+
}
63+
64+
for _, tc := range tests {
65+
t.Run(tc.name, func(t *testing.T) {
66+
old := makeOldRuleSet(tc.oldIPs, tc.oldPorts, tc.policy)
67+
svc := &v1.Service{Spec: v1.ServiceSpec{Ports: func() []v1.ServicePort {
68+
ps := make([]v1.ServicePort, len(tc.svcPorts))
69+
for i, p := range tc.svcPorts {
70+
ps[i] = v1.ServicePort{Port: p}
71+
}
72+
return ps
73+
}()}}
74+
got := ruleChanged(old, tc.newACL, svc)
75+
if got != tc.wantChange {
76+
t.Errorf("ruleChanged() = %v, want %v", got, tc.wantChange)
77+
}
78+
})
79+
}
80+
}
81+
82+
func TestParsePorts(t *testing.T) {
83+
tests := []struct {
84+
name string
85+
input string
86+
want []int32
87+
wantErr bool
88+
}{
89+
{"ValidSingle", "80", []int32{80}, false},
90+
{"ValidMultiple", "80,443", []int32{80, 443}, false},
91+
{"ValidRange", "100-102", []int32{100, 101, 102}, false},
92+
{"Combined", "80,100-102,8080", []int32{80, 100, 101, 102, 8080}, false},
93+
{"Spaces", " 80 , 443-445 ", []int32{80, 443, 444, 445}, false},
94+
{"InvalidRangeFormat", "1-2-3", nil, true},
95+
{"InvalidRangeFormat2", "100-", nil, true},
96+
{"NonNumeric", "abc", nil, true},
97+
{"NonNumeric2", "80,a", nil, true},
98+
{"NonNumeric3", "a-100", nil, true},
99+
{"StartGreaterThanEnd", "200-100", nil, true},
100+
}
101+
102+
for _, tc := range tests {
103+
t.Run(tc.name, func(t *testing.T) {
104+
got, err := parsePorts(tc.input)
105+
if (err != nil) != tc.wantErr {
106+
t.Fatalf("parsePorts(%q) error = %v, wantErr %v", tc.input, err, tc.wantErr)
107+
}
108+
if !tc.wantErr && !reflect.DeepEqual(got, tc.want) {
109+
t.Errorf("parsePorts(%q) = %v, want %v", tc.input, got, tc.want)
110+
}
111+
})
112+
}
113+
}

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

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json
2+
apiVersion: chainsaw.kyverno.io/v1alpha1
3+
kind: Test
4+
metadata:
5+
name: lb-update-port
6+
labels:
7+
all:
8+
lke:
9+
spec:
10+
namespace: "lb-update-port"
11+
steps:
12+
- name: Create pods and services
13+
try:
14+
- apply:
15+
file: create-pods-services.yaml
16+
catch:
17+
- describe:
18+
apiVersion: v1
19+
kind: Pod
20+
- describe:
21+
apiVersion: v1
22+
kind: Service
23+
- name: Check that loadbalancer ip is assigned
24+
try:
25+
- assert:
26+
resource:
27+
apiVersion: v1
28+
kind: Service
29+
metadata:
30+
name: svc-test
31+
status:
32+
(loadBalancer.ingress[0].ip != null): true
33+
- name: Fetch loadbalancer ip and check both pods reachable
34+
try:
35+
- script:
36+
content: |
37+
set -e
38+
sleep 30
39+
IP=$(kubectl get svc svc-test -n $NAMESPACE -o json | jq -r .status.loadBalancer.ingress[0].ip)
40+
41+
podnames=()
42+
43+
for i in {1..10}; do
44+
if [[ ${#podnames[@]} -lt 2 ]]; then
45+
output=$(curl -s $IP:80 | jq -e .podName || true)
46+
47+
if [[ "$output" == *"test-"* ]]; then
48+
unique=true
49+
for i in "${array[@]}"; do
50+
if [[ "$i" == "$output" ]]; then
51+
unique=false
52+
break
53+
fi
54+
done
55+
if [[ "$unique" == true ]]; then
56+
podnames+=($output)
57+
fi
58+
fi
59+
else
60+
break
61+
fi
62+
sleep 10
63+
done
64+
65+
if [[ ${#podnames[@]} -lt 2 ]]; then
66+
echo "all pods failed to respond"
67+
else
68+
echo "all pods responded"
69+
fi
70+
check:
71+
($error == null): true
72+
(contains($stdout, 'all pods responded')): true
73+
- name: Update service
74+
try:
75+
- apply:
76+
file: update-port-service.yaml
77+
- name: Check pods reachable on new port
78+
try:
79+
- script:
80+
content: |
81+
set -e
82+
#wait for changes to propagate to the LB
83+
sleep 60
84+
IP=$(kubectl get svc svc-test -n $NAMESPACE -o json | jq -r .status.loadBalancer.ingress[0].ip)
85+
86+
podnames=()
87+
88+
for i in {1..10}; do
89+
if [[ ${#podnames[@]} -lt 2 ]]; then
90+
output=$(curl -s $IP:8080 | jq -e .podName || true)
91+
92+
if [[ "$output" == *"test-"* ]]; then
93+
unique=true
94+
for i in "${array[@]}"; do
95+
if [[ "$i" == "$output" ]]; then
96+
unique=false
97+
break
98+
fi
99+
done
100+
if [[ "$unique" == true ]]; then
101+
podnames+=($output)
102+
fi
103+
fi
104+
else
105+
break
106+
fi
107+
sleep 10
108+
done
109+
110+
if [[ ${#podnames[@]} -lt 2 ]]; then
111+
echo "all pods failed to respond"
112+
else
113+
echo "all pods responded"
114+
fi
115+
check:
116+
($error == null): true
117+
(contains($stdout, 'all pods responded')): true
118+
- name: Fetch firewall ID and check ports are updated
119+
try:
120+
- script:
121+
content: |
122+
set -e
123+
for i in {1..10}; do
124+
nbid=$(KUBECONFIG=$KUBECONFIG NAMESPACE=$NAMESPACE LINODE_TOKEN=$LINODE_TOKEN ../scripts/get-nb-id.sh)
125+
126+
fw=$(curl -s --request GET \
127+
-H "Authorization: Bearer $LINODE_TOKEN" \
128+
-H "Content-Type: application/json" \
129+
-H "accept: application/json" \
130+
"https://api.linode.com/v4/nodebalancers/${nbid}/firewalls" || true)
131+
echo "$fw" | jq -r '.data[].rules.inbound[]'
132+
if echo "$fw" | jq -r '.data[].rules.inbound[].ports' | grep 8080 ; then
133+
echo "firewall rule updated with new port"
134+
break
135+
fi
136+
sleep 10
137+
done
138+
check:
139+
($error == null): true
140+
(contains($stdout, 'firewall rule updated with new port')): true
141+
- name: Delete Pods
142+
try:
143+
- delete:
144+
ref:
145+
apiVersion: v1
146+
kind: Pod
147+
- name: Delete Service
148+
try:
149+
- delete:
150+
ref:
151+
apiVersion: v1
152+
kind: Service

0 commit comments

Comments
 (0)