Combine backend map files to fix path based routing #18840

JacobTanenbaum · 2018-03-05T18:27:51Z

combined the two maps for insecure routes os_edge_http_be.map and os_route_http_expose.map and the two maps for secure routes os_edge_reencrypt.map and os_edge_http_be.map reducing the number of map files and fixing path based routing

Bug 1534816

JacobTanenbaum · 2018-03-05T18:28:04Z

@knobunc PTAL

imcsk8 · 2018-03-05T20:12:48Z

/retest

JacobTanenbaum · 2018-03-05T21:03:23Z

/test extended_networking_minimal

JacobTanenbaum · 2018-03-07T19:03:00Z

/retest

JacobTanenbaum · 2018-03-08T16:19:24Z

/test gcp

JacobTanenbaum · 2018-03-08T18:36:37Z

/test extended_networking_minimal

JacobTanenbaum · 2018-03-09T14:34:40Z

/test extended_conformance_install

ramr

@JacobTanenbaum the cleanup looks great, thx. Just a minor nit/comment re: the inconsistency of the names and an extra comma.

ramr · 2018-03-12T01:21:52Z

images/router/haproxy/Dockerfile

@@ -11,7 +11,7 @@ RUN INSTALL_PKGS="haproxy18" && \
    yum clean all && \
    mkdir -p /var/lib/haproxy/router/{certs,cacerts} && \
    mkdir -p /var/lib/haproxy/{conf,run,bin,log} && \
-    touch /var/lib/haproxy/conf/{{os_http_be,os_edge_http_be,os_tcp_be,os_sni_passthrough,os_reencrypt,os_route_http_expose,os_route_http_redirect,cert_config,os_wildcard_domain}.map,haproxy.config} && \
+    touch /var/lib/haproxy/conf/{{os_http_be,os_edge_reencrypt_be,os_tcp_be,os_sni_passthrough,,os_route_http_redirect,cert_config,os_wildcard_domain}.map,haproxy.config} && \


Remove extra comma: os_tcp_be,os_sni_passthrough,,os_route_http_redirect

ramr · 2018-03-12T01:28:19Z

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

-  # Note: If no match, haproxy uses the default_backend, no other
-  #       use_backend directives below this will be processed.
-  use_backend be_http:%[base,map_reg(/var/lib/haproxy/conf/os_http_be.map)]
+  use_backend %[base,map_reg(/var/lib/haproxy/conf/os_http_be.map)]


Any reason why you only added be_http: and be_edge_http: to the map backend entry value and not for the redirect and tcp maps? It makes it inconsistent.

I left out the redirects because that is a specific operation that does not link to a use_backend statement. It is determining to allow redirect based on if there is an entry in the map.

No worries - I was just asking. I've made the change in my branch to make it consistent as part of the dynamic config changes anyway so its cool.

JacobTanenbaum · 2018-03-14T17:33:32Z

/test cmd

ramr

LGTM

ramr · 2018-03-15T20:26:24Z

images/router/haproxy/Dockerfile

@@ -11,7 +11,7 @@ RUN INSTALL_PKGS="haproxy18" && \
    yum clean all && \
    mkdir -p /var/lib/haproxy/router/{certs,cacerts} && \
    mkdir -p /var/lib/haproxy/{conf,run,bin,log} && \
-    touch /var/lib/haproxy/conf/{{os_http_be,os_edge_http_be,os_tcp_be,os_sni_passthrough,os_reencrypt,os_route_http_expose,os_route_http_redirect,cert_config,os_wildcard_domain}.map,haproxy.config} && \
+    touch /var/lib/haproxy/conf/{{os_http_be,os_edge_reencrypt_be,os_tcp_be,os_sni_passthrough,os_route_http_redirect,cert_config,os_wildcard_domain}.map,haproxy.config} && \


Ooh, I was just merging in these changes into my branch and I noticed the map is still called os_edge_http_be and not os_edge_reencrypt_be and it is still being referenced as "os_edge_http_be" in the use backend directives. So this PR needs changes.

This is now done, but github hasn't noticed.

combined the two maps for insecure routes os_edge_http_be.map and os_route_http_expose.map and the two maps for secure routes os_reencrypt.map and os_edge_http_be.map reducing the number of map files and fixing path based routing Bug 1534816

JacobTanenbaum · 2018-03-19T18:55:21Z

/test gcp

pravisankar

LGTM

rajatchopra · 2018-03-19T21:06:25Z

/lgtm

openshift-ci-robot · 2018-03-19T21:06:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JacobTanenbaum, rajatchopra

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~~ [rajatchopra]

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

ramr

LGTM

smarterclayton · 2018-03-20T03:08:36Z

Please add an e2e test that validates path based routing (if it really was broken) in the extended suite.

…

On Mon, Mar 19, 2018 at 5:06 PM, OpenShift CI Robot < ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *APPROVED* This pull-request has been approved by: *JacobTanenbaum <#18840#>*, *rajatchopra <#18840 (comment)>* The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands>. The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - images/router/OWNERS <https://github.com/openshift/origin/blob/master/images/router/OWNERS> [rajatchopra] Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#18840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4UVDHvDsAeqHMhhj5A1fkfRYfP9ks5tgB3bgaJpZM4ScxSI> .

smarterclayton · 2018-03-20T03:08:40Z

