[occm] Skip LB rename for shared load balancers from other clusters

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).

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))
 			}
 		}
 

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

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))
+		})
+	}
+}

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))

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]))