[occm] Tag load balancers with cluster identity to prevent name collisions - #3103
[occm] Tag load balancers with cluster identity to prevent name collisions#3103enginrect wants to merge 7 commits into
Conversation
k8s-ci-robot
added
the
release-note
|
Welcome @enginrect! |
|
Hi @enginrect. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
size/L
|
Hi @kayrus @stephenfin @zetaab — first-time contributor here. This PR addresses the long-standing cross-cluster LB collision issue (refs #2241, #2571, #2624) with an additive, backward-compatible Could one of you take a look and add Thanks! |
123ffe4 to
bbf5f4f
Compare
k8s-ci-robot
added
cncf-cla: no
9bb737f to
b620ccf
Compare
k8s-ci-robot
added
cncf-cla: yes
|
Quick update: I've rebased on top of master (which now has the SHA-pinned actions from #3100) and bumped the helm chart patch version to 2.35.1 — |
| // 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 |
There was a problem hiding this comment.
clusterUID can be defined only in LoadBalancer struct, please remove it from here.
There was a problem hiding this comment.
Good catch — thanks! You're absolutely right, that was a cohesion miss on my part. clusterUID is only consumed by the LB code path so it shouldn't pollute the global OpenStack struct. Removed in 56756ea; the field now lives only on LoadBalancer where it belongs.
| 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 |
There was a problem hiding this comment.
you can set clusterUID value here with:
clusterID := fetchClusterUID(os.kclient)There was a problem hiding this comment.
Thank you, that's a much cleaner pattern. Fetching lazily inside LoadBalancer() keeps the kube-system namespace lookup out of the global Initialize() path and limits the change to the LB construction site. As a nice side effect, clusters that disable LB now skip the lookup entirely. Done in 56756ea.
There was a problem hiding this comment.
you can filter by tags using ListOpts.Tags. filterLoadBalancersByClusterID doesn't make sense.
UPD: please ignore this comment
There was a problem hiding this comment.
I see my earlier comment, but I've forgotten my reasoning for changing my mind. Let's add a comment explaining why we're not using API-level tag filtering here. Primarily because we need to log warnings for unmatched tags and for transition period.
That said, I think we should still consider server-side filtering to reduce load on the Octavia API. Do you have any suggestions or concerns about this approach?
There was a problem hiding this comment.
I think server-side filtering is hard to adopt at this point, mainly because of the legacy LBs. The Octavia tag filter can only express "has tag X", so I don't think a single call can return "LBs tagged with our cluster id, plus LBs with no cluster-id tag at all". To keep the legacy case working we would need a second unfiltered list call as a fallback, and in the common case that doubles the API requests instead of reducing them. Another concern is that we would stop seeing foreign-tagged LBs entirely, so the warning log for the collision case couldn't be emitted.
That said, I think once the tag has been deployed widely enough that the legacy fallback can be dropped, switching to ListOpts.Tags would be a good follow-up optimization. I added a comment in the code explaining this reasoning.
| // 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" |
There was a problem hiding this comment.
Great question — and the honest answer is: no, it's not used anywhere, sorry about the leftover. The original intent was to emit a Warning event via eventRecorder from getLoadbalancerByName when we drop a foreign-tagged LB, but plumbing the event recorder and *v1.Service into that free-standing function felt out of scope for this PR (the existing warning log already covers operator visibility), so I dropped the emission and forgot to remove the constant. Cleaned up in 56756ea.
|
Thanks for the review @kayrus! Pushed the changes in 56756ea:
|
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
The check job was tripping on SA1019 because the new Verified locally:
|
k8s-ci-robot
added
the
needs-rebase
bdabd79 to
efe341d
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Rebased on top of current master (which now includes The only conflict was in Verified locally before pushing: All 8 changed files are unchanged in scope from before the rebase — no new code was introduced beyond the version number update. Conflict marker scan ( @kayrus — all checks were green before the rebase, and local verification is clean. Would appreciate |
|
I'm affected by this issue, so I'd like to see this PR merged (assuming it does as described), but I'm not sure what the AI/LLM policy is on this repo and I noticed the replies from @enginrect in this PR are quite obviously LLM generated. Looking at their profile their entire bio is clearly written by LLM too, so one begins to wonder if there's even a human actively controlling this account. I'd have hoped people would state up front if they're using AI for a contribution to an open-source project, and to what extent they are doing so... This appears on the surface like an agent is handling a lot of this without human interaction, which I would personally say warrants extra scrutiny. |
|
@JonnyHaystack I want to clarify that this PR comes from a real issue I personally experienced while operating Kubernetes on top of OpenStack using OCCM. The problem described in the issue affected my actual environment, and the patch reflects my own intent and understanding of the behavior I wanted to fix. Firstly, (True)I did use Cursor assistance for some parts of the coding and for drafting responses. This is my first contribution here, so I may also have been overly detailed or overly polished in my replies. Since I am Korean and English is not my first language, I also used AI assistance to make sure my communication was clear and accurate. I am still fully responsible for the change, the design decision, the final code, and the discussion in this PR. This is not an autonomous AI-generated contribution. If maintainers(of this repo) have a specific policy, disclosure requirement, or extra review/security process around AI-assisted contributions, I will follow it and rework on this. |
|
Thanks for making that clear 🙂 hoping to see this merged! |
kubernetes-prow
Bot
added
the
needs-rebase
efe341d to
7adc9fe
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
removed
the
needs-rebase
…sions OCCM constructs Octavia load balancer names as kube_service_<cluster-name>_<namespace>_<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 kubernetes#2241, kubernetes#2571, kubernetes#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_<uid>. 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.
- 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.
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.
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.
7adc9fe to
25e35f2
Compare
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.
| `--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)). |
There was a problem hiding this comment.
nit: avoid linking to issues in documentation. I found only one exception in the repo: this line references an OCCM issue because it describes a workaround for an unfixed bug rather than standard behavior. For typical use cases and resolved issues, documentation should stand alone without issue references.
| // 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 { |
There was a problem hiding this comment.
The uid is the primary input - it's what drives the logic. Let's put it first.
| func withClusterIDTag(tags []string, uid string) []string { | |
| func withClusterIDTag(uid string, tags []string) []string { |
And please put this function just after the withLBNameTag func.
There was a problem hiding this comment.
I see my earlier comment, but I've forgotten my reasoning for changing my mind. Let's add a comment explaining why we're not using API-level tag filtering here. Primarily because we need to log warnings for unmatched tags and for transition period.
That said, I think we should still consider server-side filtering to reduce load on the Octavia API. Do you have any suggestions or concerns about this approach?
| // 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) { |
There was a problem hiding this comment.
I think it makes more sense to convert this function to a LoadBalancer method. That would simplify calls like getLoadbalancerByName(ctx, lbaas.lb, name, legacyName, lbaas.clusterUID) to just lbaas.getLoadbalancerByName(ctx, name, legacyName). Also move this method close to other LoadBalancer methods.
|
Just a heads-up: our current e2e test coverage is poor, and I'd like to merge #3153 before this PR. Could you write e2e tests covering the clusterID tag functionality based on #3153 and submit them as a follow-up PR? Let me know if this works for you. Add following e2e tests:
|
| // 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. | ||
| func stripReservedTags(tags []string) []string { |
There was a problem hiding this comment.
add a clusterIDTagPrefix prefix as an exception for reserved tags
- 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.
Documentation should stand alone without references to resolved issues.
4 participants
What this PR does / why we need it:
OCCM identifies an existing Octavia load balancer for a Service by name on
the first reconcile (via
getLoadbalancerByName). The name formatkube_service_<cluster-name>_<namespace>_<service>defaults to a<cluster-name>ofkubernetes, so two Kubernetes clusters in the sameOpenStack project that happen to use the default cluster-name and have
Services with identical namespace/name produce identical load balancer
names. Octavia does not enforce uniqueness of names, so OCCM in cluster B
ends up adopting and overwriting cluster A's load balancer. This has been
reported repeatedly (see #2241, #2571, #2624) and the standing guidance
"set a unique
--cluster-name" is correct but does not actually defendagainst the failure mode.
This PR adds a stable Kubernetes cluster identifier - the UID of the
kube-systemnamespace - as a load balancer tag of the formkube_cluster_id_<uid>. Lookup behaviour:kube_cluster_id_<our-uid>tag are kept.kube_cluster_id_*tag fall back to the legacybehaviour (preserves existing deployments and externally-created LBs).
kube_cluster_id_*tags are treated asNotFound, with a warning. OCCM will then create its own load balancer
rather than overwriting one that belongs to another cluster.
The cluster UID is read once at controller-manager start-up. If the
lookup fails (RBAC denial, missing namespace, etc.) the safeguard is
disabled and OCCM falls back to the legacy name-based behaviour, so the
change is strictly additive. Pre-existing load balancers also gain the
kube_cluster_id_*tag during the next reconciliation.Which issue this PR fixes(if applicable):
fixes #3102
Special notes for reviewers:
kube_cluster_id_*tag keep the previousbehaviour. They are tagged on the next successful reconcile.
loadbalancer.openstack.org/load-balancer-idannotation (i.e. onevery reconcile after the first one) go through
GetLoadbalancerByID,which is unaffected.
getonnamespacesis added to both the manifestClusterRole (
manifests/controller-manager/cloud-controller-manager-roles.yaml)and the helm chart (
charts/openstack-cloud-controller-manager/templates/clusterrole.yaml).If the verb is unavailable the safeguard simply degrades to the legacy
behaviour with a warning log; OCCM does not refuse to start.
already gated by
svcConf.supportLBTagsand behaves as before on olderclouds.
TestFilterLoadBalancersByClusterIDcovers the matching, legacy,foreign-only, and mixed cases.
TestFetchClusterUIDcovers happy path and graceful degradation(missing namespace, forbidden) with a fake clientset.
How to verify manually:
go test ./pkg/openstack/...A reproduction of the original failure mode (two clusters in the same
project, same
--cluster-name, same Service ns/name) is described in#3102.
Release note: