Skip to content

Rewrite url after running all remap plugins #4531

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.

Sign up for GitHub

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

Merged
bryancall merged 1 commit into from
Nov 29, 2018
Merged

Rewrite url after running all remap plugins #4531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions proxy/http/remap/RemapPlugins.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,24 +101,24 @@ RemapPlugins::run_single_remap()
return 1;
}

if (TSREMAP_NO_REMAP == plugin_retcode || TSREMAP_NO_REMAP_STOP == plugin_retcode) {
// After running the first plugin, rewrite the request URL. This is doing the default rewrite rule
// to handle the case where no plugin ever rewrites.
//
// XXX we could probably optimize this a bit more by keeping a flag and only rewriting the request URL
// if no plugin has rewritten it already.
if (_cur == 1) {
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());
}
if (TSREMAP_DID_REMAP_STOP == plugin_retcode || TSREMAP_DID_REMAP == plugin_retcode) {
_rewriten++;
}

if (TSREMAP_NO_REMAP_STOP == plugin_retcode || TSREMAP_DID_REMAP_STOP == plugin_retcode) {
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

@d2r d2r Nov 29, 2018

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?

Debug("url_rewrite", "breaking remap plugin chain since last plugin said we should stop");
return 1;
}

if (_cur >= map->plugin_count()) {
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());
}
// Normally, we would callback into this function but we dont have anything more to do!
Debug("url_rewrite", "completed all remap plugins for rule id %d", map->map_id);
return 1;
Expand Down
11 changes: 8 additions & 3 deletions proxy/http/remap/RemapPlugins.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,17 @@
* A class that represents a queue of plugins to run
**/
struct RemapPlugins : public Continuation {
RemapPlugins() : _cur(0) {}
RemapPlugins() : _cur(0), _rewriten(0) {}
RemapPlugins(HttpTransact::State *s, URL *u, HTTPHdr *h, host_hdr_info *hi)
: _cur(0), _s(s), _request_url(u), _request_header(h), _hh_ptr(hi)
: _cur(0), _rewriten(0), _s(s), _request_url(u), _request_header(h), _hh_ptr(hi)
{
}

~RemapPlugins() override { _cur = 0; }
~RemapPlugins() override
{
_cur = 0;
_rewriten = 0;
}
// Some basic setters
void
setState(HttpTransact::State *state)
Expand Down Expand Up @@ -75,6 +79,7 @@ struct RemapPlugins : public Continuation {

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

@d2r d2r Nov 29, 2018

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

@bryancall bryancall Nov 29, 2018

Choose a reason for hiding this comment

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

I missed that :)

HttpTransact::State *_s = nullptr;
URL *_request_url = nullptr;
HTTPHdr *_request_header = nullptr;
Expand Down