Introduce URL rewriting policy [THREESCALE-296] #529

davidor · 2017-12-12T15:08:07Z

This is a first version of the URL rewriting policy. I'd rather agree on the basics first and add more functionality later.

Currently, this policy works well when placed after the apicast policy in the policy chain. However, it does not when placed before the apicast policy. In that case, the URL would be rewritten, and the matching rules would apply over that rewritten URL. To make that work, we need to make a few changes in the code before. For example, we need to stop relying on ngx.var.request when matching mapping rules, because it does not change after rewriting the url with ngx.req.set_uri.

mikz · 2017-12-12T15:11:04Z

gateway/src/apicast/policy/url_rewriting/policy.lua

+-- Returns the substitute function to be applied according to the rewrite
+-- operation. The result can be ngx.re.sub or ngx.re.gsub.
+local function substitute_func(rewrite_operation)
+  if rewrite_operation == 'sub' then


Maybe better to have a dispatch table?

OK. I don't have a strong preference about this one. I applied that pattern in the headers policy https://github.com/3scale/apicast/blob/d8ddb6a8313e22637d7b0c3bca35a3c645d106ad/gateway/src/apicast/policy/headers/policy.lua#L58

I have a feeling it could have better performance, but can't really say without a benchmark/profile/jit trace.

mikz · 2017-12-12T15:16:05Z

gateway/src/apicast/policy/url_rewriting/schema.json

