Add an ENV to control ipv6 behavior in the router #15229

knobunc · 2017-07-16T13:22:52Z

This patch adds an environent variable called ROUTER_IP_V4_V6_MODE
which must be set to "v4" (the default), "v6", or "v4v6" to control
whether the router to binds to IPv4, IPv6, or both.

Note that when set to 'v6' or 'v4v6', the X-Forwarded-For and
Forwarded http headers will be in IPv6 form (even when the connection
was an IPv4 one, if set to 'v4v6').

Fixes bug 1471255 (https://bugzilla.redhat.com/show_bug.cgi?id=1471255)

knobunc · 2017-07-16T13:24:37Z

I am working on adding tests that can inspect the headers. [done]

I also need to create a docs PR.

I'm not sure about the name of the environment variable. Would it be better to have a variable named "ROUTER_IP_VERSION" and have it take "v4" (the default), "v6", "v4v6"?

@openshift/networking PTAL

smarterclayton · 2017-07-16T22:30:14Z

ROUTER_IP_MODE? Is this a bool or a multiway setting? On Jul 16, 2017, at 9:24 AM, Ben Bennett <notifications@github.com> wrote: I am working on adding tests that can inspect the headers. I also need to create a docs PR. I'm not sure about the name of the environment variable. Would it be better to have a variable named "ROUTER_IP_VERSION" and have it take "v4" (the default), "v6", "v4v6"? @openshift/networking <https://github.com/orgs/openshift/teams/networking> PTAL — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#15229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4rKGl3LDQdFKVxtigVdbGci3FJCks5sOg8YgaJpZM4OZS4K> .

knobunc · 2017-07-17T11:25:56Z

[test]

knobunc · 2017-07-17T11:30:29Z

@smarterclayton Right now it is a bool. With the current flag, we always bind IPv4, but allow control of whether to do v4 and v6. When I was putting the PR together I wondered if the semantics of the multi-way are a little more understandable.

I'm not sure about ROUTER_IP_MODE... I'll think about it a little, but at the moment it doesn't scream "look here for IPv6 support".

smarterclayton · 2017-07-17T12:31:41Z

ROUTER_IP_V4_V6_MODE

…

On Mon, Jul 17, 2017 at 7:30 AM, Ben Bennett ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> Right now it is a bool. With the current flag, we always bind IPv4, but allow control of whether to do v4 and v6. When I was putting the PR together I wondered if the semantics of the multi-way are a little more understandable. I'm not sure about ROUTER_IP_MODE... I'll think about it a little, but at the moment it doesn't scream "look here for IPv6 support". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4dO-a7fcdxRJj7DjDlpJYnFwQE9ks5sO0XXgaJpZM4OZS4K> .

imcsk8 · 2017-07-17T13:26:41Z

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

+{{- else }}
+  bind :{{env "ROUTER_SERVICE_HTTP_PORT" "80"}}
+{{- end }}
+    {{- if matchPattern "true|TRUE" (env "ROUTER_USE_PROXY_PROTOCOL" "") }} accept-proxy{{ end }}


is this indentation intended?
it does not seem to be part of a block

imcsk8 · 2017-07-17T13:33:41Z

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

@@ -199,7 +204,9 @@ frontend public
 # determined by the next backend in the chain which may be an app backend (passthrough termination) or a backend
 # that terminates encryption in this router (edge)
 frontend public_ssl
-  bind :::{{env "ROUTER_SERVICE_HTTPS_PORT" "443"}} v4v6 {{ if matchPattern "true|TRUE" (env "ROUTER_USE_PROXY_PROTOCOL" "") }} accept-proxy{{ end }}
+  bind :::{{env "ROUTER_SERVICE_HTTPS_PORT" "443"}}
+    {{- if matchPattern "true|TRUE" (env "ROUTER_ALLOW_IPV6" "")}} v4v6 {{ end }}


This approach is different than the one used for the port 80 in which the if sentence add the whole bind statement and in here it only adds the "v4v6" option to it.
It might be better to keep them consistent

Whoops! Thanks. I had them both this way before and then realized there was a problem (we needed to lose the :: as well). Then forgot to change both places.

rajatchopra

+1 to ROUTER_IP_V4_V6_MODE
Rest looks fine.

This patch adds an environent variable called ROUTER_IP_V4_V6_MODE which must be set to "v4" (the default), "v6", or "v4v6" to control whether the router to binds to IPv4, IPv6, or both. Note that when set to 'v6' or 'v4v6', the X-Forwarded-For and Forwarded http headers will be in IPv6 form (even when the connection was an IPv4 one, if set to 'v4v6'). Fixes bug 1471255 (https://bugzilla.redhat.com/show_bug.cgi?id=1471255)

knobunc · 2017-07-19T17:31:47Z

Added test cases @openshift/networking PTAL

pecameron

+1 to ROUTER_IP_V4_V6_MODE
Are you thinking of a command line option?
LGTM

knobunc · 2017-07-19T19:13:04Z

[testextended][extended: networking]

openshift-bot · 2017-07-19T19:17:22Z

Evaluated for origin testextended up to 7821bd5

eparis · 2017-07-19T19:52:56Z

ben, feel free to merge on clean test.

This patch adds an environent variable called ROUTER_IP_V4_V6_MODE which must be set to "v4" (the default), "v6", or "v4v6" to control whether the router to binds to IPv4, IPv6, or both. Note that when set to 'v6' or 'v4v6', the X-Forwarded-For and Forwarded http headers will be in IPv6 form (even when the connection was an IPv4 one, if set to 'v4v6'). Back-port of openshift#15229 Fixes bug 1472976 (https://bugzilla.redhat.com/show_bug.cgi?id=1472976)

knobunc · 2017-07-19T20:06:18Z

[test] last flaked on #15024

openshift-bot · 2017-07-19T21:07:54Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/882/) (Base Commit: 3bb18b6) (PR Branch Commit: 7821bd5) (Extended Tests: networking)

knobunc · 2017-07-20T14:35:54Z

[test] last flaked on #15337

knobunc · 2017-07-20T14:42:10Z

[merge] This has passed each test cleanly... just not on the same run. i.e. each run has flaked in different places.

openshift-bot · 2017-07-20T14:48:19Z

Evaluated for origin merge up to 7821bd5

knobunc · 2017-07-20T20:35:11Z

[test] flaked on #15351

openshift-bot · 2017-07-20T20:40:30Z

Evaluated for origin test up to 7821bd5

openshift-bot · 2017-07-20T22:45:42Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3354/) (Base Commit: d784563) (PR Branch Commit: 7821bd5)

