Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ingress V1: path matching mode #2135

Closed
jpeach opened this issue Jan 22, 2020 · 8 comments · Fixed by #3471
Closed

Ingress V1: path matching mode #2135

jpeach opened this issue Jan 22, 2020 · 8 comments · Fixed by #3471
Assignees
Labels
area/ingress Issues or PRs related to the Ingress API.
Milestone

Comments

@jpeach
Copy link
Contributor

jpeach commented Jan 22, 2020

Support Ingress V1 path changes:

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md#path-as-a-prefix

Paths proposal

Explicitly state the match mode of the path.
Support the existing implementation-specific behavior.
Support a portable prefix match and future expansion of behavior.
Add a field ingress.spec.rules.http.paths.pathType to indicate the desired interpretation of the meaning of the path

@jpeach jpeach added the area/ingress Issues or PRs related to the Ingress API. label Jan 22, 2020
@jpeach jpeach mentioned this issue Jan 22, 2020
6 tasks
@jpeach
Copy link
Contributor Author

jpeach commented Jan 23, 2020

cmluciano/kubernetes#2

@sunjayBhatia
Copy link
Member

sunjayBhatia commented Mar 11, 2021

So we already support regex path matching on Ingress resources but the current implementation is such that the path match type is ignored if you put something in the path field that "looks like" a regex.

Seems like this regex path match deal is better suited for just the ImplementationSpecific path match type and should be documented as such, however that would be a breaking change (though IMO it sounds more correct).

So moving forward how does this sound:

  • If you provide a path that "looks like" a regex, you will get a regex match in practice, no matter what you set on the path type
    • As mentioned above, the alternative option would be to honor the path type set and only do a regex match if you set the type to implementation specific but this would be breaking
    • We could also do the "looks like regex" check only if you don't specify a path type Not possible, path type is validated
  • If you do not specify a path match type, you get a prefix match Not possible, path type is validated
  • If you specify a prefix path match type, you get a prefix match
  • If you specify an exact path match type, you get an exact match
  • If you specify an implementation specific path match type, you get a regex match

@youngnick
Copy link
Member

We could also put a changelog note and change it, although that would also not be ideal. I anticipate that the Ingress code is going to be around a long time though, so we should make it as tidy as posisble.

If we don't want to do that, @sunjayBhatia's solution above seems like the only way.

@sunjayBhatia
Copy link
Member

Another weird caveat that the spec wants: https://github.com/kubernetes-sigs/ingress-controller-conformance/blob/79bd068cbb31d77c2a060f61eea795f9791a3d48/features/path_rules.feature#L162

Prefix matches are not string prefix matches, but path label matches

See also https://kubernetes.io/docs/concepts/services-networking/ingress/#examples


Kind | Path(s) | Request path(s) | Matches?
...
Prefix | /aaa/bbb | /aaa/bbbxyz | No, does not match string prefix

@youngnick
Copy link
Member

We specifically had to stop doing prefix matches using Go's Path support because of weird interactions with inclusion for HTTPProxy, but if we're going to meet the spec, maybe the Ingress validation code can just treat them as path labels.

@sunjayBhatia
Copy link
Member

To close the loop what we ended up coming up with in office hours is the following:

  • Exact match gives you exact matching
  • Prefix match gives you path segment matching
  • Implementation specific will give you a regex match if you supply a string that "looks like" a regex and otherwise a string prefix match
    • To ensure people relying on the old behavior have a path match they can use
    • If you don't specify a path match type right now you get "ImplementationSpecific" filled in for you, so this should hopefully seamlessly work for most cases

sunjayBhatia added a commit that referenced this issue Mar 26, 2021
…fix match updated to match spec (#3471)

Since we already had support for regex path matching without checking
the type of the path match, we now have the following rules to ensure backwards compatibility:

- If you specify a prefix path match type, you get a prefix match
  - prefix matches only match path segments, not a string prefix
  - trailing slashes ignored from prefix match string
- If you specify an exact path match type, you get an exact match
- If you specify an implementation specific path match type you get path matching behavior that Contour already supports
  - If it looks like a regex we give a regex match
  - otherwise a string prefix match like Contour already does
  - ImplementationSpecific match is the default if not set when an Ingress
  resource is created so this should give people the default they are
  already using

Fixes #2135

Signed-off-by: Sunjay Bhatia <[email protected]>
@shashankram
Copy link
Contributor

shashankram commented Apr 1, 2021

To close the loop what we ended up coming up with in office hours is the following:

  • Exact match gives you exact matching

  • Prefix match gives you path segment matching

  • Implementation specific will give you a regex match if you supply a string that "looks like" a regex and otherwise a string prefix match

    • To ensure people relying on the old behavior have a path match they can use
    • If you don't specify a path match type right now you get "ImplementationSpecific" filled in for you, so this should hopefully seamlessly work for most cases

I must admit this is a great way to blend additional functionality with simplicity. In the openservicemesh.io project that I work on, I am upgrading our Ingress capability to v1 and was wondering how ImplementationSpecific path types can be interpreted in a reasonable way, given that our software must work at present with multiple ingress controllers. Do I have permission to borrow the looks like a regex idea for ImplementationSpecific path matches? @projectcontour/maintainers (/cc @youngnick, @stevesloka)

@sunjayBhatia
Copy link
Member

This path matching behavior was done b/c that was all Contour supported before implementing Ingress v1 fully and I want to say was that way b/c it mimiced the nginx Ingress implementation which came first? Someone who has been around longer can check me on that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ingress Issues or PRs related to the Ingress API.
Projects
None yet
4 participants