+          "options": {
+            "type": "string"
+          },
+          "break": {


We are considering also other mutually exclusive values like: last, redirect, persistent right?

IMO might be nicer to model it in a way they are truly mutually exclusive by being a value of one key: "type": "break".

But since we are not releasing this yet, It could wait until we have some more options.

mikz · 2017-12-12T15:16:41Z

t/apicast-policy-url-rewriting.t

+use lib 't';
+use TestAPIcastBlackbox 'no_plan';
+
+repeat_each(1);


Any reason why we can't have default 2 runs ?

No. My bad. Copied it from the wrong place.

davidor · 2017-12-12T16:50:44Z

I changed the couple of things that you mentioned in your comments @mikz

mikz · 2017-12-12T16:54:19Z

gateway/src/apicast/policy/url_rewriting/policy.lua

+end
+
+--- Initialize a URL rewriting policy
+-- @tparam[opt] table config. Contains the rewrite commands.


the dot after config looks like extra. You can see it at https://3244-53965666-gh.circle-artifacts.com/0/doc/modules/policy.url_rewriting.policy.html

Well spotted. Fixed.

mikz · 2017-12-12T16:55:09Z

gateway/src/apicast/policy/url_rewriting/policy.lua

+--   - replace: string that will replace whatever is matched by the regex.
+--   - options[opt]: options to control how the regex match will be done.
+--     Accepted options are the ones in 'ngx.re.sub' and 'ngx.re.gsub'.
+--   - break[opt]: defaults to false. When set to true, if the command rewrote


This is not recognized by the ldoc and looks kind of ugly in the generated doc. Not sure how better describe it though.

mikz

👍

MarkCheshire · 2017-12-13T09:17:17Z

I don't think it is a valid use case to apply the rewrite policy before the Apicast policy, so I recommend to avoid adding that complexity.

andrewdavidmackenzie · 2017-12-13T09:21:08Z

I agree - we should document that in some customer visible place so that people looking at using this policy see that they should place it after.

andrewdavidmackenzie · 2017-12-13T10:13:53Z

Talking with @davidor I understand some more and so expand the explanation.....

I think this is the first policy we add that has a coupling with an existing policy (apicast policy)?
i.e. Mapping rules matches paths of the request and increments metrics etc, and the URL rewrite policy can change request paths AFTER Mapping rules have been applied.

So, at the moment (if we assume that the user understands that mapping rules are applied to the original path and then url rewrite happens later), all works fine.

Longer-term I think it's valid to consider mapping rules "one more policy", but at the moment it's pretty baked-in to both apicast (via the apicast policy) and System UI.
I think in System for AMP 2.2 we are not planning to extract this as an optional policy that can be placed anywhere in the pipeline (please correct me if I'm wrong).

So, for AMP 2.2 - we have it all baked-in and assuming the order is understood by the customer - it all works.

If in the future we break out Mapping Rules as an optional, re-orderable policy that could be put after URL rewriting - then the behaviour would change wrt now. The customer would need to understand that and write Mapping rules knowing the urls produced by the previously executed url rewrite policy.

Does that make sense?

mikz · 2017-12-13T10:27:34Z

It does make sense. But it is all possible now without extracting mapping rules into own policy.
It is enough to move extracting mapping rules into different phase - from access to rewrite.

We have a need to do something between mapping rules / credentials extraction and calling 3scale backend (#495). Customers override internal methods to add custom metrics.

By moving the mapping rules and credentials extraction into rewrite phase we allow custom policies to hook into the process and modify matched metrics and credentials before they are sent to backend.

That gives us enough flexibility in 2.2 release without extracting Mapping Rules into own policy.

And rewriting URLs policy should work both when it is before/after APIcast policy.
Simply because you indicate the rewriting should project in the mapped metrics or not.
When used before you indicate that you want mapping rules to be applied on rewritten URL.
When used after you want to apply mapping rules on what client sent not considering the rewritten URL.

The change is very small and the use case is valid. But the code in question is frequently overridden by customers: https://github.com/3scale/apicast/blob/632d0f38a436b95c864be0c901713ff0cd3dbc5c/gateway/src/apicast/configuration.lua#L215
So we would like to also give them means to accomplish what they want without overriding internal methods. Hence the extraction of mapping rules / credentials to the rewrite phase.
That is needed for other policies to work anyway (like SOAP that needs to add custom metric based on a header).

I appreciate the concern, but we are going to make it work and make the code better and faster in the process. Simply because we need it for other things too.

octobot assigned davidor Dec 12, 2017

mikz reviewed Dec 12, 2017

View reviewed changes

davidor force-pushed the url-rewriting-policy branch from 1ddbcfa to 0ca2ff6 Compare December 12, 2017 16:50

mikz reviewed Dec 12, 2017

View reviewed changes

mikz approved these changes Dec 12, 2017

View reviewed changes

davidor force-pushed the url-rewriting-policy branch 3 times, most recently from b5dbb59 to c324262 Compare December 12, 2017 17:33

davidor added 4 commits December 12, 2017 18:35

Add URL rewriting policy

b05eb3c

policy/url_rewriting: add json schema

776bce9

t: test URL rewriting policy

d0b3696

CHANGELOG: add entry for url rewriting policy

37b3b35

davidor force-pushed the url-rewriting-policy branch from c324262 to 37b3b35 Compare December 12, 2017 17:36

davidor merged commit c3bde5a into master Dec 12, 2017

davidor deleted the url-rewriting-policy branch December 12, 2017 17:55

davidor mentioned this pull request Dec 13, 2017

[WIP] URL rewriting #462

Closed

davidor mentioned this pull request Dec 13, 2017

Make it possible to place url rewriting before apicast in the policy chain #531

Merged

mikz changed the title ~~Introduce URL rewriting policy~~ Introduce URL rewriting policy [THREESCALE-296] Apr 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce URL rewriting policy [THREESCALE-296] #529

Introduce URL rewriting policy [THREESCALE-296] #529

davidor commented Dec 12, 2017

mikz Dec 12, 2017

davidor Dec 12, 2017

mikz Dec 12, 2017

mikz Dec 12, 2017

mikz Dec 12, 2017

davidor Dec 12, 2017

davidor commented Dec 12, 2017

mikz Dec 12, 2017

davidor Dec 12, 2017

mikz Dec 12, 2017

davidor Dec 12, 2017

mikz left a comment

MarkCheshire commented Dec 13, 2017

andrewdavidmackenzie commented Dec 13, 2017

andrewdavidmackenzie commented Dec 13, 2017 •

edited

mikz commented Dec 13, 2017

Introduce URL rewriting policy [THREESCALE-296] #529

Introduce URL rewriting policy [THREESCALE-296] #529

Conversation

davidor commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

MarkCheshire commented Dec 13, 2017

andrewdavidmackenzie commented Dec 13, 2017

andrewdavidmackenzie commented Dec 13, 2017 • edited

mikz commented Dec 13, 2017

andrewdavidmackenzie commented Dec 13, 2017 •

edited