From 0860d79cb850702dfed4eaa6a0f3d06776b2cdbb Mon Sep 17 00:00:00 2001 From: John Rushford Date: Wed, 22 Jan 2020 16:45:48 +0000 Subject: [PATCH] Fixes an issue where NextHopSelectionStrategy did not implement an available nexthop or parent check when proxy.config.http.no_dns_just_forward_to_parent is enabled. --- proxy/http/HttpTransact.cc | 16 +++++++++++++- proxy/http/remap/NextHopSelectionStrategy.cc | 21 ++++++++++++++++--- proxy/http/remap/NextHopSelectionStrategy.h | 1 + .../unit-tests/test_NextHopRoundRobin.cc | 21 ++++++++++++++++++- 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 1ae438ca355..ffd58146182 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -243,6 +243,20 @@ markParentUp(HttpTransact::State *s) } } +// wrapper to choose between a remap next hop strategy or use parent.config +// remap next hop strategy is preferred +inline static bool +parentExists(HttpTransact::State *s) +{ + url_mapping *mp = s->url_map.getMapping(); + if (mp && mp->strategy) { + return mp->strategy->nextHopExists(s->state_machine->sm_id); + } else if (s->parent_params) { + return s->parent_params->parentExists(&s->request_data); + } + return false; +} + // wrapper to choose between a remap next hop strategy or use parent.config // remap next hop strategy is preferred inline static void @@ -1444,7 +1458,7 @@ HttpTransact::HandleRequest(State *s) ats_ip_copy(&s->request_data.dest_ip, &addr); } - if (s->parent_params->parentExists(&s->request_data)) { + if (parentExists(s)) { // If the proxy is behind and firewall and there is no // DNS service available, we just want to forward the request // the parent proxy. In this case, we never find out the diff --git a/proxy/http/remap/NextHopSelectionStrategy.cc b/proxy/http/remap/NextHopSelectionStrategy.cc index a101dfe818e..0507350c97e 100644 --- a/proxy/http/remap/NextHopSelectionStrategy.cc +++ b/proxy/http/remap/NextHopSelectionStrategy.cc @@ -251,15 +251,15 @@ NextHopSelectionStrategy::markNextHopDown(const uint64_t sm_id, ParentResult &re } new_fail_count = old_count + 1; } // end of lock_guard - NH_Debug("parent_select", "[%" PRIu64 "] Parent fail count increased to %d for %s:%d", sm_id, new_fail_count, - h->hostname.c_str(), h->getPort(scheme)); + NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Parent fail count increased to %d for %s:%d", sm_id, new_fail_count, h->hostname.c_str(), + h->getPort(scheme)); } if (new_fail_count >= fail_threshold) { h->set_unavailable(); NH_Note("[%" PRIu64 "] Failure threshold met failcount:%d >= threshold:%" PRIu64 ", http parent proxy %s:%d marked down", sm_id, new_fail_count, fail_threshold, h->hostname.c_str(), h->getPort(scheme)); - NH_Debug("parent_select", "[%" PRIu64 "] NextHop %s:%d marked unavailable, h->available=%s", sm_id, h->hostname.c_str(), + NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] NextHop %s:%d marked unavailable, h->available=%s", sm_id, h->hostname.c_str(), h->getPort(scheme), (h->available) ? "true" : "false"); } } @@ -289,6 +289,21 @@ NextHopSelectionStrategy::markNextHopUp(const uint64_t sm_id, ParentResult &resu } } +bool +NextHopSelectionStrategy::nextHopExists(const uint64_t sm_id) +{ + for (uint32_t gg = 0; gg < groups; gg++) { + for (uint32_t hh = 0; hh < host_groups[gg].size(); hh++) { + HostRecord *p = host_groups[gg][hh].get(); + if (p->available) { + NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] found available next hop %s", sm_id, p->hostname.c_str()); + return true; + } + } + } + return false; +} + namespace YAML { template <> struct convert { diff --git a/proxy/http/remap/NextHopSelectionStrategy.h b/proxy/http/remap/NextHopSelectionStrategy.h index 200c94cd8ca..77a8b11cea6 100644 --- a/proxy/http/remap/NextHopSelectionStrategy.h +++ b/proxy/http/remap/NextHopSelectionStrategy.h @@ -195,6 +195,7 @@ class NextHopSelectionStrategy void markNextHopDown(const uint64_t sm_id, ParentResult &result, const uint64_t fail_threshold, const uint64_t retry_time, time_t now = 0); void markNextHopUp(const uint64_t sm_id, ParentResult &result); + bool nextHopExists(const uint64_t sm_id); std::string strategy_name; bool go_direct = true; diff --git a/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc b/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc index df47f3c4aff..05c260cc5de 100644 --- a/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc +++ b/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc @@ -122,6 +122,9 @@ SCENARIO("Testing NextHopRoundRobin class, using policy 'rr-strict'", "[NextHopR strategy->findNextHop(10012, result, rdata, fail_threshold, retry_time); CHECK(result.result == ParentResultType::PARENT_DIRECT); + // check that nextHopExists() returns false when all parents are down. + CHECK(strategy->nextHopExists(10012) == false); + // change the request time to trigger a retry. time_t now = (time(nullptr) + 5); @@ -224,7 +227,7 @@ SCENARIO("Testing NextHopRoundRobin class, using policy 'rr-ip'", "[NextHopRound } } - WHEN("using the 'rr-stric' strategy.") + WHEN("using the 'rr-strict' strategy.") { uint64_t fail_threshold = 1; uint64_t retry_time = 1; @@ -235,23 +238,39 @@ SCENARIO("Testing NextHopRoundRobin class, using policy 'rr-ip'", "[NextHopRound REQUIRE(nhf.strategies_loaded == true); REQUIRE(strategy != nullptr); + // call and test parentExists(), this call should not affect + // findNextHop() round robin strict results + CHECK(strategy->nextHopExists(29000) == true); + // first request. memcpy(&rdata.client_ip, &sa1, sizeof(sa1)); ParentResult result; strategy->findNextHop(30000, result, rdata, fail_threshold, retry_time); CHECK(strcmp(result.hostname, "p4.foo.com") == 0); + // call and test parentExists(), this call should not affect + // findNextHop round robin strict results. + CHECK(strategy->nextHopExists(29000) == true); + // second request. memcpy(&rdata.client_ip, &sa2, sizeof(sa2)); result.reset(); strategy->findNextHop(30001, result, rdata, fail_threshold, retry_time); CHECK(strcmp(result.hostname, "p3.foo.com") == 0); + // call and test parentExists(), this call should not affect + // findNextHop() round robin strict results + CHECK(strategy->nextHopExists(29000) == true); + // third request with same client ip, result should still be p3 result.reset(); strategy->findNextHop(30002, result, rdata, fail_threshold, retry_time); CHECK(strcmp(result.hostname, "p3.foo.com") == 0); + // call and test parentExists(), this call should not affect + // findNextHop() round robin strict results. + CHECK(strategy->nextHopExists(29000) == true); + // fourth request with same client ip and same result indicating a failure should result in p4 // being selected. strategy->findNextHop(30003, result, rdata, fail_threshold, retry_time);