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

Miciah · 2022-05-11T22:08:12Z

This is a manual cherry-pick of #681 and #691. #582 introduced conflicts that required resolution.

`setDefaultPublishingStrategy`: Reformat with `switch`

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.

`setDefaultPublishingStrategy`: Fix PROXY protocol

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 PR 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.

pkg/operator/controller/ingress/controller.go (setDefaultPublishingStrategy): Fix logic to properly handle null values for status.endpointPublishingStrategy.hostNetwork or status.endpointPublishingStrategy.nodePort.

`setDefaultPublishingStrategy`: Deep copy, tests

Make a copy of spec.endpointPublishingStrategy to avoid mutating it, and add unit tests for setDefaultDomain and setDefaultPublishingStrategy.

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.

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 · 2022-05-11T22:08:22Z

@Miciah: This pull request references Bugzilla bug 2084336, 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

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1997226 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1997226 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0, 4.10.z
bug has dependents

Requesting review from QA contact:
/cc @lihongan

In response to this:

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

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.

candita · 2022-05-12T17:16:35Z

Step e2e-aws-operator-gather-must-gather failed after 10m30s.

/test e2e-aws-operator

candita · 2022-05-12T19:30:52Z

pkg/operator/controller/ingress/controller.go

 		statusNP := ic.Status.EndpointPublishingStrategy.NodePort
 		specNP := effectiveStrategy.NodePort
-		if specNP != nil && statusNP != nil && specNP.Protocol != statusNP.Protocol {
+		if specNP != nil && specNP.Protocol != statusNP.Protocol {


I still have a question about this - if someone sets ic.Spec.EndpointPublishingStrategy.NodePort to nil (or removes it), then it won't update ic.Status nor return true here, will it?

Line 375 initializes ic.Spec.EndpointPublishingStrategy.NodePort if it is nil. In fact, for that reason, we probably don't need the specNP != nil check anyway.

cluster-ingress-operator/pkg/operator/controller/ingress/controller.go

Line 351 in 8b258e5

effectiveStrategy := ic.Spec.EndpointPublishingStrategy

cluster-ingress-operator/pkg/operator/controller/ingress/controller.go

Line 366 in 8b258e5

switch effectiveStrategy.Type {

cluster-ingress-operator/pkg/operator/controller/ingress/controller.go

Lines 373 to 375 in 8b258e5

case operatorv1.NodePortServiceStrategyType:

if effectiveStrategy.NodePort == nil {

effectiveStrategy.NodePort = &operatorv1.NodePortStrategy{}

Okay. That had a code smell and I guess this explains it.

candita · 2022-05-12T20:34:27Z

I'll leave it up to you whether you'd like to add the unit tests in this PR.

/lgtm

openshift-ci · 2022-05-12T20:35:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [Miciah,candita]

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

Miciah · 2022-06-03T14:35:23Z

Added #691 for the unit tests. After some discussion, we decided that the deepcopy that #691 adds is safe to backport.

openshift-ci · 2022-06-03T14:36:54Z

@Miciah: This pull request references Bugzilla bug 2084336, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 1997226 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 1997226 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0, 4.10.z
bug has dependents

Requesting review from QA contact:
/cc @quarterpin

In response to this:

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

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.

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.

lihongan · 2022-06-06T01:36:57Z

/retest

lihongan · 2022-06-06T01:37:24Z

/label cherry-pick-approved,

openshift-ci · 2022-06-06T01:37:26Z

@lihongan: The label(s) /label cherry-pick-approved, cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, backport-risk-assessed, cherry-pick-approved

In response to this:

/label cherry-pick-approved,

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.

lihongan · 2022-06-06T01:37:41Z

/label cherry-pick-approved

quarterpin · 2022-06-29T08:48:39Z

/label qe-approved
Verified via pre-merge workflow. More details can be found in https://bugzilla.redhat.com/show_bug.cgi?id=2084336#c1.

candita · 2022-07-06T17:43:34Z

/assign candita

candita · 2022-07-06T18:00:06Z

RUN TestHTTPHeaderCapture
operator_test.go:1682: failed to find output:
.....
operator_test.go:1689: failed to observe the expected log message: timed out waiting for the condition
62443
panic.go:613: deleted ingresscontroller headercapture
62444
--- FAIL: TestHTTPHeaderCapture (270.57s)

/test e2e-aws-operator

candita · 2022-07-06T20:59:28Z

From the must-gather log:

ClusterOperators:
clusteroperator/authentication is degraded because APIServerDeploymentDegraded: 1 of 3 requested instances are unavailable for apiserver.openshift-oauth-apiserver ()
OAuthServerDeploymentDegraded: 1 of 3 requested instances are unavailable for oauth-openshift.openshift-authentication ()
clusteroperator/machine-config is not upgradeable because One or more machine config pools are updating, please see oc get mcp for further details
clusteroperator/openshift-apiserver is degraded because APIServerDeploymentDegraded: 1 of 3 requested instances are unavailable for apiserver.openshift-apiserver ()

/test e2e-aws-operator

candita · 2022-07-06T22:54:55Z

/lgtm

openshift-ci · 2022-07-07T01:20:10Z

@Miciah: all tests passed!

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

openshift-ci · 2022-07-07T01:23:41Z

@Miciah: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#757

Bugzilla bug 2084336 has been moved to the MODIFIED state.

In response to this:

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

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 Masters added 2 commits May 11, 2022 18:05

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 11, 2022

openshift-ci bot requested review from lihongan, candita and ironcladlou May 11, 2022 22:08

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022

candita reviewed May 12, 2022

View reviewed changes

openshift-ci bot assigned candita May 12, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2022

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2022

Miciah added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Jun 3, 2022

openshift-ci bot assigned lihongan, melvinjoseph86, quarterpin and ShudiLi Jun 3, 2022

openshift-ci bot requested review from quarterpin and removed request for ironcladlou June 3, 2022 14:36

Miciah force-pushed the cherry-pick-681-to-release-4.9 branch from acff139 to c19f75e Compare June 3, 2022 14:47

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 6, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2022

openshift-ci bot merged commit a3a3396 into openshift:release-4.9 Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Miciah commented May 11, 2022 •

edited

openshift-ci bot commented May 11, 2022

candita commented May 12, 2022

candita May 12, 2022

Miciah May 12, 2022

candita May 12, 2022

candita commented May 12, 2022

openshift-ci bot commented May 12, 2022

Miciah commented Jun 3, 2022

openshift-ci bot commented Jun 3, 2022

lihongan commented Jun 6, 2022

lihongan commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

lihongan commented Jun 6, 2022

quarterpin commented Jun 29, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

openshift-ci bot commented Jul 7, 2022

openshift-ci bot commented Jul 7, 2022

	case operatorv1.NodePortServiceStrategyType:
	if effectiveStrategy.NodePort == nil {
	effectiveStrategy.NodePort = &operatorv1.NodePortStrategy{}

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

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

Conversation

Miciah commented May 11, 2022 • edited

setDefaultPublishingStrategy: Reformat with switch

setDefaultPublishingStrategy: Fix PROXY protocol

setDefaultPublishingStrategy: Deep copy, tests

openshift-ci bot commented May 11, 2022

candita commented May 12, 2022

candita May 12, 2022

Choose a reason for hiding this comment

Miciah May 12, 2022

Choose a reason for hiding this comment

candita May 12, 2022

Choose a reason for hiding this comment

candita commented May 12, 2022

openshift-ci bot commented May 12, 2022

Miciah commented Jun 3, 2022

openshift-ci bot commented Jun 3, 2022

lihongan commented Jun 6, 2022

lihongan commented Jun 6, 2022

openshift-ci bot commented Jun 6, 2022

lihongan commented Jun 6, 2022

quarterpin commented Jun 29, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

candita commented Jul 6, 2022

openshift-ci bot commented Jul 7, 2022

openshift-ci bot commented Jul 7, 2022

Miciah commented May 11, 2022 •

edited

`setDefaultPublishingStrategy`: Reformat with `switch`

`setDefaultPublishingStrategy`: Fix PROXY protocol

`setDefaultPublishingStrategy`: Deep copy, tests