OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

gcs278 · 2024-09-10T21:17:48Z

Due to the AWS CCM leaking resources when the load balancer type is changed on a service, the cloud team is now blocking updates to the load balancer type. As a result, the Ingress Operator will require the service to be deleted and recreated when the Ingress Controller's load balancer type changes.

This change introduces a Progressing=True status message when the load balancer type is modified, instructing the user on how to effectuate the change. Additionally, the service.beta.kubernetes.io/aws-load-balancer-type annotation is now added to the managedAtServiceCreationLBServiceAnnotations map along with other annotations that require service deletion.

This commit does not alter the function of the LB type in the status, which has always (incorrectly) represented the desired state. However, with this change, a new scenario is introduced where the desired LB type may differ from the actual state. Consequently, the LB type status may no longer reliably reflect the actual state as it previously did, potentially surprising users who mistakenly assumed it always indicated the current state.

openshift-ci-robot · 2024-09-10T21:17:53Z

@gcs278: This pull request references Jira Issue OCPBUGS-16728, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

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

In response to this:

Due to the AWS CCM leaking resources when the load balancer type is changed on a service, the cloud team is now blocking updates to the load balancer type. As a result, the Ingress Operator will require the service to be deleted and recreated when the Ingress Controller's load balancer type changes.

This change introduces a Progressing=True status message when the load balancer type is modified, instructing the user on how to effectuate the change. Additionally, the
service.beta.kubernetes.io/aws-load-balancer-type annotation is now unmanaged.

WIP:

I'd like to confirm interactions with also changing Subnets, or EIPAllocations, or Scope at the same time.

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.

lihongan · 2024-09-12T05:31:28Z

@gcs278 tested and found one issue:
After switching we got message

ingresscontroller "nlb" is progressing: IngressControllerProgressing: One or more status conditions indicate progressing: LoadBalancerProgressing=True (OperandsProgressing: One or more managed resources are progressing: The IngressController load balancer type was changed from "NLB" to "Classic".  
To effectuate this change, you must delete the service: `oc -n openshift-ingress delete svc/router-nlb`; the service load-balancer will then be deprovisioned and a new one created. This will most likely cause the new load-balancer to have a different host name and IP address and cause disruption. 
To return to the previous state, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/nlb --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"type":"LoadBalancerService","loadBalancer":{"providerParameters":{"type":"AWS","aws":{"type":"Classic"}}}}}}'`), DeploymentRollingOut=True (DeploymentRollingOut: Waiting for router deployment rollout to finish: 1 old replica(s) are pending termination......

The command to revert the change is wrong, it should be {"type":"NLB"} because the LB type was changed from "NLB" to "Classic".

lihongan · 2024-09-12T05:38:21Z

Another concern is the ingresscontroller status, should it always show the actual status? If yes, then the spec and status might be different before deleting the svc to effectuate the change.
And seeing same thing when changing the scope (Internal<>External)

gcs278 · 2024-09-12T14:26:23Z

@gcs278 tested and found one issue: After switching we got message

@lihongan Thanks, good catch! Fixed.

Another concern is the ingresscontroller status, should it always show the actual status? If yes, then the spec and status might be different before deleting the svc to effectuate the change. And seeing same thing when changing the scope (Internal<>External)

Yea it's a great observation, I just talked about this with @Miciah yesterday. This PR won't address having the LB Type in the status reflect the actual LB Type status. It's complicated, and will require much more refactoring than I'm willing to bite off with this PR. I think we should consider this a technical debt item that we should try to solve later.

The LB Type in the status will mostly be equivalent to the LB Type in the spec (for now).

gcs278 · 2024-09-12T21:20:53Z

I've taken the WIP off the PR. I'm pretty content with the approach taken from some ideas of @Miciah (i.e. two disjoint "managed" annotation lists). Some parts of the code actually were simplified and code was removed as a result.

I've also dropped the usage of "auto effectuation" or "manual effectuation" as I don't think this aptly conveys the idea that you need to delete/recreate the service (based on conversations with other team members). I went for managedAtServiceCreationLBServiceAnnotations to explicitly describe that the annotations are only managed when the service is created, implying the service will need to be recreated for updates to take affect. I'm open to ideas still though.

gcs278 · 2024-09-13T01:48:46Z

e2e-aws-operator unrelated failure:

=== RUN   TestAll/serial/TestUpdateDefaultIngressControllerSecret
    util_test.go:540: conflict when updating ingress controller openshift-ingress-operator/default: Operation cannot be fulfilled on ingresscontrollers.operator.openshift.io "default": the object has been modified; please apply your changes to the latest version and try again, retrying...

