diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 72eeed75..cda865f4 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -64,6 +64,7 @@ func TestHandler_getDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) + mockStore.On("GetServiceConfig", ctx, "", mock.Anything).Return(nil, config.ErrNotFound) // No account_id / account_ids filter → calculateCommitmentMetrics fetches the // uncapped active set across all accounts via GetActivePurchaseHistory. mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) @@ -1200,6 +1201,7 @@ func TestHandler_getDashboardSummary_CurrentSavingsPopulated(t *testing.T) { mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) // No account_id / account_ids filter, so calculateCommitmentMetrics fetches // across all accounts via GetActivePurchaseHistory (uncapped, active-only). + mockStore.On("GetServiceConfig", ctx, "", mock.Anything).Return(nil, config.ErrNotFound) mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return(purchases, nil) mockAuth, req := adminDashboardReq(ctx) @@ -1242,6 +1244,7 @@ func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return( []config.RecommendationRecord{{Service: "EC2", Savings: 400.0}}, nil) + mockStore.On("GetServiceConfig", ctx, "", "EC2").Return(nil, config.ErrNotFound) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) // No account filter, so the all-accounts fetch path (GetActivePurchaseHistory // with an empty scope) runs. @@ -1287,6 +1290,7 @@ func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *test {Service: "EC2", Savings: 500.0}, {Service: "RDS", Savings: 300.0}, }, nil) + mockStore.On("GetServiceConfig", ctx, "", mock.Anything).Return(nil, config.ErrNotFound) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) // No account filter: the all-accounts active fetch path runs. mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return( diff --git a/internal/api/handler_recommendations_global_filters_integration_test.go b/internal/api/handler_recommendations_global_filters_integration_test.go new file mode 100644 index 00000000..bacbe974 --- /dev/null +++ b/internal/api/handler_recommendations_global_filters_integration_test.go @@ -0,0 +1,120 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/sts" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type globalFilterProvider struct { + provider.FactoryInterface + provider.Provider + provider.RecommendationsClient +} + +func (p globalFilterProvider) CreateAndValidateProvider(_ context.Context, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { + if name != "aws" || cfg != nil { + return nil, fmt.Errorf("unexpected provider request: %s", name) + } + return p, nil +} + +func (p globalFilterProvider) GetRecommendationsClient(context.Context) (provider.RecommendationsClient, error) { + return p, nil +} + +func (globalFilterProvider) GetAllRecommendations(context.Context) ([]common.Recommendation, error) { + return []common.Recommendation{{Provider: common.ProviderAWS, Service: common.ServiceEC2, Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: "1yr", PaymentOption: "all-upfront", EstimatedSavings: 100}}, nil +} + +type globalFilterSTS struct{} + +func (globalFilterSTS) GetCallerIdentity(context.Context, *sts.GetCallerIdentityInput, ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) { + return &sts.GetCallerIdentityOutput{Account: aws.String("123456789012")}, nil +} + +func TestGlobalFiltersCollectedRecommendationsAPI(t *testing.T) { + for _, registered := range []bool{false, true} { + t.Run(fmt.Sprintf("registered=%t", registered), func(t *testing.T) { + ctx := t.Context() + pg, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) }) + require.NoError(t, migrations.RunMigrations(ctx, pg.DB.Pool(), "../database/postgres/migrations", "", "")) + store := config.NewPostgresStore(pg.DB) + require.NoError(t, store.SaveGlobalConfig(ctx, &config.GlobalConfig{EnabledProviders: []string{"aws"}, DefaultTerm: 1, DefaultPayment: "all-upfront"})) + account := uuid.NewString() + if registered { + require.NoError(t, store.CreateCloudAccount(ctx, &config.CloudAccount{ID: account, Name: "host", Provider: "aws", ExternalID: "123456789012", Enabled: false})) + } + collector := scheduler.NewScheduler(scheduler.SchedulerConfig{ConfigStore: store, ProviderFactory: globalFilterProvider{}, STSClient: globalFilterSTS{}, EmailSender: &stubEmailNotifier{}, IsLambda: true}) + result, err := collector.CollectRecommendations(ctx, "") + require.NoError(t, err) + require.Empty(t, result.FailedProviders) + require.Equal(t, 1, result.Recommendations) + rows, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, rows, 1) + if registered { + require.Equal(t, &account, rows[0].CloudAccountID) + } else { + require.Nil(t, rows[0].CloudAccountID) + } + var nulls int + require.NoError(t, pg.DB.Pool().QueryRow(ctx, "SELECT count(*) FROM recommendations WHERE cloud_account_id IS NULL").Scan(&nulls)) + assert.Equal(t, !registered, nulls == 1) + authStore := auth.NewPostgresStore(pg.DB) + csrf := []byte(strings.Repeat("f", 32)) + _, token, _ := marketplaceSession(t, authStore, []auth.Permission{{Action: auth.ActionView, Resource: "recommendations"}}, []string{"*"}, csrf) + h := NewHandler(HandlerConfig{ConfigStore: store, Scheduler: collector, AuthService: &marketplaceAuthFixture{service: auth.NewService(auth.ServiceConfig{Store: authStore, CSRFKey: csrf})}}) + request := func(path string, out any) { + t.Helper() + response, err := h.HandleRequest(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer " + token}, RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "GET", Path: path}}}) + require.NoError(t, err) + require.Equal(t, 200, response.StatusCode, response.Body) + require.NoError(t, json.Unmarshal([]byte(response.Body), out)) + } + var initial RecommendationsResponse + request("/api/recommendations", &initial) + require.Len(t, initial.Recommendations, 1, "no global configuration") + for _, enabled := range []bool{true, false, true} { + require.NoError(t, store.SaveServiceConfig(ctx, &config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: enabled, Term: 1, Payment: "all-upfront", Coverage: 100})) + var list RecommendationsResponse + request("/api/recommendations", &list) + var detail RecommendationDetailResponse + request("/api/recommendations/"+rows[0].ID+"/detail", &detail) + var summary DashboardSummaryResponse + request("/api/dashboard/summary", &summary) + if enabled { + assert.Len(t, list.Recommendations, 1) + assert.Equal(t, 1, summary.TotalRecommendations) + assert.Equal(t, 100.0, summary.PotentialMonthlySavings) + assert.Empty(t, detail.HiddenBy) + } else { + assert.Empty(t, list.Recommendations) + assert.Zero(t, summary.TotalRecommendations) + assert.Zero(t, summary.PotentialMonthlySavings) + assert.Equal(t, []string{"enabled=false"}, detail.HiddenBy) + } + } + }) + } +} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index b79b32e5..9e1edd88 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -1040,6 +1040,7 @@ func TestHandler_HandleRequest_GetDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", mock.Anything).Return(globalCfg, nil) + mockStore.On("GetServiceConfig", mock.Anything, "", "rds").Return(nil, config.ErrNotFound) // No account_id / account_ids filter → calculateCommitmentMetrics fetches the // uncapped active set across all accounts via GetActivePurchaseHistory. mockStore.On("GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) diff --git a/internal/config/recommendation_overrides.go b/internal/config/recommendation_overrides.go index 0712b4c6..c241a880 100644 --- a/internal/config/recommendation_overrides.go +++ b/internal/config/recommendation_overrides.go @@ -68,12 +68,9 @@ func (c *globalConfigCache) lookup(ctx context.Context, store AccountConfigReade // ResolveServiceConfig(provider, service, global, override). Returns a map // keyed by AccountConfigKey -> resolved *ServiceConfig. // -// Triples are skipped (not present in the map) when: -// - rec.CloudAccountID is nil — no per-account override possible (e.g. -// AWS ambient-credentials path). -// - Neither a global ServiceConfig nor a per-account override exists for the -// (provider, service) pair — no configuration to apply, so callers treat -// the triple as "no filter applies". +// A nil CloudAccountID uses an empty account key and resolves only global +// configuration. Triples with neither global configuration nor an account +// override are omitted from the map. // // When a per-account override exists but no global ServiceConfig does, the // override is applied against a synthesized default baseline (Enabled: true) @@ -105,10 +102,10 @@ func ResolveAccountConfigsForRecs( for i := range recs { rec := &recs[i] - if rec.CloudAccountID == nil { - continue + accountID := "" + if rec.CloudAccountID != nil { + accountID = *rec.CloudAccountID } - accountID := *rec.CloudAccountID key := AccountConfigKey(accountID, rec.Provider, rec.Service) if _, ok := seen[key]; ok { continue @@ -120,9 +117,12 @@ func ResolveAccountConfigsForRecs( return resolved, err } - override, err := store.GetAccountServiceOverride(ctx, accountID, rec.Provider, rec.Service) - if err != nil { - return resolved, fmt.Errorf("get override %s/%s/%s: %w", accountID, rec.Provider, rec.Service, err) + var override *AccountServiceOverride + if rec.CloudAccountID != nil { + override, err = store.GetAccountServiceOverride(ctx, accountID, rec.Provider, rec.Service) + if err != nil { + return resolved, fmt.Errorf("get override %s/%s/%s: %w", accountID, rec.Provider, rec.Service, err) + } } // Skip the triple only when both global and override are absent — there is diff --git a/internal/config/recommendation_overrides_test.go b/internal/config/recommendation_overrides_test.go index c75b88ae..bc65aae0 100644 --- a/internal/config/recommendation_overrides_test.go +++ b/internal/config/recommendation_overrides_test.go @@ -53,15 +53,45 @@ func TestResolveAccountConfigsForRecs_EmptyRecs(t *testing.T) { assert.Zero(t, reader.overrideCalls) } -func TestResolveAccountConfigsForRecs_NilCloudAccountSkipped(t *testing.T) { - reader := &fakeAccountConfigReader{} +func TestResolveAccountConfigsForRecs_NilCloudAccountUsesGlobal(t *testing.T) { + reader := &fakeAccountConfigReader{globals: map[string]*ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: false}, + }, overrideErr: errors.New("ambient must not query overrides")} recs := []RecommendationRecord{ {Provider: "aws", Service: "rds", CloudAccountID: nil}, // ambient + {Provider: "aws", Service: "rds", CloudAccountID: nil}, } got, err := ResolveAccountConfigsForRecs(context.Background(), reader, recs) assert.NoError(t, err) - assert.Empty(t, got, "nil CloudAccountID recs are skipped") - assert.Zero(t, reader.globalCalls) + assert.Equal(t, reader.globals["aws|rds"], got[AccountConfigKey("", "aws", "rds")]) + assert.Equal(t, 1, reader.globalCalls) + assert.Zero(t, reader.overrideCalls) +} + +func TestResolveAccountConfigsForRecs_AmbientAndRegistered(t *testing.T) { + reader := &fakeAccountConfigReader{ + globals: map[string]*ServiceConfig{"aws|rds": {Enabled: false}}, + overrides: map[string]*AccountServiceOverride{"acct-A|aws|rds": {Enabled: boolPtr(true)}}, + } + got, err := ResolveAccountConfigsForRecs(t.Context(), reader, []RecommendationRecord{ + {Provider: "aws", Service: "rds"}, acctRec("acct-A", "aws", "rds"), + }) + assert.NoError(t, err) + assert.Equal(t, &ServiceConfig{Enabled: false}, got[AccountConfigKey("", "aws", "rds")]) + assert.True(t, got[AccountConfigKey("acct-A", "aws", "rds")].Enabled) + assert.Equal(t, 1, reader.globalCalls) + assert.Equal(t, 1, reader.overrideCalls) +} + +func TestResolveAccountConfigsForRecs_AmbientMissingAndErrors(t *testing.T) { + for _, lookupErr := range []error{nil, fmt.Errorf("absent: %w", ErrNotFound), errors.New("database unavailable")} { + reader := &fakeAccountConfigReader{globalErr: lookupErr} + got, err := ResolveAccountConfigsForRecs(t.Context(), reader, []RecommendationRecord{{Provider: "aws", Service: "rds"}}) + assert.Equal(t, lookupErr != nil && !errors.Is(lookupErr, ErrNotFound), err != nil) + assert.Empty(t, got) + assert.Equal(t, 1, reader.globalCalls) + assert.Zero(t, reader.overrideCalls) + } } func TestResolveAccountConfigsForRecs_OverridePresent_ResolvedConfigReflectsOverride(t *testing.T) { diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c9ba275e..418e4b29 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1154,18 +1154,20 @@ func (s *Scheduler) GetRecommendationByID(ctx context.Context, id string) (rec * // Check whether the account-override filter would drop this rec. This is // a read-only call — we never drop it here, only report the reasons. + resolved, resolveErr := config.ResolveAccountConfigsForRecs(ctx, s.config, recs) + if resolveErr != nil { + // Non-fatal: if the override check fails we surface the rec without + // a hidden_by marker (over-show is the safer default). + logging.Errorf("GetRecommendationByID: override resolution failed; returning rec without hidden_by: %v", resolveErr) + return found, nil, nil + } + accountID := "" if found.CloudAccountID != nil { - resolved, resolveErr := config.ResolveAccountConfigsForRecs(ctx, s.config, recs) - if resolveErr != nil { - // Non-fatal: if the override check fails we surface the rec without - // a hidden_by marker (over-show is the safer default). - logging.Errorf("GetRecommendationByID: override resolution failed; returning rec without hidden_by: %v", resolveErr) - return found, nil, nil - } - cfg := resolved[config.AccountConfigKey(*found.CloudAccountID, found.Provider, found.Service)] - if cfg != nil { - hiddenBy = overrideHiddenReasons(found, cfg) - } + accountID = *found.CloudAccountID + } + cfg := resolved[config.AccountConfigKey(accountID, found.Provider, found.Service)] + if cfg != nil { + hiddenBy = overrideHiddenReasons(found, cfg) } return found, hiddenBy, nil diff --git a/internal/scheduler/scheduler_global_filters_integration_test.go b/internal/scheduler/scheduler_global_filters_integration_test.go new file mode 100644 index 00000000..bab3be87 --- /dev/null +++ b/internal/scheduler/scheduler_global_filters_integration_test.go @@ -0,0 +1,102 @@ +//go:build integration + +package scheduler + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGlobalFiltersPersistedAmbientAndRegistered(t *testing.T) { + ctx := t.Context() + pg, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) }) + require.NoError(t, migrations.RunMigrations(ctx, pg.DB.Pool(), "../database/postgres/migrations", "", "")) + store := config.NewPostgresStore(pg.DB) + account := uuid.NewString() + require.NoError(t, store.CreateCloudAccount(ctx, &config.CloudAccount{ID: account, Name: "registered", Provider: "aws", ExternalID: "111111111111", Enabled: true})) + records := make([]config.RecommendationRecord, 0, 10) + for i, id := range []*string{nil, &account} { + for _, count := range []int{1, 10, 11} { + records = append(records, config.RecommendationRecord{ID: fmt.Sprintf("%d-%d", i, count), Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: fmt.Sprintf("db.m5.%d", count), Engine: "mysql", Count: count, Term: 1, Payment: "all-upfront", CloudAccountID: id}) + } + records = append(records, + config.RecommendationRecord{ID: fmt.Sprintf("%d-sibling", i), Provider: "aws", Service: "rds", Region: "us-west-2", ResourceType: "db.r6g.large", Engine: "postgres", Count: 12, Term: 1, Payment: "all-upfront", CloudAccountID: id}, + config.RecommendationRecord{ID: fmt.Sprintf("%d-other", i), Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront", CloudAccountID: id}) + } + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), records, nil)) + s := &Scheduler{config: store, isLambda: true} + _, err = store.GetServiceConfig(ctx, "aws", "rds") + require.ErrorIs(t, err, config.ErrNotFound) + all, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, all, 10) + for _, tc := range []struct { + name string + policy config.ServiceConfig + want []string + reason string + }{ + {"disabled", config.ServiceConfig{}, nil, "enabled=false"}, + {"enabled", config.ServiceConfig{Enabled: true}, []string{"1", "10", "11", "sibling"}, ""}, + {"minimum", config.ServiceConfig{Enabled: true, MinCount: 10}, []string{"10", "11", "sibling"}, ""}, + {"include engine", config.ServiceConfig{Enabled: true, IncludeEngines: []string{"postgres"}}, []string{"sibling"}, "engine"}, + {"exclude engine", config.ServiceConfig{Enabled: true, ExcludeEngines: []string{"mysql"}}, []string{"sibling"}, "engine"}, + {"include region", config.ServiceConfig{Enabled: true, IncludeRegions: []string{"us-west-2"}}, []string{"sibling"}, "region"}, + {"exclude region", config.ServiceConfig{Enabled: true, ExcludeRegions: []string{"us-east-1"}}, []string{"sibling"}, "region"}, + {"include type", config.ServiceConfig{Enabled: true, IncludeTypes: []string{"db.m5.10"}}, []string{"10"}, "resource_type"}, + {"exclude type", config.ServiceConfig{Enabled: true, ExcludeTypes: []string{"db.m5.1"}}, []string{"10", "11", "sibling"}, "resource_type"}, + {"exclude wins", config.ServiceConfig{Enabled: true, IncludeRegions: []string{"us-east-1", "us-west-2"}, ExcludeRegions: []string{"us-east-1"}}, []string{"sibling"}, "region"}, + } { + t.Run(tc.name, func(t *testing.T) { + tc.policy.Provider, tc.policy.Service, tc.policy.Term, tc.policy.Payment = "aws", "rds", 1, "all-upfront" + require.NoError(t, store.SaveServiceConfig(ctx, &tc.policy)) + got, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + ids := make([]string, 0, len(got)) + for _, rec := range got { + ids = append(ids, rec.ID) + _, hidden, err := s.GetRecommendationByID(ctx, rec.ID) + require.NoError(t, err) + assert.Empty(t, hidden, "visible recommendation %s", rec.ID) + } + want := make([]string, 2, 2+2*len(tc.want)) + want[0], want[1] = "0-other", "1-other" + for _, suffix := range tc.want { + want = append(want, "0-"+suffix, "1-"+suffix) + } + assert.ElementsMatch(t, want, ids) + for _, id := range []string{"0-1", "1-1"} { + rec, hidden, err := s.GetRecommendationByID(ctx, id) + require.NoError(t, err) + require.NotNil(t, rec) + if tc.reason != "" { + assert.Contains(t, hidden, tc.reason) + } else { + assert.Empty(t, hidden) + } + } + }) + } + t.Run("registered override wins", func(t *testing.T) { + require.NoError(t, store.SaveServiceConfig(ctx, &config.ServiceConfig{Provider: "aws", Service: "rds", Term: 1, Payment: "all-upfront"})) + require.NoError(t, store.SaveAccountServiceOverride(ctx, &config.AccountServiceOverride{AccountID: account, Provider: "aws", Service: "rds", Enabled: boolPtr(true)})) + got, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + ids := make([]string, 0, len(got)) + for _, rec := range got { + ids = append(ids, rec.ID) + } + assert.ElementsMatch(t, []string{"0-other", "1-other", "1-1", "1-10", "1-11", "1-sibling"}, ids) + }) +} diff --git a/internal/scheduler/scheduler_overrides.go b/internal/scheduler/scheduler_overrides.go index 04a757ad..0c24b195 100644 --- a/internal/scheduler/scheduler_overrides.go +++ b/internal/scheduler/scheduler_overrides.go @@ -44,10 +44,8 @@ func (s *Scheduler) applyAccountOverrides(ctx context.Context, recs []config.Rec // ServiceConfig says enabled=false, or whose engine / region / resource_type // is rejected by the resolved include/exclude lists. // -// Recs without a CloudAccountID (e.g. AWS ambient-credentials path) and -// recs whose triple has no resolved entry (no global ServiceConfig row, or -// the resolver caller skipped them) pass through unfiltered — there is no -// per-account policy to enforce on them. +// Recs without a CloudAccountID use global configuration. Recs whose triple +// has no resolved entry pass through unfiltered. func filterRecsByResolvedConfigs( recs []config.RecommendationRecord, resolved map[string]*config.ServiceConfig, @@ -57,11 +55,11 @@ func filterRecsByResolvedConfigs( out := make([]config.RecommendationRecord, 0, len(recs)) for i := range recs { rec := recs[i] - if rec.CloudAccountID == nil { - out = append(out, rec) - continue + accountID := "" + if rec.CloudAccountID != nil { + accountID = *rec.CloudAccountID } - cfg := resolved[config.AccountConfigKey(*rec.CloudAccountID, rec.Provider, rec.Service)] + cfg := resolved[config.AccountConfigKey(accountID, rec.Provider, rec.Service)] if cfg == nil { out = append(out, rec) continue diff --git a/internal/scheduler/scheduler_overrides_test.go b/internal/scheduler/scheduler_overrides_test.go index 16fa6827..823f1fdb 100644 --- a/internal/scheduler/scheduler_overrides_test.go +++ b/internal/scheduler/scheduler_overrides_test.go @@ -151,7 +151,7 @@ func TestApplyAccountOverrides_NoGlobalConfig_RecsPassThrough(t *testing.T) { assert.Len(t, recs, 1, "no global config -> no per-account policy applies -> rec passes through") } -func TestApplyAccountOverrides_NilCloudAccountID_PassesThrough(t *testing.T) { +func TestApplyAccountOverrides_NilCloudAccountID_UsesGlobal(t *testing.T) { ctx := context.Background() rec := config.RecommendationRecord{ ID: "ambient", Provider: "aws", Service: "ec2", @@ -168,7 +168,37 @@ func TestApplyAccountOverrides_NilCloudAccountID_PassesThrough(t *testing.T) { recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) require.NoError(t, err) - assert.Len(t, recs, 1, "nil CloudAccountID recs are not subject to per-account override policy") + assert.Empty(t, recs, "global policy also applies without an account override") +} + +func TestApplyAccountOverrides_AmbientControls(t *testing.T) { + for _, lookupErr := range []error{nil, errors.New("database unavailable")} { + store := &mockOverrideStore{getGlobalErr: lookupErr, recs: []config.RecommendationRecord{{ID: "ambient", Provider: "aws", Service: "rds"}}} + s := &Scheduler{config: store, isLambda: true} + got, err := s.ListRecommendations(t.Context(), config.RecommendationFilter{}) + require.NoError(t, err) + assert.Equal(t, store.recs, got) + rec, hidden, err := s.GetRecommendationByID(t.Context(), "ambient") + require.NoError(t, err) + require.NotNil(t, rec) + assert.Empty(t, hidden) + } +} + +func TestFilterRecsByResolvedConfigs_AmbientIsolation(t *testing.T) { + recs := []config.RecommendationRecord{ + {ID: "drop", Provider: "aws", Service: "rds", Engine: "mysql"}, + {ID: "engine-less", Provider: "aws", Service: "rds"}, + {ID: "other-service", Provider: "aws", Service: "ec2", Engine: "mysql"}, + {ID: "other-provider", Provider: "azure", Service: "rds", Engine: "mysql"}, + } + original := append([]config.RecommendationRecord(nil), recs...) + resolved := map[string]*config.ServiceConfig{ + config.AccountConfigKey("", "aws", "rds"): {Enabled: true, IncludeEngines: []string{"postgres"}}, + } + got := filterRecsByResolvedConfigs(recs, resolved) + assert.Equal(t, original[1:], got) + assert.Equal(t, original, recs, "filter must preserve caller's backing array") } func TestApplyAccountOverrides_IncludeEngineMatch(t *testing.T) { diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 7c450c49..368d3fd8 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -924,6 +924,7 @@ func TestScheduler_ListRecommendations(t *testing.T) { Return(&config.RecommendationsFreshness{LastCollectedAt: &now}, nil) mockStore.On("ListStoredRecommendations", ctx, mock.Anything). Return(cached, nil) + mockStore.On("GetServiceConfig", ctx, "aws", mock.Anything).Return(nil, config.ErrNotFound) // Non-Lambda path resolves the effective stale TTL from the DB config. mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours, @@ -953,6 +954,7 @@ func TestScheduler_ListRecommendations_StaleHoursZeroDisablesBackgroundRefresh(t Return(&config.RecommendationsFreshness{LastCollectedAt: &old}, nil) mockStore.On("ListStoredRecommendations", ctx, mock.Anything). Return(cached, nil) + mockStore.On("GetServiceConfig", ctx, "aws", "ec2").Return(nil, config.ErrNotFound) // Disable sentinel: 0 must NOT trigger a background refresh. mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ RecommendationsCacheStaleHours: 0,