[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Daniel Fuchs]

Hi Rob,

Assuming the new webrev is:

http://cr.openjdk.java.net/~robm/8160768/webrev.09

then it looks much better to me.

I haven't re-reviewed everything in details -  just looked
at the updates (LdapDnsProviderService,
LdapDnsProvider signature change, ServiceLocator)

BTW: if I'm not mistaken returning an empty Optional is
equivalent, in the end, to returning an Optional containing
a LdapDnsProviderResult which returns an empty list of endpoints?
I guess that's OK, I don't see any value in preventing an
LdapDnsProviderResult to have an empty endpoint list anyway.

best regards,

-- daniel


On 06/11/2018 15:52, Rob McKenna wrote:
> Thanks folks,
> 
> Vyom, I've updated service to be volatile.
> 
> On 30/10/18 14:25, Daniel Fuchs wrote:
>> Hi Rob,
>>
>> LdapCtxFactory.java
>>
>>   187             for (String u : r.getEndpoints()) {
>>   188                 try {
>>   189                     ctx = getLdapCtxFromUrl(
>>   190                             r.getDomainName(), new LdapURL(u), env);
>>   191                 } catch (NamingException e) {
>>   192                     // try the next element
>>   193                     lastException = e;
>>   194                 }
>>   195             }
>>
>> is a break statement missing after line 190?
>>
>> If not then can you add a comment explaining why only the last
>> context is retained (and returned?)
>>
>> Alternatively, if a break is indeed missing, these three lines
>> could be moved into the for loop above:
>>
>>   206             // Record the URL that created the context
>>   207             ctx.setProviderUrl(url);
>>   208             return ctx;
>>
>> and maybe lines 206-207 could be moved into the
>> getLdapCtxFromUrl() method?
>>
> 
> Yes, you're right, we should return the first successful result.
> 
>> LdapDnsProviderService.java:
>>
>> Why is this class non final? If it can be made final then
>> the protected methods should probably become package.
>>
> 
> Good point, fixed.
> 
>> LdapDnsProvider.java:
>>
>> It is strange to see new APIs with Hashtable in the method
>> signature. I understand that our implementation will need
>> an Hashtable at some point to call javax.naming.spi.NamingProvider,
>> but how likely is it that a clean room implementation of
>> a LdapDnsProvider will need to do that?
>>
>> Maybe we could have Map<?,?> in the signature instead - and
>> leave the burden to our implementation - somewhere in ServiceLocator,
>> to adapt back to Hashtable where it needs to?
> 
> So I've altered the signature of the method to take a Map<?, ?> as
> proposed. I've added a getLdapService(String domainName, Map<?,?> environment)
> method to ServiceLocator which delegates to the existing method after
> conversion. Hopefully this addresses your concerns.
> 
> I'll update the CSR accordingly once this review is complete.
> 
>      -Rob
> 
>>
>>
>> best regards,
>>
>> -- daniel
>>
>>
>> On 25/10/2018 17:34, Rob McKenna wrote:
>>> This recently received CSR approval, so it seems like a good time to pick
>>> the codereview up again:
>>>
>>> http://cr.openjdk.java.net/~robm/8160768/webrev.08/
>>>
>>> Referencing:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
>>>
>>> 1) I'm copying the behaviour from
>>> LdapCtxFactory.getInitialContext(Hashtable<?,?>) where an empty String is
>>> the default value used when the provider url is null.
>>>
>>> I don't think HostnameChecker (by way of SNIHostname) will accept either
>>> empty or null values when doing the comparison.
>>>
>>> Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
>>> appears to pass the String "null" to
>>> StartTlsResponseImpl.setConnection(Connection, String), which attempts
>>> to substitute null values with the Connections host so there may be a bug
>>> here.
>>>
>>> I'm happy to allow null returns here if necessary. Sean, can you
>>> comment on the distinction between null & "" as far as hostname
>>> verification is concerned?
>>>
>>> 2) In the latest iteration lookupEndpoints() returns an
>>> Optional<LdapDnsResult>. Currently neither getEndpoints() nor
>>> getDomainName() can be null. (they can be an empty list and/or an empty
>>> String however)
>>>
>>> 3) Corrected.
>>>
>>> 4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
>>>
>>>       -Rob
>>>
>>