Bug 1904582: Assume ingresscontroller is external absent status #502
Conversation
If an ingresscontroller's status.endpointPublishingStrategy.type field is "LoadBalancerService" but status.endpointPublishingStrategy.loadBalancer is nil, assume the ingresscontroller should have an external load balancer. Commit 8cd622f added support for internal scope for load balancers. In doing so, it changed the operator logic to initialize status.endpointPublishingStrategy.loadBalancer to indicate the scope when an ingresscontroller was created, using the corresponding fields in the ingresscontroller's spec to determine the scope. The same commit also changed operator logic to assume that the ingresscontroller has internal scope if status.endpointPublishingStrategy.loadBalancer is nil. However, if an ingresscontroller already exists, the operator does not update status.endpointPublishingStrategy.loadBalancer. This commit was made in OpenShift 4.2, which means that if a cluster was upgraded from 4.1, any ingresscontrollers that were created prior to the upgrade would not have status.endpointPublishingStrategy.loadBalancer set. Subsequently, commit 982682f made the ingresscontroller's scope mutable, meaning if an ingresscontroller has an external load balancer and status.endpointPublishingStrategy.loadBalancer indicates that the ingresscontroller should have an internal load balancer, then the operator changes the load balancer from external to internal. In combination, those two commits cause the operator to change an ingresscontroller's load balancer from external to internal if the cluster has been upgraded from 4.1 → 4.2 → ... → 4.6 and the ingresscontroller was originally created on OpenShift 4.1. This commit rectifies the situation by amending the logic that was added in commit 8cd622f to assume that a nil value for status.endpointPublishingStrategy.loadBalancer means that the load balancer should be external. This assumption is valid because status.endpointPublishingStrategy.loadBalancer is nil exactly when the ingresscontroller was created on an OpenShift 4.1 cluster, and external scope was the only supported option on OpenShift 4.1. * pkg/operator/controller/ingress/load_balancer_service.go (desiredLoadBalancerService): Assume nil status.endpointPublishingStrategy.loadBalancer means external scope.
openshift-ci-robot
added
the
approved
|
We'll need this backported to 4.6 (which made scope mutable) but no further. |
|
@Miciah: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you. In response to this:
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. |
|
/retitle Bug 1904582: Assume ingresscontroller is external absent status |
openshift-ci-robot
changed the title
|
@Miciah: This pull request references Bugzilla bug 1904582, which is invalid:
Comment In response to this:
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. |
openshift-ci-robot
added
bugzilla/severity-unspecified
|
/bugzilla refresh |
|
@Miciah: An error was encountered adding this pull request to the external tracker bugs for bug 1904582 on the Bugzilla server at https://bugzilla.redhat.com:
In response to this:
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. |
|
/bugzilla refresh |
|
@Miciah: An error was encountered adding this pull request to the external tracker bugs for bug 1904582 on the Bugzilla server at https://bugzilla.redhat.com:
In response to this:
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. |
|
LGTM once the bugzilla is linked |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/severity-urgent
|
@sdodson: This pull request references Bugzilla bug 1904582, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Does this open the door for the potential reverse problem? If someone manually changed a 4.1.z cluster's ingresscontroller to have internal load balancer after the cluster was initially installed, will this inadvertently change that back to an external load balancer? Or would any manual change to point the apps domain at an internal load balancer have been reverted by the ingress operator anyway, before this change? |
No, if the administrator changed the scope, then that means there is an explicit value for
What kind of manual changes do you mean? Directly modifying the |
And to complete that thought, #472 means that changes to cluster-ingress-operator/pkg/operator/controller/ingress/controller.go Lines 365 to 367 in f2d7665 |
I was referring to a user modifying the cloud resources directly for started-in-4.1 clusters where users could not choose internal versus external load balancers via the API. Back then if you wanted an internal only load balancer, your only option was to modify the cloud resources directly, right? And I don't think we reconciled manual changes away? I know of no way to determine what, if any, clusters would have done that and if it was never a suggested or supported option for older clusters than my concern is probably irrelevant. |
|
@Miciah: All pull requests linked via external trackers have merged: Bugzilla bug 1904582 has been moved to the MODIFIED state. In response to this:
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: new pull request created: #503 In response to this:
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. |
That's true, we didn't reconcile away user-added annotations on the service, but as you say, we have never suggested or supported administrators doing that. In general, directly modifying operator-managed resources is unsupported. |
|
There have been cases where we suggested setting |
If an ingresscontroller's
status.endpointPublishingStrategy.typefield isLoadBalancerServicebutstatus.endpointPublishingStrategy.loadBalancerisnil, assume the ingresscontroller should have an external load balancer.#254 added support for internal scope for load balancers. In doing so, it changed the operator logic to initialize
status.endpointPublishingStrategy.loadBalancerto indicate the scope when an ingresscontroller was created, using the corresponding fields in the ingresscontroller'sspecto determine the scope. The same change also changed operator logic to assume that the ingresscontroller has internal scope ifstatus.endpointPublishingStrategy.loadBalancerisnil. However, if an ingresscontroller already exists, the operator does not updatestatus.endpointPublishingStrategy.loadBalancer. This change was made in OpenShift 4.2, which means that if a cluster was upgraded from 4.1, any ingresscontrollers that were created prior to the upgrade would not havestatus.endpointPublishingStrategy.loadBalancerset.Subsequently, #472 made the ingresscontroller's scope mutable, meaning if an ingresscontroller has an external load balancer and
status.endpointPublishingStrategy.loadBalancerindicates that the ingresscontroller should have an internal load balancer, then the operator changes the load balancer from external to internal.In combination, those two changes cause the operator to change an ingresscontroller's load balancer from external to internal if the cluster has been upgraded from 4.1 → 4.2 → ... → 4.6 and the ingresscontroller was originally created on OpenShift 4.1.
This PR rectifies the situation by amending the logic that was added in #254 to assume that a
nilvalue forstatus.endpointPublishingStrategy.loadBalancermeans that the load balancer should be external. This assumption is valid becausestatus.endpointPublishingStrategy.loadBalancerisnilexactly when the ingresscontroller was created on an OpenShift 4.1 cluster, and external scope was the only supported option on OpenShift 4.1.pkg/operator/controller/ingress/load_balancer_service.go(desiredLoadBalancerService): Assumenilstatus.endpointPublishingStrategy.loadBalancermeans external scope.