[occm] Tag load balancers with cluster identity to prevent name collisions

charts/openstack-cloud-controller-manager/Chart.yaml

@@ -4,7 +4,7 @@ description: Openstack Cloud Controller Manager Helm Chart
 icon: https://object-storage-ca-ymq-1.vexxhost.net/swift/v1/6e4619c416ff4bd19e1c087f27a43eea/www-images-prod/openstack-logo/OpenStack-Logo-Vertical.png
 home: https://github.com/kubernetes/cloud-provider-openstack
 name: openstack-cloud-controller-manager
-version: 2.36.0
+version: 2.36.1
 maintainers:
   - name: eumel8
     email: f.kloeker@telekom.de

charts/openstack-cloud-controller-manager/templates/clusterrole.yaml

@@ -96,3 +96,9 @@ rules:
   - list
   - get
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - namespaces
+  verbs:
+  - get

docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md

@@ -11,6 +11,7 @@
     - [Restrict Access For LoadBalancer Service](#restrict-access-for-loadbalancer-service)
     - [Use PROXY protocol to preserve client IP](#use-proxy-protocol-to-preserve-client-ip)
     - [Sharing load balancer with multiple Services](#sharing-load-balancer-with-multiple-services)
+    - [Running multiple Kubernetes clusters in one OpenStack project](#running-multiple-kubernetes-clusters-in-one-openstack-project)
     - [IPv4 / IPv6 dual-stack services](#ipv4--ipv6-dual-stack-services)
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
@@ -627,6 +628,55 @@ $ openstack loadbalancer listener list --loadbalancer 2b224530-9414-4302-8163-5a
 
 The load balancer will be deleted after `service-2` is deleted.
 
+### Running multiple Kubernetes clusters in one OpenStack project
+
+OCCM names each load balancer it creates as `kube_service_<cluster-name>_<namespace>_<service>`,
+where `<cluster-name>` comes from the kube-controller-manager `--cluster-name` flag and
+defaults to `kubernetes`. When two Kubernetes clusters share the same OpenStack project
+and use the same `--cluster-name`, namespace and service name, the resulting load balancer
+names collide. OpenStack does not require load balancer names to be unique, so previously
+the second cluster could "adopt" the load balancer that the first cluster owned and start
+overwriting its configuration. The recommended way to avoid this remains to set a unique
+`--cluster-name` on every Kubernetes cluster (see issues
+[#2241](https://github.com/kubernetes/cloud-provider-openstack/issues/2241),
+[#2571](https://github.com/kubernetes/cloud-provider-openstack/issues/2571),
+[#2624](https://github.com/kubernetes/cloud-provider-openstack/issues/2624)).
+
+To make OCCM resilient even when that recommendation isn't followed, OCCM also tags
+every load balancer it creates with the UID of the cluster's `kube-system` namespace,
+which is a stable and unique identifier per Kubernetes cluster. The tag has the form
+`kube_cluster_id_<uid>`. When OCCM looks up a load balancer by name on the first
+reconcile of a Service, it ignores load balancers that carry a `kube_cluster_id_*` tag
+for a *different* cluster and creates a new one instead. Load balancers that don't
+carry any `kube_cluster_id_*` tag (legacy load balancers, or load balancers created by
+external tooling) keep their previous behaviour for backward compatibility, and gain
+the cluster-id tag on the next reconcile.
+
+OCCM reads the `kube-system` namespace UID once at start-up; this requires the
+`get` verb on the `namespaces` resource (already part of the standard cloud-controller
+RBAC). If the lookup fails (for example because of a custom RBAC restriction) OCCM logs
+a warning and continues without the safeguard, falling back to the legacy name-based
+behaviour.
+
+After successful reconcile, the load balancer tags will look similar to:
+
+```shell
+$ openstack loadbalancer show <lb-id> -c name -c tags
++-------+--------------------------------------------------------------------------+
+| Field | Value                                                                    |
++-------+--------------------------------------------------------------------------+
+| name  | kube_service_kubernetes_default_service-1                                |
+| tags  | kube_service_kubernetes_default_service-1,                               |
+|       | kube_cluster_id_11111111-2222-3333-4444-555555555555                     |
++-------+--------------------------------------------------------------------------+
+```
+
+> NOTE: This safeguard only protects new (or freshly reconciled) load balancers. It
+> does not retroactively resolve an already-stolen load balancer between two running
+> clusters. If you suspect cross-cluster collisions, set unique `--cluster-name` on
+> each cluster and let OCCM rename the existing resources (see
+> [PR #2552](https://github.com/kubernetes/cloud-provider-openstack/pull/2552)).
+
 ### IPv4 / IPv6 dual-stack services
 Since Kubernetes 1.20, Kubernetes clusters can run in dual-stack mode,
 which allows simultaneous usage of both IPv4 and IPv6 addresses in the cluster.

manifests/controller-manager/cloud-controller-manager-roles.yaml

@@ -93,6 +93,12 @@ items:
     - list
     - get
     - watch
+  - apiGroups:
+    - ""
+    resources:
+    - namespaces
+    verbs:
+    - get
 - apiVersion: rbac.authorization.k8s.io/v1
   kind: ClusterRole
   metadata:

pkg/openstack/loadbalancer.go

@@ -105,8 +105,25 @@ const (
 	poolFormat     = poolPrefix + "%d_%s"
 	monitorPrefix  = "monitor_"
 	monitorFormat  = monitorPrefix + "%d_%s"
+
+	// clusterIDTagPrefix is the prefix used for the load balancer tag that
+	// carries a stable Kubernetes cluster identifier (the kube-system
+	// namespace UID). Together with the existing servicePrefix tag it allows
+	// OCCM to disambiguate load balancers when multiple Kubernetes clusters
+	// share the same OpenStack project and a service name happens to collide.
+	clusterIDTagPrefix = "kube_cluster_id_"
 )
 
+// clusterIDTag formats the Octavia load balancer tag carrying the cluster
+// identifier. It returns the empty string when uid is empty so callers can
+// safely append the result without a separate nil-check.
+func clusterIDTag(uid string) string {
+	if uid == "" {
+		return ""
+	}
+	return clusterIDTagPrefix + uid
+}
+
 // LbaasV2 is a LoadBalancer implementation based on Octavia
 type LbaasV2 struct {
 	LoadBalancer
@@ -153,8 +170,17 @@ type listenerKey struct {
 	Port     int
 }
 
-// getLoadbalancerByName get the load balancer which is in valid status by the given name/legacy name.
-func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClient, name string, legacyName string) (*loadbalancers.LoadBalancer, error) {
+// getLoadbalancerByName gets the load balancer which is in valid status by the given name/legacy name.
+//
+// When clusterUID is non-empty, the returned load balancer must either carry
+// the matching clusterIDTagPrefix tag for that UID, or carry no clusterIDTagPrefix
+// tag at all (legacy load balancer that pre-dates the tag). Load balancers
+// whose name matches but that carry a different cluster-id tag belong to
+// another Kubernetes cluster sharing the OpenStack project; they are ignored
+// and the lookup returns ErrNotFound, which causes OCCM to create a new load
+// balancer instead of accidentally adopting (and overwriting) one that is
+// owned by a different cluster.
+func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClient, name string, legacyName string, clusterUID string) (*loadbalancers.LoadBalancer, error) {
 	var validLBs []loadbalancers.LoadBalancer
 
 	opts := loadbalancers.ListOpts{
@@ -187,16 +213,71 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien
 		}
 	}
 
+	validLBs, foreignFound := filterLoadBalancersByClusterID(validLBs, clusterUID)
+
 	if len(validLBs) > 1 {
 		return nil, cpoerrors.ErrMultipleResults
 	}
 	if len(validLBs) == 0 {
+		if foreignFound {
+			klog.Warningf("Found a load balancer named %q in OpenStack but it belongs to a different Kubernetes cluster "+
+				"(no %s%s tag); ignoring it. A new load balancer will be created.", name, clusterIDTagPrefix, clusterUID)
+		}
 		return nil, cpoerrors.ErrNotFound
 	}
 
 	return &validLBs[0], nil
 }
 
+// filterLoadBalancersByClusterID returns the subset of lbs that may belong to
+// the cluster identified by clusterUID. The selection rules are:
+//
+//   - If clusterUID is empty, the filter is a no-op (the caller has no cluster
+//     identity to match on).
+//   - Load balancers carrying the matching clusterIDTagPrefix+clusterUID tag
+//     are kept (strongly owned by this cluster).
+//   - If none of the load balancers carries any clusterIDTagPrefix tag, all of
+//     them are kept. This preserves the legacy behaviour for load balancers
+//     created before the tag was introduced or by tools that don't set it.
+//   - Otherwise (every candidate carries a foreign clusterIDTagPrefix tag) all
+//     load balancers are dropped. The second return value is true in that case
+//     to let the caller emit a more specific log line / event.
+func filterLoadBalancersByClusterID(lbs []loadbalancers.LoadBalancer, clusterUID string) ([]loadbalancers.LoadBalancer, bool) {
+	if clusterUID == "" || len(lbs) == 0 {
+		return lbs, false
+	}
+	wantTag := clusterIDTag(clusterUID)
+	var owned []loadbalancers.LoadBalancer
+	taggedAny := false
+	for _, lb := range lbs {
+		hasTag := false
+		match := false
+		for _, tag := range lb.Tags {
+			if strings.HasPrefix(tag, clusterIDTagPrefix) {
+				hasTag = true
+				if tag == wantTag {
+					match = true
+				}
+			}
+		}
+		if match {
+			owned = append(owned, lb)
+		}
+		if hasTag {
+			taggedAny = true
+		}
+	}
+	if len(owned) > 0 {
+		return owned, false
+	}
+	if !taggedAny {
+		// Legacy load balancers without any cluster-id tag, behave as before.
+		return lbs, false
+	}
+	// All candidates are tagged for some other cluster.
+	return nil, true
+}
+
 func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener {
 	newListeners := []listeners.Listener{}
 	for _, existingListener := range existingListeners {
@@ -236,6 +317,9 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust
 
 	if svcConf.supportLBTags {
 		createOpts.Tags = []string{svcConf.lbName}
+		if tag := clusterIDTag(lbaas.clusterUID); tag != "" {
+			createOpts.Tags = append(createOpts.Tags, tag)
+		}
 	}
 
 	if svcConf.flavorID != "" {
@@ -339,7 +423,7 @@ func (lbaas *LbaasV2) GetLoadBalancer(ctx context.Context, clusterName string, s
 	if lbID != "" {
 		loadbalancer, err = openstackutil.GetLoadbalancerByID(ctx, lbaas.lb, lbID)
 	} else {
-		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, name, legacyName)
+		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, name, legacyName, lbaas.clusterUID)
 	}
 	if err != nil && cpoerrors.IsNotFound(err) {
 		return nil, false, nil
@@ -1737,7 +1821,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 		}
 	} else {
 		legacyName := lbaas.getLoadBalancerLegacyName(service)
-		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName)
+		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName, lbaas.clusterUID)
 		if err != nil {
 			if err != cpoerrors.ErrNotFound {
 				return nil, fmt.Errorf("error getting loadbalancer for Service %s: %v", serviceName, err)
@@ -1826,11 +1910,19 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 	// save address into the annotation
 	lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, addr)
 
-	// add LB name to load balancer tags.
+	// add LB name and cluster-identity tags to the load balancer.
 	if svcConf.supportLBTags {
 		lbTags := loadbalancer.Tags
+		changed := false
 		if !slices.Contains(lbTags, lbName) {
 			lbTags = append(lbTags, lbName)
+			changed = true
+		}
+		if tag := clusterIDTag(lbaas.clusterUID); tag != "" && !slices.Contains(lbTags, tag) {
+			lbTags = append(lbTags, tag)
+			changed = true
+		}
+		if changed {
 			klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", lbTags)
 			if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, lbTags); err != nil {
 				return nil, err
@@ -1912,7 +2004,7 @@ func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName
 		// This is a Service created before shared LB is supported.
 		name := lbaas.GetLoadBalancerName(ctx, clusterName, service)
 		legacyName := lbaas.getLoadBalancerLegacyName(service)
-		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, name, legacyName)
+		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, name, legacyName, lbaas.clusterUID)
 		if err != nil {
 			return err
 		}
@@ -2100,7 +2192,7 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName
 		loadbalancer, err = openstackutil.GetLoadbalancerByID(ctx, lbaas.lb, svcConf.lbID)
 	} else {
 		// This may happen when this Service creation was failed previously.
-		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName)
+		loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName, lbaas.clusterUID)
 	}
 	if err != nil && !cpoerrors.IsNotFound(err) {
 		return err