Add location-modifier annotation to allow simple reg-ex locations #1415

abetterway2feel · 2017-09-25T16:07:07Z

Pass location modifier in annotation

metadata:
  ...
  annotations:
   ...
    ingress.kubernetes.io/location-modifier: "~"

Ingress paths can now have regex:

rules:
  - host: ...
    http:
      paths:
      - path: "/users/.*/documents"
        backend:
          serviceName: myservice1
          servicePort: 8080
  - host: ...
    http:
      paths:
      - path: "/users/.*/records"
        backend:
          serviceName: myservice2
          servicePort: 8080

In nginx a modifier can be passed which decides how locations will be parsed, i.e. regex or no regex.
This change allows a location modifier to be passed as an annotation.
If no location-modifier is passed but 'rewrite-target' is provided, then a default of "*~" is assumed.

k8s-ci-robot · 2017-09-25T16:07:09Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-reviewable · 2017-09-25T16:07:13Z

This change is

abetterway2feel · 2017-09-25T16:19:27Z

controllers/nginx/pkg/template/template_test.go

 		}

 		newLoc := buildLocation(loc)
 		if tc.Location != newLoc {
-			t.Errorf("%s: expected '%v' but returned %v", k, tc.Location, newLoc)
+			t.Errorf("%s: expected '%v' but returned '%v'", k, tc.Location, newLoc)


quotes around the second argument were missing.

abetterway2feel · 2017-09-25T16:23:55Z

core/pkg/ingress/annotations/rewrite/main.go

-	appRoot          = "ingress.kubernetes.io/app-root"
+	rewriteTo                      = "ingress.kubernetes.io/rewrite-target"
+	locationModifier               = "ingress.kubernetes.io/location-modifier"
+	defaultRewriteLocationModifier = "~*"


is 'location-modifier' too nginx specific to be in core?

abetterway2feel · 2017-09-25T16:32:19Z

CLA has been signed....

coveralls · 2017-09-25T16:57:19Z

Coverage increased (+0.2%) to 43.719% when pulling e406dce22905a9871f84f243e45f908bebe9b023 on gesundheitscloud:pq/location_modifier_annotation into e43641f on kubernetes:master.

coveralls · 2017-09-25T17:09:53Z

Coverage increased (+0.1%) to 43.707% when pulling e406dce22905a9871f84f243e45f908bebe9b023 on gesundheitscloud:pq/location_modifier_annotation into e43641f on kubernetes:master.

coveralls · 2017-09-25T17:12:10Z

Coverage increased (+0.1%) to 43.707% when pulling e406dce22905a9871f84f243e45f908bebe9b023 on gesundheitscloud:pq/location_modifier_annotation into e43641f on kubernetes:master.

coveralls · 2017-09-27T12:07:50Z

Coverage increased (+0.2%) to 43.886% when pulling d282ddda0a77dc74ebcca3fe8f643d461b2f780b on gesundheitscloud:pq/location_modifier_annotation into 6e24dc6 on kubernetes:master.

coveralls · 2017-09-27T12:10:20Z

Coverage increased (+0.1%) to 43.874% when pulling d282ddda0a77dc74ebcca3fe8f643d461b2f780b on gesundheitscloud:pq/location_modifier_annotation into 6e24dc6 on kubernetes:master.

coveralls · 2017-09-27T15:33:14Z

Coverage increased (+0.2%) to 43.886% when pulling 7ff049f on gesundheitscloud:pq/location_modifier_annotation into 6e24dc6 on kubernetes:master.

paalkr · 2017-09-29T07:04:04Z

Thank you!

This pull request will probably partially fix #1260 as well.
The trailing slash issue seems still unresolved though #1399

paalkr · 2017-09-29T07:17:45Z

Is there a docker image containing this modification available somewhere?

theintz · 2017-09-29T09:16:25Z

@aledbf @nicksardo could you guys take a quick look at this please? it's a minor change that fixes a major issue. Thanks.

paalkr · 2017-09-29T14:57:29Z

Actually, I think the patch will completely fix (not partially as I earlier stated) the issues described in #1260

abetterway2feel · 2017-10-05T11:16:47Z

@paalkr I'm afraid the docker image I'm using is in a private repo... should be easily build from the branch though

abetterway2feel · 2017-10-11T09:48:22Z

rebased...

coveralls · 2017-10-11T10:02:12Z

Coverage increased (+0.2%) to 33.707% when pulling a609b86607a7ba99e429fdf7c934da2e6563fa65 on gesundheitscloud:pq/location_modifier_annotation into a18daab on kubernetes:master.

aledbf · 2018-04-06T17:30:16Z

@pussinboots thank you. I will review this weekend and merge (if I don't find anything else during testing)

In nginx a modifier can be passed which decides how locations will be parsed, i.e. regex or no regex. This change allows a location modifier to be passed as an annotatione e.g. ingress.kubernetes.io/location-modifier: "~" If no location-modifier is passed but 'rewrite-target' is provided, then a default of "*~" is assumed.

aledbf · 2018-04-08T14:16:26Z

@pussinboots please check this example

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: http-svc-1
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "Ingress: $ingress_name";
spec:
  rules:
  - host: foo.bar
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /users/otherpath/documents/foo

---

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: http-svc-2
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "Ingress: $ingress_name";
spec:
  rules:
  - host: foo.bar
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /users/demo/documents

---

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: http-svc-3
  annotations:
    nginx.ingress.kubernetes.io/location-modifier: '~'
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "Ingress: $ingress_name";
spec:
  rules:
  - host: foo.bar
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /users/(.*)/documents

---

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: http-svc-4
  annotations:
    nginx.ingress.kubernetes.io/location-modifier: '~'
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "Ingress: $ingress_name";
spec:
  rules:
  - host: foo.bar
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /users/(.*)/records

---

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: http-svc-5
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: |
      more_set_headers "Ingress: $ingress_name";
spec:
  rules:
  - host: foo.bar
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /users/sample/records

---

Executing the next example should return http-svc-5 in the Ingress header but returns http-svc-4

$ http $(minikube ip):31321/users/sample/records Host:foo.bar
HTTP/1.1 200 OK
Connection: keep-alive
Content-Encoding: gzip
Content-Type: text/plain
Date: Sun, 08 Apr 2018 14:14:12 GMT
Ingress: http-svc-4
Server: nginx/1.13.11
Transfer-Encoding: chunked
Vary: Accept-Encoding

Hostname: http-svc-7dd9588c5-kbmsw

Pod Information:
	node name:	minikube
	pod name:	http-svc-7dd9588c5-kbmsw
	pod namespace:	default
	pod IP:	172.17.0.5

Server values:
	server_version=nginx: 1.13.3 - lua: 10008

Request Information:
	client_address=172.17.0.6
	method=GET
	real path=/users/sample/records
	query=
	request_version=1.1
	request_uri=http://foo.bar:8080/users/sample/records

Request Headers:
	accept=*/*
	accept-encoding=gzip, deflate
	connection=close
	host=foo.bar
	user-agent=HTTPie/0.9.8
	x-forwarded-for=172.17.0.1
	x-forwarded-host=foo.bar
	x-forwarded-port=80
	x-forwarded-proto=http
	x-original-uri=/users/sample/records
	x-real-ip=172.17.0.1
	x-scheme=http

Request Body:
	-no body in request-

aledbf · 2018-04-08T14:17:28Z

@pussinboots any overlap between "normal" paths and regular expression ends routing traffic to the route with regular expressions.

pussinboots · 2018-04-08T16:03:07Z

First of all thanks for finding this out. It is also not covered yet in the e2e test case.
I find it also a confusing but looks like that is the behaviour of nginx. location documentation

I found a good short explanation here

location  = / {
  # matches the query / only.
  [ configuration A ] 
}
location  / {
  # matches any query, since all queries begin with /, but regular
  # expressions and any longer conventional blocks will be
  # matched first.
  [ configuration B ] 
}
location /documents/ {
  # matches any query beginning with /documents/ and continues searching,
  # so regular expressions will be checked. This will be matched only if
  # regular expressions don't find a match.
  [ configuration C ] 
}
location ^~ /images/ {
  # matches any query beginning with /images/ and halts searching,
  # so regular expressions will not be checked.
  [ configuration D ] 
}
location ~* \.(gif|jpg|jpeg)$ {
  # matches any request ending in gif, jpg, or jpeg. However, all
  # requests to the /images/ directory will be handled by
  # Configuration D.   
  [ configuration E ] 
}

aledbf · 2018-04-08T16:11:10Z

I find it also a confusing but looks like that is the behaviour of nginx

@pussinboots this is the reason why I never implemented this feature.
Is too hard to build the locations and test checking for overlaps. One of the solutions implies to switch completely to regex or strict match but I always found some edge case where that doesn't work
For instance for my example:

location = /users/sample/records {
}

location ~ ^/users/(.*)/records {
}

or

location ~ ^/users/sample/records {
}

location ~ ^/users/(.*)/records {
}

those approaches work but imply we need to check all the paths in the model before creating the nginx.conf file.

pussinboots · 2018-04-08T16:42:56Z

That is what i also discover now. :-)

To simplify things here i would suppose to make it possible at least for now to define
regex and exact matches. So that your example from above would work.

If you setup ingress paths for nginx you should maybe aware of the different location
operators and its behaviour.

I will extend the regex test with your test cases.

pussinboots · 2018-04-09T09:04:13Z

Hi what you think about instead of dealing with checking the paths
for overlapping in the ingress nginx module itself.

It could be supported by a tool that you can run and check if the url
matches as you expected like an end2end test for the nginx configuration?

aledbf · 2018-04-18T13:15:57Z

@pussinboots I thin this approach can help to simplify the regex support https://forum.nginx.org/read.php?2,279504,279512#msg-279512
Basically, we can nest the location definitions like

location /users{
  location ~ /sample/records(.*) {
  }

  location ~ (.*)/records {
  }
}

(not sure it works)

pussinboots · 2018-04-26T21:35:16Z

That is a good idea. How would the ingress configuration look like?

    backend:
       path: /users/sample/records(.*)
          serviceName: svc1
       path: /users/(.*)/records
          serviceName: svc2

Or does ingress support nested paths?

aledbf · 2018-05-04T18:25:27Z

To everyone following this thread:

I think I found a way to fix the lack of regex support but before opening a PR I need help with concrete examples of Ingresses to use as a test case. Please attach comments with examples of ingresses with at least two or more paths where at least one is a regex.
Thanks in advance for the help to fix this issue.

Edit: this will not require additional annotations
Edit 2: also, please post examples of requests (path) and the expected choice from the Ingress

lalomartins · 2018-05-04T18:37:44Z

This is (an excerpt of) my actual real-life use-case. The paths to grpc services are namespaced with dots instead of slashes, which is unfortunate if I want to route them with an ingress.

  rules:
  - host: api.listenfield.com
    http:
      paths:
      - path: /listenfield\.auth\..*/
        backend:
          serviceName: auth-service
          servicePort: grpc
      - path: /listenfield\.repo\..*/
        backend:
          serviceName: farm-repo-api-service
          servicePort: grpc
      - path: /hj/repo/
        backend:
          serviceName: farm-repo-api-json-service
          servicePort: http

valorl · 2018-05-16T09:22:30Z

This is a must-have use-case for us as all our REST services are organized in this way:

  rules:
  - host: api.company.com
    http:
      paths:
      - path: ^v1/users.*$
        backend:
          serviceName: user-service
          servicePort: http
      - path: ^v1/users/.*/products$
        backend:
          serviceName: user-products-service
          servicePort: http

http://api.company.com/v1/users/123 would be routed to user-service (returning user with ID 123)
http://api.company.com/v1/users/123/products would be routed to user-products-service (returning all the products assigned to user with ID 123)

laurieodgers · 2018-06-12T07:25:18Z

We're looking to migrate an existing API into nginx ingress and this is a must have in order for it to work


  rules:
  - host: api.company.com
    http:
      paths:
      - path: ^/v2/Properties/[A-Z0-9]{32}/Messages$
        backend:
          serviceName: properties-service
          servicePort: http
      - path: ^/v2/WhoAmI$
        backend:
          serviceName: whoami-service
          servicePort: http

barryib · 2018-07-13T14:41:21Z

@aledbf Any chance to see this PR merged ?

aledbf · 2018-07-13T14:54:23Z

@barryib no in the current state. I hope I can work on this in the next release cycle

liuyangc3 · 2018-07-17T09:51:27Z

hi guys, acorrding #1360 (comment) maybe this works?

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: fooserver
spec:
  rules:
  - host: foo.example.com
    http:
      paths:
      - path: /@fake {} location ~ /some_regex
        backend:
          serviceName: fooserver
          servicePort: 8080

liuyangc3 · 2018-07-19T10:13:49Z

use a number can control location order, like
/@fake1, /@Fake2 ....

aledbf · 2018-10-02T02:26:35Z

Closing. Implemented in #3145

aledbf · 2018-10-02T02:26:45Z

Please help us to test this feature.
The image quay.io/kubernetes-ingress-controller/nginx-ingress-controller:dev contains current master where you can find this feature.

Please check doc https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/ingress-path-matching.md

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 25, 2017

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 25, 2017

abetterway2feel changed the title ~~Parse location-modifier annotation~~ Add location-modifier annotation to allow simple reg-ex locations Sep 25, 2017

abetterway2feel force-pushed the pq/location_modifier_annotation branch from 17da93d to 8a56f42 Compare September 25, 2017 16:18

abetterway2feel commented Sep 25, 2017

View reviewed changes

abetterway2feel force-pushed the pq/location_modifier_annotation branch from 8a56f42 to e406dce Compare September 25, 2017 16:21

abetterway2feel commented Sep 25, 2017

View reviewed changes

abetterway2feel force-pushed the pq/location_modifier_annotation branch 2 times, most recently from bd04459 to d282ddd Compare September 27, 2017 11:48

abetterway2feel force-pushed the pq/location_modifier_annotation branch from d282ddd to 7ff049f Compare September 27, 2017 15:04

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 27, 2017

abetterway2feel force-pushed the pq/location_modifier_annotation branch from 7ff049f to a609b86 Compare October 11, 2017 09:47

paalkr mentioned this pull request Oct 11, 2017

Modify how locations are built #970

Closed

pussinboots force-pushed the pq/location_modifier_annotation branch from e9e5449 to b2fbd09 Compare April 8, 2018 10:31

devdavidkarlsson mentioned this pull request Jun 26, 2018

Added regex option to location path #2707

Closed

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2018

This was referenced Sep 24, 2018

Revert Kube-Lego Edge Case Fix and Enforce Regex on All Locations #3123

Closed

Add "use-regex" Annotation to Toggle Regular Expression Location Modifier #3145

Merged

aledbf closed this Oct 2, 2018

Add location-modifier annotation to allow simple reg-ex locations #1415

Add location-modifier annotation to allow simple reg-ex locations #1415

Conversation

abetterway2feel commented Sep 25, 2017 • edited Loading

k8s-ci-robot commented Sep 25, 2017

k8s-reviewable commented Sep 25, 2017

abetterway2feel Sep 25, 2017

Choose a reason for hiding this comment

abetterway2feel Sep 25, 2017

Choose a reason for hiding this comment

abetterway2feel commented Sep 25, 2017

coveralls commented Sep 25, 2017

coveralls commented Sep 25, 2017

coveralls commented Sep 25, 2017

coveralls commented Sep 27, 2017

coveralls commented Sep 27, 2017

coveralls commented Sep 27, 2017

paalkr commented Sep 29, 2017

paalkr commented Sep 29, 2017

theintz commented Sep 29, 2017

paalkr commented Sep 29, 2017

abetterway2feel commented Oct 5, 2017

abetterway2feel commented Oct 11, 2017

coveralls commented Oct 11, 2017

aledbf commented Apr 6, 2018

aledbf commented Apr 8, 2018

aledbf commented Apr 8, 2018

pussinboots commented Apr 8, 2018 • edited Loading

aledbf commented Apr 8, 2018

pussinboots commented Apr 8, 2018

pussinboots commented Apr 9, 2018 • edited Loading

aledbf commented Apr 18, 2018

pussinboots commented Apr 26, 2018 • edited Loading

aledbf commented May 4, 2018 • edited Loading

lalomartins commented May 4, 2018

valorl commented May 16, 2018

laurieodgers commented Jun 12, 2018

barryib commented Jul 13, 2018

aledbf commented Jul 13, 2018

liuyangc3 commented Jul 17, 2018 • edited Loading

liuyangc3 commented Jul 19, 2018

aledbf commented Oct 2, 2018

aledbf commented Oct 2, 2018 • edited Loading

abetterway2feel commented Sep 25, 2017 •

edited

Loading

pussinboots commented Apr 8, 2018 •

edited

Loading

pussinboots commented Apr 9, 2018 •

edited

Loading

pussinboots commented Apr 26, 2018 •

edited

Loading

aledbf commented May 4, 2018 •

edited

Loading

liuyangc3 commented Jul 17, 2018 •

edited

Loading

aledbf commented Oct 2, 2018 •

edited

Loading