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

Feature Request: Support nginx style rewrite rules via rds.RedirectAction #2092

Closed
brirams opened this issue Nov 21, 2017 · 15 comments
Closed
Labels
beginner Good starter issues! enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@brirams
Copy link
Contributor

brirams commented Nov 21, 2017

Currently, routes defined via RDS can have a RedirectAction that specify the host and path a matching Route will be redirected to. This works for the cases where one knows what the destination path is up front but not so much for the cases where prefix/suffix rewriting is needed. Nginx does this by allowing one to specify a matching regex on the original path and being able to reference capture groups in the destination:

rewrite ^/foo/(bar)/(.*)$ https://$server_name/$1/$2 permanent;

Ideally, envoy would able to support that kind of referencing but, in lieu of that, being able to at least refer to the original path in a RedirectAction.PathRedirect would help get us most of the way there. The same goes for protocol rewriting.

Context: https://envoyproxy.slack.com/archives/C78M4KW76/p1510966916000159

@mattklein123 mattklein123 added beginner Good starter issues! enhancement Feature requests. Not bugs or questions. help wanted Needs help! labels Nov 21, 2017
@srikailash
Copy link
Contributor

srikailash commented Feb 19, 2018

Hello @mattklein123, I can work on this issue. Made a sample implementation here : https://github.com/srikailash/envoy/commit/54745c23052032052e4386d05ac38074c08b6cf3 . Once we decide on what exactly is going to be the name in configuration(route.proto) for the regex entries. I can change code accordingly and submit a working pull request. Let me know if that looks good.
Thanks in advance!

@alyssawilk
Copy link
Contributor

Thanks for the patch! @mattklein123 is (mostly) out for the next few weeks so I'd encourage you to do best-effort naming and send out a PR for the data plane change, and your reviewer will help on naming support :-)

@mattklein123
Copy link
Member

I think this is done.

@zuercher
Copy link
Member

The PR for this was never completed. Reopening.

@zuercher zuercher reopened this Jan 11, 2019
@zuercher
Copy link
Member

For future reference, I think there are people who would like both the ability to regex-rewrites of paths (a la #4403) and regex-rewrites of redirects.

@pepjo
Copy link

pepjo commented Apr 25, 2019

The pull request was closed 6 days ago. What is the status of this feature?

@harindaka
Copy link

Is this not possible yet?

@zuercher
Copy link
Member

The original PRs that would have implemented this were never merged. PR #8462 resurrects that implementation.

@cmluciano
Copy link
Member

/assign @cmluciano

@DevZenFlow
Copy link

This would benefit us too!

@Barborica-Alexandru
Copy link

Barborica-Alexandru commented Dec 7, 2019

I would love this feature. It would make envoy sidecars extremely usable.

@steeling
Copy link

We would like to use this for rewriting hostnames.

Our use case is for routing to specific pods within kubernetes statefulsets. We'd like to setup a an external hostname like 3.abc.xyz.com and reroute to -> myservice-3 in kubernetes.

We could even get by if there was a way to prefix match a VirtualHost and strip the unmatched prefix and add it as a header.

Is there another way to do this that I'm missing?

@sagarkal
Copy link

sagarkal commented Feb 4, 2020

We need this feature as well, since we are replacing nginx with envoy..any ETA on this?

@jespersoderlund
Copy link
Contributor

The referenced issue didn't add this for redirects :(

@esmet
Copy link
Contributor

esmet commented Feb 5, 2021

I may have duplicated this issue in #14348. The associated PR is merged.

If we feel like the above issue is in fact the same thing and the associated PR fixes it, we can close this out.

jpsim added a commit that referenced this issue Nov 28, 2022
jpsim added a commit that referenced this issue Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner Good starter issues! enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet