credentials: add GcpServiceAccountIdentityCallCredentials call credentials type (gRFC A83).#8974
credentials: add GcpServiceAccountIdentityCallCredentials call credentials type (gRFC A83).#8974Pranjali-2501 wants to merge 1 commit intogrpc:masterfrom
GcpServiceAccountIdentityCallCredentials call credentials type (gRFC A83).#8974Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8974 +/- ##
==========================================
- Coverage 83.13% 82.95% -0.18%
==========================================
Files 411 412 +1
Lines 32704 32779 +75
==========================================
+ Hits 27187 27191 +4
- Misses 4140 4193 +53
- Partials 1377 1395 +18
🚀 New features to boost your workflow:
|
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
| // a fetch recently failed, the cached error is returned until the backoff | ||
| // interval expires. Otherwise, it initiates a new token fetch or blocks | ||
| // waiting for an already-in-progress fetch to complete. | ||
| func (c *gcpServiceAccountIdentityCallCreds) GetRequestMetadata(ctx context.Context, _ ...string) (map[string]string, error) { |
There was a problem hiding this comment.
The gRFC states : If the returned HTTP status maps to UNAVAILABLE in HTTP to gRPC Status Code Mapping, then the data plane RPCs will be failed with status UNAVAILABLE; otherwise, they will be failed with status UNAUTHENTICATED. If the request fails without an HTTP status (e.g., an I/O error), all queued data plane RPCs will be failed with UNAVAILABLE status.
Where exactly are these status code mappings handled?
|
|
||
| type gcpServiceAccountIdentityCallCreds struct { | ||
| audience string | ||
| ts *auth.Credentials |
There was a problem hiding this comment.
should the field be named something like "creds" instead of ts?
| mu sync.Mutex | ||
| token *auth.Token | ||
|
|
||
| fetching chan struct{} |
There was a problem hiding this comment.
add a trailing comment for this chan
| // a fetch recently failed, the cached error is returned until the backoff | ||
| // interval expires. Otherwise, it initiates a new token fetch or blocks | ||
| // waiting for an already-in-progress fetch to complete. | ||
| func (c *gcpServiceAccountIdentityCallCreds) GetRequestMetadata(ctx context.Context, _ ...string) (map[string]string, error) { |
There was a problem hiding this comment.
| if c.isTokenValid() { | ||
| c.mu.Unlock() | ||
| return map[string]string{ | ||
| "authorization": "Bearer " + c.token.Value, |
There was a problem hiding this comment.
we are reading the token value outside the lock - can lead to data race. The unlock should be defer?
|
|
||
| fetching chan struct{} | ||
| nextRetryTime time.Time // When we can try next (backoff) | ||
| retryAttempt int // consecutive failures |
There was a problem hiding this comment.
how does the client know how many retry attempts are pending?
| c.fetching = make(chan struct{}) | ||
| c.mu.Unlock() | ||
|
|
||
| token, err := c.ts.TokenProvider.Token(context.Background()) |
There was a problem hiding this comment.
what happens if this gets stuck or hangs indefinitely? Can we configure a timeout for this context? Else all other go routines will simply keep waiting for this to come back.
| * | ||
| */ | ||
|
|
||
| package google |
There was a problem hiding this comment.
can we add a test to check the rejection over insecure connection.
|
Pull master to fix the checks. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces GcpServiceAccountIdentityCallCredentials, a new call credentials type for authenticating with GCP Service Account Identity tokens. The implementation correctly uses the cloud.google.com/go/auth/credentials/idtoken library and includes robust handling of token fetching, caching, concurrent requests, and error backoff. The accompanying tests are thorough and cover happy paths, error conditions, and concurrency scenarios.
I have a couple of suggestions to refactor parts of the GetRequestMetadata method to improve readability and reduce code duplication. These are minor improvements to an already solid implementation.
| c.mu.Lock() | ||
|
|
||
| if err != nil { | ||
| c.setBackoff(err) | ||
| close(c.fetching) | ||
| c.fetching = nil | ||
| c.mu.Unlock() | ||
| return nil, err | ||
| } | ||
|
|
||
| c.setBackoff(nil) | ||
| c.token = token | ||
| close(c.fetching) | ||
| c.fetching = nil | ||
| c.mu.Unlock() | ||
| return map[string]string{ | ||
| "authorization": "Bearer " + c.token.Value, | ||
| }, nil |
There was a problem hiding this comment.
The logic for handling the token fetch result can be simplified to reduce code duplication and improve readability. By using defer c.mu.Unlock() and consolidating the setBackoff call, you can make the code cleaner.
The setBackoff function already handles both error and non-error cases correctly, so you can call it once with the err from the token fetch.
| c.mu.Lock() | |
| if err != nil { | |
| c.setBackoff(err) | |
| close(c.fetching) | |
| c.fetching = nil | |
| c.mu.Unlock() | |
| return nil, err | |
| } | |
| c.setBackoff(nil) | |
| c.token = token | |
| close(c.fetching) | |
| c.fetching = nil | |
| c.mu.Unlock() | |
| return map[string]string{ | |
| "authorization": "Bearer " + c.token.Value, | |
| }, nil | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| close(c.fetching) | |
| c.fetching = nil | |
| c.setBackoff(err) | |
| if err != nil { | |
| return nil, err | |
| } | |
| c.token = token | |
| return map[string]string{ | |
| "authorization": "Bearer " + c.token.Value, | |
| }, nil |
| case <-wait: | ||
| return func() (map[string]string, error) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| if c.isTokenValid() { | ||
| return map[string]string{ | ||
| "authorization": "Bearer " + c.token.Value, | ||
| }, nil | ||
| } | ||
| return nil, c.lastErr | ||
| }() |
There was a problem hiding this comment.
The use of an immediately-invoked function expression here makes the code slightly harder to read. You can simplify this by moving the logic out of the anonymous function directly into the case block. This makes the control flow more straightforward.
case <-wait:
c.mu.Lock()
defer c.mu.Unlock()
if c.isTokenValid() {
return map[string]string{
"authorization": "Bearer " + c.token.Value,
}, nil
}
return nil, c.lastErr
This PR implements
GcpServiceAccountIdentityCallCredentials, a new call credentials type required by gRFC A83: xDS GCP Authentication Filter. This credential fetches and manages GCP Service Account Identity tokens for a given audience, allowing gRPC services running on GCP to authenticate RPCs.Implementation Details & gRFC Deviations
The gRFC provides detailed specifications for how this credential should manually fetch tokens from the GCE metadata server, extract the exp field, and manually calculate refresh intervals (e.g., refreshing 30 seconds early).
However, we have decided to use google auth library to fetch the token.
In Go, the standard Google API authentication packages (golang.org/x/oauth2/google) only provide access tokens, which are fundamentally different from the identity tokens required here.
To properly fetch identity tokens, we have decided to use the officially supported Google Auth library for Go:
cloud.google.com/go/auth/credentials/idtoken.The library causes the following behavioral difference from gRFC.
Behavioral Guarantees Implemented:
RELEASE NOTES: N/A