fix: #23100 Change workloadidentity token cache expiry based on token expiry. #23133

jagpreetstamber · 2025-05-23T14:38:31Z

Different Microsoft Entra tenants can enforce different lifetimes for managed identity tokens, default is 3 hours, using which earlier cache expiry was set. Lowering it to 30 minutes to avoid any issues with Entra tenants setting the token expiry to 1 hour.

Closes #23100

Checklist:

bunnyshell · 2025-05-23T14:38:36Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

crenshaw-dev · 2025-05-23T14:53:44Z

Shouldn't we just use the token's expiry? Something like this:

diff --git a/util/git/creds.go b/util/git/creds.go
index 4657c4a20..9e48a3bc9 100644
--- a/util/git/creds.go
+++ b/util/git/creds.go
@@ -743,8 +743,8 @@ func (creds AzureWorkloadIdentityCreds) getAccessToken(scope string) (string, er
 		return "", fmt.Errorf("failed to get Azure access token: %w", err)
 	}
 
-	azureTokenCache.Set(key, token, 2*time.Hour)
-	return token, nil
+	azureTokenCache.Set(key, token, token.ExpiresOn.Sub(time.Now()))
+	return token.Token, nil
 }
 
 func (creds AzureWorkloadIdentityCreds) GetAzureDevOpsAccessToken() (string, error) {
diff --git a/util/workloadidentity/workloadidentity.go b/util/workloadidentity/workloadidentity.go
index 24aef2ffa..64679b02c 100644
--- a/util/workloadidentity/workloadidentity.go
+++ b/util/workloadidentity/workloadidentity.go
@@ -13,7 +13,7 @@ const (
 )
 
 type TokenProvider interface {
-	GetToken(scope string) (string, error)
+	GetToken(scope string) (azcore.AccessToken, error)
 }
 
 type WorkloadIdentityTokenProvider struct {
@@ -29,17 +29,17 @@ func NewWorkloadIdentityTokenProvider() TokenProvider {
 	return WorkloadIdentityTokenProvider{tokenCredential: cred}
 }
 
-func (c WorkloadIdentityTokenProvider) GetToken(scope string) (string, error) {
+func (c WorkloadIdentityTokenProvider) GetToken(scope string) (azcore.AccessToken, error) {
 	if initError != nil {
-		return "", initError
+		return azcore.AccessToken{}, initError
 	}
 
 	token, err := c.tokenCredential.GetToken(context.Background(), policy.TokenRequestOptions{
 		Scopes: []string{scope},
 	})
 	if err != nil {
-		return "", err
+		return azcore.AccessToken{}, err
 	}
 
-	return token.Token, nil
+	return token, nil
 }

jagpreetstamber · 2025-05-23T14:56:49Z

+func (c WorkloadIdentityTokenProvider) GetToken(scope string) (azcore.AccessToken, error) {

Makes sense, I will update he PR

codecov · 2025-05-23T15:46:21Z

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.08%. Comparing base (b39ca15) to head (48ba4e2).
⚠️ Report is 413 commits behind head on master.

Files with missing lines	Patch %	Lines
util/helm/creds.go	86.36%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #23133      +/-   ##
==========================================
+ Coverage   60.01%   60.08%   +0.06%     
==========================================
  Files         342      342              
  Lines       57845    57872      +27     
==========================================
+ Hits        34718    34771      +53     
+ Misses      20352    20331      -21     
+ Partials     2775     2770       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

util/helm/creds.go