Skip to content

Commit a52a28a

Browse files
hrakhrak
authored
Merge pull request #128 from leaseweb/fix_lb_proto
Fix getLoadBalancerByID accidentally deleting non CCM managed rules + default to TCP proto in lb rules if proto empty
2 parents 52df887 + 397f0d3 commit a52a28a

File tree

4 files changed

+216
-4
lines changed

4 files changed

+216
-4
lines changed

‎cloudstack/cloudstack_loadbalancer.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -667,7 +667,8 @@ func (cs *CSCloud) getLoadBalancerByID(name, ipAddrID, networkID string) (*loadB
667667
return nil, fmt.Errorf("error retrieving load balancer rules by IP ID %v: %w", ipAddrID, err)
668668
}
669669

670-
for _, lbRule := range l.LoadBalancerRules {
670+
filtered := filterRulesByPrefix(l.LoadBalancerRules, lb.name+"-")
671+
for _, lbRule := range filtered {
671672
lb.rules[lbRule.Name] = lbRule
672673

673674
if lb.ipAddr != "" && lb.ipAddr != lbRule.Publicip {

‎cloudstack/cloudstack_loadbalancer_test.go‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4172,8 +4172,8 @@ func TestGetLoadBalancerByID(t *testing.T) {
41724172
listResp := &cloudstack.ListLoadBalancerRulesResponse{
41734173
Count: 2,
41744174
LoadBalancerRules: []*cloudstack.LoadBalancerRule{
4175-
{Name: "lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4176-
{Name: "lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4175+
{Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4176+
{Name: "my-lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
41774177
},
41784178
}
41794179

@@ -4204,6 +4204,51 @@ func TestGetLoadBalancerByID(t *testing.T) {
42044204
}
42054205
})
42064206

4207+
t.Run("filters out rules not matching LB name prefix", func(t *testing.T) {
4208+
ctrl := gomock.NewController(t)
4209+
t.Cleanup(ctrl.Finish)
4210+
4211+
mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
4212+
listParams := &cloudstack.ListLoadBalancerRulesParams{}
4213+
4214+
// API returns rules from multiple services sharing the same IP
4215+
listResp := &cloudstack.ListLoadBalancerRulesResponse{
4216+
Count: 4,
4217+
LoadBalancerRules: []*cloudstack.LoadBalancerRule{
4218+
{Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4219+
{Name: "my-lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4220+
{Name: "other-svc-tcp-8080", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4221+
{Name: "another-svc-tcp-9090", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4222+
},
4223+
}
4224+
4225+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams)
4226+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(listResp, nil)
4227+
4228+
cs := &CSCloud{
4229+
client: &cloudstack.CloudStackClient{
4230+
LoadBalancer: mockLB,
4231+
},
4232+
}
4233+
4234+
lb, err := cs.getLoadBalancerByID("my-lb", "ip-1", "net-1")
4235+
if err != nil {
4236+
t.Fatalf("unexpected error: %v", err)
4237+
}
4238+
if len(lb.rules) != 2 {
4239+
t.Fatalf("expected 2 rules (only my-lb- prefix), got %d", len(lb.rules))
4240+
}
4241+
if _, ok := lb.rules["my-lb-tcp-80"]; !ok {
4242+
t.Error("expected rule my-lb-tcp-80 to be present")
4243+
}
4244+
if _, ok := lb.rules["my-lb-tcp-443"]; !ok {
4245+
t.Error("expected rule my-lb-tcp-443 to be present")
4246+
}
4247+
if _, ok := lb.rules["other-svc-tcp-8080"]; ok {
4248+
t.Error("rule other-svc-tcp-8080 should have been filtered out")
4249+
}
4250+
})
4251+
42074252
t.Run("no rules found", func(t *testing.T) {
42084253
ctrl := gomock.NewController(t)
42094254
t.Cleanup(ctrl.Finish)
@@ -4341,7 +4386,7 @@ func TestGetLoadBalancerOrchestrator(t *testing.T) {
43414386
idResp := &cloudstack.ListLoadBalancerRulesResponse{
43424387
Count: 1,
43434388
LoadBalancerRules: []*cloudstack.LoadBalancerRule{
4344-
{Name: "lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
4389+
{Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"},
43454390
},
43464391
}
43474392

‎cloudstack/protocol.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ func ProtocolFromServicePort(port corev1.ServicePort, service *corev1.Service) L
101101
// CloudStack load balancer protocol name.
102102
func ProtocolFromLoadBalancer(protocol string) LoadBalancerProtocol {
103103
switch protocol {
104+
case "":
105+
fallthrough
104106
case ProtoTCP:
105107
return LoadBalancerProtocolTCP
106108
case ProtoUDP:

‎cloudstack/protocol_test.go‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package cloudstack
21+
22+
import (
23+
"testing"
24+
25+
corev1 "k8s.io/api/core/v1"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27+
)
28+
29+
func TestLoadBalancerProtocol_String(t *testing.T) {
30+
tests := []struct {
31+
name string
32+
protocol LoadBalancerProtocol
33+
want string
34+
}{
35+
{"TCP", LoadBalancerProtocolTCP, "tcp"},
36+
{"UDP", LoadBalancerProtocolUDP, "udp"},
37+
{"TCPProxy", LoadBalancerProtocolTCPProxy, "tcp-proxy"},
38+
{"Invalid", LoadBalancerProtocolInvalid, ""},
39+
}
40+
for _, tt := range tests {
41+
t.Run(tt.name, func(t *testing.T) {
42+
if got := tt.protocol.String(); got != tt.want {
43+
t.Errorf("String() = %q, want %q", got, tt.want)
44+
}
45+
})
46+
}
47+
}
48+
49+
func TestLoadBalancerProtocol_CSProtocol(t *testing.T) {
50+
tests := []struct {
51+
name string
52+
protocol LoadBalancerProtocol
53+
want string
54+
}{
55+
{"TCP", LoadBalancerProtocolTCP, "tcp"},
56+
{"UDP", LoadBalancerProtocolUDP, "udp"},
57+
{"TCPProxy", LoadBalancerProtocolTCPProxy, "tcp-proxy"},
58+
{"Invalid", LoadBalancerProtocolInvalid, ""},
59+
{"Unknown value", LoadBalancerProtocol(99), ""},
60+
}
61+
for _, tt := range tests {
62+
t.Run(tt.name, func(t *testing.T) {
63+
if got := tt.protocol.CSProtocol(); got != tt.want {
64+
t.Errorf("CSProtocol() = %q, want %q", got, tt.want)
65+
}
66+
})
67+
}
68+
}
69+
70+
func TestLoadBalancerProtocol_IPProtocol(t *testing.T) {
71+
tests := []struct {
72+
name string
73+
protocol LoadBalancerProtocol
74+
want string
75+
}{
76+
{"TCP", LoadBalancerProtocolTCP, "tcp"},
77+
{"UDP", LoadBalancerProtocolUDP, "udp"},
78+
{"TCPProxy maps to tcp", LoadBalancerProtocolTCPProxy, "tcp"},
79+
{"Invalid", LoadBalancerProtocolInvalid, ""},
80+
{"Unknown value", LoadBalancerProtocol(99), ""},
81+
}
82+
for _, tt := range tests {
83+
t.Run(tt.name, func(t *testing.T) {
84+
if got := tt.protocol.IPProtocol(); got != tt.want {
85+
t.Errorf("IPProtocol() = %q, want %q", got, tt.want)
86+
}
87+
})
88+
}
89+
}
90+
91+
func TestProtocolFromServicePort(t *testing.T) {
92+
tests := []struct {
93+
name string
94+
port corev1.ServicePort
95+
annotations map[string]string
96+
want LoadBalancerProtocol
97+
}{
98+
{
99+
name: "TCP without proxy",
100+
port: corev1.ServicePort{Protocol: corev1.ProtocolTCP},
101+
want: LoadBalancerProtocolTCP,
102+
},
103+
{
104+
name: "TCP with proxy annotation",
105+
port: corev1.ServicePort{Protocol: corev1.ProtocolTCP},
106+
annotations: map[string]string{
107+
ServiceAnnotationLoadBalancerProxyProtocol: "true",
108+
},
109+
want: LoadBalancerProtocolTCPProxy,
110+
},
111+
{
112+
name: "TCP with proxy annotation false",
113+
port: corev1.ServicePort{Protocol: corev1.ProtocolTCP},
114+
annotations: map[string]string{
115+
ServiceAnnotationLoadBalancerProxyProtocol: "false",
116+
},
117+
want: LoadBalancerProtocolTCP,
118+
},
119+
{
120+
name: "UDP",
121+
port: corev1.ServicePort{Protocol: corev1.ProtocolUDP},
122+
want: LoadBalancerProtocolUDP,
123+
},
124+
{
125+
name: "SCTP is invalid",
126+
port: corev1.ServicePort{Protocol: corev1.ProtocolSCTP},
127+
want: LoadBalancerProtocolInvalid,
128+
},
129+
}
130+
for _, tt := range tests {
131+
t.Run(tt.name, func(t *testing.T) {
132+
svc := &corev1.Service{
133+
ObjectMeta: metav1.ObjectMeta{
134+
Name: "test-svc",
135+
Annotations: tt.annotations,
136+
},
137+
}
138+
if got := ProtocolFromServicePort(tt.port, svc); got != tt.want {
139+
t.Errorf("ProtocolFromServicePort() = %v, want %v", got, tt.want)
140+
}
141+
})
142+
}
143+
}
144+
145+
func TestProtocolFromLoadBalancer(t *testing.T) {
146+
tests := []struct {
147+
name string
148+
protocol string
149+
want LoadBalancerProtocol
150+
}{
151+
{"empty string defaults to TCP", "", LoadBalancerProtocolTCP},
152+
{"tcp", "tcp", LoadBalancerProtocolTCP},
153+
{"udp", "udp", LoadBalancerProtocolUDP},
154+
{"tcp-proxy", "tcp-proxy", LoadBalancerProtocolTCPProxy},
155+
{"unknown protocol", "sctp", LoadBalancerProtocolInvalid},
156+
}
157+
for _, tt := range tests {
158+
t.Run(tt.name, func(t *testing.T) {
159+
if got := ProtocolFromLoadBalancer(tt.protocol); got != tt.want {
160+
t.Errorf("ProtocolFromLoadBalancer(%q) = %v, want %v", tt.protocol, got, tt.want)
161+
}
162+
})
163+
}
164+
}

0 commit comments

Comments
 (0)