diff --git a/cloudstack/cloudstack_loadbalancer.go b/cloudstack/cloudstack_loadbalancer.go index 52161931..f6f83608 100644 --- a/cloudstack/cloudstack_loadbalancer.go +++ b/cloudstack/cloudstack_loadbalancer.go @@ -49,9 +49,23 @@ const ( // cluster. This is a workaround for https://github.com/kubernetes/kubernetes/issues/66607 ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" - // ServiceAnnotationLoadBalancerAddress is a read-only annotation indicating the IP address assigned to the load balancer. + // ServiceAnnotationLoadBalancerAddress is the annotation for the IP address assigned to the load balancer. + // Users can set this annotation to request a specific IP address, replacing the deprecated spec.LoadBalancerIP field. + // This annotation takes precedence; spec.LoadBalancerIP is only used as a fallback. ServiceAnnotationLoadBalancerAddress = "service.beta.kubernetes.io/cloudstack-load-balancer-address" + // ServiceAnnotationLoadBalancerKeepIP is a boolean annotation that, when set to "true", + // prevents the public IP from being released when the service is deleted. + ServiceAnnotationLoadBalancerKeepIP = "service.beta.kubernetes.io/cloudstack-load-balancer-keep-ip" + + // ServiceAnnotationLoadBalancerID stores the CloudStack public IP UUID associated with the load balancer. + // Used for efficient ID-based lookups instead of keyword-based searches. + ServiceAnnotationLoadBalancerID = "service.beta.kubernetes.io/cloudstack-load-balancer-id" + + // ServiceAnnotationLoadBalancerNetworkID stores the CloudStack network UUID associated with the load balancer. + // Used together with ServiceAnnotationLoadBalancerID for scoped ID-based lookups. + ServiceAnnotationLoadBalancerNetworkID = "service.beta.kubernetes.io/cloudstack-load-balancer-network-id" + // Used to construct the load balancer name. servicePrefix = "K8s_svc_" lbNameFormat = "%s%s_%s_%s" @@ -77,7 +91,7 @@ func (cs *CSCloud) GetLoadBalancer(ctx context.Context, clusterName string, serv // Get the load balancer details and existing rules. name := cs.GetLoadBalancerName(ctx, clusterName, service) legacyName := cs.getLoadBalancerLegacyName(ctx, clusterName, service) - lb, err := cs.getLoadBalancerByName(name, legacyName) + lb, err := cs.getLoadBalancer(service, name, legacyName) if err != nil { return nil, false, err } @@ -111,7 +125,7 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s // Get the load balancer details and existing rules. name := cs.GetLoadBalancerName(ctx, clusterName, service) legacyName := cs.getLoadBalancerLegacyName(ctx, clusterName, service) - lb, err := cs.getLoadBalancerByName(name, legacyName) + lb, err := cs.getLoadBalancer(service, name, legacyName) if err != nil { return nil, err } @@ -132,48 +146,26 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s return nil, err } + // Resolve the desired IP: annotation takes precedence, spec.LoadBalancerIP is fallback. + desiredIP := getLoadBalancerAddress(service) + if !lb.hasLoadBalancerIP() { //nolint:nestif // Before allocating a new IP, check the service annotation for a previously assigned IP. // This handles recovery from partial failures where the IP was allocated and annotated - // but subsequent operations (rule creation, IP switch) failed. + // but subsequent operations (rule creation) failed. annotatedIP := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, "") if annotatedIP != "" { - specIP := service.Spec.LoadBalancerIP - if specIP == "" || specIP == annotatedIP { - // Case 1: Auto-allocated IP recovery — the annotated IP is the one we want. - found, lookupErr := lb.lookupPublicIPAddress(annotatedIP) - if lookupErr != nil { - klog.Warningf("Error looking up annotated IP %v for recovery: %v", annotatedIP, lookupErr) - } else if found { - klog.V(4).Infof("Recovered previously allocated IP %v from annotation", annotatedIP) - } - } else { - // Case 2: IP switch retry — annotation holds old IP that failed to release. - klog.V(4).Infof("Detected IP switch retry: annotation has %v, spec wants %v; attempting cleanup of old IP", annotatedIP, specIP) - oldFound, lookupErr := lb.lookupPublicIPAddress(annotatedIP) - if lookupErr != nil { - klog.Warningf("Error looking up old annotated IP %v during IP switch cleanup: %v", annotatedIP, lookupErr) - } else if oldFound { - shouldRelease, shouldErr := cs.shouldReleaseLoadBalancerIP(lb, service) - if shouldErr != nil { - klog.Warningf("Error checking if old IP %v should be released: %v", annotatedIP, shouldErr) - } else if shouldRelease { - if releaseErr := lb.releaseLoadBalancerIP(); releaseErr != nil { - klog.Warningf("Best-effort release of old IP %v failed: %v", annotatedIP, releaseErr) - } else { - klog.Infof("Released old IP %v during IP switch retry", annotatedIP) - } - } - } - // Reset so we fall through to allocate the new IP - lb.ipAddr = "" - lb.ipAddrID = "" + found, lookupErr := lb.lookupPublicIPAddress(annotatedIP) + if lookupErr != nil { + klog.Warningf("Error looking up annotated IP %v for recovery: %v", annotatedIP, lookupErr) + } else if found { + klog.V(4).Infof("Recovered previously allocated IP %v from annotation", annotatedIP) } } if !lb.hasLoadBalancerIP() { // Create or retrieve the load balancer IP. - if err := lb.getLoadBalancerIP(service.Spec.LoadBalancerIP); err != nil { + if err := lb.getLoadBalancerIP(desiredIP); err != nil { return nil, err } } @@ -181,68 +173,20 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s msg := fmt.Sprintf("Created new load balancer for service %s with algorithm '%s' and IP address %s", serviceName, lb.algorithm, lb.ipAddr) cs.eventRecorder.Event(service, corev1.EventTypeNormal, "CreatedLoadBalancer", msg) klog.Info(msg) - } else if service.Spec.LoadBalancerIP != "" && service.Spec.LoadBalancerIP != lb.ipAddr { - // LoadBalancerIP was specified and it's different from the current IP. - // Validate the target IP exists before tearing down the old config to avoid - // leaving the service in a broken state if the new IP is invalid. - if err := lb.validatePublicIPAvailable(service.Spec.LoadBalancerIP); err != nil { - return nil, fmt.Errorf("cannot switch load balancer to IP %s: %w", service.Spec.LoadBalancerIP, err) - } - - // Release the old IP first - klog.V(4).Infof("Deleting firewall rules for old ip and releasing old load balancer IP %v, switching to specified IP %v", lb.ipAddr, service.Spec.LoadBalancerIP) - - // Best-effort cleanup of existing rules bound to the current IP to avoid stale deletes / name conflicts. - for _, oldRule := range lb.rules { - proto := ProtocolFromLoadBalancer(oldRule.Protocol) - if proto == LoadBalancerProtocolInvalid { - klog.Warningf("Skipping firewall cleanup for rule %s: unrecognized protocol %q", oldRule.Name, oldRule.Protocol) - } - port64, pErr := strconv.ParseInt(oldRule.Publicport, 10, 32) - if pErr != nil { - klog.Warningf("Skipping firewall cleanup for rule %s: cannot parse port %q: %v", oldRule.Name, oldRule.Publicport, pErr) - } - if proto != LoadBalancerProtocolInvalid && pErr == nil { - if _, fwErr := lb.deleteFirewallRule(oldRule.Publicipid, int(port64), proto); fwErr != nil { - klog.V(4).Infof("Ignoring firewall rule delete error for %s: %v", oldRule.Name, fwErr) - } - } - - if delErr := lb.deleteLoadBalancerRule(oldRule); delErr != nil { - // CloudStack sometimes reports deletes as "invalid value" when the entity is already gone. - if strings.Contains(delErr.Error(), "does not exist") || strings.Contains(delErr.Error(), "Invalid parameter id value") { - klog.V(4).Infof("Load balancer rule %s already removed, continuing: %v", oldRule.Name, delErr) - - continue - } - - return nil, delErr - } - } - - // Prevent any further cleanup from trying to delete stale IDs. - lb.rules = make(map[string]*cloudstack.LoadBalancerRule) - - if err := lb.releaseLoadBalancerIP(); err != nil { - klog.Errorf("attempt to release old load balancer IP failed: %s", err.Error()) - - return nil, fmt.Errorf("failed to release old load balancer IP: %w", err) - } - - if err := lb.getLoadBalancerIP(service.Spec.LoadBalancerIP); err != nil { - klog.Errorf("failed to allocate specified IP %v: %v", service.Spec.LoadBalancerIP, err) - - return nil, fmt.Errorf("failed to allocate specified load balancer IP: %w", err) - } - - msg := fmt.Sprintf("Switched load balancer for service %s to specified IP address %s", serviceName, lb.ipAddr) - cs.eventRecorder.Event(service, corev1.EventTypeNormal, "UpdatedLoadBalancer", msg) + } else if desiredIP != "" && desiredIP != lb.ipAddr { + // IP reassignment on an active load balancer is not supported. + // Users must delete and recreate the service to change the IP. + msg := fmt.Sprintf("Load balancer IP change from %s to %s is not supported; delete and recreate the service to use a different IP", lb.ipAddr, desiredIP) + cs.eventRecorder.Event(service, corev1.EventTypeWarning, "IPChangeNotSupported", msg) + klog.Warning(msg) } klog.V(4).Infof("Load balancer %v is associated with IP %v", lb.name, lb.ipAddr) - // Set the load balancer IP address annotation on the Service + // Set the load balancer annotations on the Service setServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, lb.ipAddr) + setServiceAnnotation(service, ServiceAnnotationLoadBalancerID, lb.ipAddrID) + setServiceAnnotation(service, ServiceAnnotationLoadBalancerNetworkID, lb.networkID) for _, port := range service.Spec.Ports { // Construct the protocol name first, we need it a few times @@ -347,7 +291,7 @@ func (cs *CSCloud) UpdateLoadBalancer(ctx context.Context, clusterName string, s // Get the load balancer details and existing rules. name := cs.GetLoadBalancerName(ctx, clusterName, service) legacyName := cs.getLoadBalancerLegacyName(ctx, clusterName, service) - lb, err := cs.getLoadBalancerByName(name, legacyName) + lb, err := cs.getLoadBalancer(service, name, legacyName) if err != nil { return err } @@ -386,7 +330,7 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st // Get the load balancer details and existing rules. name := cs.GetLoadBalancerName(ctx, clusterName, service) legacyName := cs.getLoadBalancerLegacyName(ctx, clusterName, service) - lb, err := cs.getLoadBalancerByName(name, legacyName) + lb, err := cs.getLoadBalancer(service, name, legacyName) if err != nil { return err } @@ -492,10 +436,10 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st // shouldReleaseLoadBalancerIP determines whether the public IP should be released. func (cs *CSCloud) shouldReleaseLoadBalancerIP(lb *loadBalancer, service *corev1.Service) (bool, error) { - // If the IP was explicitly specified in the service spec, don't release it - // The user is responsible for managing the lifecycle of user-provided IPs - if service.Spec.LoadBalancerIP != "" && service.Spec.LoadBalancerIP == lb.ipAddr { - klog.V(4).Infof("IP %v was explicitly specified in service spec, not releasing", lb.ipAddr) + // If the keep-ip annotation is set to true, don't release the IP. + // The user is responsible for managing the lifecycle of kept IPs. + if getBoolFromServiceAnnotation(service, ServiceAnnotationLoadBalancerKeepIP, false) { + klog.V(4).Infof("IP %v has keep-ip annotation set, not releasing", lb.ipAddr) return false, nil } @@ -553,7 +497,7 @@ func (cs *CSCloud) releaseOrphanedIPIfNeeded(lb *loadBalancer, service *corev1.S } if !shouldRelease { - klog.V(4).Infof("Annotated IP %v should not be released (user-specified or has other rules)", annotatedIP) + klog.V(4).Infof("Annotated IP %v should not be released (keep-ip set or has other rules)", annotatedIP) return nil } @@ -594,6 +538,28 @@ func filterRulesByPrefix(rules []*cloudstack.LoadBalancerRule, prefix string) [] return filtered } +// getLoadBalancer tries to find the load balancer using ID-based lookup first (if annotations +// are present), then falls back to the keyword-based name lookup. +func (cs *CSCloud) getLoadBalancer(service *corev1.Service, name, legacyName string) (*loadBalancer, error) { + if ipAddrID := getLoadBalancerID(service); ipAddrID != "" { + networkID := getLoadBalancerNetworkID(service) + klog.V(4).Infof("Attempting ID-based load balancer lookup: ipAddrID=%v, networkID=%v", ipAddrID, networkID) + + lb, err := cs.getLoadBalancerByID(name, ipAddrID, networkID) + if err != nil { + return nil, err + } + + if len(lb.rules) > 0 { + return lb, nil + } + + klog.V(4).Infof("ID-based lookup returned no rules, falling back to name-based lookup") + } + + return cs.getLoadBalancerByName(name, legacyName) +} + // getLoadBalancerByName retrieves the IP address and ID and all the existing rules it can find. func (cs *CSCloud) getLoadBalancerByName(name, legacyName string) (*loadBalancer, error) { lb := &loadBalancer{ @@ -654,6 +620,50 @@ func (cs *CSCloud) getLoadBalancerByName(name, legacyName string) (*loadBalancer return lb, nil } +// getLoadBalancerByID retrieves load balancer rules by public IP ID and network ID. +// This is more reliable than keyword-based search as it uses exact ID matching. +func (cs *CSCloud) getLoadBalancerByID(name, ipAddrID, networkID string) (*loadBalancer, error) { + lb := &loadBalancer{ + CloudStackClient: cs.client, + name: name, + projectID: cs.projectID, + rules: make(map[string]*cloudstack.LoadBalancerRule), + } + + p := cs.client.LoadBalancer.NewListLoadBalancerRulesParams() + p.SetPublicipid(ipAddrID) + p.SetListall(true) + + if networkID != "" { + p.SetNetworkid(networkID) + } + + if cs.projectID != "" { + p.SetProjectid(cs.projectID) + } + + l, err := cs.client.LoadBalancer.ListLoadBalancerRules(p) + if err != nil { + return nil, fmt.Errorf("error retrieving load balancer rules by IP ID %v: %w", ipAddrID, err) + } + + for _, lbRule := range l.LoadBalancerRules { + lb.rules[lbRule.Name] = lbRule + + if lb.ipAddr != "" && lb.ipAddr != lbRule.Publicip { + klog.Warningf("Load balancer %v has rules associated with different IP's: %v, %v", lb.name, lb.ipAddr, lbRule.Publicip) + } + + lb.ipAddr = lbRule.Publicip + lb.ipAddrID = lbRule.Publicipid + lb.networkID = lbRule.Networkid + } + + klog.V(4).Infof("Load balancer %v (by ID %v) contains %d rule(s)", lb.name, ipAddrID, len(lb.rules)) + + return lb, nil +} + // verifyHosts verifies if all hosts belong to the same network, and returns the host ID's and network ID. // During rolling upgrades some nodes may not yet have a corresponding VM in CloudStack, so we tolerate // partial matches: as long as at least one node can be resolved we return the matched set and log @@ -782,31 +792,6 @@ func (lb *loadBalancer) getLoadBalancerIP(loadBalancerIP string) error { return lb.associatePublicIPAddress() } -// validatePublicIPAvailable checks that the given IP address exists in CloudStack -// without modifying any load balancer state. Used as a pre-flight check before -// tearing down an existing configuration. -func (lb *loadBalancer) validatePublicIPAvailable(ip string) error { - p := lb.Address.NewListPublicIpAddressesParams() - p.SetIpaddress(ip) - p.SetAllocatedonly(false) - p.SetListall(true) - - if lb.projectID != "" { - p.SetProjectid(lb.projectID) - } - - l, err := lb.Address.ListPublicIpAddresses(p) - if err != nil { - return fmt.Errorf("error looking up IP address %v: %w", ip, err) - } - - if l.Count != 1 { - return fmt.Errorf("IP address %v not found (got %d results)", ip, l.Count) - } - - return nil -} - // lookupPublicIPAddress checks whether the given IP address is already allocated in CloudStack. // If found and allocated, it sets lb.ipAddr and lb.ipAddrID and returns (true, nil). // If not found or not allocated, it returns (false, nil) without modifying lb state. @@ -1405,6 +1390,30 @@ func getBoolFromServiceAnnotation(service *corev1.Service, annotationKey string, return defaultSetting } +// getLoadBalancerAddress returns the desired load balancer IP address. +// It checks the ServiceAnnotationLoadBalancerAddress annotation first (preferred), +// then falls back to the deprecated spec.LoadBalancerIP field. +func getLoadBalancerAddress(service *corev1.Service) string { + if service == nil { + return "" + } + if addr := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, ""); addr != "" { + return addr + } + + return service.Spec.LoadBalancerIP //nolint:staticcheck // deprecated but kept as fallback +} + +// getLoadBalancerID returns the stored load balancer public IP UUID from the service annotation. +func getLoadBalancerID(service *corev1.Service) string { + return getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerID, "") +} + +// getLoadBalancerNetworkID returns the stored load balancer network UUID from the service annotation. +func getLoadBalancerNetworkID(service *corev1.Service) string { + return getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerNetworkID, "") +} + // setServiceAnnotation is used to create/set or update an annotation on the Service object. func setServiceAnnotation(service *corev1.Service, key, value string) { if service.ObjectMeta.Annotations == nil { diff --git a/cloudstack/cloudstack_loadbalancer_test.go b/cloudstack/cloudstack_loadbalancer_test.go index 50762e43..2ca9994a 100644 --- a/cloudstack/cloudstack_loadbalancer_test.go +++ b/cloudstack/cloudstack_loadbalancer_test.go @@ -3447,7 +3447,7 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) { } }) - t.Run("user-specified IP not released", func(t *testing.T) { + t.Run("keep-ip annotation prevents release", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3464,7 +3464,7 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) { {Id: "ip-user", Ipaddress: "10.0.0.1"}, }, }, nil) - // shouldReleaseLoadBalancerIP returns false because spec.LoadBalancerIP == lb.ipAddr + // shouldReleaseLoadBalancerIP returns false because keep-ip annotation is set cs := &CSCloud{ client: &cloudstack.CloudStackClient{ @@ -3479,11 +3479,9 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) { Namespace: "default", Annotations: map[string]string{ ServiceAnnotationLoadBalancerAddress: "10.0.0.1", + ServiceAnnotationLoadBalancerKeepIP: "true", }, }, - Spec: corev1.ServiceSpec{ - LoadBalancerIP: "10.0.0.1", // user-specified, matches annotation - }, } err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service) @@ -3601,7 +3599,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { } }) - t.Run("allocates new when annotated IP gone", func(t *testing.T) { + t.Run("auto-allocates when no IP specified", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3614,13 +3612,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { setupGetLoadBalancerByNameEmpty(mockLB) setupVerifyHosts(mockVM) - // lookupPublicIPAddress: NOT found (IP was already released) - mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(&cloudstack.ListPublicIpAddressesParams{}) - mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(&cloudstack.ListPublicIpAddressesResponse{ - Count: 0, PublicIpAddresses: []*cloudstack.PublicIpAddress{}, - }, nil) - - // Falls through to associatePublicIPAddress (new allocation) + // No annotation, no spec.LoadBalancerIP → auto-allocate via associatePublicIPAddress // GetNetworkByID called twice: once for associatePublicIPAddress, once for firewall check mockNetwork.EXPECT().GetNetworkByID("net-1", gomock.Any()).Return(&cloudstack.Network{ Id: "net-1", Service: []cloudstack.NetworkServiceInternal{{Name: "Firewall"}}, @@ -3650,9 +3642,6 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", - Annotations: map[string]string{ - ServiceAnnotationLoadBalancerAddress: "10.0.0.1", // stale IP, already gone - }, }, Spec: corev1.ServiceSpec{ Ports: []corev1.ServicePort{ @@ -3678,7 +3667,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { } }) - t.Run("IP switch retry releases old IP", func(t *testing.T) { + t.Run("annotation-specified IP is allocated on fresh LB", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3691,26 +3680,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { setupGetLoadBalancerByNameEmpty(mockLB) setupVerifyHosts(mockVM) - // lookupPublicIPAddress for old IP (10.0.0.1) - mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(&cloudstack.ListPublicIpAddressesParams{}) - mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(&cloudstack.ListPublicIpAddressesResponse{ - Count: 1, - PublicIpAddresses: []*cloudstack.PublicIpAddress{ - {Id: "ip-old", Ipaddress: "10.0.0.1"}, - }, - }, nil) - - // shouldReleaseLoadBalancerIP for old IP: no other rules - mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}) - mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ - Count: 0, LoadBalancerRules: []*cloudstack.LoadBalancerRule{}, - }, nil) - - // releaseLoadBalancerIP for old IP - mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-old").Return(&cloudstack.DisassociateIpAddressParams{}) - mockAddress.EXPECT().DisassociateIpAddress(gomock.Any()).Return(&cloudstack.DisassociateIpAddressResponse{}, nil) - - // getLoadBalancerIP for new IP (10.0.0.2) → getPublicIPAddress + // lookupPublicIPAddress for annotated IP: found (already allocated) mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(&cloudstack.ListPublicIpAddressesParams{}) mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(&cloudstack.ListPublicIpAddressesResponse{ Count: 1, @@ -3726,11 +3696,10 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - ServiceAnnotationLoadBalancerAddress: "10.0.0.1", // OLD IP from failed switch + ServiceAnnotationLoadBalancerAddress: "10.0.0.2", // user-specified desired IP }, }, Spec: corev1.ServiceSpec{ - LoadBalancerIP: "10.0.0.2", // NEW IP (spec differs from annotation → IP switch) Ports: []corev1.ServicePort{ {Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, }, @@ -3754,7 +3723,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { } }) - t.Run("old IP has other rules - not released during switch", func(t *testing.T) { + t.Run("spec.LoadBalancerIP fallback used when no annotation", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3767,27 +3736,7 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { setupGetLoadBalancerByNameEmpty(mockLB) setupVerifyHosts(mockVM) - // lookupPublicIPAddress for old IP - mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(&cloudstack.ListPublicIpAddressesParams{}) - mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(&cloudstack.ListPublicIpAddressesResponse{ - Count: 1, - PublicIpAddresses: []*cloudstack.PublicIpAddress{ - {Id: "ip-old", Ipaddress: "10.0.0.1"}, - }, - }, nil) - - // shouldReleaseLoadBalancerIP: has other rules → don't release - mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}) - mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ - Count: 1, - LoadBalancerRules: []*cloudstack.LoadBalancerRule{ - {Name: "other-svc-rule", Publicipid: "ip-old"}, - }, - }, nil) - - // No disassociate call expected! Old IP is preserved. - - // getLoadBalancerIP for new IP (10.0.0.2) + // getLoadBalancerIP for spec IP (10.0.0.2) → getPublicIPAddress mockAddress.EXPECT().NewListPublicIpAddressesParams().Return(&cloudstack.ListPublicIpAddressesParams{}) mockAddress.EXPECT().ListPublicIpAddresses(gomock.Any()).Return(&cloudstack.ListPublicIpAddressesResponse{ Count: 1, @@ -3802,12 +3751,10 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", - Annotations: map[string]string{ - ServiceAnnotationLoadBalancerAddress: "10.0.0.1", // OLD IP - }, + // No annotation — spec.LoadBalancerIP used as fallback }, Spec: corev1.ServiceSpec{ - LoadBalancerIP: "10.0.0.2", // NEW IP + LoadBalancerIP: "10.0.0.2", Ports: []corev1.ServicePort{ {Port: 80, NodePort: 30080, Protocol: corev1.ProtocolTCP}, }, @@ -3831,3 +3778,556 @@ func TestEnsureLoadBalancerAnnotationRecovery(t *testing.T) { } }) } + +func TestGetLoadBalancerAddress(t *testing.T) { + t.Run("nil service", func(t *testing.T) { + if got := getLoadBalancerAddress(nil); got != "" { + t.Errorf("getLoadBalancerAddress(nil) = %q, want empty", got) + } + }) + + t.Run("annotation takes precedence over spec", func(t *testing.T) { + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerAddress: "10.0.0.1", + }, + }, + Spec: corev1.ServiceSpec{ + LoadBalancerIP: "10.0.0.2", + }, + } + if got := getLoadBalancerAddress(service); got != "10.0.0.1" { + t.Errorf("getLoadBalancerAddress() = %q, want %q", got, "10.0.0.1") + } + }) + + t.Run("falls back to spec.LoadBalancerIP", func(t *testing.T) { + service := &corev1.Service{ + Spec: corev1.ServiceSpec{ + LoadBalancerIP: "10.0.0.2", + }, + } + if got := getLoadBalancerAddress(service); got != "10.0.0.2" { + t.Errorf("getLoadBalancerAddress() = %q, want %q", got, "10.0.0.2") + } + }) + + t.Run("both empty returns empty", func(t *testing.T) { + service := &corev1.Service{} + if got := getLoadBalancerAddress(service); got != "" { + t.Errorf("getLoadBalancerAddress() = %q, want empty", got) + } + }) +} + +func TestShouldReleaseLoadBalancerIPKeepIP(t *testing.T) { + t.Run("keep-ip true prevents release", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + cs := &CSCloud{} + lb := &loadBalancer{ + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + } + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerKeepIP: "true", + }, + }, + } + + release, err := cs.shouldReleaseLoadBalancerIP(lb, service) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if release { + t.Error("expected shouldReleaseLoadBalancerIP to return false when keep-ip is true") + } + }) + + t.Run("keep-ip false allows release", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + }, nil) + + cs := &CSCloud{} + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + } + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerKeepIP: "false", + }, + }, + } + + release, err := cs.shouldReleaseLoadBalancerIP(lb, service) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !release { + t.Error("expected shouldReleaseLoadBalancerIP to return true when keep-ip is false") + } + }) + + t.Run("keep-ip absent allows release", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + }, nil) + + cs := &CSCloud{} + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + } + service := &corev1.Service{} + + release, err := cs.shouldReleaseLoadBalancerIP(lb, service) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !release { + t.Error("expected shouldReleaseLoadBalancerIP to return true when keep-ip is absent") + } + }) + + t.Run("spec.LoadBalancerIP no longer prevents release", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + }, nil) + + cs := &CSCloud{} + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + } + service := &corev1.Service{ + Spec: corev1.ServiceSpec{ + LoadBalancerIP: "10.0.0.1", // previously this would have prevented release + }, + } + + release, err := cs.shouldReleaseLoadBalancerIP(lb, service) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !release { + t.Error("expected shouldReleaseLoadBalancerIP to return true; spec.LoadBalancerIP should no longer prevent release") + } + }) +} + +func TestGetLoadBalancerID(t *testing.T) { + t.Run("annotation present", func(t *testing.T) { + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerID: "ip-uuid-123", + }, + }, + } + if got := getLoadBalancerID(service); got != "ip-uuid-123" { + t.Errorf("getLoadBalancerID() = %q, want %q", got, "ip-uuid-123") + } + }) + + t.Run("annotation absent", func(t *testing.T) { + service := &corev1.Service{} + if got := getLoadBalancerID(service); got != "" { + t.Errorf("getLoadBalancerID() = %q, want empty string", got) + } + }) +} + +func TestGetLoadBalancerNetworkID(t *testing.T) { + t.Run("annotation present", func(t *testing.T) { + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerNetworkID: "net-uuid-456", + }, + }, + } + if got := getLoadBalancerNetworkID(service); got != "net-uuid-456" { + t.Errorf("getLoadBalancerNetworkID() = %q, want %q", got, "net-uuid-456") + } + }) + + t.Run("annotation absent", func(t *testing.T) { + service := &corev1.Service{} + if got := getLoadBalancerNetworkID(service); got != "" { + t.Errorf("getLoadBalancerNetworkID() = %q, want empty string", got) + } + }) +} + +func TestGetLoadBalancerByID(t *testing.T) { + t.Run("rules found by IP ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + 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"}, + }, + } + + 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, got %d", len(lb.rules)) + } + if lb.ipAddr != "1.2.3.4" { + t.Errorf("ipAddr = %q, want %q", lb.ipAddr, "1.2.3.4") + } + if lb.ipAddrID != "ip-1" { + t.Errorf("ipAddrID = %q, want %q", lb.ipAddrID, "ip-1") + } + if lb.networkID != "net-1" { + t.Errorf("networkID = %q, want %q", lb.networkID, "net-1") + } + }) + + t.Run("no rules found", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + listResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{}, + } + + 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) != 0 { + t.Fatalf("expected 0 rules, got %d", len(lb.rules)) + } + }) + + t.Run("API error propagated", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(nil, errors.New("API failure")) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + _, err := cs.getLoadBalancerByID("my-lb", "ip-1", "net-1") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "API failure") { + t.Errorf("error = %q, want it to contain %q", err.Error(), "API failure") + } + }) + + t.Run("with project ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + listResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{}, + } + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).DoAndReturn(func(p *cloudstack.ListLoadBalancerRulesParams) (*cloudstack.ListLoadBalancerRulesResponse, error) { + projectID, ok := p.GetProjectid() + if !ok || projectID != "proj-1" { + t.Errorf("expected projectid = %q, got %q (ok=%v)", "proj-1", projectID, ok) + } + + return listResp, nil + }) + + cs := &CSCloud{ + projectID: "proj-1", + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + _, err := cs.getLoadBalancerByID("my-lb", "ip-1", "net-1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("empty network ID omits SetNetworkid", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + listResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{}, + } + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).DoAndReturn(func(p *cloudstack.ListLoadBalancerRulesParams) (*cloudstack.ListLoadBalancerRulesResponse, error) { + _, ok := p.GetNetworkid() + if ok { + t.Error("expected networkid to not be set when empty string passed") + } + + return listResp, nil + }) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + _, err := cs.getLoadBalancerByID("my-lb", "ip-1", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) +} + +func TestGetLoadBalancerOrchestrator(t *testing.T) { + t.Run("ID annotation present and rules found - returns ID-based result", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + // ID-based lookup returns rules + idResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{ + {Name: "lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1", Networkid: "net-1"}, + }, + } + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(idResp, nil) + // Name-based lookup should NOT be called + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerID: "ip-1", + ServiceAnnotationLoadBalancerNetworkID: "net-1", + }, + }, + } + + lb, err := cs.getLoadBalancer(service, "my-lb", "legacy-lb") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(lb.rules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(lb.rules)) + } + if lb.ipAddr != "1.2.3.4" { + t.Errorf("ipAddr = %q, want %q", lb.ipAddr, "1.2.3.4") + } + }) + + t.Run("ID annotation present but no rules - falls back to name-based", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + + // First call: ID-based lookup → no rules + idParams := &cloudstack.ListLoadBalancerRulesParams{} + idResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 0, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{}, + } + + // Second call: name-based lookup → rules found + nameParams := &cloudstack.ListLoadBalancerRulesParams{} + nameResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{ + {Name: "my-lb-tcp-80", Publicip: "5.6.7.8", Publicipid: "ip-2"}, + }, + } + + gomock.InOrder( + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(idParams), + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(idResp, nil), + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(nameParams), + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(nameResp, nil), + ) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerID: "ip-stale", + ServiceAnnotationLoadBalancerNetworkID: "net-stale", + }, + }, + } + + lb, err := cs.getLoadBalancer(service, "my-lb", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(lb.rules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(lb.rules)) + } + if lb.ipAddr != "5.6.7.8" { + t.Errorf("ipAddr = %q, want %q", lb.ipAddr, "5.6.7.8") + } + }) + + t.Run("no ID annotation - goes directly to name-based", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + nameResp := &cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{ + {Name: "my-lb-tcp-80", Publicip: "1.2.3.4", Publicipid: "ip-1"}, + }, + } + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(nameResp, nil) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + service := &corev1.Service{} // no annotations + + lb, err := cs.getLoadBalancer(service, "my-lb", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(lb.rules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(lb.rules)) + } + if lb.ipAddr != "1.2.3.4" { + t.Errorf("ipAddr = %q, want %q", lb.ipAddr, "1.2.3.4") + } + }) + + t.Run("ID-based API error propagated without fallback", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + listParams := &cloudstack.ListLoadBalancerRulesParams{} + + mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(listParams) + mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(nil, errors.New("infra error")) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + } + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerID: "ip-1", + }, + }, + } + + _, err := cs.getLoadBalancer(service, "my-lb", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "infra error") { + t.Errorf("error = %q, want it to contain %q", err.Error(), "infra error") + } + }) +}