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: server-side security: bootstrap support for grpc_server_resource_name_id #4030

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 12, 2020

See
GoogleCloudPlatform/traffic-director-grpc-bootstrap#6
for more info on this field.

A follow-up PR will update the xds.GRPCServer code to read from this field instead of hardcoding to grpc/server.

@easwars easwars added no release notes Type: Feature New features or improvements in behavior labels Nov 12, 2020
@@ -222,6 +227,10 @@ func NewConfig() (*Config, error) {
configs[instance] = bc
}
config.CertProviderConfigs = configs
case "grpc_server_resource_name_id":
Copy link
Contributor

Choose a reason for hiding this comment

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

What would server use if this is not set? Would the empty string work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An empty string would be an error, and we can fail the Serve() method on the server when that happens. I'm nervous about adding that check here because that would mean that users would have to update their bootstrap configs when the gRPC version changes.

Does that sound OK to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a required field for the servers, but not for clients?

Also, with the singleton xds_client, the servers no longer have access to the bootstrap config (they can still read if they want, but it's not necessary).
How would the servers fail? With the existing approach, the best we can do is to fail the LDS watch callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for making this configurable, instead of hardcoded? What other values do we expect the users (or us) to set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is a required field for the servers, but not for clients?

Also, with the singleton xds_client, the servers no longer have access to the bootstrap config (they can still read if they want, but it's not necessary).
How would the servers fail? With the existing approach, the best we can do is to fail the LDS watch callback.

The server will want access to the bootstrap config. When it receives a Listener resource, it needs to consult the bootstrap config to figure out what certificate providers to use.
It also needs access to the bootstrap config to know the id to be used in the Listener request.

Reason for making it configurable is mainly for supporting federation.

Copy link
Contributor

@menghanl menghanl Nov 12, 2020

Choose a reason for hiding this comment

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

If the server reads the bootstrap file again, it's possible to see different value from what the client read. We should add a method to the xds_client to return the config it's using.

And this PR LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

When it receives a Listener resource, it needs to consult the bootstrap config to figure out what certificate providers to use

For this one, why can't the xds_client do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the server reads the bootstrap file again, it's possible to see different value from what the client read. We should add a method to the xds_client to return the config it's using.

And this PR LGTM.

At this point in time, the server is reading the bootstrap file and creating a client using those contents. But once the singleton change is in, the server will just call New and it will get the same xdsClient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it receives a Listener resource, it needs to consult the bootstrap config to figure out what certificate providers to use

For this one, why can't the xds_client do it?

Currently on the client side, when the xdsClient receives a Cluster response with security_configuration, it only fetches the appropriate fields from the message and puts them in the ClusterUpdate and lets the CDS LB policy deal with it (in terms of creating certificate providers etc). The reason for doing it this way is that only the CDS LB policy knows whether XdsCredentials have been used and only in that case should the security_configuration be acted upon.

The same is true on the server side as well. The received security_configuration should be acted upon only when XdsCredentials have been configured.

So, as part of the CDS LB policy changes, I did add a CertProviderConfigs method on the xdsClient to return the certificate_providers configuration. Now, on the server-side we would also need a method to read the serverResourceNameID.

I think it would be better to now clean it up and export a single method to read all bootstrap config from the xdsClient. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We can just add a Config() *bootstrap.Config to the xds-client.

@easwars easwars merged commit d2629bd into grpc:master Nov 12, 2020
@easwars easwars deleted the xds_server_resource_name branch November 12, 2020 19:13
davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020
@easwars easwars changed the title xds/bootstrap: Add support for grpc_server_resource_name_id. xds: server-side security: bootstrap support for grpc_server_resource_name_id. Mar 4, 2021
@easwars easwars changed the title xds: server-side security: bootstrap support for grpc_server_resource_name_id. xds: server-side security: bootstrap support for grpc_server_resource_name_id Mar 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants