From 8a0a485ebdca2dd0ffce04cd7f15308d59308556 Mon Sep 17 00:00:00 2001 From: enginrect Date: Thu, 30 Apr 2026 09:51:43 +0900 Subject: [PATCH 1/7] [occm] Tag load balancers with cluster identity to prevent name collisions OCCM constructs Octavia load balancer names as kube_service___. When two Kubernetes clusters share the same OpenStack project and use the same --cluster-name (default "kubernetes"), services with identical namespace/name produce identical load balancer names. Octavia does not enforce uniqueness on load balancer names, so OCCM's first-time name-based lookup can adopt and overwrite a load balancer that actually belongs to a different cluster (see issues #2241, #2571, #2624). This commit adds a stable Kubernetes cluster identifier - the UID of the kube-system namespace - as a load balancer tag of the form kube_cluster_id_. getLoadbalancerByName now ignores load balancers that carry a cluster-id tag for a different cluster and falls back to the legacy behaviour for load balancers without any cluster-id tag, so existing deployments keep working unchanged. Pre-existing load balancers gain the new tag during the next reconciliation. The cluster UID is read once at controller-manager start-up via the kube-system namespace; failure to read it (RBAC denial, missing namespace) is non-fatal and disables the safeguard, falling back to legacy name-based lookup. The cloud-controller-manager ClusterRole and the helm chart gain "get" on namespaces. --- .../templates/clusterrole.yaml | 6 + ...cations-using-loadbalancer-type-service.md | 50 ++++++++ .../cloud-controller-manager-roles.yaml | 6 + pkg/openstack/loadbalancer.go | 108 +++++++++++++++- pkg/openstack/loadbalancer_test.go | 120 ++++++++++++++++++ pkg/openstack/openstack.go | 35 ++++- pkg/openstack/openstack_test.go | 48 +++++++ 7 files changed, 365 insertions(+), 8 deletions(-) diff --git a/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml b/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml index cf03f8a11a..307acaf614 100644 --- a/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml +++ b/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml @@ -96,3 +96,9 @@ rules: - list - get - watch +- apiGroups: + - "" + resources: + - namespaces + verbs: + - get diff --git a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md b/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md index bd9b72fe7a..621e304759 100644 --- a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md +++ b/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) @@ -641,6 +642,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___`, +where `` 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_`. 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 -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. diff --git a/manifests/controller-manager/cloud-controller-manager-roles.yaml b/manifests/controller-manager/cloud-controller-manager-roles.yaml index 93a47b74c2..e5c52d698b 100644 --- a/manifests/controller-manager/cloud-controller-manager-roles.yaml +++ b/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: diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 0c224a0a9a..db2c533875 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -112,8 +112,31 @@ 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_" + + // eventLBStolen is emitted when getLoadbalancerByName finds a load + // balancer with a matching name that belongs to a different Kubernetes + // cluster (different cluster-id tag). The lookup is treated as NotFound + // so OCCM creates a new load balancer instead of stealing an existing one. + eventLBStolen = "LoadBalancerNameCollision" ) +// 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 @@ -163,8 +186,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{ @@ -197,10 +229,16 @@ 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 } @@ -244,6 +282,55 @@ func withLBNameTag(lbName, annotation string) []string { return cpoutil.Unique(append([]string{lbName}, userTags...)) } +// 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 { @@ -283,6 +370,9 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust if svcConf.supportLBTags { createOpts.Tags = withLBNameTag(svcConf.lbName, svcConf.lbTags) + if tag := clusterIDTag(lbaas.clusterUID); tag != "" && !slices.Contains(createOpts.Tags, tag) { + createOpts.Tags = append(createOpts.Tags, tag) + } } if svcConf.flavorID != "" { @@ -386,7 +476,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 @@ -1805,7 +1895,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) @@ -1894,9 +1984,13 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // save address into the annotation lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, addr) - // Ensure the LB name tag plus any tags from the Service annotation in a single update. + // Ensure the LB name tag, the cluster-identity tag, plus any tags from the + // Service annotation in a single update. if svcConf.supportLBTags { lbTags := withLBNameTag(lbName, svcConf.lbTags) + if tag := clusterIDTag(lbaas.clusterUID); tag != "" && !slices.Contains(lbTags, tag) { + lbTags = append(lbTags, tag) + } klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) if tags, changed := cpoutil.Merge(loadbalancer.Tags, lbTags); changed { klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) @@ -1981,7 +2075,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 } @@ -2169,7 +2263,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 diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 45fbd16d28..db66b321a8 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -10,6 +10,7 @@ import ( "github.com/gophercloud/gophercloud/v2" "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners" + "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers" v2monitors "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors" "github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools" "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules" @@ -2820,3 +2821,122 @@ func Test_getProxyProtocolFromServiceAnnotation(t *testing.T) { }) } } + +func TestClusterIDTag(t *testing.T) { + assert.Equal(t, "", clusterIDTag("")) + assert.Equal(t, "kube_cluster_id_abc-123", clusterIDTag("abc-123")) + assert.True(t, len(clusterIDTagPrefix) > 0) +} + +func TestFilterLoadBalancersByClusterID(t *testing.T) { + const ( + thisUID = "11111111-2222-3333-4444-555555555555" + otherUID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + thisTag = clusterIDTagPrefix + thisUID + otherTag = clusterIDTagPrefix + otherUID + serviceTag = "kube_service_kubernetes_default_test" + ) + + mkLB := func(id string, tags ...string) loadbalancers.LoadBalancer { + return loadbalancers.LoadBalancer{ID: id, Tags: tags} + } + + tests := []struct { + name string + clusterUID string + input []loadbalancers.LoadBalancer + wantIDs []string + wantForeignFnd bool + }{ + { + name: "empty clusterUID is a no-op", + clusterUID: "", + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag), + mkLB("b", serviceTag, otherTag), + }, + wantIDs: []string{"a", "b"}, + wantForeignFnd: false, + }, + { + name: "empty input list", + clusterUID: thisUID, + input: nil, + wantIDs: nil, + wantForeignFnd: false, + }, + { + name: "single matching tag is kept", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag), + }, + wantIDs: []string{"a"}, + wantForeignFnd: false, + }, + { + name: "all untagged falls back to legacy behaviour", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag), + }, + wantIDs: []string{"a"}, + wantForeignFnd: false, + }, + { + name: "foreign cluster tag is filtered out", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, otherTag), + }, + wantIDs: nil, + wantForeignFnd: true, + }, + { + name: "mixed: matching kept, foreign dropped", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag), + mkLB("b", serviceTag, otherTag), + }, + wantIDs: []string{"a"}, + wantForeignFnd: false, + }, + { + name: "mixed: untagged ignored when matching exists", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag), + mkLB("b", serviceTag), + }, + wantIDs: []string{"a"}, + wantForeignFnd: false, + }, + { + name: "all foreign returns empty with foreignFound true", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, otherTag), + mkLB("b", serviceTag, otherTag), + }, + wantIDs: nil, + wantForeignFnd: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, foreign := filterLoadBalancersByClusterID(tt.input, tt.clusterUID) + gotIDs := make([]string, 0, len(got)) + for _, lb := range got { + gotIDs = append(gotIDs, lb.ID) + } + if len(tt.wantIDs) == 0 { + assert.Empty(t, gotIDs) + } else { + assert.Equal(t, tt.wantIDs, gotIDs) + } + assert.Equal(t, tt.wantForeignFnd, foreign) + }) + } +} diff --git a/pkg/openstack/openstack.go b/pkg/openstack/openstack.go index ac07d50345..726ec6b627 100644 --- a/pkg/openstack/openstack.go +++ b/pkg/openstack/openstack.go @@ -30,6 +30,7 @@ import ( neutronports "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports" "github.com/spf13/pflag" gcfg "gopkg.in/gcfg.v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" cloudprovider "k8s.io/cloud-provider" "k8s.io/klog/v2" @@ -88,6 +89,12 @@ type LoadBalancer struct { opts LoadBalancerOpts kclient kubernetes.Interface eventRecorder record.EventRecorder + // clusterUID is a stable, cluster-scoped identifier (the kube-system namespace UID). + // It is attached as a tag on Octavia load balancers so that OCCM can disambiguate + // load balancers with the same name when multiple Kubernetes clusters share the + // same OpenStack project. An empty value disables the disambiguation and falls + // back to the legacy name-based behaviour. + clusterUID string } // LoadBalancerOpts have the options to talk to Neutron LBaaSV2 or Octavia @@ -160,6 +167,11 @@ type OpenStack struct { kclient kubernetes.Interface nodeInformer coreinformers.NodeInformer nodeInformerHasSynced func() bool + // clusterUID is the kube-system namespace UID, used as a stable cluster + // identifier on OpenStack load balancer tags. May be empty if the lookup + // failed or RBAC does not allow it; in that case OCCM falls back to the + // legacy name-based load balancer identification. + clusterUID string eventBroadcaster record.EventBroadcaster eventRecorder record.EventRecorder @@ -199,6 +211,27 @@ func (os *OpenStack) Initialize(clientBuilder cloudprovider.ControllerClientBuil os.eventBroadcaster = record.NewBroadcaster() os.eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: os.kclient.CoreV1().Events("")}) os.eventRecorder = os.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "cloud-provider-openstack"}) + os.clusterUID = fetchClusterUID(clientset) +} + +// fetchClusterUID returns the UID of the kube-system namespace, which is a +// stable, unique identifier for the Kubernetes cluster. It is used as a tag on +// Octavia load balancers so OCCM can tell its own load balancers apart from +// load balancers created by a different cluster sharing the same OpenStack +// project even when the names collide. Failure is non-fatal: callers must +// tolerate an empty string and fall back to the legacy behaviour. +func fetchClusterUID(clientset kubernetes.Interface) string { + ctx, cancel := context.WithTimeout(context.Background(), defaultTimeOut) + defer cancel() + + ns, err := clientset.CoreV1().Namespaces().Get(ctx, "kube-system", metav1.GetOptions{}) + if err != nil { + klog.Warningf("Failed to read kube-system namespace UID, load balancer cluster-identity tagging will be disabled: %v", err) + return "" + } + uid := string(ns.UID) + klog.V(2).Infof("Using kube-system namespace UID %q as cluster identity for load balancer tagging", uid) + return uid } // ReadConfig reads values from the cloud.conf @@ -372,7 +405,7 @@ func (os *OpenStack) LoadBalancer() (cloudprovider.LoadBalancer, bool) { klog.V(1).Info("Claiming to support LoadBalancer") - return &LbaasV2{LoadBalancer{secret, network, lb, os.lbOpts, os.kclient, os.eventRecorder}}, true + return &LbaasV2{LoadBalancer{secret, network, lb, os.lbOpts, os.kclient, os.eventRecorder, os.clusterUID}}, true } // Zones indicates that we support zones diff --git a/pkg/openstack/openstack_test.go b/pkg/openstack/openstack_test.go index 3291a8d0c8..d506b85f50 100644 --- a/pkg/openstack/openstack_test.go +++ b/pkg/openstack/openstack_test.go @@ -18,6 +18,7 @@ package openstack import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -30,7 +31,13 @@ import ( neutronports "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports" "github.com/spf13/pflag" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" "k8s.io/cloud-provider-openstack/pkg/util/metadata" ) @@ -1159,3 +1166,44 @@ func testConfigFromEnv(t *testing.T, cfg *Config) { t.Skip("No config found in environment") } } + +func TestFetchClusterUID(t *testing.T) { + t.Run("returns kube-system UID", func(t *testing.T) { + const wantUID = "11111111-2222-3333-4444-555555555555" + client := fake.NewSimpleClientset(&v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kube-system", + UID: types.UID(wantUID), + }, + }) + + got := fetchClusterUID(client) + if got != wantUID { + t.Errorf("fetchClusterUID() = %q, want %q", got, wantUID) + } + }) + + t.Run("returns empty when namespace missing", func(t *testing.T) { + client := fake.NewSimpleClientset() + + got := fetchClusterUID(client) + if got != "" { + t.Errorf("fetchClusterUID() = %q, want empty string", got) + } + }) + + t.Run("returns empty on RBAC denial without crashing", func(t *testing.T) { + client := fake.NewSimpleClientset() + client.PrependReactor("get", "namespaces", func(_ k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden( + schema.GroupResource{Resource: "namespaces"}, "kube-system", + errors.New("not allowed"), + ) + }) + + got := fetchClusterUID(client) + if got != "" { + t.Errorf("fetchClusterUID() = %q, want empty string on forbidden error", got) + } + }) +} From cffd00255167f4ccad7973f410b6887be75c12ba Mon Sep 17 00:00:00 2001 From: enginrect Date: Thu, 14 May 2026 08:07:40 +0900 Subject: [PATCH 2/7] Address review feedback from kayrus - Remove duplicate clusterUID field from the OpenStack struct so the identifier lives only on the LoadBalancer struct that actually uses it (better cohesion). - Drop fetchClusterUID() out of Initialize() and call it lazily inside the LoadBalancer() factory instead. Clusters that disable LB now skip the kube-system namespace lookup entirely, and the change touches only the LB construction path. - Remove the unused eventLBStolen constant. It was a leftover from an earlier draft that intended to emit a Warning event from getLoadbalancerByName(); plumbing the eventRecorder + *v1.Service into that free-standing function felt out of scope, so the emission was dropped but the constant was left behind. --- pkg/openstack/loadbalancer.go | 6 ------ pkg/openstack/openstack.go | 9 ++------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index db2c533875..e0302708af 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -119,12 +119,6 @@ const ( // 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_" - - // eventLBStolen is emitted when getLoadbalancerByName finds a load - // balancer with a matching name that belongs to a different Kubernetes - // cluster (different cluster-id tag). The lookup is treated as NotFound - // so OCCM creates a new load balancer instead of stealing an existing one. - eventLBStolen = "LoadBalancerNameCollision" ) // clusterIDTag formats the Octavia load balancer tag carrying the cluster diff --git a/pkg/openstack/openstack.go b/pkg/openstack/openstack.go index 726ec6b627..de3132f6b7 100644 --- a/pkg/openstack/openstack.go +++ b/pkg/openstack/openstack.go @@ -167,11 +167,6 @@ type OpenStack struct { kclient kubernetes.Interface nodeInformer coreinformers.NodeInformer nodeInformerHasSynced func() bool - // clusterUID is the kube-system namespace UID, used as a stable cluster - // identifier on OpenStack load balancer tags. May be empty if the lookup - // failed or RBAC does not allow it; in that case OCCM falls back to the - // legacy name-based load balancer identification. - clusterUID string eventBroadcaster record.EventBroadcaster eventRecorder record.EventRecorder @@ -211,7 +206,6 @@ func (os *OpenStack) Initialize(clientBuilder cloudprovider.ControllerClientBuil os.eventBroadcaster = record.NewBroadcaster() os.eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: os.kclient.CoreV1().Events("")}) os.eventRecorder = os.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "cloud-provider-openstack"}) - os.clusterUID = fetchClusterUID(clientset) } // fetchClusterUID returns the UID of the kube-system namespace, which is a @@ -405,7 +399,8 @@ func (os *OpenStack) LoadBalancer() (cloudprovider.LoadBalancer, bool) { klog.V(1).Info("Claiming to support LoadBalancer") - return &LbaasV2{LoadBalancer{secret, network, lb, os.lbOpts, os.kclient, os.eventRecorder, os.clusterUID}}, true + clusterUID := fetchClusterUID(os.kclient) + return &LbaasV2{LoadBalancer{secret, network, lb, os.lbOpts, os.kclient, os.eventRecorder, clusterUID}}, true } // Zones indicates that we support zones From c49b65276244f2b2c4c30c775926b56956983663 Mon Sep 17 00:00:00 2001 From: enginrect Date: Thu, 14 May 2026 08:07:40 +0900 Subject: [PATCH 3/7] test: replace deprecated fake.NewSimpleClientset with fake.NewClientset The pull-cloud-provider-openstack-check prow job runs golangci-lint v2.3.1 with staticcheck enabled, which flags fake.NewSimpleClientset as SA1019 (deprecated). TestFetchClusterUID, added earlier in this PR, used the deprecated function. Swap it for fake.NewClientset; the signature is identical (objects ...runtime.Object) and the unit tests still pass. --- pkg/openstack/openstack_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/openstack/openstack_test.go b/pkg/openstack/openstack_test.go index d506b85f50..de74bdcfd0 100644 --- a/pkg/openstack/openstack_test.go +++ b/pkg/openstack/openstack_test.go @@ -1170,7 +1170,7 @@ func testConfigFromEnv(t *testing.T, cfg *Config) { func TestFetchClusterUID(t *testing.T) { t.Run("returns kube-system UID", func(t *testing.T) { const wantUID = "11111111-2222-3333-4444-555555555555" - client := fake.NewSimpleClientset(&v1.Namespace{ + client := fake.NewClientset(&v1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: "kube-system", UID: types.UID(wantUID), @@ -1184,7 +1184,7 @@ func TestFetchClusterUID(t *testing.T) { }) t.Run("returns empty when namespace missing", func(t *testing.T) { - client := fake.NewSimpleClientset() + client := fake.NewClientset() got := fetchClusterUID(client) if got != "" { @@ -1193,7 +1193,7 @@ func TestFetchClusterUID(t *testing.T) { }) t.Run("returns empty on RBAC denial without crashing", func(t *testing.T) { - client := fake.NewSimpleClientset() + client := fake.NewClientset() client.PrependReactor("get", "namespaces", func(_ k8stesting.Action) (bool, runtime.Object, error) { return true, nil, apierrors.NewForbidden( schema.GroupResource{Resource: "namespaces"}, "kube-system", From 25e35f2584ce5a900e246aa2c80c74fb46e0a2a6 Mon Sep 17 00:00:00 2001 From: enginrect Date: Thu, 30 Jul 2026 09:04:00 +0900 Subject: [PATCH 4/7] chart: bump openstack-cloud-controller-manager to 2.36.2 An earlier commit in this PR added a "get" on "namespaces" rule to the Helm chart ClusterRole template. chart-testing requires a version bump on any chart modification. Bumping the patch version since the change is additive and backward-compatible. --- charts/openstack-cloud-controller-manager/Chart.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/openstack-cloud-controller-manager/Chart.yaml b/charts/openstack-cloud-controller-manager/Chart.yaml index cb52ebcfdb..181a21b8dd 100644 --- a/charts/openstack-cloud-controller-manager/Chart.yaml +++ b/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.1 +version: 2.36.2 maintainers: - name: eumel8 email: f.kloeker@telekom.de From 1bc41189d6a6b3e79a8c322bc037ebcbc9980452 Mon Sep 17 00:00:00 2001 From: enginrect Date: Thu, 30 Jul 2026 17:07:24 +0900 Subject: [PATCH 5/7] Fold cluster-id tag guards into withClusterIDTag Review feedback from kayrus: the empty-UID and duplicate checks around clusterIDTag were repeated at both call sites (LB creation and tag reconciliation). Replace clusterIDTag with withClusterIDTag, which takes the tag list and handles those checks itself, following the naming of the existing withLBNameTag helper. filterLoadBalancersByClusterID now builds the expected tag directly from clusterIDTagPrefix since it already guards against an empty UID. --- pkg/openstack/loadbalancer.go | 29 ++++++++++++++--------------- pkg/openstack/loadbalancer_test.go | 8 +++++--- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index e0302708af..d9bfe50aec 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -121,14 +121,19 @@ const ( 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 { +// withClusterIDTag appends the Octavia load balancer tag carrying the cluster +// identifier to tags. It is a no-op when uid is empty (the cluster identity +// could not be determined) or when the tag is already present, so callers +// don't need any guard logic of their own. +func withClusterIDTag(tags []string, uid string) []string { if uid == "" { - return "" + return tags } - return clusterIDTagPrefix + uid + tag := clusterIDTagPrefix + uid + if slices.Contains(tags, tag) { + return tags + } + return append(tags, tag) } // LbaasV2 is a LoadBalancer implementation based on Octavia @@ -293,7 +298,7 @@ func filterLoadBalancersByClusterID(lbs []loadbalancers.LoadBalancer, clusterUID if clusterUID == "" || len(lbs) == 0 { return lbs, false } - wantTag := clusterIDTag(clusterUID) + wantTag := clusterIDTagPrefix + clusterUID var owned []loadbalancers.LoadBalancer taggedAny := false for _, lb := range lbs { @@ -363,10 +368,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - createOpts.Tags = withLBNameTag(svcConf.lbName, svcConf.lbTags) - if tag := clusterIDTag(lbaas.clusterUID); tag != "" && !slices.Contains(createOpts.Tags, tag) { - createOpts.Tags = append(createOpts.Tags, tag) - } + createOpts.Tags = withClusterIDTag(withLBNameTag(svcConf.lbName, svcConf.lbTags), lbaas.clusterUID) } if svcConf.flavorID != "" { @@ -1981,10 +1983,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // Ensure the LB name tag, the cluster-identity tag, plus any tags from the // Service annotation in a single update. if svcConf.supportLBTags { - lbTags := withLBNameTag(lbName, svcConf.lbTags) - if tag := clusterIDTag(lbaas.clusterUID); tag != "" && !slices.Contains(lbTags, tag) { - lbTags = append(lbTags, tag) - } + lbTags := withClusterIDTag(withLBNameTag(lbName, svcConf.lbTags), lbaas.clusterUID) klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) if tags, changed := cpoutil.Merge(loadbalancer.Tags, lbTags); changed { klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index db66b321a8..296d01a86d 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -2822,9 +2822,11 @@ func Test_getProxyProtocolFromServiceAnnotation(t *testing.T) { } } -func TestClusterIDTag(t *testing.T) { - assert.Equal(t, "", clusterIDTag("")) - assert.Equal(t, "kube_cluster_id_abc-123", clusterIDTag("abc-123")) +func TestWithClusterIDTag(t *testing.T) { + assert.Equal(t, []string{"foo"}, withClusterIDTag([]string{"foo"}, "")) + assert.Equal(t, []string{"foo", "kube_cluster_id_abc-123"}, withClusterIDTag([]string{"foo"}, "abc-123")) + // already present, no duplicate + assert.Equal(t, []string{"kube_cluster_id_abc-123"}, withClusterIDTag([]string{"kube_cluster_id_abc-123"}, "abc-123")) assert.True(t, len(clusterIDTagPrefix) > 0) } From b7c487e89eaf9b156f32be557aae40aa66d2a320 Mon Sep 17 00:00:00 2001 From: enginrect Date: Fri, 31 Jul 2026 08:34:51 +0900 Subject: [PATCH 6/7] Address second review round from kayrus - Move withClusterIDTag next to withLBNameTag and take the uid as the first argument, since the uid is the primary input. - Convert getLoadbalancerByName into an LbaasV2 method so callers no longer need to pass the Octavia client and cluster UID around, and move it next to the other load balancer methods. - Document why the cluster-id tag is filtered client-side rather than through ListOpts tag filtering: legacy load balancers without the tag must still be found during the transition period, and foreign-tagged load balancers have to be seen to log the warning. - Treat clusterIDTagPrefix as reserved in stripReservedTags so user tags from the annotation cannot enter the cluster-identity namespace, with a test case for it. --- pkg/openstack/loadbalancer.go | 201 +++++++++++++++-------------- pkg/openstack/loadbalancer_test.go | 11 +- 2 files changed, 113 insertions(+), 99 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index d9bfe50aec..711f997f8b 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -121,21 +121,6 @@ const ( clusterIDTagPrefix = "kube_cluster_id_" ) -// withClusterIDTag appends the Octavia load balancer tag carrying the cluster -// identifier to tags. It is a no-op when uid is empty (the cluster identity -// could not be determined) or when the tag is already present, so callers -// don't need any guard logic of their own. -func withClusterIDTag(tags []string, uid string) []string { - if uid == "" { - return tags - } - tag := clusterIDTagPrefix + uid - if slices.Contains(tags, tag) { - return tags - } - return append(tags, tag) -} - // LbaasV2 is a LoadBalancer implementation based on Octavia type LbaasV2 struct { LoadBalancer @@ -185,86 +170,31 @@ type listenerKey struct { Port int } -// 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{ - Name: name, - } - allLoadbalancers, err := openstackutil.GetLoadBalancers(ctx, client, opts) - if err != nil { - return nil, err - } - - if len(allLoadbalancers) == 0 { - if len(legacyName) > 0 { - // Backoff to get load balnacer by legacy name. - opts := loadbalancers.ListOpts{ - Name: legacyName, - } - allLoadbalancers, err = openstackutil.GetLoadBalancers(ctx, client, opts) - if err != nil { - return nil, err - } - } else { - return nil, cpoerrors.ErrNotFound - } - } - - for _, lb := range allLoadbalancers { - // All the ProvisioningStatus could be found here https://developer.openstack.org/api-ref/load-balancer/v2/index.html#provisioning-status-codes - if lb.ProvisioningStatus != "DELETED" && lb.ProvisioningStatus != "PENDING_DELETE" { - validLBs = append(validLBs, lb) - } - } - - 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 -} - -// stripReservedTags drops any user-supplied tag that begins with servicePrefix. +// stripReservedTags drops any user-supplied tag that begins with servicePrefix +// or clusterIDTagPrefix. // -// Tags with this prefix are how OCCM marks resource ownership: the ownership tag -// written to a resource is always a GetLoadBalancerName value, which always -// starts with servicePrefix. Every ownership check in this file matches tags -// exclusively on that form -- either an exact match against the prefixed lbName -// (e.g. slices.Contains(tags, lbName)) or a strings.HasPrefix(tag, servicePrefix) -// scan (used for shared-LB counting and deletion decisions). +// Tags with these prefixes are how OCCM marks resource ownership: the ownership +// tag written to a resource is always a GetLoadBalancerName value, which always +// starts with servicePrefix, and the cluster-identity tag always starts with +// clusterIDTagPrefix. Every ownership check in this file matches tags +// exclusively on those forms -- an exact match against the prefixed lbName +// (e.g. slices.Contains(tags, lbName)), a strings.HasPrefix(tag, servicePrefix) +// scan (used for shared-LB counting and deletion decisions), or the +// clusterIDTagPrefix matching in filterLoadBalancersByClusterID. // -// Because of that, keeping user tags out of the servicePrefix subspace fully -// partitions the tag namespace: ownership tags live in the kube_service_* space, -// user tags live in its complement, and no user tag can ever satisfy an ownership -// check. This prevents a Service from injecting a tag matching another (possibly -// cross-tenant) load balancer's name to hijack ownership, shared-LB limits, or -// deletion. INVARIANT: any new tag-based ownership check must also key on -// servicePrefix, or this guarantee no longer holds. +// Because of that, keeping user tags out of these subspaces fully partitions +// the tag namespace: ownership tags live in the kube_service_* and +// kube_cluster_id_* spaces, user tags live in their complement, and no user tag +// can ever satisfy an ownership check. This prevents a Service from injecting a +// tag matching another (possibly cross-tenant) load balancer's name to hijack +// ownership, shared-LB limits, deletion, or cluster identity. INVARIANT: any +// new tag-based ownership check must also key on one of these prefixes, or +// this guarantee no longer holds. func stripReservedTags(tags []string) []string { result := make([]string, 0, len(tags)) for _, t := range tags { - if strings.HasPrefix(t, servicePrefix) { - klog.Warningf("Ignoring reserved tag %q: tags starting with %q are reserved for OCCM ownership tracking", t, servicePrefix) + if strings.HasPrefix(t, servicePrefix) || strings.HasPrefix(t, clusterIDTagPrefix) { + klog.Warningf("Ignoring reserved tag %q: tags starting with %q or %q are reserved for OCCM ownership tracking", t, servicePrefix, clusterIDTagPrefix) continue } result = append(result, t) @@ -281,6 +211,21 @@ func withLBNameTag(lbName, annotation string) []string { return cpoutil.Unique(append([]string{lbName}, userTags...)) } +// withClusterIDTag appends the Octavia load balancer tag carrying the cluster +// identifier for uid to tags. It is a no-op when uid is empty (the cluster +// identity could not be determined) or when the tag is already present, so +// callers don't need any guard logic of their own. +func withClusterIDTag(uid string, tags []string) []string { + if uid == "" { + return tags + } + tag := clusterIDTagPrefix + uid + if slices.Contains(tags, tag) { + return tags + } + return append(tags, tag) +} + // filterLoadBalancersByClusterID returns the subset of lbs that may belong to // the cluster identified by clusterUID. The selection rules are: // @@ -368,7 +313,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - createOpts.Tags = withClusterIDTag(withLBNameTag(svcConf.lbName, svcConf.lbTags), lbaas.clusterUID) + createOpts.Tags = withClusterIDTag(lbaas.clusterUID, withLBNameTag(svcConf.lbName, svcConf.lbTags)) } if svcConf.flavorID != "" { @@ -472,7 +417,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, lbaas.clusterUID) + loadbalancer, err = lbaas.getLoadbalancerByName(ctx, name, legacyName) } if err != nil && cpoerrors.IsNotFound(err) { return nil, false, nil @@ -508,6 +453,70 @@ func (lbaas *LbaasV2) getLoadBalancerLegacyName(service *corev1.Service) string return cloudprovider.DefaultLoadBalancerName(service) } +// getLoadbalancerByName gets the load balancer which is in valid status by the given name/legacy name. +// +// When lbaas.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 (lbaas *LbaasV2) getLoadbalancerByName(ctx context.Context, name string, legacyName string) (*loadbalancers.LoadBalancer, error) { + var validLBs []loadbalancers.LoadBalancer + + // The cluster-id tag is deliberately not part of the ListOpts (server-side + // Tags filtering): legacy load balancers created before the tag existed + // carry no cluster-id tag at all and must still be found during the + // transition period, and a foreign-tagged load balancer has to be seen + // here to log the warning below instead of being silently invisible. + opts := loadbalancers.ListOpts{ + Name: name, + } + allLoadbalancers, err := openstackutil.GetLoadBalancers(ctx, lbaas.lb, opts) + if err != nil { + return nil, err + } + + if len(allLoadbalancers) == 0 { + if len(legacyName) > 0 { + // Backoff to get load balnacer by legacy name. + opts := loadbalancers.ListOpts{ + Name: legacyName, + } + allLoadbalancers, err = openstackutil.GetLoadBalancers(ctx, lbaas.lb, opts) + if err != nil { + return nil, err + } + } else { + return nil, cpoerrors.ErrNotFound + } + } + + for _, lb := range allLoadbalancers { + // All the ProvisioningStatus could be found here https://developer.openstack.org/api-ref/load-balancer/v2/index.html#provisioning-status-codes + if lb.ProvisioningStatus != "DELETED" && lb.ProvisioningStatus != "PENDING_DELETE" { + validLBs = append(validLBs, lb) + } + } + + validLBs, foreignFound := filterLoadBalancersByClusterID(validLBs, lbaas.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, lbaas.clusterUID) + } + return nil, cpoerrors.ErrNotFound + } + + return &validLBs[0], nil +} + // The LB needs to be configured with instance addresses on the same // subnet as the LB (aka opts.SubnetID). Currently, we're just // guessing that the node's InternalIP is the right address. @@ -1891,7 +1900,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName } } else { legacyName := lbaas.getLoadBalancerLegacyName(service) - loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName, lbaas.clusterUID) + loadbalancer, err = lbaas.getLoadbalancerByName(ctx, lbName, legacyName) if err != nil { if err != cpoerrors.ErrNotFound { return nil, fmt.Errorf("error getting loadbalancer for Service %s: %v", serviceName, err) @@ -1983,7 +1992,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // Ensure the LB name tag, the cluster-identity tag, plus any tags from the // Service annotation in a single update. if svcConf.supportLBTags { - lbTags := withClusterIDTag(withLBNameTag(lbName, svcConf.lbTags), lbaas.clusterUID) + lbTags := withClusterIDTag(lbaas.clusterUID, withLBNameTag(lbName, svcConf.lbTags)) klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) if tags, changed := cpoutil.Merge(loadbalancer.Tags, lbTags); changed { klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) @@ -2068,7 +2077,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, lbaas.clusterUID) + loadbalancer, err = lbaas.getLoadbalancerByName(ctx, name, legacyName) if err != nil { return err } @@ -2256,7 +2265,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, lbaas.clusterUID) + loadbalancer, err = lbaas.getLoadbalancerByName(ctx, lbName, legacyName) } if err != nil && !cpoerrors.IsNotFound(err) { return err diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 296d01a86d..2a3403dc5b 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -210,6 +210,11 @@ func TestWithLBNameTag(t *testing.T) { annotation: servicePrefix + "a," + servicePrefix + "b", expected: []string{lbName}, }, + { + name: "user tag using the cluster-id prefix is stripped", + annotation: clusterIDTagPrefix + "some-other-cluster,team=foo", + expected: []string{lbName, "team=foo"}, + }, } for _, tc := range testCases { @@ -2823,10 +2828,10 @@ func Test_getProxyProtocolFromServiceAnnotation(t *testing.T) { } func TestWithClusterIDTag(t *testing.T) { - assert.Equal(t, []string{"foo"}, withClusterIDTag([]string{"foo"}, "")) - assert.Equal(t, []string{"foo", "kube_cluster_id_abc-123"}, withClusterIDTag([]string{"foo"}, "abc-123")) + assert.Equal(t, []string{"foo"}, withClusterIDTag("", []string{"foo"})) + assert.Equal(t, []string{"foo", "kube_cluster_id_abc-123"}, withClusterIDTag("abc-123", []string{"foo"})) // already present, no duplicate - assert.Equal(t, []string{"kube_cluster_id_abc-123"}, withClusterIDTag([]string{"kube_cluster_id_abc-123"}, "abc-123")) + assert.Equal(t, []string{"kube_cluster_id_abc-123"}, withClusterIDTag("abc-123", []string{"kube_cluster_id_abc-123"})) assert.True(t, len(clusterIDTagPrefix) > 0) } From 2e2348c7597339e39c8b8c9620a8457594fe4037 Mon Sep 17 00:00:00 2001 From: enginrect Date: Fri, 31 Jul 2026 09:00:47 +0900 Subject: [PATCH 7/7] docs: drop issue links from the multi-cluster section Documentation should stand alone without references to resolved issues. --- .../expose-applications-using-loadbalancer-type-service.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md b/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md index 621e304759..0ab6d852fa 100644 --- a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md +++ b/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md @@ -651,10 +651,7 @@ and use the same `--cluster-name`, namespace and service name, the resulting loa 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)). +`--cluster-name` on every Kubernetes cluster. 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,