Second version of helper code for sorting map files. #19492

ramr · 2018-04-24T19:31:33Z

@smarterclayton PTAL - this is based on your comments on #19219
It seems lot more work/code as opposed to a grep -v | sort -r + grep | sort -r + cat but on the flip side, there is more testing around the map generation code.

/cc @knobunc

mfojtik · 2018-04-24T19:59:48Z

pkg/router/template/util/util.go

+)
+
+// Generate a regular expression to match route hosts (and paths if any).
+func GenerateRouteRegexp(hostname, path string, wildcard bool) string {


This was actually existing code that got moved. But I can add godoc to it.

mfojtik · 2018-04-24T20:01:37Z

pkg/router/template/util/util.go

+	// build the correct subpath regex, depending on whether path ends with a segment separator
+	var pathRE, subpathRE string
+	switch {
+	case strings.TrimRight(path, "/") == "":


i think we use len() == 0 convention to check empty strings

ok - will do (its existing code that got moved to a new file).

mfojtik · 2018-04-24T20:03:48Z

pkg/router/template/template_helper.go

@@ -99,49 +104,19 @@ func genSubdomainWildcardRegexp(hostname, path string, exactPath bool) string {
 	return fmt.Sprintf(`^[^\.]*%s(|/.*)$`, expr)
 }

+// generateRouteRegexp is now legacy and around for backward
+// compatibility and allows old templates to continue running.


why keeping this wrapper?

For any existing custom haproxy templates to work, the helper function needs to exist. One scenario where this is needed is if you upgrade a router image but are still using the custom haproxy template file (based on an older template).

mfojtik · 2018-04-24T20:04:09Z

pkg/router/template/template_helper.go

-	}
-
-	return "^" + hostRE + portRE + pathRE + subpathRE + "$"
+	return templateutil.GenerateRouteRegexp(hostname, path, wildcard)


same question as above

same answer!! ;^)

mfojtik · 2018-04-24T20:04:22Z

pkg/router/template/template_helper.go

-	}
-
-	return hostname
+	return templateutil.GenCertificateHostName(hostname, wildcard)


mfojtik · 2018-04-24T20:05:05Z

pkg/router/template/template_helper.go

-	if err != nil {
-		glog.Errorf("Error reading map file %s: %v", name, err)
-		return []string{}
+// Returns a haproxy backend config for a given service alias.


i would keep the godoc standard even if this is private func

ok will do. thx

ramr · 2018-04-24T22:49:36Z

/test verify

ramr · 2018-04-25T04:36:20Z

/test gcp

ramr · 2018-04-25T18:02:21Z

Flake #19058
Flake #19505

ramr · 2018-04-25T18:02:30Z

/test gcp

knobunc

Looks good to me.

knobunc · 2018-04-30T17:34:39Z

@smarterclayton Is this the change you wanted?

…r the template helper code.

ramr · 2018-05-01T18:00:56Z

flake #19500

ramr · 2018-05-01T18:01:09Z

/retest cmd

smarterclayton · 2018-05-01T19:03:40Z

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

 {{ define "/var/lib/haproxy/conf/cert_config.map" -}}
-{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/cert_config.map" false -}}
-{{ $data }}
+{{     range $idx, $line := generateHAProxyMap "cert_config.map" . -}}


I like this a lot more

Although....

Can you get at the template name from within a template? If we did that we could avoid having the filename argument. I.e. when you create the template create a closure over the template name.

Basically, in pkg/router/template/plugin.go line 122 (inside NewTemplatePlugin) when we iterate over the loop of templates, create a closure on the specific helpers that are filename specific and create a closure with:

for _, template := range masterTemplates.Templates() { ... // copy helperFunctions into a new map name := template.Name() fns["generateHAProxyMap"] = func(State) { // use name here instead of taking it as an argument } // do the same for others template = template.Funcs(fns) templates[template.Name()] = template }

ok will do.

smarterclayton · 2018-05-01T19:05:54Z

pkg/router/template/template_helper.go

+		}
+
+		backendConfig := backendConfig(k, cfg, hascert)
+		if entry := haproxyutil.GenerateMapEntry(certConfigMap, backendConfig); entry != nil {


It would be better if this was passed in as a closure from the template name, rather than it coming from a hardcoded function.

smarterclayton · 2018-05-01T19:07:00Z

pkg/router/template/util/util.go

@@ -0,0 +1,71 @@
+package util


Don't put methods into packages called util. Is there any reason this isn't in the haproxy package? Because this looks haproxy specific.

Moved those out of the template_helper.go code as they are common - used by both nginx and older haproxy templates (the new template would just call those functions directly instead of via the the template_helper code). All expect for the last one GenerateBackendNamePrefix which I guess is sorta haproxy specific - but technically just a prefix name, so qualifies as an utility function.

smarterclayton · 2018-05-01T19:14:20Z

i like that it's tested a lot.

I'd like to remove the filename parameter to generateHAProxyMap completely and make it be a closure, see my example.

generation function to be scoped to the template its operating under.

ramr · 2018-05-02T04:12:32Z

@smarterclayton ok made the requested change - had to do it slightly differently. It needed a clone before setting the functions (creating a new template) as the template functions are shared - last one in replaces the ones set before it.

knobunc · 2018-05-03T15:27:12Z

/approve
/retest

openshift-ci-robot · 2018-05-03T15:27:18Z

[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:

~~images/router/OWNERS~~ [knobunc]
~~pkg/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

knobunc · 2018-05-07T13:22:27Z

/lgtm

openshift-ci-robot requested a review from knobunc April 24, 2018 19:31

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 24, 2018

mfojtik reviewed Apr 24, 2018

View reviewed changes

knobunc approved these changes Apr 30, 2018

View reviewed changes

knobunc self-assigned this Apr 30, 2018

knobunc requested a review from smarterclayton April 30, 2018 17:34

ramr added 3 commits April 30, 2018 13:28

Second version of helper code for sorting map files.

9e88ee0

Changes for review comments.

10f599a

Fixes as per @mfojtik review comments .. missed fixed up the godoc fo…

82cf85a

…r the template helper code.

ramr force-pushed the tmplsort-helpers branch from f08d22a to 82cf85a Compare April 30, 2018 22:33

smarterclayton reviewed May 1, 2018

View reviewed changes

Changes as per @smarterclayton review comments - make the haproxy map

5fea943

generation function to be scoped to the template its operating under.

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

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

openshift-merge-robot merged commit 1cdf917 into openshift:master May 7, 2018

ramr deleted the tmplsort-helpers branch May 7, 2018 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Second version of helper code for sorting map files. #19492

Second version of helper code for sorting map files. #19492

ramr commented Apr 24, 2018

mfojtik Apr 24, 2018

ramr Apr 24, 2018

mfojtik Apr 24, 2018

ramr Apr 24, 2018

mfojtik Apr 24, 2018

ramr Apr 24, 2018

mfojtik Apr 24, 2018

ramr Apr 24, 2018

mfojtik Apr 24, 2018

mfojtik Apr 24, 2018

ramr Apr 24, 2018

ramr commented Apr 24, 2018

ramr commented Apr 25, 2018

ramr commented Apr 25, 2018

ramr commented Apr 25, 2018

knobunc left a comment

knobunc commented Apr 30, 2018

ramr commented May 1, 2018

ramr commented May 1, 2018

smarterclayton May 1, 2018

smarterclayton May 1, 2018

smarterclayton May 1, 2018 •

edited

Loading

ramr May 2, 2018

smarterclayton May 1, 2018

smarterclayton May 1, 2018

ramr May 2, 2018

smarterclayton commented May 1, 2018

ramr commented May 2, 2018

knobunc commented May 3, 2018

openshift-ci-robot commented May 3, 2018

knobunc commented May 7, 2018

Second version of helper code for sorting map files. #19492

Second version of helper code for sorting map files. #19492

Conversation

ramr commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Apr 24, 2018

ramr commented Apr 25, 2018

ramr commented Apr 25, 2018

ramr commented Apr 25, 2018

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Apr 30, 2018

ramr commented May 1, 2018

ramr commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented May 1, 2018

ramr commented May 2, 2018

knobunc commented May 3, 2018

openshift-ci-robot commented May 3, 2018

knobunc commented May 7, 2018

smarterclayton May 1, 2018 •

edited

Loading