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

GEP: Add Path Redirects and Rewrites #726

Closed
robscott opened this issue Jul 19, 2021 · 24 comments · Fixed by #1874
Closed

GEP: Add Path Redirects and Rewrites #726

robscott opened this issue Jul 19, 2021 · 24 comments · Fixed by #1874
Assignees
Labels
kind/gep PRs related to Gateway Enhancement Proposal(GEP) priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@robscott
Copy link
Member

What would you like to be added:
This GEP attempts to cover 2 related but different features:

  • Path Redirects
  • Rewrites

Once this GEP is complete, HTTPRoute should include filters that enable basic path and hostname redirects and rewrites. Although there is definitely variation among implementations as far as what is possible, there appears to be enough common ground for a portable API.

Why is this needed:
This is a common feature request, as noted by #200 and #678.

@robscott robscott added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 19, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 17, 2021
@jpeach
Copy link
Contributor

jpeach commented Oct 17, 2021

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 17, 2021
@dacloutier
Copy link

dacloutier commented Dec 2, 2021

How can I go about requesting a change to the gateway-api to support regex/capture group Path rewrites?

Istio won't merge the PR that would support this until it gets added to the gateway api.... and many people ( including me ;D ) need regex path and capture group rewrites. (see: istio/istio#22290 & istio/api#1854)

The feature is discussed in #731 but only mentions it as a "Future extension"

Thanks for helping me get some movement on this.

@youngnick
Copy link
Contributor

youngnick commented Dec 3, 2021

@dacloutier, thanks for the feedback.

I understand this feature is important to you and a lot of people, and the reason we've left it for a future extension in #731 is that it's actually pretty complicated to write a standard way for it to work, that will work across multiple proxies/Gateway implementations, and still allow you to migrate your config between Gateway implementations (which is a primary goal of this project).

We definitely plan to get to this, but need to make sure that whatever will do will be usable to a broad base of people. Just as a quick example, it's very common for different implementations to use different regex flavors, but for the Gateway API, we need to be able to do something that will work across as many as possible.

Also, what we've found up until this point is that some things that you would think are pretty basic functionality can vary wildly between implementations and make it very difficult for there to be a single, standard way to implement them. Timeouts for HTTP connections are a great example of this - every implementation has there own set of timeouts that mean subtly different things, and we haven't been able to build a good common set to be able to make a HTTPRoute TimeoutPolicy.

Given that regex is already pretty complicated, and there are different flavors, and different implementations, we need to be a very careful with the design.

I know "we're going to get to it" is a very unsatisfying answer, but unfortunately, that's all we have at the moment. I certainly think that your enthusiasm here does a great job of pushing it up my priority list. 😄

I should also say that, as always, contributions are welcomed - often the hardest part is getting the discussion started!

@rufreakde
Copy link

Hi @youngnick,
thanks for the update and the clarification that it is on the Radar here.

Just wanted to drop this here as well. (And I am sure you already looked at this stuff yourself but I guess it does not harm)

The POSIX basic and extended

even though it is not very popular as it is rather limited it is often a subset of many Regex flavors. And for the basic Regex Rewrite this standard would mostly suffice as a solution here. (Just my honest opinion does not need to be the case though)
Of course I would love to see a more elaborate version but in terms of transportability

we need to be able to do something that will work across as many as possible

this might be a very good first step that can be extended later.

@youngnick
Copy link
Contributor

Thanks @rufreakde, you make a good point about using POSIX regex as a probably-compatible flavor, we'll make sure we keep that in mind.

@hbagdi
Copy link
Contributor

hbagdi commented Dec 6, 2021

In case anyone is interested in contributing, a thorough document that ensures that POSIX regexes (or even a subset) is supported and has the same semantics across PCRE, RE2, < other possible regex engines found in implementations >.

@youngnick
Copy link
Contributor

In case anyone is interested in contributing, a thorough document that ensures that POSIX regexes (or even a subset) is supported and has the same semantics across PCRE, RE2, .

Looks like Github ate some of your comment @hbagdi ?

@hbagdi
Copy link
Contributor

hbagdi commented Dec 10, 2021

Looks like Github ate some of your comment @hbagdi ?

🤦🏾‍♂️ Words interpreted incorrectly as Markdown ( ). Fixed now.

@rufreakde
Copy link

@hbagdi you mean some documentation for later use?
Several Different Regex Engines:
Overview

PCRE:
Documentation

