You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/03/30 21:56:42 UTC
[GitHub] [trafficserver] pbchou opened a new pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
pbchou opened a new pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657
This commit adds the ability to disable the next hop is self detection
based on the IP address and port. In addition, it allows a configurable
threshold for Via string proxy cycle detection so that it may
be increased from not allowing any cycles to a configurable number of cycles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on a change in pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
jrushford commented on a change in pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#discussion_r610888735
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -6572,16 +6585,29 @@ HttpTransact::will_this_request_self_loop(State *s)
int via_len;
const char *via_string = via_field->value_get(&via_len);
- if (via_string && ptr_len_str(via_string, via_len, uuid)) {
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- TxnDebug("http_transact", "Incoming via: %.*s has (%s[%s] (%s))", via_len, via_string, s->http_config_param->proxy_hostname,
- uuid, s->http_config_param->proxy_request_via_string);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Multi-Hop Cycle Detected", "request#cycle_detected");
- return true;
+ if (via_string) {
+ char *current = (char *)via_string;
+ int len = via_len;
+ TxnDebug("http_transact", "Incoming via: \"%.*s\" --has-- (%s[%s] (%s))", via_len, via_string,
+ s->http_config_param->proxy_hostname, uuid, s->http_config_param->proxy_request_via_string);
+ while (char *result = (char *)ptr_len_str(current, len, uuid)) {
Review comment:
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
pbchou commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-810605061
@jrushford this PR is intended to address Issue #7076. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#discussion_r631930016
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -6536,52 +6536,77 @@ HttpTransact::process_quick_http_filter(State *s, int method)
bool
HttpTransact::will_this_request_self_loop(State *s)
{
+ // The self-loop detection for this ATS node will allow up to max_proxy_cycles
+ // (each time it sees it returns to itself it is one cycle) before declaring a self-looping condition detected.
+ // If max_proxy_cycles is > 0 then then next-hop is disabled since --
+ // * if first cycle then it is alright okay
+ // * if not first cycle then will be detected by via string checking the next time that
+ // it enters the node
+ int max_proxy_cycles = s->txn_conf->max_proxy_cycles;
+
////////////////////////////////////////
// check if we are about to self loop //
////////////////////////////////////////
if (s->dns_info.lookup_success) {
- in_port_t dst_port = s->hdr_info.client_request.url_get()->port_get(); // going to this port.
- in_port_t local_port = s->client_info.dst_addr.host_order_port(); // already connected proxy port.
- // It's a loop if connecting to the same port as it already connected to the proxy and
- // it's a proxy address or the same address it already connected to.
- if (dst_port == local_port && (ats_ip_addr_eq(s->host_db_info.ip(), &Machine::instance()->ip.sa) ||
- ats_ip_addr_eq(s->host_db_info.ip(), s->client_info.dst_addr))) {
- switch (s->dns_info.looking_up) {
- case ORIGIN_SERVER:
- TxnDebug("http_transact", "host ip and port same as local ip and port - bailing");
- break;
- case PARENT_PROXY:
- TxnDebug("http_transact", "parent proxy ip and port same as local ip and port - bailing");
- break;
- default:
- TxnDebug("http_transact", "unknown's ip and port same as local ip and port - bailing");
- break;
+ TxnDebug("http_transact", "max_proxy_cycles = %d", max_proxy_cycles);
+ if (max_proxy_cycles == 0) {
+ in_port_t dst_port = s->hdr_info.client_request.url_get()->port_get(); // going to this port.
+ in_port_t local_port = s->client_info.dst_addr.host_order_port(); // already connected proxy port.
+ // It's a loop if connecting to the same port as it already connected to the proxy and
+ // it's a proxy address or the same address it already connected to.
+ TxnDebug("http_transact", "dst_port = %d local_port = %d", dst_port, local_port);
+ if (dst_port == local_port && (ats_ip_addr_eq(s->host_db_info.ip(), &Machine::instance()->ip.sa) ||
+ ats_ip_addr_eq(s->host_db_info.ip(), s->client_info.dst_addr))) {
+ switch (s->dns_info.looking_up) {
+ case ORIGIN_SERVER:
+ TxnDebug("http_transact", "host ip and port same as local ip and port - bailing");
+ break;
+ case PARENT_PROXY:
+ TxnDebug("http_transact", "parent proxy ip and port same as local ip and port - bailing");
+ break;
+ default:
+ TxnDebug("http_transact", "unknown's ip and port same as local ip and port - bailing");
+ break;
+ }
+ SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
+ build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Cycle Detected", "request#cycle_detected");
+ return true;
}
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Cycle Detected", "request#cycle_detected");
- return true;
}
// Now check for a loop using the Via string.
- const char *uuid = Machine::instance()->uuid.getString();
+ int count = 0;
MIMEField *via_field = s->hdr_info.client_request.field_find(MIME_FIELD_VIA, MIME_LEN_VIA);
+ std::string_view uuid{Machine::instance()->uuid.getString()};
while (via_field) {
// No need to waste cycles comma separating the via values since we want to do a match anywhere in the
// in the string. We can just loop over the dup hdr fields
int via_len;
const char *via_string = via_field->value_get(&via_len);
- if (via_string && ptr_len_str(via_string, via_len, uuid)) {
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- TxnDebug("http_transact", "Incoming via: %.*s has (%s[%s] (%s))", via_len, via_string, s->http_config_param->proxy_hostname,
- uuid, s->http_config_param->proxy_request_via_string);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Multi-Hop Cycle Detected", "request#cycle_detected");
- return true;
+ if ((count <= max_proxy_cycles) && via_string) {
+ std::string_view current{via_field->value_get()};
Review comment:
You could do
```
std::string_view current{via_string};
```
or alternatively
```
std::string_view current{via_field-value_get()};
if ( (count <= max_proxy_cycles) && ! current.empty()) {
// ....
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode merged pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode merged pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-840222545
@pbchou Sorry, I'll review it tomorrow, been busy. Maybe @SolidWallOfCode could have a look too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on a change in pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
pbchou commented on a change in pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#discussion_r608222778
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -6572,16 +6585,29 @@ HttpTransact::will_this_request_self_loop(State *s)
int via_len;
const char *via_string = via_field->value_get(&via_len);
- if (via_string && ptr_len_str(via_string, via_len, uuid)) {
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- TxnDebug("http_transact", "Incoming via: %.*s has (%s[%s] (%s))", via_len, via_string, s->http_config_param->proxy_hostname,
- uuid, s->http_config_param->proxy_request_via_string);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Multi-Hop Cycle Detected", "request#cycle_detected");
- return true;
+ if (via_string) {
+ char *current = (char *)via_string;
+ int len = via_len;
+ TxnDebug("http_transact", "Incoming via: \"%.*s\" --has-- (%s[%s] (%s))", via_len, via_string,
+ s->http_config_param->proxy_hostname, uuid, s->http_config_param->proxy_request_via_string);
+ while (char *result = (char *)ptr_len_str(current, len, uuid)) {
Review comment:
@SolidWallOfCode -- I converted to using C++17 string_view per your comment. Please review as I only read the tutorial for using this class just now. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-815858198
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#discussion_r607364887
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -6572,16 +6585,29 @@ HttpTransact::will_this_request_self_loop(State *s)
int via_len;
const char *via_string = via_field->value_get(&via_len);
- if (via_string && ptr_len_str(via_string, via_len, uuid)) {
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- TxnDebug("http_transact", "Incoming via: %.*s has (%s[%s] (%s))", via_len, via_string, s->http_config_param->proxy_hostname,
- uuid, s->http_config_param->proxy_request_via_string);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Multi-Hop Cycle Detected", "request#cycle_detected");
- return true;
+ if (via_string) {
+ char *current = (char *)via_string;
+ int len = via_len;
+ TxnDebug("http_transact", "Incoming via: \"%.*s\" --has-- (%s[%s] (%s))", via_len, via_string,
+ s->http_config_param->proxy_hostname, uuid, s->http_config_param->proxy_request_via_string);
+ while (char *result = (char *)ptr_len_str(current, len, uuid)) {
Review comment:
Maybe more like this, for switching to modern C++?
```
std::string_view current { via_field->value_get() };
std::string_view uuid { Machine::instance()->uuid};
std::string_view::size offset;
while (count <= max_proxy_cycles && std::string_view::npos != (offset = current.find(uuid))) {
current.remove_prefix(offset);
++count;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on a change in pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
pbchou commented on a change in pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#discussion_r617037696
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -6572,16 +6585,29 @@ HttpTransact::will_this_request_self_loop(State *s)
int via_len;
const char *via_string = via_field->value_get(&via_len);
- if (via_string && ptr_len_str(via_string, via_len, uuid)) {
- SET_VIA_STRING(VIA_ERROR_TYPE, VIA_ERROR_LOOP_DETECTED);
- TxnDebug("http_transact", "Incoming via: %.*s has (%s[%s] (%s))", via_len, via_string, s->http_config_param->proxy_hostname,
- uuid, s->http_config_param->proxy_request_via_string);
- build_error_response(s, HTTP_STATUS_BAD_REQUEST, "Multi-Hop Cycle Detected", "request#cycle_detected");
- return true;
+ if (via_string) {
+ char *current = (char *)via_string;
+ int len = via_len;
+ TxnDebug("http_transact", "Incoming via: \"%.*s\" --has-- (%s[%s] (%s))", via_len, via_string,
+ s->http_config_param->proxy_hostname, uuid, s->http_config_param->proxy_request_via_string);
+ while (char *result = (char *)ptr_len_str(current, len, uuid)) {
Review comment:
@SolidWallOfCode -- Can you please review? I don't think @jrushford will approve and merge without your input. I pushed another commit to do a static_cast<int> on the string_view::size_type. I did not get compiler warning when compiling as a branch of master, but when I back-ported to our internal 9.0.1, it did complain about int vs long unsigned int. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jrushford commented on pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
jrushford commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-814154548
This passes all unit and regression tests. I did some functional testing using the configs described in #7076 and all works as expected by setting proxy.config.http.max_proxy_cycles in both records.config and a header_rewrite config. I'll approve pending approval by @SolidWallOfCode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
pbchou commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-839139033
@jrushford -- Do I need to do anything to help this along, or are we just waiting for more approvals?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop edited a comment on pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
zwoop edited a comment on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-848876704
Moving out to 9.2.x for now, since we're getting closer to making a 9.1.0 release, this is too big and a new feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
pbchou commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-815150266
@jrushford -- Can you kick off the checks again? One check failed, but it does not appear to be anything I touched looking at the Jenkins CI output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] pbchou commented on pull request #7657: Add proxy.config.http.max_proxy_cycles (#7076)
Posted by GitBox <gi...@apache.org>.
pbchou commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-818954361
@zwoop -- Can you take a look at the failing clang-analyzer? It says it is in the health check module, which we did not touch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7657: Add proxy.config.http.max_proxy_cycles
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7657:
URL: https://github.com/apache/trafficserver/pull/7657#issuecomment-848876704
Moving out to 9.2.x for now, since we're getting closer to making a 9.1.0 release, this is too big.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org