Skip to content

Conversation

@takkitano
Copy link
Contributor

Described at #2877

When we configure remap.config like the following snippet, the second remap plugin using rri->requestUrl get post-remap url information from ATS.

map http://before-remap.com/ http://after-remap.com/ @plugin=<first plugin>.so @pparam=... @plugin=<second plugin>.so @pparam=...

The cause of this behavior is ATS executes url_rewrite_remap_request function after running the first remap plugin.
Therefore, behavior of a remap plugin as the first plugin is different from that as the second plugin.

I think ATS should execute all remap plugins and then rewrite url.

@zwoop
Copy link
Contributor

zwoop commented Nov 1, 2018

This has been a mess for a while, but I think this also needs to be discussed on the mailing list, to make sure we don't get surprises.

@zwoop zwoop added this to the 9.0.0 milestone Nov 1, 2018
@takkitano
Copy link
Contributor Author

takkitano commented Nov 5, 2018

OK. I will send mail about this problem to dev mailing list.

@randall
Copy link
Contributor

randall commented Nov 16, 2018

[approve ci]

@SolidWallOfCode
Copy link
Member

My one concern was that rewritten is cleared to zero every time, but the proxy object in the allocator should have that as zero and the allocator should copy it over when it's allocated for a remap operation.


private:
unsigned int _cur = 0;
unsigned int _rewriten = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor, but could we name the member _rewritten with two ts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that :)

@bryancall bryancall merged commit fc785d3 into apache:master Nov 29, 2018
if (_rewriten == 0) {
Debug("url_rewrite", "plugin did not change host, port or path, copying from mapping rule");
url_rewrite_remap_request(_s->url_map, _request_url, _s->hdr_info.client_request.method_get_wksidx());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are moving and then duplicating this code. Should we pull this out into its own function or is it OK as-is?

@d2r
Copy link
Contributor

d2r commented Nov 29, 2018

I did not notice that this had been merged while I was reviewing it.
My comments were minor though, so it's OK to disregard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants