From 67f14c2a92a52d07c4f5ed8a7c853ee90031383d Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:08:12 +0000 Subject: [PATCH 1/3] refactor(db): share one dual-column account predicate across analytics, api and config The savings_snapshots and purchase_history account predicate had three copies (internal/analytics, internal/api, internal/config), each with its own positional-arg bookkeeping. Move it into internal/accountpredicate and switch all three callers. The config copy already used the case-insensitive provider gate from issue #24 while the other two use an exact gate, so the builder takes the gate as a parameter and every caller's SQL and bind order stay byte-identical; pinning tests cover each caller and the shared builder. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/accountpredicate/accountpredicate.go | 100 ++++++++++++++++++ .../accountpredicate/accountpredicate_test.go | 82 ++++++++++++++ internal/analytics/postgres_analytics.go | 52 ++------- .../analytics/postgres_analytics_mock_test.go | 20 ++++ internal/api/analytics_postgres.go | 65 ++---------- internal/api/analytics_postgres_test.go | 37 +++++++ internal/config/store_postgres.go | 72 +++---------- .../store_postgres_account_predicate_test.go | 5 + 8 files changed, 275 insertions(+), 158 deletions(-) create mode 100644 internal/accountpredicate/accountpredicate.go create mode 100644 internal/accountpredicate/accountpredicate_test.go diff --git a/internal/accountpredicate/accountpredicate.go b/internal/accountpredicate/accountpredicate.go new file mode 100644 index 000000000..97a0fe02d --- /dev/null +++ b/internal/accountpredicate/accountpredicate.go @@ -0,0 +1,100 @@ +// Package accountpredicate builds the dual-column account WHERE fragment +// shared by every read path that scopes savings_snapshots or purchase_history +// rows to a set of cloud accounts (issue #120). Keeping the one builder here +// means a predicate fix is a single edit instead of three that can drift. +// +// Both tables carry two account identifiers, either of which may be the only +// one populated on a row: cloud_account_id (the cloud_accounts UUID FK, NULL +// on the AWS ambient and legacy rows) and account_id (the cloud-provider +// external number). Matching only cloud_account_id silently dropped every NULL +// row (issues #701/#498) and matching only account_id dropped UUID-only rows +// (#866), so the predicate ORs both: +// +// (cloud_account_id = ANY($u) +// OR AND account_id = ANY($e1) +// OR ...) +// +// The external-id half is grouped per provider so each external number only +// matches rows of its own provider, preserving the (provider, external_id) +// pairing: a filter for aws/123 never pulls azure/123 rows. The "" provider key +// (legacy raw external number, unknown provider) matches account_id with no +// provider gate. cloud_account_id is compared directly (no ::text cast) so the +// plain cloud_account_id UUID index (migration 000011) can back it. Providers +// are sorted so the SQL and its bind order are deterministic. +package accountpredicate + +import ( + "fmt" + "sort" + "strings" +) + +// ProviderMatch selects how the per-provider gate compares the stored +// provider column with the filter's provider key. +type ProviderMatch int + +const ( + // ProviderExact gates on `provider = $p`. Used by the analytics reads of + // savings_snapshots and purchase_history. + ProviderExact ProviderMatch = iota + + // ProviderCaseInsensitiveOrUnknown gates on + // `(provider = '' OR LOWER(provider) = LOWER($p))`, so a row stored as + // "AWS" still matches an "aws" filter and a row with an empty (unknown) + // stored provider matches on account_id alone, like the "" filter key + // does (issue #24). A row stored under a different, non-empty provider is + // still excluded. Used by the internal/config purchase_history reads. + ProviderCaseInsensitiveOrUnknown +) + +// Build appends the account binds to args and returns the parenthesized OR +// clause that references them by position, plus the extended args. When +// neither input yields a disjunct (both empty, or every provider key maps to +// no IDs) it returns an empty clause and args unchanged; each caller decides +// what "no account filter" means for its query. +func Build(args []any, accountUUIDs []string, externalIDsByProvider map[string][]string, match ProviderMatch) (clause string, outArgs []any) { + var ors []string + if len(accountUUIDs) > 0 { + args = append(args, accountUUIDs) + ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) + } + for _, provider := range sortedProviderKeys(externalIDsByProvider) { + exts := externalIDsByProvider[provider] + if len(exts) == 0 { + continue + } + if provider == "" { + args = append(args, exts) + ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) + continue + } + args = append(args, provider) + providerArg := len(args) + args = append(args, exts) + ors = append(ors, fmt.Sprintf(match.gateFormat(), providerArg, len(args))) + } + if len(ors) == 0 { + return "", args + } + return "(" + strings.Join(ors, " OR ") + ")", args +} + +// gateFormat returns the per-provider disjunct with two %d verbs: the +// provider bind position, then the external-ID array bind position. +func (m ProviderMatch) gateFormat() string { + if m == ProviderCaseInsensitiveOrUnknown { + return "((provider = '' OR LOWER(provider) = LOWER($%d)) AND account_id = ANY($%d))" + } + return "(provider = $%d AND account_id = ANY($%d))" +} + +// sortedProviderKeys returns the map keys in ascending order so the generated +// SQL (and its bind-arg ordering) is deterministic across calls and testable. +func sortedProviderKeys(m map[string][]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/internal/accountpredicate/accountpredicate_test.go b/internal/accountpredicate/accountpredicate_test.go new file mode 100644 index 000000000..e6d22d2da --- /dev/null +++ b/internal/accountpredicate/accountpredicate_test.go @@ -0,0 +1,82 @@ +package accountpredicate + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestBuild_PinnedSQL pins the exact clause and bind args for both provider +// gates. The three callers (internal/analytics, internal/api, internal/config) +// produced these strings before the builder moved here (issue #120 item 5); +// each caller also keeps its own pinning test. +func TestBuild_PinnedSQL(t *testing.T) { + mixed := map[string][]string{"azure": {"sub-1"}, "": {"raw"}, "aws": {"123", "456"}, "gcp": nil} + tests := []struct { + name string + base []any + uuids []string + ext map[string][]string + match ProviderMatch + wantClause string + wantArgs []any + }{ + {name: "no filter keeps args", base: []any{"s"}, match: ProviderExact, wantClause: "", wantArgs: []any{"s"}}, + {name: "providers with no ids keep args", base: []any{"s"}, ext: map[string][]string{"aws": {}, "azure": nil}, match: ProviderExact, wantClause: "", wantArgs: []any{"s"}}, + {name: "no filter, nil base", match: ProviderCaseInsensitiveOrUnknown, wantClause: "", wantArgs: nil}, + { + name: "uuids only", + base: []any{"s", "e"}, + uuids: []string{"u1", "u2"}, + match: ProviderExact, + wantClause: "(cloud_account_id = ANY($3))", + wantArgs: []any{"s", "e", []string{"u1", "u2"}}, + }, + { + name: "exact gate, mixed providers sorted", + base: []any{"s", "e"}, + uuids: []string{"u1"}, + ext: mixed, + match: ProviderExact, + wantClause: "(cloud_account_id = ANY($3) OR account_id = ANY($4) OR " + + "(provider = $5 AND account_id = ANY($6)) OR (provider = $7 AND account_id = ANY($8)))", + wantArgs: []any{"s", "e", []string{"u1"}, []string{"raw"}, "aws", []string{"123", "456"}, "azure", []string{"sub-1"}}, + }, + { + name: "case-insensitive gate, mixed providers sorted", + uuids: []string{"u1"}, + ext: mixed, + match: ProviderCaseInsensitiveOrUnknown, + wantClause: "(cloud_account_id = ANY($1) OR account_id = ANY($2) OR " + + "((provider = '' OR LOWER(provider) = LOWER($3)) AND account_id = ANY($4)) OR " + + "((provider = '' OR LOWER(provider) = LOWER($5)) AND account_id = ANY($6)))", + wantArgs: []any{[]string{"u1"}, []string{"raw"}, "aws", []string{"123", "456"}, "azure", []string{"sub-1"}}, + }, + { + name: "provider key is bound as given", + ext: map[string][]string{"AWS": {"123"}}, + match: ProviderCaseInsensitiveOrUnknown, + wantClause: "(((provider = '' OR LOWER(provider) = LOWER($1)) AND account_id = ANY($2)))", + wantArgs: []any{"AWS", []string{"123"}}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + clause, args := Build(tc.base, tc.uuids, tc.ext, tc.match) + assert.Equal(t, tc.wantClause, clause) + assert.Equal(t, tc.wantArgs, args) + }) + } +} + +// TestBuild_Deterministic guards the sorted provider order: Go map iteration +// is randomized, so an unsorted builder would fail this within a few runs. +func TestBuild_Deterministic(t *testing.T) { + ext := map[string][]string{"gcp": {"p"}, "azure": {"s"}, "aws": {"a"}, "oci": {"o"}} + first, firstArgs := Build(nil, nil, ext, ProviderExact) + for range 50 { + clause, args := Build(nil, nil, ext, ProviderExact) + assert.Equal(t, first, clause) + assert.Equal(t, firstArgs, args) + } +} diff --git a/internal/analytics/postgres_analytics.go b/internal/analytics/postgres_analytics.go index bacecca0a..46b7faf42 100644 --- a/internal/analytics/postgres_analytics.go +++ b/internal/analytics/postgres_analytics.go @@ -5,10 +5,9 @@ import ( "database/sql" "encoding/json" "fmt" - "sort" - "strings" "time" + "github.com/LeanerCloud/cloud-commitments-platform/internal/accountpredicate" "github.com/LeanerCloud/cloud-commitments-platform/internal/database" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -42,51 +41,16 @@ var _ AnalyticsStore = (*PostgresAnalyticsStore)(nil) // leading binds; the account array binds (if any) are appended after them and // the returned clause references them by the right positions. // -// savings_snapshots carries two account identifiers, either of which may be the -// only one populated on a row: account_id (the cloud-provider external number) -// and cloud_account_id (the cloud_accounts UUID FK, NULL on the AWS ambient and -// legacy rows). Matching only one column silently drops rows that carry only the -// other, so we OR both, with the external-id half grouped per provider so a -// reused external number across providers cannot leak the wrong rows. This -// mirrors api.accountFilterClause on the live purchase_history path -// (issue #701/#498/#866). Both empty -> "TRUE" (caller must enforce scope). +// The predicate itself lives in internal/accountpredicate, shared with the +// purchase_history read paths in internal/api and internal/config (issue +// #120), so a fix to it is one edit. Both empty -> "TRUE" (caller must enforce +// scope). func accountFilterClause(accountUUIDs []string, accountExternalIDsByProvider map[string][]string, baseArgs []any) (clause string, args []any) { - args = baseArgs - var ors []string - if len(accountUUIDs) > 0 { - args = append(args, accountUUIDs) - ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) - } - for _, provider := range sortedProviderKeys(accountExternalIDsByProvider) { - exts := accountExternalIDsByProvider[provider] - if len(exts) == 0 { - continue - } - if provider == "" { - args = append(args, exts) - ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) - continue - } - args = append(args, provider) - providerArg := len(args) - args = append(args, exts) - ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args))) - } - if len(ors) == 0 { + clause, args = accountpredicate.Build(baseArgs, accountUUIDs, accountExternalIDsByProvider, accountpredicate.ProviderExact) + if clause == "" { return "TRUE", args } - return "(" + strings.Join(ors, " OR ") + ")", args -} - -// sortedProviderKeys returns the map keys in ascending order so generated SQL -// (and its bind-arg ordering) is deterministic and testable. -func sortedProviderKeys(m map[string][]string) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - return keys + return clause, args } // ========================================== diff --git a/internal/analytics/postgres_analytics_mock_test.go b/internal/analytics/postgres_analytics_mock_test.go index 5f164b1a2..9d89d54c6 100644 --- a/internal/analytics/postgres_analytics_mock_test.go +++ b/internal/analytics/postgres_analytics_mock_test.go @@ -66,6 +66,26 @@ func TestAccountFilterClause(t *testing.T) { assert.Equal(t, "(account_id = ANY($3))", clause) assert.Len(t, args, 3) }) + + // Pins the exact bind args too (issue #120 item 5), so moving the builder + // into the shared package cannot reorder or drop a bind. + t.Run("bind args are pinned in clause order", func(t *testing.T) { + clause, args := accountFilterClause( + []string{"u1"}, + map[string][]string{"azure": {"sub-1"}, "": {"raw"}, "aws": {"123"}, "gcp": nil}, + base(), + ) + assert.Equal(t, + "(cloud_account_id = ANY($3) OR account_id = ANY($4) OR (provider = $5 AND account_id = ANY($6)) OR (provider = $7 AND account_id = ANY($8)))", + clause) + assert.Equal(t, []any{"start", "end", []string{"u1"}, []string{"raw"}, "aws", []string{"123"}, "azure", []string{"sub-1"}}, args) + }) + + t.Run("providers with no ids yield TRUE", func(t *testing.T) { + clause, args := accountFilterClause(nil, map[string][]string{"aws": {}}, base()) + assert.Equal(t, "TRUE", clause) + assert.Equal(t, []any{"start", "end"}, args) + }) } // TestPartitionDateCalculation verifies partition date calculation logic. diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index 7b3453da4..141c082fc 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -5,10 +5,9 @@ import ( "context" "database/sql" "fmt" - "sort" - "strings" "time" + "github.com/LeanerCloud/cloud-commitments-platform/internal/accountpredicate" "github.com/LeanerCloud/cloud-commitments-platform/internal/database" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" @@ -83,62 +82,18 @@ func dimensionToColumn(dimension string) (string, error) { // are appended after them and the returned clause references them by the right // positions. // -// purchase_history carries two account identifiers, either of which may be the -// only one populated on a row: account_id (the cloud-provider external number, -// always populated) and cloud_account_id (the cloud_accounts UUID FK, NULL on -// every direct-execute/ambient/pre-000011 row). The top-bar chip emits the -// UUID, so matching only cloud_account_id silently dropped every NULL row -// (issue #701/#498) and matching only account_id dropped UUID-only rows (#866). -// -// The external-id half is grouped per provider so each external number only -// matches rows of its own provider: -// -// (cloud_account_id = ANY($u) -// OR (provider = $p AND account_id = ANY($extsForP)) OR ...) -// -// preserving the (provider, external_id) pairing so a filter for aws/123 never -// pulls azure/123 rows. The "" provider key (legacy raw external number, unknown -// provider) matches account_id with no provider gate. cloud_account_id is -// compared directly (no ::text cast) so the plain cloud_account_id UUID index -// (migration 000011) can back it. Providers are sorted for deterministic SQL. -// Both empty -> "TRUE" (no account filter). +// purchase_history carries two account identifiers (account_id, always +// populated, and cloud_account_id, NULL on every direct-execute/ambient/ +// pre-000011 row), so the predicate ORs both with a per-provider gate on the +// external-id half. It lives in internal/accountpredicate, shared with the +// savings_snapshots reads in internal/analytics and the purchase_history reads +// in internal/config (issue #120). Both empty -> "TRUE" (no account filter). func accountFilterClause(accountUUIDs []string, accountExternalIDsByProvider map[string][]string, baseArgs []any) (clause string, args []any) { - args = baseArgs - var ors []string - if len(accountUUIDs) > 0 { - args = append(args, accountUUIDs) - ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) - } - for _, provider := range sortedProviderKeys(accountExternalIDsByProvider) { - exts := accountExternalIDsByProvider[provider] - if len(exts) == 0 { - continue - } - if provider == "" { - args = append(args, exts) - ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) - continue - } - args = append(args, provider) - providerArg := len(args) - args = append(args, exts) - ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args))) - } - if len(ors) == 0 { + clause, args = accountpredicate.Build(baseArgs, accountUUIDs, accountExternalIDsByProvider, accountpredicate.ProviderExact) + if clause == "" { return "TRUE", args } - return "(" + strings.Join(ors, " OR ") + ")", args -} - -// sortedProviderKeys returns the map keys in ascending order so the generated -// SQL (and its bind-arg ordering) is deterministic across calls and testable. -func sortedProviderKeys(m map[string][]string) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - return keys + return clause, args } // QueryHistory aggregates purchase_history rows bucketed by interval. diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index 2e44de369..05cc2c534 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -328,3 +328,40 @@ func TestQueryHistory_ExternalIDFilter(t *testing.T) { require.NoError(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } + +// TestAccountFilterClause_PinnedSQL pins the exact clause and bind args the +// purchase_history analytics queries get from the shared account predicate +// (issue #120 item 5), so moving the builder cannot change the generated SQL. +func TestAccountFilterClause_PinnedSQL(t *testing.T) { + tests := []struct { + name string + uuids []string + ext map[string][]string + wantClause string + wantArgs []any + }{ + {name: "no filter", wantClause: "TRUE", wantArgs: []any{"s", "e"}}, + {name: "providers with no ids", ext: map[string][]string{"aws": nil}, wantClause: "TRUE", wantArgs: []any{"s", "e"}}, + { + name: "uuids only", + uuids: []string{"u1", "u2"}, + wantClause: "(cloud_account_id = ANY($3))", + wantArgs: []any{"s", "e", []string{"u1", "u2"}}, + }, + { + name: "uuid, unknown provider and two providers in sorted order", + uuids: []string{"u1"}, + ext: map[string][]string{"azure": {"sub-1"}, "": {"raw"}, "aws": {"123"}}, + wantClause: "(cloud_account_id = ANY($3) OR account_id = ANY($4) OR " + + "(provider = $5 AND account_id = ANY($6)) OR (provider = $7 AND account_id = ANY($8)))", + wantArgs: []any{"s", "e", []string{"u1"}, []string{"raw"}, "aws", []string{"123"}, "azure", []string{"sub-1"}}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + clause, args := accountFilterClause(tc.uuids, tc.ext, []any{"s", "e"}) + assert.Equal(t, tc.wantClause, clause) + assert.Equal(t, tc.wantArgs, args) + }) + } +} diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c4d9767b6..161b1849a 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -16,6 +16,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/accountpredicate" "github.com/LeanerCloud/cloud-commitments-platform/internal/database" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -2631,74 +2632,27 @@ func activePurchaseHistoryQuery(asOf time.Time, accountIDs []string, externalIDs return query, args } -// appendAccountPredicate pulls the dual-column account-predicate arg-building -// branch out of GetPurchaseHistoryFiltered to keep it under the cyclomatic limit. +// appendAccountPredicate appends the dual-column account predicate to the +// GetPurchaseHistoryFiltered / GetActivePurchaseHistory AND chain. // // Either input alone is sufficient: a UUID-only filter still matches // NULL-cloud_account_id rows via the resolved external IDs, and an // external-only filter matches UUID-only rows once the caller resolves them. // Both empty returns conds/args unchanged (no account clause -> all accounts). // -// The external-id half is grouped per provider so each external number is only -// compared against rows of its own provider: -// -// (cloud_account_id = ANY($u) -// OR ((provider = '' OR LOWER(provider) = LOWER($p1)) AND account_id = ANY($e1)) -// OR ((provider = '' OR LOWER(provider) = LOWER($p2)) AND account_id = ANY($e2))) -// -// This preserves the (provider, external_id) pairing so a filter for aws/123 -// never matches azure/123 rows. The "" provider key (legacy raw external -// number, unknown provider) matches account_id with no provider gate. Providers -// are sorted for deterministic SQL. The OR is wrapped in parentheses so it -// composes with the surrounding AND chain. -// -// The provider gate is case-insensitive, and a row whose stored provider is -// empty (unknown) matches on account_id alone, the same way the "" filter key -// does. Without this, a NULL-cloud_account_id row stored as "AWS" or "" was -// silently dropped even though its account_id matched (issue #24). A row -// stored under a different, non-empty provider is still excluded. +// The predicate is built by internal/accountpredicate, shared with the +// analytics reads in internal/analytics and internal/api (issue #120). This +// path uses the case-insensitive provider gate, where a row whose stored +// provider is empty (unknown) matches on account_id alone; without it a +// NULL-cloud_account_id row stored as "AWS" or "" was silently dropped even +// though its account_id matched (issue #24). A row stored under a different, +// non-empty provider is still excluded. func appendAccountPredicate(conds []string, args []any, accountIDs []string, externalIDsByProvider map[string][]string) (outConds []string, outArgs []any) { - if len(accountIDs) == 0 && len(externalIDsByProvider) == 0 { - return conds, args - } - var ors []string - if len(accountIDs) > 0 { - args = append(args, accountIDs) - ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args))) - } - for _, provider := range sortedProviderKeys(externalIDsByProvider) { - exts := externalIDsByProvider[provider] - if len(exts) == 0 { - continue - } - if provider == "" { - args = append(args, exts) - ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args))) - continue - } - args = append(args, provider) - providerArg := len(args) - args = append(args, exts) - ors = append(ors, fmt.Sprintf( - "((provider = '' OR LOWER(provider) = LOWER($%d)) AND account_id = ANY($%d))", - providerArg, len(args))) - } - if len(ors) == 0 { + clause, args := accountpredicate.Build(args, accountIDs, externalIDsByProvider, accountpredicate.ProviderCaseInsensitiveOrUnknown) + if clause == "" { return conds, args } - conds = append(conds, "("+strings.Join(ors, " OR ")+")") - return conds, args -} - -// sortedProviderKeys returns the map keys in ascending order so the generated -// SQL (and its bind-arg ordering) is deterministic across calls and testable. -func sortedProviderKeys(m map[string][]string) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - return keys + return append(conds, clause), args } // GetPurchaseHistoryFiltered reads purchase_history rows matching the diff --git a/internal/config/store_postgres_account_predicate_test.go b/internal/config/store_postgres_account_predicate_test.go index 9f5e9cba3..6648ade23 100644 --- a/internal/config/store_postgres_account_predicate_test.go +++ b/internal/config/store_postgres_account_predicate_test.go @@ -46,6 +46,11 @@ func TestAppendAccountPredicate_ProviderGate(t *testing.T) { "((provider = '' OR LOWER(provider) = LOWER($4)) AND account_id = ANY($5)))"}, wantArgs: []any{[]string{"uuid-1"}, "aws", []string{"123"}, "azure", []string{"123"}}, }, + { + name: "providers with no ids leave conds and args unchanged", + ext: map[string][]string{"aws": {}}, + wantArgs: nil, + }, { name: "unknown-provider key has no provider gate", ext: map[string][]string{"": {"123"}}, From cb25735796907e91eadf40037826df4c1b894cbe Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:49:15 +0000 Subject: [PATCH 2/3] fix(db): key the RI utilization cache by cloud account ri_utilization_cache was keyed (region, lookback_days) and the singleflight key was region#lookback, so once the reshape path reads with per-account credentials, account B would be served account A's cached Cost Explorer utilization for the same region, and B's background refresh would join A's in-flight call. Migration 000150 adds cloud_account_id to the primary key (empties the cache, which has no account identity to backfill, under an exclusive lock so an old-release writer cannot fail the NOT NULL add). The store, interface and mock take a RIUtilizationCacheKey; the cache's singleflight key includes the account. getRIUtilization and getReshapeRecommendations resolve the deployment account once, after validating query params, and key the cache with it. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 68 ++++-- .../handler_ri_exchange_integration_test.go | 5 +- .../api/handler_ri_utilization_scope_test.go | 60 +++++- internal/api/ri_utilization_cache.go | 50 +++-- .../ri_utilization_cache_integration_test.go | 82 +++++++- internal/api/ri_utilization_cache_test.go | 196 +++++++++++++----- internal/config/interfaces.go | 8 +- .../config/store_postgres_ri_utilization.go | 31 +-- internal/config/types.go | 23 +- ..._ri_utilization_cache_account_key.down.sql | 11 + ...50_ri_utilization_cache_account_key.up.sql | 29 +++ ...0_ri_utilization_cache_account_key_test.go | 99 +++++++++ internal/mocks/stores.go | 12 +- 13 files changed, 537 insertions(+), 137 deletions(-) create mode 100644 internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.down.sql create mode 100644 internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.up.sql create mode 100644 internal/database/postgres/migrations/000150_ri_utilization_cache_account_key_test.go diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 8a26ea6d7..faf24bab3 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1412,6 +1412,23 @@ func (h *Handler) reshapeCloudAccountInScope(ctx context.Context, session *Sessi return allowed.Allows(cloudAccountID, ""), nil } +// resolveReshapeAccountScope resolves the deployment's cloud account once and +// reports whether the session's allowed_accounts permit it. Unlike +// reshapeCloudAccountInScope it always resolves, even for unrestricted +// sessions, because the RI utilization reads need the account to key their +// cache (issue #120). Returns ("", false, err) on any lookup failure. +func (h *Handler) resolveReshapeAccountScope(ctx context.Context, session *Session) (cloudAccountID string, inScope bool, err error) { + allowed, err := h.getAccountScope(ctx, session) + if err != nil { + return "", false, fmt.Errorf("failed to get allowed accounts: %w", err) + } + cloudAccountID, err = h.resolveReshapeCloudAccountID(ctx) + if err != nil { + return "", false, fmt.Errorf("failed to resolve cloud account scope: %w", err) + } + return cloudAccountID, allowed.AllowsAll() || allowed.Allows(cloudAccountID, ""), nil +} + // requireReshapeAccountScope enforces the session's allowed_accounts scope for // executeExchange (issue #93), returning 403 instead of an empty response // since exchange execution is a mutating, financially irreversible action. @@ -1542,18 +1559,22 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti return nil, err } - // Apply the session's allowed_accounts scope. - if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { - return nil, sErr - } else if !inScope { - return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil - } - + // Validate input before resolving the account, so a malformed request + // makes no STS or account lookup. lookbackDays, err := parseLookbackDaysParam(req.QueryStringParameters) if err != nil { return nil, err } + // Apply the session's allowed_accounts scope. The resolved account also + // keys the utilization cache, so two accounts never share a cached row. + cloudAccountID, inScope, sErr := h.resolveReshapeAccountScope(ctx, session) + if sErr != nil { + return nil, sErr + } else if !inScope { + return &RIUtilizationResponse{Utilization: []recommendations.RIUtilization{}}, nil + } + region := req.QueryStringParameters["region"] cfg, err := h.loadAWSConfigWithRegion(ctx, region) if err != nil { @@ -1570,7 +1591,8 @@ func (h *Handler) getRIUtilization(ctx context.Context, req *events.LambdaFuncti fetch := func(fetchCtx context.Context, days int) ([]recommendations.RIUtilization, error) { return recsAdapter.GetRIUtilization(fetchCtx, days, region) } - utilization, err := h.getRIUtilizationCache().getOrFetch(ctx, region, lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) + cacheKey := config.RIUtilizationCacheKey{CloudAccountID: cloudAccountID, Region: region, LookbackDays: lookbackDays} + utilization, err := h.getRIUtilizationCache().getOrFetch(ctx, cacheKey, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) if err != nil { return nil, fmt.Errorf("failed to get RI utilization: %w", err) } @@ -1737,18 +1759,22 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam return nil, err } - // Apply the session's allowed_accounts scope. - if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { - return nil, sErr - } else if !inScope { - return &ReshapeRecommendationsResponse{Recommendations: []exchange.ReshapeRecommendation{}}, nil - } - + // Validate input before resolving the account, so a malformed request + // makes no STS or account lookup. p, err := parseReshapeParams(req.QueryStringParameters) if err != nil { return nil, err } + // Apply the session's allowed_accounts scope. The resolved account also + // keys the utilization cache and scopes the recs lookup below. + cloudAccountID, inScope, sErr := h.resolveReshapeAccountScope(ctx, session) + if sErr != nil { + return nil, sErr + } else if !inScope { + return &ReshapeRecommendationsResponse{Recommendations: []exchange.ReshapeRecommendation{}}, nil + } + cfg, err := h.loadAWSConfigWithRegion(ctx, p.region) if err != nil { return nil, fmt.Errorf("failed to load AWS config: %w", err) @@ -1773,7 +1799,8 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam fetch := func(fetchCtx context.Context, days int) ([]recommendations.RIUtilization, error) { return recsAdapter.GetRIUtilization(fetchCtx, days, p.region) } - utilData, err := h.getRIUtilizationCache().getOrFetch(ctx, p.region, p.lookbackDays, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) + cacheKey := config.RIUtilizationCacheKey{CloudAccountID: cloudAccountID, Region: p.region, LookbackDays: p.lookbackDays} + utilData, err := h.getRIUtilizationCache().getOrFetch(ctx, cacheKey, riUtilizationCacheTTL, riUtilizationCacheStaleTTL, fetch) if err != nil { return nil, fmt.Errorf("failed to get RI utilization: %w", err) } @@ -1787,12 +1814,9 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam // surface another tenant's recs. Empty resolved account ID means // "no scope filter" for ambient-credentials deployments where // CloudAccount registration hasn't happened yet; a real ListCloudAccounts - // error aborts the request instead of silently falling through to an - // unscoped query that could match the wrong tenant's recs. - cloudAccountID, err := h.resolveReshapeCloudAccountID(ctx) - if err != nil { - return nil, fmt.Errorf("failed to resolve cloud account scope for reshape: %w", err) - } + // error aborted the request at the scope check above instead of silently + // falling through to an unscoped query that could match the wrong + // tenant's recs. currencyCode := firstNonEmptyCurrency(instances) lookup := purchaseRecLookupFromStore(h.config, cloudAccountID) recs := exchange.AnalyzeReshapingWithRecs(ctx, riInfos, utilInfos, p.threshold, p.region, currencyCode, lookup) diff --git a/internal/api/handler_ri_exchange_integration_test.go b/internal/api/handler_ri_exchange_integration_test.go index 40fe1b7fe..1e8129f12 100644 --- a/internal/api/handler_ri_exchange_integration_test.go +++ b/internal/api/handler_ri_exchange_integration_test.go @@ -174,8 +174,9 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) { "same-family m5.* alternatives must be excluded, got %s", alt.InstanceType) } - // Verify the RI utilization cache row landed in Postgres. - entry, err := store.GetRIUtilizationCache(ctx, "us-east-1", 30) + // Verify the RI utilization cache row landed in Postgres, under the + // unattributed account key the default resolver ("") yields. + entry, err := store.GetRIUtilizationCache(ctx, config.RIUtilizationCacheKey{Region: "us-east-1", LookbackDays: 30}) require.NoError(t, err) require.NotNil(t, entry) var cached []recommendations.RIUtilization diff --git a/internal/api/handler_ri_utilization_scope_test.go b/internal/api/handler_ri_utilization_scope_test.go index 19abd7731..41ce74cd0 100644 --- a/internal/api/handler_ri_utilization_scope_test.go +++ b/internal/api/handler_ri_utilization_scope_test.go @@ -59,9 +59,9 @@ func newUtilizationScopeStore() *MockConfigStore { store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } - store.On("GetRIUtilizationCache", mock.Anything, mock.Anything, mock.Anything). + store.On("GetRIUtilizationCache", mock.Anything, mock.Anything). Return((*config.RIUtilizationCacheEntry)(nil), nil).Maybe() - store.On("UpsertRIUtilizationCache", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + store.On("UpsertRIUtilizationCache", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(nil).Maybe() return store } @@ -214,3 +214,59 @@ func TestGetRIUtilization_PermissionCheckedBeforeScope(t *testing.T) { assert.Zero(t, recs.calls) auth.AssertNotCalled(t, "GetAllowedAccountsAPI", mock.Anything, mock.Anything) } + +// Two deployment accounts reading the same region and lookback must each get +// their own Cost Explorer result: the resolved cloud account is part of the +// cache key (issue #120). With the old (region, lookback_days) key the second +// account's request was served the first account's cached rows. +func TestGetRIUtilization_AccountsInSameRegionDoNotShareCache(t *testing.T) { + ctx := context.Background() + auth, req := adminDashboardReq(ctx) + req.QueryStringParameters = map[string]string{"region": "us-east-1", "lookback_days": "30"} + recs := &countingUtilRecs{} + h := newUtilizationScopeHandler(auth, newUtilizationScopeStore(), ownUtilEC2(), recs, permsAccA) + cacheStore := newFakeRIUtilCacheStore() + h.riUtilizationCacheOnce.Do(func() { h.riUtilizationCache = newRIUtilizationCache(cacheStore, false) }) + account := permsAccA + h.reshapeAccountResolver = func(_ context.Context) (string, error) { return account, nil } + + recs.rows = []recommendations.RIUtilization{{ReservedInstanceID: "ri-account-a"}} + resp, err := h.getRIUtilization(ctx, req) + require.NoError(t, err) + assert.Equal(t, []string{"ri-account-a"}, utilIDs(t, resp)) + + account = permsAccB + recs.rows = []recommendations.RIUtilization{{ReservedInstanceID: "ri-account-b"}} + resp, err = h.getRIUtilization(ctx, req) + require.NoError(t, err) + assert.Equal(t, []string{"ri-account-b"}, utilIDs(t, resp), "account B must not be served account A's cached utilization") + assert.Equal(t, 2, recs.calls, "account B's read is a cache miss and calls Cost Explorer") + + account = permsAccA + resp, err = h.getRIUtilization(ctx, req) + require.NoError(t, err) + assert.Equal(t, []string{"ri-account-a"}, utilIDs(t, resp), "account A still reads its own cached row") + assert.Equal(t, 2, recs.calls, "account A's second read is a cache hit") + + _, ok := cacheStore.latest(config.RIUtilizationCacheKey{CloudAccountID: permsAccA, Region: "us-east-1", LookbackDays: 30}) + assert.True(t, ok, "account A's row is stored under its own key") +} + +// A failure to resolve the deployment account fails the utilization read +// closed for every session, with no Cost Explorer call: without the account +// the cache row cannot be keyed (issue #120). +func TestGetRIUtilization_AccountResolutionErrorFailsClosed(t *testing.T) { + ctx := context.Background() + auth, req := adminDashboardReq(ctx) + recs := &countingUtilRecs{rows: payerUtilizationRows()} + h := newUtilizationScopeHandler(auth, newUtilizationScopeStore(), ownUtilEC2(), recs, permsAccA) + h.reshapeAccountResolver = func(_ context.Context) (string, error) { + return "", errors.New("sts get-caller-identity denied") + } + + resp, err := h.getRIUtilization(ctx, req) + + require.ErrorContains(t, err, "resolve cloud account scope") + assert.Nil(t, resp) + assert.Zero(t, recs.calls) +} diff --git a/internal/api/ri_utilization_cache.go b/internal/api/ri_utilization_cache.go index 616811890..431ff8956 100644 --- a/internal/api/ri_utilization_cache.go +++ b/internal/api/ri_utilization_cache.go @@ -52,8 +52,8 @@ func resolveDurationEnv(name string, def time.Duration) time.Duration { // that the cache needs. Declared here (and satisfied structurally) so // tests can inject a fake without depending on the full store. type riUtilizationCacheStore interface { - GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*config.RIUtilizationCacheEntry, error) - UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error + GetRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey) (*config.RIUtilizationCacheEntry, error) + UpsertRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey, payload []byte, fetchedAt time.Time) error } // riUtilizationFetcher is the signature of the function that retrieves @@ -70,8 +70,10 @@ type riUtilizationFetcher func(ctx context.Context, lookbackDays int) ([]recomme // the next reader. // // The singleflight.Group ensures that N concurrent stale-reads for the -// same (region, lookback) tuple collapse to exactly one background -// refresh, avoiding a thundering-herd CE fan-out. +// same (account, region, lookback) tuple collapse to exactly one +// background refresh, avoiding a thundering-herd CE fan-out. The account +// is part of the key so one account's refresh never stands in for +// another's (issue #120). type riUtilizationCache struct { store riUtilizationCacheStore sf singleflight.Group @@ -91,16 +93,19 @@ func newRIUtilizationCache(store riUtilizationCacheStore, isLambda bool) *riUtil // Fetch errors are NOT cached — a transient CE 5xx must not lock the // dashboard out for the full TTL. A cache-write error after a fresh // fetch is logged but not returned — the caller already has data. +// +// cacheKey.CloudAccountID must identify the account whose credentials +// fetch uses, so the row and the in-flight refresh are never shared +// between accounts. func (c *riUtilizationCache) getOrFetch( ctx context.Context, - region string, - lookbackDays int, + cacheKey config.RIUtilizationCacheKey, softTTL, hardTTL time.Duration, fetch riUtilizationFetcher, ) ([]recommendations.RIUtilization, error) { - key := fmt.Sprintf("%s#%d", region, lookbackDays) + key := riUtilizationFlightKey(cacheKey) - entry, err := c.store.GetRIUtilizationCache(ctx, region, lookbackDays) + entry, err := c.store.GetRIUtilizationCache(ctx, cacheKey) if err != nil { return nil, fmt.Errorf("failed to read ri utilization cache: %w", err) } @@ -112,7 +117,7 @@ func (c *riUtilizationCache) getOrFetch( // Corrupt payload — log and fall through to a fresh // fetch so one bad row can't lock the dashboard out // for an entire TTL window. - logging.Warnf("ri_utilization_cache: corrupt payload for region=%q lookback=%d: %v", region, lookbackDays, unmarshalErr) + logging.Warnf("ri_utilization_cache: corrupt payload (key=%s): %v", key, unmarshalErr) } else { switch { case age < softTTL: @@ -124,7 +129,7 @@ func (c *riUtilizationCache) getOrFetch( break } logging.Infof("ri_utilization_cache: serving stale + kicking background refresh (key=%s age=%s)", key, age) - c.kickBackgroundRefresh(key, region, lookbackDays, fetch) + c.kickBackgroundRefresh(key, cacheKey, fetch) return data, nil default: logging.Infof("ri_utilization_cache: entry beyond hard expiry; synchronous refetch (key=%s age=%s)", key, age) @@ -133,21 +138,28 @@ func (c *riUtilizationCache) getOrFetch( } } - data, err := fetch(ctx, lookbackDays) + data, err := fetch(ctx, cacheKey.LookbackDays) if err != nil { return nil, err } - c.storePayload(ctx, region, lookbackDays, data) + c.storePayload(ctx, cacheKey, data) return data, nil } +// riUtilizationFlightKey is the singleflight key for one cache row. Every +// field of the row key is in it, quoted so a region string can never make +// two different keys print the same. +func riUtilizationFlightKey(k config.RIUtilizationCacheKey) string { + return fmt.Sprintf("%q#%q#%d", k.CloudAccountID, k.Region, k.LookbackDays) +} + // kickBackgroundRefresh runs a single-flighted refetch in a detached // goroutine. sf.Do with the same key collapses concurrent calls to // one in-flight refresh. The refresh uses a fresh context (not the // caller's) because the caller's ctx may be canceled when the HTTP // response completes, which would abort the refresh prematurely. -func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackDays int, fetch riUtilizationFetcher) { +func (c *riUtilizationCache) kickBackgroundRefresh(key string, cacheKey config.RIUtilizationCacheKey, fetch riUtilizationFetcher) { go func() { defer func() { if r := recover(); r != nil { @@ -159,12 +171,12 @@ func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackD ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - data, err := fetch(ctx, lookbackDays) + data, err := fetch(ctx, cacheKey.LookbackDays) if err != nil { logging.Warnf("ri_utilization_cache: background refresh failed (key=%s): %v", key, err) return nil, err } - c.storePayload(ctx, region, lookbackDays, data) + c.storePayload(ctx, cacheKey, data) return nil, nil }); sfErr != nil { // error already logged inside the fetch func above @@ -175,14 +187,14 @@ func (c *riUtilizationCache) kickBackgroundRefresh(key, region string, lookbackD // storePayload marshals and upserts the fresh data. Errors are logged // but not returned — callers have the data in hand either way. -func (c *riUtilizationCache) storePayload(ctx context.Context, region string, lookbackDays int, data []recommendations.RIUtilization) { +func (c *riUtilizationCache) storePayload(ctx context.Context, cacheKey config.RIUtilizationCacheKey, data []recommendations.RIUtilization) { payload, err := json.Marshal(data) if err != nil { - logging.Warnf("ri_utilization_cache: failed to marshal payload for region=%q lookback=%d: %v", region, lookbackDays, err) + logging.Warnf("ri_utilization_cache: failed to marshal payload (key=%s): %v", riUtilizationFlightKey(cacheKey), err) return } - if err := c.store.UpsertRIUtilizationCache(ctx, region, lookbackDays, payload, time.Now()); err != nil { - logging.Warnf("ri_utilization_cache: failed to upsert row for region=%q lookback=%d: %v", region, lookbackDays, err) + if err := c.store.UpsertRIUtilizationCache(ctx, cacheKey, payload, time.Now()); err != nil { + logging.Warnf("ri_utilization_cache: failed to upsert row (key=%s): %v", riUtilizationFlightKey(cacheKey), err) } } diff --git a/internal/api/ri_utilization_cache_integration_test.go b/internal/api/ri_utilization_cache_integration_test.go index 27d3aa3ed..c9d270a0c 100644 --- a/internal/api/ri_utilization_cache_integration_test.go +++ b/internal/api/ri_utilization_cache_integration_test.go @@ -53,16 +53,17 @@ func TestRIUtilizationCache_Integration_ColdReadFetchesAndPersists(t *testing.T) return fetched, nil } - got, err := cache.getOrFetch(ctx, "us-east-1", 30, time.Hour, 2*time.Hour, fetch) + got, err := cache.getOrFetch(ctx, usEast30, time.Hour, 2*time.Hour, fetch) require.NoError(t, err) require.Equal(t, int32(1), calls.Load()) require.Len(t, got, 1) require.Equal(t, "ri-integration-1", got[0].ReservedInstanceID) // Postgres row must be present with the correct payload. - entry, err := store.GetRIUtilizationCache(ctx, "us-east-1", 30) + entry, err := store.GetRIUtilizationCache(ctx, usEast30) require.NoError(t, err) require.NotNil(t, entry) + require.Equal(t, riUtilAccountA, entry.CloudAccountID) require.Equal(t, "us-east-1", entry.Region) require.Equal(t, 30, entry.LookbackDays) var round []recommendations.RIUtilization @@ -86,12 +87,12 @@ func TestRIUtilizationCache_Integration_FreshHitNoFetch(t *testing.T) { } // Prime the cache. - _, err := cache.getOrFetch(ctx, "us-east-1", 30, time.Hour, 2*time.Hour, fetch) + _, err := cache.getOrFetch(ctx, usEast30, time.Hour, 2*time.Hour, fetch) require.NoError(t, err) require.Equal(t, int32(1), calls.Load()) // Second read: well within soft TTL. - _, err = cache.getOrFetch(ctx, "us-east-1", 30, time.Hour, 2*time.Hour, fetch) + _, err = cache.getOrFetch(ctx, usEast30, time.Hour, 2*time.Hour, fetch) require.NoError(t, err) require.Equal(t, int32(1), calls.Load(), "fresh hit must not trigger a fetch") } @@ -111,7 +112,7 @@ func TestRIUtilizationCache_Integration_StaleNonLambdaServesStaleAndRefreshes(t // Seed a row whose age is between soft (50ms) and hard (1h). stalePayload, err := json.Marshal(staleData) require.NoError(t, err) - require.NoError(t, store.UpsertRIUtilizationCache(ctx, "us-east-1", 30, stalePayload, time.Now().Add(-200*time.Millisecond))) + require.NoError(t, store.UpsertRIUtilizationCache(ctx, usEast30, stalePayload, time.Now().Add(-200*time.Millisecond))) cache := newRIUtilizationCache(store, false) // non-Lambda release := make(chan struct{}) @@ -125,7 +126,7 @@ func TestRIUtilizationCache_Integration_StaleNonLambdaServesStaleAndRefreshes(t } got := getOrFetchWithin(t, 10*time.Second, func() ([]recommendations.RIUtilization, error) { - return cache.getOrFetch(ctx, "us-east-1", 30, 50*time.Millisecond, time.Hour, fetch) + return cache.getOrFetch(ctx, usEast30, 50*time.Millisecond, time.Hour, fetch) }, "stale-read must return synchronously, not wait for background fetch") require.Len(t, got, 1) require.Equal(t, "ri-stale", got[0].ReservedInstanceID, "first call must return the stale copy") @@ -133,7 +134,7 @@ func TestRIUtilizationCache_Integration_StaleNonLambdaServesStaleAndRefreshes(t // Let the background refresh finish and wait for it to land the fresh data. releaseFetch() require.Eventually(t, func() bool { - entry, err := store.GetRIUtilizationCache(ctx, "us-east-1", 30) + entry, err := store.GetRIUtilizationCache(ctx, usEast30) if err != nil || entry == nil { return false } @@ -156,21 +157,82 @@ func TestRIUtilizationCache_Integration_HardExpiredSyncRefetch(t *testing.T) { stalePayload, err := json.Marshal(staleData) require.NoError(t, err) // Age 2h exceeds both soft (15m) and hard (1h). - require.NoError(t, store.UpsertRIUtilizationCache(ctx, "us-east-1", 30, stalePayload, time.Now().Add(-2*time.Hour))) + require.NoError(t, store.UpsertRIUtilizationCache(ctx, usEast30, stalePayload, time.Now().Add(-2*time.Hour))) cache := newRIUtilizationCache(store, false) fetch := func(ctx context.Context, lookbackDays int) ([]recommendations.RIUtilization, error) { return freshData, nil } - got, err := cache.getOrFetch(ctx, "us-east-1", 30, 15*time.Minute, time.Hour, fetch) + got, err := cache.getOrFetch(ctx, usEast30, 15*time.Minute, time.Hour, fetch) require.NoError(t, err) require.Equal(t, "ri-fresh", got[0].ReservedInstanceID, "hard-expired entry must force sync refetch") // Postgres row should have the fresh fetched_at timestamp, not // the 2h-old one we seeded. - entry, err := store.GetRIUtilizationCache(ctx, "us-east-1", 30) + entry, err := store.GetRIUtilizationCache(ctx, usEast30) require.NoError(t, err) require.NotNil(t, entry) require.Less(t, time.Since(entry.FetchedAt), 5*time.Second, "fetched_at must be updated after sync refetch") } + +// TestRIUtilizationCache_Integration_AccountsInSameRegionKeepOwnRows pins +// issue #120 on real Postgres: two accounts write the same region and lookback +// concurrently and each reads back only its own payload. Before migration +// 000150 the table's key was (region, lookback_days), so the writers +// overwrote one another and both accounts read whichever row landed last. +func TestRIUtilizationCache_Integration_AccountsInSameRegionKeepOwnRows(t *testing.T) { + ctx := context.Background() + store := setupRICacheIntegration(ctx, t) + + keyA := usEast30 + keyB := config.RIUtilizationCacheKey{CloudAccountID: riUtilAccountB, Region: "us-east-1", LookbackDays: 30} + unattributed := config.RIUtilizationCacheKey{Region: "us-east-1", LookbackDays: 30} + payloadOf := func(id string) []byte { + b, err := json.Marshal([]recommendations.RIUtilization{{ReservedInstanceID: id}}) + require.NoError(t, err) + return b + } + writers := map[config.RIUtilizationCacheKey][]byte{ + keyA: payloadOf("ri-account-a"), + keyB: payloadOf("ri-account-b"), + unattributed: payloadOf("ri-ambient"), + } + + // Several concurrent upserts per key, interleaved across keys, exercise + // the ON CONFLICT path on the new three-column key. + const perKey = 8 + start := make(chan struct{}) + var wg sync.WaitGroup + errs := make(chan error, perKey*len(writers)) + for key, payload := range writers { + for range perKey { + wg.Add(1) + go func() { + defer wg.Done() + <-start + errs <- store.UpsertRIUtilizationCache(ctx, key, payload, time.Now()) + }() + } + } + close(start) + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err) + } + + for key, payload := range writers { + entry, err := store.GetRIUtilizationCache(ctx, key) + require.NoError(t, err) + require.NotNil(t, entry, "no row for %+v", key) + require.Equal(t, key.CloudAccountID, entry.CloudAccountID) + require.JSONEq(t, string(payload), string(entry.Payload), "row for %+v holds another account's payload", key) + } + + missing, err := store.GetRIUtilizationCache(ctx, config.RIUtilizationCacheKey{ + CloudAccountID: "12000000-0000-4000-8000-00000000000c", Region: "us-east-1", LookbackDays: 30, + }) + require.NoError(t, err) + require.Nil(t, missing, "an account with no row of its own must miss, not read another account's row") +} diff --git a/internal/api/ri_utilization_cache_test.go b/internal/api/ri_utilization_cache_test.go index 78084a12d..d5358e181 100644 --- a/internal/api/ri_utilization_cache_test.go +++ b/internal/api/ri_utilization_cache_test.go @@ -16,45 +16,51 @@ import ( "github.com/stretchr/testify/require" ) +// Cloud account UUIDs for the RI utilization cache tests (issue #120). +const ( + riUtilAccountA = "12000000-0000-4000-8000-00000000000a" + riUtilAccountB = "12000000-0000-4000-8000-00000000000b" +) + +// usEast30 is the cache key most tests read and write. +var usEast30 = config.RIUtilizationCacheKey{CloudAccountID: riUtilAccountA, Region: "us-east-1", LookbackDays: 30} + // fakeRIUtilCacheStore is a minimal in-test implementation of -// riUtilizationCacheStore. Keyed by (region, lookbackDays); stores the -// raw JSON payload + fetched_at so the cache layer exercises the same -// marshaling path as the real Postgres store. +// riUtilizationCacheStore. Keyed by the full (account, region, lookback) +// key like the real table; stores the raw JSON payload + fetched_at so the +// cache layer exercises the same marshaling path as the real Postgres store. type fakeRIUtilCacheStore struct { getErr error - entries map[string]config.RIUtilizationCacheEntry + entries map[config.RIUtilizationCacheKey]config.RIUtilizationCacheEntry mu sync.Mutex } func newFakeRIUtilCacheStore() *fakeRIUtilCacheStore { - return &fakeRIUtilCacheStore{entries: map[string]config.RIUtilizationCacheEntry{}} -} - -func (f *fakeRIUtilCacheStore) key(region string, lookbackDays int) string { - return region + "#" + time.Duration(lookbackDays).String() + return &fakeRIUtilCacheStore{entries: map[config.RIUtilizationCacheKey]config.RIUtilizationCacheEntry{}} } -func (f *fakeRIUtilCacheStore) GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*config.RIUtilizationCacheEntry, error) { +func (f *fakeRIUtilCacheStore) GetRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey) (*config.RIUtilizationCacheEntry, error) { f.mu.Lock() defer f.mu.Unlock() if f.getErr != nil { return nil, f.getErr } - e, ok := f.entries[f.key(region, lookbackDays)] + e, ok := f.entries[key] if !ok { return nil, nil } return &e, nil } -func (f *fakeRIUtilCacheStore) UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error { +func (f *fakeRIUtilCacheStore) UpsertRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey, payload []byte, fetchedAt time.Time) error { f.mu.Lock() defer f.mu.Unlock() - f.entries[f.key(region, lookbackDays)] = config.RIUtilizationCacheEntry{ - Region: region, - LookbackDays: lookbackDays, - Payload: payload, - FetchedAt: fetchedAt, + f.entries[key] = config.RIUtilizationCacheEntry{ + CloudAccountID: key.CloudAccountID, + Region: key.Region, + LookbackDays: key.LookbackDays, + Payload: payload, + FetchedAt: fetchedAt, } return nil } @@ -62,28 +68,19 @@ func (f *fakeRIUtilCacheStore) UpsertRIUtilizationCache(ctx context.Context, reg // seedStale writes a cache row with fetchedAt set in the past so the // caller can control exactly how "stale" the row is relative to soft // / hard TTLs. -// -//nolint:unparam // test helper: region fixed by design across current callers -func (f *fakeRIUtilCacheStore) seedStale(t *testing.T, region string, lookbackDays int, data []recommendations.RIUtilization, age time.Duration) { +func (f *fakeRIUtilCacheStore) seedStale(t *testing.T, key config.RIUtilizationCacheKey, data []recommendations.RIUtilization, age time.Duration) { t.Helper() payload, err := json.Marshal(data) if err != nil { t.Fatalf("seedStale marshal: %v", err) } - f.mu.Lock() - defer f.mu.Unlock() - f.entries[f.key(region, lookbackDays)] = config.RIUtilizationCacheEntry{ - Region: region, - LookbackDays: lookbackDays, - Payload: payload, - FetchedAt: time.Now().Add(-age), - } + require.NoError(t, f.UpsertRIUtilizationCache(context.Background(), key, payload, time.Now().Add(-age))) } -func (f *fakeRIUtilCacheStore) latest(region string, lookbackDays int) ([]recommendations.RIUtilization, bool) { +func (f *fakeRIUtilCacheStore) latest(key config.RIUtilizationCacheKey) ([]recommendations.RIUtilization, bool) { f.mu.Lock() defer f.mu.Unlock() - e, ok := f.entries[f.key(region, lookbackDays)] + e, ok := f.entries[key] if !ok { return nil, false } @@ -103,7 +100,7 @@ func TestRIUtilizationCache_HitReturnsCachedData(t *testing.T) { return fetched, nil } - got, err := c.getOrFetch(context.Background(), "us-east-1", 30, 10*time.Second, 30*time.Second, fetch) + got, err := c.getOrFetch(context.Background(), usEast30, 10*time.Second, 30*time.Second, fetch) if err != nil { t.Fatalf("first call: %v", err) } @@ -115,7 +112,7 @@ func TestRIUtilizationCache_HitReturnsCachedData(t *testing.T) { } // Second call within soft TTL: cache hit, fetcher NOT invoked. - _, err = c.getOrFetch(context.Background(), "us-east-1", 30, 10*time.Second, 30*time.Second, fetch) + _, err = c.getOrFetch(context.Background(), usEast30, 10*time.Second, 30*time.Second, fetch) if err != nil { t.Fatalf("second call: %v", err) } @@ -133,7 +130,7 @@ func TestRIUtilizationCache_StaleOnNonLambdaServesStaleAndKicksRefresh(t *testin staleData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-stale"}} freshData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-fresh"}} // Seed a row whose age is between the soft TTL (50ms) and hard TTL (1h). - store.seedStale(t, "us-east-1", 30, staleData, 100*time.Millisecond) + store.seedStale(t, usEast30, staleData, 100*time.Millisecond) c := newRIUtilizationCache(store, false) // non-Lambda @@ -150,7 +147,7 @@ func TestRIUtilizationCache_StaleOnNonLambdaServesStaleAndKicksRefresh(t *testin } got := getOrFetchWithin(t, 10*time.Second, func() ([]recommendations.RIUtilization, error) { - return c.getOrFetch(context.Background(), "us-east-1", 30, 50*time.Millisecond, time.Hour, fetch) + return c.getOrFetch(context.Background(), usEast30, 50*time.Millisecond, time.Hour, fetch) }, "getOrFetch waited for the fetcher; SWR must return the stale copy synchronously") if len(got) != 1 || got[0].ReservedInstanceID != "ri-stale" { t.Fatalf("expected stale copy, got %+v", got) @@ -164,7 +161,7 @@ func TestRIUtilizationCache_StaleOnNonLambdaServesStaleAndKicksRefresh(t *testin releaseFetch() require.Eventually(t, func() bool { - latest, ok := store.latest("us-east-1", 30) + latest, ok := store.latest(usEast30) return ok && len(latest) == 1 && latest[0].ReservedInstanceID == "ri-fresh" }, 10*time.Second, time.Millisecond, "background refresh did not update the stored row") } @@ -197,7 +194,7 @@ func TestRIUtilizationCache_StaleOnLambdaBlocksForSyncRefetch(t *testing.T) { store := newFakeRIUtilCacheStore() staleData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-stale"}} freshData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-fresh"}} - store.seedStale(t, "us-east-1", 30, staleData, 100*time.Millisecond) + store.seedStale(t, usEast30, staleData, 100*time.Millisecond) c := newRIUtilizationCache(store, true) // Lambda mode @@ -205,7 +202,7 @@ func TestRIUtilizationCache_StaleOnLambdaBlocksForSyncRefetch(t *testing.T) { return freshData, nil } - got, err := c.getOrFetch(context.Background(), "us-east-1", 30, 50*time.Millisecond, time.Hour, fetch) + got, err := c.getOrFetch(context.Background(), usEast30, 50*time.Millisecond, time.Hour, fetch) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -220,7 +217,7 @@ func TestRIUtilizationCache_HardExpirySyncRefetch(t *testing.T) { staleData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-stale"}} freshData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-fresh"}} // Age (2h) exceeds both soft (15m) and hard (1h). - store.seedStale(t, "us-east-1", 30, staleData, 2*time.Hour) + store.seedStale(t, usEast30, staleData, 2*time.Hour) c := newRIUtilizationCache(store, false) // non-Lambda, but hard-expired @@ -228,7 +225,7 @@ func TestRIUtilizationCache_HardExpirySyncRefetch(t *testing.T) { return freshData, nil } - got, err := c.getOrFetch(context.Background(), "us-east-1", 30, 15*time.Minute, time.Hour, fetch) + got, err := c.getOrFetch(context.Background(), usEast30, 15*time.Minute, time.Hour, fetch) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -242,7 +239,7 @@ func TestRIUtilizationCache_SingleflightCollapsesConcurrentRefreshes(t *testing. store := newFakeRIUtilCacheStore() staleData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-stale"}} freshData := []recommendations.RIUtilization{{ReservedInstanceID: "ri-fresh"}} - store.seedStale(t, "us-east-1", 30, staleData, 100*time.Millisecond) + store.seedStale(t, usEast30, staleData, 100*time.Millisecond) c := newRIUtilizationCache(store, false) @@ -268,7 +265,7 @@ func TestRIUtilizationCache_SingleflightCollapsesConcurrentRefreshes(t *testing. wg.Add(1) go func() { defer wg.Done() - _, err := c.getOrFetch(context.Background(), "us-east-1", 30, 50*time.Millisecond, time.Hour, fetch) + _, err := c.getOrFetch(context.Background(), usEast30, 50*time.Millisecond, time.Hour, fetch) if err != nil { t.Errorf("getOrFetch: %v", err) } @@ -295,7 +292,7 @@ func TestRIUtilizationCache_SingleflightCollapsesConcurrentRefreshes(t *testing. // store the fresh row and every refresh goroutine to return. releaseFetch() require.Eventually(t, func() bool { - latest, ok := store.latest("us-east-1", 30) + latest, ok := store.latest(usEast30) return ok && len(latest) == 1 && latest[0].ReservedInstanceID == "ri-fresh" && len(mark.Started("riUtilizationCache).kickBackgroundRefresh")) == 0 }, 10*time.Second, time.Millisecond, "the collapsed refresh did not store the fresh row and return") @@ -321,13 +318,13 @@ func TestRIUtilizationCache_FetchErrorNotCached(t *testing.T) { return nil, sentinel } - _, err := c.getOrFetch(context.Background(), "us-east-1", 30, time.Hour, time.Hour, fetch) + _, err := c.getOrFetch(context.Background(), usEast30, time.Hour, time.Hour, fetch) if !errors.Is(err, sentinel) { t.Fatalf("expected sentinel error; got %v", err) } // Retry should hit the fetcher again — we don't want to cache // failures and lock the dashboard out for the full TTL window. - _, err = c.getOrFetch(context.Background(), "us-east-1", 30, time.Hour, time.Hour, fetch) + _, err = c.getOrFetch(context.Background(), usEast30, time.Hour, time.Hour, fetch) if !errors.Is(err, sentinel) { t.Fatalf("second call: expected sentinel error; got %v", err) } @@ -341,12 +338,7 @@ func TestRIUtilizationCache_FetchErrorNotCached(t *testing.T) { func TestRIUtilizationCache_CorruptPayloadFallsThrough(t *testing.T) { store := newFakeRIUtilCacheStore() - store.entries[store.key("us-east-1", 30)] = config.RIUtilizationCacheEntry{ - Region: "us-east-1", - LookbackDays: 30, - Payload: []byte("not-json"), - FetchedAt: time.Now(), - } + require.NoError(t, store.UpsertRIUtilizationCache(context.Background(), usEast30, []byte("not-json"), time.Now())) c := newRIUtilizationCache(store, true) fetched := []recommendations.RIUtilization{{ReservedInstanceID: "ri-fresh"}} var calls atomic.Int32 @@ -355,7 +347,7 @@ func TestRIUtilizationCache_CorruptPayloadFallsThrough(t *testing.T) { return fetched, nil } - got, err := c.getOrFetch(context.Background(), "us-east-1", 30, time.Hour, time.Hour, fetch) + got, err := c.getOrFetch(context.Background(), usEast30, time.Hour, time.Hour, fetch) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -366,7 +358,7 @@ func TestRIUtilizationCache_CorruptPayloadFallsThrough(t *testing.T) { t.Fatalf("got wrong data back: %+v", got) } // Corrupt row should have been overwritten. - entry := store.entries[store.key("us-east-1", 30)] + entry := store.entries[usEast30] var round []recommendations.RIUtilization if err := json.Unmarshal(entry.Payload, &round); err != nil { t.Fatalf("stored payload still corrupt after upsert: %v", err) @@ -396,3 +388,103 @@ func TestHandler_getRIUtilizationCache_FollowsHandlerLambdaMode(t *testing.T) { }) } } + +// TestRIUtilizationCache_AccountsInSameRegionDoNotShareRow pins issue #120: +// the cache key carries the account, so account B's read of the same region +// and lookback is a miss that fetches B's data instead of a hit on A's row. +func TestRIUtilizationCache_AccountsInSameRegionDoNotShareRow(t *testing.T) { + store := newFakeRIUtilCacheStore() + c := newRIUtilizationCache(store, false) + keyA := usEast30 + keyB := config.RIUtilizationCacheKey{CloudAccountID: riUtilAccountB, Region: "us-east-1", LookbackDays: 30} + fetchOf := func(id string, calls *atomic.Int32) riUtilizationFetcher { + return func(context.Context, int) ([]recommendations.RIUtilization, error) { + calls.Add(1) + return []recommendations.RIUtilization{{ReservedInstanceID: id}}, nil + } + } + var callsA, callsB atomic.Int32 + + gotA, err := c.getOrFetch(context.Background(), keyA, time.Hour, time.Hour, fetchOf("ri-a", &callsA)) + require.NoError(t, err) + gotB, err := c.getOrFetch(context.Background(), keyB, time.Hour, time.Hour, fetchOf("ri-b", &callsB)) + require.NoError(t, err) + gotA2, err := c.getOrFetch(context.Background(), keyA, time.Hour, time.Hour, fetchOf("ri-a", &callsA)) + require.NoError(t, err) + + require.Equal(t, "ri-a", gotA[0].ReservedInstanceID) + require.Equal(t, "ri-b", gotB[0].ReservedInstanceID, "account B was served account A's cached row") + require.Equal(t, "ri-a", gotA2[0].ReservedInstanceID) + require.Equal(t, int32(1), callsA.Load(), "account A's second read is a cache hit") + require.Equal(t, int32(1), callsB.Load(), "account B's first read is a miss") + require.Len(t, store.entries, 2) +} + +// TestRIUtilizationCache_SingleflightKeyedByAccount pins issue #120 for the +// background refresh: two accounts' stale rows for the same region and +// lookback refresh concurrently, each with its own fetcher. With the old +// region#lookback singleflight key, account B's refresh joined account A's +// in-flight call, so B's fetcher never ran and B's row stayed stale. +func TestRIUtilizationCache_SingleflightKeyedByAccount(t *testing.T) { + store := newFakeRIUtilCacheStore() + keyA := usEast30 + keyB := config.RIUtilizationCacheKey{CloudAccountID: riUtilAccountB, Region: "us-east-1", LookbackDays: 30} + store.seedStale(t, keyA, []recommendations.RIUtilization{{ReservedInstanceID: "ri-a-stale"}}, time.Minute) + store.seedStale(t, keyB, []recommendations.RIUtilization{{ReservedInstanceID: "ri-b-stale"}}, time.Minute) + c := newRIUtilizationCache(store, false) + + release := make(chan struct{}) + var releaseOnce sync.Once + releaseFetch := func() { releaseOnce.Do(func() { close(release) }) } + t.Cleanup(releaseFetch) + var callsA, callsB atomic.Int32 + blockingFetch := func(id string, calls *atomic.Int32) riUtilizationFetcher { + return func(context.Context, int) ([]recommendations.RIUtilization, error) { + calls.Add(1) + <-release + return []recommendations.RIUtilization{{ReservedInstanceID: id}}, nil + } + } + + mark := goroutines.NewMark() + gotA := getOrFetchWithin(t, 5*time.Second, func() ([]recommendations.RIUtilization, error) { + return c.getOrFetch(context.Background(), keyA, time.Second, time.Hour, blockingFetch("ri-a-fresh", &callsA)) + }, "account A's stale read waited for its refresh") + require.Equal(t, "ri-a-stale", gotA[0].ReservedInstanceID) + require.Eventually(t, func() bool { return callsA.Load() == 1 }, 10*time.Second, time.Millisecond, + "account A's background refresh never started") + + // A's refresh is now in flight and blocked. B's refresh must start its + // own fetch rather than wait on A's. + gotB := getOrFetchWithin(t, 5*time.Second, func() ([]recommendations.RIUtilization, error) { + return c.getOrFetch(context.Background(), keyB, time.Second, time.Hour, blockingFetch("ri-b-fresh", &callsB)) + }, "account B's stale read waited for a refresh") + require.Equal(t, "ri-b-stale", gotB[0].ReservedInstanceID) + require.Eventually(t, func() bool { + return callsB.Load() == 1 || len(sfDuplicatesWaiting(mark)) > 0 + }, 10*time.Second, time.Millisecond, "account B's background refresh never reached singleflight") + require.Equal(t, int32(1), callsB.Load(), "account B's refresh joined account A's in-flight call") + + releaseFetch() + require.Eventually(t, func() bool { + a, okA := store.latest(keyA) + b, okB := store.latest(keyB) + return okA && okB && a[0].ReservedInstanceID == "ri-a-fresh" && b[0].ReservedInstanceID == "ri-b-fresh" && + len(mark.Started("riUtilizationCache).kickBackgroundRefresh")) == 0 + }, 10*time.Second, time.Millisecond, "each account's row was not refreshed with its own data") +} + +func TestRIUtilizationFlightKey_IncludesEveryKeyField(t *testing.T) { + base := usEast30 + otherAccount, otherRegion, otherLookback := base, base, base + otherAccount.CloudAccountID = riUtilAccountB + otherRegion.Region = "us-west-2" + otherLookback.LookbackDays = 7 + unattributed := base + unattributed.CloudAccountID = "" + keys := map[string]bool{} + for _, k := range []config.RIUtilizationCacheKey{base, otherAccount, otherRegion, otherLookback, unattributed} { + keys[riUtilizationFlightKey(k)] = true + } + require.Len(t, keys, 5) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index b1ddea0fe..40c9e660e 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -712,10 +712,10 @@ type StoreInterface interface { // dashboard loads don't each fan out to a paid CE API call. A // per-process in-memory cache effectively never hits because each // cold container starts empty. GetRIUtilizationCache returns nil - // when the (region, lookback_days) key is absent — callers treat - // that as a miss and re-fetch. - GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*RIUtilizationCacheEntry, error) - UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error + // when the (cloud_account_id, region, lookback_days) key is absent — + // callers treat that as a miss and re-fetch. + GetRIUtilizationCache(ctx context.Context, key RIUtilizationCacheKey) (*RIUtilizationCacheEntry, error) + UpsertRIUtilizationCache(ctx context.Context, key RIUtilizationCacheKey, payload []byte, fetchedAt time.Time) error // Account registrations (self-service enrollment via federation IaC) CreateAccountRegistration(ctx context.Context, reg *AccountRegistration) error diff --git a/internal/config/store_postgres_ri_utilization.go b/internal/config/store_postgres_ri_utilization.go index ec89905c4..8b1835098 100644 --- a/internal/config/store_postgres_ri_utilization.go +++ b/internal/config/store_postgres_ri_utilization.go @@ -10,16 +10,17 @@ import ( ) // GetRIUtilizationCache returns the cached Cost Explorer utilization -// result for (region, lookback_days) or nil if no row exists. Staleness -// is evaluated by the caller — the query doesn't filter on fetched_at -// so a caller with a longer TTL than expected can still use the row. -func (s *PostgresStore) GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*RIUtilizationCacheEntry, error) { +// result for (cloud_account_id, region, lookback_days) or nil if no row +// exists. Staleness is evaluated by the caller — the query doesn't filter +// on fetched_at so a caller with a longer TTL than expected can still use +// the row. +func (s *PostgresStore) GetRIUtilizationCache(ctx context.Context, key RIUtilizationCacheKey) (*RIUtilizationCacheEntry, error) { var entry RIUtilizationCacheEntry err := s.db.QueryRow(ctx, ` - SELECT region, lookback_days, payload, fetched_at + SELECT cloud_account_id, region, lookback_days, payload, fetched_at FROM ri_utilization_cache - WHERE region = $1 AND lookback_days = $2 - `, region, lookbackDays).Scan(&entry.Region, &entry.LookbackDays, &entry.Payload, &entry.FetchedAt) + WHERE cloud_account_id = $1 AND region = $2 AND lookback_days = $3 + `, key.CloudAccountID, key.Region, key.LookbackDays).Scan(&entry.CloudAccountID, &entry.Region, &entry.LookbackDays, &entry.Payload, &entry.FetchedAt) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return nil, nil @@ -30,18 +31,18 @@ func (s *PostgresStore) GetRIUtilizationCache(ctx context.Context, region string } // UpsertRIUtilizationCache writes or overwrites the cached utilization -// payload for (region, lookback_days). fetchedAt should be "now" at the -// time of the underlying Cost Explorer call — readers compare this -// against their TTL to decide freshness. -func (s *PostgresStore) UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error { +// payload for (cloud_account_id, region, lookback_days). fetchedAt should +// be "now" at the time of the underlying Cost Explorer call — readers +// compare this against their TTL to decide freshness. +func (s *PostgresStore) UpsertRIUtilizationCache(ctx context.Context, key RIUtilizationCacheKey, payload []byte, fetchedAt time.Time) error { if _, err := s.db.Exec(ctx, ` - INSERT INTO ri_utilization_cache (region, lookback_days, payload, fetched_at) - VALUES ($1, $2, $3, $4) - ON CONFLICT (region, lookback_days) + INSERT INTO ri_utilization_cache (cloud_account_id, region, lookback_days, payload, fetched_at) + VALUES ($1, $2, $3, $4, $5) + ON CONFLICT (cloud_account_id, region, lookback_days) DO UPDATE SET payload = EXCLUDED.payload, fetched_at = EXCLUDED.fetched_at - `, region, lookbackDays, payload, fetchedAt); err != nil { + `, key.CloudAccountID, key.Region, key.LookbackDays, payload, fetchedAt); err != nil { return fmt.Errorf("failed to upsert ri_utilization_cache: %w", err) } return nil diff --git a/internal/config/types.go b/internal/config/types.go index 7041f89be..cbcc4efd3 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -981,17 +981,30 @@ type SuccessfulCollect struct { CloudAccountID *string } +// RIUtilizationCacheKey identifies one cached Cost Explorer +// GetReservationUtilization result. CloudAccountID is the cloud_accounts +// UUID of the account whose credentials made the call, or "" when the +// deployment's ambient identity is not registered in cloud_accounts. It is +// part of the key so two accounts reading the same region never share a +// row (issue #120). +type RIUtilizationCacheKey struct { + CloudAccountID string + Region string + LookbackDays int +} + // RIUtilizationCacheEntry is a single cached Cost Explorer -// GetReservationUtilization result keyed by (region, lookback_days). +// GetReservationUtilization result keyed by RIUtilizationCacheKey. // Payload is the JSON encoding of the caller's utilization slice — kept // opaque here so the config package stays free of AWS-provider types. // TTL freshness is evaluated in the caller (api-layer cache wrapper) // based on FetchedAt vs. a caller-supplied TTL. type RIUtilizationCacheEntry struct { - Region string - LookbackDays int - Payload []byte - FetchedAt time.Time + CloudAccountID string + Region string + LookbackDays int + Payload []byte + FetchedAt time.Time } // PurchaseHistoryFilter is the filter set consumed by diff --git a/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.down.sql b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.down.sql new file mode 100644 index 000000000..7a28cb06a --- /dev/null +++ b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.down.sql @@ -0,0 +1,11 @@ +-- Reverse 000150 (issue #120). Rows from different accounts can share +-- (region, lookback_days), which the restored key cannot hold, so the cache is +-- emptied first; the next read refetches from Cost Explorer. +LOCK TABLE ri_utilization_cache IN ACCESS EXCLUSIVE MODE; + +DELETE FROM ri_utilization_cache; + +ALTER TABLE ri_utilization_cache DROP CONSTRAINT IF EXISTS ri_utilization_cache_pkey; +ALTER TABLE ri_utilization_cache DROP COLUMN IF EXISTS cloud_account_id; +ALTER TABLE ri_utilization_cache + ADD CONSTRAINT ri_utilization_cache_pkey PRIMARY KEY (region, lookback_days); diff --git a/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.up.sql b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.up.sql new file mode 100644 index 000000000..4c71db764 --- /dev/null +++ b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key.up.sql @@ -0,0 +1,29 @@ +-- Add the account identity to the RI utilization cache key (issue #120). +-- +-- The cached payload is one AWS account's Cost Explorer +-- GetReservationUtilization result, but the key was only +-- (region, lookback_days). Once the RI exchange / reshape path reads with +-- per-account credentials, account A's row would be served to account B for +-- the same region and lookback within the TTL. cloud_account_id is the +-- cloud_accounts UUID of the account whose credentials made the call, or '' +-- when the deployment's ambient identity is not registered in cloud_accounts +-- (a primary key column cannot be NULL). It is TEXT with no foreign key: this +-- is a cache, and a row left behind by a deleted account is never read again. +-- +-- Existing rows carry no account identity, so they are deleted rather than +-- guessed at; the next read refetches from Cost Explorer. The table is locked +-- first so a writer still running the previous release cannot insert a row +-- between the DELETE and the NOT NULL column add (which would fail the +-- migration). Such a writer's later upserts fail with "no unique or exclusion +-- constraint matching the ON CONFLICT specification"; the cache logs and +-- ignores upsert errors, so it only loses that cache write. +LOCK TABLE ri_utilization_cache IN ACCESS EXCLUSIVE MODE; + +DELETE FROM ri_utilization_cache; + +ALTER TABLE ri_utilization_cache + ADD COLUMN IF NOT EXISTS cloud_account_id TEXT NOT NULL; + +ALTER TABLE ri_utilization_cache DROP CONSTRAINT IF EXISTS ri_utilization_cache_pkey; +ALTER TABLE ri_utilization_cache + ADD CONSTRAINT ri_utilization_cache_pkey PRIMARY KEY (cloud_account_id, region, lookback_days); diff --git a/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key_test.go b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key_test.go new file mode 100644 index 000000000..0dd71b40c --- /dev/null +++ b/internal/database/postgres/migrations/000150_ri_utilization_cache_account_key_test.go @@ -0,0 +1,99 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const ( + m150AccountA = "15000000-0000-4000-8000-00000000000a" + m150AccountB = "15000000-0000-4000-8000-00000000000b" +) + +// m150PrimaryKey returns ri_utilization_cache's primary key columns in key +// order. +func m150PrimaryKey(ctx context.Context, t *testing.T, pool *pgxpool.Pool) []string { + t.Helper() + rows, err := pool.Query(ctx, ` + SELECT a.attname + FROM pg_index i + JOIN LATERAL unnest(i.indkey) WITH ORDINALITY AS k(attnum, ord) ON true + JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = k.attnum + WHERE i.indrelid = 'ri_utilization_cache'::regclass AND i.indisprimary + ORDER BY k.ord`) + require.NoError(t, err) + defer rows.Close() + var cols []string + for rows.Next() { + var c string + require.NoError(t, rows.Scan(&c)) + cols = append(cols, c) + } + require.NoError(t, rows.Err()) + return cols +} + +func m150Count(ctx context.Context, t *testing.T, pool *pgxpool.Pool) int { + t.Helper() + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM ri_utilization_cache`).Scan(&n)) + return n +} + +// TestMigration_RIUtilizationCacheAccountKey covers 000150 (issue #120): the +// cache key gains cloud_account_id, rows written without an account identity +// are dropped, two accounts can hold rows for the same region and lookback, +// a write must name its account, and the down migration restores the +// (region, lookback_days) key. +func TestMigration_RIUtilizationCacheAccountKey(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + + const version = 150 + prev := previousMigrationVersion(t, version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + require.Equal(t, []string{"region", "lookback_days"}, m150PrimaryKey(ctx, t, pool)) + + // A pre-000150 row carries no account identity. + _, err := pool.Exec(ctx, ` + INSERT INTO ri_utilization_cache (region, lookback_days, payload, fetched_at) + VALUES ('us-east-1', 30, '[{"ReservedInstanceID":"ri-legacy"}]', NOW())`) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + assert.Equal(t, []string{"cloud_account_id", "region", "lookback_days"}, m150PrimaryKey(ctx, t, pool)) + assert.Zero(t, m150Count(ctx, t, pool), "rows without an account identity are dropped, not attributed to one") + + insert := func(account string) error { + _, insertErr := pool.Exec(ctx, ` + INSERT INTO ri_utilization_cache (cloud_account_id, region, lookback_days, payload, fetched_at) + VALUES ($1, 'us-east-1', 30, '[]', NOW())`, account) + return insertErr + } + require.NoError(t, insert(m150AccountA)) + require.NoError(t, insert(m150AccountB), "a second account can cache the same region and lookback") + require.NoError(t, insert(""), "the unregistered ambient identity has its own key") + assert.Error(t, insert(m150AccountA), "one account still has one row per region and lookback") + + _, err = pool.Exec(ctx, ` + INSERT INTO ri_utilization_cache (region, lookback_days, payload, fetched_at) + VALUES ('us-west-2', 30, '[]', NOW())`) + assert.Error(t, err, "a write that does not name its account is refused") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + assert.Equal(t, []string{"region", "lookback_days"}, m150PrimaryKey(ctx, t, pool), "down restores the old key") + assert.Zero(t, m150Count(ctx, t, pool), "down empties the cache so per-account rows cannot collide on the old key") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + assert.Equal(t, []string{"cloud_account_id", "region", "lookback_days"}, m150PrimaryKey(ctx, t, pool), "re-up") +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index ea6706bb0..c38eb2de3 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1852,12 +1852,12 @@ func (m *MockConfigStore) SetRecommendationsCollectionPartial(ctx context.Contex return args.Error(0) } -func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, region string, lookbackDays int) (*config.RIUtilizationCacheEntry, error) { - m.record("GetRIUtilizationCache", ctx, region, lookbackDays) +func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey) (*config.RIUtilizationCacheEntry, error) { + m.record("GetRIUtilizationCache", ctx, key) if !isExpected(&m.Mock, "GetRIUtilizationCache") { return nil, nil } - args := m.Called(ctx, region, lookbackDays) + args := m.Called(ctx, key) if args.Get(0) == nil { return nil, args.Error(1) } @@ -1868,12 +1868,12 @@ func (m *MockConfigStore) GetRIUtilizationCache(ctx context.Context, region stri return v, args.Error(1) } -func (m *MockConfigStore) UpsertRIUtilizationCache(ctx context.Context, region string, lookbackDays int, payload []byte, fetchedAt time.Time) error { - m.record("UpsertRIUtilizationCache", ctx, region, lookbackDays, payload, fetchedAt) +func (m *MockConfigStore) UpsertRIUtilizationCache(ctx context.Context, key config.RIUtilizationCacheKey, payload []byte, fetchedAt time.Time) error { + m.record("UpsertRIUtilizationCache", ctx, key, payload, fetchedAt) if !isExpected(&m.Mock, "UpsertRIUtilizationCache") { return nil } - args := m.Called(ctx, region, lookbackDays, payload, fetchedAt) + args := m.Called(ctx, key, payload, fetchedAt) return args.Error(0) } From 68604c1a2c073b14fc1db920e3d3c4e008995f2a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 3/3] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA=