diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 86e891cd2..754bbb9b7 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -539,10 +539,13 @@ describe('Settings Module', () => { notification_days_before: 3, // Grace-period inputs default to 7 per provider when the DOM // doesn't include the new inputs (older test harness setup). - // The save helper reads missing elements as "empty" → default 7. + // The save helper reads missing elements as "empty" -> default 7. grace_period_days: { aws: 7, azure: 7, gcp: 7 }, recommendations_cache_stale_hours: 24, recommendations_lookback_days: 7, + // offering_class select is absent in this test harness (no DOM element); + // saveGlobalSettings falls back to 'convertible'. + offering_class: 'convertible', }); }); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index f614433f8..7a6823c3a 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -289,6 +289,10 @@ export interface Config { // Keys: 'aws' / 'azure' / 'gcp'. A missing key defaults to 7 on // the backend. Explicit 0 disables the feature for that provider. grace_period_days?: Record; + // EC2 Reserved Instance offering class. "convertible" (default) allows + // future exchanges for different families/sizes/OS; "standard" is + // ~5% cheaper but locked to the exact instance type for the full term. + offering_class?: 'convertible' | 'standard'; ri_exchange_enabled?: boolean; ri_exchange_mode?: string; ri_exchange_utilization_threshold?: number; diff --git a/frontend/src/index.html b/frontend/src/index.html index aab91e8e6..d4c026f49 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -497,6 +497,22 @@

Purchasing Settings

+
+ AWS Reserved Instance Class +

Controls whether EC2 Reserved Instances are purchased as Convertible or Standard. Convertible RIs can be exchanged for a different instance family, size, OS, or region later. Standard RIs are approximately 5% cheaper but are locked to the exact instance type for the full term and cannot be exchanged.

+
+
+ +
+
+ +
+
+
+
AWS Service Defaults diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index cc64614f0..ab6498048 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3202,6 +3202,16 @@ export async function loadGlobalSettings(): Promise { populateGraceInput('setting-grace-azure', gpMap['azure']); populateGraceInput('setting-grace-gcp', gpMap['gcp']); + // EC2 RI offering class. Absent/null from the API means "convertible" + // (backend default). Only "convertible" and "standard" are valid; any + // other value from a future API version falls back to "convertible" so + // the dropdown always has a valid selection. + const offeringClassSelect = byId('setting-ec2-offering-class'); + if (offeringClassSelect) { + const oc = data.global.offering_class; + offeringClassSelect.value = (oc === 'standard') ? 'standard' : 'convertible'; + } + // Recommendations cycle params const staleHoursInput = byId('setting-recs-stale-hours'); if (staleHoursInput) { @@ -3476,6 +3486,9 @@ export async function saveGlobalSettings(e: Event): Promise { return; } + const rawOfferingClass = byId('setting-ec2-offering-class')?.value ?? 'convertible'; + const offeringClass: 'convertible' | 'standard' = (rawOfferingClass === 'standard') ? 'standard' : 'convertible'; + const settings: api.Config = { enabled_providers: enabledProviders, notification_email: byId('setting-notification-email')?.value || '', @@ -3488,6 +3501,7 @@ export async function saveGlobalSettings(e: Event): Promise { grace_period_days: gracePeriodDays, recommendations_cache_stale_hours: rawStaleHours, recommendations_lookback_days: parseInt(byId('setting-recs-lookback-days')?.value || '7', 10), + offering_class: offeringClass, }; // Include laddering_enabled in the payload when the Purchasing panel's @@ -3625,6 +3639,9 @@ export async function resetSettings(): Promise { populateGraceInput('setting-grace-azure', 7); populateGraceInput('setting-grace-gcp', 7); + const offeringClassSelect = byId('setting-ec2-offering-class'); + if (offeringClassSelect) offeringClassSelect.value = 'convertible'; + const staleHoursInput = byId('setting-recs-stale-hours'); if (staleHoursInput) staleHoursInput.value = '24'; const lookbackSelect = byId('setting-recs-lookback-days'); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 4e29f7e4a..2b9e51dfb 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -373,6 +373,8 @@ export interface GlobalConfig { // suppression feature. Keys: 'aws' / 'azure' / 'gcp'. Missing keys // fall back to the backend default (7). Explicit 0 = disabled. grace_period_days?: Record; + // EC2 Reserved Instance offering class. "convertible" (default) or "standard". + offering_class?: 'convertible' | 'standard'; // Age (hours) after which the recommendations cache triggers a background // stale-while-revalidate refresh. 0 disables automatic background refresh. // Valid range: 0–8760. Default: 24. diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index cff7b69c2..3a6e0080e 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -81,7 +81,8 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo recommendations_cache_stale_hours, recommendations_lookback_days, COALESCE(purchase_delay_hours, 0), COALESCE(laddering_enabled, false), - COALESCE(ladder_execution_enabled, false) + COALESCE(ladder_execution_enabled, false), + offering_class FROM global_config WHERE id = 1 ` @@ -113,6 +114,7 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo &config.PurchaseDelayHours, &config.LadderingEnabled, &config.LadderExecutionEnabled, + &config.OfferingClass, ) if err != nil { @@ -135,6 +137,7 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo RecommendationsCacheStaleHours: DefaultRecommendationsCacheStaleHours, RecommendationsLookbackDays: DefaultRecommendationsLookbackDays, PurchaseDelayHours: DefaultPurchaseDelayHours, + OfferingClass: "convertible", }, nil } return nil, fmt.Errorf("failed to get global config: %w", err) @@ -211,8 +214,8 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - purchase_delay_hours, laddering_enabled, ladder_execution_enabled - ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22) + purchase_delay_hours, laddering_enabled, ladder_execution_enabled, offering_class + ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23) ON CONFLICT (id) DO UPDATE SET enabled_providers = $1, notification_email = $2, @@ -236,6 +239,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G purchase_delay_hours = $20, laddering_enabled = $21, ladder_execution_enabled = $22, + offering_class = $23, updated_at = NOW() ` @@ -259,7 +263,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G riExchangeUtilizationThreshold = 95.0 } - // Marshal GracePeriodDays → JSON text column. Empty map encodes as + // Marshal GracePeriodDays -> JSON text column. Empty map encodes as // "{}" so the DB column is never NULL and GetGlobalConfig can // treat "{}" and "" uniformly as "no explicit entries". gracePeriodJSON := "{}" @@ -271,6 +275,14 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G gracePeriodJSON = string(gpBytes) } + // Default offering_class to "convertible" when unset so the DB column + // never stores an empty string (the NOT NULL DEFAULT 'convertible' + // column handles inserts, but upserts overwrite with whatever we pass). + offeringClass := config.OfferingClass + if offeringClass == "" { + offeringClass = "convertible" + } + _, err := q.Exec(ctx, query, config.EnabledProviders, config.NotificationEmail, @@ -294,6 +306,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G config.PurchaseDelayHours, config.LadderingEnabled, config.LadderExecutionEnabled, + offeringClass, ) if err != nil { diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index f2c979c78..cf152869f 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -5,7 +5,9 @@ import ( "testing" "time" + "github.com/pashagolub/pgxmock/v4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // These tests exercise the real PostgresStore methods to gain code coverage. @@ -535,3 +537,75 @@ func TestPostgresStore_GetAllPurchaseHistory_NilDB(t *testing.T) { assert.True(t, panicked, "expected panic with nil db connection") } + +// TestSaveGlobalConfig_OfferingClassBindsAt23 is the HOLE 2 regression guard +// (issue #694): the real PostgresStore.SaveGlobalConfig must bind offering_class +// as the 23rd positional argument ($23), with laddering_enabled at $21 and +// ladder_execution_enabled at $22. +// The testablePostgresStore in store_postgres_mock_test.go is a hand-maintained +// copy that omits the field entirely, so no fast test guarded the placeholder +// count until now. +// +// Using pgxmock directly against the real PostgresStore (not the hand-maintained +// testablePostgresStore wrapper) ensures the live query is tested, not a stale copy. +func TestSaveGlobalConfig_OfferingClassBindsAt23(t *testing.T) { + ctx := context.Background() + mock, err := pgxmock.NewPool() + require.NoError(t, err) + defer mock.Close() + + // Wire the pgxmock pool directly into the real PostgresStore via the + // unexported db field (test is in package config so this is allowed). + store := &PostgresStore{db: mock} + + email := "ops@example.com" + cfg := &GlobalConfig{ + EnabledProviders: []string{"aws"}, + NotificationEmail: &email, + ApprovalRequired: true, + DefaultTerm: 12, + DefaultPayment: "all-upfront", + DefaultCoverage: 80.0, + DefaultRampSchedule: "immediate", + OfferingClass: "standard", + } + + // Expect exactly 23 args; pgxmock validates arg count and types. + // The 21st arg is laddering_enabled; the 22nd is ladder_execution_enabled; + // the 23rd arg must be "standard" (offering_class). + // If the real query regresses to a different arg count, pgxmock + // will return an unexpected-call error and the test will fail. + mock.ExpectExec(`INSERT INTO global_config`). + WithArgs( + pgxmock.AnyArg(), // $1 enabled_providers + pgxmock.AnyArg(), // $2 notification_email + pgxmock.AnyArg(), // $3 approval_required + pgxmock.AnyArg(), // $4 default_term + pgxmock.AnyArg(), // $5 default_payment + pgxmock.AnyArg(), // $6 default_coverage + pgxmock.AnyArg(), // $7 default_ramp_schedule + pgxmock.AnyArg(), // $8 ri_exchange_enabled + pgxmock.AnyArg(), // $9 ri_exchange_mode + pgxmock.AnyArg(), // $10 ri_exchange_utilization_threshold + pgxmock.AnyArg(), // $11 ri_exchange_max_per_exchange_usd + pgxmock.AnyArg(), // $12 ri_exchange_max_daily_usd + pgxmock.AnyArg(), // $13 ri_exchange_lookback_days + pgxmock.AnyArg(), // $14 auto_collect + pgxmock.AnyArg(), // $15 collection_schedule + pgxmock.AnyArg(), // $16 notification_days_before + pgxmock.AnyArg(), // $17 grace_period_days + pgxmock.AnyArg(), // $18 recommendations_cache_stale_hours + pgxmock.AnyArg(), // $19 recommendations_lookback_days + pgxmock.AnyArg(), // $20 purchase_delay_hours + pgxmock.AnyArg(), // $21 laddering_enabled + pgxmock.AnyArg(), // $22 ladder_execution_enabled + "standard", // $23 offering_class -- the field this test guards + ). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + + err = store.SaveGlobalConfig(ctx, cfg) + require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 23 args") + + require.NoError(t, mock.ExpectationsWereMet(), + "offering_class must be bound as the 23rd argument to SaveGlobalConfig") +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index e29285cd4..1135c926f 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -61,6 +61,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } rows := pgxmock.NewRows(cols).AddRow( []string{"aws"}, strPtr("ops@example.com"), true, @@ -73,6 +74,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { 0, false, false, + "convertible", ) mock.ExpectQuery("SELECT").WillReturnRows(rows) @@ -83,6 +85,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { assert.Equal(t, "ops@example.com", *cfg.NotificationEmail) assert.Equal(t, 24, cfg.RecommendationsCacheStaleHours) assert.Equal(t, 7, cfg.RecommendationsLookbackDays) + assert.Equal(t, "convertible", cfg.OfferingClass) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -115,6 +118,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } baseRow := func(graceJSON string) []any { return []any{ @@ -128,6 +132,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { 0, false, false, + "convertible", } } @@ -186,6 +191,7 @@ var globalConfigCols = []string{ "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } // TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite proves the F2 @@ -213,8 +219,9 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { "{}", 24, 7, 48, - false, // laddering_enabled = false - false, // ladder_execution_enabled = false + false, // laddering_enabled = false + false, // ladder_execution_enabled = false + "convertible", // offering_class ) // Strict order: the SELECT and the UPSERT must sit between the same @@ -223,7 +230,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("SELECT", 1)) mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) - mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(22)...). + mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(23)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -270,6 +277,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack(t *testing.T) { 0, false, false, + "convertible", ) mock.ExpectBegin() diff --git a/internal/config/types.go b/internal/config/types.go index 3b202f15d..a3f3d1535 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -50,7 +50,7 @@ type GlobalConfig struct { // RecommendationsLookbackDays is the AWS Cost Explorer lookback window // (days) used when fetching fresh recommendations. Must be one of 7, - // 30, or 60 — the AWS Cost Explorer LookbackPeriodInDays enum. + // 30, or 60 -- the AWS Cost Explorer LookbackPeriodInDays enum. // GCP CUD Recommender has no equivalent lookback parameter (fixed // internally); this setting applies to AWS only. // Default: 7. @@ -78,6 +78,14 @@ type GlobalConfig struct { // operator explicitly opts in. Fail-loud: wireLadderWriteSide returns // a typed ErrLadderExecutionDisabled when this is false. LadderExecutionEnabled bool `json:"ladder_execution_enabled" db:"ladder_execution_enabled"` + + // OfferingClass controls the EC2 Reserved Instance offering class used + // during purchase. Accepted values: "convertible" (default) and + // "standard". Convertible RIs can be exchanged for a different + // instance family/size/region/OS; Standard RIs are locked to the exact + // instance type for the full term but are ~5% cheaper. + // Unknown values are rejected at purchase time with an explicit error. + OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` } // DefaultGracePeriodDays is the fallback window used when a provider diff --git a/internal/config/validation.go b/internal/config/validation.go index 014014100..2f5d80677 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -166,6 +166,11 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { return "", false } +// ValidOfferingClasses lists the accepted EC2 RI offering class values for +// GlobalConfig. The empty string is also accepted (maps to "convertible" at +// purchase time to preserve pre-694 behavior). +var ValidOfferingClasses = []string{"convertible", "standard"} + // ValidRampScheduleTypes lists all supported ramp schedule types. var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"} @@ -189,16 +194,26 @@ func (c *GlobalConfig) Validate() error { if err := validateCoverage(c.DefaultCoverage); err != nil { return err } + if err := c.validateScheduleAndNotifications(); err != nil { + return err + } + if err := validateOfferingClass(c.OfferingClass); err != nil { + return err + } + return c.validateRecommendationsFields() +} + +// validateScheduleAndNotifications validates collection_schedule and +// notification_days_before. Extracted to keep Validate's cyclomatic +// complexity under the project limit. +func (c *GlobalConfig) validateScheduleAndNotifications() error { if !isValidCollectionSchedule(c.CollectionSchedule) { return fmt.Errorf("invalid collection_schedule: %q (valid: hourly, daily, weekly)", c.CollectionSchedule) } if c.NotificationDaysBefore < 0 || c.NotificationDaysBefore > MaxNotificationDaysBefore { return fmt.Errorf("notification_days_before must be between 0 and %d, got: %d", MaxNotificationDaysBefore, c.NotificationDaysBefore) } - if err := c.validateGracePeriodDays(); err != nil { - return err - } - return c.validateRecommendationsFields() + return c.validateGracePeriodDays() } // validateRecommendationsFields validates the recommendation-cycle parameters @@ -658,3 +673,20 @@ func (c *LadderConfigDB) validateLadderRampSchedule() error { } return nil } + +// validateOfferingClass validates the EC2 RI offering class value. +// The empty string is accepted (maps to "convertible" at purchase time). +// Only lowercase "convertible" and "standard" are valid non-empty values; +// the check is case-sensitive to match how resolveOfferingClassType parses +// the field in providers/aws/services/ec2/client.go. +func validateOfferingClass(oc string) error { + if oc == "" { + return nil + } + for _, valid := range ValidOfferingClasses { + if oc == valid { + return nil + } + } + return fmt.Errorf("invalid offering_class: %q (valid: %s)", oc, strings.Join(ValidOfferingClasses, ", ")) +} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 22ffacd80..94aa7ad02 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -263,6 +263,58 @@ func TestGlobalConfig_Validate(t *testing.T) { }, wantErr: false, }, + // Issue #694: OfferingClass validation on PUT + { + name: "offering_class empty is valid (defaults to convertible)", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "", + }, + wantErr: false, + }, + { + name: "offering_class convertible is valid", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "convertible", + }, + wantErr: false, + }, + { + name: "offering_class standard is valid", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "standard", + }, + wantErr: false, + }, + { + name: "offering_class wrong case is rejected", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "Convertible", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, + { + name: "offering_class STANDARD is rejected (case-sensitive)", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "STANDARD", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, + { + name: "offering_class unknown value is rejected", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "premium", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, } for _, tt := range tests { diff --git a/internal/database/postgres/migrations/000090_ec2_ri_offering_class.down.sql b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.down.sql new file mode 100644 index 000000000..a31499202 --- /dev/null +++ b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.down.sql @@ -0,0 +1 @@ +ALTER TABLE global_config DROP COLUMN IF EXISTS offering_class; diff --git a/internal/database/postgres/migrations/000090_ec2_ri_offering_class.up.sql b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.up.sql new file mode 100644 index 000000000..3bb4de059 --- /dev/null +++ b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.up.sql @@ -0,0 +1,6 @@ +-- offering_class controls whether EC2 Reserved Instances are purchased as +-- Convertible (exchangeable for different families/sizes/OS; default) or +-- Standard (~5% cheaper but locked to the exact instance type for the full +-- term). An empty value is treated as "convertible" in application code. +ALTER TABLE global_config + ADD COLUMN IF NOT EXISTS offering_class TEXT NOT NULL DEFAULT 'convertible'; diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 026f586e7..0eefefcee 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -101,7 +101,10 @@ func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.Purchas } } - totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, exec, plan, accountID, provCfg) + totalSavings, totalUpfront, purchaseErrors, procErr := m.processPurchaseRecommendations(ctx, exec, plan, accountID, provCfg) + if procErr != nil { + return procErr + } if len(purchaseErrors) > 0 && !anyRecPurchased(exec.Recommendations) { // Nothing committed — a clean total failure. @@ -198,6 +201,15 @@ func (m *Manager) executeMultiAccount(ctx context.Context, baseExec *config.Purc return fmt.Errorf("%w: %s", errAllAccountsFailed, strings.Join(errs, "; ")) } +// saveExecutionStatusBestEffort saves the execution record and logs any error. +// Used in error paths where we need to persist the failure status but cannot +// propagate the save error (the original error is already being returned). +func (m *Manager) saveExecutionStatusBestEffort(ctx context.Context, exec *config.PurchaseExecution) { + if err := m.config.SavePurchaseExecution(ctx, exec); err != nil { + logging.Warnf("execution: failed to persist error status for account %v: %v", exec.CloudAccountID, err) + } +} + // executeForAccount runs a single plan execution for one cloud account. // It creates a new PurchaseExecution record tagged with cloud_account_id, resolves // per-account credentials, executes purchases, and saves the result. @@ -241,36 +253,17 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha } accountID := account.ExternalID - totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, &acctExec, plan, accountID, provCfg) - - // #642: a per-account run can be a partial success — some recs committed - // to purchase_history (Purchased=true) while others failed. Marking the - // row "failed" in that case is wrong (it hides the real commitments and - // invites a re-approve that double-buys). Record "partially_completed" - // instead so the row reflects reality, and still send the confirmation - // for the recs that did purchase. - partial := len(purchaseErrors) > 0 && anyRecPurchased(acctExec.Recommendations) - switch { - case partial: - now := time.Now() - acctExec.Status = "partially_completed" - // Append so any audit-gap note already stamped by - // aggregatePurchaseOutcomes (issue #621) survives alongside the - // per-rec failure list. - acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) - acctExec.CompletedAt = &now - case len(purchaseErrors) > 0: + totalSavings, totalUpfront, purchaseErrors, procErr := m.processPurchaseRecommendations(ctx, &acctExec, plan, accountID, provCfg) + if procErr != nil { acctExec.Status = "failed" - acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) - default: - now := time.Now() - acctExec.Status = "completed" - acctExec.CompletedAt = &now + acctExec.Error = procErr.Error() + m.saveExecutionStatusBestEffort(ctx, &acctExec) + return false, procErr } - // committed is the gate executeMultiAccount uses to distinguish a partial - // run from a flat failure (issue #1014): true when any rec purchased. - committed = anyRecPurchased(acctExec.Recommendations) + // Resolve the final status/error/completion stamp from the per-rec outcome + // (#642), and learn whether any rec committed (#1014). + partial, committed := applyAccountOutcome(&acctExec, purchaseErrors) if saveErr := m.config.SavePurchaseExecution(ctx, &acctExec); saveErr != nil { return committed, fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr) @@ -311,6 +304,41 @@ func (m *Manager) persistFailedAccountExecution(ctx context.Context, acctExec *c return cause } +// applyAccountOutcome stamps the final Status / Error / CompletedAt on acctExec +// from the per-rec purchase outcome and reports whether the run was a partial +// success and whether any rec committed. +// +// A per-account run can be a partial success (#642): some recs committed to +// purchase_history (Purchased=true) while others failed. Marking the row +// "failed" in that case is wrong (it hides the real commitments and invites a +// re-approve that double-buys), so record "partially_completed" instead and let +// the caller still send the confirmation for the recs that did purchase. +// +// committed is the gate executeMultiAccount uses to distinguish a partial run +// from a flat failure (#1014): true when any rec purchased. +func applyAccountOutcome(acctExec *config.PurchaseExecution, purchaseErrors []string) (partial, committed bool) { + committed = anyRecPurchased(acctExec.Recommendations) + partial = len(purchaseErrors) > 0 && committed + switch { + case partial: + now := time.Now() + acctExec.Status = "partially_completed" + // Append so any audit-gap note already stamped by + // aggregatePurchaseOutcomes (issue #621) survives alongside the + // per-rec failure list. + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + acctExec.CompletedAt = &now + case len(purchaseErrors) > 0: + acctExec.Status = "failed" + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + default: + now := time.Now() + acctExec.Status = "completed" + acctExec.CompletedAt = &now + } + return partial, committed +} + // resolveSingleAccountProvider derives per-account credentials for the // single-account execution path. Returns (nil, "", nil) when no account can be // identified (ambient credentials are used in that case), or when no recs are @@ -502,7 +530,7 @@ type recPurchaseOutcome struct { index int } -func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (float64, float64, []string) { //nolint:gocritic // unnamedResult: return names would conflict with body locals +func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (totalSavings, totalUpfront float64, purchaseErrors []string, procErr error) { // ExecutionID is carried into PurchaseOptions so executeSinglePurchase // can tag every per-rec log line with the owning exec UUID. Without // this, CloudWatch filtering by exec ID returns zero hits and a stuck @@ -513,13 +541,28 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf ExecutionID: exec.ExecutionID, } + // Load GlobalConfig to pick up the OfferingClass setting for EC2 RI + // purchases. A load error is fatal: proceeding with an empty OfferingClass + // silently defaults to "convertible" even when the operator configured + // "standard", which is an irreversible, more-expensive purchase. Fail the + // run so the operator can diagnose the DB issue and retry with correct + // settings (no-silent-fallbacks-on-money-paths rule). + globalCfg, err := m.config.GetGlobalConfig(ctx) + if err != nil { + return 0, 0, nil, fmt.Errorf("purchase[%s]: failed to load GlobalConfig for offering class: %w", + exec.ExecutionID, err) + } + if globalCfg != nil { + opts.OfferingClass = globalCfg.OfferingClass + } + // Build the list of selected indices once so the fan-out closure only // has to look up rec[i] (no second pass over the full slice). selected := selectedIndices(exec.Recommendations) if len(selected) == 0 { logging.Infof("purchase[%s]: no selected recommendations, nothing to execute (account=%s plan=%q)", exec.ExecutionID, accountID, plan.Name) - return 0, 0, nil + return 0, 0, nil, nil } logging.Infof("purchase[%s]: dispatching %d recommendation(s) for account=%s plan=%q", exec.ExecutionID, len(selected), accountID, plan.Name) @@ -570,7 +613,8 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // there are no concurrent writes to totals, exec.Recommendations, or // purchaseErrors (05-N2). Do NOT move the aggregation inside the FanOut // closure or run it concurrently with the fan-out. - return m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + totalSavings, totalUpfront, purchaseErrors = m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + return totalSavings, totalUpfront, purchaseErrors, nil } // aggregatePurchaseOutcomes walks the fan-out results serially and writes diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 17702d5b7..502ca53f2 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1772,3 +1772,55 @@ func TestManager_ExecutePurchase_SingleAccount_StampsTargetAccount(t *testing.T) }) } } + +// TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting is the +// issue #694 regression guard for HOLE 1: when GetGlobalConfig returns an error, +// processPurchaseRecommendations must return an error and must NOT proceed with +// an empty OfferingClass that silently defaults to "convertible". +// +// The pre-fix code logged the error and continued, so a transient DB failure +// would cause a standard-configured RI to be purchased as convertible (more +// expensive, and irreversible). This test asserts the function returns the DB +// error and PurchaseCommitment is never called. +func TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) + + dbErr := errors.New("connection refused: GlobalConfig read failed") + // Register the GetGlobalConfig expectation so the mock routes to testify + // (rather than the no-op default that returns an empty config). + mockStore.On("GetGlobalConfig", ctx).Return(nil, dbErr) + + exec := &config.PurchaseExecution{ + ExecutionID: "exec-globalcfg-err", + PlanID: "", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Savings: 50, Selected: true}, + }, + } + plan := &config.PurchasePlan{Name: "Direct purchase"} + + manager := &Manager{ + config: mockStore, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + _, _, purchaseErrors, procErr := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + + // The function must return an error derived from the DB failure. + require.Error(t, procErr, "GlobalConfig load failure must abort processing, not silently default to convertible") + assert.Contains(t, procErr.Error(), "GlobalConfig", "error must mention GlobalConfig so operators can diagnose the DB issue") + + // purchaseErrors must be nil: no rec-level errors should be emitted + // because the function must abort before reaching the fan-out. + assert.Nil(t, purchaseErrors, "no rec-level errors expected — function must abort before the fan-out") + + // PurchaseCommitment must NEVER be called: assert the factory was never + // invoked, which is the clearest proxy for "no cloud API call happened". + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider") + + mockStore.AssertExpectations(t) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) +} diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index eaaaf374b..61d96837b 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -67,7 +67,8 @@ func captureIdempotencyTokens(t *testing.T, exec *config.PurchaseExecution) map[ // provCfg is nil: the mock factory matches mock.Anything for it and the // real factory is never reached. - _, _, errs := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + _, _, errs, procErr := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + require.NoError(t, procErr, "processPurchaseRecommendations must not fail in the happy path") require.Empty(t, errs, "all recs must commit so the captured token reflects a real purchase") return tokens } diff --git a/pkg/common/types.go b/pkg/common/types.go index 9a494611d..4a379c43b 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -334,6 +334,12 @@ type PurchaseOptions struct { // CloudWatch / DB execution row (issue #667). The CLI purchase path has // no owning execution and leaves it empty. ExecutionID string + // OfferingClass is the EC2 Reserved Instance offering class for this + // purchase: "convertible" (exchangeable) or "standard" (locked, ~5% + // cheaper). Empty means the caller has not set one; the EC2 client + // defaults to "convertible" to preserve pre-694 behaviour. + // Only meaningful for EC2 RI purchases; ignored by other providers. + OfferingClass string } // NormalizeSource lowercases s and returns it when it matches an allowed diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 0e9af15e0..ba1816ad7 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -145,7 +145,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati } // Find the offering ID - offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) + offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID, opts.OfferingClass) if err != nil { result.Error = fmt.Errorf("failed to find offering: %w", err) return result, result.Error @@ -374,6 +374,23 @@ type ec2OfferingQuery struct { scope string duration int64 wantOfferingType types.OfferingTypeValues + offeringClass types.OfferingClassType +} + +// resolveOfferingClassType maps the GlobalConfig string value to the AWS SDK +// type. "convertible" and "" both produce OfferingClassTypeConvertible so the +// absence of a stored value preserves the pre-694 default. Any other string +// is an explicit error: the caller must not silently fall back, because a DB +// typo would otherwise buy the wrong class (feedback_empty_string_vs_error.md). +func resolveOfferingClassType(s string) (types.OfferingClassType, error) { + switch s { + case "", "convertible": + return types.OfferingClassTypeConvertible, nil + case "standard": + return types.OfferingClassTypeStandard, nil + default: + return "", fmt.Errorf("unknown EC2 RI offering class %q: must be \"convertible\" or \"standard\"", s) + } } // buildEC2OfferingQuery resolves the typed lookup parameters from a rec, @@ -410,13 +427,17 @@ func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDet // typed lookup. Typed fields land on AWS's primary indices; only scope has no // typed equivalent and stays in Filters[]. func describeInputFromQuery(q ec2OfferingQuery, nextToken *string) *ec2.DescribeReservedInstancesOfferingsInput { + oc := q.offeringClass + if oc == "" { + oc = types.OfferingClassTypeConvertible + } return &ec2.DescribeReservedInstancesOfferingsInput{ InstanceType: q.instanceType, ProductDescription: q.productDesc, InstanceTenancy: q.tenancy, MinDuration: aws.Int64(q.duration), MaxDuration: aws.Int64(q.duration), - OfferingClass: types.OfferingClassTypeConvertible, + OfferingClass: oc, OfferingType: q.wantOfferingType, IncludeMarketplace: aws.Bool(false), MaxResults: aws.Int32(100), @@ -461,11 +482,18 @@ func (c *Client) buildEC2QueryFromRec(rec common.Recommendation) (ec2OfferingQue // // execID is the purchase execution UUID for log correlation; pass "" when // calling outside of a purchase flow (ValidateOffering, GetOfferingDetails). -func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) { +// offeringClassStr is the GlobalConfig.OfferingClass value; "" is treated as +// "convertible" to preserve pre-694 behaviour. Unknown values fail loudly. +func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string, offeringClassStr string) (string, error) { q, err := c.buildEC2QueryFromRec(rec) if err != nil { return "", err } + oc, err := resolveOfferingClassType(offeringClassStr) + if err != nil { + return "", fmt.Errorf("offering class config error: %w", err) + } + q.offeringClass = oc tag := execID if tag == "" { @@ -537,15 +565,19 @@ func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, wantType t return "" } -// ValidateOffering checks if an offering exists without purchasing +// ValidateOffering checks if an offering exists without purchasing. +// Uses the convertible class (empty = convertible default) since the +// validation path has no GlobalConfig context. func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation) error { - _, err := c.findOfferingID(ctx, rec, "") + _, err := c.findOfferingID(ctx, rec, "", "") return err } -// GetOfferingDetails retrieves offering details +// GetOfferingDetails retrieves offering details. +// Uses the convertible class (empty = convertible default) since the +// details-fetch path has no GlobalConfig context. func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - offeringID, err := c.findOfferingID(ctx, rec, "") + offeringID, err := c.findOfferingID(ctx, rec, "", "") if err != nil { return nil, err } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index ae621601b..05f623f61 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -38,6 +38,19 @@ func (m *MockEC2Client) DescribeReservedInstancesOfferings(ctx context.Context, return args.Get(0).(*ec2.DescribeReservedInstancesOfferingsOutput), args.Error(1) } +// lastDescribeOfferingsInput captures the most recent +// DescribeReservedInstancesOfferings params for assertion in integration tests. +// Only used in tests that set this field explicitly; nil means uncaptured. +type capturingMockEC2Client struct { + MockEC2Client + LastDescribeOfferingsInput *ec2.DescribeReservedInstancesOfferingsInput +} + +func (m *capturingMockEC2Client) DescribeReservedInstancesOfferings(ctx context.Context, params *ec2.DescribeReservedInstancesOfferingsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesOfferingsOutput, error) { + m.LastDescribeOfferingsInput = params + return m.MockEC2Client.DescribeReservedInstancesOfferings(ctx, params, optFns...) +} + func (m *MockEC2Client) DescribeReservedInstances(ctx context.Context, params *ec2.DescribeReservedInstancesInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesOutput, error) { args := m.Called(ctx, params) if args.Get(0) == nil { @@ -610,7 +623,7 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { }, nil).Once() } - _, err := client.findOfferingID(context.Background(), rec, "") + _, err := client.findOfferingID(context.Background(), rec, "", "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "pagination cap reached") @@ -653,7 +666,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { }, }, nil).Once() - _, err := client.findOfferingID(context.Background(), rec, "") + _, err := client.findOfferingID(context.Background(), rec, "", "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "no offerings found") @@ -691,7 +704,7 @@ func TestFindOfferingID_HappyPath(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), rec, "") + id, err := client.findOfferingID(context.Background(), rec, "", "") assert.NoError(t, err) assert.Equal(t, "offering-ok", id) @@ -1072,3 +1085,143 @@ func TestClient_CancelMarketplaceListing_APIError(t *testing.T) { assert.Contains(t, err.Error(), "CancelReservedInstancesListing failed") mockEC2.AssertExpectations(t) } + +func TestResolveOfferingClassType(t *testing.T) { + t.Parallel() + tests := []struct { + input string + want types.OfferingClassType + wantErr bool + }{ + {"convertible", types.OfferingClassTypeConvertible, false}, + {"", types.OfferingClassTypeConvertible, false}, // empty = default = convertible + {"standard", types.OfferingClassTypeStandard, false}, + {"STANDARD", "", true}, // case-sensitive + {"unknown", "", true}, // unknown value must error + {"Convertible", "", true}, // wrong case must error + } + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { + t.Parallel() + got, err := resolveOfferingClassType(tc.input) + if tc.wantErr { + assert.Error(t, err, "expected error for input %q", tc.input) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.want, got) + } + }) + } +} + +func TestDescribeInputFromQuery_OfferingClass(t *testing.T) { + t.Parallel() + base := ec2OfferingQuery{ + instanceType: types.InstanceTypeT3Micro, + productDesc: types.RIProductDescriptionLinuxUnix, + tenancy: types.TenancyDefault, + scope: string(types.ScopeRegional), + duration: 94608000, + wantOfferingType: types.OfferingTypeValuesNoUpfront, + } + + t.Run("convertible by default (empty field)", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = "" + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeConvertible, inp.OfferingClass) + }) + + t.Run("convertible explicit", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = types.OfferingClassTypeConvertible + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeConvertible, inp.OfferingClass) + }) + + t.Run("standard explicit", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = types.OfferingClassTypeStandard + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeStandard, inp.OfferingClass) + }) +} + +// TestFindOfferingID_OfferingClassReachesSDKCall is the integration test for +// issue #694: it verifies that the offeringClassStr argument passed to +// findOfferingID is wired all the way through to the OfferingClass field on +// the outbound DescribeReservedInstancesOfferings SDK call. The test fails if +// the wiring regresses (e.g. the field is dropped or hardcoded). +func TestFindOfferingID_OfferingClassReachesSDKCall(t *testing.T) { + t.Parallel() + + rec := common.Recommendation{ + ResourceType: "m5.large", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + offeringOutput := &ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{ + { + ReservedInstancesOfferingId: aws.String("offering-std-123"), + InstanceType: types.InstanceTypeM5Large, + OfferingType: types.OfferingTypeValuesAllUpfront, + }, + }, + } + + tests := []struct { + name string + offeringClassStr string + wantOfferingClass types.OfferingClassType + }{ + { + name: "empty string defaults to convertible", + offeringClassStr: "", + wantOfferingClass: types.OfferingClassTypeConvertible, + }, + { + name: "convertible explicit reaches SDK as convertible", + offeringClassStr: "convertible", + wantOfferingClass: types.OfferingClassTypeConvertible, + }, + { + name: "standard reaches SDK as standard", + offeringClassStr: "standard", + wantOfferingClass: types.OfferingClassTypeStandard, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cap := &capturingMockEC2Client{} + cap.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(offeringOutput, nil).Once() + + client := &Client{client: cap, region: "us-east-1"} + + id, err := client.findOfferingID(context.Background(), rec, "", tc.offeringClassStr) + assert.NoError(t, err) + assert.Equal(t, "offering-std-123", id) + + if assert.NotNil(t, cap.LastDescribeOfferingsInput, "DescribeReservedInstancesOfferings must have been called") { + assert.Equal(t, tc.wantOfferingClass, cap.LastDescribeOfferingsInput.OfferingClass, + "OfferingClass on the SDK call must match the configured value") + } + cap.AssertExpectations(t) + }) + } +}