From 0ebf0354cbf543e0e1bdea7a0dc97d1fa9cfdaa6 Mon Sep 17 00:00:00 2001 From: jasongwartz Date: Thu, 25 Oct 2018 18:35:48 +0200 Subject: [PATCH] Adds CustomHTTPErrors ingress annotation and test Adds per-server/location error-catch functionality to nginx template Adds documentation Reduces template duplication with helper function for CUSTOM_ERRORS data Updates documentation Adds e2e test for customerrors Removes AllCustomHTTPErrors, replaces with template function with deduplication and adds e2e test of deduplication Fixes copy-paste error in test, adds additional test cases Reverts noop change in controller.go (unused now) --- .../nginx-configuration/annotations.md | 12 ++ internal/ingress/annotations/annotations.go | 3 + .../ingress/annotations/annotations_test.go | 36 ++++++ .../annotations/customhttperrors/main.go | 57 +++++++++ internal/ingress/controller/controller.go | 2 + .../ingress/controller/template/template.go | 55 ++++++-- internal/ingress/types.go | 3 + rootfs/etc/nginx/template/nginx.tmpl | 18 ++- test/e2e/annotations/customhttperrors.go | 119 ++++++++++++++++++ 9 files changed, 294 insertions(+), 11 deletions(-) create mode 100644 internal/ingress/annotations/customhttperrors/main.go create mode 100644 test/e2e/annotations/customhttperrors.go diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index a9e4f9f4e..909031215 100644 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -31,6 +31,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/base-url-scheme](#rewrite)|string| |[nginx.ingress.kubernetes.io/client-body-buffer-size](#client-body-buffer-size)|string| |[nginx.ingress.kubernetes.io/configuration-snippet](#configuration-snippet)|string| +|[nginx.ingress.kubernetes.io/custom-http-errors](#custom-http-errors)|[]int| |[nginx.ingress.kubernetes.io/default-backend](#default-backend)|string| |[nginx.ingress.kubernetes.io/enable-cors](#enable-cors)|"true" or "false"| |[nginx.ingress.kubernetes.io/cors-allow-origin](#enable-cors)|string| @@ -213,6 +214,17 @@ 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: ` to specify a custom default backend. +### 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. +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" +``` + ### Enable CORS To enable Cross-Origin Resource Sharing (CORS) in an Ingress rule, add the annotation diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 4ab72a3e5..c59ab19d1 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -33,6 +33,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/clientbodybuffersize" "k8s.io/ingress-nginx/internal/ingress/annotations/connection" "k8s.io/ingress-nginx/internal/ingress/annotations/cors" + "k8s.io/ingress-nginx/internal/ingress/annotations/customhttperrors" "k8s.io/ingress-nginx/internal/ingress/annotations/defaultbackend" "k8s.io/ingress-nginx/internal/ingress/annotations/influxdb" "k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist" @@ -72,6 +73,7 @@ type Ingress struct { ConfigurationSnippet string Connection connection.Config CorsConfig cors.Config + CustomHTTPErrors []int DefaultBackend *apiv1.Service Denied error ExternalAuth authreq.Config @@ -112,6 +114,7 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor { "ConfigurationSnippet": snippet.NewParser(cfg), "Connection": connection.NewParser(cfg), "CorsConfig": cors.NewParser(cfg), + "CustomHTTPErrors": customhttperrors.NewParser(cfg), "DefaultBackend": defaultbackend.NewParser(cfg), "ExternalAuth": authreq.NewParser(cfg), "Proxy": proxy.NewParser(cfg), diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index a75e1a5f6..4f62c884d 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -43,6 +43,7 @@ var ( annotationAffinityCookieName = parser.GetAnnotationWithPrefix("session-cookie-name") annotationAffinityCookieHash = parser.GetAnnotationWithPrefix("session-cookie-hash") annotationUpstreamHashBy = parser.GetAnnotationWithPrefix("upstream-hash-by") + annotationCustomHTTPErrors = parser.GetAnnotationWithPrefix("custom-http-errors") ) type mockCfg struct { @@ -270,6 +271,41 @@ func TestCors(t *testing.T) { } } +func TestCustomHTTPErrors(t *testing.T) { + ec := NewAnnotationExtractor(mockCfg{}) + ing := buildIngress() + + fooAnns := []struct { + annotations map[string]string + er []int + }{ + {map[string]string{annotationCustomHTTPErrors: "404,415"}, []int{404, 415}}, + {map[string]string{annotationCustomHTTPErrors: "404"}, []int{404}}, + {map[string]string{annotationCustomHTTPErrors: ""}, []int{}}, + {map[string]string{annotationCustomHTTPErrors + "_no": "404"}, []int{}}, + {map[string]string{}, []int{}}, + {nil, []int{}}, + } + + for _, foo := range fooAnns { + ing.SetAnnotations(foo.annotations) + r := ec.Extract(ing).CustomHTTPErrors + + // Check that expected codes were created + for i := range foo.er { + if r[i] != foo.er[i] { + t.Errorf("Returned %v but expected %v", r, foo.er) + } + } + + // Check that no unexpected codes were also created + for i := range r { + if r[i] != foo.er[i] { + t.Errorf("Returned %v but expected %v", r, foo.er) + } + } + } +} /* func TestMergeLocationAnnotations(t *testing.T) { diff --git a/internal/ingress/annotations/customhttperrors/main.go b/internal/ingress/annotations/customhttperrors/main.go new file mode 100644 index 000000000..12b50d6c7 --- /dev/null +++ b/internal/ingress/annotations/customhttperrors/main.go @@ -0,0 +1,57 @@ +/* +Copyright 2015 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package customhttperrors + +import ( + "strconv" + "strings" + + extensions "k8s.io/api/extensions/v1beta1" + + "k8s.io/ingress-nginx/internal/ingress/annotations/parser" + "k8s.io/ingress-nginx/internal/ingress/resolver" +) + +type customhttperrors struct { + r resolver.Resolver +} + +// NewParser creates a new custom http errors annotation parser +func NewParser(r resolver.Resolver) parser.IngressAnnotation { + return customhttperrors{r} +} + +// Parse parses the annotations contained in the ingress to use +// custom http errors +func (e customhttperrors) Parse(ing *extensions.Ingress) (interface{}, error) { + c, err := parser.GetStringAnnotation("custom-http-errors", ing) + if err != nil { + return nil, err + } + + cSplit := strings.Split(c, ",") + var codes []int + for _, i := range cSplit { + num, err := strconv.Atoi(i) + if err != nil { + return nil, err + } + codes = append(codes, num) + } + + return codes, nil +} diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 5739f805b..e1b88ca6b 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -351,6 +351,7 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([] loc.InfluxDB = anns.InfluxDB loc.DefaultBackend = anns.DefaultBackend loc.BackendProtocol = anns.BackendProtocol + loc.CustomHTTPErrors = anns.CustomHTTPErrors if loc.Redirect.FromToWWW { server.RedirectFromToWWW = true @@ -391,6 +392,7 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([] InfluxDB: anns.InfluxDB, DefaultBackend: anns.DefaultBackend, BackendProtocol: anns.BackendProtocol, + CustomHTTPErrors: anns.CustomHTTPErrors, } if loc.Redirect.FromToWWW { diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 0f8b3ee65..393df6c04 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -151,14 +151,16 @@ var ( "serverConfig": func(all config.TemplateConfig, server *ingress.Server) interface{} { return struct{ First, Second interface{} }{all, server} }, - "isValidClientBodyBufferSize": isValidClientBodyBufferSize, - "buildForwardedFor": buildForwardedFor, - "buildAuthSignURL": buildAuthSignURL, - "buildOpentracing": buildOpentracing, - "proxySetHeader": proxySetHeader, - "buildInfluxDB": buildInfluxDB, - "enforceRegexModifier": enforceRegexModifier, - "stripLocationModifer": stripLocationModifer, + "isValidClientBodyBufferSize": isValidClientBodyBufferSize, + "buildForwardedFor": buildForwardedFor, + "buildAuthSignURL": buildAuthSignURL, + "buildOpentracing": buildOpentracing, + "proxySetHeader": proxySetHeader, + "buildInfluxDB": buildInfluxDB, + "enforceRegexModifier": enforceRegexModifier, + "stripLocationModifer": stripLocationModifer, + "buildCustomErrorDeps": buildCustomErrorDeps, + "collectCustomErrorsPerServer": collectCustomErrorsPerServer, } ) @@ -941,3 +943,40 @@ func proxySetHeader(loc interface{}) string { return "proxy_set_header" } + +// 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) interface{} { + return struct { + ProxySetHeaders map[string]string + ErrorCodes []int + }{ + ProxySetHeaders: proxySetHeaders, + ErrorCodes: errorCodes, + } +} + +// collectCustomErrorsPerServer 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 { + server, ok := input.(*ingress.Server) + if !ok { + glog.Errorf("expected a '*ingress.Server' type but %T was returned", input) + return nil + } + + codesMap := make(map[int]bool) + for _, loc := range server.Locations { + for _, code := range loc.CustomHTTPErrors { + codesMap[code] = true + } + } + + uniqueCodes := make([]int, 0, len(codesMap)) + for key := range codesMap { + uniqueCodes = append(uniqueCodes, key) + } + + return uniqueCodes +} diff --git a/internal/ingress/types.go b/internal/ingress/types.go index fea5b0c28..91011c4e6 100644 --- a/internal/ingress/types.go +++ b/internal/ingress/types.go @@ -253,6 +253,9 @@ type Location struct { // BackendProtocol indicates which protocol should be used to communicate with the service // By default this is HTTP BackendProtocol string `json:"backend-protocol"` + // CustomHTTPErrors specifies the error codes that should be intercepted. + // +optional + CustomHTTPErrors []int `json:"custom-http-errors"` } // SSLPassthroughBackend describes a SSL upstream server configured diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 82370bba7..19441efb9 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -575,7 +575,7 @@ http { {{ $cfg.ServerSnippet }} {{ end }} - {{ template "CUSTOM_ERRORS" $all }} + {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors) }} } ## end server {{ $server.Hostname }} @@ -677,7 +677,7 @@ http { proxy_pass http://upstream_balancer; } - {{ template "CUSTOM_ERRORS" $all }} + {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders $cfg.CustomHTTPErrors) }} } } @@ -696,7 +696,7 @@ stream { {{/* definition of templates to avoid repetitions */}} {{ define "CUSTOM_ERRORS" }} {{ $proxySetHeaders := .ProxySetHeaders }} - {{ range $errCode := .Cfg.CustomHTTPErrors }} + {{ range $errCode := .ErrorCodes }} location @custom_{{ $errCode }} { internal; @@ -818,6 +818,8 @@ stream { {{ $server.ServerSnippet }} {{ end }} + {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $all.ProxySetHeaders (collectCustomErrorsPerServer $server)) }} + {{ $enforceRegex := enforceRegexModifier $server.Locations }} {{ range $location := $server.Locations }} {{ $path := buildLocation $location $enforceRegex }} @@ -1173,6 +1175,16 @@ stream { proxy_set_header X-Service-Port $service_port; {{ end }} + {{/* if a location-specific error override is set, add the proxy_intercept here */}} + {{ if $location.CustomHTTPErrors }} + # Custom error pages per ingress + proxy_intercept_errors on; + {{ end }} + + {{ range $errCode := $location.CustomHTTPErrors }} + error_page {{ $errCode }} = @custom_{{ $errCode }};{{ end }} + + {{ buildProxyPass $server.Hostname $all.Backends $location }} {{ if (or (eq $location.Proxy.ProxyRedirectFrom "default") (eq $location.Proxy.ProxyRedirectFrom "off")) }} proxy_redirect {{ $location.Proxy.ProxyRedirectFrom }}; diff --git a/test/e2e/annotations/customhttperrors.go b/test/e2e/annotations/customhttperrors.go new file mode 100644 index 000000000..c74b62985 --- /dev/null +++ b/test/e2e/annotations/customhttperrors.go @@ -0,0 +1,119 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package annotations + +import ( + "fmt" + "strings" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "k8s.io/ingress-nginx/test/e2e/framework" +) + +var _ = framework.IngressNginxDescribe("Annotations - custom-http-errors", func() { + f := framework.NewDefaultFramework("custom-http-errors") + + BeforeEach(func() { + f.NewEchoDeploymentWithReplicas(2) + }) + + AfterEach(func() { + }) + + It("should set proxy_intercept_errors", func() { + host := "customerrors.foo.com" + + errorCodes := []string{"404", "500"} + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(errorCodes, ","), + } + + ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return Expect(server).Should(ContainSubstring("proxy_intercept_errors on;")) + }) + }) + + It("should create error routes", func() { + host := "customerrors.foo.com" + errorCodes := []string{"404", "500"} + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(errorCodes, ","), + } + + ing := framework.NewSingleIngress(host, "/test", host, f.IngressController.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + + for _, code := range errorCodes { + f.WaitForNginxServer(host, + func(server string) bool { + return Expect(server).Should(ContainSubstring(fmt.Sprintf("@custom_%s", code))) + }) + } + }) + + It("should set up error_page routing", func() { + host := "customerrors.foo.com" + errorCodes := []string{"404", "500"} + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(errorCodes, ","), + } + + ing := framework.NewSingleIngress(host, "/test", host, f.IngressController.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + + for _, code := range errorCodes { + f.WaitForNginxServer(host, + func(server string) bool { + return Expect(server).Should(ContainSubstring(fmt.Sprintf("error_page %s = @custom_%s", code, code))) + }) + } + }) + + It("should create only one of each error route", func() { + host := "customerrors.foo.com" + errorCodes := [][]string{{"404", "500"}, {"400", "404"}} + + for i, codeSet := range errorCodes { + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(codeSet, ","), + } + + ing := framework.NewSingleIngress( + fmt.Sprintf("%s-%d", host, i), fmt.Sprintf("/test-%d", i), + host, f.IngressController.Namespace, "http-svc", 80, &annotations) + f.EnsureIngress(ing) + } + + for _, codeSet := range errorCodes { + for _, code := range codeSet { + f.WaitForNginxServer(host, + func(server string) bool { + count := strings.Count(server, fmt.Sprintf("location @custom_%s", code)) + return Expect(count).Should(Equal(1)) + }) + } + } + }) +})