Doesn't have to be part of this PR On Mon, Mar 19, 2018 at 11:08 PM, Clayton Coleman <[email protected]> wrote:

…

Please add an e2e test that validates path based routing (if it really was broken) in the extended suite. On Mon, Mar 19, 2018 at 5:06 PM, OpenShift CI Robot < ***@***.***> wrote: > [APPROVALNOTIFIER] This PR is *APPROVED* > > This pull-request has been approved by: *JacobTanenbaum > <#18840#>*, *rajatchopra > <#18840 (comment)>* > > The full list of commands accepted by this bot can be found here > <https://go.k8s.io/bot-commands>. > > The pull request process is described here > <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> > Needs approval from an approver in each of these files: > > - images/router/OWNERS > <https://github.com/openshift/origin/blob/master/images/router/OWNERS> > [rajatchopra] > > Approvers can indicate their approval by writing /approve in a comment > Approvers can cancel approval by writing /approve cancel in a comment > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#18840 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABG_p4UVDHvDsAeqHMhhj5A1fkfRYfP9ks5tgB3bgaJpZM4ScxSI> > . >

dcbw · 2018-03-20T16:20:44Z

/test gcp

knobunc · 2018-03-26T13:04:32Z

/retest

0xmichalis · 2018-03-28T12:51:20Z

/refresh

0xmichalis · 2018-03-28T12:51:40Z

/test end_to_end

openshift-ci-robot requested review from knobunc and smarterclayton March 5, 2018 18:27

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

pravisankar changed the title ~~Combine backend map files to fix bath based routing~~ Combine backend map files to fix path based routing Mar 5, 2018

pravisankar added the component/routing label Mar 5, 2018

knobunc self-assigned this Mar 7, 2018

knobunc requested review from ramr and rajatchopra and removed request for smarterclayton March 7, 2018 19:53

ramr reviewed Mar 12, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1534816v2 branch from 3d1349b to 2f42e0d Compare March 14, 2018 15:06

ramr approved these changes Mar 14, 2018

View reviewed changes

ramr suggested changes Mar 15, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1534816v2 branch from 2f42e0d to 8d5346e Compare March 19, 2018 14:18

pravisankar approved these changes Mar 19, 2018

View reviewed changes

openshift-ci-robot assigned rajatchopra Mar 19, 2018

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2018

ramr approved these changes Mar 20, 2018

View reviewed changes

openshift-merge-robot merged commit 3f52063 into openshift:master Mar 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine backend map files to fix path based routing #18840

Combine backend map files to fix path based routing #18840

JacobTanenbaum commented Mar 5, 2018

JacobTanenbaum commented Mar 5, 2018

imcsk8 commented Mar 5, 2018

JacobTanenbaum commented Mar 5, 2018

JacobTanenbaum commented Mar 7, 2018

JacobTanenbaum commented Mar 8, 2018

JacobTanenbaum commented Mar 8, 2018

JacobTanenbaum commented Mar 9, 2018

ramr left a comment

ramr Mar 12, 2018

ramr Mar 12, 2018

JacobTanenbaum Mar 14, 2018

ramr Mar 14, 2018

JacobTanenbaum commented Mar 14, 2018

ramr left a comment

ramr Mar 15, 2018

knobunc Mar 26, 2018

JacobTanenbaum commented Mar 19, 2018

pravisankar left a comment

rajatchopra commented Mar 19, 2018

openshift-ci-robot commented Mar 19, 2018

ramr left a comment

smarterclayton commented Mar 20, 2018 via email

smarterclayton commented Mar 20, 2018 via email

dcbw commented Mar 20, 2018

knobunc commented Mar 26, 2018

0xmichalis commented Mar 28, 2018

0xmichalis commented Mar 28, 2018

Combine backend map files to fix path based routing #18840

Combine backend map files to fix path based routing #18840

Conversation

JacobTanenbaum commented Mar 5, 2018

JacobTanenbaum commented Mar 5, 2018

imcsk8 commented Mar 5, 2018

JacobTanenbaum commented Mar 5, 2018

JacobTanenbaum commented Mar 7, 2018

JacobTanenbaum commented Mar 8, 2018

JacobTanenbaum commented Mar 8, 2018

JacobTanenbaum commented Mar 9, 2018

ramr left a comment

Choose a reason for hiding this comment

ramr Mar 12, 2018

Choose a reason for hiding this comment

ramr Mar 12, 2018

Choose a reason for hiding this comment

JacobTanenbaum Mar 14, 2018

Choose a reason for hiding this comment

ramr Mar 14, 2018

Choose a reason for hiding this comment

JacobTanenbaum commented Mar 14, 2018

ramr left a comment

Choose a reason for hiding this comment

ramr Mar 15, 2018

Choose a reason for hiding this comment

knobunc Mar 26, 2018

Choose a reason for hiding this comment

JacobTanenbaum commented Mar 19, 2018

pravisankar left a comment

Choose a reason for hiding this comment

rajatchopra commented Mar 19, 2018

openshift-ci-robot commented Mar 19, 2018

ramr left a comment

Choose a reason for hiding this comment

smarterclayton commented Mar 20, 2018 via email

smarterclayton commented Mar 20, 2018 via email

dcbw commented Mar 20, 2018

knobunc commented Mar 26, 2018

0xmichalis commented Mar 28, 2018

0xmichalis commented Mar 28, 2018