From d223a02fa02192a6ac8d53d9ca1d3413e068fee0 Mon Sep 17 00:00:00 2001 From: 234u34k <234u34k@users.noreply.github.com> Date: Sun, 1 Mar 2026 16:46:10 +0000 Subject: [PATCH 1/3] occm: skip LB rename for shared load balancers from other clusters When a Service references a shared load balancer via annotation, the OCCM rename logic (added in #2552) sees a different cluster name in the LB name and renames it. This breaks multi-cluster shared LB setups because the owning cluster loses its LB name and tags. Before renaming, verify that the LB's namespace and service name components match the current Service. This distinguishes a legitimate cluster-name change (same service, new cluster name) from a cross-cluster shared LB access (different service entirely). --- pkg/openstack/loadbalancer.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index c67ba1f1b4..38a9aed302 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -1695,13 +1695,20 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName } // Here we test for a clusterName that could have had changed in the deployment. + // Only rename when the LB was originally created for this Service (i.e. the + // namespace and service name components match). Without this guard, a shared + // LB from a different cluster gets incorrectly renamed. if lbHasOldClusterName(loadbalancer, clusterName) { - msg := "Loadbalancer %s has a name of %s with incorrect cluster-name component. Renaming it to %s." - klog.Infof(msg, loadbalancer.ID, loadbalancer.Name, lbName) - lbaas.eventRecorder.Eventf(service, corev1.EventTypeWarning, eventLBRename, msg, loadbalancer.ID, loadbalancer.Name, lbName) - loadbalancer, err = renameLoadBalancer(ctx, lbaas.lb, loadbalancer, lbName, clusterName) - if err != nil { - return nil, fmt.Errorf("failed to update load balancer %s with an updated name: %w", svcConf.lbID, err) + oldClusterName := getClusterName("", loadbalancer.Name) + expectedOldName := cpoutil.Sprintf255(lbFormat, servicePrefix, oldClusterName, service.Namespace, service.Name) + if loadbalancer.Name == expectedOldName { + msg := "Loadbalancer %s has a name of %s with incorrect cluster-name component. Renaming it to %s." + klog.Infof(msg, loadbalancer.ID, loadbalancer.Name, lbName) + lbaas.eventRecorder.Eventf(service, corev1.EventTypeWarning, eventLBRename, msg, loadbalancer.ID, loadbalancer.Name, lbName) + loadbalancer, err = renameLoadBalancer(ctx, lbaas.lb, loadbalancer, lbName, clusterName) + if err != nil { + return nil, fmt.Errorf("failed to update load balancer %s with an updated name: %w", svcConf.lbID, err) + } } } From 935a8462d0f8b10c28455e8910770af97ee63a05 Mon Sep 17 00:00:00 2001 From: 234u34k <234u34k@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:30:46 -0700 Subject: [PATCH 2/3] occm: add enable-lb-rename option, cover rename guard with tests The name-based guard cannot distinguish a shared load balancer created by another cluster's Service with the same namespace and name from this cluster's own load balancer after a cluster-name change. Introduce an enable-lb-rename option (default true, keeping #2552 behavior) so deployments sharing load balancers across clusters can disable renaming altogether. Extract the ownership check into lbNameMatchesService, log at V(4) when a rename is skipped, document the new option, and add regression tests covering the #2682 scenarios. --- ...sing-openstack-cloud-controller-manager.md | 9 +++ pkg/openstack/loadbalancer.go | 15 ++-- pkg/openstack/loadbalancer_rename.go | 23 ++++++ pkg/openstack/loadbalancer_rename_test.go | 79 +++++++++++++++++++ pkg/openstack/openstack.go | 2 + pkg/openstack/openstack_test.go | 3 + 6 files changed, 125 insertions(+), 6 deletions(-) diff --git a/docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md b/docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md index cdb8663dcd..94c426213c 100644 --- a/docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md +++ b/docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md @@ -320,6 +320,15 @@ Although the openstack-cloud-controller-manager was initially implemented with N * `max-shared-lb` The maximum number of Services that share a load balancer. Default: 2 +* `enable-lb-rename` + When the cluster-name of a running cluster changes, the OCCM renames the load balancers it manages + (including their listeners, pools, health monitors and tags) so that their names follow the new + cluster-name. A load balancer is only renamed when its name matches the name generated for the + reconciled Service, which protects shared load balancers referenced from another cluster via the + `loadbalancer.openstack.org/load-balancer-id` annotation. This protection cannot work when the + other cluster's Service has the same namespace and name, so set this option to false to disable + renaming altogether when sharing load balancers across clusters. Default: true + * `provider-requires-serial-api-calls` Some Octavia providers do not support creating fully-populated loadbalancers using a single [API call](https://docs.openstack.org/api-ref/load-balancer/v2/?expanded=create-a-load-balancer-detail#creating-a-fully-populated-load-balancer). diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 38a9aed302..cd21d1793d 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -1695,13 +1695,13 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName } // Here we test for a clusterName that could have had changed in the deployment. - // Only rename when the LB was originally created for this Service (i.e. the - // namespace and service name components match). Without this guard, a shared - // LB from a different cluster gets incorrectly renamed. + // Rename only when the LB name proves it was created for this exact Service + // under a previous cluster-name (see lbNameMatchesService). Renaming a shared + // LB created by a Service from another cluster breaks the owning cluster, so + // when in doubt the rename is skipped. In setups sharing load balancers across + // clusters renaming can be disabled entirely with enable-lb-rename=false. if lbHasOldClusterName(loadbalancer, clusterName) { - oldClusterName := getClusterName("", loadbalancer.Name) - expectedOldName := cpoutil.Sprintf255(lbFormat, servicePrefix, oldClusterName, service.Namespace, service.Name) - if loadbalancer.Name == expectedOldName { + if lbaas.opts.EnableLBRename && lbNameMatchesService(loadbalancer, service) { msg := "Loadbalancer %s has a name of %s with incorrect cluster-name component. Renaming it to %s." klog.Infof(msg, loadbalancer.ID, loadbalancer.Name, lbName) lbaas.eventRecorder.Eventf(service, corev1.EventTypeWarning, eventLBRename, msg, loadbalancer.ID, loadbalancer.Name, lbName) @@ -1709,6 +1709,9 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName if err != nil { return nil, fmt.Errorf("failed to update load balancer %s with an updated name: %w", svcConf.lbID, err) } + } else { + klog.V(4).InfoS("Not renaming load balancer with a stale cluster-name component", + "lbID", loadbalancer.ID, "lbName", loadbalancer.Name, "service", klog.KObj(service)) } } diff --git a/pkg/openstack/loadbalancer_rename.go b/pkg/openstack/loadbalancer_rename.go index 8adda9a791..9a9c13ffa4 100644 --- a/pkg/openstack/loadbalancer_rename.go +++ b/pkg/openstack/loadbalancer_rename.go @@ -29,6 +29,7 @@ import ( "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers" "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors" "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools" + corev1 "k8s.io/api/core/v1" openstackutil "k8s.io/cloud-provider-openstack/pkg/util/openstack" ) @@ -45,6 +46,28 @@ func lbHasOldClusterName(loadbalancer *loadbalancers.LoadBalancer, clusterName s return existingClusterName != clusterName } +// lbNameMatchesService returns true when the load balancer's name is exactly the +// name OCCM would generate for this Service under the cluster-name component +// currently embedded in the load balancer's name. This is the only proof derivable +// from Octavia data that the load balancer was created for this Service and can +// therefore be renamed safely after a cluster-name change. Without this check a +// shared load balancer created by a Service from another cluster gets renamed, +// together with its tags and children, breaking the cluster that owns it. +// +// Note the inherent limitation: when another cluster runs a Service with the same +// namespace and name, its load balancer's name is indistinguishable from a former +// name of this cluster's own load balancer. Operators sharing load balancers +// between such clusters should disable renaming with enable-lb-rename=false. +func lbNameMatchesService(loadbalancer *loadbalancers.LoadBalancer, service *corev1.Service) bool { + oldClusterName := getClusterName("", loadbalancer.Name) + if oldClusterName == "" { + // The name cannot be decomposed, so ownership cannot be proven. + return false + } + expectedName := util.Sprintf255(lbFormat, servicePrefix, oldClusterName, service.Namespace, service.Name) + return loadbalancer.Name == expectedName +} + // decomposeLBName returns clusterName based on object name func getClusterName(resourcePrefix, objectName string) string { // This is not 100% bulletproof when string was cut because full name would exceed 255 characters, but honestly diff --git a/pkg/openstack/loadbalancer_rename_test.go b/pkg/openstack/loadbalancer_rename_test.go index 3e2a751fea..038a28936e 100644 --- a/pkg/openstack/loadbalancer_rename_test.go +++ b/pkg/openstack/loadbalancer_rename_test.go @@ -20,8 +20,11 @@ import ( "strings" "testing" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/cloud-provider-openstack/pkg/util" + "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers" "github.com/stretchr/testify/assert" ) @@ -126,3 +129,79 @@ func TestDecomposeLBName(t *testing.T) { }) } } + +func TestLbNameMatchesService(t *testing.T) { + tests := []struct { + name string + lbName string + namespace string + serviceName string + expected bool + }{ + { + // The case renaming was introduced for in #2552: the LB of this + // exact Service, carrying a stale cluster-name component. + name: "LB created for this Service under an old cluster-name", + lbName: "kube_service_oldcluster_kns_fakeservice", + namespace: "kns", + serviceName: "fakeservice", + expected: true, + }, + { + // #2682: a Service references a shared LB created by a Service + // with a different name in another cluster. + name: "shared LB created for a different Service name", + lbName: "kube_service_clustera_kns_fakeservice", + namespace: "kns", + serviceName: "otherservice", + expected: false, + }, + { + // #2682: same Service name, different namespace. + name: "shared LB created in a different namespace", + lbName: "kube_service_clustera_kns_fakeservice", + namespace: "otherns", + serviceName: "fakeservice", + expected: false, + }, + { + // #2682 with the same namespace and name in both clusters. From + // Octavia data alone this is indistinguishable from the legitimate + // rename above, hence true. Such setups need enable-lb-rename=false. + name: "shared LB from another cluster with identical namespace and name", + lbName: "kube_service_clustera_kns_fakeservice", + namespace: "kns", + serviceName: "fakeservice", + expected: true, + }, + { + name: "LB with a name not decomposable into components", + lbName: "kube_service_", + namespace: "kns", + serviceName: "fakeservice", + expected: false, + }, + { + // Long names are cut to 255 characters on creation, the comparison + // must reconstruct the cut name identically. + name: "255-cut LB name still matches its Service", + lbName: util.Sprintf255(lbFormat, servicePrefix, "oldcluster", "namespace"+strings.Repeat("foo", 100), "name"), + namespace: "namespace" + strings.Repeat("foo", 100), + serviceName: "name", + expected: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + lb := &loadbalancers.LoadBalancer{Name: test.lbName} + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: test.namespace, + Name: test.serviceName, + }, + } + assert.Equal(t, test.expected, lbNameMatchesService(lb, service)) + }) + } +} diff --git a/pkg/openstack/openstack.go b/pkg/openstack/openstack.go index ac07d50345..504139e1fe 100644 --- a/pkg/openstack/openstack.go +++ b/pkg/openstack/openstack.go @@ -118,6 +118,7 @@ type LoadBalancerOpts struct { EnableIngressHostname bool `gcfg:"enable-ingress-hostname"` // Used with proxy protocol by adding a dns suffix to the load balancer IP address. Default false. IngressHostnameSuffix string `gcfg:"ingress-hostname-suffix"` // Used with proxy protocol by adding a dns suffix to the load balancer IP address. Default nip.io. MaxSharedLB int `gcfg:"max-shared-lb"` // Number of Services in maximum can share a single load balancer. Default 2 + EnableLBRename bool `gcfg:"enable-lb-rename"` // if false, load balancers are never renamed after a cluster-name change. Default true ContainerStore string `gcfg:"container-store"` // Used to specify the store of the tls-container-ref ProviderRequiresSerialAPICalls bool `gcfg:"provider-requires-serial-api-calls"` // default false, the provider supports the "bulk update" API call // revive:disable:var-naming @@ -226,6 +227,7 @@ func ReadConfig(config io.Reader) (Config, error) { cfg.LoadBalancer.TlsContainerRef = "" cfg.LoadBalancer.ContainerStore = "barbican" cfg.LoadBalancer.MaxSharedLB = 2 + cfg.LoadBalancer.EnableLBRename = true cfg.LoadBalancer.ProviderRequiresSerialAPICalls = false err := gcfg.FatalOnly(gcfg.ReadInto(&cfg, config)) diff --git a/pkg/openstack/openstack_test.go b/pkg/openstack/openstack_test.go index 3291a8d0c8..04cc6280dc 100644 --- a/pkg/openstack/openstack_test.go +++ b/pkg/openstack/openstack_test.go @@ -133,6 +133,9 @@ func TestReadConfig(t *testing.T) { if !cfg.LoadBalancer.CreateMonitor { t.Errorf("incorrect lb.create-monitor: %t", cfg.LoadBalancer.CreateMonitor) } + if !cfg.LoadBalancer.EnableLBRename { + t.Errorf("incorrect lb.enable-lb-rename default: %t", cfg.LoadBalancer.EnableLBRename) + } if cfg.LoadBalancer.MonitorDelay.Duration != 1*time.Minute { t.Errorf("incorrect lb.monitor-delay: %s", cfg.LoadBalancer.MonitorDelay) } From 8fe627a0a21d98989e29e23b152865b6751cabc8 Mon Sep 17 00:00:00 2001 From: 234u34k <234u34k@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:04:42 -0700 Subject: [PATCH 3/3] Test enable-lb-rename=false behavior, capitalization fix --- pkg/openstack/openstack.go | 2 +- pkg/openstack/openstack_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/openstack/openstack.go b/pkg/openstack/openstack.go index 504139e1fe..3c3fa813f4 100644 --- a/pkg/openstack/openstack.go +++ b/pkg/openstack/openstack.go @@ -118,7 +118,7 @@ type LoadBalancerOpts struct { EnableIngressHostname bool `gcfg:"enable-ingress-hostname"` // Used with proxy protocol by adding a dns suffix to the load balancer IP address. Default false. IngressHostnameSuffix string `gcfg:"ingress-hostname-suffix"` // Used with proxy protocol by adding a dns suffix to the load balancer IP address. Default nip.io. MaxSharedLB int `gcfg:"max-shared-lb"` // Number of Services in maximum can share a single load balancer. Default 2 - EnableLBRename bool `gcfg:"enable-lb-rename"` // if false, load balancers are never renamed after a cluster-name change. Default true + EnableLBRename bool `gcfg:"enable-lb-rename"` // If false, load balancers are never renamed after a cluster-name change. Default true ContainerStore string `gcfg:"container-store"` // Used to specify the store of the tls-container-ref ProviderRequiresSerialAPICalls bool `gcfg:"provider-requires-serial-api-calls"` // default false, the provider supports the "bulk update" API call // revive:disable:var-naming diff --git a/pkg/openstack/openstack_test.go b/pkg/openstack/openstack_test.go index 04cc6280dc..705eef4424 100644 --- a/pkg/openstack/openstack_test.go +++ b/pkg/openstack/openstack_test.go @@ -153,6 +153,19 @@ func TestReadConfig(t *testing.T) { } } +func TestReadConfigDisableLBRename(t *testing.T) { + cfg, err := ReadConfig(strings.NewReader(` + [LoadBalancer] + enable-lb-rename = false + `)) + if err != nil { + t.Fatalf("Should succeed when enable-lb-rename is false: %v", err) + } + if cfg.LoadBalancer.EnableLBRename { + t.Error("incorrect lb.enable-lb-rename: true") + } +} + func TestReadClouds(t *testing.T) { dir, err := filepath.Abs(filepath.Dir(os.Args[0]))