Template helper code to sort map files #19219
Conversation
openshift-ci-robot
added
the
size/L
|
/retest |
ramr
changed the title
openshift-ci-robot
added
size/XL
ramr
changed the title
…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
added
lgtm
|
/approve |
|
[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:
Approvers can indicate their approval by writing |
| @@ -569,7 +606,7 @@ backend be_tcp:{{$cfgIdx}} | |||
| "<cert>: <domain-set>" is important as this allows us to use | |||
| wildcards and/or use a deny set with !<domain> in the future. | |||
| */}} | |||
| {{ define "/var/lib/haproxy/conf/cert_config.map" -}} | |||
| {{ define "/var/lib/haproxy/conf/.tmp/cert_config.map" -}} | |||
There was a problem hiding this comment.
Why do the temp file instead of just having a sort helper for .State?
There was a problem hiding this comment.
so what would make more sense is to just generate the sorted config info via different helper functions [its different config in some cases] and just do a simple range in the template and put that info into a file. There was also pushback on adding more helper functions - mentioned on the sprint calls and from the past, so ...
| // non-wildcard paths first and adds the sorted wildcard paths at the end. | ||
| // Note: The reversed sorting ensures we do a longest path match first. | ||
| func sortedMapData(name string, sortSubGroups bool) []string { | ||
| lines, err := fileutil.ReadLines(name) |
There was a problem hiding this comment.
Reading a file from within a helper is dangerous, this means a template can read arbitrary files off disk (not possible before), which could lead to a future security hole and allow access to certificate data or other data like the service account key. I’d prefer not to allow any template to read from disk - it’s just one less escape vector for the template engine. An author could trivially use this to read the router credentials, and in combination with a future vulnerability could be very dangerous.
There was a problem hiding this comment.
I do understand those consequences but that said, one doesn't really to read that cert/key info from disk ... all that config is available inside the template in .State.
But yeah not doing a file read/passing a name is definitely better ... so, based on the above comment (aka just doing a simple range and write in the template and computing the info inside different helper functions) - if that works for you, I can create a new PR next week ... got to pay my dues to the other devil this weekend!
Add template helper code to sort haproxy map files so that wildcard routes don't
take precedence over routes that start with a number.
Based on discussions in PR #19076 to fix issue #16724
@smarterclayton @jmprusi