DevFW-CICD/ingress-nginx-helm
14
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #3145 from Shopify/regex-modifier

Browse source 
Add "use-regex" Annotation to Toggle Regular Expression Location Modifier
This commit is contained in:
k8s-ci-robot 2018-10-01 11:31:43 -07:00 committed by GitHub
commit d9f58144eb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 325 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
docs/examples/rewrite/README.md
View file

@ -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

155
docs/user-guide/ingress-path-matching.md Normal file
View file

@ -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/.+\/?(?<baseuri>.*) {
...
}
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/.+\/?(?<baseuri>.*)`
- `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 `\/?(?<baseuri>.*)` is appended. For example if the path defined within `test-ingress-2` was `/foo/.+` then the location block for `^/foo/.+\/?(?<baseuri>.*)` 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.

23
docs/user-guide/nginx-configuration/annotations.md
View file

@ -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.

7
internal/ingress/annotations/rewrite/main.go
View file

@ -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
}

17
internal/ingress/annotations/rewrite/main_test.go
View file

@ -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")
}
}

4
internal/ingress/controller/controller.go
View file

@ -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)
}

27
internal/ingress/controller/template/template.go
View file

@ -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 := `(?<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
}

28
internal/ingress/controller/template/template_test.go
View file

@ -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<base href="
"redirect / to /something with sticky enabled": {
"/",
"/something",
`~* /`,
`~* ^/`,
`
rewrite (?i)/(.*) /something/$1 break;
rewrite (?i)/$ /something/ break;
@ -302,7 +302,7 @@ proxy_pass http://sticky-upstream-name;
"redirect / to /something with sticky and dynamic config enabled": {
"/",
"/something",
`~* /`,
`~* ^/`,
`
rewrite (?i)/(.*) /something/$1 break;
rewrite (?i)/$ /something/ break;
@ -332,22 +332,10 @@ proxy_pass http://sticky-upstream-name;
false,
false,
true},
"do not use ^~ location modifier on index when ingress does not use rewrite target but at least one other ingress does": {
"/",
"/",
"/",
"proxy_pass http://upstream-name;",
false,
"",
false,
false,
false,
false,
true},
"use ^~ location modifier when ingress does not use rewrite target but at least one other ingress does": {
"use ~* location modifier when ingress does not use rewrite/regex target but at least one other ingress does": {
"/something",
"/something",
"^~ /something",
"~* ^/something",
"proxy_pass http://upstream-name;",
false,
"",
@ -425,7 +413,7 @@ func TestBuildLocation(t *testing.T) {
Rewrite: rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL},
}
newLoc := buildLocation(loc, tc.atLeastOneNeedsRewrite)
newLoc := buildLocation(loc, tc.enforceRegex)
if tc.Location != newLoc {
t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
}

10
rootfs/etc/nginx/template/nginx.tmpl
View file

@ -451,9 +451,9 @@ http {
{{/* build the maps that will be use to validate the Whitelist */}}
{{ range $server := $servers }}
{{ $usesRewrite := atLeastOneNeedsRewrite $server.Locations }}
{{ $enforceRegex := enforceRegexModifier $server.Locations }}
{{ range $location := $server.Locations }}
{{ $path := buildLocation $location $usesRewrite }}
{{ $path := buildLocation $location $enforceRegex }}
{{ if isLocationAllowed $location }}
{{ if gt (len $location.Whitelist.CIDR) 0 }}
@ -891,9 +891,9 @@ stream {
{{ $server.ServerSnippet }}
{{ end }}
{{ $usesRewrite := atLeastOneNeedsRewrite $server.Locations }}
{{ $enforceRegex := enforceRegexModifier $server.Locations }}
{{ range $location := $server.Locations }}
{{ $path := buildLocation $location $usesRewrite }}
{{ $path := buildLocation $location $enforceRegex }}
{{ $proxySetHeader := proxySetHeader $location }}
{{ $authPath := buildAuthLocation $location }}
@ -1131,7 +1131,7 @@ stream {
{{ buildInfluxDB $location.InfluxDB }}
{{ if not (empty $location.Redirect.URL) }}
if ($uri ~* {{ $path }}) {
if ($uri ~* {{ stripLocationModifer $path }}) {
return {{ $location.Redirect.Code }} {{ $location.Redirect.URL }};
}
{{ end }}

92
test/e2e/annotations/rewrite.go
View file

@ -120,7 +120,6 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
It("should use correct longest path match", func() {
host := "rewrite.bar.com"
expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/.well-known/acme/challenge", host)
By("creating a regular ingress definition")
ing := framework.NewSingleIngress("kube-lego", "/.well-known/acme/challenge", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{})
@ -139,7 +138,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
Get(f.IngressController.HTTPURL+"/.well-known/acme/challenge").
Set("Host", host).
End()
expectBodyRequestURI := fmt.Sprintf("request_uri=http://%v:8080/.well-known/acme/challenge", host)
Expect(len(errs)).Should(Equal(0))
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(body).Should(ContainSubstring(expectBodyRequestURI))
@ -155,7 +154,7 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
err = f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, "location ~* / {")
return strings.Contains(server, "location ~* ^/ {") && strings.Contains(server, "location ~* ^/.well-known/acme/challenge {")
})
Expect(err).NotTo(HaveOccurred())
@ -164,7 +163,94 @@ var _ = framework.IngressNginxDescribe("Annotations - Rewrite", func() {
Get(f.IngressController.HTTPURL+"/.well-known/acme/challenge").
Set("Host", host).
End()
Expect(len(errs)).Should(Equal(0))
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(body).Should(ContainSubstring(expectBodyRequestURI))
})
It("should use ~* location modifier if regex annotation is present", func() {
host := "rewrite.bar.com"
By("creating a regular ingress definition")
ing := framework.NewSingleIngress("foo", "/foo", host, f.IngressController.Namespace, "http-svc", 80, &map[string]string{})
_, 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 {")
})
Expect(err).NotTo(HaveOccurred())
By(`creating an ingress definition with the use-regex amd rewrite-target 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.+", 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 {") && strings.Contains(server, `location ~* ^/foo.+\/?(?<baseuri>.*) {`)
})
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]\/?(?<baseuri>.*) {`)
})
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))