From 60c9dfa89d939585c28d6238560a3ed66becefa1 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:58:35 +1000 Subject: [PATCH] fix: remove rds panics --- cmd/rds-agent/engine.go | 4 +- cmd/rds-agent/engine_mariadb_test.go | 5 +- cmd/rds-agent/engine_postgres.go | 26 ++++---- cmd/rds-agent/engine_test.go | 36 +++++++---- spinifex/daemon/daemon.go | 3 + spinifex/handlers/rds/backup.go | 24 +++---- spinifex/handlers/rds/backup_test.go | 9 +-- spinifex/handlers/rds/engine.go | 35 +++++----- spinifex/handlers/rds/engine_mariadb.go | 2 +- spinifex/handlers/rds/engine_postgres.go | 2 +- spinifex/handlers/rds/enginefamily_test.go | 74 ++++++++++++++++++++-- spinifex/handlers/rds/iam.go | 14 ++-- spinifex/handlers/rds/iam_test.go | 8 ++- spinifex/handlers/rds/paramcatalog.go | 28 +++++--- spinifex/handlers/rds/paramcatalog_test.go | 60 ++++++++++++++++++ 15 files changed, 237 insertions(+), 93 deletions(-) diff --git a/cmd/rds-agent/engine.go b/cmd/rds-agent/engine.go index 42f8c2ac6..c56b494c5 100644 --- a/cmd/rds-agent/engine.go +++ b/cmd/rds-agent/engine.go @@ -108,9 +108,7 @@ var engineLayouts = map[string]engineLayout{ dataMount: "/var/lib/postgresql", port: 5432, newProbe: newPostgresProbe, - newEngine: func(cfg config, run commandRunner, startSess sessionRunner, probe *engineProbe) (engine, error) { - return newPostgresEngine(cfg, run, startSess, probe), nil - }, + newEngine: newPostgresEngineFromCatalog, }, engineMariaDB: { binDir: "/usr/bin", diff --git a/cmd/rds-agent/engine_mariadb_test.go b/cmd/rds-agent/engine_mariadb_test.go index 96f79b399..d99c2a5e9 100644 --- a/cmd/rds-agent/engine_mariadb_test.go +++ b/cmd/rds-agent/engine_mariadb_test.go @@ -129,10 +129,11 @@ func newScriptedTestMariaDBEngine(t *testing.T, run commandRunner) *mariadbEngin // that is registered: what a spec says about a parameter is what the guest must // see, and an unlisted name must read as static rather than as dynamic. func TestControlPlaneRulesFrom_TakesTheClassificationFromTheCatalog(t *testing.T) { - rules := controlPlaneRulesFrom(postgresEngineMeta) + meta := testPostgresEngineMeta(t) + rules := controlPlaneRulesFrom(meta) for name, want := range map[string]bool{"shared_buffers": true, "work_mem": false} { - spec, ok := postgresEngineMeta.LookupParameter(name) + spec, ok := meta.LookupParameter(name) if !ok { t.Fatalf("the catalog no longer carries %s", name) } diff --git a/cmd/rds-agent/engine_postgres.go b/cmd/rds-agent/engine_postgres.go index ca7de963b..c266ea3e1 100644 --- a/cmd/rds-agent/engine_postgres.go +++ b/cmd/rds-agent/engine_postgres.go @@ -70,19 +70,6 @@ func postgresProbeState(host string, run probeRunner) probeStateFn { } } -// Resolved once, under the name the control plane knows this implementation by. -// A lookup failure is a mismatch between the agent and the control plane it was -// built against, so it fails at startup rather than at the first rotation. -var postgresEngineMeta = mustLookupEngine(enginePostgres) - -func mustLookupEngine(name string) handlers_rds.Engine { - engine, err := handlers_rds.LookupEngine(name) - if err != nil { - panic("rds-agent: " + err.Error()) - } - return engine -} - // The include the resolved set is rendered to, and the copy of the last one the // engine accepted. Both live beside the data rather than in /etc, matching // rds-init: a class change boots a fresh root volume and would revert them. @@ -114,9 +101,18 @@ hostnossl all all ::/0 reject postgresForceSSLRuleCount = 2 ) -func newPostgresEngine(cfg config, run commandRunner, startSess sessionRunner, probe *engineProbe) *postgresEngine { +// The layout's factory resolves the control plane metadata during startup. +func newPostgresEngineFromCatalog(cfg config, run commandRunner, startSess sessionRunner, probe *engineProbe) (engine, error) { + meta, err := handlers_rds.LookupEngine(enginePostgres) + if err != nil { + return nil, fmt.Errorf("this image bakes %s, which this build's control plane does not offer: %w", enginePostgres, err) + } + return newPostgresEngine(cfg, meta, run, startSess, probe), nil +} + +func newPostgresEngine(cfg config, meta handlers_rds.Engine, run commandRunner, startSess sessionRunner, probe *engineProbe) *postgresEngine { return &postgresEngine{ - meta: postgresEngineMeta, + meta: meta, run: run, startSess: startSess, psql: filepath.Join(cfg.EngineBinDir, "psql"), diff --git a/cmd/rds-agent/engine_test.go b/cmd/rds-agent/engine_test.go index c74daa54a..07d1d2c36 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -140,10 +140,19 @@ func (r *recordingRunner) run(_ context.Context, c command) (string, error) { return r.out, r.err } +func testPostgresEngineMeta(t *testing.T) handlers_rds.Engine { + t.Helper() + meta, err := handlers_rds.LookupEngine(enginePostgres) + if err != nil { + t.Fatalf("LookupEngine(%s): %v", enginePostgres, err) + } + return meta +} + func newTestEngine(t *testing.T, run commandRunner) *postgresEngine { t.Helper() cfg := testEngineConfig(t) - return newPostgresEngine(cfg, withPostgresReadBacks(cfg.EngineDataDir, run), nil, newPostgresProbe(cfg, staticProbe(0))) + return newPostgresEngine(cfg, testPostgresEngineMeta(t), withPostgresReadBacks(cfg.EngineDataDir, run), nil, newPostgresProbe(cfg, staticProbe(0))) } // The same engine with the read-backs left to the runner, for the cases that are @@ -152,7 +161,7 @@ func newTestEngine(t *testing.T, run commandRunner) *postgresEngine { func newScriptedTestEngine(t *testing.T, run commandRunner) *postgresEngine { t.Helper() cfg := testEngineConfig(t) - return newPostgresEngine(cfg, run, nil, newPostgresProbe(cfg, staticProbe(0))) + return newPostgresEngine(cfg, testPostgresEngineMeta(t), run, nil, newPostgresProbe(cfg, staticProbe(0))) } func testEngineConfig(t *testing.T) config { @@ -479,10 +488,11 @@ func (r *tlsReadBackRunner) run(ctx context.Context, c command) (string, error) return r.recordingRunner.run(ctx, c) } -func forceSSLParameter(value string) []handlers_rds.Parameter { +func forceSSLParameter(t *testing.T, value string) []handlers_rds.Parameter { + t.Helper() return []handlers_rds.Parameter{ {Name: "work_mem", Value: "4096"}, - {Name: postgresEngineMeta.TLSEnforcementParameter(), Value: value}, + {Name: testPostgresEngineMeta(t).TLSEnforcementParameter(), Value: value}, } } @@ -492,7 +502,7 @@ func TestPostgresEngine_ApplyParametersWritesTheEnforcementRule(t *testing.T) { runner := &tlsReadBackRunner{ssl: "on", ruleRows: postgresForceSSLRuleCount} engine := newScriptedTestEngine(t, runner.run) - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("1")); err != nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "1")); err != nil { t.Fatalf("ApplyParameters: %v", err) } rule, err := os.ReadFile(engine.forceSSLRulePath()) @@ -516,7 +526,7 @@ func TestPostgresEngine_ApplyParametersRemovesAStaleEnforcementRule(t *testing.T t.Fatalf("write the rule the restored volume carried: %v", err) } - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("0")); err != nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "0")); err != nil { t.Fatalf("ApplyParameters: %v", err) } if _, err := os.Stat(engine.forceSSLRulePath()); !os.IsNotExist(err) { @@ -547,7 +557,7 @@ func TestPostgresEngine_ApplyParametersRefusesAnUnparsableEnforcementValue(t *te runner := &tlsReadBackRunner{ssl: "on"} engine := newScriptedTestEngine(t, runner.run) - _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("yes")) + _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "yes")) if err == nil || !strings.Contains(err.Error(), "neither 1 nor 0") { t.Fatalf("ApplyParameters error = %v, want a refusal naming the unreadable value", err) } @@ -562,7 +572,7 @@ func TestPostgresEngine_ApplyParametersRefusesToEnforceWithoutTLS(t *testing.T) runner := &tlsReadBackRunner{ssl: "off"} engine := newScriptedTestEngine(t, runner.run) - _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("1")) + _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "1")) if err == nil || !strings.Contains(err.Error(), "serving without TLS") { t.Fatalf("ApplyParameters error = %v, want a refusal naming the engine's own TLS state", err) } @@ -592,7 +602,7 @@ func TestPostgresEngine_ApplyParametersFailsAnUnverifiableEnforcement(t *testing runner := &tlsReadBackRunner{ssl: "on", ruleRows: tc.ruleRows, brokenRows: tc.brokenRows} engine := newScriptedTestEngine(t, runner.run) - _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(tc.value)) + _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, tc.value)) if err == nil || !strings.Contains(err.Error(), tc.want) { t.Fatalf("ApplyParameters error = %v, want %q", err, tc.want) } @@ -606,13 +616,13 @@ func TestPostgresEngine_ApplyParametersFailsAnUnverifiableEnforcement(t *testing func TestPostgresEngine_ApplyParametersWithdrawsBothFilesTogether(t *testing.T) { runner := &tlsReadBackRunner{ssl: "on", ruleRows: postgresForceSSLRuleCount} engine := newScriptedTestEngine(t, runner.run) - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("1")); err != nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "1")); err != nil { t.Fatalf("first ApplyParameters: %v", err) } // Turning enforcement off, verified against a pg_hba that still carries the // rule — so the apply fails after both files have already been replaced. - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("0")); err == nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "0")); err == nil { t.Fatal("ApplyParameters succeeded against an unverifiable enforcement") } installed, err := os.ReadFile(engine.params.installedPath()) @@ -639,14 +649,14 @@ func TestPostgresEngine_RestoreLastKnownGoodReDerivesEnforcement(t *testing.T) { runner := &tlsReadBackRunner{ssl: "on", ruleRows: postgresForceSSLRuleCount} engine := newScriptedTestEngine(t, runner.run) - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("1")); err != nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "1")); err != nil { t.Fatalf("ApplyParameters(enforcing): %v", err) } if err := engine.RecordServingParameters(t.Context()); err != nil { t.Fatalf("RecordServingParameters: %v", err) } runner.ruleRows = 0 - if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter("0")); err != nil { + if _, err := engine.ApplyParameters(t.Context(), forceSSLParameter(t, "0")); err != nil { t.Fatalf("ApplyParameters(not enforcing): %v", err) } if _, err := os.Stat(engine.forceSSLRulePath()); !os.IsNotExist(err) { diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index b6cd62bc2..005c2f21e 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1808,6 +1808,9 @@ func (d *Daemon) startCluster() error { // whichever node the queue group picks, so a node without it would make the // first boot of a DB instance fail intermittently rather than not at all. d.rdsService, err = initServiceWithRetry("RDS service", func() (*handlers_rds.Service, error) { + if registryErr := handlers_rds.ValidateEngineRegistry(); registryErr != nil { + return nil, registryErr + } deps, depsErr := d.buildRDSDeps() if depsErr != nil { return nil, depsErr diff --git a/spinifex/handlers/rds/backup.go b/spinifex/handlers/rds/backup.go index 9379678cc..179ada89a 100644 --- a/spinifex/handlers/rds/backup.go +++ b/spinifex/handlers/rds/backup.go @@ -31,12 +31,6 @@ const ( // would pay the whole storage cost above for a fraction of the coverage. defaultBackupRetentionDays = 7 - // The UTC blocks an unnamed window is assigned inside. Deliberately disjoint: - // the two derived windows then cannot overlap, which is the pair AWS refuses - // to accept and the pair that would quiesce an engine mid-maintenance. - defaultBackupWindowBlock = "03:00-11:00" //nolint:gosec // a UTC window block, not a credential - defaultMaintenanceWindowBlock = "11:00-19:00" - // How many automated snapshots one retention pass may delete. A pass that // under-collects is corrected by the next one two minutes later; a pass that // walks an unbounded number of snapshots is not. @@ -60,6 +54,13 @@ const ( automatedBackupRetryShiftCap = 3 ) +// The UTC blocks an unnamed window is assigned inside. Deliberately disjoint, +// so derived backup and maintenance windows cannot overlap. +var ( + defaultBackupWindowBlock = dailyWindow{start: 3 * time.Hour, end: 11 * time.Hour} + defaultMaintenanceWindowBlock = dailyWindow{start: 11 * time.Hour, end: 19 * time.Hour} +) + // The operator-tunable backup settings: bounds and defaults, never a per-instance // setting — those live on the DB instance record. type BackupPolicy struct { @@ -114,20 +115,15 @@ func (s *Service) maintenanceWindowBlock() dailyWindow { return s.windowBlock(s.deps.Backup.MaintenanceWindowBlock, defaultMaintenanceWindowBlock, "maintenance") } -func (s *Service) windowBlock(configured, fallback, kind string) dailyWindow { +func (s *Service) windowBlock(configured string, fallback dailyWindow, kind string) dailyWindow { if configured != "" { if block, err := parseDailyWindow("block", configured); err == nil { return block } slog.Warn("rds: the configured "+kind+" window block is malformed; using the built-in block", - "block", configured, "fallback", fallback) + "block", configured, "fallback", fallback.String()) } - block, err := parseDailyWindow("block", fallback) - if err != nil { - // Unreachable: the fallbacks are constants this package's tests parse. - panic("rds: built-in " + kind + " window block is malformed: " + err.Error()) - } - return block + return fallback } // The window in force for this instance. A record that names one uses it; one diff --git a/spinifex/handlers/rds/backup_test.go b/spinifex/handlers/rds/backup_test.go index acb7477b1..f3520242a 100644 --- a/spinifex/handlers/rds/backup_test.go +++ b/spinifex/handlers/rds/backup_test.go @@ -321,13 +321,8 @@ func TestWindowBlock_FallsBackToTheBuiltInBlock(t *testing.T) { MaintenanceWindowBlock: "19:00", }}) - builtIn, err := parseDailyWindow("block", defaultBackupWindowBlock) - require.NoError(t, err) - assert.Equal(t, builtIn, svc.backupWindowBlock()) - - builtIn, err = parseDailyWindow("block", defaultMaintenanceWindowBlock) - require.NoError(t, err) - assert.Equal(t, builtIn, svc.maintenanceWindowBlock()) + assert.Equal(t, defaultBackupWindowBlock, svc.backupWindowBlock()) + assert.Equal(t, defaultMaintenanceWindowBlock, svc.maintenanceWindowBlock()) } func TestValidateRetentionPeriod_BoundsTheRetention(t *testing.T) { diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index fbba29567..a3f93a711 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -63,40 +63,45 @@ var engines = map[string]Engine{ // The same registry keyed by parameter-group family, for the callers that hold // only a family string and have no instance to derive an engine from. Family // and engine are 1:1 by construction, so one registry serves both. -var enginesByFamily = indexEnginesByFamily() +var enginesByFamily, engineRegistryValidationErr = indexEnginesByFamily(engines) -func indexEnginesByFamily() map[string]Engine { - out := make(map[string]Engine, len(engines)) - for _, engine := range engines { - // An engine registered without its own name rule would panic on the first - // create that names a database, rather than here where every build sees it. +// ValidateEngineRegistry reports invalid built-in engine metadata before RDS starts. +func ValidateEngineRegistry() error { + if err := validateParameterCatalogs(); err != nil { + return err + } + return engineRegistryValidationErr +} + +func indexEnginesByFamily(registry map[string]Engine) (map[string]Engine, error) { + out := make(map[string]Engine, len(registry)) + for _, engine := range registry { if engine.validateDBName == nil { - panic("rds: engine " + engine.Name + " registers no DBName rule") + return nil, errors.New("rds: engine " + engine.Name + " registers no DBName rule") } - // An engine without these would tell a customer nothing about what a - // crash-consistent snapshot or an unclean stop of it recovers, on the two - // events that say so. + // Without these, snapshot and unclean-stop events cannot explain what the + // engine will recover. if engine.crashRecoveryNote == "" { - panic("rds: engine " + engine.Name + " registers no crash-recovery note") + return nil, errors.New("rds: engine " + engine.Name + " registers no crash-recovery note") } if engine.uncleanStopNote == "" { - panic("rds: engine " + engine.Name + " registers no unclean-stop note") + return nil, errors.New("rds: engine " + engine.Name + " registers no unclean-stop note") } // A name no catalog entry answers to would leave the guest deriving // enforcement from a key nothing ever writes, which reads as not enforcing. if name := engine.tlsEnforcementParameter; name != "" { spec, ok := engine.catalog[name] if !ok || spec.DataType != ParamTypeBoolean { - panic("rds: engine " + engine.Name + " names " + name + ", which is not a boolean parameter it exposes") + return nil, errors.New("rds: engine " + engine.Name + " names " + name + ", which is not a boolean parameter it exposes") } } family := engine.ParameterGroupFamily() if _, exists := out[family]; exists { - panic("rds: parameter group family " + family + " is claimed by two engines") + return nil, errors.New("rds: parameter group family " + family + " is claimed by two engines") } out[family] = engine } - return out + return out, nil } // The pinned version an AMI lookup resolves against, which is the major alone: diff --git a/spinifex/handlers/rds/engine_mariadb.go b/spinifex/handlers/rds/engine_mariadb.go index e795ba8c7..f72d0e3e0 100644 --- a/spinifex/handlers/rds/engine_mariadb.go +++ b/spinifex/handlers/rds/engine_mariadb.go @@ -49,7 +49,7 @@ var engineMariaDB = Engine{ // Platform-owned settings are absent, except the ones AWS exposes as modifiable // and this platform pins: those are present and unmodifiable, so a refusal reads // as policy rather than as a missing feature. -var mariadbParameterCatalog = buildParameterCatalog( +var mariadbParameterCatalog, mariadbParameterCatalogErr = buildParameterCatalog( // Connections and threads. max_connections is what a size-derived default // matters most for: RDS's own formula is {DBInstanceClassMemory/12582880}. ParameterSpec{ diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/handlers/rds/engine_postgres.go index f9aa8442f..34914a268 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/handlers/rds/engine_postgres.go @@ -43,7 +43,7 @@ var enginePostgres = Engine{ // Platform-owned settings are absent, except the ones AWS exposes as modifiable // and this platform pins: those are present and unmodifiable, so a refusal reads // as policy rather than as a missing feature. -var postgresParameterCatalog = buildParameterCatalog( +var postgresParameterCatalog, postgresParameterCatalogErr = buildParameterCatalog( // Connections. max_connections is what a size-derived default matters most // for: RDS's own formula is LEAST({DBInstanceClassMemory/9531392}, 5000). ParameterSpec{ diff --git a/spinifex/handlers/rds/enginefamily_test.go b/spinifex/handlers/rds/enginefamily_test.go index db11afe32..3574ee801 100644 --- a/spinifex/handlers/rds/enginefamily_test.go +++ b/spinifex/handlers/rds/enginefamily_test.go @@ -34,18 +34,18 @@ var engineUnderTest = Engine{ reservedUsernamePrefixes: []string{"testdb_"}, maxUsernameLen: 80, validateDBName: dbNameRule(64), - catalog: buildParameterCatalog( - ParameterSpec{ + catalog: map[string]ParameterSpec{ + "buffer_size": { Name: "buffer_size", DataType: ParamTypeInteger, ApplyType: ApplyTypeStatic, IsModifiable: true, Min: 1, Max: 1024, Default: "64", Unit: "MB", Description: "Buffer the server allocates at startup, in MB.", }, - ParameterSpec{ + "connection_limit": { Name: "connection_limit", DataType: ParamTypeInteger, ApplyType: ApplyTypeDynamic, IsModifiable: true, Min: 1, Max: 1000, Default: "100", Description: "Maximum concurrent connections to the server.", }, - ), + }, validateCombinations: func([]Parameter) error { return nil }, crashRecoveryNote: "It will recover when it is restored.", uncleanStopNote: "It will recover on the next start.", @@ -57,13 +57,75 @@ func registerTestEngine(t *testing.T) { t.Helper() require.NotContains(t, engines, engineUnderTest.Name) engines[engineUnderTest.Name] = engineUnderTest - enginesByFamily = indexEnginesByFamily() + indexed, err := indexEnginesByFamily(engines) + require.NoError(t, err) + enginesByFamily = indexed t.Cleanup(func() { delete(engines, engineUnderTest.Name) - enginesByFamily = indexEnginesByFamily() + indexed, err := indexEnginesByFamily(engines) + require.NoError(t, err) + enginesByFamily = indexed }) } +func TestValidateEngineRegistry(t *testing.T) { + require.NoError(t, ValidateEngineRegistry()) +} + +func TestIndexEnginesByFamily_RejectsInvalidMetadata(t *testing.T) { + noDBNameRule := engineUnderTest + noDBNameRule.validateDBName = nil + noCrashRecoveryNote := engineUnderTest + noCrashRecoveryNote.crashRecoveryNote = "" + noUncleanStopNote := engineUnderTest + noUncleanStopNote.uncleanStopNote = "" + invalidTLSParameter := engineUnderTest + invalidTLSParameter.tlsEnforcementParameter = "not-a-boolean-parameter" + + tests := []struct { + name string + registry map[string]Engine + wantErr string + }{ + { + name: "missing DB name rule", + registry: map[string]Engine{"test": noDBNameRule}, + wantErr: "registers no DBName rule", + }, + { + name: "missing crash recovery note", + registry: map[string]Engine{"test": noCrashRecoveryNote}, + wantErr: "registers no crash-recovery note", + }, + { + name: "missing unclean stop note", + registry: map[string]Engine{"test": noUncleanStopNote}, + wantErr: "registers no unclean-stop note", + }, + { + name: "invalid TLS enforcement parameter", + registry: map[string]Engine{"test": invalidTLSParameter}, + wantErr: "is not a boolean parameter it exposes", + }, + { + name: "duplicate parameter group family", + registry: map[string]Engine{ + "first": engineUnderTest, + "second": engineUnderTest, + }, + wantErr: "is claimed by two engines", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + indexed, err := indexEnginesByFamily(tt.registry) + require.ErrorContains(t, err, tt.wantErr) + assert.Nil(t, indexed) + }) + } +} + func testEngineGroupInput(name string) *rds.CreateDBParameterGroupInput { return &rds.CreateDBParameterGroupInput{ DBParameterGroupName: aws.String(name), diff --git a/spinifex/handlers/rds/iam.go b/spinifex/handlers/rds/iam.go index 8f1d4b173..239cdc85e 100644 --- a/spinifex/handlers/rds/iam.go +++ b/spinifex/handlers/rds/iam.go @@ -31,7 +31,7 @@ var InternalAgentActions = []string{ // Never rds:*: granting the customer surface here would let a Postgres RCE on // one DB VM manage its account's whole fleet. -var instanceRoleInlinePolicy = func() string { +func instanceRoleInlinePolicy() (string, error) { actions := make(handlers_iam.StringOrArr, 0, len(InternalAgentActions)) for _, action := range InternalAgentActions { actions = append(actions, "rds:"+action) @@ -45,10 +45,10 @@ var instanceRoleInlinePolicy = func() string { }}, }) if err != nil { - panic("rds: cannot marshal the instance-role policy: " + err.Error()) + return "", fmt.Errorf("marshal instance-role policy: %w", err) } - return string(doc) -}() + return string(doc), nil +} // Resolved per launch rather than held, mirroring EKS: the KV-backed IAM // service has no responders until JetStream is up, so an eager build races @@ -65,8 +65,12 @@ func ensureInstanceProfile(provider IAMProvider, accountID string) (string, erro if iamSvc == nil { return "", errors.New("rds: IAM service is unavailable") } + policy, err := instanceRoleInlinePolicy() + if err != nil { + return "", fmt.Errorf("rds: construct instance-role inline policy: %w", err) + } profileARN, err := handlers_iam.EnsureSystemInstanceProfile(iamSvc, accountID, - InstanceRoleName, instanceRoleInlinePolicyName, instanceRoleInlinePolicy) + InstanceRoleName, instanceRoleInlinePolicyName, policy) if err != nil { return "", fmt.Errorf("rds: ensure instance profile %s for account %s: %w", InstanceRoleName, accountID, err) diff --git a/spinifex/handlers/rds/iam_test.go b/spinifex/handlers/rds/iam_test.go index 8af13819f..e2fba7ebd 100644 --- a/spinifex/handlers/rds/iam_test.go +++ b/spinifex/handlers/rds/iam_test.go @@ -28,8 +28,10 @@ func rdsInstanceProfileARN(accountID string) string { // internal actions and nothing wildcarded that could reach the customer surface. func TestInstanceRolePolicy_GrantsOnlyTheInternalActions(t *testing.T) { t.Parallel() + policy, err := instanceRoleInlinePolicy() + require.NoError(t, err) var doc handlers_iam.PolicyDocument - require.NoError(t, json.Unmarshal([]byte(instanceRoleInlinePolicy), &doc)) + require.NoError(t, json.Unmarshal([]byte(policy), &doc)) require.Len(t, doc.Statement, 1) stmt := doc.Statement[0] @@ -81,9 +83,11 @@ func TestEnsureInstanceProfile_IsIdempotent(t *testing.T) { assert.Equal(t, 1, f.CreateRoleCalls, "the second launch must find the role, not create it") assert.Equal(t, 1, f.CreateInstanceProfileCalls, "the second launch must find the instance profile") require.Len(t, f.PolicyCalls, 2, "the grant is re-asserted on every launch") + policy, err := instanceRoleInlinePolicy() + require.NoError(t, err) for _, put := range f.PolicyCalls { assert.Equal(t, instanceRoleInlinePolicyName, aws.StringValue(put.PolicyName)) - assert.Equal(t, instanceRoleInlinePolicy, aws.StringValue(put.PolicyDocument)) + assert.Equal(t, policy, aws.StringValue(put.PolicyDocument)) } } diff --git a/spinifex/handlers/rds/paramcatalog.go b/spinifex/handlers/rds/paramcatalog.go index 99a706cb3..4467d2ba4 100644 --- a/spinifex/handlers/rds/paramcatalog.go +++ b/spinifex/handlers/rds/paramcatalog.go @@ -1,6 +1,7 @@ package handlers_rds import ( + "errors" "fmt" "maps" "math" @@ -89,27 +90,36 @@ type ParameterSpec struct { optionFileName string } -// Indexes the specs by name and fails the build-equivalent — process start — on -// a malformed entry, so a catalog typo cannot reach a customer's create. -func buildParameterCatalog(specs ...ParameterSpec) map[string]ParameterSpec { +// Indexes the specs by name and returns malformed entries to startup validation. +func buildParameterCatalog(specs ...ParameterSpec) (map[string]ParameterSpec, error) { out := make(map[string]ParameterSpec, len(specs)) for _, spec := range specs { switch { case spec.Name == "": - panic("rds: parameter catalog holds an unnamed entry") + return nil, errors.New("rds: parameter catalog holds an unnamed entry") case spec.Default == "" && spec.DefaultFor == nil: - panic("rds: parameter catalog entry " + spec.Name + " has no default") + return nil, fmt.Errorf("rds: parameter catalog entry %s has no default", spec.Name) case spec.Default != "" && spec.DefaultFor != nil: - panic("rds: parameter catalog entry " + spec.Name + " has both a literal and a computed default") + return nil, fmt.Errorf("rds: parameter catalog entry %s has both a literal and a computed default", spec.Name) case (spec.DefaultFor == nil) != (spec.MaxFor == nil): - panic("rds: parameter catalog entry " + spec.Name + " must pair its computed default and class ceiling") + return nil, fmt.Errorf("rds: parameter catalog entry %s must pair its computed default and class ceiling", spec.Name) } if _, exists := out[spec.Name]; exists { - panic("rds: parameter catalog entry " + spec.Name + " is duplicated") + return nil, fmt.Errorf("rds: parameter catalog entry %s is duplicated", spec.Name) } out[spec.Name] = spec } - return out + return out, nil +} + +func validateParameterCatalogs() error { + if postgresParameterCatalogErr != nil { + return fmt.Errorf("rds: validate PostgreSQL parameter catalog: %w", postgresParameterCatalogErr) + } + if mariadbParameterCatalogErr != nil { + return fmt.Errorf("rds: validate MariaDB parameter catalog: %w", mariadbParameterCatalogErr) + } + return nil } // The catalog entry for a parameter name, or false when the engine has no such diff --git a/spinifex/handlers/rds/paramcatalog_test.go b/spinifex/handlers/rds/paramcatalog_test.go index a1047e211..f3bdcda5e 100644 --- a/spinifex/handlers/rds/paramcatalog_test.go +++ b/spinifex/handlers/rds/paramcatalog_test.go @@ -15,6 +15,66 @@ import ( // customer can reach it. var catalogEngines = []Engine{enginePostgres, engineMariaDB} +func TestBuildParameterCatalog_ReturnsValidationErrors(t *testing.T) { + computedDefault := func(int64) string { return "1" } + classCeiling := func(int64) int64 { return 2 } + tests := []struct { + name string + specs []ParameterSpec + wantErr string + }{ + { + name: "valid entry", + specs: []ParameterSpec{{Name: "valid", Default: "1"}}, + }, + { + name: "unnamed entry", + specs: []ParameterSpec{{Default: "1"}}, + wantErr: "holds an unnamed entry", + }, + { + name: "missing default", + specs: []ParameterSpec{{Name: "missing"}}, + wantErr: "has no default", + }, + { + name: "literal and computed defaults", + specs: []ParameterSpec{{ + Name: "both", Default: "1", DefaultFor: computedDefault, MaxFor: classCeiling, + }}, + wantErr: "has both a literal and a computed default", + }, + { + name: "computed default without ceiling", + specs: []ParameterSpec{{ + Name: "unpaired", DefaultFor: computedDefault, + }}, + wantErr: "must pair its computed default and class ceiling", + }, + { + name: "duplicate entry", + specs: []ParameterSpec{ + {Name: "duplicate", Default: "1"}, + {Name: "duplicate", Default: "2"}, + }, + wantErr: "is duplicated", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + catalog, err := buildParameterCatalog(tt.specs...) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + assert.Nil(t, catalog) + return + } + require.NoError(t, err) + assert.Len(t, catalog, len(tt.specs)) + }) + } +} + // The failure this guards is the one the phase opens by warning about, arriving // through the default path rather than a customer typo: a catalog change that // makes the smallest class unbootable has to fail here rather than in a create.