xds/gcp_authn: implement GCP Authentication filter (gRFC A83). - #9119
Conversation
|
/gemini review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9119 +/- ##
==========================================
- Coverage 83.24% 83.23% -0.02%
==========================================
Files 419 420 +1
Lines 33884 34008 +124
==========================================
+ Hits 28206 28305 +99
- Misses 4258 4272 +14
- Partials 1420 1431 +11
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces GCP Service Account Identity credentials and a new xDS HTTP filter, gcp_authn, which attaches these credentials to outgoing RPCs using cluster metadata. The implementation features an LRU cache for credential management. Key review feedback points out critical bugs involving incorrect access paths for cluster metadata and security settings, which would cause nil pointer dereferences or compilation failures. Furthermore, the reviewer identified significant thread-safety issues in the cache resizing logic that could lead to data races. A minor suggestion was also made to declare the early expiry duration as a constant.
4531cb1 to
1c010a2
Compare
easwars
left a comment
There was a problem hiding this comment.
Still haven't looked at the last two tests in internal/xds/httpfilter/gcp_authn/gcp_authn_filter_e2e_test.go
|
I spent a little more time on the implementation of
But I also did not like the earlier approach where we had too many manual calls to
// sf is used to deduplicate credential creation for the same audience.
sf singleflight.Group
// getExisting retrieves the PerRPCCredentials for a specified audience if it
// already exists in the cache. It updates the access order in the LRU list.
//
// The boolean return value indicates if an entry was found in the cache.
func (c *lruCache) getExisting(audience string) (credentials.PerRPCCredentials, error, bool) {
c.mu.Lock()
defer c.mu.Unlock()
if entry, ok := c.cache[audience]; ok {
c.lruList.MoveToFront(entry.el)
return entry.creds, nil, true
}
return nil, nil, false
}
func (c *lruCache) getOrCreate(ctx context.Context, audience string) (credentials.PerRPCCredentials, error) {
if creds, err, ok := c.getExisting(audience); ok {
return creds, err
}
val, err, _ := c.sf.Do(audience, func() (interface{}, error) {
return google.NewServiceAccountIdentityCredentials(ctx, audience)
})
if err != nil {
return nil, err
}
creds := val.(credentials.PerRPCCredentials)
c.mu.Lock()
defer c.mu.Unlock()
if entry, ok := c.cache[audience]; ok {
c.lruList.MoveToFront(entry.el)
return entry.creds, nil
}
if uint64(len(c.cache)) >= c.cacheSize {
c.removeOldestLocked()
}
el := c.lruList.PushFront(audience)
c.cache[audience] = &cacheEntry{creds: creds, el: el}
return creds, nil
} |
|
I have changed |
| e := c.lruList.PushFront(audience) | ||
| c.cache[audience] = &cacheEntry{creds: creds, elem: e} |
There was a problem hiding this comment.
Minor nit:
| e := c.lruList.PushFront(audience) | |
| c.cache[audience] = &cacheEntry{creds: creds, elem: e} | |
| c.cache[audience] = &cacheEntry{ | |
| creds: creds, | |
| elem: c.lruList.PushFront(audience), | |
| } |
| t.Fatalf("BuildClientInterceptor() returned interceptor with filtername = %q, want %q", interceptor.filterName, filterName) | ||
| } | ||
| if interceptor.cache == nil || interceptor.cache.cacheSize != tc.wantCacheSize { | ||
| t.Fatalf("BuildClientInterceptor() returned interceptor with cacheSize = %d, want %d", interceptor.cache.cacheSize, tc.wantCacheSize) |
There was a problem hiding this comment.
Nit: if interceptor.cache == nil, then this line will panic when you try to access interceptor.cache.cacheSize.
| t.Fatalf("BuildClientInterceptor() returned interceptor with filtername = %q, want %q", interceptor.filterName, filterName) | ||
| } | ||
| if interceptor.cache == nil || interceptor.cache.cacheSize != tc.wantCacheSize[i] { | ||
| t.Fatalf("BuildClientInterceptor() returned interceptor with cacheSize = %d, want %d", interceptor.cache.cacheSize, tc.wantCacheSize) |
| tests := []struct { | ||
| name string | ||
| cfgs []httpfilter.FilterConfig | ||
| wantCacheSize []uint64 |
There was a problem hiding this comment.
Nit: s/wantCacheSize/wantCacheSizes/
Alternatively, you could define cfg1, cfg2 and wantCacheSize1 and wantCacheSize2.
There was a problem hiding this comment.
Replaced wantCacheSize with wantCacheSizes.
I would like to keep it as slice of sizes and cfgs.
| // filterName is the name of the HTTP filter instance in the xDS | ||
| // configuration. |
There was a problem hiding this comment.
Nit: Expand this comment to say that the filtername is used as the key in the cluster metadata to look up the audience value for the cluster to which the RPC is destined to.
| return nil, status.Errorf(codes.Unavailable, "gcpauthn: cluster config for %q is invalid or missing: %v", clusterName, clusterResult.Err) | ||
| } | ||
|
|
||
| m := clusterResult.Config.Cluster.Metadata |
There was a problem hiding this comment.
Nit: You can get rid of the local m here.
| lruList *list.List | ||
|
|
||
| // cache maps audience keys to their corresponding cacheEntry pointers, | ||
| // allowing O(1) lookup and promotion. |
There was a problem hiding this comment.
Nit: what does promotion mean here?
There was a problem hiding this comment.
promotion means moving the existing entry to the starting of the list, marking it as most recently used element.
Updated the comment to "cache maps audience keys to their corresponding cacheEntry pointers, allowing O(1) lookups and updates in the LRU list." to make more sense
|
|
||
| for _, err := range errs { | ||
| if err != nil { | ||
| t.Fatalf("EmptyCall() failed: %v", err) |
There was a problem hiding this comment.
Nit: It might be useful to print all errors here instead of just the first one. So, consider using t.Errorf and if we do that we would also get the added benefit of seeing the requestCount value below.
…(#83) This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [google.golang.org/grpc](https://github.com/grpc/grpc-go) | `v1.82.0` → `v1.83.1` |  |  | --- ### Release Notes <details> <summary>grpc/grpc-go (google.golang.org/grpc)</summary> ### [`v1.83.1`](https://github.com/grpc/grpc-go/releases/tag/v1.83.1): Release 1.83.1 [Compare Source](grpc/grpc-go@v1.83.0...v1.83.1) ### Security - xds/rbac: Fix a bug where nested `Principal` or `Permission` rules with `:scheme` or `grpc-` prefixed header matchers were not rejected, which could cause DENY rules to fail open. ([#​9258](grpc/grpc-go#9258)) - Special Thanks: [@​nvxbug](https://github.com/nvxbug) - xds/rbac: Fix a bug where the `host` header matcher was not being replaced with `:authority` in nested `Principal` or `Permission` rules. ([#​9258](grpc/grpc-go#9258)) - Special Thanks: [@​nvxbug](https://github.com/nvxbug) - xds/rbac: Fix a bug where a header matcher whose name was not lowercase, such as `X-Role`, matched no header, which could cause DENY rules to fail open. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) - xds/rbac: Fix a bug where a `:scheme` or `grpc-` prefixed header matcher was accepted when its name was not lowercase. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) - xds/rbac: Fix a bug where a `Host` header matcher was not replaced with `:authority`. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) ### Performance - transport: Restrict memory overhead of buffering small data frames. ([#​9331](grpc/grpc-go#9331)) ### [`v1.83.0`](https://github.com/grpc/grpc-go/releases/tag/v1.83.0): Release 1.83.0 [Compare Source](grpc/grpc-go@v1.82.1...v1.83.0) ### Security - server: Stop reading from connections when flooded by HTTP/2 frames to mitigate resource exhaustion. The default value for this limit is 100 frames, excluding DATA and HEADERS, and may be changed by setting environment variable `GRPC_GO_EXPERIMENTAL_CONTROL_BUFFER_THROTTLE_LIMIT`. - xds/rbac: Support `Metadata` and `RequestedServerName` permissions matcher fields. If present in a DENY rule, previously these would be ignored and fail-open. - xds/rbac: Fix panic when parsing unsupported fields in `NotRule`/`NotId` permissions. - xds/rbac: Support the deprecated `source_ip` principal identifier by treating it as equivalent to `direct_remote_ip`. - xds: Fix panic when parsing route header matchers configured with empty `exact_match`, `prefix_match`, or `suffix_match` strings. ([#​9223](grpc/grpc-go#9223)) ### New Features - xds/googlec2p: Enable DirectPath over Interconnect support for on-premises clients via the `force-xds` target URI query parameter. ([#​9133](grpc/grpc-go#9133)) - xds: Enable xDS configuration to control which fields get propagated from ORCA backend metric reports to LRS load reports. ([#​9145](grpc/grpc-go#9145)) - authz: Add `OnPolicyUpdate` callback to `FileWatcherOptions` to notify when an authz policy is loaded or updated. ([#​9142](grpc/grpc-go#9142)) - Special Thanks: [@​hnefatl](https://github.com/hnefatl) - xds: Add support for the GCP Authentication HTTP Filter, which automatically fetches and attaches GCP Service Account Identity JWT tokens to outgoing RPCs. - This feature can be enabled by setting environment variable `GRPC_EXPERIMENTAL_XDS_GCP_AUTHENTICATION_FILTER=true`. ([#​9119](grpc/grpc-go#9119)) - xds: Add support for xDS-based HTTP CONNECT proxies. - This feature can be enabled by setting environment variable `GRPC_EXPERIMENTAL_XDS_HTTP_CONNECT=true`. ([#​9151](grpc/grpc-go#9151)) - xds: Add support for `contains_match` in route header matchers. ([#​9223](grpc/grpc-go#9223)) ### Bug Fixes - credentials/alts: Fix panic when processing malformed frames by validating that the message frame length exceeds the message type field size. ([#​9197](grpc/grpc-go#9197)) - grpc: Fix compilation on Plan 9 targets (`GOOS=plan9`), broken since v1.81.0. ([#​9255](grpc/grpc-go#9255)) - Special Thanks: [@​Yusufihsangorgel](https://github.com/Yusufihsangorgel) ### [`v1.82.1`](https://github.com/grpc/grpc-go/releases/tag/v1.82.1): Release 1.82.1 [Compare Source](grpc/grpc-go@v1.82.0...v1.82.1) ### Security - server: Stop reading from the connection when flooded by HTTP/2 frames. The default value for this limit is 100 frames, excluding DATA and HEADERS, and may be changed by setting environment variable `GRPC_GO_EXPERIMENTAL_CONTROL_BUFFER_THROTTLE_LIMIT`. - xds/rbac: Support `Metadata` and `RequestedServerName` permissions matcher fields. If present in a DENY rule, previously these would be ignored and fail-open. - xds/rbac: Fix panic when parsing unsupported fields in `NotRule`/`NotId` permissions. - xds/rbac: Support the deprecated `source_ip` principal identifier by treating it as equivalent to `direct_remote_ip`. </details> --- ### Configuration 📅 **Schedule**: (in timezone Europe/Paris) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQ0LjMxLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbInR5cGUvbWlub3IiXX0=--> Reviewed-on: https://git.erwanleboucher.dev/eleboucher/runner-k8s-plugin/pulls/83
This PR implements the xDS GCP Authentication HTTP filter for gRPC-Go, as specified in gRFC A83.
The implementation provides a mechanism for attaching service account identity JWT tokens as gRPC call credentials on GCP, featuring a two-level caching strategy (filter-level cache of credentials objects and credential-level cache of tokens) to minimize latency and metadata server load.
RELEASE NOTES:
GRPC_EXPERIMENTAL_XDS_GCP_AUTHENTICATION_FILTERenvironment variable.