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

Miciah · 2021-11-30T21:20:19Z

`setDefaultPublishingStrategy`: Reformat with `switch`

Refactor the update logic in setDefaultPublishingStrategy. Also, fix setDefaultPublishingStrategy to return true if the scope changed. (Nothing uses this return value, so this change is only for correctness.)

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.

@miheer, does this look like it will resolve the issue?

openshift-ci · 2021-11-30T21:21:26Z

@Miciah: An error was encountered querying GitHub for users with public email ([email protected]) for bug 1997226 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

Bug 1997226: 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.

miheer · 2021-12-01T07:09:13Z

pkg/operator/controller/ingress/controller.go

+			// If the ProviderParameters field does not exist for spec or status,
+			// just propagate (or remove) ProviderParameters in its entirety
+			// (as long as GCP parameters are specified one way or the other).
+			if specLB.ProviderParameters == nil && statusLB.ProviderParameters != nil && statusLB.ProviderParameters.GCP != nil ||


@Miciah just to understand... Why do we add conditions specific for GCP only ?

GCP is the only platform where the provider parameters can be changed without deleting and recreating the service load-balancer. In particular, GCP has the "global client access" option that can be turned on and off. The only other platform with provider parameters is AWS, which has an option to specify whether a Classic ELB or an NLB should be provisioned, and changing that option requires deleting and recreating the service load-balancer.

Refactor the update logic in setDefaultPublishingStrategy. Also, fix setDefaultPublishingStrategy to return true if the scope changed. (Nothing uses this return value, so this change is only for correctness.) * 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.

Miciah · 2021-12-16T19:03:58Z

Rebased to resolve the conflict from #582, and also fixed a tiny, non-consequential issue related to the return value of setDefaultPublishingStrategy when the scope has been change.

openshift-ci · 2021-12-16T19:04:25Z

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

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @quarterpin

Details

In response to this:

Bug 1997226: 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 · 2021-12-17T15:17:33Z

Must-gather failed (couldn't connect to the API).
/test e2e-aws-single-node

miheer · 2021-12-17T16:45:03Z

@Miciah we don't need unit tests for this ? Or may we can do it in a followup PR ?

miheer · 2021-12-17T16:59:50Z

/lgtm

miheer · 2021-12-17T16:59:59Z

/approve

openshift-ci · 2021-12-17T17:01:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,miheer]

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

candita · 2021-12-17T18:18:32Z

pkg/operator/controller/ingress/controller.go

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


Is it possible to try to set NodePort to nil? Then if specIP == nil, we might want to delete statusNP?

candita · 2021-12-17T18:26:52Z

pkg/operator/controller/ingress/controller.go

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


Suggested change

statusNP.Protocol = specNP.Protocol

ic.Status.EndpointPublishingStrategy.NodePort.Protocol = specNP.Protocol

candita · 2021-12-17T18:27:26Z

pkg/operator/controller/ingress/controller.go

+		statusHN := ic.Status.EndpointPublishingStrategy.HostNetwork
+		specHN := effectiveStrategy.HostNetwork
+		if specHN != nil && specHN.Protocol != statusHN.Protocol {
+			statusHN.Protocol = specHN.Protocol


Suggested change

statusHN.Protocol = specHN.Protocol

ic.Status.EndpointPublishingStrategy.HostNetwork.Protocol = specHN.Protocol

ic.Status.EndpointPublishingStrategy.HostNetwork is a pointer, so the following:

statusHN := ic.Status.EndpointPublishingStrategy.HostNetwork statusHN.Protocol = specHN.Protocol

is equivalent to the following:

ic.Status.EndpointPublishingStrategy.HostNetwork.Protocol = specHN.Protocol

I'll rustle up some unit tests.

It would be nice to see the same style throughout this switch then.

candita · 2021-12-17T18:30:21Z

@Miciah I made some comments. Can you add a unit test for this?

openshift-bot · 2021-12-17T18:46:39Z

/retest-required

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

openshift-ci · 2021-12-17T21:02:02Z

@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 · 2021-12-17T21:02:29Z

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

openshift/cluster-ingress-operator#681

Bugzilla bug 1997226 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1997226: 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 · 2022-03-16T15:48:55Z

/cherry-pick release-4.9
/cherry-pick release-4.8

openshift-cherrypick-robot · 2022-03-16T16:19:35Z

@Miciah: #681 failed to apply on top of branch "release-4.9":

Applying: setDefaultPublishingStrategy: Reformat with switch
Using index info to reconstruct a base tree...
M	pkg/operator/controller/ingress/controller.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/controller/ingress/controller.go
CONFLICT (content): Merge conflict in pkg/operator/controller/ingress/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 setDefaultPublishingStrategy: Reformat with switch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.9
/cherry-pick release-4.8

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 bot requested review from alebedev87 and ironcladlou November 30, 2021 21:30

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2021

miheer reviewed Dec 1, 2021

View reviewed changes

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

Miciah Masters added 2 commits December 16, 2021 14:01

Miciah force-pushed the BZ1997226-setDefaultPublishingStrategy-fix-PROXY-protocol branch from ded5193 to 4bfff11 Compare December 16, 2021 19:03

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

openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Dec 16, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 16, 2021

openshift-ci bot requested a review from quarterpin December 16, 2021 19:04

openshift-ci bot assigned miheer Dec 17, 2021

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

candita reviewed Dec 17, 2021

View reviewed changes

openshift-merge-robot merged commit 10ea22d into openshift:master Dec 17, 2021

Miciah mentioned this pull request Dec 18, 2021

Bug 1997226: setDefaultPublishingStrategy: Deep copy, tests #691

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

	statusNP.Protocol = specNP.Protocol
	ic.Status.EndpointPublishingStrategy.NodePort.Protocol = specNP.Protocol

	statusHN.Protocol = specHN.Protocol
	ic.Status.EndpointPublishingStrategy.HostNetwork.Protocol = specHN.Protocol

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

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

Uh oh!

Conversation

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

Uh oh!

setDefaultPublishingStrategy: Reformat with switch

setDefaultPublishingStrategy: Fix PROXY protocol

Uh oh!

openshift-ci bot commented Nov 30, 2021

Uh oh!

miheer Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah commented Dec 16, 2021

Uh oh!

openshift-ci bot commented Dec 16, 2021

Uh oh!

Miciah commented Dec 17, 2021

Uh oh!

miheer commented Dec 17, 2021

Uh oh!

miheer commented Dec 17, 2021

Uh oh!

miheer commented Dec 17, 2021

Uh oh!

openshift-ci bot commented Dec 17, 2021

Uh oh!

candita Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

candita Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

candita Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

candita Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

candita commented Dec 17, 2021

Uh oh!

openshift-bot commented Dec 17, 2021

Uh oh!

openshift-ci bot commented Dec 17, 2021

Uh oh!

openshift-ci bot commented Dec 17, 2021

Uh oh!

Miciah commented Mar 16, 2022

Uh oh!

openshift-cherrypick-robot commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Miciah commented Nov 30, 2021 •

edited

Loading

`setDefaultPublishingStrategy`: Reformat with `switch`

`setDefaultPublishingStrategy`: Fix PROXY protocol

miheer Dec 1, 2021 •

edited

Loading

candita Dec 17, 2021 •

edited

Loading