[occm] Allow changing cluster-name on existing deployments

pkg/openstack/events.go

@@ -22,4 +22,5 @@ const (
 	eventLBSourceRangesIgnored         = "LoadBalancerSourceRangesIgnored"
 	eventLBAZIgnored                   = "LoadBalancerAvailabilityZonesIgnored"
 	eventLBFloatingIPSkipped           = "LoadBalancerFloatingIPSkipped"
+	eventLBRename                      = "LoadBalancerRename"
 )

pkg/openstack/loadbalancer.go

@@ -49,7 +49,6 @@ import (
 // Note: when creating a new Loadbalancer (VM), it can take some time before it is ready for use,
 // this timeout is used for waiting until the Loadbalancer provisioning status goes to ACTIVE state.
 const (
-	servicePrefix                       = "kube_service_"
 	defaultLoadBalancerSourceRangesIPv4 = "0.0.0.0/0"
 	defaultLoadBalancerSourceRangesIPv6 = "::/0"
 	activeStatus                        = "ACTIVE"
@@ -93,10 +92,14 @@ const (
 	ServiceAnnotationLoadBalancerID = "loadbalancer.openstack.org/load-balancer-id"
 
 	// Octavia resources name formats
+	servicePrefix  = "kube_service_"
 	lbFormat       = "%s%s_%s_%s"
-	listenerFormat = "listener_%d_%s"
-	poolFormat     = "pool_%d_%s"
-	monitorFormat  = "monitor_%d_%s"
+	listenerPrefix = "listener_"
+	listenerFormat = listenerPrefix + "%d_%s"
+	poolPrefix     = "pool_"
+	poolFormat     = poolPrefix + "%d_%s"
+	monitorPrefix  = "monitor_"
+	monitorFormat  = monitorPrefix + "%d_%s"
 )
 
 // LbaasV2 is a LoadBalancer implementation based on Octavia
@@ -1550,13 +1553,11 @@ func (lbaas *LbaasV2) checkListenerPorts(service *corev1.Service, curListenerMap
 	return nil
 }
 
-func (lbaas *LbaasV2) updateServiceAnnotations(service *corev1.Service, annotations map[string]string) {
+func (lbaas *LbaasV2) updateServiceAnnotation(service *corev1.Service, key, value string) {
 	if service.ObjectMeta.Annotations == nil {
 		service.ObjectMeta.Annotations = map[string]string{}
 	}
-	for key, value := range annotations {
-		service.ObjectMeta.Annotations[key] = value
-	}
+	service.ObjectMeta.Annotations[key] = value
 }
 
 // createLoadBalancerStatus creates the loadbalancer status from the different possible sources
@@ -1608,6 +1609,17 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 			return nil, fmt.Errorf("failed to get load balancer %s: %v", svcConf.lbID, err)
 		}
 
+		// Here we test for a clusterName that could have had changed in the deployment.
+		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(lbaas.lb, loadbalancer, lbName, clusterName)
+			if err != nil {
+				return nil, fmt.Errorf("failed to update load balancer %s with an updated name", svcConf.lbID)
+			}
+		}
+
 		// If this LB name matches the default generated name, the Service 'owns' the LB, but it's also possible for this
 		// LB to be shared by other Services.
 		// If the names don't match, this is a LB this Service wants to attach.
@@ -1656,6 +1668,9 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 		isLBOwner = true
 	}
 
+	// Make sure LB ID will be saved at this point.
+	lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerID, loadbalancer.ID)
+
 	if loadbalancer.ProvisioningStatus != activeStatus {
 		return nil, fmt.Errorf("load balancer %s is not ACTIVE, current provisioning status: %s", loadbalancer.ID, loadbalancer.ProvisioningStatus)
 	}
@@ -1722,12 +1737,11 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 			return nil, err
 		}
 	}
