Changes CustomHTTPErrors annotation to use custom default backend

Updates e2e test

Removes focus from e2e test

Fixes renamed function

Adds tests for new template funcs

Addresses gofmt

Updates e2e test, fixes custom-default-backend test by creating service

Updates docs

docs/user-guide/nginx-configuration/annotations.md

@@ -248,23 +248,23 @@ nginx.ingress.kubernetes.io/configuration-snippet: |
   more_set_headers "Request-Id: $req_id";
 ```
 
-### Default Backend
-
-The ingress controller requires a [default backend](../default-backend.md).
-This service handles the response when the service in the Ingress rule does not have endpoints.
-This is a global configuration for the ingress controller. In some cases could be required to return a custom content or format. In this scenario we can use the annotation `nginx.ingress.kubernetes.io/default-backend: <svc name>` to specify a custom default backend.  This `<svc name>` is a reference to a service inside of the same namespace in which you are applying this annotation.
-
 ### Custom HTTP Errors
 
-Like the [`custom-http-errors`](./configmap.md#custom-http-errors) value in the ConfigMap, this annotation will set NGINX `proxy-intercept-errors`, but only for the NGINX location associated with this ingress.
+Like the [`custom-http-errors`](./configmap.md#custom-http-errors) value in the ConfigMap, this annotation will set NGINX `proxy-intercept-errors`, but only for the NGINX location associated with this ingress. If a [default backend annotation](#default-backend) is specified on the ingress, the errors will be routed to that annotation's default backend service (instead of the global default backend).
 Different ingresses can specify different sets of error codes. Even if multiple ingress objects share the same hostname, this annotation can be used to intercept different error codes for each ingress (for example, different error codes to be intercepted for different paths on the same hostname, if each path is on a different ingress).
 If `custom-http-errors` is also specified globally, the error values specified in this annotation will override the global value for the given ingress' hostname and path.
 
 Example usage:
 ```
-custom-http-errors: "404,415"
+nginx.ingress.kubernetes.io/custom-http-errors: "404,415"
 ```
 
+### Default Backend
+
+This annotation is of the form `nginx.ingress.kubernetes.io/default-backend: <svc name>` to specify a custom default backend.  This `<svc name>` is a reference to a service inside of the same namespace in which you are applying this annotation. This annotation overrides the global default backend.
+
+This service will be handle the response when the service in the Ingress rule does not have active endpoints. It will also handle the error responses if both this annotation and the [custom-http-errors annotation](#custom-http-errors) is set.
+
 ### Enable CORS
 
 To enable Cross-Origin Resource Sharing (CORS) in an Ingress rule, add the annotation

internal/ingress/controller/controller.go

@@ -581,24 +581,25 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
 		isHTTPSfrom := []*ingress.Server{}
 		for _, server := range servers {
 			for _, location := range server.Locations {
-				if upstream.Name == location.Backend {
-					if len(upstream.Endpoints) == 0 {
-						klog.V(3).Infof("Upstream %q has no active Endpoint", upstream.Name)
-						// check if the location contains endpoints and a custom default backend
-						if location.DefaultBackend != nil {
-							sp := location.DefaultBackend.Spec.Ports[0]
-							endps := getEndpoints(location.DefaultBackend, &sp, apiv1.ProtocolTCP, n.store.GetServiceEndpoints)
-							if len(endps) > 0 {
-								klog.V(3).Infof("Using custom default backend for location %q in server %q (Service \"%v/%v\")",
-									location.Path, server.Hostname, location.DefaultBackend.Namespace, location.DefaultBackend.Name)
+				if shouldCreateUpstreamForLocationDefaultBackend(upstream, location) {
+					sp := location.DefaultBackend.Spec.Ports[0]
+					endps := getEndpoints(location.DefaultBackend, &sp, apiv1.ProtocolTCP, n.store.GetServiceEndpoints)
+					if len(endps) > 0 {
 
-								nb := upstream.DeepCopy()
-								name := fmt.Sprintf("custom-default-backend-%v", upstream.Name)
-								nb.Name = name
-								nb.Endpoints = endps
-								aUpstreams = append(aUpstreams, nb)
-								location.Backend = name
-							}
+						name := fmt.Sprintf("custom-default-backend-%v", location.DefaultBackend.GetName())
+						klog.V(3).Infof("Creating \"%v\" upstream based on default backend annotation", name)
+
+						nb := upstream.DeepCopy()
+						nb.Name = name
+						nb.Endpoints = endps
+						aUpstreams = append(aUpstreams, nb)
+						location.DefaultBackendUpstreamName = name
+
+						if len(upstream.Endpoints) == 0 {
+							klog.V(3).Infof("Upstream %q has no active Endpoint, so using custom default backend for location %q in server %q (Service \"%v/%v\")",
+								upstream.Name, location.Path, server.Hostname, location.DefaultBackend.Namespace, location.DefaultBackend.Name)
+
+							location.Backend = name
 						}
 					}
 
@@ -611,6 +612,8 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
 							isHTTPSfrom = append(isHTTPSfrom, server)
 						}
 					}
+				} else {
+					location.DefaultBackendUpstreamName = "upstream-default-backend"
 				}
 			}
 		}
@@ -1347,3 +1350,10 @@ func getRemovedIngresses(rucfg, newcfg *ingress.Configuration) []string {
 
 	return oldIngresses.Difference(newIngresses).List()
 }
+
+// checks conditions for whether or not an upstream should be created for a custom default backend
+func shouldCreateUpstreamForLocationDefaultBackend(upstream *ingress.Backend, location *ingress.Location) bool {
+	return (upstream.Name == location.Backend) &&
+		(len(upstream.Endpoints) == 0 || len(location.CustomHTTPErrors) != 0) &&
+		location.DefaultBackend != nil
+}

internal/ingress/controller/template/template.go

@@ -28,6 +28,7 @@ import (
 	"os/exec"
 	"reflect"
 	"regexp"
+	"sort"
 	"strings"
 	text_template "text/template"
 	"time"
@@ -150,17 +151,17 @@ var (
 		"serverConfig": func(all config.TemplateConfig, server *ingress.Server) interface{} {
 			return struct{ First, Second interface{} }{all, server}
 		},
-		"isValidByteSize":              isValidByteSize,
-		"buildForwardedFor":            buildForwardedFor,
-		"buildAuthSignURL":             buildAuthSignURL,
-		"buildOpentracing":             buildOpentracing,
-		"proxySetHeader":               proxySetHeader,
-		"buildInfluxDB":                buildInfluxDB,
-		"enforceRegexModifier":         enforceRegexModifier,
-		"stripLocationModifer":         stripLocationModifer,
-		"buildCustomErrorDeps":         buildCustomErrorDeps,
-		"collectCustomErrorsPerServer": collectCustomErrorsPerServer,
-		"opentracingPropagateContext":  opentracingPropagateContext,
+		"isValidByteSize":                    isValidByteSize,
+		"buildForwardedFor":                  buildForwardedFor,
+		"buildAuthSignURL":                   buildAuthSignURL,
+		"buildOpentracing":                   buildOpentracing,
+		"proxySetHeader":                     proxySetHeader,
+		"buildInfluxDB":                      buildInfluxDB,
+		"enforceRegexModifier":               enforceRegexModifier,
+		"stripLocationModifer":               stripLocationModifer,
+		"buildCustomErrorDeps":               buildCustomErrorDeps,
+		"opentracingPropagateContext":        opentracingPropagateContext,
+		"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
 	}
 )
 
@@ -884,41 +885,72 @@ func proxySetHeader(loc interface{}) string {
 
 // buildCustomErrorDeps is a utility function returning a struct wrapper with
 // the data required to build the 'CUSTOM_ERRORS' template
-func buildCustomErrorDeps(proxySetHeaders map[string]string, errorCodes []int, enableMetrics bool) interface{} {
+func buildCustomErrorDeps(upstreamName string, errorCodes []int, enableMetrics bool) interface{} {
 	return struct {
-		ProxySetHeaders map[string]string
-		ErrorCodes      []int
-		EnableMetrics   bool
+		UpstreamName  string
+		ErrorCodes    []int
+		EnableMetrics bool
 	}{
-		ProxySetHeaders: proxySetHeaders,
-		ErrorCodes:      errorCodes,
-		EnableMetrics:   enableMetrics,
+		UpstreamName:  upstreamName,
+		ErrorCodes:    errorCodes,
+		EnableMetrics: enableMetrics,
 	}
 }
 
-// collectCustomErrorsPerServer is a utility function which will collect all
+type errorLocation struct {
+	UpstreamName string
+	Codes        []int
+}
+
+// buildCustomErrorLocationsPerServer is a utility function which will collect all
 // custom error codes for all locations of a server block, deduplicates them,
-// and returns a unique set (for the template to create @custom_xxx locations)
-func collectCustomErrorsPerServer(input interface{}) []int {
+// and returns a set which is unique by default-upstream and error code. It returns an array
+// of errorLocations, each of which contain the upstream name and a list of
+// error codes for that given upstream, so that sufficiently unique
+// @custom error location blocks can be created in the template
+func buildCustomErrorLocationsPerServer(input interface{}) interface{} {
 	server, ok := input.(*ingress.Server)
 	if !ok {
 		klog.Errorf("expected a '*ingress.Server' type but %T was returned", input)
 		return nil
 	}
 
-	codesMap := make(map[int]bool)
+	codesMap := make(map[string]map[int]bool)
 	for _, loc := range server.Locations {
-		for _, code := range loc.CustomHTTPErrors {
-			codesMap[code] = true
+		backendUpstream := loc.DefaultBackendUpstreamName
+
+		var dedupedCodes map[int]bool
+		if existingMap, ok := codesMap[backendUpstream]; ok {
+			dedupedCodes = existingMap
+		} else {
+			dedupedCodes = make(map[int]bool)
 		}
+
+		for _, code := range loc.CustomHTTPErrors {
+			dedupedCodes[code] = true
+		}
+		codesMap[backendUpstream] = dedupedCodes
 	}
 
-	uniqueCodes := make([]int, 0, len(codesMap))
-	for key := range codesMap {
-		uniqueCodes = append(uniqueCodes, key)
+	errorLocations := []errorLocation{}
+
+	for upstream, dedupedCodes := range codesMap {
+		codesForUpstream := []int{}
+		for code := range dedupedCodes {
+			codesForUpstream = append(codesForUpstream, code)
+		}
+		sort.Ints(codesForUpstream)
+		errorLocations = append(errorLocations, errorLocation{
+			UpstreamName: upstream,
+			Codes:        codesForUpstream,
+		})
 	}
 
-	return uniqueCodes
+	sort.Slice(errorLocations, func(i, j int) bool {
+		return errorLocations[i].UpstreamName < errorLocations[j].UpstreamName
+	})
+
+	return errorLocations
 }
 
 func opentracingPropagateContext(loc interface{}) string {

internal/ingress/controller/template/template_test.go

@@ -22,7 +22,6 @@ import (
 	"os"
 	"path"
 	"reflect"
-	"sort"
 	"strings"
 	"testing"
 
@@ -903,26 +902,106 @@ func TestGetIngressInformation(t *testing.T) {
 	}
 }
 
-func TestCollectCustomErrorsPerServer(t *testing.T) {
-	invalidType := &ingress.Ingress{}
-	customErrors := collectCustomErrorsPerServer(invalidType)
-	if customErrors != nil {
-		t.Errorf("Expected %v but returned %v", nil, customErrors)
-	}
-
-	server := &ingress.Server{
-		Locations: []*ingress.Location{
-			{CustomHTTPErrors: []int{404, 405}},
-			{CustomHTTPErrors: []int{404, 500}},
+func TestBuildCustomErrorLocationsPerServer(t *testing.T) {
+	testCases := []struct {
+		server          interface{}
+		expectedResults []errorLocation
+	}{
+		{ // Single ingress
+			&ingress.Server{Locations: []*ingress.Location{
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-backend",
+					CustomHTTPErrors:           []int{401, 402},
+				},
+			}},
+			[]errorLocation{
+				{
+					UpstreamName: "custom-default-backend-test-backend",
+					Codes:        []int{401, 402},
+				},
+			},
+		},
+		{ // Two ingresses, overlapping error codes, same backend
+			&ingress.Server{Locations: []*ingress.Location{
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-backend",
+					CustomHTTPErrors:           []int{401, 402},
+				},
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-backend",
+					CustomHTTPErrors:           []int{402, 403},
+				},
+			}},
+			[]errorLocation{
+				{
+					UpstreamName: "custom-default-backend-test-backend",
+					Codes:        []int{401, 402, 403},
+				},
+			},
+		},
+		{ // Two ingresses, overlapping error codes, different backends
+			&ingress.Server{Locations: []*ingress.Location{
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-one",
+					CustomHTTPErrors:           []int{401, 402},
+				},
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-two",
+					CustomHTTPErrors:           []int{402, 403},
+				},
+			}},
+			[]errorLocation{
+				{
+					UpstreamName: "custom-default-backend-test-one",
+					Codes:        []int{401, 402},
+				},
+				{
+					UpstreamName: "custom-default-backend-test-two",
+					Codes:        []int{402, 403},
+				},
+			},
+		},
+		{ // Many ingresses, overlapping error codes, different backends
+			&ingress.Server{Locations: []*ingress.Location{
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-one",
+					CustomHTTPErrors:           []int{401, 402},
+				},
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-one",
+					CustomHTTPErrors:           []int{501, 502},
+				},
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-two",
+					CustomHTTPErrors:           []int{409, 410},
+				},
+				{
+					DefaultBackendUpstreamName: "custom-default-backend-test-two",
+					CustomHTTPErrors:           []int{504, 505},
+				},
+			}},
+			[]errorLocation{
+				{
+					UpstreamName: "custom-default-backend-test-one",
+					Codes:        []int{401, 402, 501, 502},
+				},
+				{
+					UpstreamName: "custom-default-backend-test-two",
+					Codes:        []int{409, 410, 504, 505},
+				},
+			},
 		},
 	}
 
-	expected := []int{404, 405, 500}
-	uniqueCodes := collectCustomErrorsPerServer(server)
-	sort.Ints(uniqueCodes)
-
-	if !reflect.DeepEqual(expected, uniqueCodes) {
-		t.Errorf("Expected '%v' but got '%v'", expected, uniqueCodes)
+	for _, c := range testCases {
+		response := buildCustomErrorLocationsPerServer(c.server)
+		if results, ok := response.([]errorLocation); ok {
+			if !reflect.DeepEqual(c.expectedResults, results) {
+				t.Errorf("Expected %+v but got %+v", c.expectedResults, results)
+			}
+		} else {
+			t.Error("Unable to convert to []errorLocation")
+		}
 	}
 }
 

internal/ingress/types.go

@@ -287,6 +287,9 @@ type Location struct {
 	// DefaultBackend allows the use of a custom default backend for this location.
 	// +optional
 	DefaultBackend *apiv1.Service `json:"defaultBackend,omitempty"`
+	// DefaultBackendUpstreamName is the upstream-formatted string for the name of
+	// this location's custom default backend
+	DefaultBackendUpstreamName string `json:"defaultBackendUpstreamName,omitempty"`
 	// XForwardedPrefix allows to add a header X-Forwarded-Prefix to the request with the
 	// original location.
 	// +optional

internal/ingress/types_equals.go

@@ -479,6 +479,10 @@ func (l1 *Location) Equal(l2 *Location) bool {
 		return false
 	}
 
+	if l1.DefaultBackendUpstreamName != l2.DefaultBackendUpstreamName {
+		return false
+	}
+
 	return true
 }
 

rootfs/etc/nginx/template/nginx.tmpl

@@ -423,7 +423,7 @@ http {
     {{ end }}
 
     {{ range $errCode := $cfg.CustomHTTPErrors }}
-    error_page {{ $errCode }} = @custom_{{ $errCode }};{{ end }}
+    error_page {{ $errCode }} = @custom_upstream-default-backend_{{ $errCode }};{{ end }}
 
     proxy_ssl_session_reuse on;
 
@@ -603,7 +603,7 @@ http {
         {{ $cfg.ServerSnippet }}
         {{ end }}
 
-        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors $all.EnableMetrics) }}
+        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps "upstream-default-backend" $cfg.CustomHTTPErrors $all.EnableMetrics) }}
     }
     ## end server {{ $server.Hostname }}
 
@@ -798,10 +798,10 @@ stream {
 
 {{/* definition of templates to avoid repetitions */}}
 {{ define "CUSTOM_ERRORS" }}
-        {{ $proxySetHeaders := .ProxySetHeaders }}
         {{ $enableMetrics := .EnableMetrics }}
+        {{ $upstreamName := .UpstreamName }}
         {{ range $errCode := .ErrorCodes }}
-        location @custom_{{ $errCode }} {
+        location @custom_{{ $upstreamName }}_{{ $errCode }} {
             internal;
 
             proxy_intercept_errors off;
@@ -815,7 +815,7 @@ stream {
             proxy_set_header       X-Service-Port     $service_port;
             proxy_set_header       Host               $best_http_host;
 
-            set $proxy_upstream_name "upstream-default-backend";
+            set $proxy_upstream_name {{ $upstreamName }};
 
             rewrite                (.*) / break;
 
@@ -924,7 +924,10 @@ stream {
         {{ $server.ServerSnippet }}
         {{ end }}
 
-        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server) $all.EnableMetrics) }}
+        {{ range $errorLocation := (buildCustomErrorLocationsPerServer $server) }}
+        {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $errorLocation.UpstreamName $errorLocation.Codes $all.EnableMetrics) }}
+        {{ end }}
+
 
         {{ $enforceRegex := enforceRegexModifier $server.Locations }}
         {{ range $location := $server.Locations }}
@@ -1326,8 +1329,7 @@ stream {
             {{ end }}
 
             {{ range $errCode := $location.CustomHTTPErrors }}
-            error_page {{ $errCode }} = @custom_{{ $errCode }};{{ end }}
-
+            error_page {{ $errCode }} = @custom_{{ $location.DefaultBackendUpstreamName }}_{{ $errCode }};{{ end }}
 
             {{ buildProxyPass $server.Hostname $all.Backends $location }}
             {{ if (or (eq $location.Proxy.ProxyRedirectFrom "default") (eq $location.Proxy.ProxyRedirectFrom "off")) }}

test/e2e/annotations/customhttperrors.go

@@ -22,12 +22,17 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-
 	extensions "k8s.io/api/extensions/v1beta1"
 
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
+const defaultBackendName = "upstream-default-backend"
+
+func errorBlockName(upstreamName string, errorCode string) string {
+	return fmt.Sprintf("@custom_%s_%s", upstreamName, errorCode)
+}
+
 var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func() {
 	f := framework.NewDefaultFramework("custom-http-errors")
 
@@ -61,12 +66,12 @@ var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func(
 
 		By("configuring error_page directive")
 		for _, code := range errorCodes {
-			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("error_page %s = @custom_%s", code, code)))
+			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("error_page %s = %s", code, errorBlockName("upstream-default-backend", code))))
 		}
 
 		By("creating error locations")
 		for _, code := range errorCodes {
-			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("location @custom_%s", code)))
+			Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", code))))
 		}
 
 		By("updating configuration when only custom-http-error value changes")
@@ -82,9 +87,9 @@ var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func(
 			}
 			return false
 		})
-		Expect(serverConfig).Should(ContainSubstring("location @custom_503"))
-		Expect(serverConfig).ShouldNot(ContainSubstring("location @custom_400"))
-		Expect(serverConfig).ShouldNot(ContainSubstring("location @custom_500"))
+		Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", "503"))))
+		Expect(serverConfig).ShouldNot(ContainSubstring(fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", "404"))))
+		Expect(serverConfig).ShouldNot(ContainSubstring(fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", "500"))))
 
 		By("ignoring duplicate values (503 in this case) per server")
 		annotations["nginx.ingress.kubernetes.io/custom-http-errors"] = "404, 503"
@@ -94,7 +99,26 @@ var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func(
 			serverConfig = sc
 			return strings.Contains(serverConfig, "location /else")
 		})
-		count := strings.Count(serverConfig, fmt.Sprintf("location @custom_503"))
+		count := strings.Count(serverConfig, fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", "503")))
 		Expect(count).Should(Equal(1))
+
+		By("using the custom default-backend from annotation for upstream")
+		customDefaultBackend := "from-annotation"
+		f.NewEchoDeploymentWithNameAndReplicas(customDefaultBackend, 1)
+
+		err = framework.UpdateIngress(f.KubeClientSet, f.IngressController.Namespace, host, func(ingress *extensions.Ingress) error {
+			ingress.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/default-backend"] = customDefaultBackend
+			return nil
+		})
+		Expect(err).ToNot(HaveOccurred())
+		f.WaitForNginxServer(host, func(sc string) bool {
+			if serverConfig != sc {
+				serverConfig = sc
+				return true
+			}
+			return false
+		})
+		Expect(serverConfig).Should(ContainSubstring(errorBlockName(fmt.Sprintf("custom-default-backend-%s", customDefaultBackend), "503")))
+		Expect(serverConfig).Should(ContainSubstring(fmt.Sprintf("error_page %s = %s", "503", errorBlockName(fmt.Sprintf("custom-default-backend-%s", customDefaultBackend), "503"))))
 	})
 })