[release-4.8] Bug 2084337: Fix enabling PROXY protocol on an upgraded cluster #758
Conversation
Refactor the update logic in setDefaultPublishingStrategy. * pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Use a switch statement for the update logic so that the logic only looks at parameters related to the selected endpoint publishing strategy type.
Fix the update logic in setDefaultPublishingStrategy so that updates are properly handled when status.endpointPublishingStrategy.hostNetwork or status.endpointPublishingStrategy.nodePort is null. Before OpenShift 4.8, the IngressController API did not have any fields under the status.endpointPublishingStrategy.hostNetwork and status.endpointPublishingStrategy.nodePort fields. As result, these fields could be null even if the spec.endpointPublishingStrategy.type field was set to "HostNetwork" or "NodePortService". OpenShift 4.8 added status.endpointPublishingStrategy.hostNetwork.protocol and status.endpointPublishingStrategy.nodePort.protocol fields, and the operator now sets default values for these fields when the operator admits or re-admits an ingresscontroller that specifies the "HostNetwork" or "NodePortService" strategy type, respectively. However, a cluster that was upgraded from a version of OpenShift before 4.8 could have an already admitted ingresscontroller with null values for status.endpointPublishingStrategy.hostNetwork and status.endpointPublishingStrategy.nodePort even when ingresscontroller specifies the "HostNetwork" or "NodePortService" strategy type. In this case, the operator ignored updates to the spec.endpointPublishingStrategy.hostNetwork.protocol or spec.endpointPublishingStrategy.nodePort.protocol fields. This commit fixes the update logic so that it correctly updates the status.endpointPublishingStrategy.hostNetwork.protocol or status.endpointPublishingStrategy.nodePort.protocol field when status.endpointPublishingStrategy.hostNetwork or status.endpointPublishingStrategy.nodePort is null, the spec.endpointPublishingStrategy.hostNetwork.protocol or spec.endpointPublishingStrategy.nodePort.protocol field is set, and the strategy type is "HostNetwork" or "NodePortService", respectively. This commit fixes bug 1997226. https://bugzilla.redhat.com/show_bug.cgi?id=1997226 * pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Fix logic to properly handle null values for status.endpointPublishingStrategy.hostNetwork or status.endpointPublishingStrategy.nodePort.
openshift-ci
bot
added
bugzilla/severity-high
|
@Miciah: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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. |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required |
Make a copy of spec.endpointPublishingStrategy to avoid mutating it, and add unit tests for setDefaultDomain and setDefaultPublishingStrategy. Follow-up to commit 4bfff11. * pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Use a deep copy of ic.Spec.EndpointPublishingStrategy. * pkg/operator/controller/ingress/controller_test.go (TestSetDefaultDomain) (TestSetDefaultPublishingStrategySetsPlatformDefaults) (TestSetDefaultPublishingStrategyHandlesUpdates): New tests.
|
@Miciah: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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
added
the
backport-risk-assessed
|
/assign candace |
|
@candita: GitHub didn't allow me to assign the following users: candace. Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn 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. |
|
/assign candita |
|
/lgtm |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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. |
|
|
/test e2e-aws-single-node |
|
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is invalid:
Comment DetailsIn 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 Recalculating validity in case the underlying Bugzilla bug has changed. |
openshift-ci
bot
added
bugzilla/valid-bug
|
@openshift-bot: This pull request references Bugzilla bug 2084337, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@Miciah: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/label cherry-pick-approved |
openshift-ci
bot
added
the
cherry-pick-approved
|
@Miciah: All pull requests linked via external trackers have merged: Bugzilla bug 2084337 has been moved to the MODIFIED state. DetailsIn 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. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-ingress-operator-container-v4.8.0-202311261141.p0.g7163d38.assembly.stream for distgit ose-cluster-ingress-operator. |
8 participants
This is a manual cherry-pick of #681 and #691. #582 introduced conflicts that required resolution.
setDefaultPublishingStrategy: Reformat withswitchRefactor the update logic in
setDefaultPublishingStrategy.pkg/operator/controller/ingress/controller.go(setDefaultPublishingStrategy): Use a switch statement for the update logic so that the logic only looks at parameters related to the selected endpoint publishing strategy type.setDefaultPublishingStrategy: Fix PROXY protocolFix the update logic in
setDefaultPublishingStrategyso that updates are properly handled whenstatus.endpointPublishingStrategy.hostNetworkorstatus.endpointPublishingStrategy.nodePortis null.Before OpenShift 4.8, the IngressController API did not have any fields under the
status.endpointPublishingStrategy.hostNetworkandstatus.endpointPublishingStrategy.nodePortfields. As result, these fields could be null even if thespec.endpointPublishingStrategy.typefield was set to "HostNetwork" or "NodePortService".OpenShift 4.8 added
status.endpointPublishingStrategy.hostNetwork.protocolandstatus.endpointPublishingStrategy.nodePort.protocolfields, and the operator now sets default values for these fields when the operator admits or re-admits an ingresscontroller that specifies the "HostNetwork" or "NodePortService" strategy type, respectively.However, a cluster that was upgraded from a version of OpenShift before 4.8 could have an already admitted ingresscontroller with null values for
status.endpointPublishingStrategy.hostNetworkandstatus.endpointPublishingStrategy.nodePorteven when ingresscontroller specifies the "HostNetwork" or "NodePortService" strategy type.In this case, the operator ignored updates to the
spec.endpointPublishingStrategy.hostNetwork.protocolorspec.endpointPublishingStrategy.nodePort.protocolfields.This PR fixes the update logic so that it correctly updates the
status.endpointPublishingStrategy.hostNetwork.protocolorstatus.endpointPublishingStrategy.nodePort.protocolfield whenstatus.endpointPublishingStrategy.hostNetworkorstatus.endpointPublishingStrategy.nodePortis null, thespec.endpointPublishingStrategy.hostNetwork.protocolorspec.endpointPublishingStrategy.nodePort.protocolfield is set, and the strategy type is "HostNetwork" or "NodePortService", respectively.pkg/operator/controller/ingress/controller.go(setDefaultPublishingStrategy): Fix logic to properly handle null values forstatus.endpointPublishingStrategy.hostNetworkorstatus.endpointPublishingStrategy.nodePort.setDefaultPublishingStrategy: Deep copy, testsMake a copy of
spec.endpointPublishingStrategyto avoid mutating it, and add unit tests forsetDefaultDomainandsetDefaultPublishingStrategy.pkg/operator/controller/ingress/controller.go(setDefaultPublishingStrategy): Use a deep copy ofic.Spec.EndpointPublishingStrategy.pkg/operator/controller/ingress/controller_test.go(TestSetDefaultDomain,TestSetDefaultPublishingStrategySetsPlatformDefaults,TestSetDefaultPublishingStrategyHandlesUpdates): New tests.