-	// Add annotation to Service and add LB name to load balancer tags.
-	annotationUpdate := map[string]string{
-		ServiceAnnotationLoadBalancerID:      loadbalancer.ID,
-		ServiceAnnotationLoadBalancerAddress: addr,
-	}
-	lbaas.updateServiceAnnotations(service, annotationUpdate)
+
+	// save address into the annotation
+	lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, addr)
+
+	// add LB name to load balancer tags.
 	if svcConf.supportLBTags {
 		lbTags := loadbalancer.Tags
 		if !cpoutil.Contains(lbTags, lbName) {

pkg/openstack/loadbalancer_rename.go

@@ -0,0 +1,149 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed 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 openstack
+
+import (
+	"fmt"
+	"k8s.io/cloud-provider-openstack/pkg/util"
+	"regexp"
+	"strings"
+
+	"github.com/gophercloud/gophercloud"
+
+	"github.com/gophercloud/gophercloud/openstack/loadbalancer/v2/listeners"
+	"github.com/gophercloud/gophercloud/openstack/loadbalancer/v2/loadbalancers"
+	"github.com/gophercloud/gophercloud/openstack/loadbalancer/v2/monitors"
+	"github.com/gophercloud/gophercloud/openstack/loadbalancer/v2/pools"
+	openstackutil "k8s.io/cloud-provider-openstack/pkg/util/openstack"
+)
+
+// lbHasOldClusterName checks if the OCCM LB prefix is present and if so, validates the cluster-name
+// component value. Returns true if the cluster-name component of the loadbalancer's name doesn't match
+// clusterName.
+func lbHasOldClusterName(loadbalancer *loadbalancers.LoadBalancer, clusterName string) bool {
+	if !strings.HasPrefix(loadbalancer.Name, servicePrefix) {
+		// This one was probably not created by OCCM, let's leave it as is.
+		return false
+	}
+	existingClusterName := getClusterName("", loadbalancer.Name)
+
+	return existingClusterName != clusterName
+}
+
+// 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
+	// it's highly unlikely, because it would mean cluster name, namespace and name would together need to exceed 200
+	// characters. As a precaution the _<name> part is treated as optional in the regexp, assuming the longest trim
+	// that can happen will reach namespace, but never the clusterName. This fails if there's _ in clusterName, but
+	// we can't do nothing about it.
+	lbNameRegex := regexp.MustCompile(fmt.Sprintf("%s%s(.+?)_[^_]+(_[^_]+)?$", resourcePrefix, servicePrefix)) // this is static
+
+	matches := lbNameRegex.FindAllStringSubmatch(objectName, -1)
+	if matches == nil {
+		return ""
+	}
+	return matches[0][1]
+}
+
+// replaceClusterName tries to cut servicePrefix, replaces clusterName and puts back the prefix if it was there
+func replaceClusterName(oldClusterName, clusterName, objectName string) string {
+	// Remove the prefix first
+	var found bool
+	objectName, found = strings.CutPrefix(objectName, servicePrefix)
+	objectName = strings.Replace(objectName, oldClusterName, clusterName, 1)
+	if found {
+		// This should always happen because we check that in lbHasOldClusterName, but just for safety.
+		objectName = servicePrefix + objectName
+	}
+	// We need to cut the name or tag to 255 characters, just as regular LB creation does.
+	return util.CutString255(objectName)
+}
+
+// renameLoadBalancer renames all the children and then the LB itself to match new lbName.
+// The purpose is handling a change of clusterName.
+func renameLoadBalancer(client *gophercloud.ServiceClient, loadbalancer *loadbalancers.LoadBalancer, lbName, clusterName string) (*loadbalancers.LoadBalancer, error) {
+	lbListeners, err := openstackutil.GetListenersByLoadBalancerID(client, loadbalancer.ID)
+	if err != nil {
+		return nil, err
+	}
+	for _, listener := range lbListeners {
+		if !strings.HasPrefix(listener.Name, listenerPrefix) {
+			// It doesn't seem to be ours, let's not touch it.
+			continue
+		}
+
+		oldClusterName := getClusterName(fmt.Sprintf("%s[0-9]+_", listenerPrefix), listener.Name)
+
+		if oldClusterName != clusterName {
+			// First let's handle pool which we assume is a child of the listener. Only one pool per one listener.
+			lbPool, err := openstackutil.GetPoolByListener(client, loadbalancer.ID, listener.ID)
+			if err != nil {
+				return nil, err
+			}
+			oldClusterName = getClusterName(fmt.Sprintf("%s[0-9]+_", poolPrefix), lbPool.Name)
+			if oldClusterName != clusterName {
+				if lbPool.MonitorID != "" {
+					// If monitor exists, let's handle it first, as we treat it as child of the pool.
+					monitor, err := openstackutil.GetHealthMonitor(client, lbPool.MonitorID)
+					if err != nil {
+						return nil, err
+					}
+					oldClusterName := getClusterName(fmt.Sprintf("%s[0-9]+_", monitorPrefix), monitor.Name)
+					if oldClusterName != clusterName {
+						monitor.Name = replaceClusterName(oldClusterName, clusterName, monitor.Name)
+						err = openstackutil.UpdateHealthMonitor(client, monitor.ID, monitors.UpdateOpts{Name: &monitor.Name}, loadbalancer.ID)
+						if err != nil {
+							return nil, err
+						}
+					}
+				}
+
+				// Monitor is handled, let's rename the pool.
+				lbPool.Name = replaceClusterName(oldClusterName, clusterName, lbPool.Name)
+				err = openstackutil.UpdatePool(client, loadbalancer.ID, lbPool.ID, pools.UpdateOpts{Name: &lbPool.Name})
+				if err != nil {
+					return nil, err
+				}
+			}
+
+			for i, tag := range listener.Tags {
+				// There might be tags for shared listeners, that's why we analyze each tag on its own.
+				oldClusterNameTag := getClusterName("", tag)
+				if oldClusterNameTag != "" && oldClusterNameTag != clusterName {
+					listener.Tags[i] = replaceClusterName(oldClusterNameTag, clusterName, tag)
+				}
+			}
+			listener.Name = replaceClusterName(oldClusterName, clusterName, listener.Name)
+			err = openstackutil.UpdateListener(client, loadbalancer.ID, listener.ID, listeners.UpdateOpts{Name: &listener.Name, Tags: &listener.Tags})
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	// At last we rename the LB. This is to make sure we only stop retrying to rename the LB once all of the children
+	// are handled.
+	for i, tag := range loadbalancer.Tags {
+		// There might be tags for shared lbs, that's why we analyze each tag on its own.
+		oldClusterNameTag := getClusterName("", tag)
+		if oldClusterNameTag != "" && oldClusterNameTag != clusterName {
+			loadbalancer.Tags[i] = replaceClusterName(oldClusterNameTag, clusterName, tag)
+		}
+	}
+	return openstackutil.UpdateLoadBalancer(client, loadbalancer.ID, loadbalancers.UpdateOpts{Name: &lbName, Tags: &loadbalancer.Tags})
+}

pkg/openstack/loadbalancer_rename_test.go

@@ -0,0 +1,127 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed 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 openstack
+
+import (
+	"k8s.io/cloud-provider-openstack/pkg/util"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReplaceClusterName(t *testing.T) {
+	tests := []struct {
+		name           string
+		oldClusterName string
+		clusterName    string
+		objectName     string
+		expected       string
+	}{
+		{
+			name:           "Simple kubernetes replace",
+			oldClusterName: "kubernetes",
+			clusterName:    "cluster123",
+			objectName:     "kube_service_kubernetes_namespace_name",
+			expected:       "kube_service_cluster123_namespace_name",
+		},
+		{
+			name:           "Simple kube replace",
+			oldClusterName: "kube",
+			clusterName:    "cluster123",
+			objectName:     "kube_service_kube_namespace_name",
+			expected:       "kube_service_cluster123_namespace_name",
+		},
+		{
+			name:           "Replace, no prefix",
+			oldClusterName: "kubernetes",
+			clusterName:    "cluster123",
+			objectName:     "foobar_kubernetes_namespace_name",
+			expected:       "foobar_cluster123_namespace_name",
+		},
+		{
+			name:           "Replace, not found",
+			oldClusterName: "foobar",
+			clusterName:    "cluster123",
+			objectName:     "kube_service_kubernetes_namespace_name",
+			expected:       "kube_service_kubernetes_namespace_name",
+		},
+		{
+			name:           "Replace, cut 255",
+			oldClusterName: "kubernetes",
+			clusterName:    "cluster123",
+			objectName:     "kube_service_kubernetes_namespace_name" + strings.Repeat("foo", 100),
+			expected: "kube_service_cluster123_namespace_namefoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo" +
+				"foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo" +
+				"foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoof",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := replaceClusterName(test.oldClusterName, test.clusterName, test.objectName)
+			assert.Equal(t, test.expected, result)
+		})
+	}
+}
+
+func TestDecomposeLBName(t *testing.T) {
+	tests := []struct {
+		name           string
+		resourcePrefix string
+		objectName     string
+		expected       string
+	}{
+		{
+			name:           "Simple kubernetes",
+			resourcePrefix: "",
+			objectName:     "kube_service_kubernetes_namespace_name",
+			expected:       "kubernetes",
+		},
+		{
+			name:           "Kubernetes with prefix",
+			resourcePrefix: "listener_",
+			objectName:     "listener_kube_service_kubernetes_namespace_name",
+			expected:       "kubernetes",
+		},
+		{
+			name:           "Example with _ in clusterName",
+			resourcePrefix: "listener_",
+			objectName:     "listener_kube_service_kubernetes_123_namespace_name",
+			expected:       "kubernetes_123",
+		},
+		{
+			name:           "No match",
+			resourcePrefix: "listener_",
+			objectName:     "FOOBAR",
+			expected:       "",
+		},
+		{
+			name:           "Looong namespace, so string is cut, but no _ in clusterName",
+			resourcePrefix: "listener_",
+			objectName:     util.CutString255("listener_kube_service_kubernetes_namespace" + strings.Repeat("foo", 100) + "_name"),
+			expected:       "kubernetes",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := getClusterName(test.resourcePrefix, test.objectName)
+			assert.Equal(t, test.expected, result)
+		})
+	}
+}

pkg/openstack/loadbalancer_test.go

@@ -1231,13 +1231,8 @@ func TestLbaasV2_updateServiceAnnotations(t *testing.T) {
 		},
 	}
 
-	annotations := map[string]string{
-		"key1": "value1",
-		"key2": "value2",
-	}
-
 	lbaas := LbaasV2{}
-	lbaas.updateServiceAnnotations(service, annotations)
+	lbaas.updateServiceAnnotation(service, "key1", "value1")
 
 	serviceAnnotations := make([]map[string]string, 0)
 	for key, value := range service.ObjectMeta.Annotations {
@@ -1246,7 +1241,6 @@ func TestLbaasV2_updateServiceAnnotations(t *testing.T) {
 
 	expectedAnnotations := []map[string]string{
 		{"key1": "value1"},
-		{"key2": "value2"},
 	}
 
 	assert.ElementsMatch(t, expectedAnnotations, serviceAnnotations)