OCPBUGS-16728: Add admission policy to deny changing an AWS LB type on an existing service #362

JoelSpeed · 2024-08-19T14:40:52Z

The AWS CCM does not account for changes to the type of the load balancer. The recommended and supportable approach is for users to delete and recreate the service, rather than changing the annotation live.

This PR adds policy to prevent services from adding/removing/changing the annotation to control which kind of load balancer the service represents, such that the annotation must be set on create for NLB load balancers.

This should prevent any leaking of NLB/CLB resources when users mistakenly change the load balancer type.

openshift-ci-robot · 2024-08-19T14:40:58Z

@JoelSpeed: This pull request references Jira Issue OCPBUGS-16728, which is invalid:

expected the bug to target the "4.18.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

The AWS CCM does not account for changes to the type of the load balancer. The recommended and supportable approach is for users to delete and recreate the service, rather than changing the annotation live.

This PR adds policy to prevent services from adding/removing/changing the annotation to control which kind of load balancer the service represents, such that the annotation must be set on create for NLB load balancers.

This should prevent any leaking of NLB/CLB resources when users mistakenly change the load balancer type.

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 openshift-eng/jira-lifecycle-plugin repository.

JoelSpeed · 2024-08-19T14:43:34Z

/jira refresh

openshift-ci-robot · 2024-08-19T14:43:42Z

@JoelSpeed: This pull request references Jira Issue OCPBUGS-16728, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-08-19T14:45:47Z

[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 ask for approval from joelspeed. For more information see the Kubernetes Code Review Process.

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

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

theobarberbany · 2024-08-19T15:36:38Z

pkg/cloud/aws/assets/validating-admission-policy.yaml

+    - expression: |
+        (has(object.objectMeta.annotations) && 'service.beta.kubernetes.io/aws-load-balancer-type' in object.objectMeta.annotations) ==
+          (has(oldObject.objectMeta.annotations) && 'service.beta.kubernetes.io/aws-load-balancer-type' in oldObject.objectMeta.annotations) &&
+          object.objectMeta.annotations['service.beta.kubernetes.io/aws-load-balancer-type'] == oldObject.objectMeta.annotations['service.beta.kubernetes.io/aws-load-balancer-type']


I'm not 100% following how this works, so this is more for my understanding - shout if I'm wrong:

We assert that our old object and our new object have both got the annotation matching service.beta.kubernetes.io/aws-load-balancer-type. We need this because otherwise the controller would not be able to set the value initially?

Then we check the value of the annotation remains equal:

object.objectMeta.annotations['service.beta.kubernetes.io/aws-load-balancer-type'] == oldObject.objectMeta.annotations['service.beta.kubernetes.io/aws-load-balancer-type']

If this annotation were being changed, we would fail the validation - so we would fail that update, disasllowing changing it's value.

Or am I missing something?

We assert that our old object and our new object have both got the annotation matching service.beta.kubernetes.io/aws-load-balancer-type. We need this because otherwise the controller would not be able to set the value initially?

You're close but missing one nuance, we check that whether "they have the annotation or not" is equal. If one has the annotation and the other does not, then the user changed something, and we aren't allowing that here.

Ahh gotcha! Makes sense!

theobarberbany · 2024-08-19T15:38:20Z

/lgtm

JoelSpeed · 2024-08-19T15:44:37Z

/hold

This needs testing

CC @sunzhaohua2 I'd be interested to see if any QE cases are affected by this change as well, I'll give you a ping when it's ready to test

openshift-ci · 2024-08-19T16:13:27Z

New changes are detected. LGTM label has been removed.

…ervice

openshift-bot · 2024-12-03T01:01:05Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

JoelSpeed · 2024-12-10T16:27:02Z

/remove-lifecycle stale

openshift-bot · 2025-03-11T01:01:02Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

JoelSpeed · 2025-03-11T10:10:12Z

/remove-lifecycle stale

openshift-ci · 2025-03-26T19:22:03Z

@JoelSpeed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-openstack-ovn	`a21723c`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-vsphere-ovn	`a21723c`	link	false	`/test e2e-vsphere-ovn`
ci/prow/e2e-azure-ovn-upgrade	`a21723c`	link	false	`/test e2e-azure-ovn-upgrade`
ci/prow/e2e-azure-ovn	`a21723c`	link	false	`/test e2e-azure-ovn`
ci/prow/level0-clusterinfra-azure-ipi-proxy-tests	`a21723c`	link	false	`/test level0-clusterinfra-azure-ipi-proxy-tests`
ci/prow/unit	`a21723c`	link	true	`/test unit`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-bot · 2025-06-25T01:00:22Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

JoelSpeed · 2025-06-25T07:28:10Z

/remove-lifecycle stale

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 19, 2024

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 19, 2024

openshift-ci bot requested review from lihongan, racheljpg and theobarberbany August 19, 2024 14:44

theobarberbany reviewed Aug 19, 2024

View reviewed changes

openshift-ci bot assigned theobarberbany Aug 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2024

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 19, 2024

JoelSpeed force-pushed the prevent-nlb-clb-switch branch 8 times, most recently from 051246e to 3cded47 Compare August 22, 2024 13:41

Add admission policy to deny changing an AWS LB type on an existing s…

a21723c

…ervice

JoelSpeed force-pushed the prevent-nlb-clb-switch branch from 3cded47 to a21723c Compare September 3, 2024 09:21

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 3, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 10, 2024

gcs278 mentioned this pull request Jan 31, 2025

OCPBUGS-16728: Require Service Deletion for LB Type Updates openshift/cluster-ingress-operator#1142

Open

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 11, 2025

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 11, 2025

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 25, 2025

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 25, 2025

OCPBUGS-16728: Add admission policy to deny changing an AWS LB type on an existing service #362

Are you sure you want to change the base?

OCPBUGS-16728: Add admission policy to deny changing an AWS LB type on an existing service #362

Uh oh!

Conversation

JoelSpeed commented Aug 19, 2024

Uh oh!

openshift-ci-robot commented Aug 19, 2024

Uh oh!

JoelSpeed commented Aug 19, 2024

Uh oh!

openshift-ci-robot commented Aug 19, 2024

Uh oh!

openshift-ci bot commented Aug 19, 2024

Uh oh!

theobarberbany Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

theobarberbany Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

theobarberbany commented Aug 19, 2024

Uh oh!

JoelSpeed commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 19, 2024

Uh oh!

openshift-bot commented Dec 3, 2024

Uh oh!

JoelSpeed commented Dec 10, 2024

Uh oh!

openshift-bot commented Mar 11, 2025

Uh oh!

JoelSpeed commented Mar 11, 2025

Uh oh!

openshift-ci bot commented Mar 26, 2025

Uh oh!

openshift-bot commented Jun 25, 2025

Uh oh!

JoelSpeed commented Jun 25, 2025

Uh oh!

Uh oh!

theobarberbany Aug 19, 2024 •

edited

Loading

JoelSpeed commented Aug 19, 2024 •

edited

Loading