Introduce URL rewriting policy [THREESCALE-296]

[davidor]

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]

Maybe better to have a dispatch table?

[davidor]

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

[mikz]

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

[mikz]

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]

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

[davidor]

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

[davidor]

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

[mikz]

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

[davidor]

Well spotted. Fixed.

[mikz]

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

[davidor]

Fixed :)

[mikz]

👍

[MarkCheshire]

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]

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]

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]

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.