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

Update KIngress rule path to be a literal string #364

Merged

Conversation

impl
Copy link
Contributor

@impl impl commented Mar 9, 2021

Changes

🧹 The documentation comment in ingress_types.go called for the path of a rule to be an extended POSIX regular expression, but this behavior has not been implemented correctly by any of the networking layers. The Serving component also treats it as a literal when it sets up the path.

This change clarifies the comment to make it clear that this is, in fact, a literal string prefix, and not a regular expression.

You can see the implementation in the Serving component here:

The implementation in net-istio tries to use a regular expression, but because Envoy uses exact matching for regular expressions instead of prefix matching, it actually seemingly deviates from the behavior of other networking layers.

On the other hand, net-contour just uses a regular ol' prefix match.

If this change makes sense to you all, I would be happy to open an additional PR against net-istio to make it use a prefix match as well.

See also our discussion over in the Ambassador repository here: emissary-ingress/emissary#3224

/kind cleanup
/cc @kflynn

The documentation comment called for the path of a rule to be an
extended POSIX regular expression, but this behavior has not been
implemented correctly by any of the networking layers. The Serving
component also treats it as a literal when it sets up the path.

This change clarifies the comment to make it clear that this is, in
fact, a literal string prefix, and not a regular expression.
@knative-prow-robot knative-prow-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Mar 9, 2021
@knative-prow-robot
Copy link

@impl: GitHub didn't allow me to request PR reviews from the following users: kflynn.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Changes

🧹 The documentation comment in ingress_types.go called for the path of a rule to be an extended POSIX regular expression, but this behavior has not been implemented correctly by any of the networking layers. The Serving component also treats it as a literal when it sets up the path.

This change clarifies the comment to make it clear that this is, in fact, a literal string prefix, and not a regular expression.

You can see the implementation in the Serving component here:

The implementation in net-istio tries to use a regular expression, but because Envoy uses exact matching for regular expressions instead of prefix matching, it actually seemingly deviates from the behavior of other networking layers.

On the other hand, net-contour just uses a regular ol' prefix match.

If this change makes sense to you all, I would be happy to open an additional PR against net-istio to make it use a prefix match as well.

See also our discussion over in the Ambassador repository here: emissary-ingress/emissary#3224

/kind cleanup
/cc @kflynn

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.

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 9, 2021
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 9, 2021
@knative-prow-robot
Copy link

Welcome @impl! It looks like this is your first PR to knative/networking 🎉

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2021
@knative-prow-robot
Copy link

Hi @impl. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #364 (b0bb3e5) into main (42a36ac) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #364   +/-   ##
=======================================
  Coverage   95.30%   95.30%           
=======================================
  Files          37       37           
  Lines         766      766           
=======================================
  Hits          730      730           
  Misses         25       25           
  Partials       11       11           
Impacted Files Coverage Δ
pkg/apis/networking/v1alpha1/ingress_types.go 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42a36ac...b0bb3e5. Read the comment docs.

@nak3
Copy link
Contributor

nak3 commented Mar 10, 2021

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 10, 2021
@nak3
Copy link
Contributor

nak3 commented Mar 10, 2021

/lgtm

I guess that the comment was aligned with k8s's Ingress v1 but the k8s's ingress also drops the comment lines and just support exact and prefix type as per kubernetes/api@08fd6c9 kubernetes/kubernetes#88587
And as @impl also explained, Knative's networking layer does not cover regex so I think it is OK for this change.

/assign @tcnghia @ZhiminXiang

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2021
@tcnghia
Copy link
Contributor

tcnghia commented Mar 10, 2021

/lgtm

1 similar comment
@ZhiminXiang
Copy link

/lgtm

@impl
Copy link
Contributor Author

impl commented Mar 10, 2021

Thanks! That explanation makes a lot of sense. I'll follow up with a fix PR to net-istio and a small docs update to net-contour. net-kourier seems fine as-is.

/assign @grantr

@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tcnghia

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2021
@knative-prow-robot knative-prow-robot merged commit 5a157db into knative:main Mar 10, 2021
@impl impl deleted the simplify-ingress-path-spec branch March 10, 2021 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants