Cloud HA strategy should automatically use PROXY support when available. #84

ramr · 2018-12-11T02:05:14Z

Ref: https://jira.coreos.com/browse/NE-120

@ironcladlou @Miciah @pravisankar PTAL thx

pravisankar · 2018-12-11T03:08:56Z

assets/router/service-cloud.yaml

  # Name is set at runtime.
  namespace: openshift-ingress
+  annotations:
+    service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: "*"


This service is created for any cloud provider environments (AWS, GCP, etc.) but the new annotation is specific to AWS (or the annotation name is misleading?)
May be we could inject the annotation only when the platform is AWS? Similar to what we do here:

cluster-ingress-operator/cmd/cluster-ingress-operator/main.go

Line 102 in 1c6c23e

case ic.Platform.AWS != nil:

Yeah, the service and the deployment both need configured as a unit when we're using a cloud HA strategy and we're on a supported cloud.

ramr · 2018-12-12T00:00:10Z

/retest

ramr · 2018-12-12T00:00:39Z

@pravisankar addressed your comments. PTAL thx

pravisankar

Rest of the changes LGTM

pravisankar · 2018-12-12T02:39:47Z

pkg/manifests/manifests.go

 	s.Spec.Selector["router"] = name

+	if f.installConfig.Platform.AWS != nil {
+		if s.Annotations == nil {


This is not needed, s.Annotations is already initialized here:

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

Line 228 in 1924e28

if s.Annotations == nil {

D'oh yeah!! Am still zonked with all the meds. Fixing now ... Thanks.

ramr · 2018-12-12T04:30:20Z

hmm, the /test e2e-aws is failing because of proxy support enabled.

pravisankar

/lgtm

pravisankar · 2018-12-12T04:32:59Z

/hold
e2e-aws tests need to be fixed

ramr · 2018-12-12T04:37:09Z

It is like a catch-22-thingy. The feature is to automatically enable the proxy protocol on haproxy (and the aws LB) but the tests that run can't with proxy protocol enabled on haproxy I think. Going to retest a few times just to confirm.

ramr · 2018-12-12T04:37:15Z

/retest

ramr · 2018-12-12T05:48:43Z

/retest

ironcladlou · 2018-12-12T16:09:54Z

pkg/manifests/manifests.go

 	}
 	s.Spec.Selector["router"] = name

+	if f.installConfig.Platform.AWS != nil {


Shouldn't this be in RouterServiceCloud which is the LB service?

Yes it was -- that's the reason why was also initializing the annotations which I removed last night as per Ravi's comments but didn't look closely. It somehow looks like rebasing it moved it to the previous block/function. I fixed this but there's another issue will put in a separate comment.

ironcladlou · 2018-12-12T19:29:31Z

Could we do an e2e test for this?

ramr · 2018-12-12T23:34:13Z

@ironcladlou there's a problem with automatically enabling Proxy Protocol support on both haproxy and the AWS cloud load balancer.

The reason is when we enable this - the haproxy router only speaks the proxy protocol and doesn't understand http[s] - it expects the cloud load balancer to send a proxy protocol request wrapping the http request.

Now a consequence of this is that all the tests that use curl or equivalent http requests in golang code to talk directly to the haproxy router will fail. As the router's not going to understand that request. It is like a catch-22 on the environments where we run the unit tests and enable this. It makes sense for production env like settings (but for dev/ci it will cause problems).

My 2 bits would be to make this not be the default in the case tests are run.
Either let this setting be controlled by a ClusterIngressSpec field ala EnableLoadBalancerProxyProtocol == true. Or alternatively use the reverse like a env variable or something to always disable it for the ci/tests environments. Thoughts? Thx

ironcladlou · 2018-12-13T01:01:17Z

Assuming we want PROXY to be the default when supported (we can discuss that assumption separately) — what if during tests we talked to the router through as many supported layers as possible to maximize our test coverage? For example, if on the default AWS install we assert support for AWS ELB in front of the routers, then tests executing in that configuration could speak to the router through its ELB. Thoughts?

P.S., do you already have a list of which tests are trying to talk to the router directly?

ironcladlou · 2018-12-13T01:03:17Z

/cc @smarterclayton re: the test strategy

ramr · 2018-12-13T03:08:45Z

@ironcladlou answers below:

So my only concern re: PROXY protocol being default or not is that in the case it is the default,
there is really no way to turn that off in case someone wants to do. Well other than either remove
aws creds or change ClusterIngressSpec.HighAvailability. We have seen environments that have
one user-[micro]-service is talking another using the router others where they are actually
sending health checks for apps using HTTP internally in the cluster. With the PROXY protocol that's
not going to work - change needed to use the the external ELB.
We could talk thru' the AWS LB for the tests but since they are written at various times/people -- there's a lot of different tools those do end up using and so different fixes neeed.
Example: https://github.com/openshift/origin/blob/master/test/extended/router/router.go#L69 use the url helper but https://github.com/openshift/origin/blob/master/test/extended/router/scoped.go#L275 uses curl directly etc ... not clean!
So as re: list of tests - most of the routers extended tests are in
https://github.com/openshift/origin/tree/master/test/extended/router
but I don't think that would be the entire set. I just ran a simple grep curl origin/test as an example
and showed up a few others ala: https://github.com/openshift/origin/blob/master/test/extended/builds/service.go#L19

Thx

ramr · 2018-12-17T21:43:49Z

Depends on openshift/origin#21680 to fix a few of the broken tests ...

And fixes as per review comments.

…p the test once we fix the internal router service selection bug.

…g api.

ramr · 2019-01-15T04:57:16Z

Rebased this but I think I still need some help with testing this ... I can't boot up an installer cluster with the HEAD - I can only boot up a cluster with v0.90 but that doesn't have the Infrastructure changes. Plus I think origin router tests may need a change.

ramr · 2019-01-15T05:00:35Z

/retest

Flake: error: unable to upload the new image manifest: received unexpected HTTP status: 504 Gateway Time-out

knobunc · 2019-01-16T21:11:59Z

/retest

ramr · 2019-01-16T21:31:33Z

FYI, the router tests will fail until they send requests to the ELB rather than the router service (since the router service will now only "speak" the proxy protocol). This needs openshift/origin#21799 to merge.

ramr · 2019-01-17T20:09:08Z

/retest

ramr · 2019-01-18T00:27:13Z

/retest

ramr · 2019-01-23T00:11:49Z

/test all

ironcladlou · 2019-01-23T18:47:28Z

Looks like this is once again good to go.

/hold cancel

ironcladlou · 2019-01-23T18:47:35Z

/lgtm

openshift-ci-robot · 2019-01-23T18:47:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah, pravisankar, ramr

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,ironcladlou,pravisankar,ramr]

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

ironcladlou · 2019-01-23T20:05:17Z

/retest

ramr · 2019-01-23T22:54:07Z

/retest

last error: "waiting for openshift-console URL: context deadline exceeded"

openshift-bot · 2019-01-24T05:23:33Z

/retest

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

openshift-bot · 2019-01-24T13:26:39Z

/retest

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

ironcladlou · 2019-01-24T18:54:25Z

/retest

ironcladlou · 2019-01-25T00:53:57Z

/retest

openshift-bot · 2019-01-25T09:36:38Z

/retest

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

openshift-bot · 2019-01-25T11:37:37Z

/retest

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

openshift-bot · 2019-01-25T13:38:38Z

/retest

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

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 11, 2018

openshift-ci-robot requested review from Miciah and knobunc December 11, 2018 02:05

pravisankar reviewed Dec 11, 2018

View reviewed changes

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

ramr force-pushed the ne-120 branch from 2fe0dae to 1924e28 Compare December 11, 2018 23:08

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

pravisankar suggested changes Dec 12, 2018

View reviewed changes

ramr force-pushed the ne-120 branch from 1924e28 to 49f39bb Compare December 12, 2018 03:12

openshift-ci-robot assigned pravisankar Dec 12, 2018

pravisankar approved these changes Dec 12, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2018

ironcladlou reviewed Dec 12, 2018

View reviewed changes

ramr force-pushed the ne-120 branch from 49f39bb to f7981eb Compare December 12, 2018 23:23

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018

openshift-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 18, 2018

ramr added 5 commits January 14, 2019 20:42

Cloud HA strategy should automatically use PROXY support when available.

9cef3ff

And fixes as per review comments.

Add e2e test for proxy protocol + add some details how we can shore u…

1b7b0ec

…p the test once we fix the internal router service selection bug.

Fixes based on Miciah's review comments.

17f0a51

Changes (+ rebased for conflicts) to use the new infrastructure confi…

685478b

…g api.

Fix parameters changed to NewFactory - rebase picks up old changes.

ae89da0

ramr force-pushed the ne-120 branch from 8b0f386 to ae89da0 Compare January 15, 2019 04:54

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2019

ramr mentioned this pull request Jan 15, 2019

Fix the router tests host name to use the external load balancer service openshift/origin#21799

Merged

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2019

openshift-merge-robot merged commit 9fb155f into openshift:master Jan 25, 2019

Cloud HA strategy should automatically use PROXY support when available. #84

Cloud HA strategy should automatically use PROXY support when available. #84

Uh oh!

Conversation

ramr commented Dec 11, 2018

Uh oh!

pravisankar Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

ironcladlou Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

pravisankar Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

ramr Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

pravisankar left a comment

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Dec 12, 2018

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

ironcladlou Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

ramr Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Dec 12, 2018

Uh oh!

ramr commented Dec 12, 2018

Uh oh!

ironcladlou commented Dec 13, 2018

Uh oh!

ironcladlou commented Dec 13, 2018

Uh oh!

ramr commented Dec 13, 2018

Uh oh!

ramr commented Dec 17, 2018

Uh oh!

ramr commented Jan 15, 2019

Uh oh!

ramr commented Jan 15, 2019

Uh oh!

knobunc commented Jan 16, 2019

Uh oh!

ramr commented Jan 16, 2019

Uh oh!

ramr commented Jan 17, 2019

Uh oh!

ramr commented Jan 18, 2019

Uh oh!

ramr commented Jan 23, 2019

Uh oh!

ironcladlou commented Jan 23, 2019

Uh oh!

ironcladlou commented Jan 23, 2019

Uh oh!

openshift-ci-robot commented Jan 23, 2019

Uh oh!

ironcladlou commented Jan 23, 2019

Uh oh!

ramr commented Jan 23, 2019

Uh oh!