Make the router handle ports properly (don't strip them). - #12580
Conversation
|
[test] |
|
[test] |
|
@rajatchopra a quick review would be highly appreciated. Thx |
ramr
changed the title
|
@JacobTanenbaum if you could also give it a view that would be awesome. @danwinship / @dcbw @knobunc is out today and I have been out sick all week but I got to finish this up - its for the trello card: https://trello.com/c/MEZZinj8/378-5-make-the-router-handle-ports-properly-don-t-strip-them |
|
Error: Flake: #12558 |
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
I don't know haproxy configuration at all, so all I can really comment on is:
| } | ||
| } | ||
|
|
||
| return fmt.Sprintf("^%s(|:[0-9]+)%s(|/.*)$", hostRE, path) |
There was a problem hiding this comment.
shouldn't path be QuoteMeta()ed?
There was a problem hiding this comment.
Good catch. Yes, it should be.
|
[test] |
|
Evaluated for origin test up to 3a47229 |
| @@ -102,6 +102,7 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp | |||
| "matchValues": matchValues, //compares a given string to a list of allowed strings | |||
|
|
|||
| "genSubdomainWildcardRegexp": genSubdomainWildcardRegexp, //generates a regular expression matching the subdomain for hosts (and paths) with a wildcard policy | |||
There was a problem hiding this comment.
This function does not get used in the template now. Remove it?
There was a problem hiding this comment.
Yeah, I put a comment on the actual function - we can but it would break any existing template - for example if you use a config map. So my 2 cents are to keep it around a bit and remove it later - that's just safer.
| } | ||
|
|
||
| // genSubdomainWildcardRegexp is now legacy and around for backward | ||
| // compatibility and allows old templates to continue running. |
There was a problem hiding this comment.
I say remove it. Old templates with new router - how far will we go?
There was a problem hiding this comment.
if you use config maps, you will run into issues once you upgrade/the latest image changes underneath you.
| } | ||
| } | ||
|
|
||
| return fmt.Sprintf("^%s(|:[0-9]+)%s(|/.*)$", hostRE, path) |
There was a problem hiding this comment.
Could we have a unit test to verify the behaviour of the regex? Just split this into a function and test that function's output only. Maybe a follow up PR.
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13112/) (Base Commit: 8492871) |
|
Same flake: |
|
@rajatchopra @danwinship thanks for the quick reviews. Could one of you please hit merge. Thx. |
|
[merge] |
|
Evaluated for origin merge up to 3a47229 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13133/) (Base Commit: 2cc89a9) (Image: devenv-rhel7_5744) |
5 participants
Associated trello card: https://trello.com/c/MEZZinj8/378-5-make-the-router-handle-ports-properly-don-t-strip-them