Add http/2 support for haproxy router. #19968

ramr · Jun 11, 2018

Adds http2 support for haproxy.
Ref: https://trello.com/c/qzvlzuyx/27-3-implement-router-http-2-support-terminating-at-the-router-router

To run:

$ oc adm router --latest-images=true
$ oc set env dc/router ROUTER_ENABLE_HTTP2=true
$ #  add edge secured and/or reencrypt routes:
$ #  Example: use routes from https://github.com/ramr/nodejs-header-echo

Test:

$ curl -k --http2 -vvv --resolve edge.header.test:443:127.0.0.1 https://edge.header.test/
$ curl -k --http2  -vvv --resolve reencrypt.header.test:443:127.0.0.1 https://reencrypt.header.test/

You should see that the protocol negotiation (between haproxy and the client) uses HTTP2 and there's a new header sent to the backend x-forwarded-proto-version: h2.

@ironcladlou @knobunc PTAL thx

/hold
This is a 3.11 feature.

knobunc · Jun 12, 2018

/hold
This is a 3.11 feature.

knobunc · Jun 12, 2018

knobunc approved these changes Jun 12, 2018

View changes

/lgtm
Can you add a docs PR for the environment variable please?

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, ramr

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:

~~images/router/OWNERS~~ [knobunc]

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

openshift-ci-robot · Jun 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, ramr

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:

~~images/router/OWNERS~~ [knobunc]

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

@knobunc

@knobunc ack on submitting a docs PR.

ramr · Jun 12, 2018

@knobunc ack on submitting a docs PR.

pravisankar · Jun 12, 2018

pravisankar approved these changes Jun 12, 2018

View changes

Minor comment otherwise LGTM

pravisankar · Jun 12, 2018

images/router/haproxy/conf/haproxy-config.template

@@ -219,6 +219,7 @@ frontend fe_sni
    {{- if isTrue (env "ROUTER_STRICT_SNI") }} strict-sni {{ end }}
    {{- ""}} crt {{firstMatch ".+" .DefaultCertificate "/var/lib/haproxy/conf/default_pub_keys.pem"}}
    {{- ""}} crt-list /var/lib/haproxy/conf/cert_config.map accept-proxy
+    {{- if isTrue (env "ROUTER_ENABLE_HTTP2") }} alpn h2,http/1.1{{ end }}


Do we really need ROUTER_ENABLE_HTTP2 env when we know that protocol downgrade happen automatically based on the client?

It is intentionally disabled by default as per the requirements in the trello card: https://trello.com/c/qzvlzuyx/27-3-implement-router-http-2-support-terminating-at-the-router-router (Acceptance criteria bullet 1)

Edited trello link pointer

What is the underlying justification of the cited requirement for a new piece of config? What are the specific risks of always enabling http2? When would a user want to disable it?

So as re: your questions:
3. The flag is flipped ... in that it is disabled by default and has to be explicitly enabled by the user/admin.
2. I don't see risks to always enable http2 as we do downgrade the connections to http/1.x if http/2 is not supported but that said one reason I can see is this is a new feature (and so interactions with different/older clients are something to also consider) - making it more prudent to have a flag that enables it as needed.

@knobunc can probably more on the cited requirement. Suspect it might be 2 but ...

Yeah, it's new in haproxy and there have already been security vulnerabilities around it. I want to let it soak for a while before turning it on all the time.

@pravisankar

@pravisankar as re: your question on the call, to test protocol downgrade to as example http/1.0, use -0 with curl.
E.g. curl -0 -k -vvv --resolve reencrypt.header.test:443:127.0.0.1 https://reencrypt.header.test/

ramr · Jun 12, 2018

@pravisankar as re: your question on the call, to test protocol downgrade to as example http/1.0, use -0 with curl.
E.g. curl -0 -k -vvv --resolve reencrypt.header.test:443:127.0.0.1 https://reencrypt.header.test/

Associated docs PR: openshift/openshift-docs#10057

ramr · Jun 12, 2018

Associated docs PR: openshift/openshift-docs#10057

When would we enable this by default? How do we get enough soak testing on this to enable by default?

smarterclayton · Jun 12, 2018

When would we enable this by default? How do we get enough soak testing on this to enable by default?

@smarterclayton

@smarterclayton I'd enable it on int then starter in 3.11 and if all is well, enable by default in 3.12.

knobunc · Jun 12, 2018

@smarterclayton I'd enable it on int then starter in 3.11 and if all is well, enable by default in 3.12.

/hold cancel

knobunc · Jun 27, 2018

/hold cancel

/retest

knobunc · Jun 27, 2018

/retest