core: lookup TXT records when doing name resolution #2912
Conversation
|
|
||
| /** | ||
| * A resolver that uses the JNDI interface. This class is capable of looking up both addresses | ||
| * and text records, but does not provide ordering guarantees. When the {@link JdkResolver} is |
There was a problem hiding this comment.
What? Why would JdkResolver be unavailable?
There was a problem hiding this comment.
It might not be prefered. I don't think it can use nsAuthority, while the jndi one can. Someone might prefer the latter behavior.
Removed anyways.
| import javax.annotation.Nullable; | ||
| import javax.annotation.concurrent.GuardedBy; | ||
| import javax.naming.NamingEnumeration; |
There was a problem hiding this comment.
It doesn't seem like Android has javax.naming. Or at least the older ones don't. Have you tested on Android?
There was a problem hiding this comment.
There was a problem hiding this comment.
I saw the DnsContextFactory, which would imply that it does, but I also see things like this though: https://issues.apache.org/jira/browse/LOG4J2-703
So that may be newer versions have it. But I also didn't see javax.naming at https://developer.android.com/reference/packages.html
There was a problem hiding this comment.
Looks like it is absent. Progaurd complains about refs to missing javax.naming classes, but not the com.sun ones. I think if I check for both, it will properly disable itself.
| @VisibleForTesting | ||
| static final class JndiResolver extends DelegateResolver { | ||
|
|
||
| private static final String[] rrTypes = new String[]{"A", "AAAA", "TXT"}; |
There was a problem hiding this comment.
Can we just request TXT? You mentioning that ANY was used makes me nervous, and it would be rare to use the A and AAAA here. Let's just KISS and only query for what we need.
| static final class CompositeResolver extends DelegateResolver { | ||
|
|
||
| private final DelegateResolver jdkResovler; | ||
| @Nullable private final DelegateResolver jndiResovler; |
There was a problem hiding this comment.
Make jndiResolver required? It seems you only use this if it is available.
There was a problem hiding this comment.
Done.
I feel like you are pushing into premature simplification. This code was very easy to modify to accept results from other, future resolvers, but now it is going to be harder.
| try { | ||
| inetAddrs = getAllByName(host); | ||
| } catch (UnknownHostException e) { | ||
| resolvedInetAddrs = delegateResolver.resolve(host); |
There was a problem hiding this comment.
So the TXT doesn't go anywhere?
There was a problem hiding this comment.
Currently No. There are no consumers yet.
| private static final String[] rrTypes = new String[]{"TXT"}; | ||
|
|
||
| @Override | ||
| ResolutionResults resolve(String host) throws NamingException { |
There was a problem hiding this comment.
Do we want to catch any exceptions from this, so that a failure to get TXT doesn't hose everything?
We may want to discuss with others what should happen if we get A/AAAA but have I/O errors durring TXT.
There was a problem hiding this comment.
I added it, but it should be fairly loud. Not honoring the service config if present is something a user would want to fix. It shouldn't reach this point if the JNDI resolver isn't present.
|
PTAL |
| * in turn calls into libc which sorts addresses in order of reachability. | ||
| */ | ||
| @VisibleForTesting | ||
| static final class JdkResolver extends DelegateResolver { |
There was a problem hiding this comment.
This could be just a singleton object.
|
@carl-mastrangelo, what about my "but until the TXT is used for something, I'm not sure we want to do the I/O" comment? |
|
@ejona86 good point. I added a boolean to disable it. It won't see any any actual usage outside of a test. |
No description provided.