diff --git a/docs/examples/rewrite/README.md b/docs/examples/rewrite/README.md index 6fecb0496..3ad2593bb 100644 --- a/docs/examples/rewrite/README.md +++ b/docs/examples/rewrite/README.md @@ -20,6 +20,7 @@ Rewriting can be controlled using the following annotations: |nginx.ingress.kubernetes.io/ssl-redirect|Indicates if the location section is accessible SSL only (defaults to True when Ingress contains a Certificate)|bool| |nginx.ingress.kubernetes.io/force-ssl-redirect|Forces the redirection to HTTPS even if the Ingress is not TLS Enabled|bool| |nginx.ingress.kubernetes.io/app-root|Defines the Application Root that the Controller must redirect if it's in '/' context|string| +|nginx.ingress.kubernetes.io/use-regex|Indicates if the paths defined on an Ingress use regular expressions|bool| ## Validation diff --git a/docs/user-guide/ingress-path-matching.md b/docs/user-guide/ingress-path-matching.md new file mode 100644 index 000000000..7d326682d --- /dev/null +++ b/docs/user-guide/ingress-path-matching.md @@ -0,0 +1,155 @@ +# Ingress Path Matching + +## Regular Expression Support + +The ingress controller supports **case insensitive** regular expressions in the `spec.rules.http.paths.path` field. __Currently curly braces `{}` cannot be used in regular expressions due to a [known issue](https://github.com/kubernetes/ingress-nginx/issues/3155).__ + + +See the [description](./nginx-configuration/annotations.md#use-regex) of the `use-regex` annotation for more details. + +``` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress + annotations: + nginx.ingress.kubernetes.io/use-regex: true +spec: + host: test.com + rules: + - http: + paths: + - path: /foo/.* + backend: + serviceName: test + servicePort: 80 +``` + +The preceding ingress definition would translate to the following location block within the NGINX configuration for the `test.com` server: + +``` +location ~* ^/foo/.* { + ... +} +``` + +## Path Priority + +In NGINX, regular expressions follow a **first match** policy. In order to enable more acurate path matching, ingress-nginx first orders the paths by descending length before writing them to the NGINX template as location blocks. + +__Please read the [warning](#warning) before using regular expressions in your ingress definitions.__ + +### Example + +Let the following two ingress definitions be created: + +``` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress-1 +spec: + host: test.com + rules: + - http: + paths: + - path: /foo/bar + backend: + serviceName: test + servicePort: 80 + - path: /foo/bar/ + backend: + serviceName: test + servicePort: 80 +``` + +``` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress-2 + annotations: + nginx.ingress.kubernetes.io/rewrite-target: / +spec: + host: test.com + rules: + - http: + paths: + - path: /foo/bar/.+ + backend: + serviceName: test + servicePort: 80 +``` + + + +The ingress controller would define the following location blocks, in order of descending length, within the NGINX template for the `test.com` server: + +``` +location ~* ^/foo/bar/.+\/?(?.*) { + ... +} + +location ~* ^/foo/bar/ { + ... +} + +location ~* ^/foo/bar { + ... +} +``` +The following request URI's would match the corresponding location blocks: +- `test.com/foo/bar/1` matches `~* ^/foo/bar/.+\/?(?.*)` +- `test.com/foo/bar/` matches `~* ^/foo/bar/` +- `test.com/foo/bar` matches `~* ^/foo/bar` + +__IMPORTANT NOTES__: +- paths created under the `rewrite-ingress` are sorted before `\/?(?.*)` is appended. For example if the path defined within `test-ingress-2` was `/foo/.+` then the location block for `^/foo/.+\/?(?.*)` would be the LAST block listed. +- If the `use-regex` OR `rewrite-target` annotation is used on any Ingress for a given host, then the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. + + +## Warning +The following example describes a case that may inflict unwanted path matching behaviour. + +This case is expected and a result of NGINX's a first match policy for paths that use the regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location). For more information about how a path is chosen, please read the following article: ["Understanding Nginx Server and Location Block Selection Algorithms"](https://www.digitalocean.com/community/tutorials/understanding-nginx-server-and-location-block-selection-algorithms). + +### Example + +Let the following ingress be defined: + +``` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress-1 + annotations: + nginx.ingress.kubernetes.io/use-regex: true +spec: + host: test.com + rules: + - http: + paths: + - path: /foo/bar/bar + backend: + serviceName: test + servicePort: 80 + - path: /foo/bar/[A-Z0-9]{3} + backend: + serviceName: test + servicePort: 80 +``` + +The ingress controller would define the following location blocks (in this order) within the NGINX template for the `test.com` server: + +``` +location ~* ^/foo/bar/[A-Z0-9]{3} { + ... +} + +location ~* ^/foo/bar/bar { + ... +} +``` + +A request to `test.com/foo/bar/bar` would match the `^/foo/[A-Z0-9]{3}` location block instead of the longest EXACT matching path. + diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 9c570a8f8..5476554fa 100644 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -86,6 +86,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/influxdb-port](#influxdb)|string| |[nginx.ingress.kubernetes.io/influxdb-host](#influxdb)|string| |[nginx.ingress.kubernetes.io/influxdb-server-name](#influxdb)|string| +|[nginx.ingress.kubernetes.io/use-regex](#use-regex)|bool| ### Rewrite @@ -630,3 +631,25 @@ Example: ```yaml nginx.ingress.kubernetes.io/backend-protocol: "HTTPS" ``` + +### Use Regex + +Using the `nginx.ingress.kubernetes.io/use-regex` annotation will indicate whether or not the paths defined on an Ingress use regular expressions. The default value is `false`. + +The following will indicate that regular expression paths are being used: +```yaml +nginx.ingress.kubernetes.io/use-regex: "true" +``` + +The following will indicate that regular expression paths are __not__ being used: +```yaml +nginx.ingress.kubernetes.io/use-regex: "false" +``` + +When this annotation is set to `true`, the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. + +Additionally, if the [`rewrite-target` annotation](#rewrite) is used on any Ingress for a given host, then the case insensitive regular expression [location modifier](https://nginx.org/en/docs/http/ngx_http_core_module.html#location) will be enforced on ALL paths for a given host regardless of what Ingress they are defined on. + +Please read about [ingress path matching](../ingress-path-matching.md) before using this modifier. + + diff --git a/internal/ingress/annotations/rewrite/main.go b/internal/ingress/annotations/rewrite/main.go index a1624549e..270bdb1a7 100644 --- a/internal/ingress/annotations/rewrite/main.go +++ b/internal/ingress/annotations/rewrite/main.go @@ -38,6 +38,8 @@ type Config struct { ForceSSLRedirect bool `json:"forceSSLRedirect"` // AppRoot defines the Application Root that the Controller must redirect if it's in '/' context AppRoot string `json:"appRoot"` + // UseRegex indicates whether or not the locations use regex paths + UseRegex bool `json:useRegex` } // Equal tests for equality between two Redirect types @@ -66,6 +68,9 @@ func (r1 *Config) Equal(r2 *Config) bool { if r1.AppRoot != r2.AppRoot { return false } + if r1.UseRegex != r2.UseRegex { + return false + } return true } @@ -94,6 +99,7 @@ func (a rewrite) Parse(ing *extensions.Ingress) (interface{}, error) { abu, _ := parser.GetBoolAnnotation("add-base-url", ing) bus, _ := parser.GetStringAnnotation("base-url-scheme", ing) ar, _ := parser.GetStringAnnotation("app-root", ing) + ur, _ := parser.GetBoolAnnotation("use-regex", ing) return &Config{ Target: rt, @@ -102,5 +108,6 @@ func (a rewrite) Parse(ing *extensions.Ingress) (interface{}, error) { SSLRedirect: sslRe, ForceSSLRedirect: fSslRe, AppRoot: ar, + UseRegex: ur, }, nil } diff --git a/internal/ingress/annotations/rewrite/main_test.go b/internal/ingress/annotations/rewrite/main_test.go index b54e3bfdc..71721ad93 100644 --- a/internal/ingress/annotations/rewrite/main_test.go +++ b/internal/ingress/annotations/rewrite/main_test.go @@ -178,3 +178,20 @@ func TestAppRoot(t *testing.T) { t.Errorf("Unexpected value got in AppRoot") } } + +func TestUseRegex(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("use-regex")] = "true" + ing.SetAnnotations(data) + + i, _ := NewParser(mockBackend{redirect: true}).Parse(ing) + redirect, ok := i.(*Config) + if !ok { + t.Errorf("expected a App Context") + } + if redirect.UseRegex != true { + t.Errorf("Unexpected value got in UseRegex") + } +} diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 15467c227..02d8b23a3 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -620,6 +620,10 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([] sort.SliceStable(value.Locations, func(i, j int) bool { return value.Locations[i].Path > value.Locations[j].Path }) + + sort.SliceStable(value.Locations, func(i, j int) bool { + return len(value.Locations[i].Path) > len(value.Locations[j].Path) + }) aServers = append(aServers, value) } diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index a0b96cc14..8bee8589f 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -156,7 +156,8 @@ var ( "buildOpentracing": buildOpentracing, "proxySetHeader": proxySetHeader, "buildInfluxDB": buildInfluxDB, - "atLeastOneNeedsRewrite": atLeastOneNeedsRewrite, + "enforceRegexModifier": enforceRegexModifier, + "stripLocationModifer": stripLocationModifer, } ) @@ -296,8 +297,13 @@ func needsRewrite(location *ingress.Location) bool { return false } -// atLeastOneNeedsRewrite checks if the nginx.ingress.kubernetes.io/rewrite-target annotation is used on the '/' path -func atLeastOneNeedsRewrite(input interface{}) bool { +func stripLocationModifer(path string) string { + return strings.TrimLeft(path, "~* ") +} + +// enforceRegexModifier checks if the "rewrite-target" or "use-regex" annotation +// is used on any location path within a server +func enforceRegexModifier(input interface{}) bool { locations, ok := input.([]*ingress.Location) if !ok { glog.Errorf("expected an '[]*ingress.Location' type but %T was returned", input) @@ -305,7 +311,7 @@ func atLeastOneNeedsRewrite(input interface{}) bool { } for _, location := range locations { - if needsRewrite(location) { + if needsRewrite(location) || location.Rewrite.UseRegex { return true } } @@ -314,7 +320,9 @@ func atLeastOneNeedsRewrite(input interface{}) bool { // buildLocation produces the location string, if the ingress has redirects // (specified through the nginx.ingress.kubernetes.io/rewrite-target annotation) -func buildLocation(input interface{}, rewrite bool) string { +// TODO: return quotes around returned location path to prevent regex from breaking under certain conditions, see: +// https://github.com/kubernetes/ingress-nginx/issues/3155 +func buildLocation(input interface{}, enforceRegex bool) string { location, ok := input.(*ingress.Location) if !ok { glog.Errorf("expected an '*ingress.Location' type but %T was returned", input) @@ -324,7 +332,7 @@ func buildLocation(input interface{}, rewrite bool) string { path := location.Path if needsRewrite(location) { if path == slash { - return fmt.Sprintf("~* %s", path) + return fmt.Sprintf("~* ^%s", path) } // baseuri regex will parse basename from the given location baseuri := `(?.*)` @@ -335,11 +343,8 @@ func buildLocation(input interface{}, rewrite bool) string { return fmt.Sprintf(`~* ^%s%s`, path, baseuri) } - if rewrite { - if path == slash { - return path - } - return fmt.Sprintf(`^~ %s`, path) + if enforceRegex { + return fmt.Sprintf(`~* ^%s`, path) } return path } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index c65ad0ee9..534964f84 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -50,7 +50,7 @@ var ( XForwardedPrefix bool DynamicConfigurationEnabled bool SecureBackend bool - atLeastOneNeedsRewrite bool + enforceRegex bool }{ "when secure backend enabled": { "/", @@ -127,7 +127,7 @@ var ( "redirect / to /jenkins": { "/", "/jenkins", - "~* /", + "~* ^/", ` rewrite (?i)/(.*) /jenkins/$1 break; rewrite (?i)/$ /jenkins/ break; @@ -191,7 +191,7 @@ proxy_pass http://upstream-name; "redirect / to /jenkins and rewrite": { "/", "/jenkins", - "~* /", + "~* ^/", ` rewrite (?i)/(.*) /jenkins/$1 break; rewrite (?i)/$ /jenkins/ break; @@ -286,7 +286,7 @@ subs_filter '(<(?:H|h)(?:E|e)(?:A|a)(?:D|d)(?:[^">]|"[^"]*")*>)' '$1.*) {`) + }) + Expect(err).NotTo(HaveOccurred()) + + By("ensuring '/foo' matches '~* ^/foo'") + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL+"/foo"). + Set("Host", host). + End() + expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/foo", host) + Expect(len(errs)).Should(Equal(0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + Expect(body).Should(ContainSubstring(expectBodyRequestURI)) + + By("ensuring '/foo/bar' matches '~* ^/foo.+'") + resp, body, errs = gorequest.New(). + Get(f.IngressController.HTTPURL+"/foo/bar"). + Set("Host", host). + End() + expectBodyRequestURI = fmt.Sprintf("request_uri=http://%v:8080/new/backend", host) + Expect(len(errs)).Should(Equal(0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + Expect(body).Should(ContainSubstring(expectBodyRequestURI)) + }) + + It("should fail to use longest match for documented warning", func() { + host := "rewrite.bar.com" + + By("creating a regular ingress definition") + ing := framework.NewSingleIngress("foo", "/foo/bar/bar", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{}) + _, err := f.EnsureIngress(ing) + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + By(`creating an ingress definition with the use-regex annotation`) + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/use-regex": "true", + "nginx.ingress.kubernetes.io/rewrite-target": "/new/backend", + } + ing = framework.NewSingleIngress("regex", "/foo/bar/[a-z][a-z][a-z]", 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, "location ~* ^/foo/bar/bar {") && strings.Contains(server, `location ~* ^/foo/bar/[a-z][a-z][a-z]\/?(?.*) {`) + }) + Expect(err).NotTo(HaveOccurred()) + + By("check that '/foo/bar/bar' does not match the longest exact path") + resp, body, errs := gorequest.New(). + Get(f.IngressController.HTTPURL+"/foo/bar/bar"). + Set("Host", host). + End() + expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/new/backend", host) Expect(len(errs)).Should(Equal(0)) Expect(resp.StatusCode).Should(Equal(http.StatusOK)) Expect(body).Should(ContainSubstring(expectBodyRequestURI))