Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: make channel creds required in bootstrap file #7396

Merged
merged 4 commits into from Sep 11, 2020

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Sep 4, 2020

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.

String type = JsonUtil.getString(channelCreds, "type");
if (type == null) {
throw new IOException("Invalid bootstrap: 'xds_servers' contains server with "
+ "unknown type 'channel_creds'.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, fixed the message.

logger.log(XdsLogLevel.INFO, "Channel credentials option: {0}", type);
ChannelCreds creds = new ChannelCreds(type, JsonUtil.getObject(channelCreds, "config"));
channelCredsOptions.add(creds);
if (rawChannelCredsList == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fair. Updated to not allow empty list.

@voidzcy
Copy link
Contributor Author

voidzcy commented Sep 11, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

voidzcy commented Sep 11, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

voidzcy commented Sep 11, 2020

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.

@voidzcy voidzcy merged commit 8ac6362 into grpc:master Sep 11, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants