Re: RFC - Client side configuration for a SNI proxy
-1
I hate to do this but I really feel like we went backwards on this change.
> On Mar 11, 2020, at 3:03 PM, Bill Burcham wrote:
> PoolFactory {
> setProxyAddress(String host, int port);
> }
>
> ClientCacheFactory {
> setPoolProxyAddress(String host, int port);
> }
It gives the user no information about what type of proxy is in use. So
documentation must specify that it is only SNI. It assumes that SNI and any
other proxy would have a host and port only.
Also consider if we used SRV records to discover the proxy, would I need to set
hostname to null or the service name, what about port since it comes from the
SRV record. Would I set port to 0?.
What about default ports for well know proxy services like SOCKS? Again, 0?
> This should address Johns desire for an API that doesn't grow with each new
> proxy type. And it should address Udo's desire for extensibility.
I really struggle to see how this satisfies future extensibility given the
parameter lock in. Providing a class allows the proxy to define all its
options. Consider SniProxyConfig that takes no parameters, it could use the SRV
records for “_sniproxy._tcp” queried on the current domain to discover the
proxy. Or if given just a hostname could SRV query “_sniproxy._tcp.hostname”
and fall back to A “hostname” on the default port.
>
> This API sets the stage for a potential follow-on RFC to support other
> proxy types. One way that might work would be addition of
> setProxyType(String type)/setPoolProxyType(String type) methods. That RFC
> might reserve the "SNI" proxy type. That RFC might specify an SPI (per
> Jacob's proposal) that would let Geode users register their own proxy types
> e.g. SOCKS.
Adding disconnected method to set the proxy type by string doesn’t sit well for
me. What strings are valid? When we add this method what does it mean if we
don’t set it, its always SNI?
How would you deprecate a proxy? If the proxy configuration is a class I can
discover it in my IDE and deprecate the class when not supported anymore. Even
the original Enum idea could have deprecated enum values.
I would vote for the original RFC over this given that it weakens the type
safety of the API.
To illustrate my point on the SPI see
https://github.com/apache/geode/compare/develop...pivotal-jbarrett:wip/proxy-spi
.
Its a quick hack and some things might not be perfect, like discovering the
factories, but it should get the idea across.
-Jake
Re: RFC - Client side configuration for a SNI proxy
+1 to Bill's proposal.
-Dan
On Wed, Mar 11, 2020 at 3:10 PM Udo Kohlmeyer wrote:
> Bill,
>
> thank you for writing up *_our_* discussion of wrt this matter.
>
> +1 on the new API suggestion
>
> +1 to the future expansion, using an SPI approach to extend into support
> SOCKS5 aswell.
>
> Great team effort to get this one over the line.
>
> --Udo
>
> On 3/11/20 3:03 PM, Bill Burcham wrote:
> > In order to expeditiously address the current need (SNI proxy
> > configuration) and leave the door open to possible future expansion to
> > other proxy protocols, possibly via an SPI as Jacob alluded, I'd like to
> > propose modifying the current RFC as follows:
> >
> > * eliminate the ProxyType enum
> > * change the name of the setProxy methods to stipulate that it is the
> > _address_ that is being set e.g. setProxyAddress, setPoolProxyAddress
> > * separate the proxyAddress into two parts: host and port. That gives us
> a
> > little stronger typing in line with Anthony's concerns and Dan's latest
> > proposal
> >
> > So I'm proposing this:
> >
> > PoolFactory {
> >setProxyAddress(String host, int port);
> > }
> >
> > ClientCacheFactory {
> >setPoolProxyAddress(String host, int port);
> > }
> >
> > This should address Johns desire for an API that doesn't grow with each
> new
> > proxy type. And it should address Udo's desire for extensibility.
> >
> > This API sets the stage for a potential follow-on RFC to support other
> > proxy types. One way that might work would be addition of
> > setProxyType(String type)/setPoolProxyType(String type) methods. That RFC
> > might reserve the "SNI" proxy type. That RFC might specify an SPI (per
> > Jacob's proposal) that would let Geode users register their own proxy
> types
> > e.g. SOCKS.
> >
> > Can we all live with this approach? I'm looking for your +1s please!
> >
> > On Tue, Mar 10, 2020 at 4:54 PM Jacob Barrett
> wrote:
> >
> >> I think we should look at it from the standpoint of an SPI. The proxy
> work
> >> should be pluggable. One implementation we will provide out of the box
> is
> >> SNI.
> >>
> >> With this in mind geode-core (or better yet geode-proxy-spi) should only
> >> define a few interfaces. ProxyConfiguration and ProxyFactory.
> >>
> >> The code that creates client socket in geode-core should depend on the
> >> geode-proxy module that has logic to service discover ProxyFactory.
> Given a
> >> ProxyConfiguration instance it should be able to
> load/discover/interrogate
> >> for the correct ProxyFactory. The socket factory asks the ProxyFactory
> for
> >> a proxied socket.
> >>
> >> The geode-proxy-sni module would then implement all Thai for SNI proxy.
> >>
> >> Pluggable and modular.
> >>
> >> -Jake
> >>
> >>
> >>> On Mar 10, 2020, at 11:19 AM, Dan Smith wrote:
> >>>
> >>> The other reason I like setProxy(SniProxyConfiguration) is that it
> makes
> >> it
> >>> clear to the user what a legal value to pass in is. with
> >>> setProxy(ProxyConfiguration) the user can be mislead into thinking they
> >> are
> >>> supposed to implement ProxyConfiguration, but at runtime our code will
> >>> break if the object is not a SniProxyConfiguration. Anytime we can
> take a
> >>> runtime check for a bad argument and make it an API constraint that is
> >>> enforced at compile time I think that's a win.
> >>>
> >>> That said, I don't really care that much. I personally think even
> >>> setSniProxy(String host, int port) or setProxy(ProxyType.SNI, String
> >> host,
> >>> int port) would be fine - we may someday add SOCKS5 support, but I
> doubt
> >>> we'll ever hardcode support for more than 2 or 3 types of proxies. We'd
> >> be
> >>> more likely to provide a more generic way to completely override the
> >>> connection code.
> >>>
> >>> -Dan
> >>>
> On Tue, Mar 10, 2020 at 10:44 AM John Blum wrote:
>
> Again, we should keep the API footprint *small* and quit adding
> >> overrides
> for every type of Proxy we would (potentially) support. What is the
> >> point
> of an abstraction/type-hierarchy if you don't use it?
>
> The Enum would give users the possible range of currently supported
> >> Proxies
> anyway.
>
> Additionally, if you wanted something more specific, then you could
> introduce a generic type signature, which is more useful for return
> >> types,
> but work equally well for parameters as well...
>
> interface PoolFactory {
>
> void setProxy(P config);
>
> }
>
> Of course, this assumes you would have multiple different types of
> PoolFactory, which Geode does not currently, but could.
>
> Generic parameters are more useful for return types, and the entire
> PoolFactory, and don't necessarily need a generic type signature on
> the
> enclosing type, for example:
>
> interface PoolFactory {
>
> P getProxy();
>
> }
>
> -j
>
>
> > On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlm
Re: RFC - Client side configuration for a SNI proxy
Bill,
thank you for writing up *_our_* discussion of wrt this matter.
+1 on the new API suggestion
+1 to the future expansion, using an SPI approach to extend into support
SOCKS5 aswell.
Great team effort to get this one over the line.
--Udo
On 3/11/20 3:03 PM, Bill Burcham wrote:
In order to expeditiously address the current need (SNI proxy
configuration) and leave the door open to possible future expansion to
other proxy protocols, possibly via an SPI as Jacob alluded, I'd like to
propose modifying the current RFC as follows:
* eliminate the ProxyType enum
* change the name of the setProxy methods to stipulate that it is the
_address_ that is being set e.g. setProxyAddress, setPoolProxyAddress
* separate the proxyAddress into two parts: host and port. That gives us a
little stronger typing in line with Anthony's concerns and Dan's latest
proposal
So I'm proposing this:
PoolFactory {
setProxyAddress(String host, int port);
}
ClientCacheFactory {
setPoolProxyAddress(String host, int port);
}
This should address Johns desire for an API that doesn't grow with each new
proxy type. And it should address Udo's desire for extensibility.
This API sets the stage for a potential follow-on RFC to support other
proxy types. One way that might work would be addition of
setProxyType(String type)/setPoolProxyType(String type) methods. That RFC
might reserve the "SNI" proxy type. That RFC might specify an SPI (per
Jacob's proposal) that would let Geode users register their own proxy types
e.g. SOCKS.
Can we all live with this approach? I'm looking for your +1s please!
On Tue, Mar 10, 2020 at 4:54 PM Jacob Barrett wrote:
I think we should look at it from the standpoint of an SPI. The proxy work
should be pluggable. One implementation we will provide out of the box is
SNI.
With this in mind geode-core (or better yet geode-proxy-spi) should only
define a few interfaces. ProxyConfiguration and ProxyFactory.
The code that creates client socket in geode-core should depend on the
geode-proxy module that has logic to service discover ProxyFactory. Given a
ProxyConfiguration instance it should be able to load/discover/interrogate
for the correct ProxyFactory. The socket factory asks the ProxyFactory for
a proxied socket.
The geode-proxy-sni module would then implement all Thai for SNI proxy.
Pluggable and modular.
-Jake
On Mar 10, 2020, at 11:19 AM, Dan Smith wrote:
The other reason I like setProxy(SniProxyConfiguration) is that it makes
it
clear to the user what a legal value to pass in is. with
setProxy(ProxyConfiguration) the user can be mislead into thinking they
are
supposed to implement ProxyConfiguration, but at runtime our code will
break if the object is not a SniProxyConfiguration. Anytime we can take a
runtime check for a bad argument and make it an API constraint that is
enforced at compile time I think that's a win.
That said, I don't really care that much. I personally think even
setSniProxy(String host, int port) or setProxy(ProxyType.SNI, String
host,
int port) would be fine - we may someday add SOCKS5 support, but I doubt
we'll ever hardcode support for more than 2 or 3 types of proxies. We'd
be
more likely to provide a more generic way to completely override the
connection code.
-Dan
On Tue, Mar 10, 2020 at 10:44 AM John Blum wrote:
Again, we should keep the API footprint *small* and quit adding
overrides
for every type of Proxy we would (potentially) support. What is the
point
of an abstraction/type-hierarchy if you don't use it?
The Enum would give users the possible range of currently supported
Proxies
anyway.
Additionally, if you wanted something more specific, then you could
introduce a generic type signature, which is more useful for return
types,
but work equally well for parameters as well...
interface PoolFactory {
void setProxy(P config);
}
Of course, this assumes you would have multiple different types of
PoolFactory, which Geode does not currently, but could.
Generic parameters are more useful for return types, and the entire
PoolFactory, and don't necessarily need a generic type signature on the
enclosing type, for example:
interface PoolFactory {
P getProxy();
}
-j
On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer wrote:
I disagree.
I think /setProxy(ProxyConfiguration)/ is 1st prize.
If we are concerned that users will not know WHAT options are
available.. We could always have a static builder for our supported
options.
--Udo
On 3/10/20 10:07 AM, Dan Smith wrote:
Ok, how about this?
setProxy(SniProxyConfiguration config)
interface SniProxyConfiguration extends ProxyConfiguration {
static SniProxyConfiguration create(String host, int port);
String getHost();
int getPort()
}
The main difference here from John's proposal being that setProxy
takes a
SniProxyConfiguration, rather than the base ProxyConfiguration. We can
overload setProxy later if needed.
The reason I'm not as keen on setProxy(ProxyCon
Re: RFC - Client side configuration for a SNI proxy
In order to expeditiously address the current need (SNI proxy
configuration) and leave the door open to possible future expansion to
other proxy protocols, possibly via an SPI as Jacob alluded, I'd like to
propose modifying the current RFC as follows:
* eliminate the ProxyType enum
* change the name of the setProxy methods to stipulate that it is the
_address_ that is being set e.g. setProxyAddress, setPoolProxyAddress
* separate the proxyAddress into two parts: host and port. That gives us a
little stronger typing in line with Anthony's concerns and Dan's latest
proposal
So I'm proposing this:
PoolFactory {
setProxyAddress(String host, int port);
}
ClientCacheFactory {
setPoolProxyAddress(String host, int port);
}
This should address Johns desire for an API that doesn't grow with each new
proxy type. And it should address Udo's desire for extensibility.
This API sets the stage for a potential follow-on RFC to support other
proxy types. One way that might work would be addition of
setProxyType(String type)/setPoolProxyType(String type) methods. That RFC
might reserve the "SNI" proxy type. That RFC might specify an SPI (per
Jacob's proposal) that would let Geode users register their own proxy types
e.g. SOCKS.
Can we all live with this approach? I'm looking for your +1s please!
On Tue, Mar 10, 2020 at 4:54 PM Jacob Barrett wrote:
> I think we should look at it from the standpoint of an SPI. The proxy work
> should be pluggable. One implementation we will provide out of the box is
> SNI.
>
> With this in mind geode-core (or better yet geode-proxy-spi) should only
> define a few interfaces. ProxyConfiguration and ProxyFactory.
>
> The code that creates client socket in geode-core should depend on the
> geode-proxy module that has logic to service discover ProxyFactory. Given a
> ProxyConfiguration instance it should be able to load/discover/interrogate
> for the correct ProxyFactory. The socket factory asks the ProxyFactory for
> a proxied socket.
>
> The geode-proxy-sni module would then implement all Thai for SNI proxy.
>
> Pluggable and modular.
>
> -Jake
>
>
> > On Mar 10, 2020, at 11:19 AM, Dan Smith wrote:
> >
> > The other reason I like setProxy(SniProxyConfiguration) is that it makes
> it
> > clear to the user what a legal value to pass in is. with
> > setProxy(ProxyConfiguration) the user can be mislead into thinking they
> are
> > supposed to implement ProxyConfiguration, but at runtime our code will
> > break if the object is not a SniProxyConfiguration. Anytime we can take a
> > runtime check for a bad argument and make it an API constraint that is
> > enforced at compile time I think that's a win.
> >
> > That said, I don't really care that much. I personally think even
> > setSniProxy(String host, int port) or setProxy(ProxyType.SNI, String
> host,
> > int port) would be fine - we may someday add SOCKS5 support, but I doubt
> > we'll ever hardcode support for more than 2 or 3 types of proxies. We'd
> be
> > more likely to provide a more generic way to completely override the
> > connection code.
> >
> > -Dan
> >
> >> On Tue, Mar 10, 2020 at 10:44 AM John Blum wrote:
> >>
> >> Again, we should keep the API footprint *small* and quit adding
> overrides
> >> for every type of Proxy we would (potentially) support. What is the
> point
> >> of an abstraction/type-hierarchy if you don't use it?
> >>
> >> The Enum would give users the possible range of currently supported
> Proxies
> >> anyway.
> >>
> >> Additionally, if you wanted something more specific, then you could
> >> introduce a generic type signature, which is more useful for return
> types,
> >> but work equally well for parameters as well...
> >>
> >> interface PoolFactory {
> >>
> >> void setProxy(P config);
> >>
> >> }
> >>
> >> Of course, this assumes you would have multiple different types of
> >> PoolFactory, which Geode does not currently, but could.
> >>
> >> Generic parameters are more useful for return types, and the entire
> >> PoolFactory, and don't necessarily need a generic type signature on the
> >> enclosing type, for example:
> >>
> >> interface PoolFactory {
> >>
> >> P getProxy();
> >>
> >> }
> >>
> >> -j
> >>
> >>
> >>> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer wrote:
> >>>
> >>> I disagree.
> >>>
> >>> I think /setProxy(ProxyConfiguration)/ is 1st prize.
> >>>
> >>> If we are concerned that users will not know WHAT options are
> >>> available.. We could always have a static builder for our supported
> >>> options.
> >>>
> >>> --Udo
> >>>
> On 3/10/20 10:07 AM, Dan Smith wrote:
> Ok, how about this?
>
> setProxy(SniProxyConfiguration config)
>
> interface SniProxyConfiguration extends ProxyConfiguration {
> static SniProxyConfiguration create(String host, int port);
> String getHost();
> int getPort()
> }
>
> The main difference here from John's proposal being that setProxy
> >> takes a
> SniProxyConfiguration, rather than
