Add sorting template helper code to sort haproxy map files so that wildcard

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 #19076 to fix issue #16724

Author: ramr

images/router/haproxy/Dockerfile

@@ -10,7 +10,7 @@ RUN INSTALL_PKGS="haproxy18" && \
     rpm -V $INSTALL_PKGS && \
     yum clean all && \
     mkdir -p /var/lib/haproxy/router/{certs,cacerts} && \
-    mkdir -p /var/lib/haproxy/{conf,run,bin,log} && \
+    mkdir -p /var/lib/haproxy/{conf/.tmp,run,bin,log} && \
     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} && \
     setcap 'cap_net_bind_service=ep' /usr/sbin/haproxy && \
     chown -R :0 /var/lib/haproxy && \

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

@@ -471,7 +471,7 @@ backend be_tcp:{{$cfgIdx}}
 			[sub]domain regexps. This map is used to check if
 			a host matches a [sub]domain with has wildcard support.
 */}}
-{{ define "/var/lib/haproxy/conf/os_wildcard_domain.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_wildcard_domain.map" -}}
 {{     if isTrue (env "ROUTER_ALLOW_WILDCARD_ROUTES") -}}
 {{       range $idx, $cfg := .State -}}
 {{         if ne $cfg.Host "" -}}
@@ -480,9 +480,14 @@ backend be_tcp:{{$cfgIdx}}
 {{           end -}}
 {{         end -}}
 {{       end -}}
-{{     end -}}{{/* end if router allows wildcard routes */}}
-{{ end -}}{{/* end wildcard domain map template */}}
+{{     end -}}{{/* end if router allows wildcard routes */ -}}
+{{ end -}}{{/* end temporary wildcard domain map template */}}
 
+{{ define "/var/lib/haproxy/conf/os_wildcard_domain.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_wildcard_domain.map" true -}}
+{{ $data }}
+{{     end -}}
+{{ end -}}{{/* end wildcard domain map template */}}
 
 
 {{/*
@@ -491,7 +496,7 @@ backend be_tcp:{{$cfgIdx}}
                                                 be_edge_http for edge routes with InsecureEdgeTerminationPolicy Allow
                                                 be_secure for reencrypt routes with InsecureEdgeTerminationPolicy Allow
 */}}
-{{ define "/var/lib/haproxy/conf/os_http_be.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_http_be.map" -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (ne $cfg.Host "") (eq $cfg.TLSTermination "") -}}
 {{generateRouteRegexp $cfg.Host $cfg.Path $cfg.IsWildcard}} be_http:{{$idx}}
@@ -506,12 +511,19 @@ backend be_tcp:{{$cfgIdx}}
 {{     end -}}
 {{ end -}}
 
+{{ define "/var/lib/haproxy/conf/os_http_be.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_http_be.map" true -}}
+{{ $data }}
+{{     end -}}
+{{ end -}}{{/* end http host map template */}}
+
+
 {{/*
     os_edge_reencrypt_be.map : contains a mapping of www.example.com -> <service name>. This map is similar to os_http_be.map but for tls routes.
                          by attaching prefix: be_edge_http for edge terminated routes
                                               be_secure for reencrypt routes
 */}}
-{{ define "/var/lib/haproxy/conf/os_edge_reencrypt_be.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_edge_reencrypt_be.map" -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (ne $cfg.Host "") (eq $cfg.TLSTermination "edge") -}}
 {{generateRouteRegexp $cfg.Host $cfg.Path $cfg.IsWildcard}} be_edge_http:{{$idx}}
@@ -520,6 +532,12 @@ backend be_tcp:{{$cfgIdx}}
 {{generateRouteRegexp $cfg.Host $cfg.Path $cfg.IsWildcard}} be_secure:{{$idx}}
 {{       end -}}
 {{     end -}}
+{{ end -}}{{/* end temporary edge http host map template */}}
+
+{{ define "/var/lib/haproxy/conf/os_edge_reencrypt_be.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_edge_reencrypt_be.map" true -}}
+{{ $data }}
+{{     end -}}
 {{ end -}}{{/* end edge http host map template */}}
 
 
@@ -528,37 +546,56 @@ backend be_tcp:{{$cfgIdx}}
     Map is used to redirect insecure traffic to use a secure scheme (https)
     if acls match for routes that have the insecure option set to redirect.
 */}}
-{{ define "/var/lib/haproxy/conf/os_route_http_redirect.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_route_http_redirect.map" -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (ne $cfg.Host "") (eq $cfg.InsecureEdgeTerminationPolicy "Redirect") -}}
 {{generateRouteRegexp $cfg.Host $cfg.Path $cfg.IsWildcard}} {{$idx}}
 {{       end -}}
 {{     end -}}
+{{ end -}}{{/* end temporary redirect http host map template */}}
+
+{{ define "/var/lib/haproxy/conf/os_route_http_redirect.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_route_http_redirect.map" true -}}
+{{ $data }}
+{{     end -}}
 {{ end -}}{{/* end redirect http host map template */}}
 
 
 {{/*
     os_tcp_be.map: contains a mapping of www.example.com -> <service name>.  This map is used to discover the correct backend
                         by attaching a prefix (be_tcp: or be_secure:) by use_backend statements if acls are matched.
 */}}
-{{ define "/var/lib/haproxy/conf/os_tcp_be.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_tcp_be.map" -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (eq $cfg.Path "") (and (ne $cfg.Host "") (matchValues (print $cfg.TLSTermination) "passthrough" "reencrypt")) -}}
 {{generateRouteRegexp $cfg.Host "" $cfg.IsWildcard}} {{$idx}}
 {{       end -}}
 {{     end -}}
+{{ end -}}{{/* end temporary tcp host map template */}}
+
+{{ define "/var/lib/haproxy/conf/os_tcp_be.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_tcp_be.map" true -}}
+{{ $data }}
+{{     end -}}
 {{ end -}}{{/* end tcp host map template */}}
 
+
 {{/*
     os_sni_passthrough.map: contains a mapping of routes that expect to have an sni header and should be passed
     					through to the host_be.  Driven by the termination type of the ServiceAliasConfigs
 */}}
-{{ define "/var/lib/haproxy/conf/os_sni_passthrough.map" -}}
+{{ define "/var/lib/haproxy/conf/.tmp/os_sni_passthrough.map" -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (eq $cfg.Path "") (eq $cfg.TLSTermination "passthrough") -}}
 {{generateRouteRegexp $cfg.Host "" $cfg.IsWildcard}} 1
 {{       end -}}
 {{     end -}}
+{{ end -}}{{/* end temporary sni passthrough map template */}}
+
+{{ define "/var/lib/haproxy/conf/os_sni_passthrough.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/os_sni_passthrough.map" true -}}
+{{ $data }}
+{{     end -}}
 {{ end -}}{{/* end sni passthrough map template */}}
 
 {{/*
@@ -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" -}}
 {{     $workingDir := .WorkingDir -}}
 {{     range $idx, $cfg := .State -}}
 {{       if and (ne $cfg.Host "") (matchValues (print $cfg.TLSTermination) "edge" "reencrypt") -}}
@@ -579,4 +616,10 @@ backend be_tcp:{{$cfgIdx}}
 {{         end -}}
 {{      end -}}
 {{     end -}}
-{{ end }}{{/* end cert_config map template */}}
+{{ end }}{{/* end temporary cert_config map template */}}
+
+{{ define "/var/lib/haproxy/conf/cert_config.map" -}}
+{{     range $idx, $data := sortedMapData "/var/lib/haproxy/conf/.tmp/cert_config.map" false -}}
+{{ $data }}
+{{     end -}}
+{{ end -}}{{/* end cert_config map template */}}

images/router/haproxy/reload-haproxy

