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 c67ba1f1b4..cd21d1793d 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -1695,13 +1695,23 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName } // Here we test for a clusterName that could have had changed in the deployment. + // 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) { - 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) + 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) + 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) + } + } 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..3c3fa813f4 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..705eef4424 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) } @@ -150,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]))