diff --git a/charts/openstack-cloud-controller-manager/Chart.yaml b/charts/openstack-cloud-controller-manager/Chart.yaml index 49cee5ef22..cb52ebcfdb 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.0 +version: 2.36.1 maintainers: - name: eumel8 email: f.kloeker@telekom.de 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 a74a3b44b3..f6e0f0b4d5 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) @@ -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___`, +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 0a007e840a..62c3093f24 100644 --- a/pkg/openstack/loadbalancer.go +++ b/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 diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 27304a45ad..f4f46d4f55 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" @@ -2675,3 +2676,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..de3132f6b7 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 @@ -201,6 +208,26 @@ func (os *OpenStack) Initialize(clientBuilder cloudprovider.ControllerClientBuil os.eventRecorder = os.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "cloud-provider-openstack"}) } +// 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 func ReadConfig(config io.Reader) (Config, error) { if config == nil { @@ -372,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}}, 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 diff --git a/pkg/openstack/openstack_test.go b/pkg/openstack/openstack_test.go index 3291a8d0c8..de74bdcfd0 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.NewClientset(&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.NewClientset() + + 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.NewClientset() + 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) + } + }) +}