From 698960e9b77eb7f5bc8100333800dfa580619fbb Mon Sep 17 00:00:00 2001 From: chriss-de <54666227+chriss-de@users.noreply.github.com> Date: Wed, 13 Nov 2024 22:02:48 +0100 Subject: [PATCH] Config/Annotations: Add `relative-redirects`. (#12161) --- .../nginx-configuration/annotations-risk.md | 1 + .../nginx-configuration/configmap.md | 12 ++ .../ingress/annotations/redirect/redirect.go | 24 ++++ .../annotations/redirect/redirect_test.go | 19 ++++ internal/ingress/controller/config/config.go | 6 + internal/ingress/defaults/main.go | 5 + rootfs/etc/nginx/template/nginx.tmpl | 8 ++ test/e2e/annotations/relativeredirects.go | 107 ++++++++++++++++++ 8 files changed, 182 insertions(+) create mode 100644 test/e2e/annotations/relativeredirects.go diff --git a/docs/user-guide/nginx-configuration/annotations-risk.md b/docs/user-guide/nginx-configuration/annotations-risk.md index 3e3b93986..be24c0930 100755 --- a/docs/user-guide/nginx-configuration/annotations-risk.md +++ b/docs/user-guide/nginx-configuration/annotations-risk.md @@ -103,6 +103,7 @@ | Redirect | from-to-www-redirect | Low | location | | Redirect | permanent-redirect | Medium | location | | Redirect | permanent-redirect-code | Low | location | +| Redirect | relative-redirects | Low | location | | Redirect | temporal-redirect | Medium | location | | Redirect | temporal-redirect-code | Low | location | | Rewrite | app-root | Medium | location | diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index aa877d5a8..9f093f6b2 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -223,6 +223,7 @@ The following table shows a configuration option's name, type, and the default v | [debug-connections](#debug-connections) | []string | "127.0.0.1,1.1.1.1/24" | | | [strict-validate-path-type](#strict-validate-path-type) | bool | "true" | | | [grpc-buffer-size-kb](#grpc-buffer-size-kb) | int | 0 | | +| [relative-redirects](#relative-redirects) | bool | false | | ## add-headers @@ -1382,3 +1383,14 @@ Sets the configuration for the GRPC Buffer Size parameter. If not set it will us _References:_ [https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_buffer_size](https://nginx.org/en/docs/http/ngx_http_grpc_module.html#grpc_buffer_size) + +## relative-redirects + +Use relative redirects instead of absolute redirects. Absolute redirects are the default in nginx. RFC7231 allows relative redirects since 2014. +Similar to the Ingress rule annotation `nginx.ingress.kubernetes.io/relative-redirects`. + +_**default:**_ "false" + +_References:_ +- [https://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect](https://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect) +- [https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2](https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2) diff --git a/internal/ingress/annotations/redirect/redirect.go b/internal/ingress/annotations/redirect/redirect.go index 0716e1ce1..edc3d279c 100644 --- a/internal/ingress/annotations/redirect/redirect.go +++ b/internal/ingress/annotations/redirect/redirect.go @@ -38,6 +38,7 @@ type Config struct { URL string `json:"url"` Code int `json:"code"` FromToWWW bool `json:"fromToWWW"` + Relative bool `json:"relative"` } const ( @@ -46,6 +47,7 @@ const ( temporalRedirectAnnotationCode = "temporal-redirect-code" permanentRedirectAnnotation = "permanent-redirect" permanentRedirectAnnotationCode = "permanent-redirect-code" + relativeRedirectsAnnotation = "relative-redirects" ) var redirectAnnotations = parser.Annotation{ @@ -83,6 +85,12 @@ var redirectAnnotations = parser.Annotation{ Risk: parser.AnnotationRiskLow, // Low, as it allows just a set of options Documentation: `This annotation allows you to modify the status code used for permanent redirects.`, }, + relativeRedirectsAnnotation: { + Validator: parser.ValidateBool, + Scope: parser.AnnotationScopeLocation, + Risk: parser.AnnotationRiskLow, + Documentation: `If enabled, redirects issued by nginx will be relative. See https://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect`, + }, }, } @@ -109,6 +117,11 @@ func (r redirect) Parse(ing *networking.Ingress) (interface{}, error) { return nil, err } + rr, err := parser.GetBoolAnnotation(relativeRedirectsAnnotation, ing, r.annotationConfig.Annotations) + if err != nil && !errors.IsMissingAnnotations(err) { + return nil, err + } + tr, err := parser.GetStringAnnotation(temporalRedirectAnnotation, ing, r.annotationConfig.Annotations) if err != nil && !errors.IsMissingAnnotations(err) { return nil, err @@ -132,6 +145,7 @@ func (r redirect) Parse(ing *networking.Ingress) (interface{}, error) { URL: tr, Code: trc, FromToWWW: r3w, + Relative: rr, }, nil } @@ -154,6 +168,13 @@ func (r redirect) Parse(ing *networking.Ingress) (interface{}, error) { URL: pr, Code: prc, FromToWWW: r3w, + Relative: rr, + }, nil + } + + if rr { + return &Config{ + Relative: rr, }, nil } @@ -177,6 +198,9 @@ func (r1 *Config) Equal(r2 *Config) bool { if r1.FromToWWW != r2.FromToWWW { return false } + if r1.Relative != r2.Relative { + return false + } return true } diff --git a/internal/ingress/annotations/redirect/redirect_test.go b/internal/ingress/annotations/redirect/redirect_test.go index b5c34879e..f4734ae5b 100644 --- a/internal/ingress/annotations/redirect/redirect_test.go +++ b/internal/ingress/annotations/redirect/redirect_test.go @@ -193,3 +193,22 @@ func TestIsValidURL(t *testing.T) { t.Errorf("expected nil but got %v", err) } } + +func TestParseAnnotations(t *testing.T) { + ing := new(networking.Ingress) + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix(relativeRedirectsAnnotation)] = "true" + ing.SetAnnotations(data) + + _, err := NewParser(&resolver.Mock{}).Parse(ing) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + // test ingress using the annotation without a TLS section + _, err = NewParser(&resolver.Mock{}).Parse(ing) + if err != nil { + t.Errorf("unexpected error parsing ingress with relative-redirects") + } +} diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index a0275697f..f4d202f00 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -549,6 +549,10 @@ type Configuration struct { // https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_intercept_errors DisableProxyInterceptErrors bool `json:"disable-proxy-intercept-errors,omitempty"` + // Disable absolute redirects and enables relative redirects. + // https://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect + RelativeRedirects bool `json:"relative-redirects"` + // Sets the ipv4 addresses on which the server will accept requests. BindAddressIpv4 []string `json:"bind-address-ipv4,omitempty"` @@ -834,6 +838,7 @@ func NewDefault() Configuration { VariablesHashMaxSize: 2048, UseHTTP2: true, DisableProxyInterceptErrors: false, + RelativeRedirects: false, ProxyStreamTimeout: "600s", ProxyStreamNextUpstream: true, ProxyStreamNextUpstreamTimeout: "600s", @@ -857,6 +862,7 @@ func NewDefault() Configuration { SSLRedirect: true, CustomHTTPErrors: []int{}, DisableProxyInterceptErrors: false, + RelativeRedirects: false, DenylistSourceRange: []string{}, WhitelistSourceRange: []string{}, SkipAccessLogURLs: []string{}, diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go index cfad388ef..af0a41d66 100644 --- a/internal/ingress/defaults/main.go +++ b/internal/ingress/defaults/main.go @@ -125,6 +125,11 @@ type Backend struct { // Default: false UsePortInRedirects bool `json:"use-port-in-redirects"` + // Enables or disables relative redirects. By default nginx uses absolute redirects. + // http://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect + // Default: false + RelativeRedirects bool `json:"relative-redirects"` + // Enable stickiness by client-server mapping based on a NGINX variable, text or a combination of both. // A consistent hashing method will be used which ensures only a few keys would be remapped to different // servers on upstream group changes diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index e40cef244..ad41ec7ee 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -459,6 +459,10 @@ http { proxy_intercept_errors on; {{ end }} + {{ if $cfg.RelativeRedirects }} + absolute_redirect off; + {{ end }} + {{ range $errCode := $cfg.CustomHTTPErrors }} error_page {{ $errCode }} = @custom_upstream-default-backend_{{ $errCode }};{{ end }} @@ -1343,6 +1347,10 @@ stream { satisfy {{ $location.Satisfy }}; {{ end }} + {{ if $location.Redirect.Relative }} + absolute_redirect off; + {{ end }} + {{/* if a location-specific error override is set, add the proxy_intercept here */}} {{ if and $location.CustomHTTPErrors (not $location.DisableProxyInterceptErrors) }} # Custom error pages per ingress diff --git a/test/e2e/annotations/relativeredirects.go b/test/e2e/annotations/relativeredirects.go new file mode 100644 index 000000000..430b357e4 --- /dev/null +++ b/test/e2e/annotations/relativeredirects.go @@ -0,0 +1,107 @@ +/* +Copyright 2023 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" + "strings" + + "github.com/onsi/ginkgo/v2" + "github.com/stretchr/testify/assert" + "k8s.io/ingress-nginx/test/e2e/framework" +) + +const ( + relativeRedirectsHostname = "rr.foo.com" + relativeRedirectsRedirectPath = "/something" + relativeRedirectsRelativeRedirectURL = "/new-location" +) + +var _ = framework.DescribeAnnotation("relative-redirects", func() { + f := framework.NewDefaultFramework("relative-redirects") + + ginkgo.BeforeEach(func() { + f.NewHttpbunDeployment() + f.NewEchoDeployment() + }) + + ginkgo.It("configures Nginx correctly", func() { + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/relative-redirects": "true", + } + + ing := framework.NewSingleIngress(relativeRedirectsHostname, "/", relativeRedirectsHostname, f.Namespace, framework.HTTPBunService, 80, annotations) + f.EnsureIngress(ing) + + var serverConfig string + f.WaitForNginxServer(relativeRedirectsHostname, func(srvCfg string) bool { + serverConfig = srvCfg + return strings.Contains(serverConfig, fmt.Sprintf("server_name %s", relativeRedirectsHostname)) + }) + + ginkgo.By("turning off absolute_redirect directive") + assert.Contains(ginkgo.GinkgoT(), serverConfig, "absolute_redirect off;") + }) + + ginkgo.It("should respond with absolute URL in Location", func() { + absoluteRedirectURL := fmt.Sprintf("http://%s%s", relativeRedirectsHostname, relativeRedirectsRelativeRedirectURL) + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/permanent-redirect": relativeRedirectsRelativeRedirectURL, + "nginx.ingress.kubernetes.io/relative-redirects": "false", + } + + ginkgo.By("setup ingress") + ing := framework.NewSingleIngress(relativeRedirectsHostname, relativeRedirectsRedirectPath, relativeRedirectsHostname, f.Namespace, framework.EchoService, 80, annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(relativeRedirectsHostname, func(srvCfg string) bool { + return strings.Contains(srvCfg, fmt.Sprintf("server_name %s", relativeRedirectsHostname)) + }) + + ginkgo.By("sending request to redirected URL path") + f.HTTPTestClient(). + GET(relativeRedirectsRedirectPath). + WithHeader("Host", relativeRedirectsHostname). + Expect(). + Status(http.StatusMovedPermanently). + Header("Location").Equal(absoluteRedirectURL) + }) + + ginkgo.It("should respond with relative URL in Location", func() { + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/permanent-redirect": relativeRedirectsRelativeRedirectURL, + "nginx.ingress.kubernetes.io/relative-redirects": "true", + } + + ginkgo.By("setup ingress") + ing := framework.NewSingleIngress(relativeRedirectsHostname, relativeRedirectsRedirectPath, relativeRedirectsHostname, f.Namespace, framework.EchoService, 80, annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(relativeRedirectsHostname, func(srvCfg string) bool { + return strings.Contains(srvCfg, fmt.Sprintf("server_name %s", relativeRedirectsHostname)) + }) + + ginkgo.By("sending request to redirected URL path") + f.HTTPTestClient(). + GET(relativeRedirectsRedirectPath). + WithHeader("Host", relativeRedirectsHostname). + Expect(). + Status(http.StatusMovedPermanently). + Header("Location").Equal(relativeRedirectsRelativeRedirectURL) + }) +})