/test e2e-aws-operator

Install failure
/test e2e-aws-operator-techpreview

unrelated
/test e2e-aws-ovn-upgrade
/test e2e-gcp-ovn

lihongan · 2024-09-13T04:38:36Z

@gcs278 Another issue is observed. Because PROXY proxy is enabled on Classic but disabled on NLB, so before deleting the LB service to effectuate the change, I can see the router deployment is rolled out with ENV ROUTER_USE_PROXY_PROTOCOL updated and service annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol is changed, that might be potential problem.

gcs278 · 2024-09-13T13:53:39Z

@gcs278 Another issue is observed. Because PROXY proxy is enabled on Classic but disabled on NLB, so before deleting the LB service to effectuate the change, I can see the router deployment is rolled out with ENV ROUTER_USE_PROXY_PROTOCOL updated and service annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol is changed, that might be potential problem.

Ah good catch. I forgot that somethings rely on the status LB Type to be accurate. I need to think about how to fix this.
/wip

Miciah · 2024-10-02T15:01:34Z

/assign

gcs278 · 2024-12-17T19:12:35Z

/retest

shellyyang1989 · 2024-12-26T07:20:06Z

/retest-required

openshift-bot · 2025-01-01T00:01:01Z

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

openshift-ci-robot · 2025-01-01T00:01:31Z

@openshift-bot: This pull request references Jira Issue OCPBUGS-16728, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @lihongan

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

In response to this:

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

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.

gcs278 · 2025-01-03T15:24:39Z

/retest-required

Some links in `load_balancer_service.go` were invalid due to K8S documentation URL changes.

Refactor to introduce a new map, `managedAtServiceCreationLBServiceAnnotations`, which tracks annotations that are managed by the operator but are only applied to the load balancer at the time of service creation. These annotations are also responsible for triggering the `Progressing: True` status when modified. This addition not only improves documentation of these annotations, but also simplifies the code. The `loadBalancerServiceAnnotationsChanged` function has been adapted to replace existing logic in `shouldRecreateLoadBalancer`, allowing the removal of the `scopeEqual` function as well.

Due to the AWS CCM leaking resources when the load balancer type is changed on a service, the cloud team is now blocking updates to the load balancer type. As a result, the Ingress Operator will require the service to be deleted and recreated when the Ingress Controller's load balancer type changes. This change introduces a `Progressing: True` status message when the load balancer type is modified, instructing the user on how to effectuate the change. This commit does not alter the function of the LB type in the status, which has always (incorrectly) represented the desired state. However, with this change, a new scenario is introduced where the desired LB type may differ from the actual state. Consequently, the LB type status may no longer reliably reflect the actual state as it previously did, potentially surprising users who mistakenly assumed it always indicated the current state.

IsProxyProtocolNeeded must get the LB Type from the service, as the status now may not accurately reflect the actual LB Type during an update. However, we can't rely only on the service because it doesn’t exist during the initial bootstrap (and potentially when it gets deleted). In that case, it's safe to use the status to determine the anticipated LB Type. Without this commit results in a misalignment of the proxy protocol value between the load balancer and the router deployment when an LB type change is pending. This commit will be squashed into the previous one, as both are dependent on each other. I've kept them separate for easier review.

Previously, the fix for OCPBUGS-38217 cleared inactive LB parameters in the status in setDefaultPublishingStrategy. This was valid at the time, as the desired LB type would always become the actual LB type. However, with the introduction of pending LB type changes, setDefaultPublishingStrategy was now prematurely clearing parameters for the current (but soon-to-be inactive) LB type. This caused issues because certain LB parameters, such as classicLoadBalancer.subnets or networkLoadBalancer.eipAllocations, reflect the actual state. As a result, setDefaultPublishingStrategy was clearing them too early, without knowing a type change was pending. The solution is to move the clearing logic to syncIngressControllerStatus in status.go, where it can detect pending LB type changes and avoid clearing parameters for the still-active LB type. This means that during a pending type change, both networkLoadBalancer and classicLoadBalancer parameters are present. This commit will be squashed into the previous one, as both are dependent on each other. I've kept them separate for easier review.

openshift-ci · 2025-01-09T16:35:10Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-01-09T21:04:18Z

