Feat/ambient credentials expiration

[irby]

Temp: Check for expiration of ambient credentials

[Copilot]

🟡 Changes recommended

The new ambient-token expiration handling currently logs at Info level without preventing use of an already-expired token, which can create noisy logs and lead to immediate runtime failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds diagnostic logging around ambient (cloud-provided) access token expiration when generating the Command OAuth configuration, and extends the Helm chart to optionally set a pod priorityClassName, alongside documentation/changelog updates.

Changes:

• Log the exp claim (when present) from ambient access tokens during server config creation.
• Add priorityClassName to the Helm chart values and render it into the Deployment pod spec when set.
• Update chart documentation, contributing guide structure, and bump the changelog to v2.6.1.

File summaries

File	Description
internal/command/command.go	Adds parsing/logging of ambient token expiration during OAuth server config generation.
deploy/charts/command-cert-manager-issuer/values.yaml	Introduces priorityClassName chart value (default empty).
deploy/charts/command-cert-manager-issuer/templates/deployment.yaml	Conditionally sets priorityClassName on the PodSpec.
deploy/charts/command-cert-manager-issuer/README.md	Updates the configuration table to reflect current chart values (including priorityClassName).
CONTRIBUTING.md	Restructures contribution/development guidance and updates requirements to match go.mod.
CHANGELOG.md	Adds v2.6.1 notes for the chart/docs updates.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new token source implementation has a confirmed Azure error-wrapping inconsistency and context-capture/documentation issues that can impact reliability when cached clients outlive the originating reconcile context.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

internal/command/client.go:168

• This comment references newAzureDefaultCredentialSource, but that function no longer exists after the refactor, so it's misleading/outdated.

	// Fail fast if the credentials/scopes are wrong, same as
	// newAzureDefaultCredentialSource does today.
	if _, err := src.Token(); err != nil {

internal/command/client.go:243

• This comment uses second-person phrasing ("your") and references newAzureDefaultCredentialSource, which doesn't exist anymore; please reword to be accurate and impersonal.

	// Fail fast if the credentials/scopes are wrong, same as your
	// newAzureDefaultCredentialSource does today.
	if _, err := src.Token(); err != nil {

internal/command/client.go:239

• Same caching issue as Azure: this source is stored inside a cached client, so capturing a cancelable reconcile ctx can make future token refreshes fail. Strip cancellation before storing ctx.

	src := &gcpTokenSource{
		ctx:      ctx,
		audience: audience,
		scopes:   scopes,
	}

• Files reviewed: 8/9 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new token-source implementation introduces security/performance concerns (JWT claims logged at Info level; GCP TokenSource mutex serializes requests) and lacks accompanying tests for the new rotation/refresh behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (3) — in code that hasn't changed since the last review.

internal/command/client.go:124

• This stores the full Azure access token string in-memory in a.last just to detect rotation, which unnecessarily retains a secret. Prefer tracking a non-secret value (e.g., the expiry timestamp) for rotation detection.

This issue also appears on line 215 of the same file.
internal/command/client.go:132

• JWT claim logging is emitted at Info level, which can leak identity details (e.g., oid/email/appid) into normal logs. Consider moving this block to a verbose logger (e.g., V(1)+) so it’s only shown when explicitly enabled.

This issue also appears on line 224 of the same file.
CHANGELOG.md:4

• The PR description focuses on ambient credential expiration/refresh, but the v2.6.1 changelog entry doesn’t mention that behavior change. Adding a brief feature bullet will help users understand why this release is needed.

internal/command/client.go:227

• JWT claim logging is emitted at Info level, which can leak identity details (e.g., email/sub) into normal logs. Consider moving this block to a verbose logger (e.g., V(1)+) so it’s only shown when explicitly enabled.

		log.Info("==== BEGIN DEBUG: Default Google ID Token JWT ======")
		printClaims(log, token.AccessToken, []string{"aud", "iss", "sub", "email"})
		log.Info("==== END DEBUG:  Default Google ID Token JWT ======")
		g.claimsShown = true

internal/command/client.go:218

• This stores the full GCP ID token string in-memory in g.last just to detect rotation, which unnecessarily retains a secret. Prefer tracking a non-secret value (e.g., the expiry timestamp) for rotation detection.

	if token.AccessToken != g.last {
		log.Info(fmt.Sprintf("Access token issued from GCP. Token expires at UTC time %s", token.Expiry.UTC().Format(time.RFC3339)))
		g.last = token.AccessToken
	}

internal/command/client.go:146

• New ambient token source behavior (refreshable oauth2.TokenSource, rotation/claims logging) is not covered by unit tests, even though internal/command already has tests. Consider adding tests using a fake azcore.TokenCredential and a fake oauth2.TokenSource to verify rotation detection and one-time claim logging without requiring real cloud credentials.

func newAzureTokenSource(ctx context.Context, scopes []string) (oauth2.TokenSource, error) {
	log := log.FromContext(ctx)
	log.Info("creating new Azure Default Token Source")

• Files reviewed: 8/9 changed files
• Comments generated: 1
• Review effort level: Lite