xds: remove UpstreamTlsContext from XdsChannelBuilder #6924
Conversation
| if (overrideAuthority != null) { | ||
| builder = builder.overrideAuthority(overrideAuthority); | ||
| } | ||
| doAnswer( |
There was a problem hiding this comment.
this seems very hard / messy to use the SdsNameResolver. Can you do something similar to FakeNameResolver in ManagedChannelImplTest. it is not the best example, but seems simpler than this. also, it is using less resources and provider easier interface to test. Mock should be last option to consider. (e.g. this code can be easily converted to fake impl which will be much shorter and easier to read)
There was a problem hiding this comment.
I find doing mock and doAnswer simpler than how ManagedChannelImplTest.FakeNameResolverFactory.FakeNameResolver does it because you mock/construct the required input inline. But I realize this is subjective and if mock is not the preferred pattern for most people I will switch to it.
There was a problem hiding this comment.
no, this is not subjective at all.
mock is definitely anti pattern, in this case it is very easy to not use the mock (even as it is).
But the worse part is the TestSdsNameResolver extends the DnsNameResolver for no reason. The actual use case in test will be the interceptor (Callback) intercepts the onResult and returns a fake result. However, because it is a real DnsNameResolver, it uses thread for no reason, it performs IO for no reason. it even can call Listner#onError unless it is localhost (otherwise it will be environment dependent), and therefore it is pretty much localhost only NameResolver. List can goes on and on. Why we do all of those if we can just make a fake NameResolver, expects a URI and returns a specific value or an error?
There was a problem hiding this comment.
I wanted to do something more general than only for localhost (assuming it could be useful later) so decided to extend DnsNameResolver to be able to add the "mock" attribute for all returned addresses.
I agree the
ManagedChannelImplTest.FakeNameResolverFactory.FakeNameResolver pattern is less intrusive and easier to understand and I will use it. I will be "hardcoding" the localhost -> 127.0.0.1 mapping in my FakeNameResolver. There are cases where the loopback adapter (i.e. 127.0.0.1) is not configured or localhost does not resolve to 127.0.0.1 (but a configured local IP) although very rare - which was another reason I used the DnsNameResolver ("the environment").
| public void clientSdsProtocolNegotiatorNewHandler_nullTlsContext() { | ||
| ClientSdsProtocolNegotiator pn = | ||
| new ClientSdsProtocolNegotiator(/* upstreamTlsContext= */ null); | ||
| public void clientSdsProtocolNegotiatorNewHandler_nullTlsContextAttribute() { |
There was a problem hiding this comment.
nit: should be noTlsContext, null is not allowed.
| port = findFreePort(); | ||
| URI expectedUri = new URI("sdstest://localhost:" + port); | ||
| fakeNameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri).build(); | ||
| channelBuilder = |
There was a problem hiding this comment.
this seems worse than what it used to be. this is not even used elsewhere.
| .build() | ||
| : Attributes.EMPTY; | ||
| fakeNameResolverFactory.setServers( | ||
| Collections.singletonList(new EquivalentAddressGroup(socketAddress, attrs))); |
There was a problem hiding this comment.
nit/java optional suggestion: using ImmutableList is preferred than Collections.singletonList especially if ImmutableList is already imported.
|
|
||
| @Override | ||
| public String getDefaultScheme() { | ||
| return "fake"; |
There was a problem hiding this comment.
this doesn't match to sdstest
| @@ -210,4 +240,121 @@ public void unaryRpc(SimpleRequest req, StreamObserver<SimpleResponse> responseO | |||
| responseObserver.onCompleted(); | |||
| } | |||
| } | |||
|
|
|||
| private static final class FakeNameResolverFactory extends NameResolver.Factory { | |||
There was a problem hiding this comment.
you can remove unused code. private unused code is dead code.
|
Thanks @creamsoup ! |
No description provided.