@gcs278: 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-hypershift	`1a57afd`	link	true	`/test e2e-hypershift`
ci/prow/e2e-azure-operator	`1a57afd`	link	true	`/test e2e-azure-operator`
ci/prow/e2e-aws-ovn-single-node	`1a57afd`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-gcp-ovn	`1a57afd`	link	false	`/test e2e-gcp-ovn`
ci/prow/e2e-gcp-operator	`1a57afd`	link	true	`/test e2e-gcp-operator`
ci/prow/hypershift-e2e-aks	`1a57afd`	link	true	`/test hypershift-e2e-aks`
ci/prow/e2e-aws-ovn-upgrade	`1a57afd`	link	true	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-aws-ovn-serial	`1a57afd`	link	true	`/test e2e-aws-ovn-serial`

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.

Miciah · 2025-01-31T05:16:28Z

pkg/operator/controller/ingress/load_balancer_service.go

@@ -23,8 +24,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/apimachinery/pkg/util/sets"


This import got erroneously moved.

Miciah · 2025-01-31T05:24:56Z

pkg/operator/controller/ingress/load_balancer_service.go

+			if _, ok := platformsWithMutableScope[platform]; !ok {
+				for name := range annotation {
+					if result[platform] == nil {
+						result[platform] = sets.New[string]()
+					}
+					result[platform].Insert(name)
+				}
+			}


A little easier to read:

Suggested change

if _, ok := platformsWithMutableScope[platform]; !ok {

for name := range annotation {

if result[platform] == nil {

result[platform] = sets.New[string]()

}

result[platform].Insert(name)

}

}

if _, ok := platformsWithMutableScope[platform]; ok {

continue

}

for name := range annotation {

if result[platform] == nil {

result[platform] = sets.New[string]()

}

result[platform].Insert(name)

}

Miciah · 2025-01-31T05:25:48Z

pkg/operator/controller/ingress/load_balancer_service.go

