diff --git a/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml b/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml index 3c050dfd27..d5e300449e 100644 --- a/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml +++ b/charts/openstack-cloud-controller-manager/templates/clusterrole.yaml @@ -86,3 +86,9 @@ rules: - get - list - 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..3e70792fed 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,58 @@ $ 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. + +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. + +A load balancer that carries this cluster's `kube_cluster_id_*` tag *and* a tag of +another cluster at the same time is ambiguous: OCCM cannot tell which cluster owns it, +so it neither adopts nor deletes it. The reconcile fails with an error and OCCM records +a `LoadBalancerClusterIDConflict` Warning event on the Service. Remove the stale +`kube_cluster_id_*` tags from the load balancer to resolve it. + +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 ce1d0b37de..97da7dd5a2 100644 --- a/manifests/controller-manager/cloud-controller-manager-roles.yaml +++ b/manifests/controller-manager/cloud-controller-manager-roles.yaml @@ -99,6 +99,12 @@ items: - get - list - watch + - apiGroups: + - "" + resources: + - namespaces + verbs: + - get - apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: diff --git a/pkg/openstack/events.go b/pkg/openstack/events.go index 8996cafd2f..9ec47967eb 100644 --- a/pkg/openstack/events.go +++ b/pkg/openstack/events.go @@ -24,4 +24,5 @@ const ( eventLBFloatingIPSkipped = "LoadBalancerFloatingIPSkipped" eventLBRename = "LoadBalancerRename" eventLBLbMethodUnknown = "LoadBalancerLbMethodUnknown" + eventLBClusterIDConflict = "LoadBalancerClusterIDConflict" ) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 0c224a0a9a..106af8513f 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -112,6 +112,13 @@ 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_" ) // LbaasV2 is a LoadBalancer implementation based on Octavia @@ -163,71 +170,31 @@ 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) { - 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) - } - } - - if len(validLBs) > 1 { - return nil, cpoerrors.ErrMultipleResults - } - if len(validLBs) == 0 { - 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) @@ -244,6 +211,103 @@ 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) +} + +// clusterIDTags returns the cluster-identity tags carried by lb, i.e. the tags +// starting with clusterIDTagPrefix. A load balancer managed by a single cluster +// carries exactly one of them. +func clusterIDTags(lb loadbalancers.LoadBalancer) []string { + var tags []string + for _, tag := range lb.Tags { + if strings.HasPrefix(tag, clusterIDTagPrefix) { + tags = append(tags, tag) + } + } + return tags +} + +// 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 +// and no other cluster-id tag are kept (strongly owned by this cluster). +// - A load balancer carrying the matching tag together with a foreign +// cluster-id tag is ambiguous: it claims to belong to this cluster and to +// another one at the same time. It is reported through the second return +// value so the caller can refuse to touch it rather than pick a winner. +// - 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 only foreign clusterIDTagPrefix tags) +// all load balancers are dropped. The third return value is true in that +// case to let the caller emit a more specific log line / event. +// +// Several cluster-id tags none of which matches clusterUID make a load balancer +// foreign, not ambiguous: this cluster has no claim to it, so it is dropped +// like any other foreign load balancer and a new one is created instead. +func filterLoadBalancersByClusterID(lbs []loadbalancers.LoadBalancer, clusterUID string) ([]loadbalancers.LoadBalancer, *loadbalancers.LoadBalancer, bool) { + if clusterUID == "" || len(lbs) == 0 { + return lbs, nil, false + } + wantTag := clusterIDTagPrefix + clusterUID + var owned []loadbalancers.LoadBalancer + taggedAny := false + for i := range lbs { + tags := clusterIDTags(lbs[i]) + if len(tags) == 0 { + continue + } + taggedAny = true + if !slices.Contains(tags, wantTag) { + continue + } + if len(tags) > 1 { + return nil, &lbs[i], false + } + owned = append(owned, lbs[i]) + } + if len(owned) > 0 { + return owned, nil, false + } + if !taggedAny { + // Legacy load balancers without any cluster-id tag, behave as before. + return lbs, nil, false + } + // All candidates are tagged for some other cluster. + return nil, nil, true +} + +// clusterIDConflictError reports a load balancer that carries cluster-identity +// tags of more than one Kubernetes cluster. OCCM cannot tell which cluster owns +// it, so it must neither adopt nor delete it: acting on it would let anyone able +// to tag a load balancer in the same OpenStack project take over, or destroy, +// another cluster's Service. The condition is raised as a Warning event on the +// Service and as an error so the reconciliation fails visibly instead of +// silently creating a duplicate load balancer. +func (lbaas *LbaasV2) clusterIDConflictError(service *corev1.Service, lb *loadbalancers.LoadBalancer) error { + const msg = "load balancer %s carries the cluster-identity tags of more than one Kubernetes cluster (%s) and cannot be used for Service %s; remove the stale %s* tags from the load balancer" + tags := strings.Join(clusterIDTags(*lb), ", ") + serviceName := fmt.Sprintf("%s/%s", service.Namespace, service.Name) + lbaas.eventRecorder.Eventf(service, corev1.EventTypeWarning, eventLBClusterIDConflict, msg, lb.ID, tags, serviceName, clusterIDTagPrefix) + klog.Warningf(msg, lb.ID, tags, serviceName, clusterIDTagPrefix) + return fmt.Errorf(msg, lb.ID, tags, serviceName, clusterIDTagPrefix) +} + func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener { newListeners := []listeners.Listener{} for _, existingListener := range existingListeners { @@ -282,7 +346,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - createOpts.Tags = withLBNameTag(svcConf.lbName, svcConf.lbTags) + createOpts.Tags = withClusterIDTag(lbaas.clusterUID, withLBNameTag(svcConf.lbName, svcConf.lbTags)) } if svcConf.flavorID != "" { @@ -378,7 +442,6 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust // GetLoadBalancer returns whether the specified load balancer exists and its status func (lbaas *LbaasV2) GetLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service) (*corev1.LoadBalancerStatus, bool, error) { name := lbaas.GetLoadBalancerName(ctx, clusterName, service) - legacyName := lbaas.getLoadBalancerLegacyName(service) lbID := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerID, "") var loadbalancer *loadbalancers.LoadBalancer var err error @@ -386,7 +449,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 = lbaas.getLoadbalancerByName(ctx, service, name) } if err != nil && cpoerrors.IsNotFound(err) { return nil, false, nil @@ -422,6 +485,77 @@ 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, falling back to the Service's 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. +// +// A load balancer claimed by this cluster and by another one at the same time +// is rejected with an error rather than ignored, see clusterIDConflictError. +func (lbaas *LbaasV2) getLoadbalancerByName(ctx context.Context, service *corev1.Service, name 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 { + legacyName := lbaas.getLoadBalancerLegacyName(service) + if len(legacyName) == 0 { + return nil, cpoerrors.ErrNotFound + } + // 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 + } + } + + 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, conflictingLB, foreignFound := filterLoadBalancersByClusterID(validLBs, lbaas.clusterUID) + + if conflictingLB != nil { + return nil, lbaas.clusterIDConflictError(service, conflictingLB) + } + 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. @@ -1804,8 +1938,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName } } } else { - legacyName := lbaas.getLoadBalancerLegacyName(service) - loadbalancer, err = getLoadbalancerByName(ctx, lbaas.lb, lbName, legacyName) + loadbalancer, err = lbaas.getLoadbalancerByName(ctx, service, lbName) if err != nil { if err != cpoerrors.ErrNotFound { return nil, fmt.Errorf("error getting loadbalancer for Service %s: %v", serviceName, err) @@ -1894,9 +2027,10 @@ 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) + 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) @@ -1980,8 +2114,7 @@ func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName } else { // 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 = lbaas.getLoadbalancerByName(ctx, service, name) if err != nil { return err } @@ -2152,7 +2285,6 @@ func (lbaas *LbaasV2) deleteLoadBalancer(ctx context.Context, loadbalancer *load func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName string, service *corev1.Service) error { lbName := lbaas.GetLoadBalancerName(ctx, clusterName, service) - legacyName := lbaas.getLoadBalancerLegacyName(service) var err error var loadbalancer *loadbalancers.LoadBalancer isSharedLB := false @@ -2169,7 +2301,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 = lbaas.getLoadbalancerByName(ctx, service, lbName) } if err != nil && !cpoerrors.IsNotFound(err) { return err diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 45fbd16d28..5499e36c49 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -10,12 +10,14 @@ 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" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors" ) @@ -209,6 +211,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 { @@ -2820,3 +2827,216 @@ 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("abc-123", []string{"foo"})) + // already present, no duplicate + assert.Equal(t, []string{"kube_cluster_id_abc-123"}, withClusterIDTag("abc-123", []string{"kube_cluster_id_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" + thirdUID = "99999999-8888-7777-6666-555555555555" + thisTag = clusterIDTagPrefix + thisUID + otherTag = clusterIDTagPrefix + otherUID + thirdTag = clusterIDTagPrefix + thirdUID + 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 + wantConflictID 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, + }, + { + name: "matching tag alongside a foreign one is a conflict", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag, otherTag), + }, + wantIDs: nil, + wantConflictID: "a", + wantForeignFnd: false, + }, + { + name: "conflicting load balancer wins over an otherwise adoptable one", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag), + mkLB("b", serviceTag, thisTag, otherTag), + }, + wantIDs: nil, + wantConflictID: "b", + wantForeignFnd: false, + }, + { + name: "conflicting load balancer is not masked by an untagged one", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag), + mkLB("b", serviceTag, thisTag, otherTag), + }, + wantIDs: nil, + wantConflictID: "b", + wantForeignFnd: false, + }, + { + name: "several foreign tags without ours stay foreign, not a conflict", + clusterUID: thisUID, + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, otherTag, thirdTag), + }, + wantIDs: nil, + wantForeignFnd: true, + }, + { + name: "empty clusterUID does not flag a conflict", + clusterUID: "", + input: []loadbalancers.LoadBalancer{ + mkLB("a", serviceTag, thisTag, otherTag), + }, + wantIDs: []string{"a"}, + wantForeignFnd: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, conflicting, 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) + } + if tt.wantConflictID == "" { + assert.Nil(t, conflicting) + } else { + if assert.NotNil(t, conflicting) { + assert.Equal(t, tt.wantConflictID, conflicting.ID) + } + } + assert.Equal(t, tt.wantForeignFnd, foreign) + }) + } +} + +func TestClusterIDConflictError(t *testing.T) { + const ( + thisTag = clusterIDTagPrefix + "11111111-2222-3333-4444-555555555555" + otherTag = clusterIDTagPrefix + "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + ) + + recorder := record.NewFakeRecorder(1) + lbaas := &LbaasV2{LoadBalancer{eventRecorder: recorder}} + service := &corev1.Service{ + ObjectMeta: v1.ObjectMeta{Name: "test", Namespace: "default"}, + } + lb := &loadbalancers.LoadBalancer{ID: "lb-id", Tags: []string{"kube_service_kubernetes_default_test", thisTag, otherTag}} + + err := lbaas.clusterIDConflictError(service, lb) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "lb-id") + assert.Contains(t, err.Error(), thisTag) + assert.Contains(t, err.Error(), otherTag) + assert.Contains(t, err.Error(), "default/test") + } + + select { + case event := <-recorder.Events: + assert.Contains(t, event, corev1.EventTypeWarning) + assert.Contains(t, event, eventLBClusterIDConflict) + assert.Contains(t, event, "lb-id") + default: + t.Fatal("expected a Warning event to be recorded") + } +} 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) + } + }) +}