Skip to content

fix(auth): keep realm isolation opt-in for gcp to preserve legacy paths#2085

Merged
aknysh merged 11 commits intocloudposse:mainfrom
shirkevich:fix/auth-realm-mandatory
Feb 19, 2026
Merged

fix(auth): keep realm isolation opt-in for gcp to preserve legacy paths#2085
aknysh merged 11 commits intocloudposse:mainfrom
shirkevich:fix/auth-realm-mandatory

Conversation

@shirkevich
Copy link
Contributor

@shirkevich shirkevich commented Feb 17, 2026

Summary

Align GCP auth behavior with #2075: realm isolation is opt-in and empty realm keeps backward-compatible paths.

What changed

  • When neither auth.realm nor ATMOS_AUTH_REALM is set, GCP credential files now use legacy paths without a realm segment.
  • Updated GCP service-account realm handling to preserve empty realm behavior.
  • Updated GCP path helpers/docs/comments to treat realm as optional in legacy mode.
  • Updated tests to enforce empty-realm backward compatibility and removed auto-realm assumptions.

Why

PR #2075 restored backward-compatible behavior after #2043 by making realm isolation opt-in. This PR ensures GCP follows the same model and does not reintroduce always-on realm isolation semantics.

Summary by CodeRabbit

  • New Features

    • Added support for legacy credential paths when realm is empty, enabling backward-compatible configurations.
    • Enhanced GCP authentication with improved Application Default Credentials (ADC) handling.
  • Bug Fixes

    • Relaxed realm requirement to allow empty values, improving flexibility in credential path management.
  • Tests

    • Added comprehensive test coverage for GCP ADC authentication flows and realm handling scenarios.

shirkevich and others added 2 commits February 17, 2026 18:27
…lm for GCP

The mandatory empty-realm rejection in NewAuthManager (added in 0848941)
was overly broad — it broke AWS/Azure users who don't configure auth.realm.

Changes:
- Remove the blanket empty-realm rejection from NewAuthManager. Empty realm
  is now allowed; AWS/Azure handle it gracefully via filepath.Join.
- GCP service account identity: when realm is empty, auto-generate a
  deterministic SHA-256 based realm from ServiceAccountEmail (first 8 bytes,
  prefixed with "auto-"). This ensures unique credential paths per account
  without requiring explicit auth.realm configuration.
- GCP still errors if both realm AND email are empty (no fallback possible).
- Update tests: requireRealm() with valid email now succeeds, empty-email
  test still asserts ErrEmptyRealm.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 35 tests across 4 files covering the ADC authentication flow:
- Provider layer: stateless behavior, realm independence, lifecycle
- GCP project identity: credential passthrough, realm independence
- GCP service account identity: impersonation, auto-realm isolation
- Manager level: chain building, whoami, multi-identity scenarios

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@shirkevich shirkevich requested a review from a team as a code owner February 17, 2026 15:40
@github-actions github-actions bot added the size/xl Extra large size PR label Feb 17, 2026
@mergify
Copy link

mergify bot commented Feb 17, 2026

Warning

This PR exceeds the recommended limit of 1,000 lines.

Large PRs are difficult to review and may be rejected due to their size.

Please verify that this PR does not address multiple issues.
Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

@aknysh aknysh added the patch A minor, backward compatible change label Feb 17, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 17, 2026

📝 Walkthrough

Walkthrough

Allows empty propagated realms from the manager and makes realm handling conditional downstream; adds extensive ADC-focused tests for GCP project and service-account identities, ADC provider, provider/identity chain resolution, and manager integrations. No public API signatures changed.

Changes

