trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: trafficserver git commit: [TS-4024] wire tracing enhancements. This closes #337.
Date Tue, 12 Jan 2016 16:48:52 GMT
This doesn't look like it addresses Alan's last comment about "unifying the SSL error reporting
logic"

> On Jan 12, 2016, at 7:48 AM, shinrich@apache.org wrote:
> 
> Repository: trafficserver
> Updated Branches:
>  refs/heads/master 9399a7641 -> bb40d788b
> 
> 
> [TS-4024] wire tracing enhancements.  This closes #337.
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/bb40d788
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/bb40d788
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/bb40d788
> 
> Branch: refs/heads/master
> Commit: bb40d788bb092670b05e3d084bc14e330e3afa96
> Parents: 9399a76
> Author: ericcarlschwartz <eschwartz1991@gmail.com>
> Authored: Sat Nov 14 13:26:58 2015 -0800
> Committer: shinrich <shinrich@yahoo-inc.com>
> Committed: Tue Jan 12 09:44:00 2016 -0600
> 
> ----------------------------------------------------------------------
> iocore/net/SSLConfig.cc         |  8 ++++----
> iocore/net/SSLNetVConnection.cc | 39 ++++++++++++++++++++++++++----------
> mgmt/RecordsConfig.cc           |  8 ++++----
> proxy/http/HttpSM.cc            | 31 ++++++++++++++--------------
> 4 files changed, 52 insertions(+), 34 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/iocore/net/SSLConfig.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc
> index 7bb60fe..63540ce 100644
> --- a/iocore/net/SSLConfig.cc
> +++ b/iocore/net/SSLConfig.cc
> @@ -307,10 +307,10 @@ SSLConfigParams::initialize()
>   REC_ReadConfigInt32(ssl_allow_client_renegotiation, "proxy.config.ssl.allow_client_renegotiation");
> 
>   // SSL Wire Trace configurations
> -  REC_ReadConfigInteger(ssl_wire_trace_enabled, "proxy.config.ssl.wire_trace_enabled");
> +  REC_EstablishStaticConfigInt32(ssl_wire_trace_enabled, "proxy.config.ssl.wire_trace_enabled");
>   if (ssl_wire_trace_enabled) {
>     // wire trace specific source ip
> -    REC_ReadConfigStringAlloc(ssl_wire_trace_addr, "proxy.config.ssl.wire_trace_addr");
> +    REC_EstablishStaticConfigStringAlloc(ssl_wire_trace_addr, "proxy.config.ssl.wire_trace_addr");
>     if (ssl_wire_trace_addr) {
>       ssl_wire_trace_ip = new IpAddr();
>       ssl_wire_trace_ip->load(ssl_wire_trace_addr);
> @@ -318,8 +318,8 @@ SSLConfigParams::initialize()
>       ssl_wire_trace_ip = NULL;
>     }
>     // wire trace percentage of requests
> -    REC_ReadConfigInteger(ssl_wire_trace_percentage, "proxy.config.ssl.wire_trace_percentage");
> -    REC_ReadConfigStringAlloc(ssl_wire_trace_server_name, "proxy.config.ssl.wire_trace_server_name");
> +    REC_EstablishStaticConfigInt32(ssl_wire_trace_percentage, "proxy.config.ssl.wire_trace_percentage");
> +    REC_EstablishStaticConfigStringAlloc(ssl_wire_trace_server_name, "proxy.config.ssl.wire_trace_server_name");
>   } else {
>     ssl_wire_trace_addr = NULL;
>     ssl_wire_trace_ip = NULL;
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/iocore/net/SSLNetVConnection.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
> index 7199efa..dc48c63 100644
> --- a/iocore/net/SSLNetVConnection.cc
> +++ b/iocore/net/SSLNetVConnection.cc
> @@ -286,12 +286,16 @@ ssl_read_from_net(SSLNetVConnection *sslvc, EThread *lthread, int64_t
&ret)
>         Debug("ssl.error", "[SSL_NetVConnection::ssl_read_from_net] SSL_ERROR_ZERO_RETURN");
>         break;
>       case SSL_ERROR_SSL:
> -      default:
> -        TraceIn(trace, sslvc->get_remote_addr(), sslvc->get_remote_port(), "SSL
Error: sslErr=%d, errno=%d", sslErr, errno);
> +      default: {
> +        char buf[512];
> +        unsigned long e = ERR_peek_last_error();
> +        ERR_error_string_n(e, buf, sizeof(buf));
> +        TraceIn(trace, sslvc->get_remote_addr(), sslvc->get_remote_port(), "SSL
Error: sslErr=%d, ERR_get_error=%ld (%s) errno=%d",
> +                sslErr, e, buf, errno);
>         event = SSL_READ_ERROR;
>         ret = errno;
>         SSL_CLR_ERR_INCR_DYN_STAT(sslvc, ssl_error_ssl, "[SSL_NetVConnection::ssl_read_from_net]:
errno=%d", errno);
> -        break;
> +      } break;
>       } // switch
>       break;
>     } // while( block_write_avail > 0 )
> @@ -833,11 +837,15 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, int64_t
&wattempted, i
>       Debug("ssl.error", "SSL_write-SSL_ERROR_ZERO_RETURN");
>       break;
>     case SSL_ERROR_SSL:
> -    default:
> -      TraceOut(trace, get_remote_addr(), get_remote_port(), "SSL Error: sslErr=%d, errno=%d",
err, errno);
> +    default: {
> +      char buf[512];
> +      unsigned long e = ERR_peek_last_error();
> +      ERR_error_string_n(e, buf, sizeof(buf));
> +      TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL Error: sslErr=%d, ERR_get_error=%ld
(%s) errno=%d", err, e, buf,
> +              errno);
>       r = -errno;
>       SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, "SSL_write-SSL_ERROR_SSL errno=%d",
errno);
> -      break;
> +    } break;
>     }
>     return (r);
>   }
> @@ -1232,10 +1240,15 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
>     TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server handshake ERROR_WANT_ACCEPT");
>     return EVENT_CONT;
> 
> -  case SSL_ERROR_SSL:
> +  case SSL_ERROR_SSL: {
>     SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, "SSLNetVConnection::sslServerHandShakeEvent,
SSL_ERROR_SSL errno=%d", errno);
> -    TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server handshake ERROR_SSL");
> +    char buf[512];
> +    unsigned long e = ERR_peek_last_error();
> +    ERR_error_string_n(e, buf, sizeof(buf));
> +    TraceIn(trace, get_remote_addr(), get_remote_port(),
> +            "SSL server handshake ERROR_SSL: sslErr=%d, ERR_get_error=%ld (%s) errno=%d",
ssl_error, e, buf, errno);
>     return EVENT_ERROR;
> +  }
> 
>   case SSL_ERROR_ZERO_RETURN:
>     TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server handshake ERROR_ZERO_RETURN");
> @@ -1335,15 +1348,19 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
> 
> 
>   case SSL_ERROR_SSL:
> -  default:
> +  default: {
>     err = errno;
>     // FIXME -- This triggers a retry on cases of cert validation errors....
>     Debug("ssl", "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_SSL");
>     SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, "SSLNetVConnection::sslClientHandShakeEvent,
SSL_ERROR_SSL errno=%d", errno);
>     Debug("ssl.error", "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_SSL");
> -    TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL client handshake SSL_ERROR");
> +    char buf[512];
> +    unsigned long e = ERR_peek_last_error();
> +    ERR_error_string_n(e, buf, sizeof(buf));
> +    TraceIn(trace, get_remote_addr(), get_remote_port(),
> +            "SSL client handshake ERROR_SSL: sslErr=%d, ERR_get_error=%ld (%s) errno=%d",
ssl_error, e, buf, errno);
>     return EVENT_ERROR;
> -    break;
> +  } break;
>   }
>   return EVENT_CONT;
> }
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/mgmt/RecordsConfig.cc
> ----------------------------------------------------------------------
> diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
> index c390ac5..34e180d 100644
> --- a/mgmt/RecordsConfig.cc
> +++ b/mgmt/RecordsConfig.cc
> @@ -1304,13 +1304,13 @@ static const RecordElement RecordsConfig[] =
>   ,
>   {RECT_CONFIG, "proxy.config.ssl.handshake_timeout_in", RECD_INT, "0", RECU_RESTART_TS,
RR_NULL, RECC_INT, "[0-65535]", RECA_NULL}
>   ,
> -  {RECT_CONFIG, "proxy.config.ssl.wire_trace_enabled", RECD_INT, "0", RECU_RESTART_TS,
RR_NULL, RECC_INT, "[0-2]", RECA_NULL}
> +  {RECT_CONFIG, "proxy.config.ssl.wire_trace_enabled", RECD_INT, "0", RECU_DYNAMIC,
RR_NULL, RECC_INT, "[0-2]", RECA_NULL}
>   ,
> -  {RECT_CONFIG, "proxy.config.ssl.wire_trace_addr", RECD_STRING, NULL , RECU_RESTART_TS,
RR_NULL, RECC_IP, "[0-255]\\.[0-255]\\.[0-255]\\.[0-255]", RECA_NULL}
> +  {RECT_CONFIG, "proxy.config.ssl.wire_trace_addr", RECD_STRING, NULL , RECU_DYNAMIC,
RR_NULL, RECC_IP, "[0-255]\\.[0-255]\\.[0-255]\\.[0-255]", RECA_NULL}
>   ,
> -  {RECT_CONFIG, "proxy.config.ssl.wire_trace_percentage", RECD_INT, "0", RECU_RESTART_TS,
RR_NULL, RECC_INT, "[0-100]", RECA_NULL}
> +  {RECT_CONFIG, "proxy.config.ssl.wire_trace_percentage", RECD_INT, "0", RECU_DYNAMIC,
RR_NULL, RECC_INT, "[0-100]", RECA_NULL}
>   ,
> -  {RECT_CONFIG, "proxy.config.ssl.wire_trace_server_name", RECD_STRING, NULL , RECU_RESTART_TS,
RR_NULL, RECC_STR, ".*", RECA_NULL}
> +  {RECT_CONFIG, "proxy.config.ssl.wire_trace_server_name", RECD_STRING, NULL , RECU_DYNAMIC,
RR_NULL, RECC_STR, ".*", RECA_NULL}
>   ,
>   //##############################################################################
>   //#
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/proxy/http/HttpSM.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
> index 5fae64c..ab97380 100644
> --- a/proxy/http/HttpSM.cc
> +++ b/proxy/http/HttpSM.cc
> @@ -5636,24 +5636,25 @@ HttpSM::attach_server_session(HttpServerSession *s)
> 
>   // es - is this a concern here in HttpSM?  Does it belong somewhere else?
>   // Get server and client connections
> -  UnixNetVConnection *server_vc = (UnixNetVConnection *)(server_session->get_netvc());
> +  UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection *>(server_session->get_netvc());
>   UnixNetVConnection *client_vc = (UnixNetVConnection *)(ua_session->get_netvc());
>   SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(client_vc);
> -  if (ssl_vc != NULL) { // if incoming connection is SSL
> -    bool client_trace = ssl_vc->getSSLTrace();
> -    if (client_trace) {
> -      // get remote address and port to mark corresponding traces
> -      const sockaddr *remote_addr = ssl_vc->get_remote_addr();
> -      uint16_t remote_port = ssl_vc->get_remote_port();
> -      server_vc->setOriginTrace(true);
> -      server_vc->setOriginTraceAddr(remote_addr);
> -      server_vc->setOriginTracePort(remote_port);
> -    } else {
> -      server_vc->setOriginTrace(false);
> -      server_vc->setOriginTraceAddr(NULL);
> -      server_vc->setOriginTracePort(0);
> +  bool associated_connection = false;
> +  if (server_vc) { // if server_vc isn't a PluginVC
> +    if (ssl_vc) { // if incoming connection is SSL
> +      bool client_trace = ssl_vc->getSSLTrace();
> +      if (client_trace) {
> +        // get remote address and port to mark corresponding traces
> +        const sockaddr *remote_addr = ssl_vc->get_remote_addr();
> +        uint16_t remote_port = ssl_vc->get_remote_port();
> +        server_vc->setOriginTrace(true);
> +        server_vc->setOriginTraceAddr(remote_addr);
> +        server_vc->setOriginTracePort(remote_port);
> +        associated_connection = true;
> +      }
>     }
> -  } else {
> +  }
> +  if (!associated_connection && server_vc) {
>     server_vc->setOriginTrace(false);
>     server_vc->setOriginTraceAddr(NULL);
>     server_vc->setOriginTracePort(0);
> 


Mime
View raw message