Thanks for the review James.
I agree with most of your points -- with a couple of exceptions:
On Wed, 9 Oct 2013 10:41:42 -0700 James Peach wrote:
> On Oct 8, 2013, at 1:19 PM, ushac...@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.
There's also a small (3) - create a single point of configuration for all ATS
ports.
> - How do multiple plugins use this API?
I fully agree (and was planning to add support for this) - You're right that
without this the API is probably too specific.
> - How do plugins use this to accept on SSL sockets?
In the current implementation - they don't. There are a couple of issues to
consider here:
1) How do we allow protocol ordering (what if I want a protocol plugin to
handle the connection _before_ the SSL engine)
2) Protocol Plugin chaining implementation
> - We already have 3 *Accept() APIs, why can't they support this
> requirement?
Isn't it worthwhile to have a way to specify that we require the port to be
pre-opened? The other option I can see (without adding API like
TSPortDescriptorRendevous) would be to pass a flag....
I don't see a problem with creating multiple variants (in the API layer only)
to emphasise the difference in behaviour.
> - The name breaks API conventions (ie. there's no such object as a
> TSPluginDescriptor)
Agreed - TSNamedAccept(STRING) maybe....
> 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().
In progress on the first half - I dislike TSPortDescriptorRendevous for two
reasons:
1) 99.9% of uses would call TSPortDescriptorRendevous and then
TSPortDescriptorAccept
2) Multiple ports with the same owner tag will make the API a bit more
complicated since we'll need to return a list
> 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
Cheers,
Uri
