-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: client-side security: xDS credentials implementation #3888
Conversation
@ZhenLian @jiangtaoli2016 |
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.
LGTM for the overall API and the security-related code such as handshake, etc.
Gentle ping @dfawley |
credentials/xds/xds.go
Outdated
// On the client side, rootProvider is mandatory. IdentityProvider is | ||
// optional based on whether the client is doing TLS or mTLS. | ||
if isClient && chi.rootProvider == nil { | ||
return errors.New("root certificate provider is missing") |
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.
These are going to surface to users IIUC; we should maybe have a better description of what went wrong in that case.
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.
How about now?
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 this mean when it happens? It's a programming error on our part? Or a usage error on the user's part? The error should tell the end user what to do: either how to fix the problem or let them know it's a grpc-go bug.
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.
This is an extreme edge case I would think where the control plane is not sending us the correct config. So, I have added another line to the error message asking the user to check the control plane config.
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.
Updating github state
credentials/xds/xds.go
Outdated
// SetHandshakeInfo returns a copy of attr which is updated with hInfo. | ||
func SetHandshakeInfo(attr *attributes.Attributes, hInfo *HandshakeInfo) *attributes.Attributes { | ||
return attr.WithValues(handshakeAttrKey{}, hInfo) | ||
} | ||
|
||
// GetHandshakeInfo returns a pointer to the HandshakeInfo stored in attr. | ||
func GetHandshakeInfo(attr *attributes.Attributes) *HandshakeInfo { |
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.
Please make these work on resolver.Address
es instead of attributes, e.g.:
grpc-go/internal/hierarchy/hierarchy.go
Line 35 in 759569b
func Get(addr resolver.Address) []string { |
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.
I have changed SetHandshakeInfo
to work on resolver.Address
since this will be invoked from the CDS balancer and it has a resolver.Address
at that point.
GetHandshakeInfo
is called from the credentials code, and there is no resolver.Address
at that point.
credentials/xds/xds.go
Outdated
// On the client side, rootProvider is mandatory. IdentityProvider is | ||
// optional based on whether the client is doing TLS or mTLS. | ||
if isClient && chi.rootProvider == nil { | ||
return errors.New("root certificate provider is missing") |
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 this mean when it happens? It's a programming error on our part? Or a usage error on the user's part? The error should tell the end user what to do: either how to fix the problem or let them know it's a grpc-go bug.
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.
A couple small things but otherwise looks great!
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.
PTAL.
credentials/xds/xds.go
Outdated
// updated with hInfo. | ||
func SetHandshakeInfo(addr resolver.Address, hInfo *HandshakeInfo) resolver.Address { | ||
addr.Attributes = addr.Attributes.WithValues(handshakeAttrKey{}, hInfo) | ||
return addr | ||
} | ||
|
||
// GetHandshakeInfo returns a pointer to the HandshakeInfo stored in attr. |
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.
Should this be unexported? Does anyone else need access to it? If so that would eliminate the asymmetry of the API (setter takes address, getter takes attributes).
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.
Done.
No description provided.