From 09832557eda1c571687eeee246b0f08c48c193e3 Mon Sep 17 00:00:00 2001 From: Karl Stoney Date: Mon, 23 Jul 2018 14:46:28 +0100 Subject: [PATCH 1/2] Add permanent-redirect-code annotation --- .../nginx-configuration/annotations.md | 5 + .../ingress/annotations/redirect/redirect.go | 13 +- .../annotations/redirect/redirect_test.go | 127 ++++++++++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 internal/ingress/annotations/redirect/redirect_test.go diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 56551b4e6d..0ca62be345 100644 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -43,6 +43,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/limit-connections](#rate-limiting)|number| |[nginx.ingress.kubernetes.io/limit-rps](#rate-limiting)|number| |[nginx.ingress.kubernetes.io/permanent-redirect](#permanent-redirect)|string| +|[nginx.ingress.kubernetes.io/permanent-redirect-code](#permanent-redirect-code)|number| |[nginx.ingress.kubernetes.io/proxy-body-size](#custom-max-body-size)|string| |[nginx.ingress.kubernetes.io/proxy-cookie-domain](#proxy-cookie-domain)|string| |[nginx.ingress.kubernetes.io/proxy-connect-timeout](#custom-timeouts)|number| @@ -366,6 +367,10 @@ To configure this setting globally for all Ingress rules, the `limit-rate-after` This annotation allows to return a permanent redirect instead of sending data to the upstream. For example `nginx.ingress.kubernetes.io/permanent-redirect: https://www.google.com` would redirect everything to Google. +### Permanent Redirect Code + +This annotation allows you to modify the status code used for permanent redirects. For example `nginx.ingress.kubernetes.io/permanent-redirect-code: '308'` would return your permanet-redirect with a 308. + ### SSL Passthrough The annotation `nginx.ingress.kubernetes.io/ssl-passthrough` allows to configure TLS termination in the pod and not in NGINX. diff --git a/internal/ingress/annotations/redirect/redirect.go b/internal/ingress/annotations/redirect/redirect.go index d94ede184a..cecf88c835 100644 --- a/internal/ingress/annotations/redirect/redirect.go +++ b/internal/ingress/annotations/redirect/redirect.go @@ -28,6 +28,8 @@ import ( "k8s.io/ingress-nginx/internal/ingress/resolver" ) +const defaultPermanentRedirectCode = http.StatusMovedPermanently + // Config returns the redirect configuration for an Ingress rule type Config struct { URL string `json:"url"` @@ -73,6 +75,15 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) { return nil, err } + prc, err := parser.GetIntAnnotation("permanent-redirect-code", ing) + if err != nil && !errors.IsMissingAnnotations(err) { + return nil, err + } + + if prc < http.StatusMultipleChoices || prc > http.StatusPermanentRedirect { + prc = defaultPermanentRedirectCode + } + if pr != "" { if err := isValidURL(pr); err != nil { return nil, err @@ -80,7 +91,7 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) { return &Config{ URL: pr, - Code: http.StatusMovedPermanently, + Code: prc, FromToWWW: r3w, }, nil } diff --git a/internal/ingress/annotations/redirect/redirect_test.go b/internal/ingress/annotations/redirect/redirect_test.go new file mode 100644 index 0000000000..8692a97fd0 --- /dev/null +++ b/internal/ingress/annotations/redirect/redirect_test.go @@ -0,0 +1,127 @@ +/* +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 redirect + +import ( + "net/http" + "strconv" + "testing" + + api "k8s.io/api/core/v1" + extensions "k8s.io/api/extensions/v1beta1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + "k8s.io/ingress-nginx/internal/ingress/annotations/parser" + "k8s.io/ingress-nginx/internal/ingress/defaults" + "k8s.io/ingress-nginx/internal/ingress/resolver" +) + +const ( + defRedirect = "http://some-site.com" + defCode = http.StatusMovedPermanently +) + +func buildIngress() *extensions.Ingress { + defaultBackend := extensions.IngressBackend{ + ServiceName: "default-backend", + ServicePort: intstr.FromInt(80), + } + + return &extensions.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + Spec: extensions.IngressSpec{ + Backend: &extensions.IngressBackend{ + ServiceName: "default-backend", + ServicePort: intstr.FromInt(80), + }, + Rules: []extensions.IngressRule{ + { + Host: "foo.bar.com", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{ + { + Path: "/foo", + Backend: defaultBackend, + }, + }, + }, + }, + }, + }, + }, + } +} + +type mockBackend struct { + resolver.Mock + redirect bool +} + +func (m mockBackend) GetDefaultBackend() defaults.Backend { + return defaults.Backend{SSLRedirect: m.redirect} +} +func TestPermanentRedirectWithDefaultCode(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirect + ing.SetAnnotations(data) + + i, err := NewParser(mockBackend{}).Parse(ing) + if err != nil { + t.Errorf("Unexpected error with ingress: %v", err) + } + redirect, ok := i.(*Config) + if !ok { + t.Errorf("expected a Redirect type") + } + if redirect.URL != defRedirect { + t.Errorf("Expected %v as redirect but returned %s", defRedirect, redirect.URL) + } + if redirect.Code != defCode { + t.Errorf("Expected %v as redirect to have a code %s but had %s", defRedirect, strconv.Itoa(defCode), strconv.Itoa(redirect.Code)) + } +} + +func TestPermanentRedirectWithCustomCode(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirect + data[parser.GetAnnotationWithPrefix("permanent-redirect-code")] = "308" + ing.SetAnnotations(data) + + i, err := NewParser(mockBackend{}).Parse(ing) + if err != nil { + t.Errorf("Unexpected error with ingress: %v", err) + } + redirect, ok := i.(*Config) + if !ok { + t.Errorf("expected a Redirect type") + } + if redirect.URL != defRedirect { + t.Errorf("Expected %v as redirect but returned %s", defRedirect, redirect.URL) + } + if redirect.Code != http.StatusPermanentRedirect { + t.Errorf("Expected %v as redirect to have a code %s but had %s", defRedirect, strconv.Itoa(http.StatusPermanentRedirect), strconv.Itoa(redirect.Code)) + } +} From 39966f48cdb78586c6b218c722a7823895745353 Mon Sep 17 00:00:00 2001 From: Antoine Cotten Date: Sat, 28 Jul 2018 14:27:19 +0200 Subject: [PATCH 2/2] Add e2e test for redirect annotations Minor refactoring of parser and unit tests --- .../ingress/annotations/redirect/redirect.go | 14 +- .../annotations/redirect/redirect_test.go | 126 +++++++----------- test/e2e/annotations/redirect.go | 121 +++++++++++++++++ 3 files changed, 173 insertions(+), 88 deletions(-) create mode 100644 test/e2e/annotations/redirect.go diff --git a/internal/ingress/annotations/redirect/redirect.go b/internal/ingress/annotations/redirect/redirect.go index cecf88c835..52b540974f 100644 --- a/internal/ingress/annotations/redirect/redirect.go +++ b/internal/ingress/annotations/redirect/redirect.go @@ -50,7 +50,7 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { // rule used to create a redirect in the paths defined in the rule. // If the Ingress contains both annotations the execution order is // temporal and then permanent -func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) { +func (r redirect) Parse(ing *extensions.Ingress) (interface{}, error) { r3w, _ := parser.GetBoolAnnotation("from-to-www-redirect", ing) tr, err := parser.GetStringAnnotation("temporal-redirect", ing) @@ -84,11 +84,7 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) { prc = defaultPermanentRedirectCode } - if pr != "" { - if err := isValidURL(pr); err != nil { - return nil, err - } - + if pr != "" || r3w { return &Config{ URL: pr, Code: prc, @@ -96,12 +92,6 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) { }, nil } - if r3w { - return &Config{ - FromToWWW: r3w, - }, nil - } - return nil, errors.ErrMissingAnnotations } diff --git a/internal/ingress/annotations/redirect/redirect_test.go b/internal/ingress/annotations/redirect/redirect_test.go index 8692a97fd0..6c60341aa0 100644 --- a/internal/ingress/annotations/redirect/redirect_test.go +++ b/internal/ingress/annotations/redirect/redirect_test.go @@ -1,5 +1,5 @@ /* -Copyright 2015 The Kubernetes Authors. +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. @@ -21,107 +21,81 @@ import ( "strconv" "testing" - api "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" - meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" - "k8s.io/ingress-nginx/internal/ingress/defaults" "k8s.io/ingress-nginx/internal/ingress/resolver" ) const ( - defRedirect = "http://some-site.com" - defCode = http.StatusMovedPermanently + defRedirectURL = "http://some-site.com" ) -func buildIngress() *extensions.Ingress { - defaultBackend := extensions.IngressBackend{ - ServiceName: "default-backend", - ServicePort: intstr.FromInt(80), +func TestPermanentRedirectWithDefaultCode(t *testing.T) { + rp := NewParser(resolver.Mock{}) + if rp == nil { + t.Fatalf("Expected a parser.IngressAnnotation but returned nil") } - return &extensions.Ingress{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - Spec: extensions.IngressSpec{ - Backend: &extensions.IngressBackend{ - ServiceName: "default-backend", - ServicePort: intstr.FromInt(80), - }, - Rules: []extensions.IngressRule{ - { - Host: "foo.bar.com", - IngressRuleValue: extensions.IngressRuleValue{ - HTTP: &extensions.HTTPIngressRuleValue{ - Paths: []extensions.HTTPIngressPath{ - { - Path: "/foo", - Backend: defaultBackend, - }, - }, - }, - }, - }, - }, - }, - } -} - -type mockBackend struct { - resolver.Mock - redirect bool -} + ing := new(extensions.Ingress) -func (m mockBackend) GetDefaultBackend() defaults.Backend { - return defaults.Backend{SSLRedirect: m.redirect} -} -func TestPermanentRedirectWithDefaultCode(t *testing.T) { - ing := buildIngress() - - data := map[string]string{} - data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirect + data := make(map[string]string, 1) + data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirectURL ing.SetAnnotations(data) - i, err := NewParser(mockBackend{}).Parse(ing) + i, err := rp.Parse(ing) if err != nil { t.Errorf("Unexpected error with ingress: %v", err) } redirect, ok := i.(*Config) if !ok { - t.Errorf("expected a Redirect type") + t.Errorf("Expected a Redirect type") } - if redirect.URL != defRedirect { - t.Errorf("Expected %v as redirect but returned %s", defRedirect, redirect.URL) + if redirect.URL != defRedirectURL { + t.Errorf("Expected %v as redirect but returned %s", defRedirectURL, redirect.URL) } - if redirect.Code != defCode { - t.Errorf("Expected %v as redirect to have a code %s but had %s", defRedirect, strconv.Itoa(defCode), strconv.Itoa(redirect.Code)) + if redirect.Code != defaultPermanentRedirectCode { + t.Errorf("Expected %v as redirect to have a code %d but had %d", defRedirectURL, defaultPermanentRedirectCode, redirect.Code) } } func TestPermanentRedirectWithCustomCode(t *testing.T) { - ing := buildIngress() - - data := map[string]string{} - data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirect - data[parser.GetAnnotationWithPrefix("permanent-redirect-code")] = "308" - ing.SetAnnotations(data) - - i, err := NewParser(mockBackend{}).Parse(ing) - if err != nil { - t.Errorf("Unexpected error with ingress: %v", err) + rp := NewParser(resolver.Mock{}) + if rp == nil { + t.Fatalf("Expected a parser.IngressAnnotation but returned nil") } - redirect, ok := i.(*Config) - if !ok { - t.Errorf("expected a Redirect type") - } - if redirect.URL != defRedirect { - t.Errorf("Expected %v as redirect but returned %s", defRedirect, redirect.URL) + + testCases := map[string]struct { + input int + expectOutput int + }{ + "valid code": {http.StatusPermanentRedirect, http.StatusPermanentRedirect}, + "invalid code": {http.StatusTeapot, defaultPermanentRedirectCode}, } - if redirect.Code != http.StatusPermanentRedirect { - t.Errorf("Expected %v as redirect to have a code %s but had %s", defRedirect, strconv.Itoa(http.StatusPermanentRedirect), strconv.Itoa(redirect.Code)) + + for n, tc := range testCases { + t.Run(n, func(t *testing.T) { + ing := new(extensions.Ingress) + + data := make(map[string]string, 2) + data[parser.GetAnnotationWithPrefix("permanent-redirect")] = defRedirectURL + data[parser.GetAnnotationWithPrefix("permanent-redirect-code")] = strconv.Itoa(tc.input) + ing.SetAnnotations(data) + + i, err := rp.Parse(ing) + if err != nil { + t.Errorf("Unexpected error with ingress: %v", err) + } + redirect, ok := i.(*Config) + if !ok { + t.Errorf("Expected a redirect Config type") + } + if redirect.URL != defRedirectURL { + t.Errorf("Expected %v as redirect but returned %s", defRedirectURL, redirect.URL) + } + if redirect.Code != tc.expectOutput { + t.Errorf("Expected %v as redirect to have a code %d but had %d", defRedirectURL, tc.expectOutput, redirect.Code) + } + }) } } diff --git a/test/e2e/annotations/redirect.go b/test/e2e/annotations/redirect.go new file mode 100644 index 0000000000..a2ce10d25e --- /dev/null +++ b/test/e2e/annotations/redirect.go @@ -0,0 +1,121 @@ +/* +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" + "net/http" + "strconv" + "strings" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/parnurzeal/gorequest" + + "k8s.io/ingress-nginx/test/e2e/framework" +) + +func noRedirectPolicyFunc(gorequest.Request, []gorequest.Request) error { + return http.ErrUseLastResponse +} + +var _ = framework.IngressNginxDescribe("Annotations - Redirect", func() { + f := framework.NewDefaultFramework("redirect") + + BeforeEach(func() { + }) + + AfterEach(func() { + }) + + It("should respond with a standard redirect code", func() { + By("setting permanent-redirect annotation") + + host := "redirect" + redirectPath := "/something" + redirectURL := "http://redirect.example.com" + + annotations := map[string]string{"nginx.ingress.kubernetes.io/permanent-redirect": redirectURL} + + ing := framework.NewSingleIngress(host, redirectPath, host, f.IngressController.Namespace, "http-svc", 80, &annotations) + _, err := f.EnsureIngress(ing) + + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("if ($uri ~* %s) {", redirectPath)) && + strings.Contains(server, fmt.Sprintf("return 301 %s;", redirectURL)) + }) + Expect(err).NotTo(HaveOccurred()) + + By("sending request to redirected URL path") + + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL+redirectPath). + Set("Host", host). + RedirectPolicy(noRedirectPolicyFunc). + End() + + Expect(errs).To(BeNil()) + Expect(resp.StatusCode).Should(BeNumerically("==", http.StatusMovedPermanently)) + Expect(resp.Header.Get("Location")).Should(Equal(redirectURL)) + Expect(body).Should(ContainSubstring("nginx/")) + }) + + It("should respond with a custom redirect code", func() { + By("setting permanent-redirect-code annotation") + + host := "redirect" + redirectPath := "/something" + redirectURL := "http://redirect.example.com" + redirectCode := http.StatusFound + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/permanent-redirect": redirectURL, + "nginx.ingress.kubernetes.io/permanent-redirect-code": strconv.Itoa(redirectCode), + } + + ing := framework.NewSingleIngress(host, redirectPath, host, f.IngressController.Namespace, "http-svc", 80, &annotations) + _, err := f.EnsureIngress(ing) + + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("if ($uri ~* %s) {", redirectPath)) && + strings.Contains(server, fmt.Sprintf("return %d %s;", redirectCode, redirectURL)) + }) + Expect(err).NotTo(HaveOccurred()) + + By("sending request to redirected URL path") + + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL+redirectPath). + Set("Host", host). + RedirectPolicy(noRedirectPolicyFunc). + End() + + Expect(errs).To(BeNil()) + Expect(resp.StatusCode).Should(BeNumerically("==", redirectCode)) + Expect(resp.Header.Get("Location")).Should(Equal(redirectURL)) + Expect(body).Should(ContainSubstring("nginx/")) + }) +})