From 77d268df31773fa4247bfdf96c4dfe654e776b5a Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 20 Mar 2026 12:16:29 +0100 Subject: [PATCH 1/2] fix: Default to TCP protocol when proto empty When createLoadBalancerRule is called with an empty protocol, the API will return the loadbalancerrule without a protocol field defined when calling listLoadBalancerRules. CloudStack appears to default to TCP in this case, but this is not reflected in listLoadBalancerRulesResponse --- cloudstack/protocol.go | 2 + cloudstack/protocol_test.go | 164 ++++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100644 cloudstack/protocol_test.go diff --git a/cloudstack/protocol.go b/cloudstack/protocol.go index 87202242..df4710e8 100644 --- a/cloudstack/protocol.go +++ b/cloudstack/protocol.go @@ -101,6 +101,8 @@ func ProtocolFromServicePort(port corev1.ServicePort, service *corev1.Service) L // CloudStack load balancer protocol name. func ProtocolFromLoadBalancer(protocol string) LoadBalancerProtocol { switch protocol { + case "": + fallthrough case ProtoTCP: return LoadBalancerProtocolTCP case ProtoUDP: diff --git a/cloudstack/protocol_test.go b/cloudstack/protocol_test.go new file mode 100644 index 00000000..fcc4227e --- /dev/null +++ b/cloudstack/protocol_test.go @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package cloudstack + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestLoadBalancerProtocol_String(t *testing.T) { + tests := []struct { + name string + protocol LoadBalancerProtocol + want string + }{ + {"TCP", LoadBalancerProtocolTCP, "tcp"}, + {"UDP", LoadBalancerProtocolUDP, "udp"}, + {"TCPProxy", LoadBalancerProtocolTCPProxy, "tcp-proxy"}, + {"Invalid", LoadBalancerProtocolInvalid, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.protocol.String(); got != tt.want { + t.Errorf("String() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestLoadBalancerProtocol_CSProtocol(t *testing.T) { + tests := []struct { + name string + protocol LoadBalancerProtocol + want string + }{ + {"TCP", LoadBalancerProtocolTCP, "tcp"}, + {"UDP", LoadBalancerProtocolUDP, "udp"}, + {"TCPProxy", LoadBalancerProtocolTCPProxy, "tcp-proxy"}, + {"Invalid", LoadBalancerProtocolInvalid, ""}, + {"Unknown value", LoadBalancerProtocol(99), ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.protocol.CSProtocol(); got != tt.want { + t.Errorf("CSProtocol() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestLoadBalancerProtocol_IPProtocol(t *testing.T) { + tests := []struct { + name string + protocol LoadBalancerProtocol + want string + }{ + {"TCP", LoadBalancerProtocolTCP, "tcp"}, + {"UDP", LoadBalancerProtocolUDP, "udp"}, + {"TCPProxy maps to tcp", LoadBalancerProtocolTCPProxy, "tcp"}, + {"Invalid", LoadBalancerProtocolInvalid, ""}, + {"Unknown value", LoadBalancerProtocol(99), ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.protocol.IPProtocol(); got != tt.want { + t.Errorf("IPProtocol() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestProtocolFromServicePort(t *testing.T) { + tests := []struct { + name string + port corev1.ServicePort + annotations map[string]string + want LoadBalancerProtocol + }{ + { + name: "TCP without proxy", + port: corev1.ServicePort{Protocol: corev1.ProtocolTCP}, + want: LoadBalancerProtocolTCP, + }, + { + name: "TCP with proxy annotation", + port: corev1.ServicePort{Protocol: corev1.ProtocolTCP}, + annotations: map[string]string{ + ServiceAnnotationLoadBalancerProxyProtocol: "true", + }, + want: LoadBalancerProtocolTCPProxy, + }, + { + name: "TCP with proxy annotation false", + port: corev1.ServicePort{Protocol: corev1.ProtocolTCP}, + annotations: map[string]string{ + ServiceAnnotationLoadBalancerProxyProtocol: "false", + }, + want: LoadBalancerProtocolTCP, + }, + { + name: "UDP", + port: corev1.ServicePort{Protocol: corev1.ProtocolUDP}, + want: LoadBalancerProtocolUDP, + }, + { + name: "SCTP is invalid", + port: corev1.ServicePort{Protocol: corev1.ProtocolSCTP}, + want: LoadBalancerProtocolInvalid, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-svc", + Annotations: tt.annotations, + }, + } + if got := ProtocolFromServicePort(tt.port, svc); got != tt.want { + t.Errorf("ProtocolFromServicePort() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestProtocolFromLoadBalancer(t *testing.T) { + tests := []struct { + name string + protocol string + want LoadBalancerProtocol + }{ + {"empty string defaults to TCP", "", LoadBalancerProtocolTCP}, + {"tcp", "tcp", LoadBalancerProtocolTCP}, + {"udp", "udp", LoadBalancerProtocolUDP}, + {"tcp-proxy", "tcp-proxy", LoadBalancerProtocolTCPProxy}, + {"unknown protocol", "sctp", LoadBalancerProtocolInvalid}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ProtocolFromLoadBalancer(tt.protocol); got != tt.want { + t.Errorf("ProtocolFromLoadBalancer(%q) = %v, want %v", tt.protocol, got, tt.want) + } + }) + } +} From 397f0d3decbec33fc03934aba5e570bc042d81f7 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Fri, 20 Mar 2026 13:08:22 +0100 Subject: [PATCH 2/2] fix: Filter LB rules by name prefix in getLoadBalancerByID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getLoadBalancerByID queries by publicipid+networkid, which returns all LB rules on that IP — including rules belonging to other services. The cleanup loop in EnsureLoadBalancer then deletes these foreign rules. Apply filterRulesByPrefix (already used in getLoadBalancerByName) to keep only rules matching the current service's name prefix. Co-Authored-By: Claude Opus 4.6 (1M context) --- cloudstack/cloudstack_loadbalancer.go | 3 +- cloudstack/cloudstack_loadbalancer_test.go | 51 ++++++++++++++++++++-- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/cloudstack/cloudstack_loadbalancer.go b/cloudstack/cloudstack_loadbalancer.go index f7bcdbf2..1c8114bc 100644 --- a/cloudstack/cloudstack_loadbalancer.go +++ b/cloudstack/cloudstack_loadbalancer.go @@ -667,7 +667,8 @@ func (cs *CSCloud) getLoadBalancerByID(name, ipAddrID, networkID string) (*loadB return nil, fmt.Errorf("error retrieving load balancer rules by IP ID %v: %w", ipAddrID, err) } - for _, lbRule := range l.LoadBalancerRules { + filtered := filterRulesByPrefix(l.LoadBalancerRules, lb.name+"-") + for _, lbRule := range filtered { lb.rules[lbRule.Name] = lbRule if lb.ipAddr != "" && lb.ipAddr != lbRule.Publicip { diff --git a/cloudstack/cloudstack_loadbalancer_test.go b/cloudstack/cloudstack_loadbalancer_test.go index 51ca6aee..51d1e8e7 100644 --- a/cloudstack/cloudstack_loadbalancer_test.go +++ b/cloudstack/cloudstack_loadbalancer_test.go @@ -4172,8 +4172,8 @@ func TestGetLoadBalancerByID(t *testing.T) { listResp := &cloudstack.ListLoadBalancerRulesResponse{ Count: 2, LoadBalancerRules: []*cloudstack.LoadBalancerRule{ - {Name: "lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, - {Name: "lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "my-lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, }, } @@ -4204,6 +4204,51 @@ func TestGetLoadBalancerByID(t *testing.T) { } }) + t.Run("filters out rules not matching LB name prefix", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + // API returns rules from multiple services sharing the same IP + listResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 4, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{ + {Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "my-lb-tcp-443", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "other-svc-tcp-8080", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "another-svc-tcp-9090", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + }, + } + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(listResp, nil) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + lb, err := cs.getLoadBalancerByID("my-lb", "ip-1", "net-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(lb.rules) != 2 { + t.Fatalf("expected 2 rules (only my-lb- prefix), got %d", len(lb.rules)) + } + if _, ok := lb.rules["my-lb-tcp-80"]; !ok { + t.Error("expected rule my-lb-tcp-80 to be present") + } + if _, ok := lb.rules["my-lb-tcp-443"]; !ok { + t.Error("expected rule my-lb-tcp-443 to be present") + } + if _, ok := lb.rules["other-svc-tcp-8080"]; ok { + t.Error("rule other-svc-tcp-8080 should have been filtered out") + } + }) + t.Run("no rules found", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -4341,7 +4386,7 @@ func TestGetLoadBalancerOrchestrator(t *testing.T) { idResp := &cloudstack.ListLoadBalancerRulesResponse{ Count: 1, LoadBalancerRules: []*cloudstack.LoadBalancerRule{ - {Name: "lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + {Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, }, }