Cohort / File(s) Summary
GCP Service Account Identity - Logic & docs
pkg/auth/identities/gcp_service_account/identity.go
Relaxed realm enforcement: empty realm is allowed and handled by identity-level logic and docs updated to describe realm-as-optional behavior.
GCP Project Identity Tests
pkg/auth/identities/gcp_project/identity_test.go
Adds ADC-based tests: credential passthrough, project override, env behavior, lifecycle and realm independence.
GCP Service Account Identity Tests
pkg/auth/identities/gcp_service_account/identity_test.go
Extensive ADC impersonation and empty-realm tests, realm determinism/isolation checks, auth lifecycle and delegate-chain scenarios; many new test cases and assertions.
Auth Manager: realm acceptance & tests
pkg/auth/manager.go, pkg/auth/manager_extended_test.go
Stops enforcing non-empty realm at manager level; comment and test description updated to reflect empty realm propagation.
Manager GCP ADC Integration Tests
pkg/auth/manager_gcp_adc_test.go
New comprehensive manager-level ADC tests: provider/identity registration, chain construction (ADC→project, ADC→service-account, chaining), realm propagation, env extraction, provider listing, and error cases.
ADC Provider Tests
pkg/auth/providers/gcp_adc/provider_test.go
Adds stateless and realm-independent ADC provider tests: prepare environment, env preservation, project extraction, email handling, and full lifecycle checks.
GCP cloud files & path handling
pkg/auth/cloud/gcp/files.go, pkg/auth/cloud/gcp/files_test.go, pkg/auth/cloud/gcp/setup.go, pkg/auth/cloud/gcp/doc.go
GetProviderDir and related comments updated to accept empty realm (legacy path behavior); tests adjusted to expect legacy path when realm is empty; docs updated.
Realm package docs & tests
pkg/auth/realm/realm.go, pkg/auth/realm/realm_test.go
Comments and test messages updated to state empty realm yields legacy/identity-handled fallback; no functional API changes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuthManager
  participant ADC as "GCP ADC Provider"
  participant Identity as "Identity (gcp_project / gcp_service_account)"
  participant Store as "CredentialStore"

  Client->>AuthManager: Request auth (ADC flow)
  AuthManager->>ADC: Authenticate via ADC provider
  ADC-->>AuthManager: Token, ProjectID, Email
  AuthManager->>Identity: Build chain / forward ADC creds
  Identity->>Identity: resolve realm (empty -> identity-level fallback)
  Identity->>Store: Persist credentials under resolved path
  Store-->>AuthManager: Ack
  AuthManager-->>Client: Return auth result (env, project, token)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • aknysh
  • osterman
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 74.19% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: making realm isolation opt-in for GCP to preserve legacy paths without realm subdirectories when realm is unset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)

390-584: Solid ADC critical-path test coverage for gcp/project.

Tests cover passthrough, no-base-creds, full lifecycle, and realm-independence well. The use of t.Setenv in the lifecycle test (line 461-463) is good practice.

Minor note: bare type assertions like creds.(*types.GCPCredentials) at lines 452, 485, 541, 567 will panic on wrong type rather than producing a clean test failure. Consider using the two-value form with require.True(t, ok) as done elsewhere in this file (e.g., line 425-426). Not a blocker since panics in tests still surface failures.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 390 - 584,
Replace the bare type assertions that may panic with the safe two-value form and
assert the success before using the value: where tests call
creds.(*types.GCPCredentials) or loadedCreds.(*types.GCPCredentials) (e.g., in
TestADC_ProjectIdentity_PassthroughCreds,
TestADC_ProjectIdentity_EmptyRealm_FullLifecycle,
TestADC_ProjectIdentity_NoBaseCreds, and
TestADC_ProjectIdentity_RealmIndependent), change them to v, ok :=
creds.(*types.GCPCredentials) (or loadedCreds) and add require.True(t, ok)
immediately after (keeping the existing require.NoError checks), then use v
instead of the original assertion.
pkg/auth/manager.go (1)

134-137: Correct removal of mandatory realm enforcement.

Letting identity types handle their own realm requirements is a clean separation of concerns.