+		for platform, annotation := range InternalLBAnnotations {
+			if _, ok := platformsWithMutableScope[platform]; !ok {
+				for name := range annotation {


Suggested change

for platform, annotation := range InternalLBAnnotations {

if _, ok := platformsWithMutableScope[platform]; !ok {

for name := range annotation {

for platform, annotations := range InternalLBAnnotations {

if _, ok := platformsWithMutableScope[platform]; !ok {

for name := range annotations {

Miciah · 2025-01-31T05:58:50Z

pkg/operator/controller/ingress/load_balancer_service_test.go

+				t.Errorf("expected reason %s, got %s", tc.expectChangedAnnotations, changedAnnotations)
+			}
+			if !cmp.Equal(changedFields, tc.expectChangedFields, sliceCmpOpts...) {
+				t.Errorf("expected reason %s, got %s", tc.expectChangedFields, changedFields)


Suggested change

t.Errorf("expected reason %s, got %s", tc.expectChangedAnnotations, changedAnnotations)

}

if !cmp.Equal(changedFields, tc.expectChangedFields, sliceCmpOpts...) {

t.Errorf("expected reason %s, got %s", tc.expectChangedFields, changedFields)

t.Errorf("expected changedAnnotations %s, got %s", tc.expectChangedAnnotations, changedAnnotations)

}

if !cmp.Equal(changedFields, tc.expectChangedFields, sliceCmpOpts...) {

t.Errorf("expected changedFields %s, got %s", tc.expectChangedFields, changedFields)

Miciah · 2025-01-31T06:03:42Z

pkg/operator/controller/ingress/load_balancer_service.go

@@ -774,7 +792,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
 	// Since the status logic runs after the controller sets the annotations, it checks for
 	// any discrepancy (in case modified) between the desired annotation values of the controller
 	// and the current annotation values.
-	changed, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLoadBalancerServiceAnnotations)
+	changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)


Suggested change

changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)

changed, _, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)

Miciah · 2025-01-31T06:10:29Z

pkg/operator/controller/ingress/load_balancer_service.go

+		changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
+		err := fmt.Errorf(changedMsg)


I don't think we want double format expansion, do we?

Suggested change

changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

err := fmt.Errorf(changedMsg)

err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

changedMsg := err.Error()

or

Suggested change

changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

err := fmt.Errorf(changedMsg)

changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

err := fmt.Errorf("%s", changedMsg)

or import "errors" and then

Suggested change

changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

err := fmt.Errorf(changedMsg)

changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)

err := errors.New(changedMsg)

Miciah · 2025-01-31T06:19:34Z

pkg/operator/controller/ingress/load_balancer_service.go

+	haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service)
+	if platform.Type == configv1.AWSPlatformType {
+		// Tricky: LB Type in the status indicates the *desired* scope (user desire + defaulting).
+		wantLBType := getAWSLoadBalancerTypeInStatus(ic)


This logic is confusing. Why is haveLBType computed outside the if statement and wantLBType inside? I think haveLBType will give operatorv1.AWSClassicLoadBalancer on non-AWS platforms, which is kind of silly. Moreover, it seems to me that all the AWS-specific code (for LB type, subnets, and EIP) should be inside a single if platform.Type == configv1.AWSPlatformType { block.

Miciah · 2025-01-31T06:21:37Z

pkg/operator/controller/ingress/load_balancer_service.go

 		if _, ok := platformsWithMutableScope[platform.Type]; !ok {
-			err = fmt.Errorf("%[1]s  To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created.  This will most likely cause the new load-balancer to have a different host name and IP address from the old one's.  Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'`", err.Error(), service.Namespace, service.Name, ic.Name, haveScope)
+			ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)


Suggested change

ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)

ocPatchRevertCmd := fmt.Sprintf(`oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"loadBalancer":{"scope":"%[2]s"}}}}'`, ic.Name, haveScope)

Miciah · 2025-01-31T06:23:25Z

pkg/operator/controller/ingress/load_balancer_service.go

 		if _, ok := platformsWithMutableScope[platform.Type]; !ok {
-			err = fmt.Errorf("%[1]s  To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created.  This will most likely cause the new load-balancer to have a different host name and IP address from the old one's.  Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'`", err.Error(), service.Namespace, service.Name, ic.Name, haveScope)
+			ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)
+			err = fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))


I think you have extra format expansion again, here and everywhere where you have fmt.Errorf(effectuateMessage(...)).

Miciah · 2025-01-31T06:26:25Z

pkg/operator/controller/ingress/load_balancer_service_test.go

+			expect: false,
+		},
+		{
+			description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",


Suggested change

description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",

description: "if the service.beta.kubernetes.io/aws-load-balancer-type annotation is added",

Miciah · 2025-01-31T06:29:43Z

test/e2e/operator_test.go

@@ -1370,19 +1370,23 @@ func TestAWSResourceTagsChanged(t *testing.T) {
 	assertLoadBalancerServiceAnnotationWithPollImmediate(t, kclient, defaultIC, awsLBAdditionalResourceTags, expectedTags)
 }

+// TestAWSLBTypeChange verifies that process of changing the IngressController's


Suggested change

// TestAWSLBTypeChange verifies that process of changing the IngressController's

// TestAWSLBTypeChange verifies the process of changing the IngressController's

Miciah · 2025-01-31T06:44:01Z

pkg/operator/controller/ingress/controller_test.go

+		},
+		{
+			description: "loadbalancer strategy with ELB, but a service with NLB shouldn't use PROXY",
+			strategy:    &loadBalancerStrategyWithNLB,


Suggested change

strategy: &loadBalancerStrategyWithNLB,

strategy: &loadBalancerStrategy,

based on the description of the test case.

Miciah · 2025-01-31T06:45:39Z

pkg/operator/controller/ingress/load_balancer_service.go

-	want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled)
+	proxyNeeded, err := IsProxyProtocolNeeded(ic, platform, current)
+	if err != nil {
+		return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)


Suggested change

return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)

return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %w", ic.Namespace, ic.Name, err)

Miciah · 2025-01-31T06:46:03Z

pkg/operator/controller/ingress/controller.go

+
+	proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus, currentLBService)
+	if err != nil {
+		errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err))


Suggested change

errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err))

errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %w", ci.Namespace, ci.Name, err))

Miciah · 2025-01-31T07:00:10Z

pkg/operator/controller/ingress/controller_test.go

-		{
-			name:                    "loadbalancer type changed from ELB to NLB, with old ELB parameters removal",
-			ic:                      makeIC(spec(nlb()), status(elbWithNullParameters())),
-			expectedResult:          true,
-			expectedIC:              makeIC(spec(nlb()), status(nlbWithNullParameters())),
-			domainMatchesBaseDomain: true,
-		},
-		{
-			name:                    "loadbalancer type changed from NLB to ELB, with old NLB parameters removal",
-			ic:                      makeIC(spec(elb()), status(nlbWithNullParameters())),
-			expectedResult:          true,
-			expectedIC:              makeIC(spec(elb()), status(elbWithNullParameters())),
-			domainMatchesBaseDomain: true,
-		},


Is there a reason you removed these test cases rather than changing them to match the new behavior? Granted, TestSetDefaultPublishingStrategyHandlesUpdates is already a bit unwieldy, and you did add plenty of test coverage in this commit, and that should be adequate.

gcs278 · 2025-01-31T20:12:57Z