RE2:
Documentation
Github Issue talking about compatibility and the RE2:POSIX option

@shaneutt
Copy link
Member

shaneutt commented Nov 2, 2022

@robscott as per our discussions at Kubecon, it would seem we want to move this into standard for v0.6.0, or maybe v0.6.1 is that right? Is this even the best issue to track this with?

@robscott
Copy link
Member Author

robscott commented Nov 7, 2022

As much as I'd love to fit this in, we don't have conformance test coverage for this yet. Since we're already running behind on the v0.6.0 release, I'm going to bump this to the v0.7.0 release.

@robscott robscott modified the milestones: v0.6.0, v0.7.0 Nov 7, 2022
@robscott robscott added kind/gep PRs related to Gateway Enhancement Proposal(GEP) and removed kind/feature Categorizes issue or PR as related to a new feature. labels Feb 6, 2023
@robscott
Copy link
Member Author

robscott commented Feb 6, 2023

Hey everyone, we're hoping to graduate this feature to standard release channel as part of v0.7.0. Here's where I think we are with graduation criteria:

  • Implemented by several implementations.
  • Conformance test framework is in place, with some coverage of basic functionality.
  • Validation is well thought out.
  • Most of the API surface is being exercised by users.
  • Approval from subproject owners + KEP reviewers.

I think we've accomplished most of the graduation criteria, but I'm looking for some help determining which implementations have support for this to see if we can check that box.

Update: It looks like we have enough implementations in place to graduate to standard channel.

So far I know the following have support:

  • Envoy Gateway
  • Istio
  • Contour

And the following do not yet:

  • GKE
  • Kong

@youngnick, @skriss, @pleshakov or anyone else, are there any additional implementations supporting this feature?

@pleshakov
Copy link
Contributor

@robscott at NGINX Kubernetes Gateway we have implemented request redirect filter but without support for the path field yet link

@skriss
Copy link
Contributor

skriss commented Feb 7, 2023

@robscott Contour has largely implemented both, the only field I know we don't yet have implemented is redirect path rewrites which is marked experimental.

@Xunzhuo
Copy link
Member

Xunzhuo commented Feb 7, 2023

@robscott Envoy Gateway has implemented all of them now.

@robscott
Copy link
Member Author

I think my initial comment was a bit confusing, this GEP covers:

  1. Path Redirects
  2. Entirety of Rewrite filter

Both are currently marked as experimental and we're trying to find implementations that support them so we can graduate to beta.

@skriss does your comment above mean that Contour supports 2 but not 1?

@skriss
Copy link
Contributor

skriss commented Feb 14, 2023

@robscott thanks for the clarification - Contour now has support for both, we got path redirects merged last week.

@shaneutt
Copy link
Member

What's next for us here? Does anyone specifically want to champion this one and push it forward?

@shaneutt shaneutt added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Feb 21, 2023
@shaneutt shaneutt moved this from Backlog to In Progress in Gateway API: The Road to GA Feb 21, 2023
@shaneutt shaneutt added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Feb 22, 2023
@robscott
Copy link
Member Author

I believe the next steps are to confirm that we have at least 2 implementations that pass conformance tests for this feature. It sounds like we probably do based on the comments above. If that's the case, we should look into graduating this to standard in v0.7.0.

@danehans
Copy link
Contributor

For Envoy Gateway, the HTTPRouteRedirectPath conformance test was disabled due to envoyproxy/gateway#993.

@arkodg
Copy link
Contributor

arkodg commented Mar 14, 2023

For Envoy Gateway, the HTTPRouteRedirectPath conformance test was disabled due to envoyproxy/gateway#993.

thanks for raising this @danehans
We anticipate that once #1805 is addressed, Envoy Gateway should be able to support HTTPRouteRedirectPath.

@shaneutt shaneutt removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 14, 2023
@github-project-automation github-project-automation bot moved this from Experimental to Implementable in Gateway API Enhancement Proposals (GEPs) Mar 29, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Gateway API: The Road to GA Mar 29, 2023
@shaneutt shaneutt moved this from Implementable to Complete in Gateway API Enhancement Proposals (GEPs) Aug 22, 2023
@Bryce-huang
Copy link

Why is Substitution not found here?

@robscott
Copy link
Member Author

@Bryce-huang I think you want #2177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/gep PRs related to Gateway Enhancement Proposal(GEP) priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
Development

Successfully merging a pull request may close this issue.