@@ -72,11 +72,6 @@ function haproxyHealthCheck() {
 retries=20
 
 
-# sort the path based map files for the haproxy map_beg function
-for mapfile in "$haproxy_conf_dir"/*.map; do
-  sort -r "$mapfile" -o "$mapfile"
-done
-
 old_pids=$(ps -A -opid,args | grep haproxy | egrep -v -e 'grep|reload-haproxy' | awk '{print $1}' | tr '\n' ' ')
 
 reload_status=0

pkg/router/template/router.go

@@ -9,6 +9,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"reflect"
+	"sort"
 	"strings"
 	"sync"
 	"text/template"
@@ -400,7 +401,13 @@ func (r *templateRouter) writeConfig() error {
 
 	glog.V(4).Infof("Router certificate manager config committed")
 
-	for path, template := range r.templates {
+	pathNames := make([]string, 0)
+	for k := range r.templates {
+		pathNames = append(pathNames, k)
+	}
+	sort.Strings(pathNames)
+	for _, path := range pathNames {
+		template := r.templates[path]
 		file, err := os.Create(path)
 		if err != nil {
 			return fmt.Errorf("error creating config file %s: %v", path, err)

pkg/router/template/template_helper.go

@@ -5,13 +5,16 @@ import (
 	"math/rand"
 	"os"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"text/template"
 
 	"github.com/golang/glog"
 
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
+	templateutil "github.com/openshift/origin/pkg/router/template/util"
+	fileutil "github.com/openshift/origin/pkg/util/file"
 )
 
 func isTrue(s string) bool {
@@ -170,6 +173,25 @@ func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
 	return endpoints
 }
 
+// sortedMapData returns the sorted data in a haproxy map. The returned data is
+// in alphabetically reverse order. The sortSubGroups option sorts the
+// 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)
+	if err != nil {
+		glog.Errorf("Error reading map file %s: %v", name, err)
+		return []string{}
+	}
+
+	if sortSubGroups {
+		return templateutil.SortMapPaths(lines, `^[^\.]*\.`)
+	}
+
+	sort.Sort(sort.Reverse(sort.StringSlice(lines)))
+	return lines
+}
+
 var helperFunctions = template.FuncMap{
 	"endpointsForAlias":        endpointsForAlias,        //returns the list of valid endpoints
 	"processEndpointsForAlias": processEndpointsForAlias, //returns the list of valid endpoints after processing them
@@ -184,4 +206,6 @@ var helperFunctions = template.FuncMap{
 
 	"isTrue":     isTrue,     //determines if a given variable is a true value
 	"firstMatch": firstMatch, //anchors provided regular expression and evaluates against given strings, returns the first matched string or ""
+
+	"sortedMapData": sortedMapData, //returns sorted map contents. The data can optionally be sorted on the non-wildcard and wildcard sub-groups
 }

pkg/router/template/template_helper_test.go

@@ -1,7 +1,10 @@
 package templaterouter
 
 import (
+	"fmt"
+	"io/ioutil"
 	"regexp"
+	"strings"
 	"testing"
 )
 
@@ -297,3 +300,112 @@ func TestMatchPattern(t *testing.T) {
 		}
 	}
 }
+
+func createTempMapFile(prefix string, data []string) (string, error) {
+	var err error
+	name := ""
+	if tempFile, err := ioutil.TempFile("", prefix); err != nil {
+		return "", fmt.Errorf("unexpected error creating temp file: %v", err)
+	}
+
+	name = tempFile.Name()
+	if err = tempFile.Close(); err != nil {
+		return name, fmt.Errorf("unexpected error creating temp file: %v", err)
+	}
+
+	if err := ioutil.WriteFile(name, []byte(strings.Join(data, "\n")), 0664); err != nil {
+		return name, fmt.Errorf("unexpected error writing temp file %s: %v", name, err)
+	}
+
+	return name, nil
+}
+
+func TestSortMapData(t *testing.T) {
+	testData := []string{
+		`^api-stg\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ stg:api-route`,
+		`^api-prod\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ prod:api-route`,
+		`^[^\.]*\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ prod:wildcard-route`,
+		`^3dev\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ dev:api-route`,
+		`^api-prod\.127\.0\.0\.1\.nip\.io(:[0-9]+)?/x/y/z(/.*)?$ prod:api-path-route`,
+		`^3app-admin\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ dev:admin-route`,
+		`^[^\.]*\.foo\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ devel2:foo-wildcard-route`,
+		`^zzz-production\.wildcard\.test(:[0-9]+)?/x/y/z(/.*)?$ test:api-route`,
+		`^backend-app\.127\.0\.0\.1\.nip\.io(:[0-9]+)?(/.*)?$ prod:backend-route`,
+		`^[^\.]*\.foo\.wildcard\.test(:[0-9]+)?(/.*)?$ devel2:foo-wildcard-test`,
+	}
+
+	expectedOrder := []string{
+		"test:api-route",
+		"prod:backend-route",
+		"stg:api-route",
+		"prod:api-path-route",
+		"prod:api-route",
+		"dev:api-route",
+		"dev:admin-route",
+		"devel2:foo-wildcard-test",
+		"devel2:foo-wildcard-route",
+		"prod:wildcard-route",
+	}
+
+	fileName, err := createTempMapFile("sort-map-data-test", testData)
+	if err != nil {
+		t.Errorf("TestSortMapData error: %v", err)
+	}
+
+	lines := sortedMapData(fileName, true)
+	if len(lines) != len(expectedOrder) {
+		t.Errorf("TestSortMapData sorted data length %d did not match expected length %d",
+			len(lines), len(expectedOrder))
+	}
+	for idx, suffix := range expectedOrder {
+		if !strings.HasSuffix(lines[idx], suffix) {
+			t.Errorf("TestSortMapData sorted data %s at index %d did not match expectation %s",
+				lines[idx], idx, suffix)
+		}
+	}
+}
+
+func TestSortMapCertConfigData(t *testing.T) {
+	testData := []string{
+		`/path/to/certs/stg:api-route.pem stg:api-route`,
+		`/path/to/certs/prod:api-route.pem prod:api-route`,
+		`/path/to/certs/prod:wildcard-route.pem prod:wildcard-route`,
+		`/path/to/certs/dev:api-route.pem dev:api-route`,
+		`/path/to/certs/prod:api-path-route prod:api-path-route`,
+		`/path/to/certs/dev:admin-route.pem dev:admin-route`,
+		`/path/to/certs/devel2:foo-wildcard-route.pem devel2:foo-wildcard-route`,
+		`/path/to/certs/test:api-route.pem test:api-route`,
+		`/path/to/certs/prod:backend-route.pem prod:backend-route`,
+		`/path/to/certs/devel2:foo-wildcard-test.pem devel2:foo-wildcard-test`,
+	}
+
+	expectedOrder := []string{
+		"test:api-route",
+		"stg:api-route",
+		"prod:wildcard-route",
+		"prod:backend-route",
+		"prod:api-route",
+		"prod:api-path-route",
+		"devel2:foo-wildcard-test",
+		"devel2:foo-wildcard-route",
+		"dev:api-route",
+		"dev:admin-route",
+	}
+
+	fileName, err := createTempMapFile("sort-map-cert-config-test", testData)
+	if err != nil {
+		t.Errorf("TestSortMapCertConfigData error: %v", err)
+	}
+
+	lines := sortedMapData(fileName, false)
+	if len(lines) != len(expectedOrder) {
+		t.Errorf("TestSortMapCertConfigData sorted data length %d did not match expected length %d",
+			len(lines), len(expectedOrder))
+	}
+	for idx, suffix := range expectedOrder {
+		if !strings.HasSuffix(lines[idx], suffix) {
+			t.Errorf("TestSortMapCertConfigData sorted data %s at index %d did not match expectation %s",
+				lines[idx], idx, suffix)
+		}
+	}
+}