+1 to Bill's proposal.
-Dan
On Wed, Mar 11, 2020 at 3:10 PM Udo Kohlmeyer <u...@apache.com> 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 <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
> >>
>