Thanks for the reviews @Miciah, I'm running low on time and don't think I'll be able to get to this today, so I think someone else will have to follow up, or we should merge as is. I have no preference.

Let's be sure to coordinate this with @JoelSpeed and the merging of openshift/cluster-cloud-controller-manager-operator#362 before we merge this PR. This PR only is required when openshift/cluster-cloud-controller-manager-operator#362 is merged.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2024

openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Sep 10, 2024

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

openshift-ci bot requested review from lihongan, candita and miheer September 10, 2024 21:18

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from 65eea90 to 8a299a9 Compare September 11, 2024 22:25

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from 8a299a9 to 26a1492 Compare September 12, 2024 14:21

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch 2 times, most recently from 9d3a208 to b02db5f Compare September 12, 2024 21:10

gcs278 changed the title ~~[WIP] OCPBUGS-16728: Require Service Deletion for LB Type Updates~~ OCPBUGS-16728: Require Service Deletion for LB Type Updates Sep 12, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2024

gcs278 changed the title ~~OCPBUGS-16728: Require Service Deletion for LB Type Updates~~ [WIP] OCPBUGS-16728: Require Service Deletion for LB Type Updates Sep 13, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2024

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from b02db5f to 5e2d300 Compare September 18, 2024 23:25

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from 5e2d300 to c6fd8b6 Compare September 19, 2024 01:27

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2024

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from c6fd8b6 to 07b7cbb Compare September 27, 2024 17:47

openshift-ci bot assigned Miciah Oct 2, 2024

gcs278 added 5 commits January 9, 2025 11:29

Update Broken Annotation Documentation Links

c25c6c1

Some links in `load_balancer_service.go` were invalid due to K8S documentation URL changes.

gcs278 force-pushed the OCPBUGS-16728-lb-type-change-effectuation branch from b27545b to 1a57afd Compare January 9, 2025 16:35

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2025

Miciah reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

gcs278 commented Sep 10, 2024 •

edited

Loading

openshift-ci-robot commented Sep 10, 2024

lihongan commented Sep 12, 2024

lihongan commented Sep 12, 2024

gcs278 commented Sep 12, 2024 •

edited

Loading

gcs278 commented Sep 12, 2024

gcs278 commented Sep 13, 2024

lihongan commented Sep 13, 2024

gcs278 commented Sep 13, 2024

Miciah commented Oct 2, 2024

gcs278 commented Dec 17, 2024

shellyyang1989 commented Dec 26, 2024

openshift-bot commented Jan 1, 2025

openshift-ci-robot commented Jan 1, 2025

gcs278 commented Jan 3, 2025

openshift-ci bot commented Jan 9, 2025

openshift-ci bot commented Jan 9, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

Miciah Jan 31, 2025

gcs278 commented Jan 31, 2025

	changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)
	changed, _, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)

		changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
		err := fmt.Errorf(changedMsg)

	ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)
	ocPatchRevertCmd := fmt.Sprintf(`oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"loadBalancer":{"scope":"%[2]s"}}}}'`, ic.Name, haveScope)

	description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",
	description: "if the service.beta.kubernetes.io/aws-load-balancer-type annotation is added",

	// TestAWSLBTypeChange verifies that process of changing the IngressController's
	// TestAWSLBTypeChange verifies the process of changing the IngressController's

	strategy: &loadBalancerStrategyWithNLB,
	strategy: &loadBalancerStrategy,

	return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
	return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %w", ic.Namespace, ic.Name, err)

	errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err))
	errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %w", ci.Namespace, ci.Name, err))

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Are you sure you want to change the base?

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Conversation

gcs278 commented Sep 10, 2024 • edited Loading

openshift-ci-robot commented Sep 10, 2024

lihongan commented Sep 12, 2024

lihongan commented Sep 12, 2024

gcs278 commented Sep 12, 2024 • edited Loading

gcs278 commented Sep 12, 2024

gcs278 commented Sep 13, 2024

lihongan commented Sep 13, 2024

gcs278 commented Sep 13, 2024

Miciah commented Oct 2, 2024

gcs278 commented Dec 17, 2024

shellyyang1989 commented Dec 26, 2024

openshift-bot commented Jan 1, 2025

openshift-ci-robot commented Jan 1, 2025

gcs278 commented Jan 3, 2025

openshift-ci bot commented Jan 9, 2025

openshift-ci bot commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcs278 commented Jan 31, 2025

gcs278 commented Sep 10, 2024 •

edited

Loading

gcs278 commented Sep 12, 2024 •

edited

Loading