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 <jbarr...@pivotal.io> 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 <dsm...@pivotal.io> 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 <jb...@pivotal.io> 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<P extends ProxyConfiguration> {
> >>
> >> 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 extends PoolConfiguration> P getProxy();
> >>
> >> }
> >>
> >> -j
> >>
> >>
> >>> On Tue, Mar 10, 2020 at 10:38 AM Udo Kohlmeyer <u...@apache.com> 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(ProxyConfiguration) is that it
> >> is
> >>>> hard for the user to discover the different types of
> ProxyConfiguration
> >>>> subclasses and know what is supported.
> >>>>
> >>>> -Dan
> >>>>
> >>>>> On Mon, Mar 9, 2020 at 11:23 PM John Blum <jb...@pivotal.io> wrote:
> >>>>>
> >>>>> Corrections ( :-P ), my apologies...
> >>>>>
> >>>>> Iterable<ProxyConfiguration> proxyConfigurations = ...;
> >>>>>
> >>>>> StreamSupport.stream(proxyConfiguration*s*.*spliterator*(), false)
> >>>>> .filter(config -> config.getType.isSecure()) *// This could be
> >>>>> improved; see below...*
> >>>>> .*forEach*(config -> // do something with *each* secure proxy
> >>>>> *config*).
> >>>>>
> >>>>> Although, I am sure you got the point, ;-)
> >>>>>
> >>>>> With improvement:
> >>>>>
> >>>>> interface ProxyConfiguration {
> >>>>>
> >>>>> ProxyType getType();
> >>>>>
> >>>>> // null-safe!
> >>>>> default boolean isSecure() {
> >>>>> ProxyType type = getType();
> >>>>> return type != null && type.isSecure();
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> Then:
> >>>>>
> >>>>> StreamSupport.stream(..)
> >>>>> * .filter(ProxyConfiguration::isSecure)*
> >>>>> .forEach(...);
> >>>>>
> >>>>>
> >>>>> Again, completely contrived.
> >>>>>
> >>>>> Cheers!
> >>>>> -j
> >>>>>
> >>>>>
> >>>>>
> >>>>>> On Mon, Mar 9, 2020 at 11:14 PM John Blum <jb...@pivotal.io> wrote:
> >>>>>>
> >>>>>> Yes, it's redundant (i.e. Enum + class type).
> >>>>>>
> >>>>>> However, having an Enum in addition to a specific type (1 reason I
> >>>>>> defaulted the getType() method) can still be useful, such as in a
> >>> switch
> >>>>>> statement for example. Enums are, well, easier to enumerate (useful
> >> in
> >>>>>> Streams with filters). Maybe you are going to classify certain
> >> Proxy's
> >>>>>> by Enum type, for example:
> >>>>>>
> >>>>>> enum ProxyType {
> >>>>>>
> >>>>>> ONE,
> >>>>>> TWO,
> >>>>>> THREE;
> >>>>>>
> >>>>>> // Contrived example
> >>>>>> public boolean isSecure() {
> >>>>>> return Arrays.asList(ONE, THREE).contains(this);
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> Then:
> >>>>>>
> >>>>>> Iterable<ProxyConfiguration> proxyConfigurations = ...;
> >>>>>>
> >>>>>> StreamSupport.stream(proxyConfiguration.spilterator(), false)
> >>>>>> .filter(config -> config.getType.isSecure())
> >>>>>> .ifPresent(config -> // do something with secure proxy).
> >>>>>>
> >>>>>>
> >>>>>> Food for thought.
> >>>>>>
> >>>>>> -j
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Mar 9, 2020 at 7:11 PM Jacob Barrett <jbarr...@pivotal.io>
> >>>>> wrote:
> >>>>>>> Yes it’s redundant.
> >>>>>>>
> >>>>>>>> On Mar 9, 2020, at 5:08 PM, Bill Burcham <bill.burc...@gmail.com>
> >>>>>>> wrote:
> >>>>>>>> What I like about John's full-fledged-class-per-proxy-kind is
> that
> >>>>>>>> everything that can potentially vary with proxy kind is all
> >> together
> >>>>> in
> >>>>>>> a
> >>>>>>>> single object.
> >>>>>>>>
> >>>>>>>> That being said, John, in your SniProxyConfiguration, it seems to
> >> me
> >>>>>>> that
> >>>>>>>> the class itself (SniProxyConfiguration) could easily stand for
> the
> >>>>>>> proxy
> >>>>>>>> "type". If that's true then we could view ProxyType as redundant
> >> and
> >>>>>>> simply
> >>>>>>>> eliminate it right?
> >>>>>>>>
> >>>>>>>>> On Mon, Mar 9, 2020 at 2:41 PM Jacob Barrett <
> jbarr...@pivotal.io
> >>>
> >>>>>>> wrote:
> >>>>>>>>> +1 to Anthony and John. See similar comments in the RFC.
> >>>>>>>>>
> >>>>>>>>>>> On Mar 9, 2020, at 12:08 PM, Anthony Baker <aba...@pivotal.io>
> >>>>>>> wrote:
> >>>>>>>>>> I’m not suggesting encoding the the proxy type in the URI. Just
> >>>>>>>>> wondering if we can support stronger typing than String for
> >> defining
> >>>>>>>>> host/port/url configuration. As John notes, later in the thread,
> >>>>>>> perhaps
> >>>>>>>>> using a configuration interface may help.
> >>>>>>>>>> Anthony
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> On Mar 9, 2020, at 11:11 AM, Bill Burcham <
> >> bill.burc...@gmail.com
> >>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>> Anthony and Jacob, I can see how the proposed ProxyType
> >> parameter
> >>>>>>> could
> >>>>>>>>> fit
> >>>>>>>>>>> into the scheme part of a a URI. However, the problem that
> >>>>>>> introduces is
> >>>>>>>>>>> that we would then have to pick (named) URL schemes to support.
> >>> But
> >>>>>>> URL
> >>>>>>>>>>> schemes are standardized and it's not obvious which of the
> >>> standard
> >>>>>>> ones
> >>>>>>>>>>> might apply here.
> >>>>>>>>>>>
> >>>>>>>>>>> If we couldn't adopt a standard scheme, we'd have to make one
> >> up.
> >>>>> At
> >>>>>>>>> that
> >>>>>>>>>>> point I question the value of putting the (made-up) scheme into
> >> a
> >>>>> URI
> >>>>>>>>>>> string.
> >>>>>>>>>>>
> >>>>>>>>>>> For this reason, I am a fan of the ProxyType parameter over a
> >>>>> made-up
> >>>>>>>>> URL
> >>>>>>>>>>> scheme.
> >>>>>>>>>>>
> >>>>>>>>>>> That leaves open Anthony's other idea: eliminating the
> ProxyType
> >>>>>>>>> parameter
> >>>>>>>>>>> in favor of a separate method to set each kind of proxy. In the
> >>>>>>> current
> >>>>>>>>>>> RFC, that's just one, e.g. setPoolProxyWithSNI. I guess that
> >> comes
> >>>>>>> down
> >>>>>>>>> to:
> >>>>>>>>>>> what's the likelihood of us supporting other proxy types soon,
> >> and
> >>>>>>> then
> >>>>>>>>>>> what's the value of having a single method (and multiple enums)
> >>>>>>> versus
> >>>>>>>>>>> multiple methods (and no enum). If we thought the proxyAddress
> >>>>>>> parameter
> >>>>>>>>>>> would carry different information across proxy types that might
> >>>>> tilt
> >>>>>>> us
> >>>>>>>>>>> toward the separate methods. The two on the table, however,
> >> (SNI,
> >>>>>>>>> SOCKS5)
> >>>>>>>>>>> both have identical proxyAddress information.
> >>>>>>>>>>>
> >>>>>>>>>>> On Mon, Mar 9, 2020 at 10:54 AM Bill Burcham <
> >>>>> bill.burc...@gmail.com
> >>>>>>>>> wrote:
> >>>>>>>>>>>> By popular demand we are extending the RFC review period. I
> >> know
> >>>>> Udo
> >>>>>>>>> asked
> >>>>>>>>>>>> for Friday (and Joris +1'd it), but since this is a small RFC,
> >>>>> we'd
> >>>>>>>>> like to
> >>>>>>>>>>>> try to close it by Wednesday, March 11, ok?
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Mon, Mar 9, 2020 at 10:39 AM Jacob Barrett <
> >>>>> jbarr...@pivotal.io>
> >>>>>>>>> wrote:
> >>>>>>>>>>>>> I raised similar concerns as a comment in the RFC.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Mar 9, 2020, at 10:29 AM, Anthony Baker <
> >> aba...@pivotal.io>
> >>>>>>>>> wrote:
> >>>>>>>>>>>>>> Given this new API:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> setPoolProxy(ProxyType type, String proxyAddress)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The ProxyType enum seems to be a look ahead at supporting
> >> other
> >>>>>>> kinds
> >>>>>>>>>>>>> of proxies. What is your thinking about using the enum vs
> >>>>> specific
> >>>>>>>>> named
> >>>>>>>>>>>>> API’s (e.g. setPoolProxyWithSNI).
> >>>>>>>>>>>>>> Currently the definition of the proxyAddress seems to be
> >>>>>>> dependent of
> >>>>>>>>>>>>> the proxy type. Did you consider stronger typing using an
> URI
> >>>>>>>>> parameter
> >>>>>>>>>>>>> type?
> >>>>>>>>>>>>>> Anthony
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Mar 6, 2020, at 11:04 AM, Bill Burcham <
> >>>>>>> bill.burc...@gmail.com>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>> Please review the RFC for *Client side configuration for a
> >> SNI
> >>>>>>>>> proxy*:
> >>
> https://cwiki.apache.org/confluence/display/GEODE/Client+side+configuration+for+a+SNI+proxy
> >>>>>>>>>>>>>>> Please comment by Monday, March 9, 2020.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Regards,
> >>>>>>>>>>>>>>> Bill and Ernie
> >>>>>>
> >>>>>> --
> >>>>>> -John
> >>>>>> Spring Data Team
> >>>>>
> >>>>> --
> >>>>> -John
> >>>>> Spring Data Team
> >>
> >>
> >> --
> >> -John
> >> Spring Data Team
>
>
