[occm] Skip LB rename for shared load balancers from other clusters

[234u34k]

The cluster-name rename logic from #2552 does not account for shared load balancers. When cluster B references a shared LB created by cluster A, the OCCM sees a mismatched cluster name and renames the LB, breaking cluster A.

Before renaming, verify that the LB's namespace and service name components match the current Service. This distinguishes a legitimate cluster-name change from a cross-cluster shared LB access.

Ref #2682

occm: Fixed shared load balancers being renamed when referenced by a Service from a different cluster. A new `enable-lb-rename` option (default `true`) disables renaming entirely, required when sharing a load balancer across clusters between Services with the same namespace and name.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: 234u34k / name: 234u34k (d223a02)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign anguslees for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• pkg/openstack/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @234u34k. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[jihyun-huh]

Hi @234u34k, thanks for this PR
I was looking at this with the issue #2682, since the issue seems to have gone rotten without much follow-up.

even if i have no permission to review this, the direction makes sense to me.

one thing i want to double-check, the original #2682 example seems to use same namespace and service name across clusters. for example, cluster-B reconciling a shared LB named kube_service_clustera_kns_fakeservice for kns/fakeservice, in this case, would the new expectedOldName still match and allow the rename?

If so,, should the rename path also takes the explicit shared LB into account. for example when loadbalancer.openstack.org/load-balancer-id is set and the existing LB name does not match the current Service’s expected LB name?

may be im missing part of the ownership logic here, but think a regression test can cover the #2682 examples.

cc. @kayrus @stephenfin @gouthampacha

[jihyun-huh]

/remove-lifecycle rotten

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kayrus for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[234u34k]

Confirmed. oldClusterName is derived from the LB name itself, so when both
clusters run a Service with the same namespace and name (the #2682 example),
expectedOldName reconstructs the existing name and the rename still fires.
My guard only covered the case where the tuples differ.

Gating on the load-balancer-id annotation doesn't work either: OCCM persists
that annotation on every Service it manages, and after a cluster-name change
the stale LB is only ever found through it, so the legitimate #2552 rename
flows through the same branch. And for the identical namespace/name case there
is nothing in Octavia that distinguishes the two. The LB name, its tags and its
children all look exactly like a legitimately stale LB of this cluster.

Updated the PR accordingly:

1. extracted the ownership check into lbNameMatchesService and added
   regression tests covering the [occm] Shared loadbalancer should not be renamed. #2682 examples (same and different
   namespace/name across clusters, the legitimate [occm] Allow changing cluster-name on existing deployments #2552 rename, truncated
   names)
2. added an enable-lb-rename option (default true, [occm] Allow changing cluster-name on existing deployments #2552 behavior unchanged)
   so deployments sharing load balancers across clusters with identical
   Service tuples can disable renaming, since that case is not decidable from
   the OpenStack side

Also added a V(4) log when a rename is skipped and a docs entry for the option.

@kayrus @stephenfin could one of you take a look and issue /ok-to-test? CI has
not run on this PR yet.

[gouthampacha]

/ok-to-test

[jihyun-huh]

Thanks for updating this!
Ive read the new changes and they look good to me.

one small thing I'd consider, is adding a test covering enable-lb-rename=false ,
so the new configuration path is explicitly covered.

I also tested the scenarios locally, including enable-lb-rename=false, and it worked well as expected.

[234u34k]

@stephenfin could I ask for a review from you on this at your convenience? Believe it's ready now