The comments here and at line 560-562 mention specific cloud providers (AWS/Azure/GCP). Per prior feedback, the auth manager should remain cloud-agnostic. Consider generalizing these comments, e.g., "Identity types that require filesystem isolation generate fallback realms internally." Not a blocker.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/manager.go` around lines 134 - 137, Update the provider-specific
comments in pkg/auth/manager.go to be cloud-agnostic: replace mentions of
AWS/Azure/GCP with a general explanation that empty realms are allowed for
backward compatibility and that identity types which require filesystem
isolation should generate fallback realms internally (e.g., SHA-based from their
identifiers). Locate the comment block near the auth manager logic (around the
realm handling in manager.go) and the similar comment later in the same file
(the block referencing "fallback realm"), and change both to the generalized
wording so the auth manager does not reference specific cloud providers.
pkg/auth/identities/gcp_service_account/identity_test.go (1)

995-995: Consider guarding type assertions with require.IsType.

Bare type assertions like loaded.(*types.GCPCredentials) will panic with an unhelpful stack trace if the type is wrong. A require.IsType(t, &types.GCPCredentials{}, loaded) before the assertion gives a clearer failure message. Minor nit — the pattern is used elsewhere in this file too.

Also applies to: 1079-1079, 1083-1083

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` at line 995, The
test uses a bare type assertion loaded.(*types.GCPCredentials) which can panic;
add a type guard using require.IsType(t, &types.GCPCredentials{}, loaded)
immediately before performing the assertion (e.g., replace or precede the line
assigning loadedGCP) so failures produce clear messages; apply the same change
to the other occurrences noted (the assertions at the positions around lines
referencing loaded, e.g., the other casts to *types.GCPCredentials in this file)
to ensure all type assertions are guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 390-584: Replace the bare type assertions that may panic with the
safe two-value form and assert the success before using the value: where tests
call creds.(*types.GCPCredentials) or loadedCreds.(*types.GCPCredentials) (e.g.,
in TestADC_ProjectIdentity_PassthroughCreds,
TestADC_ProjectIdentity_EmptyRealm_FullLifecycle,
TestADC_ProjectIdentity_NoBaseCreds, and
TestADC_ProjectIdentity_RealmIndependent), change them to v, ok :=
creds.(*types.GCPCredentials) (or loadedCreds) and add require.True(t, ok)
immediately after (keeping the existing require.NoError checks), then use v
instead of the original assertion.

In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Line 995: The test uses a bare type assertion loaded.(*types.GCPCredentials)
which can panic; add a type guard using require.IsType(t,
&types.GCPCredentials{}, loaded) immediately before performing the assertion
(e.g., replace or precede the line assigning loadedGCP) so failures produce
clear messages; apply the same change to the other occurrences noted (the
assertions at the positions around lines referencing loaded, e.g., the other
casts to *types.GCPCredentials in this file) to ensure all type assertions are
guarded.

In `@pkg/auth/manager.go`:
- Around line 134-137: Update the provider-specific comments in
pkg/auth/manager.go to be cloud-agnostic: replace mentions of AWS/Azure/GCP with
a general explanation that empty realms are allowed for backward compatibility
and that identity types which require filesystem isolation should generate
fallback realms internally (e.g., SHA-based from their identifiers). Locate the
comment block near the auth manager logic (around the realm handling in
manager.go) and the similar comment later in the same file (the block
referencing "fallback realm"), and change both to the generalized wording so the
auth manager does not reference specific cloud providers.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 17, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)

554-555: Nit: empty-string realm produces a less readable subtest name.

"realm=" in test output is easy to miss. Consider mapping the empty case to a display label like "empty".

