diff --git a/internal/providerfactory/bypass_guard_test.go b/internal/providerfactory/bypass_guard_test.go new file mode 100644 index 00000000..78e6f9fc --- /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 00000000..bc351ef8 --- /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 00000000..063ee506 --- /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 5696de36..36839c21 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 c7514c95..802c0a39 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)