NE-621: Support changing ingresscontroller load balancer scope #582

Miciah · 2021-03-27T06:57:47Z

Add support for changing ingresscontroller load balancer scope. On AWS and IBM cloud, this requires deleting the existing load balancer service and recreating it with the desired scope; if the administrator changes the scope on the ingresscontroller, the operator will report Progressing=True until the administrator either reverts the change or deletes the service. If the administrator deletes the service, then the operator will recreate the service with the desired scope. On Azure and GCP, it suffices to update the existing service's annotations.

This commit is based on #472, which was reverted by commit #514. Unlike the original implementation of this feature, the operator does not delete the service but instead leaves it up to the administrator to do so as needed.

pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Update scope if needed.
(ensureIngressController): Pass infraConfig to syncIngressControllerStatus.
pkg/operator/controller/ingress/load_balancer_service.go (externalLBAnnotations): New variable. Map platform type to the annotation for that platform that makes the load balancer external, if the platform requires an explicit annotation.
(desiredLoadBalancerService): Use new externalLBAnnotations variable to simplify logic.
(updateLoadBalancerService): Pass platform to loadBalancerServiceScopeChanged.
(loadBalancerServiceChanged): Add platform parameter. Check if the scope changed and if the platform supports changing scope without recreating the service, and update the appropriate annotations if so.
(loadBalancerServiceScopeChanged): New function. Check if the load balancer's scope changed.
(IsServiceInternal): New function. Return a Boolean value indicating whether the provided service is annotated to request an internal load-balancer
pkg/operator/controller/ingress/load_balancer_service_test.go (TestLoadBalancerServiceChanged): Update to pass platform status to loadBalancerServiceChanged.
pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add infraConfig parameter. Use infraConfig to get the platform type. Call the new computeIngressProgressingCondition function with the ingresscontroller, service, and platform to compute a "Progressing" status condition for the ingresscontroller.
(computeIngressProgressingCondition): New function. Compute a "Progressing" status condition. In particular, this status condition will indicate if it is necessary to delete the service so that it can be recreated with the updated scope.
pkg/operator/controller/ingress/status_test.go (TestComputeIngressProgressCondition): New test. Verify that computeIngressProgressingCondition returns the expected status condition.
pkg/operator/controller/status/controller.go (Reconcile): Pass ingresscontrollers to computeOperatorProgressingCondition.
(computeOperatorProgressingCondition): Report Progressing=True on the clusteroperator if any ingresscontroller reports Progressing=True.
pkg/operator/controller/status/controller_test.go (TestComputeOperatorProgressingCondition): Add test case where an ingresscontroller is progressing.
test/e2e/operator_test.go (TestScopeChange): New test. Verify that the operator performs the appropriate behavior for the platform on which the test is running when an ingresscontroller's scope is changed from the default external scope to internal and then back to external.

openshift-ci-robot · 2021-03-27T06:58:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

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

Miciah · 2021-03-28T00:22:57Z

/test e2e-azure-operator

Miciah · 2021-03-29T04:33:31Z

/test e2e-azure-operator

sgreene570 · 2021-03-31T18:52:06Z

pkg/operator/controller/ingress/load_balancer_service.go

+}
+
+// IsServiceInternal returns a Boolean indicating whether the provided service
+// Is annotated to request an internal load balancer.


nit

Suggested change

// Is annotated to request an internal load balancer.

// is annotated to request an internal load balancer.

Thanks! Fixed.

sgreene570 · 2021-03-31T18:58:35Z

pkg/operator/controller/ingress/status.go


+	platform, err := oputil.GetPlatformStatus(r.client, infraConfig)
+	if err != nil {
+		return fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %w", ic.Namespace, ic.Name, err)


is %w preferable for err over %v?

Yeah, IMO we should use %w in new code, and eventually update old code to use %w as well, so that we can unwrap errors and more easily check the type of an error (see https://blog.golang.org/go1.13-errors). Eventually, consistent use of %w will enable us to simplify some error-handling code (e.g., the retryable-error logic).

sgreene570 · 2021-03-31T19:07:31Z

pkg/operator/controller/ingress/status.go

+				message := fmt.Sprintf("Have load balancer with scope %q, want load balancer with scope %q.", haveScope, wantScope)
+				switch platform.Type {
+				case configv1.AWSPlatformType, configv1.IBMCloudPlatformType:
+					message = fmt.Sprintf("%s  You can delete the %s/%s service to proceed, and the service load-balancer will be deprovisioned and recreated.", message, service.Namespace, service.Name)


I think as is, this message could be confusing to a cluster administrator:

I think recreated in this context sort of implies that the new load-balancer will have the same hostname and IP as the last load-balancer. Should we explicitly state that when deleting the load-balancer service, a new load-balancer will be stood up that will most likely have a new hostname and IP?

Should we make it clear to the cluster administrator that they are free to "undo" their scope change without any reprocussions? I think some more details in general could be useful here as to not confuse a cluster administrator.

I've updated the message to convey these points. Does it look all right? (I hope it isn't too verbose.)

Looks fine to me 😁

Miciah · 2021-03-31T23:26:14Z

Rebased.

openshift-bot · 2021-06-30T02:04:36Z

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

openshift-bot · 2021-07-30T05:49:13Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

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

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-08-29T08:52:49Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-08-29T08:53:09Z

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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/test-infra repository.

Miciah · 2021-11-30T23:46:15Z

/reopen

openshift-ci · 2021-11-30T23:46:44Z

@Miciah: Reopened this PR.

Details

In response to this:

/reopen

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/test-infra repository.

frobware · 2021-12-13T21:36:02Z

/lgtm

openshift-ci · 2021-12-13T21:37:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, Miciah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,frobware]

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

openshift-bot · 2021-12-13T22:05:18Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-13T22:29:22Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-13T23:29:20Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-13T23:53:20Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T00:29:20Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T00:41:19Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T01:05:44Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T01:17:25Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2021-12-14T01:47:04Z

/hold
CI is failing because of openshift/build-machinery-go#56.

Miciah · 2021-12-14T16:35:29Z

#687 merged.
/retest

Miciah · 2021-12-14T16:46:11Z

/hold cancel

Miciah · 2021-12-14T20:47:27Z

Must-gather failed.
/test e2e-aws-operator

Miciah · 2021-12-14T22:43:02Z

TestConfigurableRouteNoConsumingUserNoRBAC and TestRouteAdmissionPolicy failed.
/test e2e-aws-operator

openshift-bot · 2021-12-15T00:26:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2021-12-15T02:26:07Z

Unschedulable worker node.
/test e2e-aws-operator

openshift-ci · 2021-12-15T06:18:49Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

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

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2021

openshift-ci-robot requested review from rfredette and sgreene570 March 27, 2021 06:58

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2021

Miciah force-pushed the mutable-publishing-scope-2 branch 2 times, most recently from ad1d844 to 5d08c79 Compare March 27, 2021 18:01

Miciah force-pushed the mutable-publishing-scope-2 branch from 5d08c79 to 1db1da3 Compare March 29, 2021 04:32

sgreene570 reviewed Mar 31, 2021

View reviewed changes

Miciah force-pushed the mutable-publishing-scope-2 branch from 1db1da3 to 1a45ce8 Compare March 31, 2021 23:24

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2021

Miciah force-pushed the mutable-publishing-scope-2 branch from 1a45ce8 to 7daeb6f Compare March 31, 2021 23:26

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2021

dustman9000 mentioned this pull request Apr 13, 2021

Support Updating Existing IngressController openshift/cloud-ingress-operator#118

Closed

openshift-ci bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 30, 2021

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 30, 2021

openshift-ci bot closed this Aug 29, 2021

Miciah mentioned this pull request Oct 4, 2021

ingress: Add mutable-publishing-scope enhancement openshift/enhancements#876

Merged

openshift-ci bot reopened this Nov 30, 2021

openshift-ci bot changed the title ~~WIP: Support changing ingresscontroller load balancer scope~~ NE-621: Support changing ingresscontroller load balancer scope Dec 13, 2021

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 Dec 13, 2021

openshift-ci bot assigned frobware Dec 13, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2021

openshift-merge-robot merged commit 8bcc62d into openshift:master Dec 15, 2021

Miciah mentioned this pull request Dec 16, 2021

Bug 1997226: Fix enabling PROXY protocol on an upgraded cluster #681

Merged

This was referenced Feb 11, 2022

OSDOCS-1768: document mutating .spec.endpointPublishingStrategy (new) openshift/openshift-docs#39451

Merged

Bug 2055470: Normalize the AWS internal LB annotation value #704

Merged

This was referenced May 11, 2022

[release-4.9] Bug 2084336: Fix enabling PROXY protocol on an upgraded cluster #757

Merged

[release-4.8] Bug 2084337: Fix enabling PROXY protocol on an upgraded cluster #758

Merged

	// Is annotated to request an internal load balancer.
	// is annotated to request an internal load balancer.

NE-621: Support changing ingresscontroller load balancer scope #582

NE-621: Support changing ingresscontroller load balancer scope #582

Uh oh!

Conversation

Miciah commented Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 27, 2021

Uh oh!

Miciah commented Mar 28, 2021

Uh oh!

Miciah commented Mar 29, 2021

Uh oh!

sgreene570 Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah commented Mar 31, 2021

Uh oh!

openshift-bot commented Jun 30, 2021

Uh oh!

openshift-bot commented Jul 30, 2021

Uh oh!

openshift-bot commented Aug 29, 2021

Uh oh!

openshift-ci bot commented Aug 29, 2021

Uh oh!

Miciah commented Nov 30, 2021

Uh oh!

openshift-ci bot commented Nov 30, 2021

Uh oh!

frobware commented Dec 13, 2021

Uh oh!

openshift-ci bot commented Dec 13, 2021

Uh oh!

openshift-bot commented Dec 13, 2021

Uh oh!

openshift-bot commented Dec 13, 2021

Uh oh!

openshift-bot commented Dec 13, 2021

Uh oh!

openshift-bot commented Dec 13, 2021

Uh oh!

openshift-bot commented Dec 14, 2021

Uh oh!

openshift-bot commented Dec 14, 2021

Uh oh!

openshift-bot commented Dec 14, 2021

Uh oh!

openshift-bot commented Dec 14, 2021

Uh oh!

Miciah commented Dec 14, 2021

Uh oh!

Miciah commented Dec 14, 2021

Uh oh!

Miciah commented Dec 14, 2021

Uh oh!

Miciah commented Dec 14, 2021

Uh oh!

Miciah commented Dec 14, 2021

Uh oh!

openshift-bot commented Dec 15, 2021

Uh oh!

Miciah commented Mar 27, 2021 •

edited

Loading