Fix for wildcard routes taking precedence over other routes #19076

jmprusi · 2018-03-23T13:58:15Z

To avoid wildcard routes (type: Subdomain) overriding other routes that start with a number,
we first sort the non-wildcard routes and then we append the wildcard routes at the end of the file.

Related: #16724

jmprusi · 2018-03-23T15:13:36Z

/retest

knobunc · 2018-03-28T19:06:36Z

@openshift/networking PTAL

ramr

Changes look good - just a couple of comments.

ramr · 2018-03-28T23:47:23Z

images/router/haproxy/reload-haproxy

 for mapfile in "$haproxy_conf_dir"/*.map; do
-  sort -r "$mapfile" -o "$mapfile"
+  grep -v -F '^[^\.]*\' "$mapfile" | sort -r -o /tmp/"$mapfile"
+  grep -F '^[^\.]*\' "$mapfile" >> /tmp/"$mapfile"


Sort it as well ala:
grep -F '^[^\.]*\' "$mapfile" | sort -r >> "/tmp/$mapfile"

ramr · 2018-03-28T23:50:32Z

images/router/haproxy/reload-haproxy

+  grep -v -F '^[^\.]*\' "$mapfile" | sort -r -o /tmp/"$mapfile"
+  grep -F '^[^\.]*\' "$mapfile" >> /tmp/"$mapfile"
+  cp /tmp/"$mapfile" "$haproxy_conf_dir"/"$mapfile"
+  rm /tmp/"$mapfile"


Could use: mv -f "/tmp/$mapfile" "$haproxy_conf_dir/$mapfile" to collapse the 2 commands.

To avoid having wildcard routes (type: Subdomain) override other routes, we first sort the non-wildcard routes and then we append the wildcard routes at the end of the file.

jmprusi · 2018-03-29T12:48:15Z

@knobunc @ramr thanks! Added your feedback!

knobunc

LGTM. Thanks!

/lgtm

knobunc · 2018-03-29T14:11:02Z

/approve

knobunc · 2018-03-29T14:11:07Z

/retest

openshift-ci-robot · 2018-03-29T14:11:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-bot · 2018-03-29T16:41:49Z

/retest

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

ramr

Thx LGTM

smarterclayton · 2018-03-30T02:19:35Z

This is reporting errors when I look at the logs:

I0330 02:15:04.127575       1 router.go:447] Router reloaded:
sort: open failed: /tmp//var/lib/haproxy/conf/cert_config.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/cert_config.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/cert_config.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_edge_reencrypt_be.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_edge_reencrypt_be.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_edge_reencrypt_be.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_http_be.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_http_be.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_http_be.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_route_http_redirect.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_route_http_redirect.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_route_http_redirect.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_sni_passthrough.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_sni_passthrough.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_sni_passthrough.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_tcp_be.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_tcp_be.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_tcp_be.map': No such file or directory
sort: open failed: /tmp//var/lib/haproxy/conf/os_wildcard_domain.map: No such file or directory
/var/lib/haproxy/reload-haproxy: line 80: /tmp//var/lib/haproxy/conf/os_wildcard_domain.map: No such file or directory
mv: cannot stat '/tmp//var/lib/haproxy/conf/os_wildcard_domain.map': No such file or directory
 - Checking http://localhost:80 ...
 - Health check ok : 0 retry attempt(s).

It looks like this has a break in it, I'm going to revert until we know for sure what is wrong.

smarterclayton · 2018-03-30T02:20:33Z

Why is this sorting being done in the reload function and not in where we generate the map files in the first place?

ramr · 2018-03-30T04:25:17Z

So its failing because the file name is fully qualified -- needs a basename $mapfile for the name in the temp directory (the temp file created for this). Doh, I completed missed that.
So changes needed ala:

fname=$(basename "$mapfile")   
grep -v -F '^[^\.]*\' "$mapfile" | sort -r -o "/tmp/$fname"`

and do the same for all the other references.
@jmprusi fyi ↑

As re: the sorting @smarterclayton think that was just done from the start as we needed to reverse sort it for the longest path match on routes. This is so we match on the longest path first.
We could well sort in the template but it would still need like a two step process to do and would be further complicated with this case of doing like a regular route and then a wildcard sort.

smarterclayton · 2018-03-30T04:47:27Z

Doing it in bash is very ugly. It is also coupling the generation logic to the reload logic. On Mar 30, 2018, at 12:25 AM, Ram Ranganathan <[email protected]> wrote: So its failing because the file name is fully qualified -- needs a basename $mapfile for the name in the temp directory (the temp file created for this). Doh, I completed missed that. So changes needed ala: fname=$(basename "$mapfile") grep -v -F '^[^\.]*\' "$mapfile" | sort -r -o "/tmp/$fname" ` and do the same for all the other references. @jmprusi <https://github.com/jmprusi> fyi ↑ As re: the sorting @smarterclayton <https://github.com/smarterclayton> think that was just done from the start as we needed to reverse sort it for the longest path match on routes. This is so we match on the longest path first. We could well sort in the template but it would still need like a two step process to do and would be further complicated with this case of doing like a regular route and then a wildcard sort. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p9t7Iu0D73xsrUSEdaffU-0IgbUmks5tjbOwgaJpZM4S4wrq> .

ramr · 2018-03-30T19:17:54Z

@smarterclayton well the template router is "sorta" (qualified) agnostic to the type of proxy underneath it. Could well do it in go code via something like a pre-processor run before the reload script is called (in the router/template code) but it would still have to be haproxy specific.
We also don't pass the router type to the infra command which we'd need since we now would have to set up the pre-processor for a haproxy specific router type, so it means some migration for existing router deployments.

smarterclayton · 2018-03-30T20:26:24Z

It sounds like a new helper. The router is agnostic but our helpers aren’t always. On Mar 30, 2018, at 3:17 PM, Ram Ranganathan <[email protected]> wrote: @smarterclayton <https://github.com/smarterclayton> well the template router is "sorta" (qualified) agnostic to the type of proxy underneath it. Could well do it in go code via something like a pre-processor run before the reload script is called (in the router/template code) but it would still have to be haproxy specific. We also don't pass the router type to the infra command which we'd need since we now would have to set up the pre-processor for a haproxy specific router type, so it means some migration for existing router deployments. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p0izCsWcajhYHJWNq-5Zd2-wu1foks5tjoTmgaJpZM4S4wrq> .

jmprusi · 2018-03-31T09:44:33Z

@smarterclayton @ramr should I open a new PR with the proper fixes (sorry) ?
edit:

Created a new PR with the required fixes:
#19187

ramr · 2018-04-02T18:55:43Z

@jmprusi Sorry for the delayed response - was traveling. So what @smarterclayton mentioned was using a new helper to do the sort. I'll work on this later this week.

smarterclayton · 2018-04-02T21:01:20Z

Yeah I would really prefer that biz logic that is required for the template like a sort be a helper rather than bash inside of the reload script.

…

On Mon, Apr 2, 2018 at 2:55 PM, Ram Ranganathan ***@***.***> wrote: @jmprusi <https://github.com/jmprusi> Sorry for the delayed response - was traveling. So what @smarterclayton <https://github.com/smarterclayton> mentioned was using a new helper to do the sort. I'll work on this later this week. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p2GykrWiqCtnQwaSvunynvR90o_Kks5tknQzgaJpZM4S4wrq> .

…ldcard routes don't take precedence over routes that start with a number. Based on discussions in PR openshift#19076 to fix issue openshift#16724

…ldcard routes don't take precedence over routes that start with a number. Make the template processing follow a particular order (based on file name), so that we can use temporary files to write map data and subsequently sort into the actual config map file. Fixup helper to also sort cert_config but non-grouped to keep existing behavior (backward compatibility). Based on discussions in PR openshift#19076 to fix issue openshift#16724

openshift-ci-robot requested review from pecameron and pweil- March 23, 2018 13:58

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 23, 2018

jmprusi changed the title ~~Fix for wildcard routes taking precedence over other routes https://github.com/openshift/origin/issues/16724~~ Fix for wildcard routes taking precedence over other routes Mar 23, 2018

knobunc self-assigned this Mar 28, 2018

knobunc requested review from ramr and rajatchopra March 28, 2018 19:05

ramr reviewed Mar 28, 2018

View reviewed changes

reload-haproxy: changes how map files are sorted

9120e05

To avoid having wildcard routes (type: Subdomain) override other routes, we first sort the non-wildcard routes and then we append the wildcard routes at the end of the file.

jmprusi force-pushed the master branch from 8b2fb24 to 9120e05 Compare March 29, 2018 12:46

knobunc approved these changes Mar 29, 2018

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 29, 2018

ramr approved these changes Mar 29, 2018

View reviewed changes

openshift-merge-robot merged commit bb8bbbe into openshift:master Mar 29, 2018

smarterclayton mentioned this pull request Mar 30, 2018

Revert "Fix for wildcard routes taking precedence over other routes" #19155

Merged

ramr mentioned this pull request Apr 2, 2018

Fix for wildcard routes taking precedence over other routes #19187

Closed

ramr mentioned this pull request Apr 4, 2018

Template helper code to sort map files #19219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for wildcard routes taking precedence over other routes #19076

Fix for wildcard routes taking precedence over other routes #19076

jmprusi commented Mar 23, 2018 •

edited

Loading

jmprusi commented Mar 23, 2018

knobunc commented Mar 28, 2018

ramr left a comment

ramr Mar 28, 2018

ramr Mar 28, 2018 •

edited

Loading

jmprusi commented Mar 29, 2018

knobunc left a comment

knobunc commented Mar 29, 2018

knobunc commented Mar 29, 2018

openshift-ci-robot commented Mar 29, 2018

openshift-bot commented Mar 29, 2018

ramr left a comment

smarterclayton commented Mar 30, 2018

smarterclayton commented Mar 30, 2018

ramr commented Mar 30, 2018 •

edited

Loading

smarterclayton commented Mar 30, 2018 via email

ramr commented Mar 30, 2018

smarterclayton commented Mar 30, 2018 via email

jmprusi commented Mar 31, 2018 •

edited

Loading

ramr commented Apr 2, 2018

smarterclayton commented Apr 2, 2018 via email

Fix for wildcard routes taking precedence over other routes #19076

Fix for wildcard routes taking precedence over other routes #19076

Conversation

jmprusi commented Mar 23, 2018 • edited Loading

jmprusi commented Mar 23, 2018

knobunc commented Mar 28, 2018

ramr left a comment

Choose a reason for hiding this comment

ramr Mar 28, 2018

Choose a reason for hiding this comment

ramr Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

jmprusi commented Mar 29, 2018

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Mar 29, 2018

knobunc commented Mar 29, 2018

openshift-ci-robot commented Mar 29, 2018

openshift-bot commented Mar 29, 2018

ramr left a comment

Choose a reason for hiding this comment

smarterclayton commented Mar 30, 2018

smarterclayton commented Mar 30, 2018

ramr commented Mar 30, 2018 • edited Loading

smarterclayton commented Mar 30, 2018 via email

ramr commented Mar 30, 2018

smarterclayton commented Mar 30, 2018 via email

jmprusi commented Mar 31, 2018 • edited Loading

ramr commented Apr 2, 2018

smarterclayton commented Apr 2, 2018 via email

jmprusi commented Mar 23, 2018 •

edited

Loading

ramr Mar 28, 2018 •

edited

Loading

ramr commented Mar 30, 2018 •

edited

Loading

jmprusi commented Mar 31, 2018 •

edited

Loading