xds: Implement GrpcService parsing and shared side channels (gRFC A102) - #9319
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9319 +/- ##
==========================================
+ Coverage 83.26% 87.45% +4.18%
==========================================
Files 423 429 +6
Lines 35236 30534 -4702
==========================================
- Hits 29341 26703 -2638
+ Misses 4402 3830 -572
+ Partials 1493 1 -1492
🚀 New features to boost your workflow:
|
|
The dependencies check flags the two new envoy extension proto packages (grpc_service/call_credentials/access_token/v3, grpc_service/channel_credentials/xds/v3) pulled in by the A102 GrpcService credential-plugin parsing — confined to the xds packages; core grpc dependencies are unchanged. |
d75cfcc to
40472da
Compare
…nn target parsing
| } | ||
| // The entry's refcount already dropped to zero and it is being | ||
| // cleaned up; remove it and create a fresh channel below. | ||
| delete(c.sideChannels, key) |
There was a problem hiding this comment.
Do we need this here? We already have a delete from map when the refcount goes to zero
| // sideChannelRelease returns an idempotent release function for the given | ||
| // channel entry. It must be called without holding sideChannelsMu, since the | ||
| // last release runs the cleanup synchronously, which acquires the mutex. | ||
| func sideChannelRelease(rc *grpcsync.RefCounted[*grpc.ClientConn]) func() error { |
There was a problem hiding this comment.
Do we need this to return error if it is always going to return nil?
| } | ||
|
|
||
| // New returns a GrpcService that parses GrpcService protos against the given | ||
| // bootstrap configuration. The trusted argument indicates whether the xDS |
There was a problem hiding this comment.
Nit: Can we change this comment a little? parses GrpcService protos against the given // bootstrap configuration is not very clear and very confusing if someone new reads this code with no context.
There was a problem hiding this comment.
Same — New no longer exists; Parse's doc is rewritten.
| } | ||
| } | ||
|
|
||
| func TestParse(t *testing.T) { |
There was a problem hiding this comment.
can we have these tests as function of grpctest.Tester
| // Tests that CreateChannel fails when the target is not allowlisted and the | ||
| // provided channel credentials are missing or unsupported, and when a call | ||
| // credentials type is not registered. | ||
| func (s) TestCreateChannel_Errors(t *testing.T) { |
There was a problem hiding this comment.
Should this be a table driven test ?
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
Haven't made a full pass, but have enough to move this forward for now.
…ess token creds, and simplify channel API
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
I have a couple of test files to review. Will do it shortly.
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| return conn, sync.OnceFunc(func() { conn.Close() }), nil |
There was a problem hiding this comment.
Can this sync.OnceFunc not also call server.Close() so that the caller doesn't have to. This will ensure creds are always cleaned up when channel closes because the caller could forget to call it.
There was a problem hiding this comment.
The OnceFunc is gone now (removed per Easwar's comment — the ref-count already guarantees a single call). On folding server.Close() into the cancel func: the seam deliberately doesn't own the config. Credential cleanup is centralized at the single ref-count-zero path in the filter (release(); entry.server.Close()), so there's no caller that could forget it — and configs whose parse fails partway need their creds released when no channel exists at all. Keeping cleanup out of a test-overridable variable also means tests that replace the seam can't accidentally leak or double-close credentials.
| // server described by the given config. The returned function closes the | ||
| // channel; it is idempotent. It is a variable so that tests can intercept | ||
| // channel creation and observe its release. | ||
| CreateExtProcChannel = func(server *grpcservice.Config) (grpc.ClientConnInterface, func(), error) { |
There was a problem hiding this comment.
Also we can move this to someplace common where proc and authz both can use it. This function will be common for both IIUC.
There was a problem hiding this comment.
Once ext_authz grows its channel path we can lift this to a shared location; with a single consumer today I'd rather not guess the shape it needs.
| // Credentials may be sourced from the bootstrap file (JSON) or from a | ||
| // GrpcService proto delivered by a trusted xDS server; the identity captures | ||
| // which, and is used to decide whether two configurations may share a | ||
| // channel. |
There was a problem hiding this comment.
This feels like a lot of implementation detail for a package level comment.
| GoogleGrpc: &v3corepb.GrpcService_GoogleGrpc{ | ||
| TargetUri: extProcAddr, | ||
| TargetUri: extProcAddr, | ||
| ChannelCredentialsPlugin: []*anypb.Any{{TypeUrl: insecureCredsTypeURL}}, |
There was a problem hiding this comment.
Can we also have tests for verifying other creds too ?
There was a problem hiding this comment.
Yeah, I was in 2 minds for this as it was a bigger lift than it looks and the unit tests were covering most of the testing. Your comment swings it in favour of adding it now.
The filter's part of this path is credentials-agnostic — it hands whatever bundle Parse built to Config.Dial — so an e2e here would re-test grpcservice functionality through the filter harness, the same misplacement Easwar flagged for the trust-policy tests. Added the coverage at the layer that owns it instead: TestConfigDialTLS in grpcservice parses a GrpcService proto with TLS channel creds (certificate-provider bootstrap) and access_token call creds, dials a TLS-serving backend, and the backend asserts the Bearer token arrives on a live RPC. Together with TestConfigDial (insecure + access_token rejected fail-closed) that covers "other creds actually work" end to end without the filter machinery.
| // credentials only when the delivering xDS management server is trusted, and | ||
| // requiring an untrusted server's target to be present in the bootstrap | ||
| // allowed_grpc_services map — is applied by grpcservice.Parse. | ||
| func parseGRPCService(gs *v3corepb.GrpcService, opts httpfilter.ParseOptions) (*grpcservice.Config, error) { |
There was a problem hiding this comment.
There is no need for this function now. It is a one liner which can be inlined at call sites.
| if got.ChannelCredentials.Bundle() == nil { | ||
| t.Error("Parse() returned channel credentials without a built bundle") | ||
| } | ||
| for i, cc := range got.CallCredentials { | ||
| if cc.Credentials() == nil { | ||
| t.Errorf("Parse() call credentials[%d] have no built credentials", i) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add a clarifying comment for it. Thanks.
| // Mirror the scheme resolution performed by grpc.NewClient: use the | ||
| // target's scheme if it parses and is registered; otherwise fall back | ||
| // to the default scheme with the whole target as the endpoint. | ||
| if u, err := url.Parse(targetURI); err == nil && resolver.Get(u.Scheme) != nil { |
There was a problem hiding this comment.
What if the target URI is sent with an unknown scheme ? Shouldn't we return an error in that case ?
For example , correct me if my understanding is wrong , if the targetURI is incorrect://something , in that case ,
u, err := url.Parse(targetURI); err == nil && resolver.Get(u.Scheme) != nil
will return false , and then we make it dns://incorrect://something which will pass the validation.
There was a problem hiding this comment.
You're reading the code right — that target passes validation here and fails at RPC time. That's deliberate: it mirrors how every channel in grpc-go treats its target, including the xDS channel itself.
Regular channels: grpc.NewClient parses the target, and when the scheme is missing or unregistered it falls back to the default scheme with the entire target as the endpoint — channel creation succeeds and the failure surfaces at RPC time as a resolution error:
Lines 1836 to 1858 in 664e87d
The xDS channel to the management server: bootstrap validates server_uri only for non-emptiness (
grpc-go/internal/xds/bootstrap/bootstrap.go
Line 570 in 664e87d
grpc-go/internal/xds/xdsclient/clientimpl.go
Line 276 in 664e87d
So validateTargetURI performs the same registry check NewClient performs, which is how I read A102's "checked against the resolver registry" for Go: the side channel accepts exactly the set of targets every other grpc-go channel accepts, and rejects exactly what NewClient rejects. We can discuss and do a follow up for this if needed.
There was a problem hiding this comment.
But the gRFC says this : The target URI must be checked against the resolver registry during xDS resource validation.
Which I would assume means that the resource should be nacked if the target URI is has an invalid scheme. It will eventually fail but I think we should verify this once.
cc : @easwars
| func certProviderConfig(resolver CertProviderConfigResolver, instanceName string) (*certprovider.BuildableConfig, error) { | ||
| cfg, ok := resolver.CertProviderConfigs()[instanceName] | ||
| if !ok { | ||
| return nil, fmt.Errorf("certificate provider instance name %q missing in bootstrap configuration", instanceName) |
There was a problem hiding this comment.
nit: add credentials: prefix
There was a problem hiding this comment.
Done — moved the prefix (and the root/identity context the callers were wrapping on) into this function, so the prefix appears exactly once.
| // a built bundle from a nil one; verify the bundle was built. | ||
| chanCreds, _ := got[target].SideChannelCredentials() | ||
| if chanCreds == nil || chanCreds.Bundle() == nil { | ||
| t.Error("SideChannelCredentials() returned no built channel credentials") |
There was a problem hiding this comment.
Nit: Maybe for completeness sake, add a field to the test table which says whether we expect call creds (or the number of call creds to expect) and verify here that call creds were in fact being built with the expected number.
| wantErr: "no supported channel credentials", | ||
| }, | ||
| { | ||
| name: "untrusted_allowlisted_uses_allowlist_creds", |
There was a problem hiding this comment.
Looks like we are not testing the untrusted case with call creds?
There was a problem hiding this comment.
Done — added an untrusted case whose allowlist carries jwt_token_file call creds; the proto's own call-creds plugin is present and ignored.
| return xdsresource.GRPCServiceConfig{}, fmt.Errorf("parseGRPCServiceConfig not implemented") | ||
| // TODO: Parse via grpcservice.Parse with the filter parse options, | ||
| // as ext_proc does, when ext_authz is wired up for gRFC A102. | ||
| parseGRPCServiceConfig = func(*v3corepb.GrpcService) (grpcservice.Config, error) { |
There was a problem hiding this comment.
Didnt notice this in the previous passes. This should also return *grpcservice.Config and not grpcservice.Config, so that once we plug in grpcservice.Parse here, things will work smoothly.
| if grpcService == nil { | ||
| return xdsresource.GRPCServiceConfig{}, nil | ||
| return grpcservice.Config{}, nil |
There was a problem hiding this comment.
Nit: We don't need this nil check. See: https://github.com/grpc/grpc-go/blob/master/internal/xds/httpfilter/ext_authz/ext_authz.go#L102-L108
| // Parse the GrpcService last, so that no error path can drop the built | ||
| // credentials: the caller owns them from here on. | ||
| var serverOpt optional.Optional[*grpcservice.Config] | ||
| if override.GetGrpcService() != nil { |
There was a problem hiding this comment.
Nit: assign override.GetGrpcService() to a local variable inside the if. That way, we don't have to call it again in the very next line.
|
Please wait for an approval from @eshitachandwani, especially for the changes to the proc filter. |
| } | ||
|
|
||
| func (b *tlsBundle) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { | ||
| rootKM, err := b.rootProvider.KeyMaterial(ctx) |
There was a problem hiding this comment.
Shouldn't we check for SPIFFE bundle map here too , similar to
There was a problem hiding this comment.
Just saw, this - its a good catch, let me do a follow up to fix this.
3 participants
Implements gRFC A102
GrpcServicesupport, wired into ext_proc as the first consumer:internal/xds/grpcservicepackage: parses theGrpcServiceproto and applies the trust decision — a trusted server's (A81) credentials come from the proto's plugins; an untrusted server's target must be in the bootstrapallowed_grpc_servicesallowlist (NACK otherwise).ClientFilterOptions.access_tokencall credentials (token sent only atPrivacyAndIntegrity).RELEASE NOTES: