TS-1889: refactor remap plugin request URL handling For (apparantly) historical reasons, the remap plugin support duplicated the request URL rewriting code. There's really no need for this, and it's actually harmful since we don't want the behavior to diverge due to the presence of a remap plugin.
This change removes the dodgy request URL rewriting code and uses the same rewriting code that is used in the non-plugin case. This is easier to understand and more reliable. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/ed93dcbf Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/ed93dcbf Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/ed93dcbf Branch: refs/heads/master Commit: ed93dcbf7e394a1ff4b57365ea0148ef7c6db249 Parents: 0a78040 Author: James Peach <[email protected]> Authored: Sat May 4 14:19:38 2013 -0700 Committer: James Peach <[email protected]> Committed: Fri May 10 20:34:54 2013 -0700 ---------------------------------------------------------------------- CHANGES | 2 + proxy/http/HttpTransact.cc | 3 +- proxy/http/remap/RemapPlugins.cc | 205 +++++---------------------------- proxy/http/remap/UrlMapping.h | 11 ++- proxy/http/remap/UrlRewrite.cc | 15 +-- proxy/http/remap/UrlRewrite.h | 3 +- 6 files changed, 48 insertions(+), 191 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 05d69a8..a7ad36c 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ Changes with Apache Traffic Server 3.3.3 + *) [TS-1889] Refactor remap plugin request URL handling. + *) [TS-1887] Make diagnostic location logging more succinct. *) [TS-1884] Remove deprecated IPv4-only NetProcessor API. http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/proxy/http/HttpTransact.cc ---------------------------------------------------------------------- diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 61fcf77..ddfdfa9 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -3484,8 +3484,9 @@ HttpTransact::handle_response_from_server(State* s) // plugin call s->server_info.state = s->current.state; - if (s->fp_tsremap_os_response) + if (s->fp_tsremap_os_response) { s->fp_tsremap_os_response(s->remap_plugin_instance, reinterpret_cast<TSHttpTxn>(s->state_machine), s->current.state); + } switch (s->current.state) { case CONNECTION_ALIVE: http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/proxy/http/remap/RemapPlugins.cc ---------------------------------------------------------------------- diff --git a/proxy/http/remap/RemapPlugins.cc b/proxy/http/remap/RemapPlugins.cc index cea8c23..d299fa7 100644 --- a/proxy/http/remap/RemapPlugins.cc +++ b/proxy/http/remap/RemapPlugins.cc @@ -31,8 +31,8 @@ RemapPlugins::run_plugin(remap_plugin_info* plugin) TSRemapStatus plugin_retcode; TSRemapRequestInfo rri; url_mapping *map = _s->url_map.getMapping(); - URL *map_from = &(map->fromURL); - URL *map_to = _s->url_map.getToURL(); + URL * map_from = _s->url_map.getFromURL(); + URL * map_to = _s->url_map.getToURL(); // This is the equivalent of TSHttpTxnClientReqGet(), which every remap plugin would // have to call. @@ -96,201 +96,52 @@ RemapPlugins::run_plugin(remap_plugin_info* plugin) int RemapPlugins::run_single_remap() { - // I should patent this - Debug("url_rewrite", "Running single remap rule for the %d%s time", _cur, _cur == 1 ? "st" : _cur == 2 ? "nd" : _cur == 3 ? "rd" : "th"); - remap_plugin_info *plugin = NULL; - TSRemapStatus plugin_retcode = TSREMAP_NO_REMAP; + url_mapping * map = _s->url_map.getMapping(); + remap_plugin_info * plugin = map->get_plugin(_cur); //get the nth plugin in our list of plugins + TSRemapStatus plugin_retcode = TSREMAP_NO_REMAP; - const char *requestPath; - int requestPathLen; - url_mapping *map = _s->url_map.getMapping(); - URL *map_from = &(map->fromURL); - URL *map_to = _s->url_map.getToURL(); - - int redirect_host_len; - - // Debugging vars - bool debug_on = false; - int retcode = 0; // 0 - no redirect, !=0 - redirected - - debug_on = is_debug_tag_set("url_rewrite"); - - if (_request_header) - plugin = map->get_plugin(_cur); //get the nth plugin in our list of plugins - - if (plugin) { - Debug("url_rewrite", "Remapping rule id: %d matched; running it now", map->map_id); - plugin_retcode = run_plugin(plugin); - } else if (_cur > 0) { - _cur++; + if (unlikely(plugin == NULL)) { Debug("url_rewrite", "There wasn't a plugin available for us to run. Completing all remap processing immediately"); return 1; } - if (_s->remap_redirect) //if redirect was set, we need to use that. - return 1; - - // skip the !plugin_modified_* stuff if we are on our 2nd plugin (or greater) and there's no more plugins - if (_cur > 0 && (_cur + 1) >= map->_plugin_count) - goto done; - - if (TSREMAP_NO_REMAP == plugin_retcode || TSREMAP_NO_REMAP_STOP == plugin_retcode) { - if (_cur > 0 ) { - //plugin didn't do anything for us, but maybe another will down the chain so lets assume there is something more for us to process - ++_cur; - Debug("url_rewrite", "Plugin didn't change anything, but we'll try the next one right now"); - return 0; - } - - Debug("url_rewrite", "plugin did not change host, port or path, copying from mapping rule"); - - int fromPathLen; - const char *toHost; - const char *toPath; - int toPathLen; - int toHostLen; - - toHost = map_to->host_get(&toHostLen); - toPath = map_to->path_get(&toPathLen); - - _request_url->host_set(toHost, toHostLen); - - int to_port = map_to->port_get_raw(); + Debug("url_rewrite", "Running single remap rule id %d for the %d%s time", + map->map_id, _cur, _cur == 1 ? "st" : _cur == 2 ? "nd" : _cur == 3 ? "rd" : "th"); - if (to_port != _request_url->port_get_raw()) - _request_url->port_set(to_port); + plugin_retcode = run_plugin(plugin); + _cur++; - int to_scheme_len, from_scheme_len; - const char *to_scheme = map_to->scheme_get(&to_scheme_len); - - if (to_scheme != map_from->scheme_get(&from_scheme_len)) - _request_url->scheme_set(to_scheme, to_scheme_len); - - map_from->path_get(&fromPathLen); - requestPath = _request_url->path_get(&requestPathLen); - // Extra byte is potentially needed for prefix path '/'. - // Added an extra 3 so that TS wouldn't crash in the field. - // Allocate a large buffer to avoid problems. - char newPathTmp[2048]; - char *newPath; - char *newPathAlloc = NULL; - unsigned int newPathLen = 0; - unsigned int newPathLenNeed = (requestPathLen - fromPathLen) + toPathLen + 8; // 3 + some padding - - if (newPathLenNeed > sizeof(newPathTmp)) { - newPath = (newPathAlloc = (char *)ats_malloc(newPathLenNeed)); - if (debug_on) { - memset(newPath, 0, newPathLenNeed); - } - } else { - newPath = &newPathTmp[0]; - if (debug_on) { - memset(newPath, 0, sizeof(newPathTmp)); - } - } - - *newPath = 0; - - // Purify load run with QT in a reverse proxy indicated - // a UMR/ABR/MSE in the line where we do a *newPath == '/' and the ink_strlcpy - // that follows it. The problem occurs if - // requestPathLen,fromPathLen,toPathLen are all 0; in this case, we never - // initialize newPath, but still de-ref it in *newPath == '/' comparison. - // The memset fixes that problem. - if (toPath) { - memcpy(newPath, toPath, toPathLen); - newPathLen += toPathLen; - } - // We might need to insert a trailing slash in the new portion of the path - // if more will be added and none is present and one will be needed. - if (!fromPathLen && requestPathLen && toPathLen && *(newPath + newPathLen - 1) != '/') { - *(newPath + newPathLen) = '/'; - newPathLen++; - } - - if (requestPath) { - //avoid adding another trailing slash if the requestPath already had one and so does the toPath - if (requestPathLen < fromPathLen) { - if (toPathLen && requestPath[requestPathLen - 1] == '/' && toPath[toPathLen - 1] == '/') { - fromPathLen++; - } - } else { - if (toPathLen && requestPath[fromPathLen] == '/' && toPath[toPathLen - 1] == '/') { - fromPathLen++; - } - } - // copy the end of the path past what has been mapped - if ((requestPathLen - fromPathLen) > 0) { - memcpy(newPath + newPathLen, requestPath + fromPathLen, requestPathLen - fromPathLen); - newPathLen += (requestPathLen - fromPathLen); - } - } - // We need to remove the leading slash in newPath if one is - // present. - if (*newPath == '/') { - memmove(newPath, newPath + 1, --newPathLen); - } + // If the plugin redirected, we need to end the remap chain now. + if (_s->remap_redirect) { + return 1; + } - _request_url->path_set(newPath, newPathLen); - - // TODO: This is horribly wrong and broken, when can this trigger??? Check - // above, we already return on _s->remap_redirect ... XXX. - if (map->homePageRedirect && fromPathLen == requestPathLen && _s->remap_redirect) { - URL redirect_url; - - redirect_url.create(NULL); - redirect_url.copy(_request_url); - - ink_assert(fromPathLen > 0); - - // Extra byte for trailing '/' in redirect - if (newPathLen > 0 && newPath[newPathLen - 1] != '/') { - newPath[newPathLen] = '/'; - newPath[++newPathLen] = '\0'; - redirect_url.path_set(newPath, newPathLen); - } - // If we have host header information, - // put it back into redirect URL - // - if (_hh_ptr != NULL) { - redirect_url.host_set(_hh_ptr->request_host, _hh_ptr->host_len); - if (redirect_url.port_get() != _hh_ptr->request_port) { - redirect_url.port_set(_hh_ptr->request_port); - } - } - // If request came in without a host, send back - // the redirect with the name the proxy is known by - if (redirect_url.host_get(&redirect_host_len) == NULL) - redirect_url.host_set(rewrite_table->ts_name, strlen(rewrite_table->ts_name)); - - if ((_s->remap_redirect = redirect_url.string_get(NULL)) != NULL) - retcode = strlen(_s->remap_redirect); - Debug("url_rewrite", "Redirected %.*s to %.*s", requestPathLen, requestPath, retcode, _s->remap_redirect); - redirect_url.destroy(); + 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); } - - ats_free(newPathAlloc); } -done: if (_cur > MAX_REMAP_PLUGIN_CHAIN) { - Error("Called run_single_remap more than 10 times. Stopping this remapping insanity now"); - Debug("url_rewrite", "Called run_single_remap more than 10 times. Stopping this remapping insanity now"); + Error("called %s more than %u times; stopping this remap insanity now", __func__, MAX_REMAP_PLUGIN_CHAIN); return 1; } - if (++_cur >= map->_plugin_count) { - //normally, we would callback into this function but we dont have anything more to do! + if (_cur >= map->_plugin_count) { + // Normally, we would callback into this function but we dont have anything more to do! Debug("url_rewrite", "We completed all remap plugins for this rule"); return 1; - } else { - Debug("url_rewrite", "Completed single remap. Attempting another via immediate callback"); - return 0; } - return 1; - ink_assert(!"not reached"); + Debug("url_rewrite", "Completed single remap. Attempting another via immediate callback"); + return 0; } int http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/proxy/http/remap/UrlMapping.h ---------------------------------------------------------------------- diff --git a/proxy/http/remap/UrlMapping.h b/proxy/http/remap/UrlMapping.h index cb3b0fc..4d1340c 100644 --- a/proxy/http/remap/UrlMapping.h +++ b/proxy/http/remap/UrlMapping.h @@ -124,20 +124,25 @@ private: }; +/** + * UrlMappingContainer wraps a url_mapping object and allows a caller to rewrite the target URL. + * This is used while evaluating remap rules. +**/ class UrlMappingContainer { public: UrlMappingContainer() : _mapping(NULL), _toURLPtr(NULL), _heap(NULL) { } - UrlMappingContainer(HdrHeap *heap) + explicit UrlMappingContainer(HdrHeap *heap) : _mapping(NULL), _toURLPtr(NULL), _heap(heap) { } - ~UrlMappingContainer() { deleteToURL(); } - URL *getToURL() const { return _toURLPtr; }; + URL * getToURL() const { return _toURLPtr; }; + URL * getFromURL() const { return _mapping ? &(_mapping->fromURL) : NULL; }; + url_mapping *getMapping() const { return _mapping; }; void set(url_mapping *m) { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/proxy/http/remap/UrlRewrite.cc ---------------------------------------------------------------------- diff --git a/proxy/http/remap/UrlRewrite.cc b/proxy/http/remap/UrlRewrite.cc index a87cfb6..37f7c98 100644 --- a/proxy/http/remap/UrlRewrite.cc +++ b/proxy/http/remap/UrlRewrite.cc @@ -117,7 +117,7 @@ check_remap_option(char *argv[], int argc, unsigned long findmode = 0, int *_ret real accessing a directory (albeit a virtual one). */ -void +static void SetHomePageRedirectFlag(url_mapping *new_mapping, URL &new_to_url) { int fromLen, toLen; @@ -708,20 +708,17 @@ UrlRewrite::_tableLookup(InkHashTable *h_table, URL *request_url, return um; } - // This is only used for redirects and reverse rules, and the homepageredirect flag // can never be set. The end result is that request_url is modified per remap container. void -UrlRewrite::_doRemap(UrlMappingContainer &mapping_container, URL *request_url) +url_rewrite_remap_request(const UrlMappingContainer& mapping_container, URL *request_url) { const char *requestPath; int requestPathLen; - - url_mapping *mapPtr = mapping_container.getMapping(); - URL *map_from = &mapPtr->fromURL; int fromPathLen; URL *map_to = mapping_container.getToURL(); + URL *map_from = mapping_container.getFromURL(); const char *toHost; const char *toPath; const char *toScheme; @@ -736,7 +733,7 @@ UrlRewrite::_doRemap(UrlMappingContainer &mapping_container, URL *request_url) toPath = map_to->path_get(&toPathLen); toScheme = map_to->scheme_get(&toSchemeLen); - Debug("url_rewrite", "_doRemap(): Remapping rule id: %d matched", mapPtr->map_id); + Debug("url_rewrite", "%s: Remapping rule id: %d matched", __func__, mapping_container.getMapping()->map_id); request_url->host_set(toHost, toHostLen); request_url->port_set(map_to->port_get_raw()); @@ -835,7 +832,7 @@ UrlRewrite::ReverseMap(HTTPHdr *response_header) if (reverseMappingLookup(&location_url, location_url.port_get(), host, host_len, reverse_mapping)) { if (i == 0) remap_found = true; - _doRemap(reverse_mapping, &location_url); + url_rewrite_remap_request(reverse_mapping, &location_url); new_loc_hdr = location_url.string_get_ref(&new_loc_length); response_header->value_set(url_headers[i].field, url_headers[i].len, new_loc_hdr, new_loc_length); } @@ -990,7 +987,7 @@ UrlRewrite::Remap_redirect(HTTPHdr *request_header, URL *redirect_url) redirect_url->copy(request_url); // Perform the actual URL rewrite - _doRemap(redirect_mapping, redirect_url); + url_rewrite_remap_request(redirect_mapping, redirect_url); return mappingType; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/ed93dcbf/proxy/http/remap/UrlRewrite.h ---------------------------------------------------------------------- diff --git a/proxy/http/remap/UrlRewrite.h b/proxy/http/remap/UrlRewrite.h index 4a89f37..e476dff 100644 --- a/proxy/http/remap/UrlRewrite.h +++ b/proxy/http/remap/UrlRewrite.h @@ -191,7 +191,6 @@ public: private: bool _valid; - void _doRemap(UrlMappingContainer &mapping_container, URL *request_url); bool _mappingLookup(MappingsStore &mappings, URL *request_url, int request_port, const char *request_host, int request_host_len, UrlMappingContainer &mapping_container); url_mapping *_tableLookup(InkHashTable * h_table, URL * request_url, int request_port, char *request_host, @@ -208,4 +207,6 @@ private: bool is_cur_mapping_regex, int &count); }; +void url_rewrite_remap_request(const UrlMappingContainer& mapping_container, URL * request_url); + #endif