openshift-bot · 2017-07-21T03:56:01Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1330/) (Base Commit: fb4423b) (PR Branch Commit: 7821bd5) (Image: devenv-rhel7_6469)

knobunc added the component/routing label Jul 16, 2017

knobunc added this to the 3.6.0 milestone Jul 16, 2017

knobunc self-assigned this Jul 16, 2017

knobunc requested review from imcsk8, rajatchopra, smarterclayton and JacobTanenbaum July 16, 2017 13:22

imcsk8 reviewed Jul 17, 2017

View reviewed changes

knobunc force-pushed the bug/bz1471255-ipv6-env branch 3 times, most recently from bbc9de8 to 522aa47 Compare July 17, 2017 20:24

rajatchopra approved these changes Jul 17, 2017

View reviewed changes

knobunc force-pushed the bug/bz1471255-ipv6-env branch from 522aa47 to 7821bd5 Compare July 19, 2017 17:22

pecameron approved these changes Jul 19, 2017

View reviewed changes

knobunc mentioned this pull request Jul 19, 2017

Add an ENV to control ipv6 behavior in the router #15351

Merged

knobunc removed this from the 3.6.0 milestone Jul 19, 2017

knobunc mentioned this pull request Jul 19, 2017

Extended.[k8s.io] Service endpoints latency should not be very high [Conformance] #15024

Closed

openshift-bot merged commit 137d702 into openshift:master Jul 21, 2017

knobunc deleted the bug/bz1471255-ipv6-env branch July 21, 2017 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an ENV to control ipv6 behavior in the router #15229

Add an ENV to control ipv6 behavior in the router #15229

knobunc commented Jul 16, 2017 •

edited

Loading

knobunc commented Jul 16, 2017 •

edited

Loading

smarterclayton commented Jul 16, 2017 via email

knobunc commented Jul 17, 2017

knobunc commented Jul 17, 2017

smarterclayton commented Jul 17, 2017 via email

imcsk8 Jul 17, 2017

imcsk8 Jul 17, 2017

knobunc Jul 17, 2017

rajatchopra left a comment

knobunc commented Jul 19, 2017

pecameron left a comment

knobunc commented Jul 19, 2017

openshift-bot commented Jul 19, 2017

eparis commented Jul 19, 2017

knobunc commented Jul 19, 2017

openshift-bot commented Jul 19, 2017

knobunc commented Jul 20, 2017

knobunc commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

knobunc commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

openshift-bot commented Jul 21, 2017 •

edited

Loading

Add an ENV to control ipv6 behavior in the router #15229

Add an ENV to control ipv6 behavior in the router #15229

Conversation

knobunc commented Jul 16, 2017 • edited Loading

knobunc commented Jul 16, 2017 • edited Loading

smarterclayton commented Jul 16, 2017 via email

knobunc commented Jul 17, 2017

knobunc commented Jul 17, 2017

smarterclayton commented Jul 17, 2017 via email

imcsk8 Jul 17, 2017

Choose a reason for hiding this comment

imcsk8 Jul 17, 2017

Choose a reason for hiding this comment

knobunc Jul 17, 2017

Choose a reason for hiding this comment

rajatchopra left a comment

Choose a reason for hiding this comment

knobunc commented Jul 19, 2017

pecameron left a comment

Choose a reason for hiding this comment

knobunc commented Jul 19, 2017

openshift-bot commented Jul 19, 2017

eparis commented Jul 19, 2017

knobunc commented Jul 19, 2017

openshift-bot commented Jul 19, 2017

knobunc commented Jul 20, 2017

knobunc commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

knobunc commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

openshift-bot commented Jul 20, 2017

openshift-bot commented Jul 21, 2017 • edited Loading

knobunc commented Jul 16, 2017 •

edited

Loading

knobunc commented Jul 16, 2017 •

edited

Loading

openshift-bot commented Jul 21, 2017 •

edited

Loading