xDS: Atomically read and write xDS security configuration client side #6796
Conversation
zasweq
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6796 +/- ##
==========================================
- Coverage 83.57% 83.53% -0.04%
==========================================
Files 285 285
Lines 30950 30935 -15
==========================================
- Hits 25865 25841 -24
- Misses 4021 4027 +6
- Partials 1064 1067 +3
|
zasweq
changed the title
zasweq
changed the title
zasweq
force-pushed
the
fix-handshake-info
branch
from
8d82f50
to
e94701d
Compare
credentials/xds/xds_client_test.go
Outdated
| @@ -219,11 +220,13 @@ func newTestContextWithHandshakeInfo(parent context.Context, root, identity cert | |||
| // Creating the HandshakeInfo and adding it to the attributes is very | |||
| // similar to what the CDS balancer would do when it intercepts calls to | |||
| // NewSubConn(). | |||
| info := xdsinternal.NewHandshakeInfo(root, identity) | |||
| var sm []matcher.StringMatcher | |||
There was a problem hiding this comment.
Nit/optional: sms to pluralize string matcher.
credentials/xds/xds_client_test.go
Outdated
| addr = xdsinternal.SetHandshakeInfo(resolver.Address{}, &uPtr) | ||
| ctx = icredentials.NewClientHandshakeInfoContext(ctx, credentials.ClientHandshakeInfo{Attributes: addr.Attributes}) |
There was a problem hiding this comment.
Can we just update the existing pointer instead of creating a new entry in the context? That's how the production code will work.
There was a problem hiding this comment.
Ah, good point. Changed.
| addr.Attributes = addr.Attributes.WithValue(handshakeAttrKey{}, hInfo) | ||
| // updated with hiPtr. | ||
| func SetHandshakeInfo(addr resolver.Address, hiPtr *unsafe.Pointer) resolver.Address { | ||
| addr.Attributes = addr.Attributes.WithValue(handshakeAttrKey{}, hiPtr) | ||
| return addr | ||
| } | ||
|
|
||
| // GetHandshakeInfo returns a pointer to the HandshakeInfo stored in attr. |
There was a problem hiding this comment.
Maybe "pointer to the *HandshakeInfo"?
| // | ||
| // Safe for concurrent access. |
There was a problem hiding this comment.
Delete this now? Or say it's immutable?
There was a problem hiding this comment.
I feel like technically it's fine to call any methods on it concurrently since it's all read only and set at init time, but since it's not really important to call out/doesn't apply I'll delete it.
xds/internal/server/conn_wrapper.go
Outdated
| @@ -128,8 +128,7 @@ func (c *connWrapper) XDSHandshakeInfo() (*xdsinternal.HandshakeInfo, error) { | |||
| c.identityProvider = ip | |||
| c.rootProvider = rp | |||
|
|
|||
| xdsHI := xdsinternal.NewHandshakeInfo(c.rootProvider, c.identityProvider) | |||
| xdsHI.SetRequireClientCert(secCfg.RequireClientCert) | |||
| xdsHI := xdsinternal.NewHandshakeInfo(c.rootProvider, c.identityProvider, nil, secCfg.RequireClientCert) | |||
There was a problem hiding this comment.
Optional: return directly instead of using a temp var.
There was a problem hiding this comment.
Sure. Good point. Switched.
This PR does a few things:
This eliminates acting on partial updates due to synchronization only between setters on individual fields, so reads and writes (within the context of one Handshake and one new update) could arbitrarily intersplice and cause correctness issues due to partial reads of handshake info configuration. Security configuration still needs to be per cluster and not set by the top level cluster, but this fixes the race.
RELEASE NOTES: