trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a..@apache.org
Subject git commit: TS-1424: Fix KeepAlive 4-tuple collision for transparent proxy
Date Sat, 24 Aug 2013 03:39:26 GMT
Updated Branches:
  refs/heads/master 14c9cf2c8 -> f63b27d52


TS-1424: Fix KeepAlive 4-tuple collision for transparent proxy


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/f63b27d5
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/f63b27d5
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/f63b27d5

Branch: refs/heads/master
Commit: f63b27d5220c95cd95764fb0ca0cb4e648b2f055
Parents: 14c9cf2
Author: Alan M. Carroll <amc@network-geographics.com>
Authored: Fri Aug 23 10:44:08 2013 -0500
Committer: Alan M. Carroll <amc@network-geographics.com>
Committed: Fri Aug 23 22:35:51 2013 -0500

----------------------------------------------------------------------
 proxy/http/HttpSM.cc       | 43 +++++++++++++++++++++++++++++++++--------
 proxy/http/HttpTransact.cc | 33 +++++++++++++++++--------------
 proxy/http/HttpTransact.h  | 20 ++++++++++++-------
 3 files changed, 66 insertions(+), 30 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f63b27d5/proxy/http/HttpSM.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 92bd92c..ab120db 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -1712,7 +1712,34 @@ HttpSM::state_http_server_open(int event, void *data)
   case VC_EVENT_ERROR:
   case NET_EVENT_OPEN_FAILED:
     t_state.current.state = HttpTransact::CONNECTION_ERROR;
-    call_transact_and_set_next_state(HttpTransact::HandleResponse);
+    // save the errno from the connect fail for future use (passed as negative value, flip
back)
+    t_state.current.server->set_connect_fail(event == NET_EVENT_OPEN_FAILED ? -reinterpret_cast<intptr_t>(data)
: EREMOTEIO);
+
+    /* If we get this error, then we simply can't bind to the 4-tuple to make the connection.
 There's no hope of
+       retries succeeding in the near future. The best option is to just shut down the connection
without further
+       comment. The only known cause for this is outbound transparency combined with use
client target address / source
+       port, as noted in TS-1424. If the keep alives desync the current connection can be
attempting to rebind the 4
+       tuple simultaneously with the shut down of an existing connection. Dropping the client
side will cause it to pick
+       a new source port and recover from this issue.
+    */
+    if (EADDRNOTAVAIL == t_state.current.server->connect_result) {
+      if (is_debug_tag_set("http_tproxy")) {
+        ip_port_text_buffer ip_c, ip_s;
+        Debug("http_tproxy", "Force close of client connect (%s->%s) due to EADDRNOTAVAIL
[%" PRId64 "]"
+              , ats_ip_nptop(&t_state.client_info.addr.sa, ip_c, sizeof ip_c)
+              , ats_ip_nptop(&t_state.server_info.addr.sa, ip_s, sizeof ip_s)
+              , sm_id
+          );
+      }
+      t_state.client_info.keep_alive = HTTP_NO_KEEPALIVE; // part of the problem, clear it.
+      if (ua_entry && ua_entry->vc) {
+        vc_table.cleanup_entry(ua_entry);
+        ua_entry = NULL;
+        ua_session = NULL;
+      }
+    } else {
+      call_transact_and_set_next_state(HttpTransact::HandleResponse);
+    }
     return 0;
   case CONGESTION_EVENT_CONGESTED_ON_F:
     t_state.current.state = HttpTransact::CONGEST_CONTROL_CONGESTED_ON_F;
@@ -2048,7 +2075,6 @@ HttpSM::process_hostdb_info(HostDBInfo * r)
       // current time when calling select_best_http().
       HostDBRoundRobin *rr = r->rr();
       ret = rr->select_best_http(&t_state.client_info.addr.sa, now, (int) t_state.txn_conf->down_server_timeout);
-      t_state.dns_info.round_robin = true;
 
       // set the srv target`s last_failure
       if (t_state.dns_info.srv_lookup_success) {
@@ -2068,7 +2094,6 @@ HttpSM::process_hostdb_info(HostDBInfo * r)
       }
     } else {
       ret = r;
-      t_state.dns_info.round_robin = false;
     }
     if (ret) {
       t_state.host_db_info = *ret;
@@ -2079,9 +2104,9 @@ HttpSM::process_hostdb_info(HostDBInfo * r)
     DebugSM("http", "[%" PRId64 "] DNS lookup failed for '%s'", sm_id, t_state.dns_info.lookup_name);
 
     t_state.dns_info.lookup_success = false;
-    t_state.dns_info.round_robin = false;
     t_state.host_db_info.app.allotment.application1 = 0;
     t_state.host_db_info.app.allotment.application2 = 0;
+    ink_assert(!t_state.host_db_info.round_robin);
   }
 
   milestones.dns_lookup_end = ink_get_hrtime();
@@ -2923,10 +2948,12 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer * p)
     p->read_vio = NULL;
     /* TS-1424: if we're outbound transparent and using the client
        source port for the outbound connection we must effectively
-       propagate server closes back to the client.
+       propagate server closes back to the client. Part of that is
+       disabling KeepAlive if the server closes.
     */
-    if (ua_session && ua_session->f_outbound_transparent && t_state.http_config_param->use_client_source_port)
+    if (ua_session && ua_session->f_outbound_transparent && t_state.http_config_param->use_client_source_port)
{
       t_state.client_info.keep_alive = HTTP_NO_KEEPALIVE;
+    }
   } else {
     server_session->attach_hostname(t_state.current.server->name);
     server_session->server_trans_stat--;
@@ -3991,7 +4018,7 @@ HttpSM::do_hostdb_update_if_necessary()
     t_state.updated_server_version = HostDBApplicationInfo::HTTP_VERSION_UNDEFINED;
   }
   // Check to see if we need to report or clear a connection failure
-  if (t_state.current.server->connect_failure) {
+  if (t_state.current.server->had_connect_fail()) {
     issue_update |= 1;
     mark_host_failure(&t_state.host_db_info, t_state.client_request_time);
   } else {
@@ -4887,7 +4914,7 @@ HttpSM::mark_server_down_on_client_abort()
         wait = 0;
       }
       if (ink_hrtime_to_sec(wait) > t_state.txn_conf->client_abort_threshold) {
-        t_state.current.server->connect_failure = true;
+        t_state.current.server->set_connect_fail(ETIMEDOUT);
         do_hostdb_update_if_necessary();
       }
     }

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f63b27d5/proxy/http/HttpTransact.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index ac074ed..e39e7e9 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -1408,7 +1408,7 @@ HttpTransact::PPDNSLookup(State* s)
     // lookup succeeded, open connection to p.p.
     ats_ip_copy(&s->parent_info.addr, s->host_db_info.ip());
     get_ka_info_from_host_db(s, &s->parent_info, &s->client_info, &s->host_db_info);
-    s->parent_info.dns_round_robin = s->dns_info.round_robin;
+    s->parent_info.dns_round_robin = s->host_db_info.round_robin;
 
     char addrbuf[INET6_ADDRSTRLEN];
     DebugTxn("http_trans", "[PPDNSLookup] DNS lookup for sm_id[%" PRId64"] successful IP:
%s",
@@ -1452,19 +1452,19 @@ void
 HttpTransact::ReDNSRoundRobin(State* s)
 {
   ink_assert(s->current.server == &s->server_info);
-  ink_assert(s->current.server->connect_failure);
+  ink_assert(s->current.server->had_connect_fail());
 
   if (s->dns_info.lookup_success) {
     // We using a new server now so clear the connection
     //  failure mark
-    s->current.server->connect_failure = 0;
+    s->current.server->clear_connect_fail();
 
     // Our ReDNS of the server succeeeded so update the necessary
     //  information and try again
     ats_ip_copy(&s->server_info.addr, s->host_db_info.ip());
     ats_ip_copy(&s->request_data.dest_ip, &s->server_info.addr);
     get_ka_info_from_host_db(s, &s->server_info, &s->client_info, &s->host_db_info);
-    s->server_info.dns_round_robin = s->dns_info.round_robin;
+    s->server_info.dns_round_robin = s->host_db_info.round_robin;
 
     char addrbuf[INET6_ADDRSTRLEN];
     DebugTxn("http_trans", "[ReDNSRoundRobin] DNS lookup for O.S. successful IP: %s",
@@ -1612,7 +1612,7 @@ HttpTransact::OSDNSLookup(State* s)
     // We've backed off from a client supplied address and found some
     // HostDB addresses. We use those if they're different from the CTA.
     // In all cases we now commit to client or HostDB for our source.
-    if (s->dns_info.round_robin) {
+    if (s->host_db_info.round_robin) {
       HostDBInfo* cta = s->host_db_info.rr()->select_next(&s->current.server->addr.sa);
       if (cta) {
         // found another addr, lock in host DB.
@@ -1635,7 +1635,7 @@ HttpTransact::OSDNSLookup(State* s)
   ats_ip_copy(&s->server_info.addr, s->host_db_info.ip());
   ats_ip_copy(&s->request_data.dest_ip, &s->server_info.addr);
   get_ka_info_from_host_db(s, &s->server_info, &s->client_info, &s->host_db_info);
-  s->server_info.dns_round_robin = s->dns_info.round_robin;
+  s->server_info.dns_round_robin = s->host_db_info.round_robin;
 
   char addrbuf[INET6_ADDRSTRLEN];
   DebugTxn("http_trans", "[OSDNSLookup] DNS lookup for O.S. successful "
@@ -3356,7 +3356,7 @@ HttpTransact::handle_response_from_parent(State* s)
   switch (s->current.state) {
   case CONNECTION_ALIVE:
     DebugTxn("http_trans", "[hrfp] connection alive");
-    s->current.server->connect_failure = 0;
+    s->current.server->connect_result = 0;
     SET_VIA_STRING(VIA_DETAIL_PP_CONNECT, VIA_DETAIL_PP_SUCCESS);
     if (s->parent_result.retry) {
       s->parent_params->recordRetrySuccess(&s->parent_result);
@@ -3371,7 +3371,7 @@ HttpTransact::handle_response_from_parent(State* s)
 
       ink_assert(s->hdr_info.server_request.valid());
 
-      s->current.server->connect_failure = 1;
+      s->current.server->connect_result = ENOTCONN;
 
       char addrbuf[INET6_ADDRSTRLEN];
       DebugTxn("http_trans", "[%d] failed to connect to parent %s", s->current.attempts,
@@ -3481,14 +3481,14 @@ HttpTransact::handle_response_from_server(State* s)
   case CONNECTION_ALIVE:
     DebugTxn("http_trans", "[hrfs] connection alive");
     SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_SUCCESS);
-    s->current.server->connect_failure = 0;
+    s->current.server->clear_connect_fail();
     handle_forward_server_connection_open(s);
     break;
   case CONGEST_CONTROL_CONGESTED_ON_F:
   case CONGEST_CONTROL_CONGESTED_ON_M:
     DebugTxn("http_trans", "[handle_response_from_server] Error. congestion control -- congested.");
     SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE);
-    s->current.server->connect_failure = 1;
+    s->current.server->set_connect_fail(EUSERS); // too many users
     handle_server_connection_not_open(s);
     break;
   case OPEN_RAW_ERROR:
@@ -3504,7 +3504,10 @@ HttpTransact::handle_response_from_server(State* s)
   case CONNECTION_CLOSED:
     /* fall through */
   case BAD_INCOMING_RESPONSE:
-    s->current.server->connect_failure = 1;
+    // Set to generic I/O error if not already set specifically.
+    if (!s->current.server->had_connect_fail())
+      s->current.server->set_connect_fail(EIO);
+
     if (is_server_negative_cached(s)) {
       max_connect_retries = s->txn_conf->connect_attempts_max_retries_dead_server;
     } else {
@@ -3549,7 +3552,7 @@ HttpTransact::handle_response_from_server(State* s)
   case ACTIVE_TIMEOUT:
     DebugTxn("http_trans", "[hrfs] connection not alive");
     SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE);
-    s->current.server->connect_failure = 1;
+    s->current.server->set_connect_fail(ETIMEDOUT);
     handle_server_connection_not_open(s);
     break;
   default:
@@ -3582,7 +3585,7 @@ HttpTransact::delete_server_rr_entry(State* s, int max_retries)
   DebugTxn("http_trans", "[%d] failed to connect to %s", s->current.attempts,
         ats_ip_ntop(&s->current.server->addr.sa, addrbuf, sizeof(addrbuf)));
   DebugTxn("http_trans", "[delete_server_rr_entry] marking rr entry " "down and finding next
one");
-  ink_assert(s->current.server->connect_failure);
+  ink_assert(s->current.server->had_connect_fail());
   ink_assert(s->current.request_to == ORIGIN_SERVER);
   ink_assert(s->current.server == &s->server_info);
   update_dns_info(&s->dns_info, &s->current, 0, &s->arena);
@@ -3609,7 +3612,7 @@ HttpTransact::retry_server_connection_not_open(State* s, ServerState_t
conn_stat
   ink_assert(s->current.state != CONNECTION_ALIVE);
   ink_assert(s->current.state != ACTIVE_TIMEOUT);
   ink_assert(s->current.attempts <= max_retries);
-  ink_assert(s->current.server->connect_failure != 0);
+  ink_assert(s->current.server->had_connect_fail());
   char addrbuf[INET6_ADDRSTRLEN];
 
   char *url_string = s->hdr_info.client_request.url_string_get(&s->arena);
@@ -3660,7 +3663,7 @@ HttpTransact::handle_server_connection_not_open(State* s)
   DebugTxn("http_trans", "[handle_server_connection_not_open] (hscno)");
   DebugTxn("http_seq", "[HttpTransact::handle_server_connection_not_open] ");
   ink_assert(s->current.state != CONNECTION_ALIVE);
-  ink_assert(s->current.server->connect_failure != 0);
+  ink_assert(s->current.server->had_connect_fail());
 
   SET_VIA_STRING(VIA_SERVER_RESULT, VIA_SERVER_ERROR);
   HTTP_INCREMENT_TRANS_STAT(http_broken_server_connections_stat);

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/f63b27d5/proxy/http/HttpTransact.h
----------------------------------------------------------------------
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index 27e4ffb..6a66fd1 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -677,7 +677,7 @@ public:
     { }
   } RedirectInfo;
 
-  typedef struct _ConnectionAttributes
+  struct ConnectionAttributes
   {
     HTTPVersion http_version;
     HTTPKeepAlive keep_alive;
@@ -687,9 +687,11 @@ public:
     bool receive_chunked_response;
     bool pipeline_possible;
     bool proxy_connect_hdr;
+    /// @c errno from the most recent attempt to connect.
+    /// zero means no failure (not attempted, succeeded).
+    int connect_result;
     char *name;
     bool dns_round_robin;
-    bool connect_failure;
     TransferEncoding_t transfer_encoding;
 
     IpEndpoint addr;    // replaces 'ip' field
@@ -709,15 +711,19 @@ public:
     /// @c true if the connection is transparent.
     bool is_transparent;
 
-    _ConnectionAttributes()
+    bool had_connect_fail() const { return 0 != connect_result; }
+    void clear_connect_fail() { connect_result = 0; }
+    void set_connect_fail(int e) { connect_result = e; }
+
+    ConnectionAttributes()
       : http_version(),
         keep_alive(HTTP_KEEPALIVE_UNDEFINED),
         receive_chunked_response(false),
         pipeline_possible(false),
         proxy_connect_hdr(false),
+        connect_result(0),
         name(NULL),
         dns_round_robin(false),
-        connect_failure(false),
         transfer_encoding(NO_TRANSFER_ENCODING),
         port(0),
         state(STATE_UNDEFINED),
@@ -727,7 +733,8 @@ public:
     {
       memset(&addr, 0, sizeof(addr));
     }
-  } ConnectionAttributes;
+
+  };
 
   typedef struct _CurrentInfo
   {
@@ -770,14 +777,13 @@ public:
     char *lookup_name;
     char srv_hostname[MAXDNAME];
     LookingUp_t looking_up;
-    bool round_robin;
     bool srv_lookup_success;
     short srv_port;
     HostDBApplicationInfo srv_app;
 
     _DNSLookupInfo()
     : attempts(0), os_addr_style(OS_ADDR_TRY_DEFAULT),
-        lookup_success(false), lookup_name(NULL), looking_up(UNDEFINED_LOOKUP), round_robin(false),
+        lookup_success(false), lookup_name(NULL), looking_up(UNDEFINED_LOOKUP),
         srv_lookup_success(false), srv_port(0)
     {
       srv_hostname[0] = '\0';


Mime
View raw message