I’m not nearly as familiar with proxies as y’all are, but thinking OO here, I
would expect an API (interface) that covers all interactions that might involve
a proxy, e.g.:
interface Proxy {
Socket connectSocket(InetSocketAddress hostAndPort);
}
You could then have several implementations, such as NoProxy (just creates a
standard socket), SNIProxy, Socks5Proxy, etc, perhaps even a DebugProxy for use
in tests or troubleshooting.
Since each Proxy implementation would be entirely self-contained and properly
initialized by its own appropriate constructors, the Geode API could simply
take a Proxy (no need to think about or constrain what future proxies might
someday be supported, as long as the interface points can be reasonably
anticipated).
The argument made in this thread that “it is hard for the user to discover the
different Proxy subclasses and know what is supported” puzzles me. All IDEs
have a quick way to see implementing classes of an interface. Javadocs also
show that information. Naming convention makes it pretty obvious too, e.g.
class Socks5Proxy implements Proxy. Even just grep'ing for “implements Proxy”
would work. The second part “know what is supported” does not need to be
complicated: if the Proxy interface fully defines the contract for all
interactions that must occur between Geode and any arbitrary Proxy
implementation, then the answer should be very simple: anything that implements
Proxy is a supported proxy implementation…
Maybe there are domain-specific reasons to hardcode support for a particular
proxy rather than enforce separation-of-concerns? I haven’t seen any
discussion on this thread even mentioning encapsulation or the potential
difficulty of doing it properly.
I do agree with Jake’s -1 to the Mar 11 iteration of this proposal. If you
really want to use a Builder pattern to constrain what proxies are permitted
for use with Geode, use a specialized builder for each supported proxy type.
Not all proxies are going to take the exact same parameters (NoProxy needs
none, while some other proxy might require additional certs or auths).
Glad to see this is being discussed. Many of Geode’s other APIs probably could
have benefitted from such a community review ;)
> On Mar 12, 2020, at 3:20 PM, Jacob Barrett <jbarr...@pivotal.io> wrote:
>
> Everyone,
>
> I would like to hear from more than us 5 people on this. It would be really
> great if others would chime in.
>
> -Jake
>
>
>
>> On Mar 12, 2020, at 3:05 PM, Bill Burcham <bill.burc...@gmail.com> wrote:
>>
>> Sure Udo. Dan is exploring some ideas now.
>>
>> All: let's consider this round of RFC closed (since it officially closed
>> yesterday) and we'll re-open it with a new deadline when we have those
>> changes in hand.
>>
>> On Thu, Mar 12, 2020 at 8:26 AM Udo Kohlmeyer <u...@apache.com> wrote:
>>
>>> Hi there Jake,
>>>
>>> Another twist to the story, but with a working (if unpolished ;) )
>>> prototype.
>>>
>>> It covers all bases of:
>>>
>>> * Type safety
>>> * Extensibility
>>> * Simple API design
>>> * API clarity
>>>
>>> It takes the best of all approaches.
>>>
>>> I like it!!
>>>
>>> +1 to this implementation.
>>>
>>> -1 to Bill's approach.
>>>
>>> @Bill, could we amend the RFC to favor this approach?
>>>
>>> --Udo
>>>
>>> On 3/11/20 11:30 PM, Jacob Barrett wrote:
>>>> -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 <bill.burc...@gmail.com>
>>> 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
>>> <
>>> 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
>>>>
>>>>
>>>
>
