New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement HTTP Forwarded header policy API #410

Merged

openshift-merge-robot merged 3 commits into openshift:master from Miciah:implement-HTTP-Forwarded-header-policy-API

Jul 31, 2020

Contributor

Miciah commented Jun 10, 2020 •

edited

This commit resolves NE-318.

pkg/operator/controller/ingress/deployment.go (RouterForwardedHeadersPolicy): New constant with the name of the related environment variable.
(desiredRouterDeployment): Set the ROUTER_SET_FORWARDED_HEADERS environment variable as appropriate.
pkg/operator/controller/ingress/deployment_test.go (TestDesiredRouterDeployment): Verify that spec.httpHeaders.forwardedHeaderPolicy has the expected effect.
test/e2e/forwarded_header_policy_test.go: New file.
(buildEchoPod, buildEchoService, buildRoute, testRouteHeaders): New helper functions.
(testPodCount): New helper variable for testRouteHeaders.
(TestForwardedHeaderPolicyAppend, TestForwardedHeaderPolicyReplace, TestForwardedHeaderPolicyNever, TestForwardedHeaderPolicyIfNone): New tests.

Implements the following enhancement: openshift/enhancements#371

Corresponding router changes: openshift/router#134

Corresponding API changes: openshift/api#688

openshift-ci-robot added the do-not-merge/work-in-progress label

openshift-ci-robot requested review from ironcladlou and knobunc

June 10, 2020 18:03

openshift-ci-robot added the approved label

Miciah commented

View reviewed changes

test/e2e/operator_test.go Outdated Show resolved Hide resolved

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 39485ea to d22ca21 Compare

June 10, 2020 20:00

Contributor Author

Miciah commented Jun 11, 2020

Failed on TestForwardedHeaderPolicy, which is expected till openshift/router#134 merges.

sgreene570 reviewed

View reviewed changes

test/e2e/operator_test.go Outdated

+              	// Verify that we get the expected behavior if we set the policy to
+              	// "if-none".  We should always receive at least 1 X-Forwarded-For
+              	// header, and if the request specifies more than 1, we should receive
+              	// the number that are in the request.

Contributor

sgreene570 Jun 18, 2020

Suggested change

      
            	// the number that are in the request.
          
            	// the number of headers specified in the request.

Does that make more sense, or am I missing something?

Contributor Author

Miciah Jun 19, 2020

I've updated the wording to be more explicit.

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from d22ca21 to 4a228a9 Compare

June 19, 2020 23:00

openshift-ci-robot added the needs-rebase label

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 4a228a9 to 4704203 Compare

June 28, 2020 00:14

openshift-ci-robot removed the needs-rebase label

Contributor Author

Miciah commented Jun 28, 2020

Rebased.

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 4704203 to 2e2a75e Compare

June 28, 2020 02:48

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 2e2a75e to 2e46cd2 Compare

July 16, 2020 14:59

openshift-ci-robot added the needs-rebase label

Miciah changed the title ~~WIP: Implement HTTP Forwarded header policy API~~ Implement HTTP Forwarded header policy API

openshift-ci-robot removed the do-not-merge/work-in-progress label

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 2e46cd2 to 47dee8f Compare

July 16, 2020 15:05

openshift-ci-robot removed the needs-rebase label

sgreene570 reviewed

View reviewed changes

test/e2e/operator_test.go Outdated

+              					}
+              				}
+              			}
+              			return numMatches == expectedMatches, nil

Contributor

sgreene570 Jul 16, 2020 •

edited

Line 1263 checks for the case where numMatches > expectedMatches. Perhaps before the return on line 1268, we should check for numMatches > 0 && numMatches < expectedMatches and return an error if that condition is true. This way, for example, the polling loop would halt if only 1 x-forwarded-for header was found in the curl response when 2 were expected, and the test failure message would be more verbose. Thoughts? I may be overthinking this, but since you're calling testRoute several times I figured it would be worth mentioning.

Contributor Author

Miciah Jul 16, 2020

That makes sense. It is unlikely that the logs will have partial headers, so returning an error if 0 < numMatches < expectedMatches should safe to do.

Contributor

sgreene570 commented Jul 16, 2020

This PR depends on openshift/router#134 for CI to pass, correct?

Contributor Author

Miciah commented Jul 16, 2020

This PR depends on openshift/router#134 for CI to pass, correct?

You're right; I mixed up my PRs. I have one PR that depends on an unmerged API change and another PR that depends on an unmerged router change...

