xds: make channel creds required in bootstrap file

[voidzcy]

This PR only adds validation to the bootstrap file to require users specify at least one channel creds configuration.

In a following up PR, creating the xDS channel will select the first supported channel creds and fail (e.g., an exception thrown) to create the channel if none is supported (instead of failing back to use parent channel's channel creds). Need to clean up the interfaces for creating XdsClient/Channel.

Also, Ideally I am thinking about having a BootstrapException. Using IOException is not accurate here.

[sanjaypujare]

This is the case of type not being set as opposed to "unknown type".

[voidzcy]

What I mean here is for "type unspecified". Reading bootstrap file should not contain the logic for interpreting the type that gRPC supports, that should be done at the time when the channel creds is actually used.

Say the bootstrap file provides 100 channel creds options, the parser should not try to look up and check one by one. Only at the time this information is used, it would interpret it: going through the list to find the first one that gRPC supports. If none can be found, the gRPC client fails.

[dapengzhang0]

It's confusing to see "unknown type" while type not provided.

"unknown type" sounds more like user provides a type but it's not recognized.

"with type unspecified" might be more clear.

[voidzcy]

Sure, fixed the message.

[sanjaypujare]

How about?

if (rawChannelCredsList == null || rawChannelCredsList.isEmpty())

[voidzcy]

Sounds fair. Updated to not allow empty list.

[voidzcy]

I came across trying to slightly modify the parsing logic (e.g., require the xds_server list not being empty). But I saw a lot of other tests start to fail, because of CommonCertProviderTestUtils.

This usage is very weird. You should just create a fake BootstrapInfo directly for your own usage, instead of relaying on the parser to parse it. Also, Bootstrapper.parseConfig() is a static helper method, it was made package private for testing the parser's logic. You should not make it public.

/cc @sanjaypujare

[sanjaypujare]

I came across trying to slightly modify the parsing logic (e.g., require the xds_server list not being empty). But I saw a lot of other tests start to fail, because of CommonCertProviderTestUtils.

Instead of making it a parsing check, you can make it a post parsing check of the parsed BootstrapInfo object. Isn't that the right thing to do?

This usage is very weird. You should just create a fake BootstrapInfo directly for your own usage, instead of relaying on the parser to parse it.

Most tests in io.grpc.xds.BootstrapperTest call parseConfig so I used that pattern. Although I agree it would be better to not rely on parseConfig. In this case you can make the xds_servers array non-empty to make it work?

Also, Bootstrapper.parseConfig() is a static helper method, it was made package private for testing the parser's logic. You should not make it public.

/cc @sanjaypujare

[voidzcy]

Instead of making it a parsing check, you can make it a post parsing check of the parsed BootstrapInfo object. Isn't that the right thing to do?

Currently it is a post-parsing check: the resolver returns an error to the channel if it finds no xDS server to use when creating the XdsClient, after reading the bootstrap file.

Since checking if xds_servers field is empty is cheap and one can also argue that it could be a syntax concern: the xds_servers JSON should not be an empty list, instead of semantics. Also, now we are requiring the channel_creds list to be non-empty in terms of syntax, I believe it's fair to apply the same constraint on xds_servers list.

Most tests in io.grpc.xds.BootstrapperTest call parseConfig so I used that pattern. Although I agree it would be better to not rely on parseConfig. In this case you can make the xds_servers array non-empty to make it work?

Tests in io.grpc.xds.BootstrapperTest are testing the Bootstrapper's logic, in which the main part is the parsing logic. Testing for other classes that use the result of bootstrapping (aka, the BootstrapInfo object) should be decoupled with how a BootstrapInfo is generated.

If you fixed your tests to not depend on how BootstrapInfo is generated, changing the behavior of parseConfig will not break your tests.

[sanjaypujare]

Instead of making it a parsing check, you can make it a post parsing check of the parsed BootstrapInfo object. Isn't that the right thing to do?

Currently it is a post-parsing check: the resolver returns an error to the channel if it finds no xDS server to use when creating the XdsClient, after reading the bootstrap file.

Since checking if xds_servers field is empty is cheap and one can also argue that it could be a syntax concern: the xds_servers JSON should not be an empty list, instead of semantics. Also, now we are requiring the channel_creds list to be non-empty in terms of syntax, I believe it's fair to apply the same constraint on xds_servers list.

Mandatory fields etc should be semantic checks instead of syntax check for various reasons (e.g. you can reuse the logic for non-JSON input if-and-when needed) where the syntax checks should be limited to JSON syntax and JSON schema validation.

Most tests in io.grpc.xds.BootstrapperTest call parseConfig so I used that pattern. Although I agree it would be better to not rely on parseConfig. In this case you can make the xds_servers array non-empty to make it work?

Tests in io.grpc.xds.BootstrapperTest are testing the Bootstrapper's logic, in which the main part is the parsing logic. Testing for other classes that use the result of bootstrapping (aka, the BootstrapInfo object) should be decoupled with how a BootstrapInfo is generated.

If you fixed your tests to not depend on how BootstrapInfo is generated, changing the behavior of parseConfig will not break your tests.

Yes I get the point and I'll modify the code to not use parseConfig but I am hoping that's not blocking this PR.

[voidzcy]

Yes I get the point and I'll modify the code to not use parseConfig but I am hoping that's not blocking this PR.

No, it's not. We are fine with existing changes in this PR. I just mean requiring xds_servers field non-empty at parsing time seems to be better, it's something we could improve.