Add support for exact paths when specified as locations (#197)

Goes some way to resolving #192. Right now, if an ingress specifies a path of e.g. /my/test/path, this will be rendered in the location block as location /my/test/path/. If a browser then hits /my/test/path it will be redirected to /my/test/path/ with a 301 permanent redirect. If /my/test/path is not a directory, but is instead an absolute path to a resource, then redirecting to /my/test/path/ simply doesn't make sense. This PR introduces the concept of "exact paths" by allowing ingresses to specify if all of their paths are absolute paths to resources. If so, we render the path as location = <raw path value>.
sky-uk · Mar 22, 2019 · b56c00f · b56c00f
1 parent ee39342
commit b56c00f
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+# v1.12.4
+* Add support for exact paths when specified as locations
+  https://github.com/sky-uk/feed/pull/197
+
 # v1.12.3
 * New flag to set the worker shutdown timeout
 

diff --git a/cmd/feed-ingress/main.go b/cmd/feed-ingress/main.go
@@ -84,6 +84,7 @@ const (
 	defaultIngressAllow                      = "0.0.0.0/0"
 	defaultIngressHealthPort                 = 8081
 	defaultIngressStripPath                  = true
+	defaultIngressExactPath                  = false
 	defaultHealthPort                        = 12082
 	defaultNginxBinary                       = "/usr/sbin/nginx"
 	defaultNginxWorkingDir                   = "/nginx"
@@ -159,6 +160,11 @@ func init() {
 			"limitations. URL encoded characters will not work correctly in some cases, and backend services will "+
 			"need to take care to properly construct URLs, such as by using the 'X-Original-URI' header."+
 			"Can be overridden with the sky.uk/strip-path annotation per ingress")
+	flag.BoolVar(&controllerConfig.DefaultExactPath, "ingress-exact-path", defaultIngressExactPath,
+		"Whether to consider the ingress path to be an exact match rather than as a prefix. For example, "+
+			"if enabled 'myhost/myapp/health' would match 'myhost/myapp/health' but not 'myhost/myapp/health/x'."+
+			" If disabled, it would match both (and redirect requests from 'myhost/myapp/health' to "+
+			" '/myhost/myapp/health/'. Can be overridden with the sky.uk/exact-path annotation per ingress")
 	flag.IntVar(&healthPort, "health-port", defaultHealthPort,
 		"Port for checking the health of the ingress controller on /health. Also provides /debug/pprof.")
 

diff --git a/controller/controller.go b/controller/controller.go
@@ -24,6 +24,7 @@ const (
 	// Deprecated: retained to maintain backwards compatibility.
 	frontendElbSchemeAnnotation = "sky.uk/frontend-elb-scheme"
 	stripPathAnnotation         = "sky.uk/strip-path"
+	exactPathAnnotation         = "sky.uk/exact-path"
 
 	// Old annotation - still supported to maintain backwards compatibility.
 	legacyBackendKeepAliveSeconds = "sky.uk/backend-keepalive-seconds"
@@ -53,6 +54,7 @@ type controller struct {
 	updaters                     []Updater
 	defaultAllow                 []string
 	defaultStripPath             bool
+	defaultExactPath             bool
 	defaultBackendTimeout        int
 	defaultBackendMaxConnections int
 	defaultProxyBufferSize       int
@@ -71,6 +73,7 @@ type Config struct {
 	Updaters                     []Updater
 	DefaultAllow                 string
 	DefaultStripPath             bool
+	DefaultExactPath             bool
 	DefaultBackendTimeoutSeconds int
 	DefaultBackendMaxConnections int
 	DefaultProxyBufferSize       int
@@ -84,6 +87,7 @@ func New(conf Config) Controller {
 		updaters:                     conf.Updaters,
 		defaultAllow:                 strings.Split(conf.DefaultAllow, ","),
 		defaultStripPath:             conf.DefaultStripPath,
+		defaultExactPath:             conf.DefaultExactPath,
 		defaultBackendTimeout:        conf.DefaultBackendTimeoutSeconds,
 		defaultBackendMaxConnections: conf.DefaultBackendMaxConnections,
 		defaultProxyBufferSize:       conf.DefaultProxyBufferSize,
@@ -182,6 +186,7 @@ func (c *controller) updateIngresses() error {
 						ServicePort:           int32(path.Backend.ServicePort.IntValue()),
 						Allow:                 c.defaultAllow,
 						StripPaths:            c.defaultStripPath,
+						ExactPath:             c.defaultExactPath,
 						BackendTimeoutSeconds: c.defaultBackendTimeout,
 						BackendMaxConnections: c.defaultBackendMaxConnections,
 						ProxyBufferSize:       c.defaultProxyBufferSize,
@@ -216,6 +221,16 @@ func (c *controller) updateIngresses() error {
 						}
 					}
 
+					if exactPath, ok := ingress.Annotations[exactPathAnnotation]; ok {
+						if exactPath == "true" {
+							entry.ExactPath = true
+						} else if exactPath == "false" {
+							entry.ExactPath = false
+						} else {
+							log.Warnf("Ingress %s has an invalid exact path annotation: %s. Using default", ingress.Name, exactPath)
+						}
+					}
+
 					if backendKeepAlive, ok := ingress.Annotations[legacyBackendKeepAliveSeconds]; ok {
 						tmp, _ := strconv.Atoi(backendKeepAlive)
 						entry.BackendTimeoutSeconds = tmp

diff --git a/controller/controller_test.go b/controller/controller_test.go
@@ -473,6 +473,54 @@ func TestUpdaterIsUpdatedOnK8sUpdates(t *testing.T) {
 			}},
 			defaultConfig(),
 		},
+		{
+			"ingress with exact path set to true",
+			createIngressesFixture(ingressHost, ingressSvcName, ingressSvcPort,
+				map[string]string{
+					ingressAllowAnnotation:      "",
+					exactPathAnnotation:         "true",
+					backendTimeoutSeconds:       "10",
+					frontendElbSchemeAnnotation: "internal",
+				}),
+			createDefaultServices(),
+			[]IngressEntry{{
+				Namespace:             ingressNamespace,
+				Name:                  ingressName,
+				Host:                  ingressHost,
+				Path:                  ingressPath,
+				ServiceAddress:        serviceIP,
+				ServicePort:           ingressSvcPort,
+				LbScheme:              "internal",
+				Allow:                 []string{},
+				ExactPath:             true,
+				BackendTimeoutSeconds: backendTimeout,
+			}},
+			defaultConfig(),
+		},
+		{
+			"ingress with exact path set to false",
+			createIngressesFixture(ingressHost, ingressSvcName, ingressSvcPort,
+				map[string]string{
+					ingressAllowAnnotation:      "",
+					exactPathAnnotation:         "false",
+					backendTimeoutSeconds:       "10",
+					frontendElbSchemeAnnotation: "internal",
+				}),
+			createDefaultServices(),
+			[]IngressEntry{{
+				Namespace:             ingressNamespace,
+				Name:                  ingressName,
+				Host:                  ingressHost,
+				Path:                  ingressPath,
+				ServiceAddress:        serviceIP,
+				ServicePort:           ingressSvcPort,
+				LbScheme:              "internal",
+				Allow:                 []string{},
+				ExactPath:             false,
+				BackendTimeoutSeconds: backendTimeout,
+			}},
+			defaultConfig(),
+		},
 		{
 			"ingress with overridden backend timeout",
 			createIngressesFixture(ingressHost, ingressSvcName, ingressSvcPort,
@@ -780,6 +828,8 @@ func createIngressesFixture(host string, serviceName string, servicePort int, in
 			annotations[ingressAllowAnnotation] = annotationVal
 		case stripPathAnnotation:
 			annotations[stripPathAnnotation] = annotationVal
+		case exactPathAnnotation:
+			annotations[exactPathAnnotation] = annotationVal
 		case frontendElbSchemeAnnotation:
 			annotations[frontendElbSchemeAnnotation] = annotationVal
 		case frontendSchemeAnnotation:

diff --git a/controller/ingress_entry.go b/controller/ingress_entry.go
@@ -32,6 +32,8 @@ type IngressEntry struct {
 	LbScheme string
 	// StripPaths before forwarding to the backend
 	StripPaths bool
+	// ExactPath indicates that the Path should be treated as an exact match rather than a prefix
+	ExactPath bool
 	// BackendTimeoutSeconds backend timeout
 	BackendTimeoutSeconds int
 	// BackendMaxConnections maximum backend connections

diff --git a/nginx/nginx.go b/nginx/nginx.go
@@ -139,6 +139,7 @@ type location struct {
 	UpstreamID            string
 	Allow                 []string
 	StripPath             bool
+	ExactPath             bool
 	BackendTimeoutSeconds int
 	ProxyBufferSize       int
 	ProxyBufferBlocks     int
@@ -461,6 +462,7 @@ func createServerEntries(entries controller.IngressEntries) []*server {
 			UpstreamID:            upstreamID(ingressEntry),
 			Allow:                 ingressEntry.Allow,
 			StripPath:             ingressEntry.StripPaths,
+			ExactPath:             ingressEntry.ExactPath,
 			BackendTimeoutSeconds: ingressEntry.BackendTimeoutSeconds,
 			ProxyBufferSize:       ingressEntry.ProxyBufferSize,
 			ProxyBufferBlocks:     ingressEntry.ProxyBufferBlocks,
@@ -497,7 +499,7 @@ func uniqueIngressEntries(entries controller.IngressEntries) []controller.Ingres
 
 	uniqueIngress := make(map[ingressKey]controller.IngressEntry)
 	for _, ingressEntry := range entries {
-		ingressEntry.Path = createNginxPath(ingressEntry.Path)
+		ingressEntry.Path = createNginxPath(ingressEntry.Path, ingressEntry.ExactPath)
 		key := ingressKey{ingressEntry.Host, ingressEntry.Path}
 		existingIngressEntry, exists := uniqueIngress[key]
 		if !exists {
@@ -515,7 +517,11 @@ func uniqueIngressEntries(entries controller.IngressEntries) []controller.Ingres
 	return uniqueIngressEntries
 }
 
-func createNginxPath(rawPath string) string {
+func createNginxPath(rawPath string, exactPath bool) string {
+	if exactPath {
+		return rawPath
+	}
+
 	nginxPath := strings.TrimSuffix(strings.TrimPrefix(rawPath, "/"), "/")
 	if len(nginxPath) == 0 {
 		nginxPath = "/"

diff --git a/nginx/nginx.tmpl b/nginx/nginx.tmpl
@@ -162,7 +162,7 @@ http {
 
         {{- range $location := $entry.Locations }}
 
-        location {{ if $location.Path }}{{ $location.Path }}{{ end }} {
+        location {{ if $location.Path }}{{ if $location.ExactPath }}= {{ end }}{{ $location.Path }}{{ end }} {
 {{- if $location.StripPath }}
             # Strip location path when proxying.
             # Beware this can cause issues with url encoded characters.

diff --git a/nginx/nginx_test.go b/nginx/nginx_test.go
@@ -482,6 +482,7 @@ func TestNginxIngressEntries(t *testing.T) {
 					ServicePort:           8080,
 					Allow:                 []string{"10.82.0.0/16"},
 					StripPaths:            true,
+					ExactPath:             false,
 					BackendTimeoutSeconds: 1,
 				},
 				{
@@ -493,6 +494,7 @@ func TestNginxIngressEntries(t *testing.T) {
 					ServicePort:           6060,
 					Allow:                 []string{"10.86.0.0/16"},
 					StripPaths:            false,
+					ExactPath:             false,
 					BackendTimeoutSeconds: 10,
 					BackendMaxConnections: 1024,
 				},
@@ -793,6 +795,25 @@ func TestNginxIngressEntries(t *testing.T) {
 				"        location /prefix-without-anyslash/ {\n",
 			},
 		},
+		{
+			"Check exact paths include equals",
+			defaultConf,
+			[]controller.IngressEntry{
+				{
+					Host:           "chris-0.com",
+					Namespace:      "core",
+					Name:           "chris-ingress",
+					Path:           "/a/test/path",
+					ServiceAddress: "service",
+					ServicePort:    9090,
+					ExactPath:      true,
+				},
+			},
+			nil,
+			[]string{
+				"        location = /a/test/path {\n",
+			},
+		},
 		{
 			"Check multiple allows work",
 			defaultConf,