Extend VS.HTTPRoute with RegexRewrite #1566

abhide · 2020-08-01T05:52:42Z

Tickets: GH-22290

abhide · 2020-08-01T06:11:54Z

/test build_api

howardjohn · 2020-08-03T00:19:08Z

networking/v1beta1/virtual_service.proto

+  oneof rewrites {
+    // Rewrite HTTP URIs and Authority headers. Rewrite cannot be used with
+    // Redirect primitive. Rewrite will be performed before forwarding.
+    HTTPRewrite rewrite = 4;


rewrite: authority: uri: regexRewrite: regex:

Seems like these are the possible options, which raises the question to me - what is being rewritten by regex, the uri or the authority? And can I do a authority prefix rewrite along with a regex rewrite of uri?

Only the URI part.

https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route/route_components.proto#envoy-api-field-route-routeaction-regex-rewrite

I don't think this is clear to users or consistent with the existing field. we should either change regex to uri or nest it under rewrite maybe?

I see.. So I can nest rewriteRegex under rewrite. Will do that change.

@howardjohn given that we can either have rewrite or regexRewrite have it nested under rewrite doesn't make sense.

Sorry, I am not sure why it doesn't make sense?

rewrite: authority: uri: regexRewrite: regex:

is clearly inconsistent to me.

rewrite: authority: uri: regix: ... prefix: ...

is ideal. However, it is not backwards compatible.

Therefor it seems that:

rewrite: authority: uri: ...prefix... uriRegix: ...regex..

is the most consistent

@howardjohn yes.. makes sense. fixed in latest changes.

nrjpoddar · 2020-08-08T20:59:23Z

@abhide can you add examples in the doc on how to use regex rewrite? This is a great functionality to add but will be difficult for users to follow without concrete examples.

abhide · 2020-08-20T19:16:39Z

@abhide can you add examples in the doc on how to use regex rewrite? This is a great functionality to add but will be difficult for users to follow without concrete examples.

@nrjpoddar Do you want me to add envoy regex examples? Essentially, we would be passing the values from VS to envoy config, correct?

howardjohn · 2020-09-28T14:49:25Z

Do you want me to add envoy regex examples? Essentially, we would be passing the values from VS to envoy config, correct?

Yes please. An example of a simple regex (we don't need a complex one, we want to show how it can be used not how regex works), and the input URL and what it will be rewritten to.

Tickets: GH-22290

abhide · 2020-10-20T23:53:55Z

@howardjohn @nrjpoddar Added a WIP patch that uses these API changes: istio/istio#28098

Looking for feedback/comments so that we can work through naming params and documentation.

abhide · 2020-10-20T23:55:14Z

networking/v1alpha3/virtual_service.proto

+  UriRegexRewrite uri_regex = 3;
+}
+
+message UriRegexRewrite {


Using same parameter names as envoy: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route/route_components.proto#envoy-api-field-route-routeaction-regex-rewrite

smawson · 2020-10-29T20:55:59Z

networking/v1alpha3/virtual_service.proto

  string authority = 2;
+
+  // uri_regex can be used for rewriting portions of path that match the
+  // pattern during forwarding the request


Suggested change

// pattern during forwarding the request

// pattern when forwarding the request.

smawson · 2020-10-29T20:56:23Z

networking/v1beta1/virtual_service.proto

  string authority = 2;
+
+  // uri_regex can be used for rewriting portions of path that match the
+  // pattern during forwarding the request


Suggested change

// pattern during forwarding the request

// pattern when forwarding the request.

guillevalin · 2021-01-26T13:11:44Z

Hi istio team,

I've been following this issue and PR since september past year, we want to migrate to istio but we need regex rewrite rules to replace nginx-ingress.

Is there any chance this PR could be merged soon?

Thank you in advance.

costinm · 2021-01-26T15:40:20Z

I am a bit concerned with this - can you add a discussion on networking WG ?
AFAIK it is possible to use an EnvoyFilter to achieve this result - so advanced users who need this are not blocked.

I don't think Istio API must mirror all the features in envoy - right now the consensus in the WG is to adopt the upstream
K8S networking APIs, which in turn allow multiple implementation besides Istio. They do allow some 'extension points'
as well - but any extension point will be vendor-specific.

I don't think RE2 based rewrite a URL is a common enough feature that would be supported cross-vendor, and it puts
unreasonable requirements on non-envoy based implementations of Istio ( proxyless ).

costinm · 2021-01-26T15:43:26Z

At least we should bounce the idea with the K8S Services WG - and the other projects planning to support the K8S APIs.
I assume since this didn't make it to 1.9 we can afford few weeks to discuss with the relevant WGs.

abhide · 2021-01-26T18:49:37Z

I am not actively working on this GH issue. Will un-assign myself so that someone can pick it up.

SpecialYang · 2021-01-27T12:45:22Z

I really need this awesome feature and want to support it. Can I pick it up? @howardjohn @hzxuzhonghu

dacloutier · 2021-10-25T20:48:05Z

... AFAIK it is possible to use an EnvoyFilter to achieve this result - so advanced users who need this are not blocked.
...
I don't think RE2 based rewrite a URL is a common enough feature that would be supported cross-vendor, and it puts unreasonable requirements on non-envoy based implementations of Istio ( proxyless ).

@costinm I'm not sure how you can say that routing/rewriting with path variables is "unreasonable" "advanced" "uncommon"

Have you never written a API that's like this: /api/someresource/{some-id}/subresource-a & /api/someresource/{some-id}/subresource-b ?

dacloutier · 2021-10-26T20:00:26Z

@SpecialYang have you build a forked image for this?

abhide requested review from dcberg, duderino, linsun, louiscryan, nrjpoddar, rshriram and smawson as code owners August 1, 2020 05:52

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 1, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 1, 2020

abhide requested a review from howardjohn August 1, 2020 05:53

abhide force-pushed the GH-22290 branch from e831469 to e6ea2ed Compare August 1, 2020 06:02

howardjohn reviewed Aug 3, 2020

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 21, 2020

abhide force-pushed the GH-22290 branch from e6ea2ed to 41ce1b0 Compare September 23, 2020 06:19

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2020

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 23, 2020

abhide removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2020

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2020

abhide mentioned this pull request Sep 23, 2020

Regex rewrites using capture groups istio/istio#22290

Closed

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2020

abhide force-pushed the GH-22290 branch from 41ce1b0 to 952b96f Compare October 20, 2020 06:14

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2020

WIP: Extend VS.HTTPRoute with RegexRewrite

dba9997

Tickets: GH-22290

abhide force-pushed the GH-22290 branch from 952b96f to dba9997 Compare October 20, 2020 06:36

istio-testing removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 20, 2020

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 20, 2020

abhide commented Oct 20, 2020

View reviewed changes

howardjohn approved these changes Oct 24, 2020

View reviewed changes

ramaraochavali approved these changes Oct 24, 2020

View reviewed changes

smawson reviewed Oct 29, 2020

View reviewed changes

abhide closed this Jan 26, 2021

SpecialYang mentioned this pull request Jan 27, 2021

Add regex rewrite #1854

Closed

	// pattern during forwarding the request
	// pattern when forwarding the request.

Extend VS.HTTPRoute with RegexRewrite #1566

Extend VS.HTTPRoute with RegexRewrite #1566

Uh oh!

Conversation

abhide commented Aug 1, 2020

Uh oh!

abhide commented Aug 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrjpoddar commented Aug 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhide commented Aug 20, 2020

Uh oh!

howardjohn commented Sep 28, 2020

Uh oh!

abhide commented Oct 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillevalin commented Jan 26, 2021

Uh oh!

costinm commented Jan 26, 2021

Uh oh!

costinm commented Jan 26, 2021

Uh oh!

abhide commented Jan 26, 2021

Uh oh!

SpecialYang commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dacloutier commented Oct 25, 2021

Uh oh!

dacloutier commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

nrjpoddar commented Aug 8, 2020 •

edited

Loading

SpecialYang commented Jan 27, 2021 •

edited

Loading