Merge pull request #19219 from ramr/sort-map

Template helper code to sort map files

Author: openshift-merge-robot

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) {
+	name := ""
+	tempFile, err := ioutil.TempFile("", prefix)
+	if 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)
+		}
+	}
+}