Miciah changed the title ~~Implement HTTP Forwarded header policy API~~ WIP: Implement HTTP Forwarded header policy API

openshift-ci-robot added the do-not-merge/work-in-progress label

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from c76af8c to d2155f0 Compare

July 17, 2020 00:40

Contributor Author

Miciah commented Jul 18, 2020

openshift/router#134 merged, so tests now should pass.
/test e2e-aws-operator

Contributor Author

Miciah commented Jul 18, 2020

/test e2e-aws-operator

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch 2 times, most recently from 306788c to 8ece45c Compare

July 19, 2020 01:41

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

5 similar comments

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

Contributor

openshift-bot commented Jul 30, 2020

/retest

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

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from e520d8d to 43b3d1a Compare

July 30, 2020 08:19

openshift-ci-robot removed the lgtm label

Contributor Author

Miciah commented Jul 30, 2020

/test e2e-aws-operator

1 similar comment

Contributor Author

Miciah commented Jul 30, 2020

/test e2e-aws-operator

Contributor Author

Miciah commented Jul 30, 2020

Provisioning failure.
/test e2e-aws-operator


          Implement HTTP Forwarded header policy API

167bcc2

This commit resolves NE-318.

https://issues.redhat.com/browse/NE-318

* pkg/operator/controller/ingress/deployment.go
(RouterForwardedHeadersPolicy): New constant with the name of the related
environment variable.
(desiredRouterDeployment): Set the ROUTER_SET_FORWARDED_HEADERS environment
variable as appropriate.
* pkg/operator/controller/ingress/deployment_test.go
(TestDesiredRouterDeployment): Verify that
spec.httpHeaders.forwardedHeaderPolicy has the expected effect.
* test/e2e/forwarded_header_policy_test.go: New file.
(buildEchoPod, buildEchoService, buildRoute, testRouteHeaders): New helper
functions.
(testPodCount): New helper variable for testRouteHeaders.
(TestForwardedHeaderPolicyAppend, TestForwardedHeaderPolicyReplace)
(TestForwardedHeaderPolicyNever, TestForwardedHeaderPolicyIfNone): New
tests.

Miciah force-pushed the implement-HTTP-Forwarded-header-policy-API branch from 43b3d1a to 167bcc2 Compare

July 30, 2020 18:23

Contributor Author

Miciah commented Jul 30, 2020

Both TestForwardedHeaderPolicyAppend and TestRouteHTTP2EnableAndDisableIngressController timed out. I'll increase the timeout for TestForwardedHeaderPolicyAppend and related tests, but it looks like the cluster was not in good order.

Contributor Author

Miciah commented Jul 30, 2020

No build log output?!
/test e2e-aws-operator

Contributor Author

Miciah commented Jul 30, 2020

/retest

Contributor Author

Miciah commented Jul 30, 2020

/test e2e-aws-operator


          Makefile: Increase timeout for test-e2e

476387a

* Makefile (test-e2e): Increase timeout to 1 hour.

Contributor Author

Miciah commented Jul 31, 2020

Last e2e-aws failure looks related to BZ1857928 – [sig-operator] an end user use OLM can subscribe to the cockroachdb operator.

e2e-aws-operator is reaching the 10-minute timeout. I'll try pushing a change to increase that timeout value.

Contributor Author

Miciah commented Jul 31, 2020

Seeing "an end user can use OLM can subscribe to the operator" fail again in e2e-aws, and seeing numerous timeouts in e2e-aws-operator that look like a generally unstable cluster.
/test e2e-aws
/test e2e-aws-operator

Contributor Author

Miciah commented Jul 31, 2020

Test container setup failed on an API failure.
/test e2e-aws

Contributor

frobware commented Jul 31, 2020

/lgtm

openshift-ci-robot assigned frobware

openshift-ci-robot added the lgtm label

Contributor

openshift-ci-robot commented Jul 31, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, 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:

~~OWNERS~~ [Miciah,frobware,sgreene570]

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

openshift-merge-robot merged commit bf76a15 into openshift:master

Miciah mentioned this pull request

OCPBUGS-17359: test/e2e: Don't use openshift/origin-node #970

Merged

This was referenced Oct 25, 2023

[release-4.13] OCPBUGS-22402: test/e2e: Don't use openshift/origin-node #991

Merged

[release-4.12] OCPBUGS-22432: test/e2e: Don't use openshift/origin-node #992

Merged

[release-4.11] OCPBUGS-22433: test/e2e: Don't use openshift/origin-node #993

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment