From 96fc237c0c7fd5882388d4c40fb59b0bf13e037f Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 08:58:24 +0000 Subject: [PATCH] refactor(provider): build credentialed Azure/GCP providers through the registry The purchase and scheduler credentialed paths called the concrete Azure and GCP constructors directly, and the Azure path pinned the subscription through the deprecated ProviderConfig.Profile field. Add a small internal/providerfactory package that builds these providers through the cloud-commitments-go provider registry, using the typed AzureSubscriptionID/GCPProjectID fields and the AzureTokenCredential/ GCPTokenSource credential slots, and use it at those four call sites. The registry-built providers are identical to the legacy ones (tests compare them field by field against the real registered factories). One deliberate difference: NewGCP rejects an empty project ID instead of letting the registry's GCP factory ask the cloud for the credential's default project. A guard test fails if any non-test file under cmd/ or internal/ calls the concrete constructors again or sets ProviderConfig.Profile. The RI exchange handler's concrete AWS/Azure imports are left for a follow-up. Refs #41 Co-Authored-By: Claude Opus 5.5 --- internal/providerfactory/bypass_guard_test.go | 112 ++++++++++ internal/providerfactory/providerfactory.go | 92 ++++++++ .../providerfactory/providerfactory_test.go | 203 ++++++++++++++++++ internal/purchase/execution.go | 13 +- internal/scheduler/scheduler.go | 10 +- 5 files changed, 422 insertions(+), 8 deletions(-) create mode 100644 internal/providerfactory/bypass_guard_test.go create mode 100644 internal/providerfactory/providerfactory.go create mode 100644 internal/providerfactory/providerfactory_test.go diff --git a/internal/providerfactory/bypass_guard_test.go b/internal/providerfactory/bypass_guard_test.go new file mode 100644 index 000000000..78e6f9fc3 --- /dev/null +++ b/internal/providerfactory/bypass_guard_test.go @@ -0,0 +1,112 @@ +package providerfactory + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + azureProviderPkg = "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + gcpProviderPkg = "github.com/LeanerCloud/cloud-commitments-go/providers/gcp" + providerPkg = "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" +) + +// bypassConstructors are the concrete constructors that skip the provider +// registry. Production code builds credentialed providers through this +// package instead (platform#41). +var bypassConstructors = map[string]map[string]bool{ + azureProviderPkg: {"NewAzureProvider": true}, + gcpProviderPkg: {"NewProvider": true, "NewProviderWithProject": true, "NewProviderWithCredentials": true}, +} + +// No non-test file under cmd/ or internal/ may call the concrete Azure/GCP +// provider constructors or set the deprecated ProviderConfig.Profile field. +func TestNoRegistryBypassInProductionCode(t *testing.T) { + root := filepath.Join("..", "..") + var violations []string + scanned := 0 + for _, dir := range []string{"cmd", "internal"} { + err := filepath.WalkDir(filepath.Join(root, dir), func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + scanned++ + violations = append(violations, registryBypasses(t, path)...) + return nil + }) + require.NoError(t, err) + } + require.Greater(t, scanned, 100, "the walk must actually reach the production sources") + assert.Empty(t, violations, + "build credentialed Azure/GCP providers with providerfactory.NewAzure/NewGCP and use the typed ProviderConfig fields") +} + +// registryBypasses returns one entry per bypassing call or deprecated +// Profile key in the file at path. +func registryBypasses(t *testing.T, path string) []string { + t.Helper() + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + require.NoError(t, err, path) + + imports := map[string]string{} // local name -> import path + for _, imp := range f.Imports { + p, err := strconv.Unquote(imp.Path.Value) + require.NoError(t, err) + name := p[strings.LastIndex(p, "/")+1:] + if imp.Name != nil { + name = imp.Name.Name + } + imports[name] = p + } + + var out []string + ast.Inspect(f, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.SelectorExpr: + if pkg := selectorPkg(node, imports); bypassConstructors[pkg][node.Sel.Name] { + out = append(out, fset.Position(node.Pos()).String()+": "+node.Sel.Name) + } + case *ast.CompositeLit: + if sel, ok := node.Type.(*ast.SelectorExpr); ok && sel.Sel.Name == "ProviderConfig" && selectorPkg(sel, imports) == providerPkg { + out = append(out, profileKeys(fset, node)...) + } + } + return true + }) + return out +} + +func selectorPkg(sel *ast.SelectorExpr, imports map[string]string) string { + id, ok := sel.X.(*ast.Ident) + if !ok { + return "" + } + return imports[id.Name] +} + +func profileKeys(fset *token.FileSet, lit *ast.CompositeLit) []string { + var out []string + for _, el := range lit.Elts { + kv, ok := el.(*ast.KeyValueExpr) + if !ok { + continue + } + if id, ok := kv.Key.(*ast.Ident); ok && id.Name == "Profile" { + out = append(out, fset.Position(kv.Pos()).String()+": ProviderConfig.Profile") + } + } + return out +} diff --git a/internal/providerfactory/providerfactory.go b/internal/providerfactory/providerfactory.go new file mode 100644 index 000000000..bc351ef8f --- /dev/null +++ b/internal/providerfactory/providerfactory.go @@ -0,0 +1,92 @@ +// Package providerfactory builds credentialed cloud providers through the +// cloud-commitments-go provider registry, so the internal layer no longer +// calls the concrete Azure/GCP constructors directly (platform#41). +// +// Credentials go in the registry's typed slots (AzureTokenCredential, +// GCPTokenSource) and the account identity in the typed +// AzureSubscriptionID / GCPProjectID fields, never the deprecated Profile +// field. +package providerfactory + +import ( + "errors" + "fmt" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "golang.org/x/oauth2" + + // The provider packages register their factories with the global + // registry in init(); importing them here guarantees "azure" and "gcp" + // are registered in every binary that uses this package. + _ "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + _ "github.com/LeanerCloud/cloud-commitments-go/providers/gcp" +) + +// Registry names of the providers built here. +const ( + azureName = "azure" + gcpName = "gcp" +) + +// ErrMissingGCPProjectID is returned by NewGCP when the project ID is empty. +var ErrMissingGCPProjectID = errors.New("providerfactory: GCP project ID is required") + +// ErrMissingGCPTokenSource is returned by NewGCP when the token source is nil. +var ErrMissingGCPTokenSource = errors.New("providerfactory: GCP token source is required") + +// NewAzure returns an Azure provider pinned to subscriptionID that +// authenticates with cred, built through the global provider registry. +// +// A nil cred leaves the provider on its ambient-credential fallback, the +// same as installing a nil credential on a directly constructed provider. +func NewAzure(subscriptionID string, cred azcore.TokenCredential) (provider.Provider, error) { + return newAzure(provider.GetRegistry(), subscriptionID, cred) +} + +// NewGCP returns a GCP provider for projectID that authenticates with ts, +// built through the global provider registry. +// +// projectID must be non-empty: the registry's GCP factory resolves an empty +// project ID by asking the cloud for the credential's default project, which +// the direct constructor never did and which could pin the provider to a +// project other than the account's. ts must be non-nil; callers in ADC mode +// use the ambient factory path instead. +func NewGCP(projectID string, ts oauth2.TokenSource) (provider.Provider, error) { + return newGCP(provider.GetRegistry(), projectID, ts) +} + +func newAzure(reg *provider.Registry, subscriptionID string, cred azcore.TokenCredential) (provider.Provider, error) { + // A nil cred converts to a nil `any`, which the Azure factory treats as + // "no credential supplied". + return build(reg, azureName, &provider.ProviderConfig{ + Name: azureName, + AzureSubscriptionID: subscriptionID, + AzureTokenCredential: cred, + }) +} + +func newGCP(reg *provider.Registry, projectID string, ts oauth2.TokenSource) (provider.Provider, error) { + if projectID == "" { + return nil, ErrMissingGCPProjectID + } + if ts == nil { + return nil, ErrMissingGCPTokenSource + } + return build(reg, gcpName, &provider.ProviderConfig{ + Name: gcpName, + GCPProjectID: projectID, + GCPTokenSource: ts, + }) +} + +func build(reg *provider.Registry, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { + prov, err := reg.GetProviderWithConfig(name, cfg) + if err != nil { + return nil, fmt.Errorf("providerfactory: build %s provider: %w", name, err) + } + if prov == nil { + return nil, fmt.Errorf("providerfactory: %s factory returned no provider", name) + } + return prov, nil +} diff --git a/internal/providerfactory/providerfactory_test.go b/internal/providerfactory/providerfactory_test.go new file mode 100644 index 000000000..063ee506f --- /dev/null +++ b/internal/providerfactory/providerfactory_test.go @@ -0,0 +1,203 @@ +package providerfactory + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" + gcpprovider "github.com/LeanerCloud/cloud-commitments-go/providers/gcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" +) + +const ( + testSubscriptionID = "00000000-0000-0000-0000-000000000001" + testProjectID = "example-project" +) + +// fakeAzureCred is an azcore.TokenCredential that never talks to Azure. +type fakeAzureCred struct{ id string } + +func (f *fakeAzureCred) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + return azcore.AccessToken{}, errors.New("fakeAzureCred: no network in tests") +} + +// fakeTokenSource is an oauth2.TokenSource that never talks to GCP. +type fakeTokenSource struct{ id string } + +func (f *fakeTokenSource) Token() (*oauth2.Token, error) { + return nil, errors.New("fakeTokenSource: no network in tests") +} + +// recordingRegistry returns a private registry whose name factory records +// the config it receives and returns want. +func recordingRegistry(t *testing.T, name string, want provider.Provider, wantErr error) (*provider.Registry, *[]*provider.ProviderConfig) { + t.Helper() + reg := provider.NewRegistry() + var got []*provider.ProviderConfig + require.NoError(t, reg.Register(name, func(cfg *provider.ProviderConfig) (provider.Provider, error) { + got = append(got, cfg) + return want, wantErr + })) + return reg, &got +} + +// stubProvider is a minimal provider.Provider for the fake-registry tests. +type stubProvider struct{ provider.Provider } + +func TestNewAzure_PassesTypedConfigToRegistry(t *testing.T) { + cred := &fakeAzureCred{id: "a"} + want := &stubProvider{} + reg, got := recordingRegistry(t, "azure", want, nil) + + prov, err := newAzure(reg, testSubscriptionID, cred) + + require.NoError(t, err) + assert.Same(t, want, prov, "must return the provider the registry built") + require.Len(t, *got, 1, "the azure factory must be called exactly once") + cfg := (*got)[0] + assert.Equal(t, "azure", cfg.Name) + assert.Equal(t, testSubscriptionID, cfg.AzureSubscriptionID) + assert.Same(t, cred, cfg.AzureTokenCredential, "the resolved credential must be passed through unchanged") + assert.Empty(t, cfg.Profile, "the deprecated Profile field must not be used") //nolint:staticcheck // SA1019: asserting the deprecated field stays unset + assert.Nil(t, cfg.GCPTokenSource) + assert.Nil(t, cfg.ProviderOverride) +} + +func TestNewAzure_NilCredentialLeavesSlotNil(t *testing.T) { + reg, got := recordingRegistry(t, "azure", &stubProvider{}, nil) + + _, err := newAzure(reg, testSubscriptionID, nil) + + require.NoError(t, err) + require.Len(t, *got, 1) + assert.Nil(t, (*got)[0].AzureTokenCredential, "a nil credential must reach the factory as a nil slot (ambient fallback)") +} + +func TestNewGCP_PassesTypedConfigToRegistry(t *testing.T) { + ts := &fakeTokenSource{id: "g"} + want := &stubProvider{} + reg, got := recordingRegistry(t, "gcp", want, nil) + + prov, err := newGCP(reg, testProjectID, ts) + + require.NoError(t, err) + assert.Same(t, want, prov) + require.Len(t, *got, 1, "the gcp factory must be called exactly once") + cfg := (*got)[0] + assert.Equal(t, "gcp", cfg.Name) + assert.Equal(t, testProjectID, cfg.GCPProjectID) + assert.Same(t, ts, cfg.GCPTokenSource, "the resolved token source must be passed through unchanged") + assert.Empty(t, cfg.Profile, "the deprecated Profile field must not be used") //nolint:staticcheck // SA1019: asserting the deprecated field stays unset + assert.Nil(t, cfg.AzureTokenCredential) + assert.Nil(t, cfg.ProviderOverride) +} + +// The registry's GCP factory resolves an empty project ID by asking the +// cloud for the credential's default project. The direct constructor never +// did that, so NewGCP must refuse before the factory runs. +func TestNewGCP_RejectsMissingInputsBeforeRegistry(t *testing.T) { + cases := []struct { + name string + projectID string + ts oauth2.TokenSource + wantErr error + }{ + {"empty project", "", &fakeTokenSource{}, ErrMissingGCPProjectID}, + {"nil token source", testProjectID, nil, ErrMissingGCPTokenSource}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reg, got := recordingRegistry(t, "gcp", &stubProvider{}, nil) + + prov, err := newGCP(reg, tc.projectID, tc.ts) + + require.ErrorIs(t, err, tc.wantErr) + assert.Nil(t, prov) + assert.Empty(t, *got, "the factory must not be called") + }) + } +} + +func TestBuild_PropagatesFactoryErrorsAndNilProviders(t *testing.T) { + boom := errors.New("boom") + reg, _ := recordingRegistry(t, "azure", nil, boom) + _, err := newAzure(reg, testSubscriptionID, &fakeAzureCred{}) + require.ErrorIs(t, err, boom) + assert.Contains(t, err.Error(), "build azure provider") + + reg, _ = recordingRegistry(t, "gcp", nil, nil) + _, err = newGCP(reg, testProjectID, &fakeTokenSource{}) + require.Error(t, err, "a nil provider with a nil error must not be returned as success") + assert.Contains(t, err.Error(), "gcp factory returned no provider") + + _, err = newAzure(provider.NewRegistry(), testSubscriptionID, nil) + require.Error(t, err, "an unregistered provider must fail") +} + +// The real registered Azure factory must produce a provider identical to the +// legacy construction it replaces in purchase/execution.go and +// scheduler/scheduler.go: NewAzureProvider(&ProviderConfig{Profile: sub}) +// followed by SetCredential(cred). +func TestNewAzure_MatchesLegacyConstruction(t *testing.T) { + cred := &fakeAzureCred{id: "legacy"} + + legacy, err := azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: testSubscriptionID}) //nolint:staticcheck // SA1019: this is the legacy baseline being compared against + require.NoError(t, err) + legacy.SetCredential(cred) + + built, err := NewAzure(testSubscriptionID, cred) + require.NoError(t, err) + got, ok := built.(*azureprovider.Provider) + require.True(t, ok, "registry must build the real Azure provider, got %T", built) + + // SetCredential also invalidates the (still empty) accounts cache, which + // bumps an internal generation counter. Do the same on the registry-built + // provider so the comparison covers every other field. + got.InvalidateAccountsCache() + assert.Equal(t, legacy, got, + "registry-built Azure provider must match the legacy construction (same subscription, credential and region)") + + // Guard against a vacuous comparison: a different credential or + // subscription must make the providers differ. + other, err := NewAzure(testSubscriptionID, &fakeAzureCred{id: "other"}) + require.NoError(t, err) + other.(*azureprovider.Provider).InvalidateAccountsCache() + assert.NotEqual(t, legacy, other, "a different credential must be detected") + + otherSub, err := NewAzure("00000000-0000-0000-0000-000000000002", cred) + require.NoError(t, err) + otherSub.(*azureprovider.Provider).InvalidateAccountsCache() + assert.NotEqual(t, legacy, otherSub, "a different subscription must be detected") +} + +// The real registered GCP factory must produce a provider identical to the +// legacy gcpprovider.NewProviderWithCredentials(ctx, project, ts) it +// replaces. The legacy constructor stored the caller's ctx, which the +// provider never reads; the baseline uses context.Background(), the value +// the registry factory stores, so the comparison covers every field. +func TestNewGCP_MatchesLegacyConstruction(t *testing.T) { + ts := &fakeTokenSource{id: "legacy"} + + legacy := gcpprovider.NewProviderWithCredentials(context.Background(), testProjectID, ts) + + built, err := NewGCP(testProjectID, ts) + require.NoError(t, err) + got, ok := built.(*gcpprovider.Provider) + require.True(t, ok, "registry must build the real GCP provider, got %T", built) + assert.Equal(t, legacy, got, + "registry-built GCP provider must match the legacy construction (same project and token source)") + + other, err := NewGCP(testProjectID, &fakeTokenSource{id: "other"}) + require.NoError(t, err) + assert.NotEqual(t, legacy, other, "a different token source must be detected") + + otherProject, err := NewGCP("other-project", ts) + require.NoError(t, err) + assert.NotEqual(t, legacy, otherProject, "a different project must be detected") +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 4e80d4671..16d372d8f 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -12,12 +12,11 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" - azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" - gcpprovider "github.com/LeanerCloud/cloud-commitments-go/providers/gcp" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/execution" + "github.com/LeanerCloud/cloud-commitments-platform/internal/providerfactory" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/google/uuid" @@ -546,13 +545,12 @@ func (m *Manager) resolveAzureProvider(ctx context.Context, account config.Cloud account.ID, time.Since(t0), err) return nil, fmt.Errorf("credentials: resolve Azure for account %s (%s): %w", account.ID, account.Name, err) } - azProv, err := azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: account.AzureSubscriptionID}) + azProv, err := providerfactory.NewAzure(account.AzureSubscriptionID, azCred) if err != nil { logging.Errorf("purchase[resolveAzureProvider]: Azure provider construction failed for account=%s after %s: %v", account.ID, time.Since(t0), err) return nil, fmt.Errorf("credentials: create Azure provider for account %s (%s): %w", account.ID, account.Name, err) } - azProv.SetCredential(azCred) logging.Infof("purchase[resolveAzureProvider]: Azure credentials resolved for account=%s in %s", account.ID, time.Since(t0)) return &provider.ProviderConfig{ProviderOverride: azProv}, nil @@ -580,7 +578,12 @@ func (m *Manager) resolveGCPProvider(ctx context.Context, account config.CloudAc account.ID, time.Since(t0)) return nil, nil } - gcpProv := gcpprovider.NewProviderWithCredentials(ctx, account.GCPProjectID, gcpTS) + gcpProv, err := providerfactory.NewGCP(account.GCPProjectID, gcpTS) + if err != nil { + logging.Errorf("purchase[resolveGCPProvider]: GCP provider construction failed for account=%s after %s: %v", + account.ID, time.Since(t0), err) + return nil, fmt.Errorf("credentials: create GCP provider for account %s (%s): %w", account.ID, account.Name, err) + } logging.Infof("purchase[resolveGCPProvider]: GCP credentials resolved for account=%s in %s", account.ID, time.Since(t0)) return &provider.ProviderConfig{ProviderOverride: gcpProv}, nil diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c7514c959..802c0a39d 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -25,6 +25,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/execution" "github.com/LeanerCloud/cloud-commitments-platform/internal/oidc" + "github.com/LeanerCloud/cloud-commitments-platform/internal/providerfactory" "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-sdk-go-v2/service/sts" "golang.org/x/sync/errgroup" @@ -909,11 +910,10 @@ func (s *Scheduler) collectAzureForAccount(ctx context.Context, acct config.Clou if err != nil { return nil, nil, fmt.Errorf("resolve credentials: %w", err) } - azProv, err := azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: acct.AzureSubscriptionID}) + azProv, err := providerfactory.NewAzure(acct.AzureSubscriptionID, azCred) if err != nil { return nil, nil, fmt.Errorf("create provider: %w", err) } - azProv.SetCredential(azCred) recClient, err := azProv.GetRecommendationsClient(ctx) if err != nil { @@ -977,7 +977,11 @@ func (s *Scheduler) collectGCPForAccount(ctx context.Context, acct config.CloudA var prov provider.Provider if gcpTS != nil { - prov = gcpprovider.NewProviderWithCredentials(ctx, acct.GCPProjectID, gcpTS) + created, createErr := providerfactory.NewGCP(acct.GCPProjectID, gcpTS) + if createErr != nil { + return nil, nil, fmt.Errorf("create provider: %w", createErr) + } + prov = created } else { // ADC mode (application_default): use ambient credentials created, createErr := s.providerFactory.CreateAndValidateProvider(ctx, "gcp", nil)