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.

Sign up for GitHub

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

Jump to bottom

Align path prefix matching with Ingress. #869

Merged
merged 1 commit into from
Sep 21, 2021
Merged

Align path prefix matching with Ingress. #869

merged 1 commit into from
Sep 21, 2021

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Sep 15, 2021

Explicitly align path prefix matching with the Ingress API by specifying
it as an prefix of path elements, not a prefix of bytes.

What type of PR is this?
/kind bug
/kind api-change

Which issue(s) this PR fixes:
This fixes #866.

Does this PR introduce a user-facing change?:

Aligned path prefix matching with Ingress by clarifying that it is a prefix of path elements.

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpeach

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 15, 2021
youngnick
youngnick reviewed Sep 16, 2021
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

/hold
for others to check though.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2021
@howardjohn
Copy link
Contributor

/assign @robscott

@howardjohn
Copy link
Contributor

howardjohn commented Sep 16, 2021 •
edited
Loading

Bonus points if someone adds envoy-native support so we don't have to convert it to a regex 🙂(performance+ simplicity - not a huge concern)

@jpeach
Copy link
Contributor Author

jpeach commented Sep 16, 2021

Bonus points if someone adds envoy-native support so we don't have to convert it to a regex (performance+ simplicity - not a huge concern)

FWIW, I was planning to expand this into two routes with an exact and prefix match. Maybe that won't work out :)

@howardjohn
Copy link
Contributor

Bonus points if someone adds envoy-native support so we don't have to convert it to a regex (performance+ simplicity - not a huge concern)

FWIW, I was planning to expand this into two routes with an exact and prefix match. Maybe that won't work out :)

I think the options are to use regex (runtime cost) or 2 routes (control plane cost, its not just a list of 2 strings but the whole route duplicated I believe). So both aren't ideal... but certainly shouldn't block the API here.

@jpeach jpeach mentioned this pull request Sep 16, 2021
Closed
@hbagdi
Copy link
Contributor

hbagdi commented Sep 20, 2021

/lgtm
Created #873 to track further validation.

Miciah
Miciah reviewed Sep 20, 2021
View reviewed changes
apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021
@youngnick
Copy link
Contributor

@jpeach, tide/prow won't let us merge things that have "fixes #something" in the commit message - it has to only be in the PR text.

Aside from that,
/lgtm

Once you've changed the commit message, feel free to cancel the hold.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021
@jpeach
Align path prefix matching with Ingress.
2d76932 
Explicitly align path prefix matching with the Ingress API by specifying
it as an prefix of path elements, not a prefix of bytes.

Signed-off-by: James Peach <jpeach@apache.org>
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Sep 21, 2021
@youngnick
Copy link
Contributor

/lgtm

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1cc9d1a into kubernetes-sigs:master Sep 21, 2021
@jpeach jpeach deleted the path-match-semantics branch September 21, 2021 01:53
skriss
skriss reviewed Sep 21, 2021
View reviewed changes
apis/v1alpha2/httproute_types.go
Comment on lines +224 to +235
// Matches based on a URL path prefix split by `/`. Matching is
// case sensitive and done on a path element by element basis. A
// path element refers to the list of labels in the path split by
// the `/` separator. A request is a match for path _p_ if every
// _p_ is an element-wise prefix of the request path.
//
// For example, `/abc`, `/abc/` and `/abc/def` match the prefix
// `/abc`, but `/abcd` does not.
PathMatchExact PathMatchType = "Exact"

// Matches the URL path exactly and with case sensitivity.
PathMatchPrefix PathMatchType = "Prefix"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpeach 👋 was just looking at this and the godoc for these two (Exact and Prefix) are switched.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpeach jpeach mentioned this pull request Sep 22, 2021
Merged
@skriss skriss mentioned this pull request Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@skriss skriss skriss left review comments

@youngnick youngnick youngnick left review comments

@hbagdi hbagdi Awaiting requested review from hbagdi

Assignees

@robscott robscott

@hbagdi hbagdi

@youngnick youngnick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define whether path prefix matches are element-wise or byte-wise
8 participants
@jpeach @k8s-ci-robot @howardjohn @hbagdi @youngnick @Miciah @skriss @robscott