From 045567d438966f17f090d2dcd492905ff3ab0a9d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:51:58 +0200 Subject: [PATCH 1/3] test(secrets): stop Azure tests from using live credential chain The azure_resolver_coverage tests called the real NewAzureResolver and made requests to test-vault.vault.azure.net. The Key Vault 401 challenge then walked the live DefaultAzureCredential chain: IMDS probe, repeated `az account get-access-token` calls, and `pwsh Get-AzAccessToken`, which reads the MSAL cache from the macOS login keychain and pops an interactive password prompt during every `go test ./...` run. The intended t.Skipf guards never fired because credential construction succeeds on any dev box. - Rewrite the live tests onto the existing fake-credential httptest harness; constructor-only tests keep exercising NewAzureResolver but make no requests and now fail loud instead of silently skipping. - Add TestMain pinning AZURE_TOKEN_CREDENTIALS=EnvironmentCredential with dummy env credentials so no test in the package can reach the az/azd/pwsh/IMDS legs, plus a regression guard test that fails if the pin is removed. Package suite now runs offline in ~4s (was minutes of network retries); verified zero az/pwsh invocations during the run. --- .../secrets/azure_credential_guard_test.go | 35 ++++ .../secrets/azure_resolver_coverage_test.go | 163 +++++++----------- 2 files changed, 97 insertions(+), 101 deletions(-) create mode 100644 internal/secrets/azure_credential_guard_test.go diff --git a/internal/secrets/azure_credential_guard_test.go b/internal/secrets/azure_credential_guard_test.go new file mode 100644 index 000000000..a12be6dd3 --- /dev/null +++ b/internal/secrets/azure_credential_guard_test.go @@ -0,0 +1,35 @@ +package secrets + +import ( + "os" + "testing" +) + +// TestMain pins the Azure credential environment for every test in this +// package so no test can walk the live DefaultAzureCredential fallback chain +// (IMDS probe, `az account get-access-token`, `pwsh Get-AzAccessToken`). The +// pwsh leg reads the MSAL token cache from the macOS login keychain, which +// pops an interactive password prompt on developer machines whenever +// `go test ./...` runs. +// +// AZURE_TOKEN_CREDENTIALS restricts the chain to EnvironmentCredential only +// (supported since azidentity v1.10), and the dummy client-secret variables +// let that credential construct without contacting anything. A test that +// accidentally acquires a real token fails fast against the dummy tenant +// instead of harvesting local developer credentials. +func TestMain(m *testing.M) { + os.Setenv("AZURE_TOKEN_CREDENTIALS", "EnvironmentCredential") + os.Setenv("AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000") + os.Setenv("AZURE_CLIENT_ID", "00000000-0000-0000-0000-000000000000") + os.Setenv("AZURE_CLIENT_SECRET", "dummy-test-client-secret") + os.Exit(m.Run()) +} + +// TestAzureCredentialChainRestricted is the regression guard for the keychain +// prompt incident: it fails if TestMain stops pinning the restricted +// credential chain. +func TestAzureCredentialChainRestricted(t *testing.T) { + if got := os.Getenv("AZURE_TOKEN_CREDENTIALS"); got != "EnvironmentCredential" { + t.Fatalf("AZURE_TOKEN_CREDENTIALS = %q; TestMain must pin it to EnvironmentCredential so tests never invoke the az/pwsh/IMDS credential legs", got) + } +} diff --git a/internal/secrets/azure_resolver_coverage_test.go b/internal/secrets/azure_resolver_coverage_test.go index 2b4cdb6e0..ce366ccc6 100644 --- a/internal/secrets/azure_resolver_coverage_test.go +++ b/internal/secrets/azure_resolver_coverage_test.go @@ -2,6 +2,8 @@ package secrets import ( "context" + "encoding/json" + "net/http" "testing" "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" @@ -9,16 +11,23 @@ import ( "github.com/stretchr/testify/require" ) -// TestAzureResolver_DirectMethods tests the actual AzureResolver methods -// These tests exercise the real code paths but may skip if Azure credentials are unavailable. +// notFoundHandler simulates the Key Vault response for a missing secret. +func notFoundHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{"code": "SecretNotFound", "message": "Secret not found"}, + }) +} + +// TestAzureResolver_DirectMethods tests the real constructor wiring. It makes +// no requests, and TestMain pins the credential chain to a dummy +// EnvironmentCredential, so construction is deterministic and offline. func TestAzureResolver_DirectMethods(t *testing.T) { ctx := context.Background() - // Try to create an Azure resolver resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } + require.NoError(t, err) defer resolver.Close() // Test that the resolver is properly configured @@ -28,89 +37,67 @@ func TestAzureResolver_DirectMethods(t *testing.T) { // TestAzureResolver_GetSecret_NonExistent tests getting a non-existent secret. func TestAzureResolver_GetSecret_NonExistent(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // Try to get a secret that definitely doesn't exist - _, err = resolver.GetSecret(ctx, "cudly-test-nonexistent-secret-12345-xyz") + _, err := resolver.GetSecret(context.Background(), "cudly-test-nonexistent-secret-12345-xyz") - // Should get an error (either not found or permission denied) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get secret") } // TestAzureResolver_GetSecretJSON_NonExistent tests getting a non-existent JSON secret. func TestAzureResolver_GetSecretJSON_NonExistent(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // Try to get a JSON secret that doesn't exist - result, err := resolver.GetSecretJSON(ctx, "cudly-test-nonexistent-json-secret-12345-xyz") + result, err := resolver.GetSecretJSON(context.Background(), "cudly-test-nonexistent-json-secret-12345-xyz") - // Should get an error assert.Error(t, err) assert.Nil(t, result) } // TestAzureResolver_ListSecrets tests listing secrets. func TestAzureResolver_ListSecrets(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"value": []map[string]any{}}) + }) + defer server.Close() - // List without filter - result, err := resolver.ListSecrets(ctx, "") + result, err := resolver.ListSecrets(context.Background(), "") - // Either succeeds or fails with permission error - if err == nil { - assert.NotNil(t, result) - } else { - assert.Contains(t, err.Error(), "failed to list secrets") - } + require.NoError(t, err) + assert.NotNil(t, result) } -// TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct tests listing secrets with a filter using direct resolver. +// TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct tests that the +// client-side prefix filter is applied while listing. func TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "value": []map[string]any{ + {"id": "https://myvault.vault.azure.net/secrets/prod-db-creds"}, + {"id": "https://myvault.vault.azure.net/secrets/dev-api-key"}, + }, + }) + }) + defer server.Close() - // List with filter - result, err := resolver.ListSecrets(ctx, "prod") + result, err := resolver.ListSecrets(context.Background(), "prod") - // Either succeeds or fails with permission error - if err == nil { - // Filter is applied client-side in Azure resolver - assert.NotNil(t, result) - } + require.NoError(t, err) + assert.Equal(t, []string{"prod-db-creds"}, result) } -// TestAzureResolver_Close_Idempotent tests that Close can be called multiple times. +// TestAzureResolver_Close_Idempotent tests that Close can be called multiple +// times. Constructor-only: no requests are made. func TestAzureResolver_Close_Idempotent(t *testing.T) { ctx := context.Background() resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } + require.NoError(t, err) // Close multiple times should not panic err1 := resolver.Close() @@ -133,9 +120,7 @@ func TestAzureResolver_DifferentVaultURLs(t *testing.T) { for _, vaultURL := range vaultURLs { t.Run(vaultURL, func(t *testing.T) { resolver, err := NewAzureResolver(ctx, vaultURL) - if err != nil { - t.Skipf("Skipping test: Azure config not available for vault %s: %v", vaultURL, err) - } + require.NoError(t, err) defer resolver.Close() assert.Equal(t, vaultURL, resolver.vaultURL) @@ -146,48 +131,30 @@ func TestAzureResolver_DifferentVaultURLs(t *testing.T) { // TestAzureResolver_ContextHandling tests context handling in Azure resolver. func TestAzureResolver_ContextHandling(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // Test with canceled context cancelledCtx, cancel := context.WithCancel(context.Background()) cancel() - // GetSecret with canceled context - _, err = resolver.GetSecret(cancelledCtx, "test-secret") - // Should fail + // GetSecret with canceled context should fail before reaching the server. + _, err := resolver.GetSecret(cancelledCtx, "test-secret") assert.Error(t, err) } // TestAzureResolver_EmptySecretID tests getting a secret with empty ID. func TestAzureResolver_EmptySecretID(t *testing.T) { - ctx := context.Background() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() - - // Empty secret ID - _, err = resolver.GetSecret(ctx, "") + _, err := resolver.GetSecret(context.Background(), "") assert.Error(t, err) } // TestAzureResolver_SpecialCharactersInSecretID tests secret IDs with special characters. func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() // Azure Key Vault secret names can contain alphanumeric and dashes testIDs := []string{ @@ -198,8 +165,8 @@ func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { for _, id := range testIDs { t.Run(id, func(t *testing.T) { - // These will fail since secrets don't exist - _, err := resolver.GetSecret(ctx, id) + // These fail because the mock vault returns 404 for every secret. + _, err := resolver.GetSecret(context.Background(), id) assert.Error(t, err) }) } @@ -207,16 +174,10 @@ func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { // TestAzureResolver_GetSecretJSON_RealMethod tests the GetSecretJSON error propagation. func TestAzureResolver_GetSecretJSON_RealMethod(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // GetSecretJSON should fail because the secret doesn't exist - result, err := resolver.GetSecretJSON(ctx, "cudly-test-nonexistent-json-secret-xyz") + result, err := resolver.GetSecretJSON(context.Background(), "cudly-test-nonexistent-json-secret-xyz") require.Error(t, err) assert.Nil(t, result) From 13246cc21ce6b7ac2bd896094eac0d0319bd0f04 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 21:05:45 +0200 Subject: [PATCH 2/3] test(azure): mock service clients in recommendations adapter tests TestRecommendationsClientAdapter_GetRecommendationsForService and TestRecommendationsClientAdapter_GetAllRecommendations built real armconsumption/armadvisor clients with a fake bearer token and issued live HTTPS requests to management.azure.com on every test run (the old comments admitted the calls were expected to fail). They discarded both return values, so they asserted nothing. Rewrite them onto the package's existing injection points (newXxxClientFn vars + getAdvisorRecsFn), the same mechanism the parallelism/ordering/error-isolation tests already use, via a shared overrideServiceClientFns helper with t.Cleanup restore. The tests now run offline and assert the returned recommendations. --- providers/azure/recommendations_test.go | 68 +++++++++++++++++++++---- 1 file changed, 57 insertions(+), 11 deletions(-) diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 8a13718d8..7c5d5a7b8 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -196,27 +196,73 @@ func TestRecommendationsClientAdapter_Fields(t *testing.T) { assert.Nil(t, adapter.cred) } +// overrideServiceClientFns swaps every per-service client constructor for the +// given fakes and restores the originals on cleanup, so adapter tests never +// build real ARM clients (which would issue live requests to +// management.azure.com). +func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp serviceRecsGetter) { + t.Helper() + origCompute := newComputeClientFn + origDatabase := newDatabaseClientFn + origCache := newCacheClientFn + origCosmos := newCosmosDBClientFn + origSP := newSavingsPlansClientFn + t.Cleanup(func() { + newComputeClientFn = origCompute + newDatabaseClientFn = origDatabase + newCacheClientFn = origCache + newCosmosDBClientFn = origCosmos + newSavingsPlansClientFn = origSP + }) + newComputeClientFn = newFakeFn(compute) + newDatabaseClientFn = newFakeFn(database) + newCacheClientFn = newFakeFn(cache) + newCosmosDBClientFn = newFakeFn(cosmos) + newSavingsPlansClientFn = newFakeFn(sp) +} + func TestRecommendationsClientAdapter_GetRecommendationsForService(t *testing.T) { + computeRec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute} + overrideServiceClientFns(t, + &fakeServiceClient{recs: []common.Recommendation{computeRec}}, + &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + adapter := &RecommendationsClientAdapter{ - cred: &mockAzureTokenCredential{}, - subscriptionID: "test-subscription", + cred: &mockAzureTokenCredential{}, + subscriptionID: "test-subscription", + getAdvisorRecsFn: noopAdvisorFn, } - // This will try to make API calls which will fail without real credentials, - // but we test the wiring is correct. Error is expected since we don't have - // real Azure credentials - the important thing is the function is wired correctly. - _, _ = adapter.GetRecommendationsForService(context.Background(), common.ServiceCompute) + recs, err := adapter.GetRecommendationsForService(context.Background(), common.ServiceCompute) + + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{computeRec}, recs) } func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { + rec := func(svc common.ServiceType) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: svc} + } + overrideServiceClientFns(t, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCompute)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceRelationalDB)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCache)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceNoSQL)}}, + &fakeServiceClient{}) + adapter := &RecommendationsClientAdapter{ - cred: &mockAzureTokenCredential{}, - subscriptionID: "test-subscription", + cred: &mockAzureTokenCredential{}, + subscriptionID: "test-subscription", + getAdvisorRecsFn: noopAdvisorFn, } - // This will try to make API calls which will fail without real credentials, - // but we test the wiring is correct. Error is expected - we just verify the function is callable. - _, _ = adapter.GetAllRecommendations(context.Background()) + recs, err := adapter.GetAllRecommendations(context.Background()) + + require.NoError(t, err) + assert.ElementsMatch(t, []common.Recommendation{ + rec(common.ServiceCompute), rec(common.ServiceRelationalDB), + rec(common.ServiceCache), rec(common.ServiceNoSQL), + }, recs) } // TestGetRecommendations_SavingsPlansServiceIncluded pins that shouldIncludeService From 50c56e1a926872649e1d2d9f8a569e136156c238 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 22:32:00 +0200 Subject: [PATCH 3/3] test(secrets): assert canceled context sends no request --- internal/secrets/azure_resolver_coverage_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/secrets/azure_resolver_coverage_test.go b/internal/secrets/azure_resolver_coverage_test.go index ce366ccc6..55a352b60 100644 --- a/internal/secrets/azure_resolver_coverage_test.go +++ b/internal/secrets/azure_resolver_coverage_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "net/http" + "sync/atomic" "testing" "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" @@ -131,7 +132,11 @@ func TestAzureResolver_DifferentVaultURLs(t *testing.T) { // TestAzureResolver_ContextHandling tests context handling in Azure resolver. func TestAzureResolver_ContextHandling(t *testing.T) { - resolver, server := newTestAzureResolver(t, notFoundHandler) + var handlerCalls atomic.Int32 + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, r *http.Request) { + handlerCalls.Add(1) + notFoundHandler(w, r) + }) defer server.Close() cancelledCtx, cancel := context.WithCancel(context.Background()) @@ -139,7 +144,8 @@ func TestAzureResolver_ContextHandling(t *testing.T) { // GetSecret with canceled context should fail before reaching the server. _, err := resolver.GetSecret(cancelledCtx, "test-secret") - assert.Error(t, err) + require.Error(t, err) + assert.Zero(t, handlerCalls.Load(), "canceled request must not reach the HTTP handler") } // TestAzureResolver_EmptySecretID tests getting a secret with empty ID.