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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Rob McKenna
As this enhancement is limited to JDK10 this frees us up to explore a
different approach.

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

In this webrev we're using the Service Provider Interface to load an
implementation of the new LdapDnsProvider class. Implementations of this
class are responsible for resolving DNS endpoints for a given ldap url
should the default implementation be insufficient in a particular
environment.

Note: if a security manager is installed the ldapDnsProvider
RuntimePermission must be granted.

    -Rob

Reply | Threaded
Open this post in threaded view
|

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

Vyom Tewari
Hi Rob,

Please find below comments.

DefaultLdapDnsProvider.java

  line 35:     convert it to for-each code will be more readable.

LdapDnsProviderService.java

  line 21: constant name does not follow Java naming convention, there
are other places as well please fix it.

getInstance() is already synchronized why you need another
"synchronized" block ?

Line 70: Please use result.getEndpoints().isEmpty() in place of
result.getEndpoints().size() == 0

LdapDnsProvider.java

constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which
does nothing, can you explain why LdapDnsProvider()  calls
LdapDnsProvider(Void unused) ?

LdapDnsProviderResult.java

   Private field  domainName & endpoints both can be final.

LdapDnsProviderTest.java

Fix the tag Order it is not correct. correct order is as follows.

/**
  * @test
  * @bug 8160768
  * @summary ctx provider tests for ldap
  * @modules java.naming/com.sun.jndi.ldap
  * @compile dnsprovider/TestDnsProvider.java
  * @run main/othervm LdapDnsProviderTest
  * @run main/othervm LdapDnsProviderTest nosm
  * @run main/othervm LdapDnsProviderTest smnodns
  * @run main/othervm LdapDnsProviderTest smdns
  */

Line 82,83 you don't need System.out.println(e); e.printStackTrace();

Line 70: you don't need this try block

Line 96 : constant name does not follow Java naming convention

Line 105: you can use try-with-resources this will avoid some boilerplate.

Thanks,
Vyom

On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:

> As this enhancement is limited to JDK10 this frees us up to explore a
> different approach.
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.06/
>
> In this webrev we're using the Service Provider Interface to load an
> implementation of the new LdapDnsProvider class. Implementations of this
> class are responsible for resolving DNS endpoints for a given ldap url
> should the default implementation be insufficient in a particular
> environment.
>
> Note: if a security manager is installed the ldapDnsProvider
> RuntimePermission must be granted.
>
>      -Rob
>

Reply | Threaded
Open this post in threaded view
|

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

Rob McKenna
Thanks Vyom, these should be fixed in:

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

Comments inline:

On 06/12/17 22:14, vyom tewari wrote:

> Hi Rob,
>
> Please find below comments.
>
> DefaultLdapDnsProvider.java
>
>  line 35:     convert it to for-each code will be more readable.
>
> LdapDnsProviderService.java
>
>  line 21: constant name does not follow Java naming convention, there are
> other places as well please fix it.
>
> getInstance() is already synchronized why you need another "synchronized"
> block ?

Ah, neglected to remove the outer synchronized keyword, thanks.

>
> Line 70: Please use result.getEndpoints().isEmpty() in place of
> result.getEndpoints().size() == 0
>
> LdapDnsProvider.java
>
> constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does
> nothing, can you explain why LdapDnsProvider()  calls LdapDnsProvider(Void
> unused) ?

I've copied this pattern from the System.LoggerFinder api and I had
forgotten what it was for too:

http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7

Section 7.3.

>
> LdapDnsProviderResult.java
>
>   Private field  domainName & endpoints both can be final.
>
> LdapDnsProviderTest.java
>
> Fix the tag Order it is not correct. correct order is as follows.
>
> /**
>  * @test
>  * @bug 8160768
>  * @summary ctx provider tests for ldap
>  * @modules java.naming/com.sun.jndi.ldap
>  * @compile dnsprovider/TestDnsProvider.java
>  * @run main/othervm LdapDnsProviderTest
>  * @run main/othervm LdapDnsProviderTest nosm
>  * @run main/othervm LdapDnsProviderTest smnodns
>  * @run main/othervm LdapDnsProviderTest smdns
>  */
>
> Line 82,83 you don't need System.out.println(e); e.printStackTrace();

I'm going to leave these in as I've had a few failing tests in the past
where I've needed to push diagnostic changes like this to determine the
root cause.

    -Rob

>
> Line 70: you don't need this try block
>
> Line 96 : constant name does not follow Java naming convention
>
> Line 105: you can use try-with-resources this will avoid some boilerplate.
>
> Thanks,
> Vyom
>
> On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
> >As this enhancement is limited to JDK10 this frees us up to explore a
> >different approach.
> >
> >http://cr.openjdk.java.net/~robm/8160768/webrev.06/
> >
> >In this webrev we're using the Service Provider Interface to load an
> >implementation of the new LdapDnsProvider class. Implementations of this
> >class are responsible for resolving DNS endpoints for a given ldap url
> >should the default implementation be insufficient in a particular
> >environment.
> >
> >Note: if a security manager is installed the ldapDnsProvider
> >RuntimePermission must be granted.
> >
> >     -Rob
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Vyom Tewari
Hi Rob,

Latest code looks good to me.

minor bit.

LdapDnsProviderService.java

Line: 71 , while loop condition is bit complex, we can simplify it little bit as follows.This will make code more readable and easy to understand.


while (iterator.hasNext())
             {
                 LdapDnsProvider p = iterator.next();
                 result = p.lookupEndpoints(url, env);
                 if(result != null && !result.getEndpoints().isEmpty()){
                     break;
                 }
             }
It is just a personal preference you can ignore it if you don't like it.

Thanks,
Vyom

On Wednesday 06 December 2017 11:54 PM, Rob McKenna wrote:

> Thanks Vyom, these should be fixed in:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.07/
>
> Comments inline:
>
> On 06/12/17 22:14, vyom tewari wrote:
>> Hi Rob,
>>
>> Please find below comments.
>>
>> DefaultLdapDnsProvider.java
>>
>>   line 35:     convert it to for-each code will be more readable.
>>
>> LdapDnsProviderService.java
>>
>>   line 21: constant name does not follow Java naming convention, there are
>> other places as well please fix it.
>>
>> getInstance() is already synchronized why you need another "synchronized"
>> block ?
> Ah, neglected to remove the outer synchronized keyword, thanks.
>
>> Line 70: Please use result.getEndpoints().isEmpty() in place of
>> result.getEndpoints().size() == 0
>>
>> LdapDnsProvider.java
>>
>> constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does
>> nothing, can you explain why LdapDnsProvider()  calls LdapDnsProvider(Void
>> unused) ?
> I've copied this pattern from the System.LoggerFinder api and I had
> forgotten what it was for too:
>
> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
>
> Section 7.3.
>
>> LdapDnsProviderResult.java
>>
>>    Private field  domainName & endpoints both can be final.
>>
>> LdapDnsProviderTest.java
>>
>> Fix the tag Order it is not correct. correct order is as follows.
>>
>> /**
>>   * @test
>>   * @bug 8160768
>>   * @summary ctx provider tests for ldap
>>   * @modules java.naming/com.sun.jndi.ldap
>>   * @compile dnsprovider/TestDnsProvider.java
>>   * @run main/othervm LdapDnsProviderTest
>>   * @run main/othervm LdapDnsProviderTest nosm
>>   * @run main/othervm LdapDnsProviderTest smnodns
>>   * @run main/othervm LdapDnsProviderTest smdns
>>   */
>>
>> Line 82,83 you don't need System.out.println(e); e.printStackTrace();
> I'm going to leave these in as I've had a few failing tests in the past
> where I've needed to push diagnostic changes like this to determine the
> root cause.
>
>      -Rob
>
>> Line 70: you don't need this try block
>>
>> Line 96 : constant name does not follow Java naming convention
>>
>> Line 105: you can use try-with-resources this will avoid some boilerplate.
>>
>> Thanks,
>> Vyom
>>
>> On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
>>> As this enhancement is limited to JDK10 this frees us up to explore a
>>> different approach.
>>>
>>> http://cr.openjdk.java.net/~robm/8160768/webrev.06/
>>>
>>> In this webrev we're using the Service Provider Interface to load an
>>> implementation of the new LdapDnsProvider class. Implementations of this
>>> class are responsible for resolving DNS endpoints for a given ldap url
>>> should the default implementation be insufficient in a particular
>>> environment.
>>>
>>> Note: if a security manager is installed the ldapDnsProvider
>>> RuntimePermission must be granted.
>>>
>>>      -Rob
>>>

Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph
In reply to this post by Rob McKenna
Hi Rob,

a little style nit: Is it really a good idea to use import java.util.*; in src/java.naming/share/classes/com/sun/jndi/ldap/LdapCtxFactory.java? I thought one is supposed to only use qualified exports nowadays (with all the IDE support).

Best regards
Christoph

-----Original Message-----
From: core-libs-dev [mailto:[hidden email]] On Behalf Of Rob McKenna
Sent: Mittwoch, 6. Dezember 2017 19:25
To: vyom tewari <[hidden email]>
Cc: Osipov, Michael <[hidden email]>; [hidden email]
Subject: Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

Thanks Vyom, these should be fixed in:

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

Comments inline:

On 06/12/17 22:14, vyom tewari wrote:

> Hi Rob,
>
> Please find below comments.
>
> DefaultLdapDnsProvider.java
>
>  line 35:     convert it to for-each code will be more readable.
>
> LdapDnsProviderService.java
>
>  line 21: constant name does not follow Java naming convention, there are
> other places as well please fix it.
>
> getInstance() is already synchronized why you need another "synchronized"
> block ?

Ah, neglected to remove the outer synchronized keyword, thanks.

>
> Line 70: Please use result.getEndpoints().isEmpty() in place of
> result.getEndpoints().size() == 0
>
> LdapDnsProvider.java
>
> constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does
> nothing, can you explain why LdapDnsProvider()  calls LdapDnsProvider(Void
> unused) ?

I've copied this pattern from the System.LoggerFinder api and I had
forgotten what it was for too:

http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7

Section 7.3.

>
> LdapDnsProviderResult.java
>
>   Private field  domainName & endpoints both can be final.
>
> LdapDnsProviderTest.java
>
> Fix the tag Order it is not correct. correct order is as follows.
>
> /**
>  * @test
>  * @bug 8160768
>  * @summary ctx provider tests for ldap
>  * @modules java.naming/com.sun.jndi.ldap
>  * @compile dnsprovider/TestDnsProvider.java
>  * @run main/othervm LdapDnsProviderTest
>  * @run main/othervm LdapDnsProviderTest nosm
>  * @run main/othervm LdapDnsProviderTest smnodns
>  * @run main/othervm LdapDnsProviderTest smdns
>  */
>
> Line 82,83 you don't need System.out.println(e); e.printStackTrace();

I'm going to leave these in as I've had a few failing tests in the past
where I've needed to push diagnostic changes like this to determine the
root cause.

    -Rob

>
> Line 70: you don't need this try block
>
> Line 96 : constant name does not follow Java naming convention
>
> Line 105: you can use try-with-resources this will avoid some boilerplate.
>
> Thanks,
> Vyom
>
> On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
> >As this enhancement is limited to JDK10 this frees us up to explore a
> >different approach.
> >
> >http://cr.openjdk.java.net/~robm/8160768/webrev.06/
> >
> >In this webrev we're using the Service Provider Interface to load an
> >implementation of the new LdapDnsProvider class. Implementations of this
> >class are responsible for resolving DNS endpoints for a given ldap url
> >should the default implementation be insufficient in a particular
> >environment.
> >
> >Note: if a security manager is installed the ldapDnsProvider
> >RuntimePermission must be granted.
> >
> >     -Rob
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Osipov, Michael
In reply to this post by Rob McKenna
Hi Rob,

a few comments on the webrev 07:

1. You have changed the semantics of domainName from null to "". This has implications on the StartTlsResponseImpl in regard with cert validation. This has to be discussion with someone who is firm with the STARTTLS command.
2. I miss documentation that says LdapDnsProviderResult can be null in LdapDnsProviderService(). If it is not null, getEndpoints() and getDomainName() must not be null. This uncertainty can easily lead to NPEs.
3. LdapCtxFactory:
+ // getDnsUrls(url, env) may throw a NamingException, which there is
+            // no need to wrap.
There is no getDnsUrls anymore
4. What is the purpose of using Void in LdapDnsProvider?


Michael

> -----Original Message-----
> From: Rob McKenna [mailto:[hidden email]]
> Sent: Wednesday, December 06, 2017 7:25 PM
> To: vyom tewari
> Cc: [hidden email]; Osipov, Michael (GS IT PD LD PLM)
> Subject: Re: [RFR] 8160768: Add capability to custom resolve host/domain
> names within the default JDNI LDAP provider
>
> Thanks Vyom, these should be fixed in:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.07/
>
> Comments inline:
>
> On 06/12/17 22:14, vyom tewari wrote:
> > Hi Rob,
> >
> > Please find below comments.
> >
> > DefaultLdapDnsProvider.java
> >
> >  line 35:     convert it to for-each code will be more readable.
> >
> > LdapDnsProviderService.java
> >
> >  line 21: constant name does not follow Java naming convention, there
> are
> > other places as well please fix it.
> >
> > getInstance() is already synchronized why you need another
> "synchronized"
> > block ?
>
> Ah, neglected to remove the outer synchronized keyword, thanks.
>
> >
> > Line 70: Please use result.getEndpoints().isEmpty() in place of
> > result.getEndpoints().size() == 0
> >
> > LdapDnsProvider.java
> >
> > constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which
> does
> > nothing, can you explain why LdapDnsProvider()  calls
> LdapDnsProvider(Void
> > unused) ?
>
> I've copied this pattern from the System.LoggerFinder api and I had
> forgotten what it was for too:
>
> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
>
> Section 7.3.
>
> >
> > LdapDnsProviderResult.java
> >
> >   Private field  domainName & endpoints both can be final.
> >
> > LdapDnsProviderTest.java
> >
> > Fix the tag Order it is not correct. correct order is as follows.
> >
> > /**
> >  * @test
> >  * @bug 8160768
> >  * @summary ctx provider tests for ldap
> >  * @modules java.naming/com.sun.jndi.ldap
> >  * @compile dnsprovider/TestDnsProvider.java
> >  * @run main/othervm LdapDnsProviderTest
> >  * @run main/othervm LdapDnsProviderTest nosm
> >  * @run main/othervm LdapDnsProviderTest smnodns
> >  * @run main/othervm LdapDnsProviderTest smdns
> >  */
> >
> > Line 82,83 you don't need System.out.println(e); e.printStackTrace();
>
> I'm going to leave these in as I've had a few failing tests in the past
> where I've needed to push diagnostic changes like this to determine the
> root cause.
>
>     -Rob
>
> >
> > Line 70: you don't need this try block
> >
> > Line 96 : constant name does not follow Java naming convention
> >
> > Line 105: you can use try-with-resources this will avoid some
> boilerplate.
> >
> > Thanks,
> > Vyom
> >
> > On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
> > >As this enhancement is limited to JDK10 this frees us up to explore a
> > >different approach.
> > >
> > >http://cr.openjdk.java.net/~robm/8160768/webrev.06/
> > >
> > >In this webrev we're using the Service Provider Interface to load an
> > >implementation of the new LdapDnsProvider class. Implementations of
> this
> > >class are responsible for resolving DNS endpoints for a given ldap url
> > >should the default implementation be insufficient in a particular
> > >environment.
> > >
> > >Note: if a security manager is installed the ldapDnsProvider
> > >RuntimePermission must be granted.
> > >
> > >     -Rob
> > >
> >