xds: fail to create xDS channel if no server with supported channel creds found #7400
Conversation
voidzcy
force-pushed
the
impl/fail_to_create_xds_channel_if_needed
branch
from
c1f54ce
to
612b791
Compare
| @@ -129,8 +128,10 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) { | |||
| xdsClientPool = attributes.get(XdsAttributes.XDS_CLIENT_POOL); | |||
| if (xdsClientPool == null) { | |||
| final BootstrapInfo bootstrapInfo; | |||
| final XdsChannel channel; | |||
There was a problem hiding this comment.
This EDS-only codepath will be deleted in #7391.
| @@ -687,8 +623,4 @@ boolean isUseProtocolV3() { | |||
| return useProtocolV3; | |||
| } | |||
| } | |||
|
|
|||
| interface XdsClientPoolFactory { | |||
There was a problem hiding this comment.
More cleanup to be done to put RefCountedXdsClientObjectPool into its own file.
There was a problem hiding this comment.
It would be easier to review if the first commit Move XdsChannelFactory to its own file and improve tests. is split into two commits with one pure move and the other pure modification; and moving interface XdsClientPoolFactory is not scattered in the first and third commit.
| @Test | ||
| public void defaultUseV2ProtocolL() throws XdsInitializationException { | ||
| XdsChannel channel = createChannel(server1); | ||
| assertThat(channel.isUseProtocolV3()).isFalse(); |
There was a problem hiding this comment.
You should also check
server1 with experimentalV3SupportEnvVar = true
and
server2 with experimentalV3SupportEnvVar = false
still do not support v3.
There was a problem hiding this comment.
Hmm... Doesn't need to be that throughout, right? It makes tests noisy to test trivial things deeply.
server1 with experimentalV3SupportEnvVar = true and server2 with experimentalV3SupportEnvVar = false
This really just tests the functionality of the environment variable, which is a temporary gate. IMO, we shouldn't bother testing it. Unit tests can also introduces bugs, so if something is trivial in implementation, we won't bother write tests for it.
I am sorry, commits are a bit screwed up.. 😢 |
| */ | ||
| @Override | ||
| XdsChannel createChannel(List<ServerInfo> servers) throws XdsInitializationException { | ||
| checkArgument(!servers.isEmpty(), "No management server provided."); |
There was a problem hiding this comment.
Might as well throw XdsInitializationException?
…he xDS channel is empty.
…reds found (grpc#7400) Create the xDS channel outside the XdsClient. Throw an XdsInitializationException if the provided server list (parsed from the bootstrap file) can not be used to create such a channel. The exception is caught by the xDS resolver and propagated to the Channel gracefully as a name resolution error.
Follow up for #7396.
The main change is to create the xDS channel outside XdsClient and creating the xDS channel can fail.