-
Notifications
You must be signed in to change notification settings - Fork 4.5k
xds: server-side security: bootstrap support for grpc_server_resource_name_id
#4030
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
Conversation
See GoogleCloudPlatform/traffic-director-grpc-bootstrap#6 for more info on this field.
@@ -222,6 +227,10 @@ func NewConfig() (*Config, error) { | |||
configs[instance] = bc | |||
} | |||
config.CertProviderConfigs = configs | |||
case "grpc_server_resource_name_id": |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
grpc_server_resource_name_id
.grpc_server_resource_name_id
.
grpc_server_resource_name_id
.grpc_server_resource_name_id
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 togrpc/server
.