From 0a4d5b27c63c8ac30901d1f2794da1b7ceb16906 Mon Sep 17 00:00:00 2001 From: Zadkiel AHARONIAN Date: Wed, 23 Mar 2022 12:44:32 +0100 Subject: [PATCH] feat(annotations): introduce enable-custom-http-errors annotation Signed-off-by: GitHub --- .../nginx-configuration/annotations-risk.md | 1 + .../nginx-configuration/annotations.md | 5 + .../ingress/annotations/annotations_test.go | 39 +++++--- .../annotations/customhttperrors/main.go | 26 ++++- .../annotations/customhttperrors/main_test.go | 94 ++++++++++++++++--- test/e2e/annotations/customhttperrors.go | 28 +++++- 6 files changed, 161 insertions(+), 32 deletions(-) diff --git a/docs/user-guide/nginx-configuration/annotations-risk.md b/docs/user-guide/nginx-configuration/annotations-risk.md index be24c09309..4debbd919c 100755 --- a/docs/user-guide/nginx-configuration/annotations-risk.md +++ b/docs/user-guide/nginx-configuration/annotations-risk.md @@ -33,6 +33,7 @@ | CorsConfig | cors-max-age | Low | ingress | | CorsConfig | enable-cors | Low | ingress | | CustomHTTPErrors | custom-http-errors | Low | location | +| CustomHTTPErrors | enable-custom-http-errors | Low | location | | CustomHeaders | custom-headers | Medium | location | | DefaultBackend | default-backend | Low | location | | Denylist | denylist-source-range | Medium | location | diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index bc72a692ce..3e9357ab70 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -49,6 +49,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/canary-weight-total](#canary)|number| |[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/enable-custom-http-errors](#custom-http-errors)|"true" or "false"| |[nginx.ingress.kubernetes.io/custom-http-errors](#custom-http-errors)|[]int| |[nginx.ingress.kubernetes.io/custom-headers](#custom-headers)|string| |[nginx.ingress.kubernetes.io/default-backend](#default-backend)|string| @@ -329,6 +330,10 @@ Like the [`custom-http-errors`](./configmap.md#custom-http-errors) value in the 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. +If you want to disable this behavior for a specific ingress, you can use the annotation `nginx.ingress.kubernetes.io/enable-custom-http-errors: "false"`. +`nginx.ingress.kubernetes.io/enable-custom-http-errors`: + indicates if the custom http errors feature should be enabled or not to this Ingress rule. The default value is `"true"`. + Example usage: ``` nginx.ingress.kubernetes.io/custom-http-errors: "404,415" diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 5df3cdc0ee..9f3fb0f54a 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -29,21 +29,22 @@ import ( ) var ( - annotationPassthrough = parser.GetAnnotationWithPrefix("ssl-passthrough") - annotationAffinityType = parser.GetAnnotationWithPrefix("affinity") - annotationAffinityMode = parser.GetAnnotationWithPrefix("affinity-mode") - annotationAffinityCanaryBehavior = parser.GetAnnotationWithPrefix("affinity-canary-behavior") - annotationCorsEnabled = parser.GetAnnotationWithPrefix("enable-cors") - annotationCorsAllowMethods = parser.GetAnnotationWithPrefix("cors-allow-methods") - annotationCorsAllowHeaders = parser.GetAnnotationWithPrefix("cors-allow-headers") - annotationCorsExposeHeaders = parser.GetAnnotationWithPrefix("cors-expose-headers") - annotationCorsAllowCredentials = parser.GetAnnotationWithPrefix("cors-allow-credentials") - defaultCorsMethods = "GET, PUT, POST, DELETE, PATCH, OPTIONS" - defaultCorsHeaders = "DNT,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization" - annotationAffinityCookieName = parser.GetAnnotationWithPrefix("session-cookie-name") - annotationUpstreamHashBy = parser.GetAnnotationWithPrefix("upstream-hash-by") - annotationCustomHTTPErrors = parser.GetAnnotationWithPrefix("custom-http-errors") - annotationCustomHeaders = parser.GetAnnotationWithPrefix("custom-headers") + annotationPassthrough = parser.GetAnnotationWithPrefix("ssl-passthrough") + annotationAffinityType = parser.GetAnnotationWithPrefix("affinity") + annotationAffinityMode = parser.GetAnnotationWithPrefix("affinity-mode") + annotationAffinityCanaryBehavior = parser.GetAnnotationWithPrefix("affinity-canary-behavior") + annotationCorsEnabled = parser.GetAnnotationWithPrefix("enable-cors") + annotationCorsAllowMethods = parser.GetAnnotationWithPrefix("cors-allow-methods") + annotationCorsAllowHeaders = parser.GetAnnotationWithPrefix("cors-allow-headers") + annotationCorsExposeHeaders = parser.GetAnnotationWithPrefix("cors-expose-headers") + annotationCorsAllowCredentials = parser.GetAnnotationWithPrefix("cors-allow-credentials") + defaultCorsMethods = "GET, PUT, POST, DELETE, PATCH, OPTIONS" + defaultCorsHeaders = "DNT,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization" + annotationAffinityCookieName = parser.GetAnnotationWithPrefix("session-cookie-name") + annotationUpstreamHashBy = parser.GetAnnotationWithPrefix("upstream-hash-by") + annotationCustomHTTPErrorsEnabled = parser.GetAnnotationWithPrefix("enable-custom-http-errors") + annotationCustomHTTPErrors = parser.GetAnnotationWithPrefix("custom-http-errors") + annotationCustomHeaders = parser.GetAnnotationWithPrefix("custom-headers") ) type mockCfg struct { @@ -298,6 +299,14 @@ func TestCustomHTTPErrors(t *testing.T) { {map[string]string{annotationCustomHTTPErrors: "404"}, []int{404}}, {map[string]string{annotationCustomHTTPErrors: ""}, []int{}}, {map[string]string{annotationCustomHTTPErrors + "_no": "404"}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "true", annotationCustomHTTPErrors: "404,415"}, []int{404, 415}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "true", annotationCustomHTTPErrors: "404"}, []int{404}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "true", annotationCustomHTTPErrors: ""}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "true", annotationCustomHTTPErrors + "_no": "404"}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "false", annotationCustomHTTPErrors: "404,415"}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "false", annotationCustomHTTPErrors: "404"}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "false", annotationCustomHTTPErrors: ""}, []int{}}, + {map[string]string{annotationCustomHTTPErrorsEnabled: "false", annotationCustomHTTPErrors + "_no": "404"}, []int{}}, {map[string]string{}, []int{}}, {nil, []int{}}, } diff --git a/internal/ingress/annotations/customhttperrors/main.go b/internal/ingress/annotations/customhttperrors/main.go index f3c72a22fc..fa8ef45332 100644 --- a/internal/ingress/annotations/customhttperrors/main.go +++ b/internal/ingress/annotations/customhttperrors/main.go @@ -22,13 +22,16 @@ import ( "strings" networking "k8s.io/api/networking/v1" + "k8s.io/klog/v2" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" + "k8s.io/ingress-nginx/internal/ingress/errors" "k8s.io/ingress-nginx/internal/ingress/resolver" ) const ( - customHTTPErrorsAnnotation = "custom-http-errors" + customHTTPErrorsAnnotation = "custom-http-errors" + customHTTPErrorsEnabledAnnotation = "enable-custom-http-errors" ) // We accept anything between 400 and 599, on a comma separated. @@ -37,13 +40,20 @@ var arrayOfHTTPErrors = regexp.MustCompile(`^(?:[4,5]\d{2},?)*$`) var customHTTPErrorsAnnotations = parser.Annotation{ Group: "backend", Annotations: parser.AnnotationFields{ + customHTTPErrorsEnabledAnnotation: { + Validator: parser.ValidateBool, + Scope: parser.AnnotationScopeLocation, + Risk: parser.AnnotationRiskLow, + Documentation: `Indicates if the custom http errors feature should be enabled or not for this Ingress rule. The default value is "true".`, + }, + customHTTPErrorsAnnotation: { Validator: parser.ValidateRegex(arrayOfHTTPErrors, true), Scope: parser.AnnotationScopeLocation, Risk: parser.AnnotationRiskLow, Documentation: `If a default backend annotation is specified on the ingress, the errors code specified on this annotation will be routed to that annotation's default backend service. Otherwise they will be routed to the global default backend. - A comma-separated list of error codes is accepted (anything between 400 and 599, like 403, 503)`, + A comma-separated list of error codes is accepted (anything between 400 and 599, like 403, 503).`, }, }, } @@ -64,6 +74,18 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { // Parse parses the annotations contained in the ingress to use // custom http errors func (e customhttperrors) Parse(ing *networking.Ingress) (interface{}, error) { + enabled, err := parser.GetBoolAnnotation(customHTTPErrorsEnabledAnnotation, ing, e.annotationConfig.Annotations) + if err != nil { + if errors.IsValidationError(err) { + klog.Warningf("%s is invalid, defaulting to 'true'", customHTTPErrorsEnabledAnnotation) + } + enabled = true + } + + if !enabled { + return []int{}, nil + } + c, err := parser.GetStringAnnotation(customHTTPErrorsAnnotation, ing, e.annotationConfig.Annotations) if err != nil { return nil, err diff --git a/internal/ingress/annotations/customhttperrors/main_test.go b/internal/ingress/annotations/customhttperrors/main_test.go index 1f87247ed9..972852c744 100644 --- a/internal/ingress/annotations/customhttperrors/main_test.go +++ b/internal/ingress/annotations/customhttperrors/main_test.go @@ -18,7 +18,6 @@ package customhttperrors import ( "reflect" - "sort" "testing" api "k8s.io/api/core/v1" @@ -28,6 +27,10 @@ import ( "k8s.io/ingress-nginx/internal/ingress/resolver" ) +const DefaultHTTPErrorsString = "400,404,500,502" + +var DefaultHTTPErrorsSlice = []int{400, 404, 500, 502} + func buildIngress() *networking.Ingress { return &networking.Ingress{ ObjectMeta: meta_v1.ObjectMeta{ @@ -47,32 +50,57 @@ func buildIngress() *networking.Ingress { } } -func TestParseInvalidAnnotations(t *testing.T) { +func TestParseCodes(t *testing.T) { ing := buildIngress() - _, err := NewParser(&resolver.Mock{}).Parse(ing) - if err == nil { - t.Errorf("expected error parsing ingress with custom-http-errors") + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("custom-http-errors")] = DefaultHTTPErrorsString + ing.SetAnnotations(data) + + i, err := NewParser(&resolver.Mock{}).Parse(ing) + if err != nil { + t.Errorf("unexpected error parsing ingress with custom-http-errors") + } + val, ok := i.([]int) + if !ok { + t.Errorf("expected a []int type") } + expected := DefaultHTTPErrorsSlice + if !reflect.DeepEqual(expected, val) { + t.Errorf("expected %v but got %v", expected, val) + } +} + +func TestEnabledSwitch(t *testing.T) { + ing := buildIngress() + data := map[string]string{} - data[parser.GetAnnotationWithPrefix("custom-http-errors")] = "400,404,abc,502" + data[parser.GetAnnotationWithPrefix("enable-custom-http-errors")] = "true" + data[parser.GetAnnotationWithPrefix("custom-http-errors")] = DefaultHTTPErrorsString ing.SetAnnotations(data) i, err := NewParser(&resolver.Mock{}).Parse(ing) - if err == nil { - t.Errorf("expected error parsing ingress with custom-http-errors") + if err != nil { + t.Errorf("unexpected error parsing ingress with custom-http-errors") } - if i != nil { - t.Errorf("expected %v but got %v", nil, i) + val, ok := i.([]int) + if !ok { + t.Errorf("expected a []int type") + } + + expected := DefaultHTTPErrorsSlice + if !reflect.DeepEqual(expected, val) { + t.Errorf("expected %v but got %v", expected, val) } } -func TestParseAnnotations(t *testing.T) { +func TestDisabledSwitch(t *testing.T) { ing := buildIngress() data := map[string]string{} - data[parser.GetAnnotationWithPrefix("custom-http-errors")] = "400,404,500,502" + data[parser.GetAnnotationWithPrefix("enable-custom-http-errors")] = "false" + data[parser.GetAnnotationWithPrefix("custom-http-errors")] = DefaultHTTPErrorsString ing.SetAnnotations(data) i, err := NewParser(&resolver.Mock{}).Parse(ing) @@ -84,10 +112,48 @@ func TestParseAnnotations(t *testing.T) { t.Errorf("expected a []int type") } - expected := []int{400, 404, 500, 502} - sort.Ints(val) + expected := []int{} + if !reflect.DeepEqual(expected, val) { + t.Errorf("expected %v but got %v", expected, val) + } +} +func TestEnabledByDefault(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("enable-custom-http-errors")] = "fakebool" + data[parser.GetAnnotationWithPrefix("custom-http-errors")] = DefaultHTTPErrorsString + ing.SetAnnotations(data) + + i, err := NewParser(&resolver.Mock{}).Parse(ing) + if err != nil { + t.Errorf("unexpected error parsing ingress with custom-http-errors") + } + val, ok := i.([]int) + if !ok { + t.Errorf("expected a []int type") + } + + expected := DefaultHTTPErrorsSlice if !reflect.DeepEqual(expected, val) { t.Errorf("expected %v but got %v", expected, val) } } + +func TestErrorOnInvalidCode(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("enable-custom-http-errors")] = "true" + data[parser.GetAnnotationWithPrefix("custom-http-errors")] = "400,404,fakeint,502" + ing.SetAnnotations(data) + + i, err := NewParser(&resolver.Mock{}).Parse(ing) + if err == nil { + t.Errorf("expected error parsing ingress with custom-http-errors") + } + if i != nil { + t.Errorf("expected %v but got %v", nil, i) + } +} diff --git a/test/e2e/annotations/customhttperrors.go b/test/e2e/annotations/customhttperrors.go index 37a3e9695f..efd397eb09 100644 --- a/test/e2e/annotations/customhttperrors.go +++ b/test/e2e/annotations/customhttperrors.go @@ -38,7 +38,7 @@ var _ = framework.DescribeAnnotation("custom-http-errors", func() { f.NewEchoDeployment() }) - ginkgo.It("configures Nginx correctly", func() { + ginkgo.It("configures Nginx correctly when enabled", func() { host := "customerrors.foo.com" errorCodes := []string{"404", "500"} @@ -119,4 +119,30 @@ var _ = framework.DescribeAnnotation("custom-http-errors", func() { assert.Contains(ginkgo.GinkgoT(), serverConfig, errorBlockName(fmt.Sprintf("custom-default-backend-%s-%s", f.Namespace, customDefaultBackend), "503")) assert.Contains(ginkgo.GinkgoT(), serverConfig, fmt.Sprintf("error_page %s = %s", "503", errorBlockName(fmt.Sprintf("custom-default-backend-%s-%s", f.Namespace, customDefaultBackend), "503"))) }) + + ginkgo.It("configures Nginx correctly when disabled", func() { + host := "customerrors.foo.com" + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/enable-custom-http-errors": "false", + "nginx.ingress.kubernetes.io/custom-http-errors": "404, 503", + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + f.EnsureIngress(ing) + + var serverConfig string + f.WaitForNginxServer(host, func(sc string) bool { + serverConfig = sc + return strings.Contains(serverConfig, fmt.Sprintf("server_name %s", host)) + }) + + f.UpdateNginxConfigMapData("custom-http-errors", "404, 503") + + ginkgo.By("not turning on proxy_intercept_errors directive") + assert.NotContains(ginkgo.GinkgoT(), serverConfig, "proxy_intercept_errors on;") + + ginkgo.By("not creating error locations") + assert.NotContains(ginkgo.GinkgoT(), serverConfig, fmt.Sprintf("location %s", errorBlockName("upstream-default-backend", "off"))) + }) })