xds: make channel creds required in bootstrap file by voidzcy · Pull Request #7396 · grpc/grpc-java

voidzcy · 2020-09-04T00:46:15Z

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 · 2020-09-04T01:45:07Z

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


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

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.

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.

Sure, fixed the message.

sanjaypujare · 2020-09-04T01:51:15Z

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


How about?

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

Sounds fair. Updated to not allow empty list.

voidzcy · 2020-09-11T00:31:27Z

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 · 2020-09-11T00:53:37Z

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 · 2020-09-11T01:41:20Z

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 · 2020-09-11T05:53:36Z

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 · 2020-09-11T08:18:28Z

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.

Make channel_creds required in bootstrap file.

5abf8a4

voidzcy requested a review from dapengzhang0 September 4, 2020 00:46

sanjaypujare reviewed Sep 4, 2020

View reviewed changes

voidzcy mentioned this pull request Sep 8, 2020

xds: fail to create xDS channel if no server with supported channel creds found #7400

Merged

Do not allow empty list of channel creds.

d47423b

dapengzhang0 approved these changes Sep 10, 2020

View reviewed changes

voidzcy added 2 commits September 10, 2020 17:17

Fix test that has empty channel_creds

8f760a0

Improve exception message.

3811246

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

xds: make channel creds required in bootstrap file (grpc#7396)

38eadbc

github-actions Bot locked as resolved and limited conversation to collaborators Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: make channel creds required in bootstrap file#7396

xds: make channel creds required in bootstrap file#7396
voidzcy merged 4 commits intogrpc:masterfrom
voidzcy:bugfix/make_xds_channel_creds_required

voidzcy commented Sep 4, 2020 •

edited

Loading

Uh oh!

sanjaypujare Sep 4, 2020

Uh oh!

voidzcy Sep 6, 2020

Uh oh!

dapengzhang0 Sep 10, 2020

Uh oh!

voidzcy Sep 11, 2020

Uh oh!

sanjaypujare Sep 4, 2020

Uh oh!

voidzcy Sep 10, 2020

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

sanjaypujare commented Sep 11, 2020

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

sanjaypujare commented Sep 11, 2020

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

voidzcy commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanjaypujare Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

voidzcy Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

voidzcy Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

voidzcy Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

sanjaypujare commented Sep 11, 2020

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

sanjaypujare commented Sep 11, 2020

Uh oh!

voidzcy commented Sep 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

voidzcy commented Sep 4, 2020 •

edited

Loading