trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: git commit: TS-2268 Add support for opening protocol traffic sockets through the traffic_manager.
Date Wed, 09 Oct 2013 17:41:42 GMT
On Oct 8, 2013, at 1:19 PM, ushachar@apache.org wrote:

> Updated Branches:
>  refs/heads/master 7ba121c9a -> 3c0c835c1
> 
> 
> TS-2268 Add support for opening protocol traffic sockets through the
> traffic_manager.

Hi Uri,

I don't much like this patch :) From the JIRA ticket, it is intended to (1) allow traffic_manager
to hold ports for plugins and (2) support the full port specification syntax.

(2) is already supported by the TSPortDescriptor API, which actually supports the requirement
better than this implementation does.

I can see how (1) is interesting, but I don't think that this is the right API. This implementation
looks a lot like it supports one specific use case (not sure what the exact requirement it),
but would be difficult to use more generally.

	- How do multiple plugins use this API?
	- How do plugins use this to accept on SSL sockets?
	- We already have 3 *Accept() APIs, why can't they support this requirement?
	- The name breaks API conventions (ie. there's no such object as a TSPluginDescriptor)

I think that a more generally useful facility would be to consider extending the TSPortDescriptor
to handle requirement (2). The trick for this will be that traffic_manager has to open the
sockets and listen(), but not ever accept(). This means that we need to communicate the descriptor
string to traffic_manager. Here are some suggestions on how we could do that:

a) Add a owner=STRING tag to the socket descriptor. This could cause traffic_manager to listen
but not accept. A subsequent call to a new API TSPortDescriptorRendevous(STRING) would return
a corresponding TSPortDescriptor that can be used with TSPortDescriptorAccept().

b) Add socket descriptor support to plugins.config. traffic_manager would parse the file and
do the initial socket setup. Then you could add a TSPlugin*() API that returns the corresponding
set of TSPortDescriptor objects that you can accept on.

c) Add scoping to the records.config syntax. This looks a lot like (a) except it adds a general
facility that might be useful in other contexts. The idea is that you allow syntax like "proxy.config.http.server_ports[SCOPE]".
This would let the "sidechannel" plugin do TSMgmtStringScopedGet("sidechannel", proxy.config.http.server_ports",
&result). Just like (a), you would need to teach traffic_manager about the scoped options,
and implement something like TSPortDescriptorRendevous(SCOPE).

J 

> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/3c0c835c
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/3c0c835c
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/3c0c835c
> 
> Branch: refs/heads/master
> Commit: 3c0c835c1b06cb5c76ae8dba5add9a8ffc07495d
> Parents: 7ba121c
> Author: Uri Shachar <ushachar@apache.org>
> Authored: Tue Oct 8 23:17:08 2013 +0300
> Committer: Uri Shachar <ushachar@apache.org>
> Committed: Tue Oct 8 23:17:08 2013 +0300
> 
> ----------------------------------------------------------------------
> CHANGES                           |  3 +++
> lib/records/I_RecHttp.h           |  8 +++++++-
> lib/records/RecHttp.cc            |  5 +++++
> proxy/InkAPI.cc                   | 19 ++++++++++++++++++-
> proxy/api/ts/experimental.h       | 16 ++++++++++++++++
> proxy/http/HttpProxyServerMain.cc |  2 +-
> 6 files changed, 50 insertions(+), 3 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index c2c52f3..fa566c0 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,9 @@
>                                                          -*- coding: utf-8 -*-
> Changes with Apache Traffic Server 4.1.0
> 
> +  *) [TS-2268] Add support for opening protocol traffic sockets through the 
> +   traffic_manager. Added TSPluginDescriptorAccept into expiremental API.
> +
>   *) [TS-2266] Add a "make rat" Makefile target, to create a RAT report. This
>    is used for verifying licensing compliance on the entire source tree.
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/lib/records/I_RecHttp.h
> ----------------------------------------------------------------------
> diff --git a/lib/records/I_RecHttp.h b/lib/records/I_RecHttp.h
> index 4bd3de0..241d870 100644
> --- a/lib/records/I_RecHttp.h
> +++ b/lib/records/I_RecHttp.h
> @@ -94,7 +94,8 @@ public:
>     TRANSPORT_DEFAULT = 0, ///< Default (normal HTTP).
>     TRANSPORT_COMPRESSED, ///< Compressed HTTP.
>     TRANSPORT_BLIND_TUNNEL, ///< Blind tunnel (no processing).
> -    TRANSPORT_SSL ///< SSL connection.
> +    TRANSPORT_SSL, ///< SSL connection.
> +    TRANSPORT_PLUGIN /// < Protocol plugin connection
>   };
> 
>   int m_fd; ///< Pre-opened file descriptor if present.
> @@ -134,6 +135,9 @@ public:
>   /// Check for SSL port.
>   bool isSSL() const;
> 
> +  /// Check for SSL port.
> +  bool isPlugin() const;
> +
>   /// Process options text.
>   /// @a opts should not contain any whitespace, only the option string.
>   /// This object's internal state is updated as specified by @a opts.
> @@ -258,6 +262,7 @@ public:
>   static char const* const OPT_TRANSPARENT_FULL; ///< Full transparency.
>   static char const* const OPT_TRANSPARENT_PASSTHROUGH; ///< Pass-through non-HTTP.
>   static char const* const OPT_SSL; ///< SSL (experimental)
> +  static char const* const OPT_PLUGIN; ///< Protocol Plugin handle (experimental)
>   static char const* const OPT_BLIND_TUNNEL; ///< Blind tunnel.
>   static char const* const OPT_COMPRESSED; ///< Compressed.
>   static char const* const OPT_HOST_RES_PREFIX; ///< Set DNS family preference.
> @@ -270,6 +275,7 @@ protected:
> };
> 
> inline bool HttpProxyPort::isSSL() const { return TRANSPORT_SSL == m_type; }
> +inline bool HttpProxyPort::isPlugin() const { return TRANSPORT_PLUGIN == m_type; }
> 
> inline IpAddr&
> HttpProxyPort::outboundIp(uint16_t family) {
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/lib/records/RecHttp.cc
> ----------------------------------------------------------------------
> diff --git a/lib/records/RecHttp.cc b/lib/records/RecHttp.cc
> index 13a20a6..e9ad2b5 100644
> --- a/lib/records/RecHttp.cc
> +++ b/lib/records/RecHttp.cc
> @@ -77,6 +77,7 @@ char const* const HttpProxyPort::OPT_TRANSPARENT_OUTBOUND = "tr-out";
> char const* const HttpProxyPort::OPT_TRANSPARENT_FULL = "tr-full";
> char const* const HttpProxyPort::OPT_TRANSPARENT_PASSTHROUGH = "tr-pass";
> char const* const HttpProxyPort::OPT_SSL = "ssl";
> +char const* const HttpProxyPort::OPT_PLUGIN = "plugin";
> char const* const HttpProxyPort::OPT_BLIND_TUNNEL = "blind";
> char const* const HttpProxyPort::OPT_COMPRESSED = "compressed";
> 
> @@ -264,6 +265,8 @@ HttpProxyPort::processOptions(char const* opts) {
>     } else if (0 == strcasecmp(OPT_SSL, item)) {
>       m_type = TRANSPORT_SSL;
>       m_inbound_transparent_p = m_outbound_transparent_p = false;
> +    } else if (0 == strcasecmp(OPT_PLUGIN, item)) {
> +      m_type = TRANSPORT_PLUGIN;
>     } else if (0 == strcasecmp(OPT_TRANSPARENT_INBOUND, item)) {
> # if TS_USE_TPROXY
>       m_inbound_transparent_p = true;
> @@ -399,6 +402,8 @@ HttpProxyPort::print(char* out, size_t n) {
>     zret += snprintf(out+zret, n-zret, ":%s", OPT_BLIND_TUNNEL);
>   else if (TRANSPORT_SSL == m_type)
>     zret += snprintf(out+zret, n-zret, ":%s", OPT_SSL);
> +  else if (TRANSPORT_PLUGIN == m_type)
> +    zret += snprintf(out+zret, n-zret, ":%s", OPT_PLUGIN);
>   else if (TRANSPORT_COMPRESSED == m_type)
>     zret += snprintf(out+zret, n-zret, ":%s", OPT_COMPRESSED);
>   if (zret >= n) return n;
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/proxy/InkAPI.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
> index 1418826..f51f4c8 100644
> --- a/proxy/InkAPI.cc
> +++ b/proxy/InkAPI.cc
> @@ -8204,7 +8204,7 @@ TSPortDescriptorParse(const char * descriptor)
> TSReturnCode
> TSPortDescriptorAccept(TSPortDescriptor descp, TSCont contp)
> {
> -  Action * action;
> +  Action * action = NULL;
>   HttpProxyPort * port = (HttpProxyPort *)descp;
>   NetProcessor::AcceptOptions net(make_net_accept_options(*port, 0 /* nthreads */));
> 
> @@ -8217,6 +8217,23 @@ TSPortDescriptorAccept(TSPortDescriptor descp, TSCont contp)
>   return action ? TS_SUCCESS : TS_ERROR;
> }
> 
> +TSReturnCode
> +TSPluginDescriptorAccept(TSCont contp)
> +{
> +  Action * action = NULL;
> +
> +  HttpProxyPort::Group& proxy_ports = HttpProxyPort::global();
> +  for ( int i = 0 , n = proxy_ports.length() ; i < n ; ++i ) {
> +    HttpProxyPort& port = proxy_ports[i];
> +    if (port.isPlugin()) {
> +      NetProcessor::AcceptOptions net(make_net_accept_options(port, 0 /* nthreads */));
> +      action = netProcessor.main_accept((INKContInternal *)contp, port.m_fd, net);
> +    }
> +  }
> +  return action ? TS_SUCCESS : TS_ERROR;
> +}
> +
> +
> int
> TSHttpTxnBackgroundFillStarted(TSHttpTxn txnp)
> {
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/proxy/api/ts/experimental.h
> ----------------------------------------------------------------------
> diff --git a/proxy/api/ts/experimental.h b/proxy/api/ts/experimental.h
> index f13edab..75037a2 100644
> --- a/proxy/api/ts/experimental.h
> +++ b/proxy/api/ts/experimental.h
> @@ -219,6 +219,22 @@ extern "C"
>    ****************************************************************************/
>   tsapi int TSHttpTxnLookingUpTypeGet(TSHttpTxn txnp);
> 
> +  /**
> +     Attempt to attach the contp continuation to sockets that have already been
> +     opened by the traffic manager and defined as belonging to plugins (based on
> +     records.config configuration). If a connection is successfully accepted,
> +     the TS_EVENT_NET_ACCEPT is delivered to the continuation. The event
> +     data will be a valid TSVConn bound to the accepted connection.
> +     In order to configure such a socket, add the "plugin" keyword to a port
> +     in proxy.config.http.server_ports like "8082:plugin"
> +     Transparency/IP settings can also be defined, but a port cannot have
> +     both the "ssl" or "plugin" keywords configured.
> +
> +     Need to update records.config comments on proxy.config.http.server_ports
> +     when this option is promoted from experimental.
> +   */
> +  tsapi TSReturnCode TSPluginDescriptorAccept(TSCont contp);
> +
> 
>   /**
>       Opens a network connection to the host specified by the 'to' sockaddr
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/3c0c835c/proxy/http/HttpProxyServerMain.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/http/HttpProxyServerMain.cc b/proxy/http/HttpProxyServerMain.cc
> index cae18bd..ad02779 100644
> --- a/proxy/http/HttpProxyServerMain.cc
> +++ b/proxy/http/HttpProxyServerMain.cc
> @@ -233,7 +233,7 @@ start_HttpProxyServer()
>     if (port.isSSL()) {
>       if (NULL == sslNetProcessor.main_accept(acceptor._accept, port.m_fd, acceptor._net_opt))
>         return;
> -    } else {
> +    } else if (! port.isPlugin()) {
>       if (NULL == netProcessor.main_accept(acceptor._accept, port.m_fd, acceptor._net_opt))
>         return;
>     }
> 


Mime
View raw message