Suggested tweak
 for _, realmValue := range realms {
-	t.Run("realm="+realmValue, func(t *testing.T) {
+	displayName := realmValue
+	if displayName == "" {
+		displayName = "empty"
+	}
+	t.Run("realm="+displayName, func(t *testing.T) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 554 - 555, The
subtest name uses "realm="+realmValue which is hard to read when realmValue is
empty; change the test to compute a display label (e.g., display := realmValue;
if display == "" { display = "empty" }) and call t.Run("realm="+display, func(t
*testing.T) { ... }) while continuing to use realmValue inside the test body for
assertions; update the loop around realms and the t.Run invocation accordingly.
pkg/auth/identities/gcp_service_account/identity_test.go (2)

898-943: Solid impersonation flow test.

One small note: line 928 sets ServiceAccountEmail: "developer@company.com" on the ADC base credentials. That's not a valid GCP service account email format (no .iam.gserviceaccount.com suffix). That's fine here since it's the user's ADC identity, not a service account — but a brief inline comment would help future readers understand why a non-SA email is intentional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 898 -
943, Add a brief inline comment in TestADC_ServiceAccount_ImpersonationFlow next
to the adcBaseCreds.ServiceAccountEmail assignment explaining that this value
intentionally omits the .iam.gserviceaccount.com suffix because it represents a
user ADC identity (gcloud user credentials) rather than a service account;
update the comment near the adcBaseCreds variable (or the ServiceAccountEmail
field) so future readers understand why a non-service-account email is used in
this test.

458-494: Deterministic and uniqueness tests are well-structured.

These two tests nail the key guarantees of SHA-based realm generation: same input → same output, different input → different output. Consider also asserting the expected exact realm value for at least one known email to serve as a golden-value regression test — if the hashing logic ever changes accidentally, a prefix-only check won't catch it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 458 -
494, Add a golden-value assertion to lock the exact generated realm for a known
email (so accidental changes to the hashing logic are caught). Update
TestRequireRealm_Deterministic (or add a small new test e.g.
TestRequireRealm_GoldenValue) to call Identity.requireRealm for a chosen
canonical email (use the existing
GCPServiceAccountIdentityPrincipal.ServiceAccountEmail value), run it locally
once to capture the current realm string, then replace the runtime equality
assertion with an assert.Equal against that hard-coded expected realm literal
while keeping the require.NoError checks; refer to Identity.requireRealm and the
Identity/principal setup to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 554-555: The subtest name uses "realm="+realmValue which is hard
to read when realmValue is empty; change the test to compute a display label
(e.g., display := realmValue; if display == "" { display = "empty" }) and call
t.Run("realm="+display, func(t *testing.T) { ... }) while continuing to use
realmValue inside the test body for assertions; update the loop around realms
and the t.Run invocation accordingly.

In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 898-943: Add a brief inline comment in
TestADC_ServiceAccount_ImpersonationFlow next to the
adcBaseCreds.ServiceAccountEmail assignment explaining that this value
intentionally omits the .iam.gserviceaccount.com suffix because it represents a
user ADC identity (gcloud user credentials) rather than a service account;
update the comment near the adcBaseCreds variable (or the ServiceAccountEmail
field) so future readers understand why a non-service-account email is used in
this test.
- Around line 458-494: Add a golden-value assertion to lock the exact generated
realm for a known email (so accidental changes to the hashing logic are caught).
Update TestRequireRealm_Deterministic (or add a small new test e.g.
TestRequireRealm_GoldenValue) to call Identity.requireRealm for a chosen
canonical email (use the existing
GCPServiceAccountIdentityPrincipal.ServiceAccountEmail value), run it locally
once to capture the current realm string, then replace the runtime equality
assertion with an assert.Equal against that hard-coded expected realm literal
while keeping the require.NoError checks; refer to Identity.requireRealm and the
Identity/principal setup to locate the code.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 17, 2026
@codecov
Copy link

codecov bot commented Feb 17, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.21%. Comparing base (501dadc) to head (a5d012e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/auth/cloud/gcp/files.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2085      +/-   ##
==========================================
+ Coverage   76.20%   76.21%   +0.01%     
==========================================
  Files         829      829              
  Lines       78555    78551       -4     
==========================================
+ Hits        59859    59867       +8     
+ Misses      14965    14946      -19     
- Partials     3731     3738       +7     
Flag Coverage Δ
unittests 76.21% <33.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/auth/cloud/gcp/setup.go 78.94% <ø> (ø)
...kg/auth/identities/gcp_service_account/identity.go 75.74% <ø> (-4.75%) ⬇️
pkg/auth/manager.go 80.89% <ø> (+0.17%) ⬆️
pkg/auth/realm/realm.go 96.36% <ø> (ø)
pkg/auth/cloud/gcp/files.go 59.44% <33.33%> (-0.90%) ⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aknysh
Copy link
Member

aknysh commented Feb 17, 2026

@shirkevich apologies again, but please review the following:

in #2075, we implemented:

Problem: CI/CD pipelines fail with AWS credential errors because credential file paths changed from {baseDir}/aws/{provider}/credentials to {baseDir}/{realm}/aws/{provider}/credentials.

Cause: PR https://github.com/cloudposse/atmos/pull/2043 auto-generated a realm from a SHA256 hash of the CLI config path, making realm isolation always-on.

Fix: Made realm isolation opt-in. When auth.realm is not configured and ATMOS_AUTH_REALM is not set, credentials use the backward-compatible path with no realm subdirectory. Also fixed CleanupAll() to safely handle empty realm.

but in this PR we are putting it back:

SHA-Based Auto-Realm (gcp_service_account/identity.go)
requireRealm() generates auto-<sha256(ServiceAccountEmail)[:8]> when realm is empty
Deterministic: same email always produces same realm
Collision-resistant: 32-bit hex space (4B+ values)
Only applies to gcp/service-account — ADC provider and gcp/project identity remain fully stateless and realm-independent

can you please make it consistent?

When auth.realm is not configured and ATMOS_AUTH_REALM is not set, credentials use the backward-compatible path with no realm subdirectory

Please point Claude at this and it will fix it. Also ask to review and add tests.

Thanks!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)

438-456: Near-duplicate of TestAuthenticate_NoBaseCreds (lines 159–175).

The only meaningful addition here is SetRealm(""). Consider whether this warrants a standalone test or could be folded into the existing one. Not a big deal — it works as ADC-group documentation — but worth noting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 438 - 456, The
test TestADC_ProjectIdentity_NoBaseCreds is a near-duplicate of
TestAuthenticate_NoBaseCreds; remove the duplication by either deleting
TestADC_ProjectIdentity_NoBaseCreds or folding its unique assertion (the call to
id.SetRealm("") and subsequent checks) into TestAuthenticate_NoBaseCreds as an
additional subtest or branch; ensure you preserve the identity creation, the
call to Authenticate, and the assertions on returned
*types.GCPCredentials.ProjectID and AccessToken so behavior stays covered.
pkg/auth/identities/gcp_service_account/identity_test.go (2)

124-144: Manual mock — consider migrating to mockgen when convenient.

MockProvider is a hand-rolled mock. The coding guidelines call for go.uber.org/mock/mockgen with //go:generate directives. This is pre-existing, but since the new ADC tests (lines 903+) depend on it, it's worth flagging for a future cleanup pass. No urgency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 124 -
144, Replace the hand-written MockProvider with a mock generated by
go.uber.org/mock/mockgen: add a //go:generate directive above the provider
interface declaration, run mockgen to produce a generated mock (e.g.,
MockProvider) and commit the generated file, then update tests that currently
import or instantiate the manual MockProvider to use the generated mock type;
reference the existing MockProvider symbol to locate the current mock to remove
and the ADC-related tests that rely on it to update their instantiation and
expectations.

458-476: Consider adding a comment to the golden value explaining its derivation.

The golden value "auto-b21924d31a143eec" correctly represents SHA256 of the test email's first 8 bytes as hex. The implementation (identity.go:88-89) is clear and the test structure is solid. Adding a brief comment like // SHA256("sa@proj.iam.gserviceaccount.com")[:8] as hex on line 475 would help future maintainers quickly understand and update the value if the hashing logic ever changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 458 -
476, Add a clarifying comment next to the golden string in
TestRequireRealm_Deterministic that documents how it was derived: note that the
expected value "auto-b21924d31a143eec" is formed by taking
SHA256("sa@proj.iam.gserviceaccount.com") and using the first 8 bytes as hex
(matching the logic in requireRealm/identity.go). Update the test function
TestRequireRealm_Deterministic to include this one-line comment immediately
above or beside the golden value so future maintainers can verify or regenerate
the expected value easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 461-464: The test's environment cleanup is incomplete:
PostAuthenticate (which calls os.Setenv for all keys returned by
id.Environment()) sets GCLOUD_PROJECT, CLOUDSDK_COMPUTE_REGION,
GOOGLE_CLOUD_ZONE, and CLOUDSDK_COMPUTE_ZONE too, but the teardown only uses
t.Setenv for GOOGLE_CLOUD_PROJECT, CLOUDSDK_CORE_PROJECT and
GOOGLE_CLOUD_REGION; update the cleanup in identity_test.go to t.Setenv for the
missing keys (GCLOUD_PROJECT, CLOUDSDK_COMPUTE_REGION, GOOGLE_CLOUD_ZONE,
CLOUDSDK_COMPUTE_ZONE) — or iterate id.Environment() in the test and call
t.Setenv for each key to ensure all env vars set by PostAuthenticate are
protected from leakage.

---

Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 438-456: The test TestADC_ProjectIdentity_NoBaseCreds is a
near-duplicate of TestAuthenticate_NoBaseCreds; remove the duplication by either
deleting TestADC_ProjectIdentity_NoBaseCreds or folding its unique assertion
(the call to id.SetRealm("") and subsequent checks) into
TestAuthenticate_NoBaseCreds as an additional subtest or branch; ensure you
preserve the identity creation, the call to Authenticate, and the assertions on
returned *types.GCPCredentials.ProjectID and AccessToken so behavior stays
covered.

In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 124-144: Replace the hand-written MockProvider with a mock
generated by go.uber.org/mock/mockgen: add a //go:generate directive above the
provider interface declaration, run mockgen to produce a generated mock (e.g.,
MockProvider) and commit the generated file, then update tests that currently
import or instantiate the manual MockProvider to use the generated mock type;
reference the existing MockProvider symbol to locate the current mock to remove
and the ADC-related tests that rely on it to update their instantiation and
expectations.
- Around line 458-476: Add a clarifying comment next to the golden string in
TestRequireRealm_Deterministic that documents how it was derived: note that the
expected value "auto-b21924d31a143eec" is formed by taking
SHA256("sa@proj.iam.gserviceaccount.com") and using the first 8 bytes as hex
(matching the logic in requireRealm/identity.go). Update the test function
TestRequireRealm_Deterministic to include this one-line comment immediately
above or beside the golden value so future maintainers can verify or regenerate
the expected value easily.

@shirkevich shirkevich changed the title fix: remove mandatory realm from auth manager, add SHA-based auto-realm fix(auth): keep realm isolation opt-in for gcp to preserve legacy paths Feb 18, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/auth/identities/gcp_service_account/identity_test.go (1)

751-788: ⚠️ Potential issue | 🟡 Minor

Missing XDG_CONFIG_HOME isolation in four sub-tests — real filesystem pollution.

The PrepareEnvironment, Logout, CredentialsExist, and LoadCredentials sub-tests don't set XDG_CONFIG_HOME. All of them eventually call GetProviderDir which invokes os.MkdirAll, creating ~/.config/atmos/gcp/gcp-adc/ in the real filesystem. The PostAuthenticate sub-test correctly sets it; the others should too.

🛠️ Proposed fix — add temp dir isolation to each affected sub-test
 t.Run("PrepareEnvironment", func(t *testing.T) {
+    tmp := t.TempDir()
+    t.Setenv("XDG_CONFIG_HOME", tmp)
     id := makeIdentity()
     _, err := id.PrepareEnvironment(context.Background(), nil)
 t.Run("Logout", func(t *testing.T) {
+    tmp := t.TempDir()
+    t.Setenv("XDG_CONFIG_HOME", tmp)
     id := makeIdentity()
     err := id.Logout(context.Background())
 t.Run("CredentialsExist", func(t *testing.T) {
+    tmp := t.TempDir()
+    t.Setenv("XDG_CONFIG_HOME", tmp)
     id := makeIdentity()
 t.Run("LoadCredentials", func(t *testing.T) {
+    tmp := t.TempDir()
+    t.Setenv("XDG_CONFIG_HOME", tmp)
     id := makeIdentity()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 751 -
788, The tests PrepareEnvironment, Logout, CredentialsExist, and LoadCredentials
call GetProviderDir which creates real ~/.config entries; to avoid filesystem
pollution, in each of those sub-tests (where you call makeIdentity() and
id.PrepareEnvironment / id.Logout / id.CredentialsExist / id.LoadCredentials)
create a temp dir via tmp := t.TempDir() and set it with
t.Setenv("XDG_CONFIG_HOME", tmp) (same pattern as the PostAuthenticate sub-test)
so the provider dir is created inside the test temp directory instead of the
real user config directory.
pkg/auth/cloud/gcp/files.go (1)

293-296: ⚠️ Potential issue | 🟡 Minor

Stale comment: WriteAccessTokenFile still says realm is required.

The comment on line 295 ("Realm is required for credential isolation.") contradicts the new opt-in behavior. Same for CleanupIdentityFiles at line 331. Per coding guidelines, comments should be updated to match the code when refactoring.

✏️ Proposed fix
 // WriteAccessTokenFile writes a simple access token file for tools that need it.
 // Uses file locking to prevent concurrent modification conflicts.
-// Realm is required for credential isolation.
+// Realm is optional; empty realm uses legacy paths without a realm subdirectory.
 // CleanupIdentityFiles removes all credential files for an identity.
 // Uses file locking to prevent concurrent modification conflicts.
-// Realm is required for credential isolation.
+// Realm is optional; empty realm uses legacy paths without a realm subdirectory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/auth/cloud/gcp/files.go` around lines 293 - 296, The doc comments for
WriteAccessTokenFile and CleanupIdentityFiles are stale — they state "Realm is
required for credential isolation." but the functions now use opt-in behavior;
update both function comments (the comment blocks above WriteAccessTokenFile and
CleanupIdentityFiles) to reflect that realm is optional and only used for
credential isolation when provided (or describe the new opt-in semantics),
removing the assertion that realm is required and ensuring wording matches
current parameter behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@pkg/auth/cloud/gcp/files.go`:
- Around line 293-296: The doc comments for WriteAccessTokenFile and
CleanupIdentityFiles are stale — they state "Realm is required for credential
isolation." but the functions now use opt-in behavior; update both function
comments (the comment blocks above WriteAccessTokenFile and
CleanupIdentityFiles) to reflect that realm is optional and only used for
credential isolation when provided (or describe the new opt-in semantics),
removing the assertion that realm is required and ensuring wording matches
current parameter behavior.

In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 751-788: The tests PrepareEnvironment, Logout, CredentialsExist,
and LoadCredentials call GetProviderDir which creates real ~/.config entries; to
avoid filesystem pollution, in each of those sub-tests (where you call
makeIdentity() and id.PrepareEnvironment / id.Logout / id.CredentialsExist /
id.LoadCredentials) create a temp dir via tmp := t.TempDir() and set it with
t.Setenv("XDG_CONFIG_HOME", tmp) (same pattern as the PostAuthenticate sub-test)
so the provider dir is created inside the test temp directory instead of the
real user config directory.

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @shirkevich

@aknysh aknysh merged commit 761d419 into cloudposse:main Feb 19, 2026
55 checks passed
@github-actions
Copy link

These changes were released in v1.207.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch A minor, backward compatible change size/xl Extra large size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments