diff --git a/cmd/rds-agent/agent.go b/cmd/rds-agent/agent.go index ba1de3964..77aee2f5e 100644 --- a/cmd/rds-agent/agent.go +++ b/cmd/rds-agent/agent.go @@ -155,8 +155,9 @@ type Agent struct { engine *postgresEngine handoffWriter func(string, *handlers_rds.GetDBBootstrapConfigOutput) error - hb *heartbeater - cmd *commander + hb *heartbeater + cmd *commander + guard *paramGuard } // Assembles from already-built parts, so tests can pass fakes; New builds the @@ -171,6 +172,7 @@ func newAgent(cfg config, cp controlPlane, probe *engineProbe) *Agent { a.engine = newPostgresEngine(cfg, execCommandRunner) a.hb = newHeartbeater(cp, probe, handlers_rds.HeartbeatInterval) a.cmd = newCommander(cp, newCommandRegistry(a.engine, newGuestStorage(cfg, execCommandRunner)), cfg.PollWait) + a.guard = newParamGuard(a.engine, probe) return a } @@ -204,6 +206,10 @@ func (a *Agent) Run(ctx context.Context) error { // Directives are only meaningful against a bootstrapped engine. go a.cmd.Run(ctx) + // Started after the handoff, so the window it measures covers the engine's + // own start rather than the time rds-init spent waiting for this agent. + go a.guard.Run(ctx) + <-ctx.Done() return nil } diff --git a/cmd/rds-agent/engine.go b/cmd/rds-agent/engine.go index fd00c51aa..426bf23ab 100644 --- a/cmd/rds-agent/engine.go +++ b/cmd/rds-agent/engine.go @@ -3,6 +3,7 @@ package main import ( "bytes" "context" + "errors" "fmt" "os" "os/exec" @@ -31,6 +32,15 @@ type engineOps interface { Stop(ctx context.Context) error } +// The rollback half of the parameter path, kept off engineOps because it is not +// a control-plane directive: nothing issues it, the agent runs it when the +// engine fails to come up after a parameter change. +type parameterRecovery interface { + // Reports whether the installed set differed and was replaced. + RestoreLastKnownGoodParameters() (bool, error) + Restart(ctx context.Context) error +} + // One child process. Env replaces the agent's own environment rather than // extending it, so a secret placed here reaches only the process that needs it. type command struct { @@ -146,14 +156,42 @@ ALTER ROLE :"master" WITH LOGIN PASSWORD :'password'; return nil } -// The parameters go beside the data rather than into /etc, matching rds-init: a -// class change boots a fresh root volume, which would otherwise revert them. +// The include the resolved parameter 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, which would +// otherwise revert them (D9). +const ( + parametersFileName = "10-rds-parameters.conf" + // Deliberately not a .conf name: include_dir globs *.conf, so the rollback + // copy must not be read as a second set of settings. + lastGoodFileName = "10-rds-parameters.last-good" +) + +// Installs the resolved set, validates it with the engine's own config parser +// before the engine ever adopts it, and reloads. A value the engine refuses is +// rolled back here rather than left on the data volume, where it would survive +// every VM replace and turn the next restart into a boot loop. func (e *postgresEngine) ApplyParameters(ctx context.Context, params []handlers_rds.Parameter) ([]string, error) { - if err := e.installParameters(params); err != nil { + // The check has to run against the file in place, because the engine parses + // the datadir's own include_dir. The window that leaves is closed from both + // ends: the rollback below, and the last-known-good restore the agent runs at + // boot when the engine does not come up. + previous, restore, err := e.installParameters(params) + if err != nil { return nil, err } + if err := e.checkConfig(ctx); err != nil { + return nil, errors.Join(fmt.Errorf("the engine rejected the parameter set: %w", err), restore()) + } if _, err := e.psqlRun(ctx, "SELECT pg_reload_conf();\n"); err != nil { - return nil, fmt.Errorf("reload the engine configuration: %w", err) + return nil, errors.Join(fmt.Errorf("reload the engine configuration: %w", err), restore()) + } + + // Recorded only once the engine has both parsed and adopted it, so the + // rollback target is a set that is known to work rather than the last one + // written. + if err := e.recordLastKnownGood(previous); err != nil { + return nil, err } // Read after the reload: a setting only becomes pending_restart once the @@ -171,14 +209,85 @@ func (e *postgresEngine) ApplyParameters(ctx context.Context, params []handlers_ return pending, nil } +func (e *postgresEngine) parametersPath() string { + return filepath.Join(e.pgData, "conf.d", parametersFileName) +} + +func (e *postgresEngine) lastGoodPath() string { + return filepath.Join(e.pgData, "conf.d", lastGoodFileName) +} + // Written as root and handed to the engine's user, at the same path and mode // rds-init installs it at, so a later boot overwrites rather than shadows it. -func (e *postgresEngine) installParameters(params []handlers_rds.Parameter) error { - dir := filepath.Join(e.pgData, "conf.d") - path := filepath.Join(dir, "10-rds-parameters.conf") - tmp := path + ".new" +// Returns the content that was there and a func putting it back, so the caller +// can undo the install without re-rendering anything. +func (e *postgresEngine) installParameters(params []handlers_rds.Parameter) (previous []byte, restore func() error, err error) { + path := e.parametersPath() + // A missing file is the first-ever apply, and its rollback is a removal. + previous, readErr := os.ReadFile(path) + if readErr != nil && !os.IsNotExist(readErr) { + return nil, nil, fmt.Errorf("read the installed parameters at %s: %w", path, readErr) + } + existed := readErr == nil - if err := os.WriteFile(tmp, []byte(renderParameters(params)), 0o600); err != nil { + if err := e.writeEngineFile(path, []byte(renderParameters(params))); err != nil { + return nil, nil, err + } + return previous, func() error { + if !existed { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("withdraw the rejected parameters at %s: %w", path, err) + } + return nil + } + return e.writeEngineFile(path, previous) + }, nil +} + +// Keeps the rollback target in step with what the engine is actually running. +// The first apply of an instance's life has no predecessor, so the freshly +// accepted set becomes the target instead. +func (e *postgresEngine) recordLastKnownGood(previous []byte) error { + content := previous + if content == nil { + var err error + if content, err = os.ReadFile(e.parametersPath()); err != nil { + return fmt.Errorf("read the accepted parameters: %w", err) + } + } + return e.writeEngineFile(e.lastGoodPath(), content) +} + +// Puts the last set the engine accepted back in place, for a restart that failed +// after a parameter change: the instance comes back on the old parameters rather +// than boot-looping into recovery. Reports whether anything was restored. +func (e *postgresEngine) RestoreLastKnownGoodParameters() (bool, error) { + lastGood, err := os.ReadFile(e.lastGoodPath()) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("read the last known good parameters: %w", err) + } + current, err := os.ReadFile(e.parametersPath()) + if err != nil && !os.IsNotExist(err) { + return false, fmt.Errorf("read the installed parameters: %w", err) + } + if bytes.Equal(current, lastGood) { + return false, nil + } + if err := e.writeEngineFile(e.parametersPath(), lastGood); err != nil { + return false, err + } + return true, nil +} + +// Atomic: a temp file in the same directory, renamed over the target. The temp +// name deliberately does not end in .conf, so a crash between write and rename +// cannot leave the engine reading a half-written include. +func (e *postgresEngine) writeEngineFile(path string, content []byte) error { + tmp := path + ".new" + if err := os.WriteFile(tmp, content, 0o600); err != nil { return fmt.Errorf("write %s: %w", tmp, err) } if err := e.chownToEngine(tmp); err != nil { @@ -192,6 +301,22 @@ func (e *postgresEngine) installParameters(params []handlers_rds.Parameter) erro return nil } +// The engine's own parser, run offline against the datadir. Reading one setting +// back is enough: postgres parses postgresql.conf and every include first, and +// exits non-zero naming the file and line of an unknown parameter or a value +// outside its range. +func (e *postgresEngine) checkConfig(ctx context.Context) error { + if _, err := e.run(ctx, command{ + Name: filepath.Join(filepath.Dir(e.psql), "postgres"), + Args: []string{"-D", e.pgData, "-C", "shared_buffers"}, + Env: []string{"PATH=" + defaultGuestPath}, + User: e.osUser, + }); err != nil { + return err + } + return nil +} + // The engine reads its own config, so a root-owned file would be unreadable to // it. A guest with no such user is a broken image, not something to work around. func (e *postgresEngine) chownToEngine(path string) error { @@ -208,12 +333,22 @@ func (e *postgresEngine) chownToEngine(path string) error { // Through the service manager rather than pg_ctl, so the supervisor records the // engine as stopped and does not restart it underneath a VM that is going down. func (e *postgresEngine) Stop(ctx context.Context) error { + return e.serviceAction(ctx, "stop") +} + +// Only the parameter rollback calls this: a restart the control plane wants goes +// through RebootDBInstance, which cycles the VM. +func (e *postgresEngine) Restart(ctx context.Context) error { + return e.serviceAction(ctx, "restart") +} + +func (e *postgresEngine) serviceAction(ctx context.Context, action string) error { if _, err := e.run(ctx, command{ Name: e.rcService, - Args: []string{e.service, "stop"}, + Args: []string{e.service, action}, Env: []string{"PATH=" + defaultGuestPath}, }); err != nil { - return fmt.Errorf("stop the %s service: %w", e.service, err) + return fmt.Errorf("%s the %s service: %w", action, e.service, err) } return nil } diff --git a/cmd/rds-agent/engine_test.go b/cmd/rds-agent/engine_test.go index 5f9c75994..ec1aabd77 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -220,14 +220,19 @@ func TestPostgresEngine_ApplyParametersInstallsAndReloads(t *testing.T) { if !strings.Contains(string(installed), "shared_buffers = '256MB'") { t.Errorf("installed parameters = %q, want the resolved value", installed) } - if len(runner.calls) != 2 { - t.Fatalf("ran %d commands, want a reload and a pending-restart read", len(runner.calls)) + if len(runner.calls) != 3 { + t.Fatalf("ran %d commands, want a config check, a reload and a pending-restart read", len(runner.calls)) } - if !strings.Contains(runner.calls[0].Stdin, "pg_reload_conf") { - t.Errorf("first statement = %q, want the reload", runner.calls[0].Stdin) + // The check runs before the reload, so a value the engine would refuse never + // reaches a running cluster. + if !slices.Contains(runner.calls[0].Args, "-C") { + t.Errorf("first command = %v, want the offline config check", runner.calls[0].Args) } - if !strings.Contains(runner.calls[1].Stdin, "pending_restart") { - t.Errorf("second statement = %q, want the pending-restart read", runner.calls[1].Stdin) + if !strings.Contains(runner.calls[1].Stdin, "pg_reload_conf") { + t.Errorf("second statement = %q, want the reload", runner.calls[1].Stdin) + } + if !strings.Contains(runner.calls[2].Stdin, "pending_restart") { + t.Errorf("third statement = %q, want the pending-restart read", runner.calls[2].Stdin) } if len(pending) != 2 || pending[0] != "shared_buffers" || pending[1] != "max_connections" { t.Errorf("pending = %v, want both settings the engine reported", pending) diff --git a/cmd/rds-agent/health.go b/cmd/rds-agent/health.go index f7db6ce55..f263124a8 100644 --- a/cmd/rds-agent/health.go +++ b/cmd/rds-agent/health.go @@ -51,25 +51,52 @@ func (p *engineProbe) setPort(port int) { p.port.Store(int64(port)) } +// What the engine is doing, before the seenHealthy latch collapses "never up" +// and "was up and went away" into one health. The parameter rollback needs them +// apart: a postmaster that is up and replaying WAL will come back on its own, one +// that is not running at all after a parameter change will not. +type engineState int + +const ( + // Not answering at all, or the probe could not run. + engineAbsent engineState = iota + // The postmaster is up and rejecting connections: startup or crash recovery. + engineRecovering + engineServing +) + // Only the heartbeat calls this, so the seenHealthy latch is single-goroutine // state. func (p *engineProbe) Check(ctx context.Context) (handlers_rds.EngineHealth, string) { + state, message := p.state(ctx) + switch state { + case engineServing: + p.seenHealthy = true + return handlers_rds.EngineHealthHealthy, "" + case engineRecovering: + return handlers_rds.EngineHealthStarting, message + default: + return p.degraded(), message + } +} + +// The raw probe result. Safe to call from a goroutine other than the +// heartbeat's: it touches no latched state. +func (p *engineProbe) state(ctx context.Context) (engineState, string) { port := strconv.FormatInt(p.port.Load(), 10) code, err := p.run(ctx, p.binary, "-h", p.host, "-p", port, "-q") switch { case err != nil: // A missing binary or broken image. Reporting healthy on the strength of - // nothing would hide it, so this degrades like an engine that did not answer. - return p.degraded(), fmt.Sprintf("engine probe could not run: %v", err) + // nothing would hide it, so this reads as absent like an engine that did + // not answer. + return engineAbsent, fmt.Sprintf("engine probe could not run: %v", err) case code == 0: - p.seenHealthy = true - return handlers_rds.EngineHealthHealthy, "" + return engineServing, "" case code == 1: - // pg_isready's "rejecting connections": the postmaster is up but in - // startup or crash recovery, which resolves on its own. - return handlers_rds.EngineHealthStarting, "engine is rejecting connections (startup or recovery)" + return engineRecovering, "engine is rejecting connections (startup or recovery)" default: - return p.degraded(), fmt.Sprintf("engine did not respond on %s:%s", p.host, port) + return engineAbsent, fmt.Sprintf("engine did not respond on %s:%s", p.host, port) } } diff --git a/cmd/rds-agent/paramrecovery.go b/cmd/rds-agent/paramrecovery.go new file mode 100644 index 000000000..f858373c9 --- /dev/null +++ b/cmd/rds-agent/paramrecovery.go @@ -0,0 +1,93 @@ +package main + +import ( + "context" + "log/slog" + "time" +) + +// How long the engine is given to come up after a boot before the installed +// parameter set is treated as the reason it has not. Generous, because an +// ordinary cold start also runs crash recovery, and a rollback that fired +// against a cluster still replaying WAL would restart it for nothing. +const parameterRollbackAfter = 5 * time.Minute + +// How often the wait re-checks. The engine reports through the same probe the +// heartbeat uses, so this costs a pg_isready per tick and nothing else. +const parameterRollbackPoll = 10 * time.Second + +// The boot-time half of the parameter safety net. A static parameter only takes +// effect at a restart, so a value the engine's config check accepted can still +// stop it from starting — a shared_buffers the host cannot allocate is the usual +// shape. When that happens the bad config is on the persistent data volume, so +// every replace and every recovery boot fails the same way. +// +// This breaks that loop: once the engine has failed to come up for long enough, +// the last set it accepted is put back and the engine is restarted. The failure +// is reported rather than swallowed, so the instance comes back on the old +// parameters and the control plane still sees that the change did not land. +type paramGuard struct { + engine parameterRecovery + probe *engineProbe + after time.Duration + poll time.Duration +} + +func newParamGuard(engine parameterRecovery, probe *engineProbe) *paramGuard { + return ¶mGuard{engine: engine, probe: probe, after: parameterRollbackAfter, poll: parameterRollbackPoll} +} + +// Runs once per agent lifetime: a rollback that did not bring the engine back +// means the parameters were not the cause, and repeating it would only churn a +// cluster that is failing for another reason. +func (g *paramGuard) Run(ctx context.Context) { + if !g.waitForEngineDown(ctx) { + return + } + + restored, err := g.engine.RestoreLastKnownGoodParameters() + if err != nil { + slog.Error("rds-agent: could not restore the last known good parameters", "err", err) + return + } + if !restored { + // The engine is already running the set it last accepted, so whatever is + // keeping it down is not the parameters. + slog.Warn("rds-agent: engine is down but its parameters are the last accepted set; not rolling back") + return + } + + slog.Error("rds-agent: engine did not start after a parameter change; rolled back to the last accepted set", + "after", g.after) + if err := g.engine.Restart(ctx); err != nil { + slog.Error("rds-agent: restart after the parameter rollback failed", "err", err) + } +} + +// Reports true once the engine has been continuously absent for the whole +// window, false if it came up or the agent is shutting down. A postmaster that +// is up and replaying WAL resets the clock: it is making progress on its own, and +// restarting it would only throw the recovery away and start it again. +func (g *paramGuard) waitForEngineDown(ctx context.Context) bool { + deadline := time.Now().Add(g.after) + timer := time.NewTimer(g.poll) + defer timer.Stop() + + for { + select { + case <-ctx.Done(): + return false + case <-timer.C: + } + switch state, _ := g.probe.state(ctx); state { + case engineServing: + return false + case engineRecovering: + deadline = time.Now().Add(g.after) + } + if time.Now().After(deadline) { + return true + } + timer.Reset(g.poll) + } +} diff --git a/cmd/rds-agent/paramrecovery_test.go b/cmd/rds-agent/paramrecovery_test.go new file mode 100644 index 000000000..ec6d97971 --- /dev/null +++ b/cmd/rds-agent/paramrecovery_test.go @@ -0,0 +1,230 @@ +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" +) + +// A runner that fails only the offline config check, so the check's own +// rejection is separable from a reload or a psql failure. +type checkFailingRunner struct { + recordingRunner + + checkErr error +} + +func (r *checkFailingRunner) run(ctx context.Context, c command) (string, error) { + if slices.Contains(c.Args, "-C") { + r.calls = append(r.calls, c) + return "", r.checkErr + } + return r.recordingRunner.run(ctx, c) +} + +// The failure this closes: a value the engine refuses would otherwise sit on the +// data volume, where it survives every VM replace and turns the next restart +// into a boot loop. +func TestPostgresEngine_ApplyParametersRollsBackAValueTheEngineRejects(t *testing.T) { + runner := &checkFailingRunner{checkErr: errors.New(`unrecognized configuration parameter "work_men"`)} + engine := newTestEngine(t, runner.run) + + // A first apply the engine accepted, which is what the rejected one has to + // roll back to. + runner.checkErr = nil + if _, err := engine.ApplyParameters(context.Background(), + []handlers_rds.Parameter{{Name: "work_mem", Value: "4096"}}); err != nil { + t.Fatalf("first ApplyParameters: %v", err) + } + runner.checkErr = errors.New(`unrecognized configuration parameter "work_men"`) + + if _, err := engine.ApplyParameters(context.Background(), + []handlers_rds.Parameter{{Name: "work_mem", Value: "999999999"}}); err == nil { + t.Fatal("ApplyParameters succeeded against a config the engine rejected") + } + + installed, err := os.ReadFile(engine.parametersPath()) + if err != nil { + t.Fatalf("read the installed parameters: %v", err) + } + if !strings.Contains(string(installed), "work_mem = '4096'") { + t.Errorf("installed parameters = %q, want the previous accepted set restored", installed) + } +} + +// The first apply of an instance's life has nothing to roll back to, so a +// rejection has to leave no include at all rather than an empty one. +func TestPostgresEngine_ApplyParametersWithdrawsTheFirstRejectedSet(t *testing.T) { + runner := &checkFailingRunner{checkErr: errors.New("configuration file contains errors")} + engine := newTestEngine(t, runner.run) + + if _, err := engine.ApplyParameters(context.Background(), + []handlers_rds.Parameter{{Name: "work_mem", Value: "0"}}); err == nil { + t.Fatal("ApplyParameters succeeded against a config the engine rejected") + } + if _, err := os.Stat(engine.parametersPath()); !os.IsNotExist(err) { + t.Errorf("the rejected include is still installed (stat err = %v)", err) + } +} + +// The rollback target has to be a set the engine actually adopted, so a second +// accepted apply leaves the first one as what a failed restart falls back to. +func TestPostgresEngine_LastKnownGoodTracksTheAcceptedSet(t *testing.T) { + runner := &recordingRunner{} + engine := newTestEngine(t, runner.run) + + for _, value := range []string{"4096", "8192"} { + if _, err := engine.ApplyParameters(context.Background(), + []handlers_rds.Parameter{{Name: "work_mem", Value: value}}); err != nil { + t.Fatalf("ApplyParameters(%s): %v", value, err) + } + } + + lastGood, err := os.ReadFile(engine.lastGoodPath()) + if err != nil { + t.Fatalf("read the last known good parameters: %v", err) + } + if !strings.Contains(string(lastGood), "work_mem = '4096'") { + t.Errorf("last known good = %q, want the set the engine ran before the latest apply", lastGood) + } + + // include_dir globs *.conf, so the rollback copy must not read as a second + // set of settings alongside the live one. + if strings.HasSuffix(engine.lastGoodPath(), ".conf") { + t.Errorf("the rollback copy at %s would be included by the engine", engine.lastGoodPath()) + } +} + +func TestPostgresEngine_RestoreLastKnownGoodParameters(t *testing.T) { + runner := &recordingRunner{} + engine := newTestEngine(t, runner.run) + + // Nothing to restore before anything has ever been applied. + restored, err := engine.RestoreLastKnownGoodParameters() + if err != nil { + t.Fatalf("RestoreLastKnownGoodParameters: %v", err) + } + if restored { + t.Error("restored a set with no last known good on disk") + } + + for _, value := range []string{"4096", "8192"} { + if _, err := engine.ApplyParameters(context.Background(), + []handlers_rds.Parameter{{Name: "work_mem", Value: value}}); err != nil { + t.Fatalf("ApplyParameters(%s): %v", value, err) + } + } + + restored, err = engine.RestoreLastKnownGoodParameters() + if err != nil { + t.Fatalf("RestoreLastKnownGoodParameters: %v", err) + } + if !restored { + t.Fatal("did not restore a set that differs from the last known good") + } + installed, err := os.ReadFile(engine.parametersPath()) + if err != nil { + t.Fatalf("read the installed parameters: %v", err) + } + if !strings.Contains(string(installed), "work_mem = '4096'") { + t.Errorf("installed = %q, want the last accepted set", installed) + } + + // Idempotent: the second call finds them already equal and does nothing, so a + // repeated rollback cannot churn a cluster that is failing for another reason. + restored, err = engine.RestoreLastKnownGoodParameters() + if err != nil { + t.Fatalf("RestoreLastKnownGoodParameters: %v", err) + } + if restored { + t.Error("restored again when the installed set already matched") + } +} + +// Stands in for the engine in the guard's tests: what matters is whether the +// rollback and the restart were reached at all. +type fakeRecovery struct { + restored bool + restoreErr error + restarts int +} + +func (f *fakeRecovery) RestoreLastKnownGoodParameters() (bool, error) { + return f.restored, f.restoreErr +} + +func (f *fakeRecovery) Restart(context.Context) error { + f.restarts++ + return nil +} + +// A probe whose result the test drives directly, standing in for pg_isready. +func stubProbe(t *testing.T, code int, err error) *engineProbe { + t.Helper() + cfg := loadConfig(filepath.Join(t.TempDir(), "absent.env")) + return newEngineProbe(cfg, func(context.Context, string, ...string) (int, error) { + return code, err + }) +} + +func newTestGuard(engine parameterRecovery, probe *engineProbe) *paramGuard { + g := newParamGuard(engine, probe) + // Real budgets are minutes, which is the point of them; the behaviour under + // test is the decision, not the wait. + g.after, g.poll = 20*time.Millisecond, time.Millisecond + return g +} + +// pg_isready exit 2 is an engine that is not there at all, which after a +// parameter change is the boot-loop shape this breaks. +func TestParamGuard_RollsBackAndRestartsWhenTheEngineNeverComesUp(t *testing.T) { + engine := &fakeRecovery{restored: true} + newTestGuard(engine, stubProbe(t, 2, nil)).Run(t.Context()) + + if engine.restarts != 1 { + t.Errorf("restarts = %d, want the engine restarted on the rolled-back set", engine.restarts) + } +} + +func TestParamGuard_DoesNothingWhenTheEngineIsServing(t *testing.T) { + engine := &fakeRecovery{restored: true} + newTestGuard(engine, stubProbe(t, 0, nil)).Run(t.Context()) + + if engine.restarts != 0 { + t.Errorf("restarts = %d, want none against a healthy engine", engine.restarts) + } +} + +// pg_isready exit 1 is a postmaster that is up and replaying WAL. It is making +// progress on its own, and restarting it would throw the recovery away. +func TestParamGuard_LeavesARecoveringEngineAlone(t *testing.T) { + engine := &fakeRecovery{restored: true} + guard := newTestGuard(engine, stubProbe(t, 1, nil)) + // Bounded so the deadline-resetting loop cannot run forever if it stops + // resetting; the assertion is that the rollback was never reached. + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + guard.Run(ctx) + + if engine.restarts != 0 { + t.Errorf("restarts = %d, want none while the engine is recovering", engine.restarts) + } +} + +// The parameters are only the cause if they differ from the set the engine last +// accepted; restarting on an identical set would churn for nothing. +func TestParamGuard_DoesNotRestartWhenTheParametersAreAlreadyTheAcceptedSet(t *testing.T) { + engine := &fakeRecovery{restored: false} + newTestGuard(engine, stubProbe(t, 2, nil)).Run(t.Context()) + + if engine.restarts != 0 { + t.Errorf("restarts = %d, want none when there was nothing to roll back", engine.restarts) + } +} diff --git a/spinifex/awserrors/awserrors.go b/spinifex/awserrors/awserrors.go index 89cb108ee..19833c221 100644 --- a/spinifex/awserrors/awserrors.go +++ b/spinifex/awserrors/awserrors.go @@ -1,6 +1,10 @@ package awserrors -import "strings" +import ( + "errors" + "fmt" + "strings" +) type ErrorMessage struct { HTTPCode int @@ -468,6 +472,13 @@ var ( ErrorDBParameterGroupNotFound = "DBParameterGroupNotFound" ErrorDBInvalidVPCNetworkState = "InvalidVPCNetworkStateFault" + ErrorDBSubnetGroupAlreadyExists = "DBSubnetGroupAlreadyExists" + ErrorDBSubnetGroupInvalidState = "InvalidDBSubnetGroupStateFault" + ErrorDBSubnetGroupDoesNotCoverAZs = "DBSubnetGroupDoesNotCoverEnoughAZs" + ErrorDBSubnetInvalid = "InvalidSubnet" + ErrorDBParameterGroupAlreadyExists = "DBParameterGroupAlreadyExists" + ErrorDBParameterGroupInvalidState = "InvalidDBParameterGroupState" + // ECR-specific error codes. ErrorRepositoryNotFound = "RepositoryNotFoundException" ErrorRepositoryPolicyNotFound = "RepositoryPolicyNotFoundException" @@ -556,6 +567,14 @@ func ResolveErrorCode(err error) (string, bool) { return "", false } +// Errorf returns an error carrying an AWS error code where ResolveErrorCode can +// find it, alongside a message for the logs and the traces. Formatting the code +// into the message with %s instead leaves it unresolvable, so a handler's 400 +// reaches the client as a 500 with its own code stripped. +func Errorf(code, format string, args ...any) error { + return fmt.Errorf(format+": %w", append(args, errors.New(code))...) +} + // ValidErrorCodeFromError resolves err or returns ErrorServerInternal. func ValidErrorCodeFromError(err error) string { if code, ok := ResolveErrorCode(err); ok { @@ -1042,6 +1061,13 @@ var ErrorLookup = map[string]ErrorMessage{ ErrorDBParameterGroupNotFound: {HTTPCode: 404, Message: "DBParameterGroupName does not refer to an existing DB parameter group."}, ErrorDBInvalidVPCNetworkState: {HTTPCode: 400, Message: "The DB subnet group does not cover all Availability Zones after it is created because of changes that were made."}, + ErrorDBSubnetGroupAlreadyExists: {HTTPCode: 400, Message: "The user already has a DB subnet group with the given name."}, + ErrorDBSubnetGroupInvalidState: {HTTPCode: 400, Message: "The DB subnet group is not in a state that allows the requested operation."}, + ErrorDBSubnetGroupDoesNotCoverAZs: {HTTPCode: 400, Message: "The DB subnet group does not cover enough Availability Zones."}, + ErrorDBSubnetInvalid: {HTTPCode: 400, Message: "The requested subnet is not valid, or multiple subnets were requested that are not all in a common VPC."}, + ErrorDBParameterGroupAlreadyExists: {HTTPCode: 400, Message: "The user already has a DB parameter group with the given name."}, + ErrorDBParameterGroupInvalidState: {HTTPCode: 400, Message: "The DB parameter group is in use or is in an invalid state. It cannot be deleted."}, + // ECR error codes ErrorRepositoryNotFound: {HTTPCode: 400, Message: "The repository could not be found. Check the spelling of the specified repository and ensure that you are performing operations on the correct registry."}, ErrorRepositoryPolicyNotFound: {HTTPCode: 400, Message: "The repository and registry do not have an associated repository policy."}, diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 5c7aeee2a..15bcc22e8 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1067,6 +1067,14 @@ func (d *Daemon) subscribeAll() error { natsSub{handlers_rds.SubjectAddTagsToResource, handleNATSRequest(d.rdsService.AddTagsToResource), "spinifex-workers"}, natsSub{handlers_rds.SubjectRemoveTagsFromResource, handleNATSRequest(d.rdsService.RemoveTagsFromResource), "spinifex-workers"}, natsSub{handlers_rds.SubjectListTagsForResource, handleNATSRequest(d.rdsService.ListTagsForResource), "spinifex-workers"}, + natsSub{handlers_rds.SubjectCreateDBSubnetGroup, handleNATSRequest(d.rdsService.CreateDBSubnetGroup), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDescribeDBSubnetGroups, handleNATSRequest(d.rdsService.DescribeDBSubnetGroups), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDeleteDBSubnetGroup, handleNATSRequest(d.rdsService.DeleteDBSubnetGroup), "spinifex-workers"}, + natsSub{handlers_rds.SubjectCreateDBParameterGroup, handleNATSRequest(d.rdsService.CreateDBParameterGroup), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDescribeDBParameterGroups, handleNATSRequest(d.rdsService.DescribeDBParameterGroups), "spinifex-workers"}, + natsSub{handlers_rds.SubjectModifyDBParameterGroup, handleNATSRequest(d.rdsService.ModifyDBParameterGroup), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDescribeDBParameters, handleNATSRequest(d.rdsService.DescribeDBParameters), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDeleteDBParameterGroup, handleNATSRequest(d.rdsService.DeleteDBParameterGroup), "spinifex-workers"}, ) } diff --git a/spinifex/gateway/rds/handler.go b/spinifex/gateway/rds/handler.go index 016748b95..fd8e12624 100644 --- a/spinifex/gateway/rds/handler.go +++ b/spinifex/gateway/rds/handler.go @@ -97,16 +97,16 @@ var actions = map[string]Handler{ "DescribeDBInstanceAutomatedBackups": pending(), // Subnet groups. - "CreateDBSubnetGroup": pending(), - "DescribeDBSubnetGroups": pending(), - "DeleteDBSubnetGroup": pending(), + "CreateDBSubnetGroup": typed(CreateDBSubnetGroup), + "DescribeDBSubnetGroups": typed(DescribeDBSubnetGroups), + "DeleteDBSubnetGroup": typed(DeleteDBSubnetGroup), // Parameter groups. - "CreateDBParameterGroup": pending(), - "DescribeDBParameterGroups": pending(), - "ModifyDBParameterGroup": pending(), - "DescribeDBParameters": pending(), - "DeleteDBParameterGroup": pending(), + "CreateDBParameterGroup": typed(CreateDBParameterGroup), + "DescribeDBParameterGroups": typed(DescribeDBParameterGroups), + "ModifyDBParameterGroup": typed(ModifyDBParameterGroup), + "DescribeDBParameters": typed(DescribeDBParameters), + "DeleteDBParameterGroup": typed(DeleteDBParameterGroup), // Tags. "AddTagsToResource": typed(AddTagsToResource), diff --git a/spinifex/gateway/rds/handler_test.go b/spinifex/gateway/rds/handler_test.go index b2a0bfac3..1ea325528 100644 --- a/spinifex/gateway/rds/handler_test.go +++ b/spinifex/gateway/rds/handler_test.go @@ -110,6 +110,9 @@ var liveActions = []string{ "RebootDBInstance", "StartDBInstance", "StopDBInstance", "DeleteDBInstance", "DescribeEvents", "AddTagsToResource", "RemoveTagsFromResource", "ListTagsForResource", + "CreateDBSubnetGroup", "DescribeDBSubnetGroups", "DeleteDBSubnetGroup", + "CreateDBParameterGroup", "DescribeDBParameterGroups", "ModifyDBParameterGroup", + "DescribeDBParameters", "DeleteDBParameterGroup", } // What is under test in this file is the action table and the XML envelope, not @@ -133,6 +136,14 @@ func newStubbedNATS(t *testing.T) *nats.Conn { &rds.ListTagsForResourceOutput{TagList: []*rds.Tag{ {Key: aws.String("env"), Value: aws.String("prod")}, }}) + respondWith(t, nc, handlers_rds.SubjectCreateDBSubnetGroup, &rds.CreateDBSubnetGroupOutput{}) + respondWith(t, nc, handlers_rds.SubjectDescribeDBSubnetGroups, &rds.DescribeDBSubnetGroupsOutput{}) + respondWith(t, nc, handlers_rds.SubjectDeleteDBSubnetGroup, &rds.DeleteDBSubnetGroupOutput{}) + respondWith(t, nc, handlers_rds.SubjectCreateDBParameterGroup, &rds.CreateDBParameterGroupOutput{}) + respondWith(t, nc, handlers_rds.SubjectDescribeDBParameterGroups, &rds.DescribeDBParameterGroupsOutput{}) + respondWith(t, nc, handlers_rds.SubjectModifyDBParameterGroup, &rds.DBParameterGroupNameMessage{}) + respondWith(t, nc, handlers_rds.SubjectDescribeDBParameters, &rds.DescribeDBParametersOutput{}) + respondWith(t, nc, handlers_rds.SubjectDeleteDBParameterGroup, &rds.DeleteDBParameterGroupOutput{}) return nc } diff --git a/spinifex/gateway/rds/parametergroup.go b/spinifex/gateway/rds/parametergroup.go new file mode 100644 index 000000000..6081f9804 --- /dev/null +++ b/spinifex/gateway/rds/parametergroup.go @@ -0,0 +1,31 @@ +package gateway_rds + +import ( + "context" + + "github.com/aws/aws-sdk-go/service/rds" + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" + "github.com/nats-io/nats.go" +) + +func CreateDBParameterGroup(ctx context.Context, input *rds.CreateDBParameterGroupInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).CreateDBParameterGroup(ctx, input, caller.AccountID) +} + +func DescribeDBParameterGroups(ctx context.Context, input *rds.DescribeDBParameterGroupsInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DescribeDBParameterGroups(ctx, input, caller.AccountID) +} + +// The result is DBParameterGroupNameMessage rather than a group, which is what +// AWS returns and what the Terraform provider reads the applied name from. +func ModifyDBParameterGroup(ctx context.Context, input *rds.ModifyDBParameterGroupInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).ModifyDBParameterGroup(ctx, input, caller.AccountID) +} + +func DescribeDBParameters(ctx context.Context, input *rds.DescribeDBParametersInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DescribeDBParameters(ctx, input, caller.AccountID) +} + +func DeleteDBParameterGroup(ctx context.Context, input *rds.DeleteDBParameterGroupInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DeleteDBParameterGroup(ctx, input, caller.AccountID) +} diff --git a/spinifex/gateway/rds/subnetgroup.go b/spinifex/gateway/rds/subnetgroup.go new file mode 100644 index 000000000..24351c50d --- /dev/null +++ b/spinifex/gateway/rds/subnetgroup.go @@ -0,0 +1,23 @@ +package gateway_rds + +import ( + "context" + + "github.com/aws/aws-sdk-go/service/rds" + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" + "github.com/nats-io/nats.go" +) + +// The nested Subnets list is rendered by the shared XML marshaller off the SDK's +// own struct tags, so nothing here shapes the response. +func CreateDBSubnetGroup(ctx context.Context, input *rds.CreateDBSubnetGroupInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).CreateDBSubnetGroup(ctx, input, caller.AccountID) +} + +func DescribeDBSubnetGroups(ctx context.Context, input *rds.DescribeDBSubnetGroupsInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DescribeDBSubnetGroups(ctx, input, caller.AccountID) +} + +func DeleteDBSubnetGroup(ctx context.Context, input *rds.DeleteDBSubnetGroupInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DeleteDBSubnetGroup(ctx, input, caller.AccountID) +} diff --git a/spinifex/handlers/rds/agent.go b/spinifex/handlers/rds/agent.go index 492e0dccb..ad716fb7a 100644 --- a/spinifex/handlers/rds/agent.go +++ b/spinifex/handlers/rds/agent.go @@ -160,7 +160,8 @@ func (s *Service) SubmitDBStateChange(ctx context.Context, input *SubmitDBStateC return nil, errors.New(awserrors.ErrorInvalidParameterValue) } if !ValidEngineHealth(input.EngineHealth) { - return nil, fmt.Errorf("%s: unknown engine health %q", awserrors.ErrorInvalidParameterValue, input.EngineHealth) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "unknown engine health %q", input.EngineHealth) } persist := s.noteBeat(accountID, input.DBInstanceIdentifier, input.EngineHealth, input.Message, false) diff --git a/spinifex/handlers/rds/arn.go b/spinifex/handlers/rds/arn.go index 0652e88ce..360aa4197 100644 --- a/spinifex/handlers/rds/arn.go +++ b/spinifex/handlers/rds/arn.go @@ -93,5 +93,5 @@ func ParseARN(arn, region, accountID string) (ParsedARN, error) { } func arnError(arn, why string) error { - return fmt.Errorf("%s: %q is not a valid RDS ARN: %s", awserrors.ErrorInvalidParameterValue, arn, why) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%q is not a valid RDS ARN: %s", arn, why) } diff --git a/spinifex/handlers/rds/arn_parse_test.go b/spinifex/handlers/rds/arn_parse_test.go index 8ce5a4074..7ec2a5c73 100644 --- a/spinifex/handlers/rds/arn_parse_test.go +++ b/spinifex/handlers/rds/arn_parse_test.go @@ -66,11 +66,11 @@ func TestParseARN_RejectsMalformedAndForeignARNs(t *testing.T) { // The registry is what makes an unregistered kind a rejection rather than an // empty tag list, so every kind the parser accepts has to be accounted for. -func TestTaggableResources_OnlyDBInstancesAreRegistered(t *testing.T) { - _, ok := taggableResources[ResourceKindDBInstance] - assert.True(t, ok, "DB instances are the resource this phase tags") - for _, kind := range []ResourceKind{ResourceKindDBSnapshot, ResourceKindDBSubnetGroup, ResourceKindDBParameterGroup} { +func TestTaggableResources_CoverEveryResourceThatExists(t *testing.T) { + for _, kind := range []ResourceKind{ResourceKindDBInstance, ResourceKindDBSubnetGroup, ResourceKindDBParameterGroup} { _, ok := taggableResources[kind] - assert.False(t, ok, "%s is registered by the phase that creates it, not this one", kind) + assert.True(t, ok, "%s is a resource that exists and so must be taggable", kind) } + _, ok := taggableResources[ResourceKindDBSnapshot] + assert.False(t, ok, "snapshots are registered by the phase that creates them, not this one") } diff --git a/spinifex/handlers/rds/create.go b/spinifex/handlers/rds/create.go index f06bfa1d0..3115b1242 100644 --- a/spinifex/handlers/rds/create.go +++ b/spinifex/handlers/rds/create.go @@ -28,12 +28,19 @@ func (s *Service) CreateDBInstance(ctx context.Context, input *rds.CreateDBInsta if err != nil { return nil, err } - placement, err := s.resolvePlacement(ctx, accountID, req) + kv, err := s.bucket(ctx, accountID) if err != nil { return nil, err } - - kv, err := s.bucket(ctx, accountID) + placement, err := s.resolvePlacement(ctx, kv, accountID, req) + if err != nil { + return nil, err + } + // Resolved before the identifier is reserved, so a create naming a group that + // does not exist leaves no record behind. The set is literals only: the + // class's memory has already been folded into every size-derived default, so + // the agent never sees a formula (D20). + parameters, err := s.resolveGroupParameters(ctx, kv, accountID, req.DBParameterGroupName, req.InstanceClass) if err != nil { return nil, err } @@ -42,11 +49,11 @@ func (s *Service) CreateDBInstance(ctx context.Context, input *rds.CreateDBInsta // Creating the record before anything is provisioned makes the identifier // uniqueness check atomic against a concurrent create on another node — a // prior read-then-write would let both pass and both launch a VM. - rec := newDBInstanceRecord(accountID, req, placement) + rec := newDBInstanceRecord(accountID, req, placement, parameters) if createErr := createJSON(ctx, kv, key, &rec); createErr != nil { if errors.Is(createErr, jetstream.ErrKeyExists) { - return nil, fmt.Errorf("%s: DB instance %s already exists", - awserrors.ErrorDBInstanceAlreadyExists, req.Identifier) + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceAlreadyExists, + "DB instance %s already exists", req.Identifier) } return nil, createErr } @@ -125,7 +132,7 @@ func (s *Service) CreateDBInstance(ctx context.Context, input *rds.CreateDBInsta // The record as it stands before any resource exists: everything the request // determined, plus the staged master password the first bootstrap fetch consumes. -func newDBInstanceRecord(accountID string, req *validatedCreate, placement *endpointPlacement) DBInstanceRecord { +func newDBInstanceRecord(accountID string, req *validatedCreate, placement *endpointPlacement, parameters []Parameter) DBInstanceRecord { now := time.Now().UTC() return DBInstanceRecord{ DBInstanceIdentifier: req.Identifier, @@ -142,12 +149,16 @@ func newDBInstanceRecord(accountID string, req *validatedCreate, placement *endp SubnetID: placement.SubnetID, VpcID: placement.VpcID, VpcSecurityGroupIDs: placement.SecurityGroupIDs, + DBSubnetGroupName: req.DBSubnetGroupName, DBParameterGroupName: req.DBParameterGroupName, DeletionProtection: req.DeletionProtection, Tags: req.Tags, - Bootstrap: BootstrapState{MasterUserPassword: req.MasterPassword}, - CreatedAt: now, - UpdatedAt: now, + Bootstrap: BootstrapState{ + MasterUserPassword: req.MasterPassword, + ResolvedParameters: parameters, + }, + CreatedAt: now, + UpdatedAt: now, } } diff --git a/spinifex/handlers/rds/create_test.go b/spinifex/handlers/rds/create_test.go index 3dc0e5a13..9ce80e827 100644 --- a/spinifex/handlers/rds/create_test.go +++ b/spinifex/handlers/rds/create_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "slices" "strings" "testing" @@ -23,6 +24,8 @@ const ( testDefaultSG = "sg-default01" testBaseDomain = "spx3.net" testDBInstanceID = "orders-db" + // The one zone this platform exposes, which every subnet reports. + testZone = "spinifexz1" ) // fakeNetwork is the customer account's VPC as the placement resolver reads it: @@ -48,8 +51,8 @@ func newFakeNetwork() *fakeNetwork { // Deliberately out of order: the resolver sorts, so the placement is the // same on every repeat regardless of how the describe happened to order. subnets: []*ec2.Subnet{ - {SubnetId: aws.String("subnet-zebra")}, - {SubnetId: aws.String("subnet-alpha")}, + {SubnetId: aws.String("subnet-zebra"), VpcId: aws.String(testDefaultVPC), AvailabilityZone: aws.String(testZone)}, + {SubnetId: aws.String("subnet-alpha"), VpcId: aws.String(testDefaultVPC), AvailabilityZone: aws.String(testZone)}, }, groups: []*ec2.SecurityGroup{ {GroupId: aws.String("sg-app01"), GroupName: aws.String("app")}, @@ -66,12 +69,25 @@ func (f *fakeNetwork) DescribeVpcs(_ context.Context, _ *ec2.DescribeVpcsInput, return &ec2.DescribeVpcsOutput{Vpcs: f.vpcs}, nil } -func (f *fakeNetwork) DescribeSubnets(_ context.Context, _ *ec2.DescribeSubnetsInput, accountID string) (*ec2.DescribeSubnetsOutput, error) { +// Honours SubnetIds the way EC2 does — a named subnet that is not in this +// account simply does not come back — which is what makes a subnet group's +// existence and ownership checks observable. +func (f *fakeNetwork) DescribeSubnets(_ context.Context, input *ec2.DescribeSubnetsInput, accountID string) (*ec2.DescribeSubnetsOutput, error) { f.accts = append(f.accts, accountID) if f.subErr != nil { return nil, f.subErr } - return &ec2.DescribeSubnetsOutput{Subnets: f.subnets}, nil + if input == nil || len(input.SubnetIds) == 0 { + return &ec2.DescribeSubnetsOutput{Subnets: f.subnets}, nil + } + wanted := aws.StringValueSlice(input.SubnetIds) + var matched []*ec2.Subnet + for _, subnet := range f.subnets { + if slices.Contains(wanted, aws.StringValue(subnet.SubnetId)) { + matched = append(matched, subnet) + } + } + return &ec2.DescribeSubnetsOutput{Subnets: matched}, nil } func (f *fakeNetwork) DescribeSecurityGroups(_ context.Context, _ *ec2.DescribeSecurityGroupsInput, accountID string) (*ec2.DescribeSecurityGroupsOutput, error) { @@ -442,14 +458,6 @@ func TestValidateCreateRequest_RejectsMalformedRequests(t *testing.T) { {"PortTooLow", func(in *rds.CreateDBInstanceInput) { in.Port = aws.Int64(80) }, awserrors.ErrorInvalidParameterValue}, {"ReservedUsername", func(in *rds.CreateDBInstanceInput) { in.MasterUsername = aws.String("rdsadmin") }, awserrors.ErrorInvalidParameterValue}, {"ShortPassword", func(in *rds.CreateDBInstanceInput) { in.MasterUserPassword = aws.String("short") }, awserrors.ErrorInvalidParameterValue}, - // Named groups do not exist until DB subnet groups land, and placing the - // endpoint somewhere else would put it in a subnet nobody chose. - {"NamedSubnetGroup", func(in *rds.CreateDBInstanceInput) { - in.DBSubnetGroupName = aws.String("db-private") - }, awserrors.ErrorDBSubnetGroupNotFound}, - {"NamedParameterGroup", func(in *rds.CreateDBInstanceInput) { - in.DBParameterGroupName = aws.String("tuned-pg") - }, awserrors.ErrorDBParameterGroupNotFound}, } for _, tc := range cases { diff --git a/spinifex/handlers/rds/delete.go b/spinifex/handlers/rds/delete.go index 5f87552c7..7371acfaa 100644 --- a/spinifex/handlers/rds/delete.go +++ b/spinifex/handlers/rds/delete.go @@ -48,8 +48,8 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta return nil, err } if rec.DeletionProtection { - return nil, fmt.Errorf("%s: DB instance %s cannot be deleted because deletion protection is enabled", - awserrors.ErrorInvalidParameterCombination, id) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "DB instance %s cannot be deleted because deletion protection is enabled", id) } if err := s.checkFinalSnapshotAvailable(ctx, kv, rec, finalSnapshot); err != nil { return nil, err @@ -59,8 +59,8 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta // Keeping the first one silently would answer a caller who asked for a final // snapshot with no snapshot, or snapshot for one who asked to skip. if rec.Status == StatusDeleting && finalSnapshot != rec.FinalSnapshotIdentifier { - return nil, fmt.Errorf("%s: DB instance %s is already being deleted with %s; a retry must repeat that choice", - awserrors.ErrorDBInstanceInvalidState, id, snapshotChoice(rec.FinalSnapshotIdentifier)) + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s is already being deleted with %s; a retry must repeat that choice", id, snapshotChoice(rec.FinalSnapshotIdentifier)) } // A repeat of a delete already under way re-runs the teardown rather than @@ -74,8 +74,8 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta rec.UpdatedAt = now if err := updateJSON(ctx, kv, DBInstanceKey(id), rev, rec); err != nil { if errors.Is(err, jetstream.ErrKeyExists) { - return nil, fmt.Errorf("%s: DB instance %s changed state concurrently; retry the delete", - awserrors.ErrorDBInstanceInvalidState, id) + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s changed state concurrently; retry the delete", id) } return nil, err } @@ -95,21 +95,21 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta // would otherwise silently destroy the only copy of the data. func validateDeleteRequest(input *rds.DeleteDBInstanceInput) (string, error) { if input == nil { - return "", fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } if aws.StringValue(input.DBInstanceIdentifier) == "" { - return "", fmt.Errorf("%s: DBInstanceIdentifier is required", awserrors.ErrorInvalidParameterValue) + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier is required") } skip := aws.BoolValue(input.SkipFinalSnapshot) identifier := aws.StringValue(input.FinalDBSnapshotIdentifier) switch { case skip && identifier != "": - return "", fmt.Errorf("%s: FinalDBSnapshotIdentifier cannot be supplied with SkipFinalSnapshot", - awserrors.ErrorInvalidParameterCombination) + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "FinalDBSnapshotIdentifier cannot be supplied with SkipFinalSnapshot") case !skip && identifier == "": - return "", fmt.Errorf("%s: FinalDBSnapshotIdentifier is required unless SkipFinalSnapshot is set", - awserrors.ErrorInvalidParameterCombination) + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "FinalDBSnapshotIdentifier is required unless SkipFinalSnapshot is set") } if identifier != "" { if err := validateDBInstanceIdentifier(identifier); err != nil { @@ -152,7 +152,7 @@ func (s *Service) checkFinalSnapshotAvailable(ctx context.Context, kv jetstream. if finalSnapshotIsOurs(&existing, rec) { return nil } - return fmt.Errorf("%s: DB snapshot %s already exists", awserrors.ErrorDBSnapshotAlreadyExists, identifier) + return awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, "DB snapshot %s already exists", identifier) } // The teardown chain. Ordering is forced by what holds what: the VM has to go @@ -231,8 +231,8 @@ func (s *Service) takeFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, } if found { if !finalSnapshotIsOurs(&existing, rec) { - return fmt.Errorf("%s: DB snapshot %s already exists", - awserrors.ErrorDBSnapshotAlreadyExists, rec.FinalSnapshotIdentifier) + return awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, + "DB snapshot %s already exists", rec.FinalSnapshotIdentifier) } return nil } diff --git a/spinifex/handlers/rds/describe.go b/spinifex/handlers/rds/describe.go index 965acdfca..f46ca2029 100644 --- a/spinifex/handlers/rds/describe.go +++ b/spinifex/handlers/rds/describe.go @@ -94,8 +94,18 @@ func (s *Service) projectDBInstance(rec *DBInstanceRecord) *rds.DBInstance { if rec.DBName != "" { out.DBName = aws.String(rec.DBName) } - if rec.VpcID != "" { - out.DBSubnetGroup = &rds.DBSubnetGroup{VpcId: aws.String(rec.VpcID)} + // The Terraform provider reads db_subnet_group_name off the describe, so an + // instance placed from a named group has to report it: an empty read-back is a + // perpetual diff on an attribute ModifyDBInstance then refuses to change. + if rec.VpcID != "" || rec.DBSubnetGroupName != "" { + out.DBSubnetGroup = &rds.DBSubnetGroup{} + if rec.VpcID != "" { + out.DBSubnetGroup.VpcId = aws.String(rec.VpcID) + } + if rec.DBSubnetGroupName != "" { + out.DBSubnetGroup.DBSubnetGroupName = aws.String(rec.DBSubnetGroupName) + out.DBSubnetGroup.SubnetGroupStatus = aws.String(subnetGroupStatusComplete) + } } for _, groupID := range rec.VpcSecurityGroupIDs { out.VpcSecurityGroups = append(out.VpcSecurityGroups, &rds.VpcSecurityGroupMembership{ diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index e6a7e2e83..3f02e40f3 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -2,7 +2,6 @@ package handlers_rds import ( "errors" - "fmt" "maps" "slices" "strings" @@ -36,18 +35,25 @@ func (e Engine) EngineVersion() string { return e.MajorVersion } -// The parameter-group name AWS clients expect when none is named. rds-7 -// materialises the group itself; here it is only the name a request may carry. +// The parameter-group name AWS clients expect when none is named. The group is +// implicit: it is resolvable and reportable without ever having been created, +// and is neither modifiable nor deletable. func (e Engine) DefaultParameterGroupName() string { - return "default." + e.Name + e.MajorVersion + return defaultParameterGroupPrefix + e.ParameterGroupFamily() +} + +// The family every parameter group of this engine belongs to. v1 pins one major +// per engine, so a family is a name rather than a version axis. +func (e Engine) ParameterGroupFamily() string { + return e.Name + e.MajorVersion } // An unknown engine is rejected at validation, before any volume or ENI exists. func LookupEngine(name string) (Engine, error) { engine, ok := engines[strings.ToLower(strings.TrimSpace(name))] if !ok { - return Engine{}, fmt.Errorf("%s: engine %q is not supported; supported engines are %s", - awserrors.ErrorInvalidParameterValue, name, strings.Join(SupportedEngines(), ", ")) + return Engine{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "engine %q is not supported; supported engines are %s", name, strings.Join(SupportedEngines(), ", ")) } return engine, nil } @@ -67,38 +73,38 @@ func (e Engine) ValidateVersion(version string) error { if major, _, ok := strings.Cut(v, "."); ok && major == e.MajorVersion { return nil } - return fmt.Errorf("%s: EngineVersion %q is not available; %s %s is the only supported version", - awserrors.ErrorInvalidParameterValue, version, e.Name, e.MajorVersion) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "EngineVersion %q is not available; %s %s is the only supported version", version, e.Name, e.MajorVersion) } // Mirrors the engine's own rules rather than a generic identifier check, so a // name the control plane accepts cannot fail at initdb time inside the guest. func (e Engine) ValidateMasterUsername(username string) error { if username == "" { - return fmt.Errorf("%s: MasterUsername is required", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUsername is required") } if len(username) > maxMasterUsernameLen { - return fmt.Errorf("%s: MasterUsername must be at most %d characters", - awserrors.ErrorInvalidParameterValue, maxMasterUsernameLen) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUsername must be at most %d characters", maxMasterUsernameLen) } if !isLetter(rune(username[0])) { - return fmt.Errorf("%s: MasterUsername must begin with a letter", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUsername must begin with a letter") } for _, r := range username { if !isLetter(r) && !isDigit(r) && r != '_' { - return fmt.Errorf("%s: MasterUsername may contain only letters, digits and underscores", - awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUsername may contain only letters, digits and underscores") } } lower := strings.ToLower(username) if slices.Contains(e.reservedUsernames, lower) { - return fmt.Errorf("%s: MasterUsername %q is reserved by %s", - awserrors.ErrorInvalidParameterValue, username, e.Name) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUsername %q is reserved by %s", username, e.Name) } for _, prefix := range e.reservedUsernamePrefixes { if strings.HasPrefix(lower, prefix) { - return fmt.Errorf("%s: MasterUsername may not begin with %q, which %s reserves", - awserrors.ErrorInvalidParameterValue, prefix, e.Name) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUsername may not begin with %q, which %s reserves", prefix, e.Name) } } return nil @@ -111,13 +117,13 @@ func ValidateMasterUserPassword(password string) error { case password == "": return errors.New(awserrors.ErrorInvalidParameterValue + ": MasterUserPassword is required") case len(password) < minMasterPasswordLen || len(password) > maxMasterPasswordLen: - return fmt.Errorf("%s: MasterUserPassword must be between %d and %d characters", - awserrors.ErrorInvalidParameterValue, minMasterPasswordLen, maxMasterPasswordLen) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword must be between %d and %d characters", minMasterPasswordLen, maxMasterPasswordLen) } for _, r := range password { if r == '/' || r == '"' || r == '@' || r == ' ' { - return fmt.Errorf("%s: MasterUserPassword may not contain '/', '\"', '@' or spaces", - awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword may not contain '/', '\"', '@' or spaces") } } return nil diff --git a/spinifex/handlers/rds/events.go b/spinifex/handlers/rds/events.go index ed81d2e9a..5e79015fc 100644 --- a/spinifex/handlers/rds/events.go +++ b/spinifex/handlers/rds/events.go @@ -163,13 +163,14 @@ func (s *Service) DescribeEvents(ctx context.Context, input *rds.DescribeEventsI sourceType := aws.StringValue(input.SourceType) sourceIdentifier := aws.StringValue(input.SourceIdentifier) if sourceType != "" && !validEventSourceType(sourceType) { - return nil, fmt.Errorf("%s: SourceType %q is not an RDS event source", awserrors.ErrorInvalidParameterValue, sourceType) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "SourceType %q is not an RDS event source", sourceType) } // AWS's own rule: an identifier is meaningless without the type, since two // resource kinds may share a name. if sourceIdentifier != "" && sourceType == "" { - return nil, fmt.Errorf("%s: SourceType is required when SourceIdentifier is supplied", - awserrors.ErrorInvalidParameterCombination) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "SourceType is required when SourceIdentifier is supplied") } kv, err := s.bucket(ctx, accountID) @@ -259,7 +260,7 @@ func eventWindow(input *rds.DescribeEventsInput) (timeWindow, error) { window.end = input.EndTime.UTC() } if window.end.Before(window.start) { - return timeWindow{}, fmt.Errorf("%s: EndTime is before StartTime", awserrors.ErrorInvalidParameterCombination) + return timeWindow{}, awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, "EndTime is before StartTime") } return window, nil } diff --git a/spinifex/handlers/rds/lifecycle.go b/spinifex/handlers/rds/lifecycle.go index 296498840..0ec277efb 100644 --- a/spinifex/handlers/rds/lifecycle.go +++ b/spinifex/handlers/rds/lifecycle.go @@ -47,7 +47,7 @@ const instanceCommandTimeout = 90 * time.Second // to, and silently ignoring it would report a failover that never happened. func (s *Service) RebootDBInstance(ctx context.Context, input *rds.RebootDBInstanceInput, accountID string) (*rds.RebootDBInstanceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } if aws.BoolValue(input.ForceFailover) { return nil, unimplemented("ForceFailover", "this platform is single-AZ; there is no standby to fail over to") @@ -93,7 +93,7 @@ func (s *Service) RebootDBInstance(ctx context.Context, input *rds.RebootDBInsta // datadir at the same address (D5/D9). func (s *Service) StopDBInstance(ctx context.Context, input *rds.StopDBInstanceInput, accountID string) (*rds.StopDBInstanceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } // A pre-stop snapshot is rds-8's; accepting it here would report a snapshot // the customer would then not find. @@ -128,7 +128,7 @@ func (s *Service) StopDBInstance(ctx context.Context, input *rds.StopDBInstanceI // The engine replays WAL when the graceful stop did not complete. func (s *Service) StartDBInstance(ctx context.Context, input *rds.StartDBInstanceInput, accountID string) (*rds.StartDBInstanceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } rec, kv, err := s.beginTransition(ctx, accountID, aws.StringValue(input.DBInstanceIdentifier), @@ -213,7 +213,7 @@ func (s *Service) stopEngineOrRecordFallback(ctx context.Context, accountID stri // the stop already running. func (s *Service) beginTransition(ctx context.Context, accountID, id string, to Status, from ...Status) (*DBInstanceRecord, jetstream.KeyValue, error) { if id == "" { - return nil, nil, fmt.Errorf("%s: DBInstanceIdentifier is required", awserrors.ErrorInvalidParameterValue) + return nil, nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier is required") } if s.deps.Instances == nil { return nil, nil, errors.New("rds: no instance command path configured") @@ -227,11 +227,12 @@ func (s *Service) beginTransition(ctx context.Context, accountID, id string, to return nil, nil, err } if !slices.Contains(from, rec.Status) { - return nil, nil, fmt.Errorf("%s: DB instance %s is %s; the operation requires it to be %s", - awserrors.ErrorDBInstanceInvalidState, id, rec.Status, joinStatuses(from)) + return nil, nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s is %s; the operation requires it to be %s", id, rec.Status, joinStatuses(from)) } if rec.InstanceID == "" { - return nil, nil, fmt.Errorf("%s: DB instance %s has no VM to act on", awserrors.ErrorDBInstanceInvalidState, id) + return nil, nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s has no VM to act on", id) } now := time.Now().UTC() @@ -245,8 +246,8 @@ func (s *Service) beginTransition(ctx context.Context, accountID, id string, to rec.UpdatedAt = now if err := updateJSON(ctx, kv, DBInstanceKey(id), rev, rec); err != nil { if errors.Is(err, jetstream.ErrKeyExists) { - return nil, nil, fmt.Errorf("%s: DB instance %s changed state concurrently; retry the operation", - awserrors.ErrorDBInstanceInvalidState, id) + return nil, nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s changed state concurrently; retry the operation", id) } return nil, nil, err } @@ -280,7 +281,7 @@ func (s *Service) failTransition(ctx context.Context, kv jetstream.KeyValue, acc "dbInstance", rec.DBInstanceIdentifier, "err", err) } s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, reason, EventCategoryFailure) - return fmt.Errorf("%s: %s", awserrors.ErrorDBInstanceInvalidState, reason) + return awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, "%s", reason) } // A read-modify-write under CAS, replayed on contention so a concurrent agent diff --git a/spinifex/handlers/rds/modify.go b/spinifex/handlers/rds/modify.go index 273749d2c..6f78593dc 100644 --- a/spinifex/handlers/rds/modify.go +++ b/spinifex/handlers/rds/modify.go @@ -74,11 +74,11 @@ func (p *modifyPlan) empty() bool { // address, and the DNS A-record are untouched by a grow and by a class change. func (s *Service) ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInstanceInput, accountID string) (*rds.ModifyDBInstanceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } id := aws.StringValue(input.DBInstanceIdentifier) if id == "" { - return nil, fmt.Errorf("%s: DBInstanceIdentifier is required", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier is required") } if err := rejectUnimplementedModify(input); err != nil { return nil, err @@ -107,8 +107,8 @@ func (s *Service) ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInsta // non-disruptive settings are record and ENI writes, legal from any settled // state. if plan.disruptive() && rec.Status != StatusAvailable && rec.Status != StatusFailed { - return nil, fmt.Errorf("%s: DB instance %s is %s; the requested modification requires it to be %s", - awserrors.ErrorDBInstanceInvalidState, id, rec.Status, StatusAvailable) + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s is %s; the requested modification requires it to be %s", id, rec.Status, StatusAvailable) } if err := s.applyImmediateModify(ctx, kv, accountID, rec, plan); err != nil { @@ -196,21 +196,21 @@ func (s *Service) planModify(ctx context.Context, input *rds.ModifyDBInstanceInp if class := aws.StringValue(input.DBInstanceClass); class != "" && class != rec.DBInstanceClass { instanceType, err := InstanceTypeForClass(class) if err != nil { - return nil, fmt.Errorf("%s: DBInstanceClass %q is not supported; supported classes are %s", - awserrors.ErrorInvalidParameterValue, class, strings.Join(SupportedInstanceClasses(), ", ")) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceClass %q is not supported; supported classes are %s", class, strings.Join(SupportedInstanceClasses(), ", ")) } plan.InstanceClass, plan.InstanceType = class, instanceType } - // The implicit default group is the only name that resolves until rds-7 - // materialises real ones, so anything else is a group that does not exist. + // Resolved against KV here rather than at apply time, so a group that does + // not exist is rejected before the instance is moved into modifying. if group := aws.StringValue(input.DBParameterGroupName); group != "" && group != rec.DBParameterGroupName { - engine, err := LookupEngine(rec.Engine) + kv, err := s.bucket(ctx, accountID) if err != nil { return nil, err } - if group != engine.DefaultParameterGroupName() { - return nil, fmt.Errorf("%s: DB parameter group %q not found", awserrors.ErrorDBParameterGroupNotFound, group) + if _, _, err := getDBParameterGroup(ctx, kv, accountID, group); err != nil { + return nil, err } plan.ParameterGroup = group } @@ -241,8 +241,8 @@ func (s *Service) planSecurityGroups(ctx context.Context, accountID string, rec return nil, nil } if rec.ENIID == "" { - return nil, fmt.Errorf("%s: DB instance %s has no endpoint ENI to re-associate", - awserrors.ErrorDBInstanceInvalidState, rec.DBInstanceIdentifier) + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s has no endpoint ENI to re-associate", rec.DBInstanceIdentifier) } // ModifyNetworkInterfaceAttribute validates the groups against the ENI's VPC // itself, but doing it here keeps the rejection ahead of every other write in @@ -260,8 +260,8 @@ func planBackupSettings(input *rds.ModifyDBInstanceInput, rec *DBInstanceRecord, if input.BackupRetentionPeriod != nil { days := aws.Int64Value(input.BackupRetentionPeriod) if days < 0 || days > maxBackupRetentionDays { - return fmt.Errorf("%s: BackupRetentionPeriod must be between 0 and %d days", - awserrors.ErrorInvalidParameterValue, maxBackupRetentionDays) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "BackupRetentionPeriod must be between 0 and %d days", maxBackupRetentionDays) } if days != rec.BackupRetentionPeriod { plan.BackupRetentionPeriod = aws.Int64(days) @@ -288,7 +288,8 @@ func (s *Service) applyImmediateModify(ctx context.Context, kv jetstream.KeyValu // leaving cleartext queued for a later window (D8). if plan.MasterUserPassword != "" { if err := s.setMasterPassword(ctx, accountID, rec.DBInstanceIdentifier, rec.MasterUsername, plan.MasterUserPassword); err != nil { - return fmt.Errorf("%s: the master password could not be applied: %w", awserrors.ErrorDBInstanceInvalidState, err) + return awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "the master password could not be applied: %w", err) } } // Done before the record write so a rejected group leaves the record still diff --git a/spinifex/handlers/rds/network.go b/spinifex/handlers/rds/network.go index eb1dfb76f..15f7a1b6c 100644 --- a/spinifex/handlers/rds/network.go +++ b/spinifex/handlers/rds/network.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go/jetstream" ) // The customer-account VPC surface CreateDBInstance reads to place the endpoint @@ -25,22 +26,36 @@ type endpointPlacement struct { SecurityGroupIDs []string } -// Until DB subnet groups land in rds-7 the placement is the account's default -// VPC, mirroring AWS's own behaviour when a request names no subnet group. A -// named group is rejected in validation rather than silently resolved here. -func (s *Service) resolvePlacement(ctx context.Context, accountID string, req *validatedCreate) (*endpointPlacement, error) { +// A named DB subnet group decides the VPC and the subnet; an unnamed one falls +// back to the account's default VPC, mirroring AWS's own behaviour. The security +// groups are validated against whichever VPC that resolved to, because an ENI +// cannot carry a group from another one. +func (s *Service) resolvePlacement(ctx context.Context, kv jetstream.KeyValue, accountID string, req *validatedCreate) (*endpointPlacement, error) { if s.deps.Network == nil { - return nil, fmt.Errorf("%s: RDS networking is not wired on this node", awserrors.ErrorServerInternal) + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "RDS networking is not wired on this node") } - vpcID, err := s.defaultVPCID(ctx, accountID) - if err != nil { - return nil, err - } - subnetID, err := s.firstSubnetID(ctx, accountID, vpcID) - if err != nil { - return nil, err + var vpcID, subnetID string + if req.DBSubnetGroupName != "" { + group, _, err := getDBSubnetGroup(ctx, kv, req.DBSubnetGroupName) + if err != nil { + return nil, err + } + subnetID, err = subnetFromGroup(group) + if err != nil { + return nil, err + } + vpcID = group.VpcID + } else { + var err error + if vpcID, err = s.defaultVPCID(ctx, accountID); err != nil { + return nil, err + } + if subnetID, err = s.firstSubnetID(ctx, accountID, vpcID); err != nil { + return nil, err + } } + groupIDs, err := s.resolveSecurityGroups(ctx, accountID, vpcID, req.SecurityGroupIDs) if err != nil { return nil, err @@ -48,6 +63,25 @@ func (s *Service) resolvePlacement(ctx context.Context, accountID string, req *v return &endpointPlacement{VpcID: vpcID, SubnetID: subnetID, SecurityGroupIDs: groupIDs}, nil } +// The subnet a group places an instance in. Sorted rather than first-listed, so +// two instances created against the same group land the same way regardless of +// the order the subnets were supplied in. Single-AZ makes the choice arbitrary +// today; when V2 makes AZs real this is the one function that changes. +func subnetFromGroup(group *DBSubnetGroupRecord) (string, error) { + ids := make([]string, 0, len(group.Subnets)) + for _, subnet := range group.Subnets { + if subnet.SubnetID != "" { + ids = append(ids, subnet.SubnetID) + } + } + if len(ids) == 0 { + return "", awserrors.Errorf(awserrors.ErrorDBInvalidVPCNetworkState, + "DB subnet group %s holds no subnet to place the DB endpoint in", group.Name) + } + slices.Sort(ids) + return ids[0], nil +} + func (s *Service) defaultVPCID(ctx context.Context, accountID string) (string, error) { out, err := s.deps.Network.DescribeVpcs(ctx, &ec2.DescribeVpcsInput{ Filters: []*ec2.Filter{{Name: aws.String("isDefault"), Values: aws.StringSlice([]string{"true"})}}, @@ -62,8 +96,8 @@ func (s *Service) defaultVPCID(ctx context.Context, accountID string) (string, e } } } - return "", fmt.Errorf("%s: no default VPC in this account to place the DB endpoint in", - awserrors.ErrorDBInvalidVPCNetworkState) + return "", awserrors.Errorf(awserrors.ErrorDBInvalidVPCNetworkState, + "no default VPC in this account to place the DB endpoint in") } // Sorted so a repeated create in the same account is placed deterministically @@ -84,8 +118,8 @@ func (s *Service) firstSubnetID(ctx context.Context, accountID, vpcID string) (s } } if len(ids) == 0 { - return "", fmt.Errorf("%s: default VPC %s has no subnet to place the DB endpoint in", - awserrors.ErrorDBInvalidVPCNetworkState, vpcID) + return "", awserrors.Errorf(awserrors.ErrorDBInvalidVPCNetworkState, + "default VPC %s has no subnet to place the DB endpoint in", vpcID) } slices.Sort(ids) return ids[0], nil @@ -120,15 +154,15 @@ func (s *Service) resolveSecurityGroups(ctx context.Context, accountID, vpcID st if len(requested) == 0 { if defaultGroupID == "" { - return nil, fmt.Errorf("%s: VPC %s has no default security group", - awserrors.ErrorDBInvalidVPCNetworkState, vpcID) + return nil, awserrors.Errorf(awserrors.ErrorDBInvalidVPCNetworkState, + "VPC %s has no default security group", vpcID) } return []string{defaultGroupID}, nil } for _, id := range requested { if _, ok := inVPC[id]; !ok { - return nil, fmt.Errorf("%s: security group %s is not in VPC %s", - awserrors.ErrorInvalidParameterValue, id, vpcID) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "security group %s is not in VPC %s", id, vpcID) } } return requested, nil diff --git a/spinifex/handlers/rds/paramcatalog.go b/spinifex/handlers/rds/paramcatalog.go new file mode 100644 index 000000000..af4fafde1 --- /dev/null +++ b/spinifex/handlers/rds/paramcatalog.go @@ -0,0 +1,610 @@ +package handlers_rds + +import ( + "fmt" + "maps" + "math" + "slices" + "strconv" + "strings" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/instancetypes" +) + +// The static PostgreSQL 18 parameter catalog, following the EKS add-on catalog +// pattern: a curated in-binary table rather than the engine's full ~350 GUCs. +// A curated subset that is genuinely validated is worth more than the whole set +// passed through unchecked — a static parameter the engine refuses at startup +// is a boot loop, and the bad config is on the persistent data volume. + +// When a parameter takes effect. Static settings are stored and reported +// pending-reboot; dynamic ones are adopted by a reload (D16). +const ( + ApplyTypeStatic = "static" + ApplyTypeDynamic = "dynamic" +) + +// AWS's own ApplyMethod values, echoed back on DescribeDBParameters. +const ( + ApplyMethodImmediate = "immediate" + ApplyMethodPendingReboot = "pending-reboot" +) + +// Where a reported value came from. AWS distinguishes these and the Terraform +// provider reads them, so a computed default must not be reported as user. +const ( + ParameterSourceUser = "user" + ParameterSourceEngineDefault = "engine-default" +) + +// The parameter data types the catalog offers. Every one is validated on input, +// so a value the API accepts is one the engine will parse. +const ( + paramTypeInteger = "integer" + paramTypeReal = "real" + paramTypeBoolean = "boolean" + paramTypeString = "string" + paramTypeEnum = "enum" +) + +// One catalog entry. Exactly one of Default and DefaultFor is set: a literal for +// the parameters whose engine default is size-independent, and a formula over +// the instance class's memory for the ones that are not (D20). +type ParameterSpec struct { + Name string + DataType string + ApplyType string + Description string + // False for the handful of settings the platform owns — changing them would + // break the endpoint, the agent's socket access or the serving certificate. + IsModifiable bool + + Default string + // Evaluated against the class's memory. The result is a literal, so the + // customer-facing API never sees a formula. + DefaultFor func(memoryMiB int64) string + + // Inclusive bounds for integer and real parameters. Both zero means + // unbounded below and above. + Min, Max float64 + // The permitted values of an enum parameter, lowest-to-highest where the + // engine gives them an order. + Enum []string + // The engine's own unit suffix, reported in AllowedValues so a customer can + // see what an integer means. Empty for unitless settings. + Unit string +} + +// The catalog, keyed by parameter name. Every entry here is one a customer has a +// real reason to tune; the platform-owned settings (port, listen_addresses, +// ssl*, unix_socket_directories, data_directory) are deliberately absent rather +// than present-and-unmodifiable, because they are not the customer's to set. +var parameterCatalog = buildParameterCatalog( + // Connections. max_connections is what a size-derived default matters most + // for: RDS's own formula is LEAST({DBInstanceClassMemory/9531392}, 5000). + ParameterSpec{ + Name: "max_connections", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 6, Max: 5000, + DefaultFor: maxConnectionsFor, + Description: "Maximum number of concurrent connections to the database server.", + }, + ParameterSpec{ + Name: "superuser_reserved_connections", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 100, Default: "3", + Description: "Connection slots reserved for superusers.", + }, + ParameterSpec{ + Name: "idle_in_transaction_session_timeout", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 2147483647, Default: "0", Unit: "ms", + Description: "Milliseconds an idle open transaction may live before it is aborted; 0 disables.", + }, + ParameterSpec{ + Name: "statement_timeout", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 2147483647, Default: "0", Unit: "ms", + Description: "Milliseconds a statement may run before it is aborted; 0 disables.", + }, + ParameterSpec{ + Name: "lock_timeout", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 2147483647, Default: "0", Unit: "ms", + Description: "Milliseconds to wait for a lock before failing the statement; 0 disables.", + }, + ParameterSpec{ + Name: "tcp_keepalives_idle", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 3600, Default: "0", Unit: "s", + Description: "Idle seconds before the server sends a TCP keepalive; 0 takes the system default.", + }, + + // Memory. Every one of these is a fraction of class memory on real RDS, so a + // literal tuned for the large end makes db.t3.micro fail to start. + ParameterSpec{ + Name: "shared_buffers", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 16, Max: 4194304, Unit: "8kB", + DefaultFor: sharedBuffersFor, + Description: "Shared memory buffers the server uses, in 8 kB blocks.", + }, + ParameterSpec{ + Name: "effective_cache_size", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1, Max: 4194304, Unit: "8kB", + DefaultFor: effectiveCacheSizeFor, + Description: "Planner assumption about the disk cache available to one query, in 8 kB blocks.", + }, + // Per sort or hash rather than per connection, so RDS leaves it a literal + // even in the memory group; a size-derived default here would multiply by + // max_connections and by the parallel workers under each of them. + ParameterSpec{ + Name: "work_mem", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 64, Max: 2147483647, Default: "4096", Unit: "kB", + Description: "Memory one sort or hash operation may use before spilling to disk, in kB.", + }, + ParameterSpec{ + Name: "maintenance_work_mem", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1024, Max: 2147483647, Unit: "kB", + DefaultFor: maintenanceWorkMemFor, + Description: "Memory a VACUUM, CREATE INDEX or ALTER TABLE ADD FOREIGN KEY may use, in kB.", + }, + ParameterSpec{ + Name: "temp_buffers", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 100, Max: 1073741823, Default: "1024", Unit: "8kB", + Description: "Per-session buffers for temporary tables, in 8 kB blocks.", + }, + ParameterSpec{ + Name: "max_prepared_transactions", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 262143, Default: "0", + Description: "Maximum simultaneously prepared transactions; 0 disables two-phase commit.", + }, + + // Parallelism. Bounded by the class's vCPU count on a real deployment, but + // PostgreSQL degrades gracefully when they exceed it, so literals are honest. + ParameterSpec{ + Name: "max_worker_processes", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 262143, Default: "8", + Description: "Maximum background processes the server may start.", + }, + ParameterSpec{ + Name: "max_parallel_workers", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 1024, Default: "8", + Description: "Maximum workers the server may use for parallel operations.", + }, + ParameterSpec{ + Name: "max_parallel_workers_per_gather", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 1024, Default: "2", + Description: "Maximum parallel workers one Gather node may start.", + }, + + // WAL and checkpoints. + ParameterSpec{ + Name: "wal_level", DataType: paramTypeEnum, ApplyType: ApplyTypeStatic, + IsModifiable: true, Enum: []string{"minimal", "replica", "logical"}, Default: "replica", + Description: "How much information is written to the WAL.", + }, + ParameterSpec{ + Name: "synchronous_commit", DataType: paramTypeEnum, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Enum: []string{"off", "local", "remote_write", "on", "remote_apply"}, Default: "on", + Description: "How much WAL processing must complete before a commit returns.", + }, + ParameterSpec{ + Name: "wal_compression", DataType: paramTypeEnum, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Enum: []string{"off", "pglz", "lz4", "zstd", "on"}, Default: "off", + Description: "Compression method for full-page images written to the WAL.", + }, + ParameterSpec{ + Name: "max_wal_size", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 2, Max: 2097151, Default: "1024", Unit: "MB", + Description: "WAL size that triggers a checkpoint, in MB.", + }, + ParameterSpec{ + Name: "min_wal_size", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 2, Max: 2097151, Default: "80", Unit: "MB", + Description: "WAL size below which old segments are recycled rather than removed, in MB.", + }, + ParameterSpec{ + Name: "checkpoint_timeout", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 30, Max: 86400, Default: "300", Unit: "s", + Description: "Maximum seconds between automatic WAL checkpoints.", + }, + ParameterSpec{ + Name: "checkpoint_completion_target", DataType: paramTypeReal, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 1, Default: "0.9", + Description: "Fraction of the checkpoint interval a checkpoint's writes are spread over.", + }, + ParameterSpec{ + Name: "wal_buffers", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: -1, Max: 262143, Default: "-1", Unit: "8kB", + Description: "Shared memory used for WAL not yet written, in 8 kB blocks; -1 derives it from shared_buffers.", + }, + ParameterSpec{ + Name: "max_wal_senders", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 262143, Default: "10", + Description: "Maximum simultaneously running WAL sender processes.", + }, + ParameterSpec{ + Name: "max_replication_slots", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 0, Max: 262143, Default: "10", + Description: "Maximum replication slots the server may define.", + }, + + // Autovacuum. Turning it off is the single most reliable way to take a + // PostgreSQL database down slowly, so it is offered but its default is on. + ParameterSpec{ + Name: "autovacuum", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "on", + Description: "Whether the autovacuum launcher runs.", + }, + ParameterSpec{ + Name: "autovacuum_max_workers", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 1, Max: 262143, Default: "3", + Description: "Maximum autovacuum worker processes running at once.", + }, + ParameterSpec{ + Name: "autovacuum_naptime", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1, Max: 2147483, Default: "60", Unit: "s", + Description: "Seconds between autovacuum runs on any one database.", + }, + ParameterSpec{ + Name: "autovacuum_vacuum_threshold", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 2147483647, Default: "50", + Description: "Row updates or deletes before a table is vacuumed.", + }, + ParameterSpec{ + Name: "autovacuum_vacuum_scale_factor", DataType: paramTypeReal, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 100, Default: "0.2", + Description: "Fraction of the table size added to autovacuum_vacuum_threshold.", + }, + ParameterSpec{ + Name: "autovacuum_analyze_threshold", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 2147483647, Default: "50", + Description: "Row inserts, updates or deletes before a table is analyzed.", + }, + ParameterSpec{ + Name: "autovacuum_analyze_scale_factor", DataType: paramTypeReal, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 100, Default: "0.1", + Description: "Fraction of the table size added to autovacuum_analyze_threshold.", + }, + ParameterSpec{ + Name: "autovacuum_vacuum_cost_limit", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: -1, Max: 10000, Default: "-1", + Description: "Cost budget one autovacuum worker spends before sleeping; -1 takes vacuum_cost_limit.", + }, + + // Planner. + // The engine's own ceiling on a planner cost is DBL_MAX, which is not a bound + // worth reporting: a cost past four digits already makes the plan choice + // insensitive to it, so the range stays one a customer can read. + ParameterSpec{ + Name: "random_page_cost", DataType: paramTypeReal, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 10000, Default: "1.1", + Description: "Planner estimate of the cost of a non-sequentially fetched page.", + }, + ParameterSpec{ + Name: "seq_page_cost", DataType: paramTypeReal, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 10000, Default: "1", + Description: "Planner estimate of the cost of a sequentially fetched page.", + }, + ParameterSpec{ + Name: "effective_io_concurrency", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 0, Max: 1000, Default: "16", + Description: "Concurrent disk I/O operations the planner assumes are useful.", + }, + ParameterSpec{ + Name: "default_statistics_target", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1, Max: 10000, Default: "100", + Description: "Default statistics target for table columns without one of their own.", + }, + ParameterSpec{ + Name: "jit", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "on", + Description: "Whether JIT compilation may be used for qualifying queries.", + }, + + // Logging. The destination and the collector are platform-owned; what is + // logged is the customer's. + ParameterSpec{ + Name: "log_min_duration_statement", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: -1, Max: 2147483647, Default: "-1", Unit: "ms", + Description: "Milliseconds a statement must run to be logged; 0 logs every statement, -1 disables.", + }, + ParameterSpec{ + Name: "log_statement", DataType: paramTypeEnum, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Enum: []string{"none", "ddl", "mod", "all"}, Default: "none", + Description: "Which SQL statements are written to the log.", + }, + ParameterSpec{ + Name: "log_min_messages", DataType: paramTypeEnum, ApplyType: ApplyTypeDynamic, + IsModifiable: true, + Enum: []string{"debug5", "debug4", "debug3", "debug2", "debug1", "info", "notice", + "warning", "error", "log", "fatal", "panic"}, + Default: "warning", + Description: "Lowest message severity written to the server log.", + }, + ParameterSpec{ + Name: "log_connections", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "off", + Description: "Whether each successful connection attempt is logged.", + }, + ParameterSpec{ + Name: "log_disconnections", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "off", + Description: "Whether the end of each session is logged, with its duration.", + }, + ParameterSpec{ + Name: "log_lock_waits", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "on", + Description: "Whether a session waiting longer than deadlock_timeout for a lock is logged.", + }, + ParameterSpec{ + Name: "log_temp_files", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: -1, Max: 2147483647, Default: "-1", Unit: "kB", + Description: "Size in kB above which a temporary file's removal is logged; 0 logs all, -1 disables.", + }, + ParameterSpec{ + Name: "log_autovacuum_min_duration", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: -1, Max: 2147483647, Default: "-1", Unit: "ms", + Description: "Milliseconds an autovacuum action must run to be logged; -1 disables.", + }, + + // Timeouts and locale. + ParameterSpec{ + Name: "deadlock_timeout", DataType: paramTypeInteger, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Min: 1, Max: 2147483647, Default: "1000", Unit: "ms", + Description: "Milliseconds to wait on a lock before checking for a deadlock.", + }, + ParameterSpec{ + Name: "timezone", DataType: paramTypeString, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "UTC", + Description: "Time zone the server displays and interprets timestamps in.", + }, + ParameterSpec{ + Name: "datestyle", DataType: paramTypeString, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "ISO, MDY", + Description: "Display format for date and time values.", + }, + ParameterSpec{ + Name: "track_activity_query_size", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, + IsModifiable: true, Min: 100, Max: 1048576, Default: "1024", Unit: "B", + Description: "Bytes of each running query pg_stat_activity retains.", + }, + ParameterSpec{ + Name: "track_io_timing", DataType: paramTypeBoolean, ApplyType: ApplyTypeDynamic, + IsModifiable: true, Default: "on", + Description: "Whether block read and write times are collected.", + }, +) + +// The bytes of class memory an RDS parameter formula divides. Real RDS exposes +// it as {DBInstanceClassMemory}, and the resolver evaluates the formulas here +// rather than passing them to the engine. +const mibToBytes = 1024 * 1024 + +// shared_buffers = {DBInstanceClassMemory/32768}, in 8 kB blocks — a quarter of +// class memory, which is RDS's own default. +func sharedBuffersFor(memoryMiB int64) string { + return strconv.FormatInt(clampInt64(memoryMiB*mibToBytes/32768, 16, 4194304), 10) +} + +// effective_cache_size = {DBInstanceClassMemory/16384}, in 8 kB blocks — half of +// class memory, the planner's assumption about what the OS is caching. +func effectiveCacheSizeFor(memoryMiB int64) string { + return strconv.FormatInt(clampInt64(memoryMiB*mibToBytes/16384, 1, 4194304), 10) +} + +// max_connections = LEAST({DBInstanceClassMemory/9531392}, 5000). The floor is +// what keeps db.t3.micro above superuser_reserved_connections plus the workers +// autovacuum and replication need, which is where the smallest class would +// otherwise fail to start. +func maxConnectionsFor(memoryMiB int64) string { + return strconv.FormatInt(clampInt64(memoryMiB*mibToBytes/9531392, 20, 5000), 10) +} + +// maintenance_work_mem = {DBInstanceClassMemory/63963136}, in kB. Capped so a +// large class does not let autovacuum_max_workers reserve the whole machine. +func maintenanceWorkMemFor(memoryMiB int64) string { + return strconv.FormatInt(clampInt64(memoryMiB*mibToBytes/63963136*1024, 16384, 2097152), 10) +} + +func clampInt64(v, lo, hi int64) int64 { + return min(max(v, lo), hi) +} + +// 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 { + out := make(map[string]ParameterSpec, len(specs)) + for _, spec := range specs { + switch { + case spec.Name == "": + panic("rds: parameter catalog holds an unnamed entry") + case spec.Default == "" && spec.DefaultFor == nil: + panic("rds: parameter catalog entry " + spec.Name + " has no default") + case spec.Default != "" && spec.DefaultFor != nil: + panic("rds: parameter catalog entry " + spec.Name + " has both a literal and a computed default") + } + if _, exists := out[spec.Name]; exists { + panic("rds: parameter catalog entry " + spec.Name + " is duplicated") + } + out[spec.Name] = spec + } + return out +} + +// The catalog entry for a parameter name, or false when the engine has no such +// setting or it is one this platform does not expose. +func LookupParameter(name string) (ParameterSpec, bool) { + spec, ok := parameterCatalog[strings.ToLower(strings.TrimSpace(name))] + return spec, ok +} + +// Sorted, so a describe returns the same order on every call and Terraform does +// not read a reshuffle as drift. +func CatalogParameterNames() []string { + return slices.Sorted(maps.Keys(parameterCatalog)) +} + +// The engine default for one parameter at one instance class: the literal, or +// the formula evaluated against the class's memory (D20). +func (s ParameterSpec) DefaultAt(memoryMiB int64) string { + if s.DefaultFor != nil { + return s.DefaultFor(memoryMiB) + } + return s.Default +} + +// The AllowedValues string AWS reports: a range for numerics, the alternatives +// for an enum or boolean. Empty for a free-form string, as AWS leaves it. +func (s ParameterSpec) AllowedValues() string { + switch s.DataType { + case paramTypeInteger, paramTypeReal: + bounds := fmt.Sprintf("%s-%s", formatBound(s.Min), formatBound(s.Max)) + if s.Unit != "" { + return bounds + " (" + s.Unit + ")" + } + return bounds + case paramTypeEnum: + return strings.Join(s.Enum, ",") + case paramTypeBoolean: + return "on,off,true,false,yes,no,1,0" + default: + return "" + } +} + +// Integral bounds print without a decimal point, so an integer parameter's range +// does not read as a real one's. +func formatBound(v float64) string { + if v == math.Trunc(v) && math.Abs(v) < 1e15 { + return strconv.FormatInt(int64(v), 10) + } + return strconv.FormatFloat(v, 'g', -1, 64) +} + +// The memory an instance class's guest has, which every size-derived default is +// computed from. An unknown class is a validation failure upstream, so this is +// the last line rather than the check. +func classMemoryMiB(instanceClass string) (int64, error) { + instanceType, err := InstanceTypeForClass(instanceClass) + if err != nil { + return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + } + memoryMiB, ok := instancetypes.DefaultMemoryMiB(instanceType) + if !ok || memoryMiB <= 0 { + return 0, awserrors.Errorf(awserrors.ErrorServerInternal, + "no memory footprint is known for instance type %s", instanceType) + } + return memoryMiB, nil +} + +// AWS accepts formulas like {DBInstanceClassMemory/32768} and references like +// DBInstanceClassMemory from a customer. This platform does not: the catalog +// validates literals, and a formula that reached the engine unvalidated would be +// a startup failure rather than an API error. +func rejectFormulaValue(name, value string) error { + if !strings.ContainsAny(value, "{}") && !strings.Contains(value, "DBInstanceClass") { + return nil + } + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "the value %q of parameter %s is a formula; only literal values are accepted, "+ + "and the size-derived defaults are computed for you", value, name) +} + +// Checks one customer-supplied value against its catalog entry. A parameter the +// catalog does not hold, one the platform owns, and one outside its own range +// are all rejected here — at the API, rather than by an engine that then refuses +// to start with the bad config already on the data volume. +func validateParameterValue(name, value string) (ParameterSpec, error) { + spec, ok := LookupParameter(name) + if !ok { + return ParameterSpec{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "%q is not a parameter this engine exposes", name) + } + if !spec.IsModifiable { + return ParameterSpec{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "parameter %s is not modifiable", spec.Name) + } + if err := rejectFormulaValue(spec.Name, value); err != nil { + return ParameterSpec{}, err + } + + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return ParameterSpec{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "parameter %s was given an empty value", spec.Name) + } + switch spec.DataType { + case paramTypeInteger: + n, err := strconv.ParseInt(trimmed, 10, 64) + if err != nil { + return ParameterSpec{}, typeError(spec, value, "an integer") + } + if float64(n) < spec.Min || float64(n) > spec.Max { + return ParameterSpec{}, rangeError(spec, value) + } + case paramTypeReal: + f, err := strconv.ParseFloat(trimmed, 64) + if err != nil { + return ParameterSpec{}, typeError(spec, value, "a number") + } + if f < spec.Min || f > spec.Max { + return ParameterSpec{}, rangeError(spec, value) + } + case paramTypeBoolean: + if !validBoolean(trimmed) { + return ParameterSpec{}, typeError(spec, value, "a boolean (on, off, true, false, yes, no, 1, 0)") + } + case paramTypeEnum: + if !slices.Contains(spec.Enum, strings.ToLower(trimmed)) { + return ParameterSpec{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "parameter %s does not accept %q; allowed values are %s", spec.Name, value, strings.Join(spec.Enum, ", ")) + } + } + return spec, nil +} + +// PostgreSQL's own boolean spellings, so a config a customer copied from the +// engine's documentation is accepted as the engine would accept it. +func validBoolean(value string) bool { + switch strings.ToLower(value) { + case "on", "off", "true", "false", "yes", "no", "1", "0": + return true + default: + return false + } +} + +func typeError(spec ParameterSpec, value, want string) error { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "parameter %s takes %s, not %q", spec.Name, want, value) +} + +func rangeError(spec ParameterSpec, value string) error { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "the value %q of parameter %s is outside its allowed range %s", value, spec.Name, spec.AllowedValues()) +} + +// The full parameter set an instance runs with: every catalog default evaluated +// at the instance's class, overlaid with the group's stored overrides. The +// result is literals only, sorted by name so a re-resolve that changed nothing +// produces a byte-identical include. +// +// Overrides are re-validated rather than trusted: a catalog whose bounds +// tightened must not keep handing the engine a value it would now reject. +func ResolveEffectiveParameters(instanceClass string, overrides map[string]string) ([]Parameter, error) { + memoryMiB, err := classMemoryMiB(instanceClass) + if err != nil { + return nil, err + } + names := CatalogParameterNames() + resolved := make([]Parameter, 0, len(names)) + for _, name := range names { + spec := parameterCatalog[name] + value := spec.DefaultAt(memoryMiB) + if override, ok := overrides[name]; ok { + if _, err := validateParameterValue(name, override); err != nil { + return nil, err + } + value = override + } + resolved = append(resolved, Parameter{Name: name, Value: value}) + } + return resolved, nil +} diff --git a/spinifex/handlers/rds/paramcatalog_test.go b/spinifex/handlers/rds/paramcatalog_test.go new file mode 100644 index 000000000..a9e11af7c --- /dev/null +++ b/spinifex/handlers/rds/paramcatalog_test.go @@ -0,0 +1,189 @@ +package handlers_rds + +import ( + "strconv" + "strings" + "testing" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// 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. +func TestParameterCatalog_DefaultsSatisfyTheirOwnConstraintsAtEveryClass(t *testing.T) { + for _, class := range SupportedInstanceClasses() { + t.Run(class, func(t *testing.T) { + memoryMiB, err := classMemoryMiB(class) + require.NoError(t, err) + + for _, name := range CatalogParameterNames() { + spec, ok := LookupParameter(name) + require.True(t, ok) + + value := spec.DefaultAt(memoryMiB) + assert.NotEmpty(t, value, "%s has an empty default at %s", name, class) + // Validated by the same function a customer's value goes through, so + // a default cannot be something the API would reject. + _, err := validateParameterValue(name, value) + assert.NoError(t, err, "the default %q of %s is not a value %s accepts", value, name, class) + } + }) + } +} + +// Every entry has to be a shape the resolver and the describe can both render, +// or a client reads a parameter with no type and no apply semantics. +func TestParameterCatalog_EntriesAreWellFormed(t *testing.T) { + for _, name := range CatalogParameterNames() { + spec, _ := LookupParameter(name) + + assert.Equal(t, name, spec.Name, "the catalog key and the spec name disagree") + assert.Contains(t, []string{ApplyTypeStatic, ApplyTypeDynamic}, spec.ApplyType, "%s", name) + assert.Contains(t, []string{paramTypeInteger, paramTypeReal, paramTypeBoolean, paramTypeString, paramTypeEnum}, + spec.DataType, "%s", name) + if spec.DataType == paramTypeEnum { + assert.NotEmpty(t, spec.Enum, "%s is an enum with no allowed values", name) + } + if spec.DataType == paramTypeInteger || spec.DataType == paramTypeReal { + assert.Less(t, spec.Min, spec.Max, "%s has an empty range", name) + } + } +} + +// D20: the size-derived defaults have to actually move with the class, or the +// formulas are decoration and one end of the D15 range is mis-tuned. +func TestParameterCatalog_SizeDerivedDefaultsScaleWithClassMemory(t *testing.T) { + smallest, err := classMemoryMiB("db.t3.micro") + require.NoError(t, err) + largest, err := classMemoryMiB("db.m5.xlarge") + require.NoError(t, err) + require.Less(t, smallest, largest) + + for _, name := range []string{"shared_buffers", "effective_cache_size", "max_connections", "maintenance_work_mem"} { + spec, ok := LookupParameter(name) + require.True(t, ok, "%s is expected to carry a computed default", name) + require.NotNil(t, spec.DefaultFor, "%s should be size-derived", name) + + small, err := strconv.ParseInt(spec.DefaultAt(smallest), 10, 64) + require.NoError(t, err) + large, err := strconv.ParseInt(spec.DefaultAt(largest), 10, 64) + require.NoError(t, err) + assert.Less(t, small, large, "%s does not grow with the class", name) + } +} + +// shared_buffers is a quarter of class memory on real RDS, and it is the setting +// whose being wrong at the small end stops the engine from starting. +func TestParameterCatalog_SharedBuffersIsAQuarterOfClassMemory(t *testing.T) { + memoryMiB, err := classMemoryMiB("db.m5.large") + require.NoError(t, err) + + blocks, err := strconv.ParseInt(sharedBuffersFor(memoryMiB), 10, 64) + require.NoError(t, err) + assert.Equal(t, memoryMiB/4, blocks*8/1024, "shared_buffers is not a quarter of the class's memory") +} + +func TestValidateParameterValue_RejectsBadInput(t *testing.T) { + cases := []struct { + name, param, value, want string + }{ + {"UnknownName", "not_a_setting", "1", "is not a parameter this engine exposes"}, + {"NotAnInteger", "max_connections", "many", "takes an integer"}, + {"BelowRange", "max_connections", "1", "outside its allowed range"}, + {"AboveRange", "max_connections", "500000", "outside its allowed range"}, + {"NotAReal", "checkpoint_completion_target", "high", "takes a number"}, + {"RealOutOfRange", "checkpoint_completion_target", "2", "outside its allowed range"}, + {"NotABoolean", "autovacuum", "maybe", "takes a boolean"}, + {"NotInEnum", "log_statement", "everything", "does not accept"}, + {"Empty", "work_mem", "", "empty value"}, + // AWS accepts these; passing one through would be a startup failure rather + // than an API error. + {"Formula", "shared_buffers", "{DBInstanceClassMemory/32768}", "is a formula"}, + {"Reference", "max_connections", "DBInstanceClassMemory", "is a formula"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := validateParameterValue(tc.param, tc.value) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), tc.want) + }) + } +} + +func TestValidateParameterValue_AcceptsEveryEnginesSpellingOfABoolean(t *testing.T) { + for _, value := range []string{"on", "OFF", "true", "False", "yes", "no", "1", "0"} { + _, err := validateParameterValue("autovacuum", value) + assert.NoError(t, err, "autovacuum rejected %q, which the engine accepts", value) + } +} + +// A group's overrides are laid over the whole catalog, not merged into a subset: +// every parameter is present, and one the group does not set carries its default. +func TestResolveEffectiveParameters_OverlaysOverridesOnDefaults(t *testing.T) { + resolved, err := ResolveEffectiveParameters("db.t3.medium", map[string]string{"work_mem": "16384"}) + require.NoError(t, err) + require.Len(t, resolved, len(CatalogParameterNames())) + + values := map[string]string{} + for _, param := range resolved { + values[param.Name] = param.Value + } + assert.Equal(t, "16384", values["work_mem"], "the override did not win") + + memoryMiB, err := classMemoryMiB("db.t3.medium") + require.NoError(t, err) + assert.Equal(t, sharedBuffersFor(memoryMiB), values["shared_buffers"], + "a parameter the group does not set should carry its computed default") +} + +// Sorted output is what lets a re-resolve that changed nothing produce a +// byte-identical include, so an apply does not restart an engine for no reason. +func TestResolveEffectiveParameters_IsSortedAndCarriesNoFormula(t *testing.T) { + resolved, err := ResolveEffectiveParameters("db.m5.xlarge", nil) + require.NoError(t, err) + + for i := 1; i < len(resolved); i++ { + assert.Less(t, resolved[i-1].Name, resolved[i].Name, "the resolved set is not sorted by name") + } + for _, param := range resolved { + assert.NotContains(t, param.Value, "{", "%s reached the agent as a formula", param.Name) + assert.NotContains(t, param.Value, "DBInstanceClass", "%s reached the agent as a reference", param.Name) + } +} + +// A stored value the catalog would now reject must not keep being handed to the +// engine, so the resolve re-validates rather than trusting what was written. +func TestResolveEffectiveParameters_RevalidatesStoredOverrides(t *testing.T) { + _, err := ResolveEffectiveParameters("db.t3.medium", map[string]string{"max_connections": "999999"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "max_connections") +} + +func TestResolveEffectiveParameters_RejectsAnUnknownClass(t *testing.T) { + _, err := ResolveEffectiveParameters("db.x99.mega", nil) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// AllowedValues is what tells a customer what an integer means, so a unitful +// parameter has to report its unit rather than a bare pair of numbers. +func TestParameterSpec_AllowedValuesDescribesTheConstraint(t *testing.T) { + shared, _ := LookupParameter("shared_buffers") + assert.Equal(t, "16-4194304 (8kB)", shared.AllowedValues()) + + logStatement, _ := LookupParameter("log_statement") + assert.Equal(t, "none,ddl,mod,all", logStatement.AllowedValues()) + + timezone, _ := LookupParameter("timezone") + assert.Empty(t, timezone.AllowedValues(), "a free-form string has no constraint to report") + + target, _ := LookupParameter("checkpoint_completion_target") + assert.True(t, strings.HasPrefix(target.AllowedValues(), "0-1"), "got %q", target.AllowedValues()) +} diff --git a/spinifex/handlers/rds/parametergroup.go b/spinifex/handlers/rds/parametergroup.go new file mode 100644 index 000000000..f423d889b --- /dev/null +++ b/spinifex/handlers/rds/parametergroup.go @@ -0,0 +1,480 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + "maps" + "slices" + "strings" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go/jetstream" +) + +// AWS's cap on one ModifyDBParameterGroup call. +const maxParametersPerModify = 20 + +// The prefix AWS reserves for the groups the service itself owns. A group named +// with it is reported and resolvable but never modifiable or deletable. +const defaultParameterGroupPrefix = "default." + +// Creates a customer-owned parameter group. It starts empty: every value is a +// catalog default until the customer overrides one, so a fresh group and the +// default group resolve to the same effective set. +func (s *Service) CreateDBParameterGroup(ctx context.Context, input *rds.CreateDBParameterGroupInput, accountID string) (*rds.CreateDBParameterGroupOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBParameterGroupName) + if err := validateDBGroupName("DBParameterGroupName", name); err != nil { + return nil, err + } + // Rejected rather than accepted-and-shadowed: a customer group under the + // reserved prefix would be indistinguishable from the implicit one. + if isDefaultParameterGroupName(name) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBParameterGroupName may not begin with %q, which the service reserves", defaultParameterGroupPrefix) + } + description := aws.StringValue(input.Description) + if err := validateDBGroupDescription("Description", description); err != nil { + return nil, err + } + family, err := validateParameterGroupFamily(aws.StringValue(input.DBParameterGroupFamily)) + if err != nil { + return nil, err + } + tags, err := validateTags(input.Tags) + if err != nil { + return nil, err + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + now := time.Now().UTC() + rec := DBParameterGroupRecord{ + Name: name, + AccountID: accountID, + Family: family, + Description: description, + Tags: tags, + CreatedAt: now, + UpdatedAt: now, + } + if err := createJSON(ctx, kv, DBParameterGroupMetaKey(name), &rec); err != nil { + if errors.Is(err, jetstream.ErrKeyExists) { + return nil, awserrors.Errorf(awserrors.ErrorDBParameterGroupAlreadyExists, + "DB parameter group %s already exists", name) + } + return nil, err + } + + slog.InfoContext(ctx, "rds: DB parameter group created", + "dbParameterGroup", name, "accountId", accountID, "family", family) + return &rds.CreateDBParameterGroupOutput{DBParameterGroup: s.projectParameterGroupRecord(&rec)}, nil +} + +// The implicit default group is reported alongside the customer's own, whether +// or not anything has materialised it yet — a client that lists groups before +// creating its first instance must still see the one it can name. +func (s *Service) DescribeDBParameterGroups(ctx context.Context, input *rds.DescribeDBParameterGroupsInput, accountID string) (*rds.DescribeDBParameterGroupsOutput, error) { + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if input != nil { + if name := aws.StringValue(input.DBParameterGroupName); name != "" { + rec, _, err := getDBParameterGroup(ctx, kv, accountID, name) + if err != nil { + return nil, err + } + return &rds.DescribeDBParameterGroupsOutput{ + DBParameterGroups: []*rds.DBParameterGroup{s.projectParameterGroupRecord(rec)}, + }, nil + } + } + + names, err := ListDBParameterGroupNames(ctx, kv) + if err != nil { + return nil, err + } + // The default group is synthesised rather than read, so it appears exactly + // once whether or not a prior create persisted it. + for _, engine := range engines { + names = append(names, engine.DefaultParameterGroupName()) + } + slices.Sort(names) + names = slices.Compact(names) + + groups := make([]*rds.DBParameterGroup, 0, len(names)) + for _, name := range names { + rec, _, err := getDBParameterGroup(ctx, kv, accountID, name) + if err != nil { + // Deleted between the listing and this read; the same answer a describe + // one tick later would give. + if strings.HasPrefix(err.Error(), awserrors.ErrorDBParameterGroupNotFound) { + continue + } + return nil, err + } + groups = append(groups, s.projectParameterGroupRecord(rec)) + } + return &rds.DescribeDBParameterGroupsOutput{DBParameterGroups: groups}, nil +} + +// Stores validated overrides, one KV key per parameter, so a modify touching one +// setting cannot clobber a concurrent change to another (D3). The whole request +// is validated before anything is written: a batch with one bad value must leave +// the group exactly as it was rather than half-applied. +func (s *Service) ModifyDBParameterGroup(ctx context.Context, input *rds.ModifyDBParameterGroupInput, accountID string) (*rds.DBParameterGroupNameMessage, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBParameterGroupName) + if name == "" { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBParameterGroupName is required") + } + if isDefaultParameterGroupName(name) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DB parameter group %s is a default group and cannot be modified", name) + } + if len(input.Parameters) == 0 { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Parameters must name at least one parameter") + } + if len(input.Parameters) > maxParametersPerModify { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "at most %d parameters may be modified in one request, got %d", maxParametersPerModify, len(input.Parameters)) + } + + updates, err := validateParameterUpdates(input.Parameters) + if err != nil { + return nil, err + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if _, _, err := getDBParameterGroup(ctx, kv, accountID, name); err != nil { + return nil, err + } + + now := time.Now().UTC() + for _, update := range updates { + if err := putJSON(ctx, kv, DBParameterGroupParamKey(name, update.Name), &DBParameterRecord{ + Name: update.Name, + Value: update.Value, + ApplyMethod: update.ApplyMethod, + UpdatedAt: now, + }); err != nil { + return nil, err + } + } + + slog.InfoContext(ctx, "rds: DB parameter group modified", + "dbParameterGroup", name, "accountId", accountID, "parameters", len(updates)) + return &rds.DBParameterGroupNameMessage{DBParameterGroupName: aws.String(name)}, nil +} + +// Merges catalog defaults with the group's stored overrides. The defaults are +// evaluated at the smallest supported class, because a parameter group is not +// bound to an instance: a customer reading a group's values before creating +// anything has to see literals, and D20 keeps formulas off this surface. +func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeDBParametersInput, accountID string) (*rds.DescribeDBParametersOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBParameterGroupName) + if name == "" { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBParameterGroupName is required") + } + source := strings.ToLower(strings.TrimSpace(aws.StringValue(input.Source))) + switch source { + case "", ParameterSourceUser, ParameterSourceEngineDefault: + default: + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Source %q is not one of %s or %s", source, ParameterSourceUser, ParameterSourceEngineDefault) + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if _, _, err := getDBParameterGroup(ctx, kv, accountID, name); err != nil { + return nil, err + } + overrides, err := ListDBParameterOverrides(ctx, kv, name) + if err != nil { + return nil, err + } + memoryMiB, err := classMemoryMiB(describeParametersClass()) + if err != nil { + return nil, err + } + + out := &rds.DescribeDBParametersOutput{} + for _, param := range CatalogParameterNames() { + spec, _ := LookupParameter(param) + value, isOverride := overrides[param] + if !isOverride { + value = spec.DefaultAt(memoryMiB) + } + if source != "" && source != parameterSource(isOverride) { + continue + } + out.Parameters = append(out.Parameters, projectParameter(spec, value, isOverride)) + } + return out, nil +} + +// Refused for a default group, and while any instance still references it — +// including one that is only deleting, so a destroy that races the teardown +// fails cleanly rather than leaving a live engine's configuration unreadable. +func (s *Service) DeleteDBParameterGroup(ctx context.Context, input *rds.DeleteDBParameterGroupInput, accountID string) (*rds.DeleteDBParameterGroupOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBParameterGroupName) + if name == "" { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBParameterGroupName is required") + } + if isDefaultParameterGroupName(name) { + return nil, awserrors.Errorf(awserrors.ErrorDBParameterGroupInvalidState, + "DB parameter group %s is a default group and cannot be deleted", name) + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if _, _, err := getDBParameterGroup(ctx, kv, accountID, name); err != nil { + return nil, err + } + users, err := instancesUsingGroup(ctx, kv, func(rec *DBInstanceRecord) bool { + return rec.DBParameterGroupName == name || + (rec.PendingModifiedValues != nil && rec.PendingModifiedValues.DBParameterGroupName == name) + }) + if err != nil { + return nil, err + } + if len(users) > 0 { + return nil, awserrors.Errorf(awserrors.ErrorDBParameterGroupInvalidState, + "DB parameter group %s is still used by %s", name, strings.Join(users, ", ")) + } + + // The values go first: a crash between the two leaves orphaned parameter keys + // under a name a later create would then inherit silently. + overrides, err := ListDBParameterOverrides(ctx, kv, name) + if err != nil { + return nil, err + } + for _, param := range slices.Sorted(maps.Keys(overrides)) { + if err := kv.Delete(ctx, DBParameterGroupParamKey(name, param)); err != nil { + return nil, fmt.Errorf("rds: delete parameter %s of group %s: %w", param, name, err) + } + } + if err := kv.Delete(ctx, DBParameterGroupMetaKey(name)); err != nil { + return nil, fmt.Errorf("rds: delete DB parameter group %s: %w", name, err) + } + + slog.InfoContext(ctx, "rds: DB parameter group deleted", "dbParameterGroup", name, "accountId", accountID) + return &rds.DeleteDBParameterGroupOutput{}, nil +} + +// One validated override from a ModifyDBParameterGroup request. +type parameterUpdate struct { + Name string + Value string + ApplyMethod string +} + +// Every entry is checked before any is written. A name repeated in one request +// keeps its last value, as AWS does. +func validateParameterUpdates(params []*rds.Parameter) ([]parameterUpdate, error) { + byName := make(map[string]parameterUpdate, len(params)) + for _, param := range params { + if param == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "a parameter entry is empty") + } + spec, err := validateParameterValue(aws.StringValue(param.ParameterName), aws.StringValue(param.ParameterValue)) + if err != nil { + return nil, err + } + method, err := resolveApplyMethod(spec, aws.StringValue(param.ApplyMethod)) + if err != nil { + return nil, err + } + byName[spec.Name] = parameterUpdate{ + Name: spec.Name, + Value: strings.TrimSpace(aws.StringValue(param.ParameterValue)), + ApplyMethod: method, + } + } + + updates := make([]parameterUpdate, 0, len(byName)) + for _, name := range slices.Sorted(maps.Keys(byName)) { + updates = append(updates, byName[name]) + } + return updates, nil +} + +// A static parameter cannot be applied immediately, which AWS rejects rather +// than silently downgrading — accepting it would tell the customer the change is +// live when the engine has not adopted it. +func resolveApplyMethod(spec ParameterSpec, requested string) (string, error) { + method := strings.ToLower(strings.TrimSpace(requested)) + switch method { + case "": + if spec.ApplyType == ApplyTypeStatic { + return ApplyMethodPendingReboot, nil + } + return ApplyMethodImmediate, nil + case ApplyMethodPendingReboot: + return ApplyMethodPendingReboot, nil + case ApplyMethodImmediate: + if spec.ApplyType == ApplyTypeStatic { + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "parameter %s is static, so ApplyMethod must be %s", spec.Name, ApplyMethodPendingReboot) + } + return ApplyMethodImmediate, nil + default: + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "ApplyMethod %q is not one of %s or %s", requested, ApplyMethodImmediate, ApplyMethodPendingReboot) + } +} + +// The stored record, or the lazily materialised default group. A default group +// is synthesised rather than written on read: the record carries nothing a write +// would preserve, and materialising it on a describe would make a read path a +// writer for no gain. +func getDBParameterGroup(ctx context.Context, kv jetstream.KeyValue, accountID, name string) (*DBParameterGroupRecord, uint64, error) { + var rec DBParameterGroupRecord + rev, found, err := getJSONRevision(ctx, kv, DBParameterGroupMetaKey(name), &rec) + if err != nil { + return nil, 0, err + } + if found { + return &rec, rev, nil + } + if engine, ok := engineForDefaultParameterGroup(name); ok { + return defaultParameterGroupRecord(engine, accountID), 0, nil + } + return nil, 0, awserrors.Errorf(awserrors.ErrorDBParameterGroupNotFound, "DB parameter group %s not found", name) +} + +// The implicit group, identical for every account. It carries no tags and no +// stored values, so it resolves to the catalog defaults alone. +func defaultParameterGroupRecord(engine Engine, accountID string) *DBParameterGroupRecord { + return &DBParameterGroupRecord{ + Name: engine.DefaultParameterGroupName(), + AccountID: accountID, + Family: engine.ParameterGroupFamily(), + Description: fmt.Sprintf("Default parameter group for %s", engine.ParameterGroupFamily()), + } +} + +func isDefaultParameterGroupName(name string) bool { + return strings.HasPrefix(strings.ToLower(name), defaultParameterGroupPrefix) +} + +// The engine whose implicit default group carries this name, so an unrecognised +// default.* name is a not-found rather than a group that resolves to nothing. +func engineForDefaultParameterGroup(name string) (Engine, bool) { + for _, engine := range engines { + if engine.DefaultParameterGroupName() == name { + return engine, true + } + } + return Engine{}, false +} + +// v1 pins one engine major, so the family is the pinned one or nothing. An empty +// family takes the pin rather than failing, since a client that omits it can +// only mean the one family this platform offers. +func validateParameterGroupFamily(family string) (string, error) { + trimmed := strings.ToLower(strings.TrimSpace(family)) + if trimmed == "" { + return enginePostgres.ParameterGroupFamily(), nil + } + for _, engine := range engines { + if trimmed == engine.ParameterGroupFamily() { + return engine.ParameterGroupFamily(), nil + } + } + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBParameterGroupFamily %q is not offered; %s is the only supported family", family, enginePostgres.ParameterGroupFamily()) +} + +// The class DescribeDBParameters evaluates the size-derived defaults at. A group +// has no instance, so the smallest supported class is the honest choice: it is +// the one whose computed values a customer most needs to see before attaching +// the group to anything. +func describeParametersClass() string { + return smallestInstanceClass() +} + +func parameterSource(isOverride bool) string { + if isOverride { + return ParameterSourceUser + } + return ParameterSourceEngineDefault +} + +// A computed default is reported as engine-default with its literal value, never +// as the formula that produced it (D20). +func projectParameter(spec ParameterSpec, value string, isOverride bool) *rds.Parameter { + out := &rds.Parameter{ + ParameterName: aws.String(spec.Name), + ParameterValue: aws.String(value), + Description: aws.String(spec.Description), + Source: aws.String(parameterSource(isOverride)), + ApplyType: aws.String(spec.ApplyType), + DataType: aws.String(spec.DataType), + IsModifiable: aws.Bool(spec.IsModifiable), + } + if allowed := spec.AllowedValues(); allowed != "" { + out.AllowedValues = aws.String(allowed) + } + // AWS reports the method a change to this parameter would take, which for a + // static setting is the only one it can take. + if spec.ApplyType == ApplyTypeStatic { + out.ApplyMethod = aws.String(ApplyMethodPendingReboot) + } else { + out.ApplyMethod = aws.String(ApplyMethodImmediate) + } + return out +} + +func (s *Service) projectParameterGroupRecord(rec *DBParameterGroupRecord) *rds.DBParameterGroup { + if rec == nil { + return nil + } + return &rds.DBParameterGroup{ + DBParameterGroupName: aws.String(rec.Name), + DBParameterGroupFamily: aws.String(rec.Family), + DBParameterGroupArn: aws.String(DBParameterGroupARN(s.region, rec.AccountID, rec.Name)), + Description: aws.String(rec.Description), + } +} + +// The effective set an instance of this class runs with under this group: catalog +// defaults evaluated at the class, overlaid with the group's stored overrides. +// A group that does not exist fails here rather than silently resolving to the +// bare defaults, which would run a database on settings nobody asked for. +func (s *Service) resolveGroupParameters(ctx context.Context, kv jetstream.KeyValue, accountID, group, instanceClass string) ([]Parameter, error) { + if _, _, err := getDBParameterGroup(ctx, kv, accountID, group); err != nil { + return nil, err + } + overrides, err := ListDBParameterOverrides(ctx, kv, group) + if err != nil { + return nil, err + } + return ResolveEffectiveParameters(instanceClass, overrides) +} diff --git a/spinifex/handlers/rds/parametergroup_test.go b/spinifex/handlers/rds/parametergroup_test.go new file mode 100644 index 000000000..de8485bfb --- /dev/null +++ b/spinifex/handlers/rds/parametergroup_test.go @@ -0,0 +1,464 @@ +package handlers_rds + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testParameterGroup = "tuned-pg" + testDefaultPG = "default.postgres18" +) + +func parameterGroupInput(name string) *rds.CreateDBParameterGroupInput { + return &rds.CreateDBParameterGroupInput{ + DBParameterGroupName: aws.String(name), + DBParameterGroupFamily: aws.String("postgres18"), + Description: aws.String("Tuned for the orders workload"), + } +} + +func modifyParameters(name string, params ...*rds.Parameter) *rds.ModifyDBParameterGroupInput { + return &rds.ModifyDBParameterGroupInput{ + DBParameterGroupName: aws.String(name), + Parameters: params, + } +} + +func parameter(name, value, applyMethod string) *rds.Parameter { + p := &rds.Parameter{ParameterName: aws.String(name), ParameterValue: aws.String(value)} + if applyMethod != "" { + p.ApplyMethod = aws.String(applyMethod) + } + return p +} + +// The values of a described group, keyed by name, with the source each was +// reported under. +func describedParameters(t *testing.T, h *createHarness, group string) map[string]*rds.Parameter { + t.Helper() + out, err := h.svc.DescribeDBParameters(t.Context(), + &rds.DescribeDBParametersInput{DBParameterGroupName: aws.String(group)}, testAccountID) + require.NoError(t, err) + + byName := make(map[string]*rds.Parameter, len(out.Parameters)) + for _, param := range out.Parameters { + byName[aws.StringValue(param.ParameterName)] = param + } + return byName +} + +func TestCreateDBParameterGroup_StoresAnEmptyGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + out, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + group := out.DBParameterGroup + require.NotNil(t, group) + assert.Equal(t, testParameterGroup, aws.StringValue(group.DBParameterGroupName)) + assert.Equal(t, "postgres18", aws.StringValue(group.DBParameterGroupFamily)) + assert.Equal(t, DBParameterGroupARN(testRegion, testAccountID, testParameterGroup), + aws.StringValue(group.DBParameterGroupArn)) + + // A fresh group and the default group resolve to the same effective set, + // because a group holds overrides rather than a copy of the catalog. + params := describedParameters(t, h, testParameterGroup) + require.Len(t, params, len(CatalogParameterNames())) + for _, param := range params { + assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(param.Source)) + } +} + +// An omitted family can only mean the one family this platform offers, so it +// takes the pin rather than failing a Terraform config that leaves it out. +func TestCreateDBParameterGroup_DefaultsTheFamilyAndRejectsAnother(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + input := parameterGroupInput(testParameterGroup) + input.DBParameterGroupFamily = nil + out, err := h.svc.CreateDBParameterGroup(t.Context(), input, testAccountID) + require.NoError(t, err) + assert.Equal(t, "postgres18", aws.StringValue(out.DBParameterGroup.DBParameterGroupFamily)) + + other := parameterGroupInput("mysql-tuned") + other.DBParameterGroupFamily = aws.String("mysql8.0") + _, err = h.svc.CreateDBParameterGroup(t.Context(), other, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// A customer group under the reserved prefix would be indistinguishable from the +// implicit one, and would then be modifiable through a name that must not be. +func TestCreateDBParameterGroup_RejectsTheReservedPrefix(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput("default.postgres18"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), "default.") +} + +func TestCreateDBParameterGroup_RejectsADuplicateName(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + _, err = h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupAlreadyExists, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// The default group is what CreateDBInstance resolves when a request names none, +// so it has to be listable and readable without anyone having created it. +func TestDescribeDBParameterGroups_ReportsTheImplicitDefault(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + named, err := h.svc.DescribeDBParameterGroups(t.Context(), + &rds.DescribeDBParameterGroupsInput{DBParameterGroupName: aws.String(testDefaultPG)}, testAccountID) + require.NoError(t, err) + require.Len(t, named.DBParameterGroups, 1) + assert.Equal(t, testDefaultPG, aws.StringValue(named.DBParameterGroups[0].DBParameterGroupName)) + + listed, err := h.svc.DescribeDBParameterGroups(t.Context(), &rds.DescribeDBParameterGroupsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, listed.DBParameterGroups, 1, "the default group is reported even with nothing created") + assert.Equal(t, testDefaultPG, aws.StringValue(listed.DBParameterGroups[0].DBParameterGroupName)) +} + +// Synthesised rather than stored, so it must appear exactly once alongside the +// customer's own groups rather than twice or not at all. +func TestDescribeDBParameterGroups_ListsCustomerGroupsBesideTheDefault(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput("zeta"), testAccountID) + require.NoError(t, err) + _, err = h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput("alpha"), testAccountID) + require.NoError(t, err) + + listed, err := h.svc.DescribeDBParameterGroups(t.Context(), &rds.DescribeDBParameterGroupsInput{}, testAccountID) + require.NoError(t, err) + + var names []string + for _, group := range listed.DBParameterGroups { + names = append(names, aws.StringValue(group.DBParameterGroupName)) + } + assert.Equal(t, []string{"alpha", testDefaultPG, "zeta"}, names) +} + +func TestDescribeDBParameterGroups_RejectsAnUnknownName(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.DescribeDBParameterGroups(t.Context(), + &rds.DescribeDBParameterGroupsInput{DBParameterGroupName: aws.String("absent")}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + + // A default.* name for an engine that does not exist is a not-found too, + // rather than a group that resolves to nothing. + _, err = h.svc.DescribeDBParameterGroups(t.Context(), + &rds.DescribeDBParameterGroupsInput{DBParameterGroupName: aws.String("default.mysql8")}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +func TestModifyDBParameterGroup_StoresValidatedOverrides(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + out, err := h.svc.ModifyDBParameterGroup(t.Context(), modifyParameters(testParameterGroup, + parameter("work_mem", "16384", ""), + parameter("shared_buffers", "65536", ApplyMethodPendingReboot), + ), testAccountID) + require.NoError(t, err) + assert.Equal(t, testParameterGroup, aws.StringValue(out.DBParameterGroupName)) + + params := describedParameters(t, h, testParameterGroup) + assert.Equal(t, "16384", aws.StringValue(params["work_mem"].ParameterValue)) + assert.Equal(t, ParameterSourceUser, aws.StringValue(params["work_mem"].Source)) + assert.Equal(t, "65536", aws.StringValue(params["shared_buffers"].ParameterValue)) + assert.Equal(t, ParameterSourceUser, aws.StringValue(params["shared_buffers"].Source)) + // Untouched parameters stay engine defaults rather than becoming user values. + assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(params["autovacuum"].Source)) +} + +// A batch with one bad value must leave the group exactly as it was: a +// half-applied modify would be a configuration nobody asked for. +func TestModifyDBParameterGroup_WritesNothingWhenOneValueIsBad(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + _, err = h.svc.ModifyDBParameterGroup(t.Context(), modifyParameters(testParameterGroup, + parameter("work_mem", "16384", ""), + parameter("max_connections", "999999", ApplyMethodPendingReboot), + ), testAccountID) + require.Error(t, err) + + params := describedParameters(t, h, testParameterGroup) + assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(params["work_mem"].Source), + "the valid half of a rejected batch must not have landed") +} + +func TestModifyDBParameterGroup_RejectsBadRequests(t *testing.T) { + cases := []struct { + name string + input *rds.ModifyDBParameterGroupInput + want string + }{ + {"UnknownParameter", modifyParameters(testParameterGroup, parameter("not_a_setting", "1", "")), + "is not a parameter this engine exposes"}, + {"OutOfRange", modifyParameters(testParameterGroup, parameter("max_connections", "999999", "")), + "outside its allowed range"}, + {"Formula", modifyParameters(testParameterGroup, parameter("shared_buffers", "{DBInstanceClassMemory/32768}", "")), + "is a formula"}, + // Telling the customer a static change is live when the engine has not + // adopted it is worse than rejecting the request. + {"ImmediateOnAStaticParameter", modifyParameters(testParameterGroup, parameter("shared_buffers", "65536", ApplyMethodImmediate)), + "is static"}, + {"UnknownApplyMethod", modifyParameters(testParameterGroup, parameter("work_mem", "16384", "eventually")), + "ApplyMethod"}, + {"NoParameters", modifyParameters(testParameterGroup), "at least one parameter"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + _, err = h.svc.ModifyDBParameterGroup(t.Context(), tc.input, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), tc.want) + }) + } +} + +// AWS's own rule, and the one that keeps the default group a stable reference: +// an instance that names it must get the catalog's values, not a tenant's edits. +func TestModifyDBParameterGroup_RefusesTheDefaultGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters(testDefaultPG, parameter("work_mem", "16384", "")), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), "cannot be modified") +} + +func TestModifyDBParameterGroup_RejectsAnUnknownGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters("absent", parameter("work_mem", "16384", "")), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// D20: a computed default reaches the customer as the literal it evaluated to, +// never as the formula that produced it. +func TestDescribeDBParameters_ReportsComputedDefaultsAsLiterals(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + params := describedParameters(t, h, testDefaultPG) + shared := params["shared_buffers"] + require.NotNil(t, shared) + + // The class is named rather than read back from describeParametersClass, which + // would make this assert the code agrees with itself. + memoryMiB, err := classMemoryMiB("db.t3.micro") + require.NoError(t, err) + assert.Equal(t, sharedBuffersFor(memoryMiB), aws.StringValue(shared.ParameterValue)) + assert.Equal(t, ParameterSourceEngineDefault, aws.StringValue(shared.Source)) + assert.Equal(t, ApplyTypeStatic, aws.StringValue(shared.ApplyType)) + assert.Equal(t, ApplyMethodPendingReboot, aws.StringValue(shared.ApplyMethod)) + assert.True(t, aws.BoolValue(shared.IsModifiable)) + assert.NotEmpty(t, aws.StringValue(shared.AllowedValues)) +} + +func TestDescribeDBParameters_FiltersOnSource(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + _, err = h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters(testParameterGroup, parameter("work_mem", "16384", "")), testAccountID) + require.NoError(t, err) + + user, err := h.svc.DescribeDBParameters(t.Context(), &rds.DescribeDBParametersInput{ + DBParameterGroupName: aws.String(testParameterGroup), + Source: aws.String(ParameterSourceUser), + }, testAccountID) + require.NoError(t, err) + require.Len(t, user.Parameters, 1) + assert.Equal(t, "work_mem", aws.StringValue(user.Parameters[0].ParameterName)) + + defaults, err := h.svc.DescribeDBParameters(t.Context(), &rds.DescribeDBParametersInput{ + DBParameterGroupName: aws.String(testParameterGroup), + Source: aws.String(ParameterSourceEngineDefault), + }, testAccountID) + require.NoError(t, err) + assert.Len(t, defaults.Parameters, len(CatalogParameterNames())-1) +} + +func TestDeleteDBParameterGroup_RemovesTheGroupAndItsValues(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + _, err = h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters(testParameterGroup, parameter("work_mem", "16384", "")), testAccountID) + require.NoError(t, err) + + _, err = h.svc.DeleteDBParameterGroup(t.Context(), + &rds.DeleteDBParameterGroupInput{DBParameterGroupName: aws.String(testParameterGroup)}, testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + // Orphaned values would be silently inherited by a later group of the same + // name, which is a configuration nobody wrote. + overrides, err := ListDBParameterOverrides(t.Context(), kv, testParameterGroup) + require.NoError(t, err) + assert.Empty(t, overrides) + + _, err = h.svc.DescribeDBParameterGroups(t.Context(), + &rds.DescribeDBParameterGroupsInput{DBParameterGroupName: aws.String(testParameterGroup)}, testAccountID) + assert.Equal(t, awserrors.ErrorDBParameterGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +func TestDeleteDBParameterGroup_RefusesTheDefaultGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.DeleteDBParameterGroup(t.Context(), + &rds.DeleteDBParameterGroupInput{DBParameterGroupName: aws.String(testDefaultPG)}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupInvalidState, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// Including a group only a deleting instance still names, so a destroy that +// races the teardown fails cleanly rather than stranding a live engine. +func TestDeleteDBParameterGroup_RefusesWhileAnInstanceReferencesIt(t *testing.T) { + for _, status := range []Status{StatusAvailable, StatusDeleting} { + t.Run(string(status), func(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + input := validCreateInput() + input.DBParameterGroupName = aws.String(testParameterGroup) + _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + require.NoError(t, h.svc.updateInstance(t.Context(), kv, testDBInstanceID, func(rec *DBInstanceRecord) { + rec.Status = status + })) + + _, err = h.svc.DeleteDBParameterGroup(t.Context(), + &rds.DeleteDBParameterGroupInput{DBParameterGroupName: aws.String(testParameterGroup)}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupInvalidState, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), testDBInstanceID) + }) + } +} + +// The group is the thing bootstrap consumes, so a create has to resolve it into +// the literals the agent installs rather than leaving the set empty. +func TestCreateDBInstance_ResolvesTheNamedGroupIntoTheBootstrapSet(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + _, err = h.svc.ModifyDBParameterGroup(t.Context(), + modifyParameters(testParameterGroup, parameter("work_mem", "16384", "")), testAccountID) + require.NoError(t, err) + + input := validCreateInput() + input.DBParameterGroupName = aws.String(testParameterGroup) + _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + + rec := h.record(t, testDBInstanceID) + assert.Equal(t, testParameterGroup, rec.DBParameterGroupName) + require.Len(t, rec.Bootstrap.ResolvedParameters, len(CatalogParameterNames())) + + values := map[string]string{} + for _, param := range rec.Bootstrap.ResolvedParameters { + values[param.Name] = param.Value + } + assert.Equal(t, "16384", values["work_mem"]) + // Evaluated against the instance's own class, which is the whole point of a + // computed default: a literal tuned elsewhere would be wrong here. + memoryMiB, err := classMemoryMiB("db.t3.medium") + require.NoError(t, err) + assert.Equal(t, sharedBuffersFor(memoryMiB), values["shared_buffers"]) +} + +// An unnamed group takes the implicit default, which resolves to the catalog +// alone rather than to nothing. +func TestCreateDBInstance_ResolvesTheDefaultGroupWhenNoneIsNamed(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.CreateDBInstance(t.Context(), validCreateInput(), testAccountID) + require.NoError(t, err) + + rec := h.record(t, testDBInstanceID) + assert.Equal(t, testDefaultPG, rec.DBParameterGroupName) + assert.Len(t, rec.Bootstrap.ResolvedParameters, len(CatalogParameterNames())) +} + +func TestCreateDBInstance_RejectsAnUnknownParameterGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + input := validCreateInput() + input.DBParameterGroupName = aws.String("absent") + _, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBParameterGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.False(t, h.recordExists(t, testDBInstanceID), "a rejected create must reserve nothing") +} + +// Both groups are taggable through the one ARN-addressed mutate path, so the +// registry entries are what make an ARN a resource rather than a rejection. +func TestTagActions_ReachBothGroupTypes(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-alpha"), testAccountID) + require.NoError(t, err) + _, err = h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput(testParameterGroup), testAccountID) + require.NoError(t, err) + + for _, arn := range []string{ + DBSubnetGroupARN(testRegion, testAccountID, testSubnetGroup), + DBParameterGroupARN(testRegion, testAccountID, testParameterGroup), + } { + _, err := h.svc.AddTagsToResource(t.Context(), &rds.AddTagsToResourceInput{ + ResourceName: aws.String(arn), + Tags: []*rds.Tag{{Key: aws.String("env"), Value: aws.String("prod")}}, + }, testAccountID) + require.NoError(t, err, "%s", arn) + + listed, err := h.svc.ListTagsForResource(t.Context(), + &rds.ListTagsForResourceInput{ResourceName: aws.String(arn)}, testAccountID) + require.NoError(t, err) + require.Len(t, listed.TagList, 1) + assert.Equal(t, "prod", aws.StringValue(listed.TagList[0].Value)) + } +} diff --git a/spinifex/handlers/rds/pending.go b/spinifex/handlers/rds/pending.go index b74e3a6a7..8f46b106c 100644 --- a/spinifex/handlers/rds/pending.go +++ b/spinifex/handlers/rds/pending.go @@ -33,8 +33,21 @@ func (s *Service) applyPendingModifications(ctx context.Context, kv jetstream.Ke // Parameters first, while the engine this modify started against is still // the one running: the disruptive step below restarts it, which is also what // puts any statically-scoped setting into effect. - if pending.DBParameterGroupName != "" { - if err := s.applyParameterGroup(ctx, kv, accountID, rec, pending.DBParameterGroupName); err != nil { + // + // A class change re-derives every size-derived default, so the set is + // recomputed when either the group or the class moves (D20). It is resolved + // against the class the instance is becoming: the include lives on the data + // volume, so it is the replacement VM that adopts it. + if pending.DBParameterGroupName != "" || pending.DBInstanceClass != "" { + group := pending.DBParameterGroupName + if group == "" { + group = rec.DBParameterGroupName + } + class := pending.DBInstanceClass + if class == "" { + class = rec.DBInstanceClass + } + if err := s.applyParameterGroup(ctx, kv, accountID, rec, group, class); err != nil { return err } } @@ -91,16 +104,29 @@ func (s *Service) applyPendingModifications(ctx context.Context, kv jetstream.Ke }) } -// Installs the resolved parameter set into the engine's config and reloads it, +// Re-resolves the effective set and installs it into the engine's config, // recording the settings the engine accepted but will not honour until it // restarts (D16). The set lives on the data volume, so it survives the VM // replace a class change performs and needs no second apply afterwards. -func (s *Service) applyParameterGroup(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord, group string) error { - pendingReboot, err := s.applyParameters(ctx, accountID, rec.DBInstanceIdentifier, rec.Bootstrap.ResolvedParameters) +// +// A re-resolve, never a merge: the whole set is recomputed from the catalog and +// the new group's overrides, so a parameter the old group set and the new one +// does not reverts to its default rather than lingering. +func (s *Service) applyParameterGroup(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord, group, instanceClass string) error { + resolved, err := s.resolveGroupParameters(ctx, kv, accountID, group, instanceClass) + if err != nil { + return err + } + pendingReboot, err := s.applyParameters(ctx, accountID, rec.DBInstanceIdentifier, resolved) if err != nil { return fmt.Errorf("apply the parameters of %s to %s: %w", group, rec.DBInstanceIdentifier, err) } + // Stored so a later VM replace boots against the same set the engine is + // already running, rather than re-deriving it from a group that may since + // have changed underneath the instance. + rec.Bootstrap.ResolvedParameters = resolved return s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + stored.Bootstrap.ResolvedParameters = resolved stored.PendingRebootParameters = pendingReboot }) } diff --git a/spinifex/handlers/rds/pending_test.go b/spinifex/handlers/rds/pending_test.go index e87129276..83ce9364d 100644 --- a/spinifex/handlers/rds/pending_test.go +++ b/spinifex/handlers/rds/pending_test.go @@ -261,3 +261,75 @@ func TestReconciler_RetriesAFailedModifyInsideTheBudget(t *testing.T) { require.NoError(t, h.rec.reconcileOnce(t.Context())) assert.Equal(t, int64(50), h.record(t).AllocatedStorage) } + +// D20: the size-derived defaults are the reason a class change has to re-resolve +// rather than carry the old set forward — a shared_buffers computed for the old +// class is wrong at the new one in whichever direction the class moved. +func TestApplyPendingModifications_ReResolvesTheParametersForTheNewClass(t *testing.T) { + h := newModifyHarness(t) + h.agent.replyWith("shared_buffers") + rec := modifyingRecord(&PendingModifiedValues{ + DBInstanceClass: "db.m5.xlarge", + RequestedAt: time.Now().UTC(), + }) + seedReplaceable(t, h, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + issued := h.agent.received() + require.NotEmpty(t, issued) + assert.Equal(t, CommandApplyParams, issued[0].Type) + + applied := map[string]string{} + for _, param := range issued[0].Parameters { + applied[param.Name] = param.Value + } + memoryMiB, err := classMemoryMiB("db.m5.xlarge") + require.NoError(t, err) + assert.Equal(t, sharedBuffersFor(memoryMiB), applied["shared_buffers"], + "the set was resolved against the class the instance is becoming") + + stored := h.record(t) + assert.Equal(t, applied["shared_buffers"], parameterValue(stored.Bootstrap.ResolvedParameters, "shared_buffers"), + "the stored set has to match what the engine was given, or the next replace boots on something else") +} + +// A re-resolve rather than a merge: a parameter the old group set and the new +// one does not reverts to its catalog default instead of lingering. +func TestApplyPendingModifications_ParameterGroupChangeRevertsTheOldGroupsValues(t *testing.T) { + h := newModifyHarness(t) + h.agent.replyWith("") + + _, err := h.svc.CreateDBParameterGroup(t.Context(), parameterGroupInput("lean"), testAccountID) + require.NoError(t, err) + + rec := modifyingRecord(&PendingModifiedValues{ + DBParameterGroupName: "lean", + RequestedAt: time.Now().UTC(), + }) + // What the old group had set, carried on the record as the engine's current + // values; the new group sets nothing. + rec.Bootstrap.ResolvedParameters = []Parameter{{Name: "work_mem", Value: "262144"}} + seedInstance(t, h.svc, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + issued := h.agent.received() + require.NotEmpty(t, issued) + applied := map[string]string{} + for _, param := range issued[0].Parameters { + applied[param.Name] = param.Value + } + workMem, _ := LookupParameter("work_mem") + assert.Equal(t, workMem.Default, applied["work_mem"], + "the old group's value should have reverted to the catalog default") +} + +func parameterValue(params []Parameter, name string) string { + for _, param := range params { + if param.Name == name { + return param.Value + } + } + return "" +} diff --git a/spinifex/handlers/rds/reconciler.go b/spinifex/handlers/rds/reconciler.go index 7e45ea582..faa96e357 100644 --- a/spinifex/handlers/rds/reconciler.go +++ b/spinifex/handlers/rds/reconciler.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go/jetstream" ) @@ -194,7 +195,7 @@ func (r *Reconciler) reconcileAccount(ctx context.Context, kv jetstream.KeyValue var failures []error for _, id := range ids { if err := r.reconcileInstance(ctx, kv, accountID, id); err != nil { - failures = append(failures, fmt.Errorf("%s: %w", id, err)) + failures = append(failures, awserrors.Errorf(id, "%w", err)) } } return errors.Join(failures...) diff --git a/spinifex/handlers/rds/records.go b/spinifex/handlers/rds/records.go index 0686812a0..cf4b71e30 100644 --- a/spinifex/handlers/rds/records.go +++ b/spinifex/handlers/rds/records.go @@ -244,6 +244,68 @@ type Parameter struct { Value string `json:"value" locationName:"Value"` } +// The db-subnet-groups/{name} record. The subnet list is stored verbatim rather +// than reduced to a placement, so when V2 makes AZs real the group needs no +// migration — only the code that chooses among its subnets changes. +type DBSubnetGroupRecord struct { + Name string `json:"name"` + AccountID string `json:"accountId"` + Description string `json:"description"` + // Every subnet the customer supplied, in request order, each with the AZ + // recorded on the subnet itself rather than a hardcoded zone. + Subnets []DBSubnetGroupSubnet `json:"subnets"` + // The one VPC they all share, which is what makes the group usable for a + // placement at all. + VpcID string `json:"vpcId"` + + Tags map[string]string `json:"tags,omitempty"` + + CreatedAt time.Time `json:"createdAt"` + UpdatedAt time.Time `json:"updatedAt"` +} + +type DBSubnetGroupSubnet struct { + SubnetID string `json:"subnetId"` + AvailabilityZone string `json:"availabilityZone,omitempty"` +} + +var _ TaggedRecord = (*DBSubnetGroupRecord)(nil) + +func (r *DBSubnetGroupRecord) GetTags() map[string]string { return r.Tags } + +func (r *DBSubnetGroupRecord) SetTags(tags map[string]string) { r.Tags = tags } + +// The db-parameter-groups/{name}/meta record. The values themselves live one key +// each under .../params/, so a modify touching one parameter cannot clobber a +// concurrent change to another (D3). +type DBParameterGroupRecord struct { + Name string `json:"name"` + AccountID string `json:"accountId"` + Family string `json:"family"` + Description string `json:"description"` + + Tags map[string]string `json:"tags,omitempty"` + + CreatedAt time.Time `json:"createdAt"` + UpdatedAt time.Time `json:"updatedAt"` +} + +var _ TaggedRecord = (*DBParameterGroupRecord)(nil) + +func (r *DBParameterGroupRecord) GetTags() map[string]string { return r.Tags } + +func (r *DBParameterGroupRecord) SetTags(tags map[string]string) { r.Tags = tags } + +// One stored override, at db-parameter-groups/{name}/params/{key}. ApplyMethod +// is the customer's request rather than a fact: whether a change lands live is +// decided by the parameter's own ApplyType (D16). +type DBParameterRecord struct { + Name string `json:"name"` + Value string `json:"value"` + ApplyMethod string `json:"applyMethod,omitempty"` + UpdatedAt time.Time `json:"updatedAt"` +} + // Separate from Status: the reconciler needs both to tell "stopped because we // stopped it" from "stopped because it died". type EngineHealth string diff --git a/spinifex/handlers/rds/service_nats.go b/spinifex/handlers/rds/service_nats.go index fca515d24..6f93c0a64 100644 --- a/spinifex/handlers/rds/service_nats.go +++ b/spinifex/handlers/rds/service_nats.go @@ -101,6 +101,46 @@ func (s *NATSService) ListTagsForResource(ctx context.Context, input *rds.ListTa SubjectListTagsForResource, input, defaultTimeout, accountID) } +func (s *NATSService) CreateDBSubnetGroup(ctx context.Context, input *rds.CreateDBSubnetGroupInput, accountID string) (*rds.CreateDBSubnetGroupOutput, error) { + return utils.NATSRequest[rds.CreateDBSubnetGroupOutput](ctx, s.nc, + SubjectCreateDBSubnetGroup, input, defaultTimeout, accountID) +} + +func (s *NATSService) DescribeDBSubnetGroups(ctx context.Context, input *rds.DescribeDBSubnetGroupsInput, accountID string) (*rds.DescribeDBSubnetGroupsOutput, error) { + return utils.NATSRequest[rds.DescribeDBSubnetGroupsOutput](ctx, s.nc, + SubjectDescribeDBSubnetGroups, input, defaultTimeout, accountID) +} + +func (s *NATSService) DeleteDBSubnetGroup(ctx context.Context, input *rds.DeleteDBSubnetGroupInput, accountID string) (*rds.DeleteDBSubnetGroupOutput, error) { + return utils.NATSRequest[rds.DeleteDBSubnetGroupOutput](ctx, s.nc, + SubjectDeleteDBSubnetGroup, input, defaultTimeout, accountID) +} + +func (s *NATSService) CreateDBParameterGroup(ctx context.Context, input *rds.CreateDBParameterGroupInput, accountID string) (*rds.CreateDBParameterGroupOutput, error) { + return utils.NATSRequest[rds.CreateDBParameterGroupOutput](ctx, s.nc, + SubjectCreateDBParameterGroup, input, defaultTimeout, accountID) +} + +func (s *NATSService) DescribeDBParameterGroups(ctx context.Context, input *rds.DescribeDBParameterGroupsInput, accountID string) (*rds.DescribeDBParameterGroupsOutput, error) { + return utils.NATSRequest[rds.DescribeDBParameterGroupsOutput](ctx, s.nc, + SubjectDescribeDBParameterGroups, input, defaultTimeout, accountID) +} + +func (s *NATSService) ModifyDBParameterGroup(ctx context.Context, input *rds.ModifyDBParameterGroupInput, accountID string) (*rds.DBParameterGroupNameMessage, error) { + return utils.NATSRequest[rds.DBParameterGroupNameMessage](ctx, s.nc, + SubjectModifyDBParameterGroup, input, defaultTimeout, accountID) +} + +func (s *NATSService) DescribeDBParameters(ctx context.Context, input *rds.DescribeDBParametersInput, accountID string) (*rds.DescribeDBParametersOutput, error) { + return utils.NATSRequest[rds.DescribeDBParametersOutput](ctx, s.nc, + SubjectDescribeDBParameters, input, defaultTimeout, accountID) +} + +func (s *NATSService) DeleteDBParameterGroup(ctx context.Context, input *rds.DeleteDBParameterGroupInput, accountID string) (*rds.DeleteDBParameterGroupOutput, error) { + return utils.NATSRequest[rds.DeleteDBParameterGroupOutput](ctx, s.nc, + SubjectDeleteDBParameterGroup, input, defaultTimeout, accountID) +} + // Requested on the Layer-1 subject, not the bus. func (s *NATSService) GetDBBootstrapConfig(ctx context.Context, input *GetDBBootstrapConfigInput, accountID string) (*GetDBBootstrapConfigOutput, error) { return utils.NATSRequest[GetDBBootstrapConfigOutput](ctx, s.nc, diff --git a/spinifex/handlers/rds/sizing.go b/spinifex/handlers/rds/sizing.go index bad88afdf..0c1fe8870 100644 --- a/spinifex/handlers/rds/sizing.go +++ b/spinifex/handlers/rds/sizing.go @@ -36,3 +36,22 @@ func InstanceTypeForClass(class string) (string, error) { func SupportedInstanceClasses() []string { return slices.Sorted(maps.Keys(dbInstanceClasses)) } + +// The supported class with the least memory. Deliberately computed from the +// footprints rather than taken as the first name SupportedInstanceClasses +// reports: that list is sorted alphabetically, so its head is db.m5.large. +func smallestInstanceClass() string { + smallest, least := "", int64(0) + for _, class := range SupportedInstanceClasses() { + memoryMiB, err := classMemoryMiB(class) + // A class with no known footprint cannot be compared; the catalog's + // self-consistency test pins that every supported class has one. + if err != nil { + continue + } + if smallest == "" || memoryMiB < least { + smallest, least = class, memoryMiB + } + } + return smallest +} diff --git a/spinifex/handlers/rds/sizing_test.go b/spinifex/handlers/rds/sizing_test.go index c56949791..ac1aa9f4a 100644 --- a/spinifex/handlers/rds/sizing_test.go +++ b/spinifex/handlers/rds/sizing_test.go @@ -56,3 +56,19 @@ func TestSupportedInstanceClasses_SortedAndComplete(t *testing.T) { "db.t3.large", "db.t3.medium", "db.t3.micro", "db.t3.small", }, SupportedInstanceClasses()) } + +// The literal matters: DescribeDBParameters reports its size-derived defaults at +// this class, and the alphabetically first class is db.m5.large — eight times the +// memory, so reading the head of the sorted list would advertise a shared_buffers +// no small instance ever runs. +func TestSmallestInstanceClass_IsTheLeastMemory(t *testing.T) { + assert.Equal(t, "db.t3.micro", smallestInstanceClass()) + + least, err := classMemoryMiB(smallestInstanceClass()) + require.NoError(t, err) + for _, class := range SupportedInstanceClasses() { + memoryMiB, err := classMemoryMiB(class) + require.NoError(t, err, "every supported class needs a known footprint") + assert.LessOrEqual(t, least, memoryMiB, "class %q is smaller than the one reported as smallest", class) + } +} diff --git a/spinifex/handlers/rds/storage.go b/spinifex/handlers/rds/storage.go index 5c48b3cce..eb0bd2533 100644 --- a/spinifex/handlers/rds/storage.go +++ b/spinifex/handlers/rds/storage.go @@ -33,11 +33,11 @@ type volumeResizer interface { func validateStorageGrow(current, requested int64) error { switch { case requested < minAllocatedStorageGiB || requested > maxAllocatedStorageGiB: - return fmt.Errorf("%s: AllocatedStorage must be between %d and %d GiB", - awserrors.ErrorInvalidParameterValue, minAllocatedStorageGiB, maxAllocatedStorageGiB) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "AllocatedStorage must be between %d and %d GiB", minAllocatedStorageGiB, maxAllocatedStorageGiB) case requested < current: - return fmt.Errorf("%s: AllocatedStorage cannot be reduced from %d to %d GiB; storage is grow-only", - awserrors.ErrorInvalidParameterCombination, current, requested) + return awserrors.Errorf(awserrors.ErrorInvalidParameterCombination, + "AllocatedStorage cannot be reduced from %d to %d GiB; storage is grow-only", current, requested) } return nil } diff --git a/spinifex/handlers/rds/store.go b/spinifex/handlers/rds/store.go index 090df537a..f40f6f4fc 100644 --- a/spinifex/handlers/rds/store.go +++ b/spinifex/handlers/rds/store.go @@ -228,14 +228,65 @@ func AccountIDFromBucketName(bucket string) string { // The DB instance identifiers held in one account bucket. An empty bucket // yields no names rather than an error. func ListDBInstanceIDs(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { - keys, err := kv.Keys(ctx) + return listNames(ctx, kv, DBInstancesPrefix()) +} + +func ListDBSubnetGroupNames(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { + return listNames(ctx, kv, DBSubnetGroupsPrefix()) +} + +// Walks the .../meta keys, which is what makes a group's own record findable +// among the per-parameter keys hanging off the same prefix. +func ListDBParameterGroupNames(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { + keys, err := bucketKeys(ctx, kv) if err != nil { - if errors.Is(err, jetstream.ErrNoKeysFound) { - return nil, nil - } - return nil, fmt.Errorf("rds: list keys: %w", err) + return nil, err + } + prefix := DBParameterGroupsPrefix() + names := make([]string, 0, len(keys)) + for _, key := range keys { + if !strings.HasPrefix(key, prefix) || !strings.HasSuffix(key, "/meta") { + continue + } + name := strings.TrimSuffix(strings.TrimPrefix(key, prefix), "/meta") + if name == "" || strings.Contains(name, "/") { + continue + } + names = append(names, name) + } + return names, nil +} + +// The stored overrides of one parameter group, keyed by parameter name. +func ListDBParameterOverrides(ctx context.Context, kv jetstream.KeyValue, group string) (map[string]string, error) { + prefix := DBParameterGroupParamsPrefix(group) + names, err := listNames(ctx, kv, prefix) + if err != nil { + return nil, err + } + out := make(map[string]string, len(names)) + for _, name := range names { + var rec DBParameterRecord + found, err := getJSON(ctx, kv, prefix+name, &rec) + if err != nil { + return nil, err + } + // A parameter reset between the listing and this read is simply gone, + // which is the answer a resolve one tick later would give too. + if found { + out[name] = rec.Value + } + } + return out, nil +} + +// The leaf names directly under prefix. A nested key belongs to a sub-space and +// is skipped rather than reported as a malformed name. +func listNames(ctx context.Context, kv jetstream.KeyValue, prefix string) ([]string, error) { + keys, err := bucketKeys(ctx, kv) + if err != nil { + return nil, err } - prefix := DBInstancesPrefix() names := make([]string, 0, len(keys)) for _, key := range keys { if !strings.HasPrefix(key, prefix) { @@ -249,3 +300,16 @@ func ListDBInstanceIDs(ctx context.Context, kv jetstream.KeyValue) ([]string, er } return names, nil } + +// An empty bucket yields no keys rather than an error, so a first-ever describe +// answers with an empty list instead of failing. +func bucketKeys(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { + keys, err := kv.Keys(ctx) + if err != nil { + if errors.Is(err, jetstream.ErrNoKeysFound) { + return nil, nil + } + return nil, fmt.Errorf("rds: list keys: %w", err) + } + return keys, nil +} diff --git a/spinifex/handlers/rds/subjects.go b/spinifex/handlers/rds/subjects.go index 8aa28b55c..0307199cb 100644 --- a/spinifex/handlers/rds/subjects.go +++ b/spinifex/handlers/rds/subjects.go @@ -32,6 +32,18 @@ const ( SubjectDescribeEvents = "rds.DescribeEvents" + // Configuration resources. Plain KV CRUD, so unlike the instance actions + // none of them drives a VM and all take the default budget. + SubjectCreateDBSubnetGroup = "rds.CreateDBSubnetGroup" + SubjectDescribeDBSubnetGroups = "rds.DescribeDBSubnetGroups" + SubjectDeleteDBSubnetGroup = "rds.DeleteDBSubnetGroup" + + SubjectCreateDBParameterGroup = "rds.CreateDBParameterGroup" + SubjectDescribeDBParameterGroups = "rds.DescribeDBParameterGroups" + SubjectModifyDBParameterGroup = "rds.ModifyDBParameterGroup" + SubjectDescribeDBParameters = "rds.DescribeDBParameters" + SubjectDeleteDBParameterGroup = "rds.DeleteDBParameterGroup" + // Tag actions address the resource by ARN, so one subject serves every // resource type rather than one per kind. SubjectAddTagsToResource = "rds.AddTagsToResource" diff --git a/spinifex/handlers/rds/subnetgroup.go b/spinifex/handlers/rds/subnetgroup.go new file mode 100644 index 000000000..010d3dd7b --- /dev/null +++ b/spinifex/handlers/rds/subnetgroup.go @@ -0,0 +1,328 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + "slices" + "strings" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go/jetstream" +) + +// AWS's own name rules. The name is a KV key rather than a DNS label, so the +// looser AWS character set is accepted as-is — but "default" is reserved. +const ( + maxDBGroupNameLen = 255 + maxDBGroupDescriptionLen = 255 + // AWS's cap on the subnets one group may hold. + maxSubnetsPerGroup = 20 +) + +// The status AWS reports on a usable group. There is no asynchronous +// provisioning behind a subnet group here, so it is complete the moment it is +// written. +const subnetGroupStatusComplete = "Complete" + +// AWS requires a DB subnet group to span at least two AZs. This platform is +// single-AZ — every subnet reports the one zone — and multi-AZ is a V2 +// milestone, so the AZ-count rule is not enforced: rejecting on it would fail +// every stock Terraform module for no safety benefit. The constraint that does +// matter is enforced below, because a group spanning two VPCs cannot host an +// instance at all. +func (s *Service) CreateDBSubnetGroup(ctx context.Context, input *rds.CreateDBSubnetGroupInput, accountID string) (*rds.CreateDBSubnetGroupOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBSubnetGroupName) + if err := validateDBGroupName("DBSubnetGroupName", name); err != nil { + return nil, err + } + description := aws.StringValue(input.DBSubnetGroupDescription) + if err := validateDBGroupDescription("DBSubnetGroupDescription", description); err != nil { + return nil, err + } + tags, err := validateTags(input.Tags) + if err != nil { + return nil, err + } + + subnets, vpcID, err := s.resolveGroupSubnets(ctx, accountID, aws.StringValueSlice(input.SubnetIds)) + if err != nil { + return nil, err + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + now := time.Now().UTC() + rec := DBSubnetGroupRecord{ + Name: name, + AccountID: accountID, + Description: description, + Subnets: subnets, + VpcID: vpcID, + Tags: tags, + CreatedAt: now, + UpdatedAt: now, + } + // Create rather than Put, so two concurrent creates of one name have exactly + // one winner instead of both reporting success over each other's subnets. + if err := createJSON(ctx, kv, DBSubnetGroupKey(name), &rec); err != nil { + if errors.Is(err, jetstream.ErrKeyExists) { + return nil, awserrors.Errorf(awserrors.ErrorDBSubnetGroupAlreadyExists, + "DB subnet group %s already exists", name) + } + return nil, err + } + + slog.InfoContext(ctx, "rds: DB subnet group created", + "dbSubnetGroup", name, "accountId", accountID, "vpcId", vpcID, "subnets", len(subnets)) + return &rds.CreateDBSubnetGroupOutput{DBSubnetGroup: s.projectSubnetGroup(&rec)}, nil +} + +// A named group that does not exist is an error, matching AWS; an unnamed +// request lists the account's groups. +func (s *Service) DescribeDBSubnetGroups(ctx context.Context, input *rds.DescribeDBSubnetGroupsInput, accountID string) (*rds.DescribeDBSubnetGroupsOutput, error) { + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if input != nil { + if name := aws.StringValue(input.DBSubnetGroupName); name != "" { + rec, _, err := getDBSubnetGroup(ctx, kv, name) + if err != nil { + return nil, err + } + return &rds.DescribeDBSubnetGroupsOutput{DBSubnetGroups: []*rds.DBSubnetGroup{s.projectSubnetGroup(rec)}}, nil + } + } + + names, err := ListDBSubnetGroupNames(ctx, kv) + if err != nil { + return nil, err + } + slices.Sort(names) + + groups := make([]*rds.DBSubnetGroup, 0, len(names)) + for _, name := range names { + var rec DBSubnetGroupRecord + found, err := getJSON(ctx, kv, DBSubnetGroupKey(name), &rec) + if err != nil { + return nil, err + } + if !found { + continue + } + groups = append(groups, s.projectSubnetGroup(&rec)) + } + return &rds.DescribeDBSubnetGroupsOutput{DBSubnetGroups: groups}, nil +} + +// Refused while any instance still names the group, including one that is only +// deleting: releasing it early would let a teardown lose the record of where its +// ENI was placed, and would make destroy ordering ambiguous. +func (s *Service) DeleteDBSubnetGroup(ctx context.Context, input *rds.DeleteDBSubnetGroupInput, accountID string) (*rds.DeleteDBSubnetGroupOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + name := aws.StringValue(input.DBSubnetGroupName) + if name == "" { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBSubnetGroupName is required") + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + if _, _, err := getDBSubnetGroup(ctx, kv, name); err != nil { + return nil, err + } + users, err := instancesUsingGroup(ctx, kv, func(rec *DBInstanceRecord) bool { + return rec.DBSubnetGroupName == name + }) + if err != nil { + return nil, err + } + if len(users) > 0 { + return nil, awserrors.Errorf(awserrors.ErrorDBSubnetGroupInvalidState, + "DB subnet group %s is still used by %s", name, strings.Join(users, ", ")) + } + + if err := kv.Delete(ctx, DBSubnetGroupKey(name)); err != nil { + return nil, fmt.Errorf("rds: delete DB subnet group %s: %w", name, err) + } + slog.InfoContext(ctx, "rds: DB subnet group deleted", "dbSubnetGroup", name, "accountId", accountID) + return &rds.DeleteDBSubnetGroupOutput{}, nil +} + +// Validates the requested subnets against the caller's own account and returns +// them with the AZ each subnet itself records. The describe is issued as the +// caller, so a subnet in another account simply does not come back and is +// reported as not found rather than leaked. +func (s *Service) resolveGroupSubnets(ctx context.Context, accountID string, requested []string) ([]DBSubnetGroupSubnet, string, error) { + if s.deps.Network == nil { + return nil, "", awserrors.Errorf(awserrors.ErrorServerInternal, "RDS networking is not wired on this node") + } + switch { + case len(requested) == 0: + return nil, "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "SubnetIds must name at least one subnet") + case len(requested) > maxSubnetsPerGroup: + return nil, "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "a DB subnet group may hold at most %d subnets, got %d", maxSubnetsPerGroup, len(requested)) + } + + out, err := s.deps.Network.DescribeSubnets(ctx, &ec2.DescribeSubnetsInput{ + SubnetIds: aws.StringSlice(requested), + }, accountID) + if err != nil { + return nil, "", fmt.Errorf("rds: describe the subnets of the DB subnet group: %w", err) + } + + found := map[string]*ec2.Subnet{} + if out != nil { + for _, subnet := range out.Subnets { + if id := aws.StringValue(subnet.SubnetId); id != "" { + found[id] = subnet + } + } + } + + vpcID := "" + subnets := make([]DBSubnetGroupSubnet, 0, len(requested)) + seen := make(map[string]bool, len(requested)) + for _, id := range requested { + if seen[id] { + return nil, "", awserrors.Errorf(awserrors.ErrorDBSubnetInvalid, "subnet %s is named more than once", id) + } + seen[id] = true + + subnet, ok := found[id] + if !ok { + return nil, "", awserrors.Errorf(awserrors.ErrorDBSubnetInvalid, + "subnet %s does not exist in this account", id) + } + subnetVPC := aws.StringValue(subnet.VpcId) + if vpcID == "" { + vpcID = subnetVPC + } + if subnetVPC != vpcID { + return nil, "", awserrors.Errorf(awserrors.ErrorDBSubnetInvalid, + "subnet %s is in VPC %s while the group's other subnets are in %s; "+ + "a DB subnet group must span one VPC", id, subnetVPC, vpcID) + } + // Read off the subnet rather than stamped from the platform's single zone, + // so the response needs no change when V2 makes AZs real. + subnets = append(subnets, DBSubnetGroupSubnet{ + SubnetID: id, + AvailabilityZone: aws.StringValue(subnet.AvailabilityZone), + }) + } + if vpcID == "" { + return nil, "", awserrors.Errorf(awserrors.ErrorDBSubnetInvalid, "the named subnets report no VPC") + } + return subnets, vpcID, nil +} + +// The record plus its revision. A missing group raises AWS's own fault, so a +// well-formed name that resolves to nothing is distinguishable from a bad one. +func getDBSubnetGroup(ctx context.Context, kv jetstream.KeyValue, name string) (*DBSubnetGroupRecord, uint64, error) { + var rec DBSubnetGroupRecord + rev, found, err := getJSONRevision(ctx, kv, DBSubnetGroupKey(name), &rec) + if err != nil { + return nil, 0, err + } + if !found { + return nil, 0, awserrors.Errorf(awserrors.ErrorDBSubnetGroupNotFound, "DB subnet group %s not found", name) + } + return &rec, rev, nil +} + +// The identifiers of every instance in the account matching uses, sorted. Both +// group deletes share it: an in-use guard that missed one instance would strand +// a live database's configuration. +func instancesUsingGroup(ctx context.Context, kv jetstream.KeyValue, uses func(*DBInstanceRecord) bool) ([]string, error) { + ids, err := ListDBInstanceIDs(ctx, kv) + if err != nil { + return nil, err + } + slices.Sort(ids) + + var users []string + for _, id := range ids { + var rec DBInstanceRecord + found, err := getJSON(ctx, kv, DBInstanceKey(id), &rec) + if err != nil { + return nil, err + } + if found && uses(&rec) { + users = append(users, id) + } + } + return users, nil +} + +func (s *Service) projectSubnetGroup(rec *DBSubnetGroupRecord) *rds.DBSubnetGroup { + if rec == nil { + return nil + } + out := &rds.DBSubnetGroup{ + DBSubnetGroupName: aws.String(rec.Name), + DBSubnetGroupDescription: aws.String(rec.Description), + DBSubnetGroupArn: aws.String(DBSubnetGroupARN(s.region, rec.AccountID, rec.Name)), + SubnetGroupStatus: aws.String(subnetGroupStatusComplete), + VpcId: aws.String(rec.VpcID), + } + for _, subnet := range rec.Subnets { + member := &rds.Subnet{ + SubnetIdentifier: aws.String(subnet.SubnetID), + SubnetStatus: aws.String("Active"), + } + if subnet.AvailabilityZone != "" { + member.SubnetAvailabilityZone = &rds.AvailabilityZone{Name: aws.String(subnet.AvailabilityZone)} + } + out.Subnets = append(out.Subnets, member) + } + return out +} + +// Shared by both group types: AWS applies the same name rules to each, and +// "default" is reserved on both because a default parameter group is implicit. +func validateDBGroupName(field, name string) error { + switch { + case name == "": + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s is required", field) + case len(name) > maxDBGroupNameLen: + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "%s must be at most %d characters", field, maxDBGroupNameLen) + case !isLetter(rune(name[0])): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s must begin with a letter", field) + case strings.EqualFold(name, "default"): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "%s may not be \"default\", which the service reserves", field) + // A slash would split the name across the KV key layout's own separator, so + // one group's parameters would land under another's prefix. + case strings.ContainsAny(name, "/ \t\n"): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s may not contain slashes or whitespace", field) + } + return nil +} + +func validateDBGroupDescription(field, description string) error { + switch { + case description == "": + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s is required", field) + case len(description) > maxDBGroupDescriptionLen: + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "%s must be at most %d characters", field, maxDBGroupDescriptionLen) + } + return nil +} diff --git a/spinifex/handlers/rds/subnetgroup_test.go b/spinifex/handlers/rds/subnetgroup_test.go new file mode 100644 index 000000000..58bd083ef --- /dev/null +++ b/spinifex/handlers/rds/subnetgroup_test.go @@ -0,0 +1,293 @@ +package handlers_rds + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testSubnetGroup = "db-private" + +func subnetGroupInput(name string, subnetIDs ...string) *rds.CreateDBSubnetGroupInput { + return &rds.CreateDBSubnetGroupInput{ + DBSubnetGroupName: aws.String(name), + DBSubnetGroupDescription: aws.String("Database subnets"), + SubnetIds: aws.StringSlice(subnetIDs), + } +} + +// Single-AZ groups are valid here: AWS's two-AZ rule would fail every stock +// Terraform module against a platform that exposes one zone, and rejecting it +// buys no safety. +func TestCreateDBSubnetGroup_AcceptsASingleSubnet(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + out, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-alpha"), testAccountID) + require.NoError(t, err) + + group := out.DBSubnetGroup + require.NotNil(t, group) + assert.Equal(t, testSubnetGroup, aws.StringValue(group.DBSubnetGroupName)) + assert.Equal(t, DBSubnetGroupARN(testRegion, testAccountID, testSubnetGroup), + aws.StringValue(group.DBSubnetGroupArn)) + assert.Equal(t, testDefaultVPC, aws.StringValue(group.VpcId)) + require.Len(t, group.Subnets, 1) + assert.Equal(t, "subnet-alpha", aws.StringValue(group.Subnets[0].SubnetIdentifier)) + // Reported from the subnet's own record rather than a hardcoded zone, so the + // response shape survives V2 making AZs real. + require.NotNil(t, group.Subnets[0].SubnetAvailabilityZone) + assert.Equal(t, testZone, aws.StringValue(group.Subnets[0].SubnetAvailabilityZone.Name)) +} + +// Stored verbatim: the group keeps every subnet the customer supplied, so the +// placement logic can change later without a record migration. +func TestCreateDBSubnetGroup_StoresEverySubnetSupplied(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.CreateDBSubnetGroup(t.Context(), + subnetGroupInput(testSubnetGroup, "subnet-zebra", "subnet-alpha"), testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + rec, _, err := getDBSubnetGroup(t.Context(), kv, testSubnetGroup) + require.NoError(t, err) + + require.Len(t, rec.Subnets, 2) + assert.Equal(t, "subnet-zebra", rec.Subnets[0].SubnetID, "the request's own order is preserved") + assert.Equal(t, "subnet-alpha", rec.Subnets[1].SubnetID) + assert.Equal(t, testDefaultVPC, rec.VpcID) +} + +func TestCreateDBSubnetGroup_RejectsAMissingSubnet(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.CreateDBSubnetGroup(t.Context(), + subnetGroupInput(testSubnetGroup, "subnet-alpha", "subnet-nowhere"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetInvalid, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), "subnet-nowhere") +} + +// The check that actually matters: a group spanning two VPCs cannot host an +// instance at all, because the endpoint ENI lives in exactly one of them. +func TestCreateDBSubnetGroup_RejectsACrossVPCSet(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + h.network.subnets = append(h.network.subnets, &ec2.Subnet{ + SubnetId: aws.String("subnet-other"), + VpcId: aws.String("vpc-other01"), + AvailabilityZone: aws.String(testZone), + }) + + _, err := h.svc.CreateDBSubnetGroup(t.Context(), + subnetGroupInput(testSubnetGroup, "subnet-alpha", "subnet-other"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetInvalid, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), "must span one VPC") +} + +// The describe is issued as the caller, so another account's subnet does not +// come back at all — it is reported as invalid rather than leaked or accepted. +func TestCreateDBSubnetGroup_RejectsACrossAccountSubnet(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.CreateDBSubnetGroup(t.Context(), + subnetGroupInput(testSubnetGroup, "subnet-foreign"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetInvalid, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Equal(t, []string{testAccountID}, h.network.accts, + "the subnet describe must be issued against the caller's own account") +} + +func TestCreateDBSubnetGroup_RejectsMalformedRequests(t *testing.T) { + cases := []struct { + name string + mutate func(*rds.CreateDBSubnetGroupInput) + want string + }{ + {"NoName", func(in *rds.CreateDBSubnetGroupInput) { in.DBSubnetGroupName = nil }, awserrors.ErrorInvalidParameterValue}, + {"ReservedName", func(in *rds.CreateDBSubnetGroupInput) { in.DBSubnetGroupName = aws.String("default") }, awserrors.ErrorInvalidParameterValue}, + {"LeadingDigit", func(in *rds.CreateDBSubnetGroupInput) { in.DBSubnetGroupName = aws.String("1group") }, awserrors.ErrorInvalidParameterValue}, + // A slash would split the name across the KV layout's own separator. + {"NameWithSlash", func(in *rds.CreateDBSubnetGroupInput) { in.DBSubnetGroupName = aws.String("db/private") }, awserrors.ErrorInvalidParameterValue}, + {"NoDescription", func(in *rds.CreateDBSubnetGroupInput) { in.DBSubnetGroupDescription = nil }, awserrors.ErrorInvalidParameterValue}, + {"NoSubnets", func(in *rds.CreateDBSubnetGroupInput) { in.SubnetIds = nil }, awserrors.ErrorInvalidParameterValue}, + {"DuplicateSubnet", func(in *rds.CreateDBSubnetGroupInput) { + in.SubnetIds = aws.StringSlice([]string{"subnet-alpha", "subnet-alpha"}) + }, awserrors.ErrorDBSubnetInvalid}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + input := subnetGroupInput(testSubnetGroup, "subnet-alpha") + tc.mutate(input) + + _, err := h.svc.CreateDBSubnetGroup(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Equal(t, tc.want, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + }) + } +} + +// The name is the reservation, so a repeat is a conflict rather than a silent +// overwrite of the first group's subnets. +func TestCreateDBSubnetGroup_RejectsADuplicateName(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-alpha"), testAccountID) + require.NoError(t, err) + + _, err = h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-zebra"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetGroupAlreadyExists, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +func TestDescribeDBSubnetGroups_ListsAndNames(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput("zeta", "subnet-alpha"), testAccountID) + require.NoError(t, err) + _, err = h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput("alpha", "subnet-zebra"), testAccountID) + require.NoError(t, err) + + listed, err := h.svc.DescribeDBSubnetGroups(t.Context(), &rds.DescribeDBSubnetGroupsInput{}, testAccountID) + require.NoError(t, err) + require.Len(t, listed.DBSubnetGroups, 2) + assert.Equal(t, "alpha", aws.StringValue(listed.DBSubnetGroups[0].DBSubnetGroupName), + "groups are sorted, so a repeated list does not read as drift") + + named, err := h.svc.DescribeDBSubnetGroups(t.Context(), + &rds.DescribeDBSubnetGroupsInput{DBSubnetGroupName: aws.String("zeta")}, testAccountID) + require.NoError(t, err) + require.Len(t, named.DBSubnetGroups, 1) + assert.Equal(t, "zeta", aws.StringValue(named.DBSubnetGroups[0].DBSubnetGroupName)) +} + +// A client polling a create would read an empty list as "gone" rather than +// "not there", so a named group that does not exist is an error. +func TestDescribeDBSubnetGroups_RejectsAnUnknownName(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + _, err := h.svc.DescribeDBSubnetGroups(t.Context(), + &rds.DescribeDBSubnetGroupsInput{DBSubnetGroupName: aws.String("absent")}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +func TestDeleteDBSubnetGroup_RemovesAnUnusedGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-alpha"), testAccountID) + require.NoError(t, err) + + _, err = h.svc.DeleteDBSubnetGroup(t.Context(), + &rds.DeleteDBSubnetGroupInput{DBSubnetGroupName: aws.String(testSubnetGroup)}, testAccountID) + require.NoError(t, err) + + _, err = h.svc.DescribeDBSubnetGroups(t.Context(), + &rds.DescribeDBSubnetGroupsInput{DBSubnetGroupName: aws.String(testSubnetGroup)}, testAccountID) + assert.Equal(t, awserrors.ErrorDBSubnetGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") +} + +// A group referenced only by a deleting instance is refused too, so teardown +// ordering stays unambiguous rather than racing the instance's own delete. +func TestDeleteDBSubnetGroup_RefusesWhileAnInstanceReferencesIt(t *testing.T) { + for _, status := range []Status{StatusAvailable, StatusDeleting} { + t.Run(string(status), func(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-alpha"), testAccountID) + require.NoError(t, err) + + input := validCreateInput() + input.DBSubnetGroupName = aws.String(testSubnetGroup) + _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + require.NoError(t, h.svc.updateInstance(t.Context(), kv, testDBInstanceID, func(rec *DBInstanceRecord) { + rec.Status = status + })) + + _, err = h.svc.DeleteDBSubnetGroup(t.Context(), + &rds.DeleteDBSubnetGroupInput{DBSubnetGroupName: aws.String(testSubnetGroup)}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetGroupInvalidState, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.Contains(t, err.Error(), testDBInstanceID) + }) + } +} + +// The group is what decides where the endpoint lands, so a create naming one has +// to place the ENI in its subnet rather than in the account's default VPC. +func TestCreateDBInstance_PlacesTheEndpointFromTheNamedSubnetGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-zebra"), testAccountID) + require.NoError(t, err) + + input := validCreateInput() + input.DBSubnetGroupName = aws.String(testSubnetGroup) + _, err = h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + + rec := h.record(t, testDBInstanceID) + assert.Equal(t, "subnet-zebra", rec.SubnetID, + "the group's own subnet is the placement, not the default VPC's first") + assert.Equal(t, testSubnetGroup, rec.DBSubnetGroupName) + assert.Equal(t, testDefaultVPC, rec.VpcID) +} + +// Asserted through the describe rather than the record: the group the instance +// was placed from is only useful to a client if it survives the projection, and +// the Terraform provider reads db_subnet_group_name from exactly here. +func TestDescribeDBInstances_ReportsTheSubnetGroupTheInstanceWasPlacedFrom(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + _, err := h.svc.CreateDBSubnetGroup(t.Context(), subnetGroupInput(testSubnetGroup, "subnet-zebra"), testAccountID) + require.NoError(t, err) + + input := validCreateInput() + input.DBSubnetGroupName = aws.String(testSubnetGroup) + created, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + require.NotNil(t, created.DBInstance) + require.NotNil(t, created.DBInstance.DBSubnetGroup) + assert.Equal(t, testSubnetGroup, aws.StringValue(created.DBInstance.DBSubnetGroup.DBSubnetGroupName)) + + out, err := h.svc.DescribeDBInstances(t.Context(), &rds.DescribeDBInstancesInput{ + DBInstanceIdentifier: aws.String(testDBInstanceID), + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.DBInstances, 1) + + group := out.DBInstances[0].DBSubnetGroup + require.NotNil(t, group) + assert.Equal(t, testSubnetGroup, aws.StringValue(group.DBSubnetGroupName)) + assert.Equal(t, testDefaultVPC, aws.StringValue(group.VpcId)) + assert.Equal(t, subnetGroupStatusComplete, aws.StringValue(group.SubnetGroupStatus)) +} + +// Placing the endpoint somewhere else would put it in a subnet nobody chose, so +// a group that does not exist fails the create rather than falling back. +func TestCreateDBInstance_RejectsAnUnknownSubnetGroup(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + + input := validCreateInput() + input.DBSubnetGroupName = aws.String("absent") + _, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorDBSubnetGroupNotFound, awserrors.ValidErrorCodeFromError(err), + "the code has to survive resolution or the client sees a 500") + assert.False(t, h.recordExists(t, testDBInstanceID), + "a rejected create must reserve nothing") +} diff --git a/spinifex/handlers/rds/tags.go b/spinifex/handlers/rds/tags.go index 4ea11c266..9b515aa25 100644 --- a/spinifex/handlers/rds/tags.go +++ b/spinifex/handlers/rds/tags.go @@ -44,20 +44,31 @@ type taggableResource struct { notFound string } -// DB instances are the only taggable resource that exists so far. rds-7 adds -// subnet groups and parameter groups here, rds-8 snapshots — one entry each, -// registered by the phase that creates the resource. +// One entry per resource kind, registered by the phase that creates the +// resource; rds-8 adds snapshots. A default parameter group is deliberately +// absent: it has no stored record, so a tag written to it would have nowhere to +// live and would read back as absent on the next apply. var taggableResources = map[ResourceKind]taggableResource{ ResourceKindDBInstance: { key: DBInstanceKey, newRecord: func() TaggedRecord { return &DBInstanceRecord{} }, notFound: awserrors.ErrorDBInstanceNotFound, }, + ResourceKindDBSubnetGroup: { + key: DBSubnetGroupKey, + newRecord: func() TaggedRecord { return &DBSubnetGroupRecord{} }, + notFound: awserrors.ErrorDBSubnetGroupNotFound, + }, + ResourceKindDBParameterGroup: { + key: DBParameterGroupMetaKey, + newRecord: func() TaggedRecord { return &DBParameterGroupRecord{} }, + notFound: awserrors.ErrorDBParameterGroupNotFound, + }, } func (s *Service) ListTagsForResource(ctx context.Context, input *rds.ListTagsForResourceInput, accountID string) (*rds.ListTagsForResourceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } resource, parsed, err := s.resolveTaggable(aws.StringValue(input.ResourceName), accountID) if err != nil { @@ -78,7 +89,7 @@ func (s *Service) ListTagsForResource(ctx context.Context, input *rds.ListTagsFo // apply converges instead of failing on the second run. func (s *Service) AddTagsToResource(ctx context.Context, input *rds.AddTagsToResourceInput, accountID string) (*rds.AddTagsToResourceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } resource, parsed, err := s.resolveTaggable(aws.StringValue(input.ResourceName), accountID) if err != nil { @@ -100,8 +111,8 @@ func (s *Service) AddTagsToResource(ctx context.Context, input *rds.AddTagsToRes // Checked against the merge rather than the request: 40 existing tags plus // 40 new ones is over the limit even though neither side is. if len(merged) > maxTagsPerResource { - return nil, fmt.Errorf("%s: a resource may carry at most %d tags; this request would leave %d", - awserrors.ErrorInvalidParameterValue, maxTagsPerResource, len(merged)) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "a resource may carry at most %d tags; this request would leave %d", maxTagsPerResource, len(merged)) } return merged, nil }) @@ -115,7 +126,7 @@ func (s *Service) AddTagsToResource(ctx context.Context, input *rds.AddTagsToRes // destroy must not fail on the tags it already removed. func (s *Service) RemoveTagsFromResource(ctx context.Context, input *rds.RemoveTagsFromResourceInput, accountID string) (*rds.RemoveTagsFromResourceOutput, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } resource, parsed, err := s.resolveTaggable(aws.StringValue(input.ResourceName), accountID) if err != nil { @@ -155,8 +166,8 @@ func (s *Service) resolveTaggable(resourceName, accountID string) (taggableResou } resource, ok := taggableResources[parsed.Kind] if !ok { - return taggableResource{}, ParsedARN{}, fmt.Errorf("%s: tagging %s resources is not supported yet", - awserrors.ErrorInvalidParameterValue, parsed.Kind) + return taggableResource{}, ParsedARN{}, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "tagging %s resources is not supported yet", parsed.Kind) } return resource, parsed, nil } @@ -209,13 +220,13 @@ func mutateTags(ctx context.Context, kv jetstream.KeyValue, resource taggableRes // within one request keeps its last value, as AWS does. func validateTags(tags []*rds.Tag) (map[string]string, error) { if len(tags) > maxTagsPerResource { - return nil, fmt.Errorf("%s: at most %d tags may be supplied, got %d", - awserrors.ErrorInvalidParameterValue, maxTagsPerResource, len(tags)) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "at most %d tags may be supplied, got %d", maxTagsPerResource, len(tags)) } out := make(map[string]string, len(tags)) for _, tag := range tags { if tag == nil { - return nil, fmt.Errorf("%s: a tag entry is empty", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "a tag entry is empty") } key := aws.StringValue(tag.Key) if err := validateTagKey(key); err != nil { @@ -223,8 +234,8 @@ func validateTags(tags []*rds.Tag) (map[string]string, error) { } value := aws.StringValue(tag.Value) if len(value) > maxTagValueLen { - return nil, fmt.Errorf("%s: tag value for key %q may be at most %d characters", - awserrors.ErrorInvalidParameterValue, key, maxTagValueLen) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "tag value for key %q may be at most %d characters", key, maxTagValueLen) } out[key] = value } @@ -234,13 +245,13 @@ func validateTags(tags []*rds.Tag) (map[string]string, error) { func validateTagKey(key string) error { switch { case key == "": - return fmt.Errorf("%s: a tag key may not be empty", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "a tag key may not be empty") case len(key) > maxTagKeyLen: - return fmt.Errorf("%s: tag key %q may be at most %d characters", - awserrors.ErrorInvalidParameterValue, key, maxTagKeyLen) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "tag key %q may be at most %d characters", key, maxTagKeyLen) case strings.HasPrefix(strings.ToLower(key), reservedTagPrefix): - return fmt.Errorf("%s: tag key %q uses the reserved %q prefix", - awserrors.ErrorInvalidParameterValue, key, reservedTagPrefix) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "tag key %q uses the reserved %q prefix", key, reservedTagPrefix) } return nil } diff --git a/spinifex/handlers/rds/validate.go b/spinifex/handlers/rds/validate.go index 872d28f0d..e3e636d07 100644 --- a/spinifex/handlers/rds/validate.go +++ b/spinifex/handlers/rds/validate.go @@ -1,7 +1,6 @@ package handlers_rds import ( - "fmt" "strings" "github.com/aws/aws-sdk-go/aws" @@ -44,9 +43,9 @@ type validatedCreate struct { MasterPassword string DBName string SecurityGroupIDs []string - // Empty means "resolve the account's default VPC subnet"; rds-7 replaces - // this with a real DB subnet group lookup. - SubnetID string + // Empty means "resolve the account's default VPC subnet", matching AWS's own + // behaviour when a request names no subnet group. + DBSubnetGroupName string DBParameterGroupName string DeletionProtection bool Tags map[string]string @@ -56,7 +55,7 @@ type validatedCreate struct { // runs afterwards, so a malformed request never reaches the VPC. func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, error) { if input == nil { - return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") } if err := rejectUnimplemented(input); err != nil { return nil, err @@ -78,14 +77,14 @@ func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, instanceClass := aws.StringValue(input.DBInstanceClass) instanceType, err := InstanceTypeForClass(instanceClass) if err != nil { - return nil, fmt.Errorf("%s: DBInstanceClass %q is not supported; supported classes are %s", - awserrors.ErrorInvalidParameterValue, instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) } storage := aws.Int64Value(input.AllocatedStorage) if storage < minAllocatedStorageGiB || storage > maxAllocatedStorageGiB { - return nil, fmt.Errorf("%s: AllocatedStorage must be between %d and %d GiB", - awserrors.ErrorInvalidParameterValue, minAllocatedStorageGiB, maxAllocatedStorageGiB) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "AllocatedStorage must be between %d and %d GiB", minAllocatedStorageGiB, maxAllocatedStorageGiB) } storageType := strings.ToLower(strings.TrimSpace(aws.StringValue(input.StorageType))) @@ -93,8 +92,8 @@ func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, storageType = storageTypeGP3 } if storageType != storageTypeGP3 { - return nil, fmt.Errorf("%s: StorageType %q is not supported; only %q is offered", - awserrors.ErrorInvalidParameterValue, storageType, storageTypeGP3) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "StorageType %q is not supported; only %q is offered", storageType, storageTypeGP3) } masterUsername := aws.StringValue(input.MasterUsername) @@ -110,24 +109,17 @@ func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, if input.Port != nil { port = aws.Int64Value(input.Port) if port < minDBPort || port > maxDBPort { - return nil, fmt.Errorf("%s: Port must be between %d and %d", - awserrors.ErrorInvalidParameterValue, minDBPort, maxDBPort) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Port must be between %d and %d", minDBPort, maxDBPort) } } - // No subnet groups exist until rds-7, so a named one cannot resolve. Saying - // so is the honest answer; silently placing the ENI somewhere else would put - // the endpoint in a subnet the customer did not choose. - if group := aws.StringValue(input.DBSubnetGroupName); group != "" { - return nil, fmt.Errorf("%s: DB subnet group %q not found", awserrors.ErrorDBSubnetGroupNotFound, group) - } - - // The implicit default group is the one name that will resolve once rds-7 - // materialises it, so accepting it now keeps a Terraform config that names - // it working across both phases. + // Both groups are resolved against KV by the caller, which is the only place + // they can be: this validates the request alone. An unnamed parameter group + // takes the engine's implicit default, matching AWS. paramGroup := aws.StringValue(input.DBParameterGroupName) - if paramGroup != "" && paramGroup != engine.DefaultParameterGroupName() { - return nil, fmt.Errorf("%s: DB parameter group %q not found", awserrors.ErrorDBParameterGroupNotFound, paramGroup) + if paramGroup == "" { + paramGroup = engine.DefaultParameterGroupName() } // Rejected before the identifier is reserved, so a create with bad tags @@ -150,7 +142,8 @@ func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, MasterPassword: masterPassword, DBName: aws.StringValue(input.DBName), SecurityGroupIDs: aws.StringValueSlice(input.VpcSecurityGroupIds), - DBParameterGroupName: engine.DefaultParameterGroupName(), + DBSubnetGroupName: aws.StringValue(input.DBSubnetGroupName), + DBParameterGroupName: paramGroup, DeletionProtection: aws.BoolValue(input.DeletionProtection), Tags: tags, }, nil @@ -161,21 +154,22 @@ func validateCreateRequest(input *rds.CreateDBInstanceInput) (*validatedCreate, func validateDBInstanceIdentifier(id string) error { switch { case id == "": - return fmt.Errorf("%s: DBInstanceIdentifier is required", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier is required") case len(id) > maxDBInstanceIdentifierLen: - return fmt.Errorf("%s: DBInstanceIdentifier must be at most %d characters", - awserrors.ErrorInvalidParameterValue, maxDBInstanceIdentifierLen) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceIdentifier must be at most %d characters", maxDBInstanceIdentifierLen) case !isLetter(rune(id[0])): - return fmt.Errorf("%s: DBInstanceIdentifier must begin with a letter", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier must begin with a letter") case strings.HasSuffix(id, "-"): - return fmt.Errorf("%s: DBInstanceIdentifier may not end with a hyphen", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBInstanceIdentifier may not end with a hyphen") case strings.Contains(id, "--"): - return fmt.Errorf("%s: DBInstanceIdentifier may not contain consecutive hyphens", awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceIdentifier may not contain consecutive hyphens") } for _, r := range id { if !isDigit(r) && r != '-' && (r < 'a' || r > 'z') { - return fmt.Errorf("%s: DBInstanceIdentifier may contain only lowercase letters, digits and hyphens", - awserrors.ErrorInvalidParameterValue) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceIdentifier may contain only lowercase letters, digits and hyphens") } } return nil @@ -234,5 +228,5 @@ func rejectUnimplemented(input *rds.CreateDBInstanceInput) error { } func unimplemented(parameter, why string) error { - return fmt.Errorf("%s: %s is not supported: %s", awserrors.ErrorInvalidParameterValue, parameter, why) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s is not supported: %s", parameter, why) } diff --git a/spinifex/instancetypes/generate.go b/spinifex/instancetypes/generate.go index 9f893bd85..facfcc6a4 100644 --- a/spinifex/instancetypes/generate.go +++ b/spinifex/instancetypes/generate.go @@ -55,6 +55,27 @@ func DefaultVCPUs(instanceType string) (vcpus int, ok bool) { return v, ok } +// defaultMemoryMiBByInstanceType is the memory counterpart of the vCPU map, and +// is host-independent for the same reason. +var defaultMemoryMiBByInstanceType = func() map[string]int64 { + m := make(map[string]int64) + for _, def := range instanceFamilyDefs { + for _, size := range def.sizes { + m[def.name+"."+size.suffix] = int64(size.memoryGB * 1024) + } + } + return m +}() + +// DefaultMemoryMiB returns the memory footprint of an instance type name; ok is +// false for an unknown type. Callers that size a guest's own configuration — +// RDS derives its engine memory parameters from it — need the figure without +// generating the whole InstanceTypeInfo the host happens to support. +func DefaultMemoryMiB(instanceType string) (memoryMiB int64, ok bool) { + v, ok := defaultMemoryMiBByInstanceType[instanceType] + return v, ok +} + // generateForGeneration creates instance types for the given CPU generation. // Cross-vendor siblings are included on x86_64 so a mixed Intel+AMD cluster // can serve either vendor family. diff --git a/tests/e2e/harness/rds.go b/tests/e2e/harness/rds.go index 14e9d6543..05e4c3f67 100644 --- a/tests/e2e/harness/rds.go +++ b/tests/e2e/harness/rds.go @@ -48,6 +48,25 @@ func WaitForDBInstanceAvailable(t *testing.T, c *AWSClient, id string, opts ...P return last } +// WaitForDBInstanceGone polls until the instance no longer exists. A group an +// instance still references cannot be deleted, and a deleting instance still +// counts, so a teardown that frees a group has to wait for the record to go. +func WaitForDBInstanceGone(t *testing.T, c *AWSClient, id string, opts ...PollOpt) { + t.Helper() + cfg := applyOpts(pollCfg{timeout: 10 * time.Minute, interval: 10 * time.Second}, opts...) + EventuallyErr(t, func() error { + instance, err := DescribeDBInstance(c, id) + if ErrorCodeIs(err, "DBInstanceNotFound") { + return nil + } + if err != nil { + return fmt.Errorf("describe-db-instances %s: %w", id, err) + } + return fmt.Errorf("%s still exists with status=%s", id, aws.StringValue(instance.DBInstanceStatus)) + }, cfg.timeout, cfg.interval) + t.Logf("DB instance %s is gone", id) +} + // DescribeDBInstance returns the single named DB instance. A named instance that // does not exist is an error from the API, not an empty list. func DescribeDBInstance(c *AWSClient, id string) (*rds.DBInstance, error) { diff --git a/tests/e2e/rds/groups_test.go b/tests/e2e/rds/groups_test.go new file mode 100644 index 000000000..974365099 --- /dev/null +++ b/tests/e2e/rds/groups_test.go @@ -0,0 +1,321 @@ +//go:build e2e + +package rds + +import ( + "fmt" + "os/exec" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/tests/e2e/harness" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // The engine version the control plane pins, so the family a customer names + // has to match what CreateDBInstance would resolve on its own. + dbParameterGroupFamily = "postgres18" + // A dynamic parameter, so the engine adopts it on a reload and the assertion + // does not have to wait out a restart. Its default is 4096 kB. + workMemKiB = "8192" +) + +// TestSubnetAndParameterGroups drives the customer-managed group path end to +// end: a subnet group the instance is placed from, a parameter group whose +// override the running engine actually reports, and the two deletes that must be +// refused while the instance holds them and accepted once it is gone. +func TestSubnetAndParameterGroups(t *testing.T) { + f := requireRDSFixture(t) + suffix := time.Now().Unix() + subnetGroup := fmt.Sprintf("%s-subnets-%d", dbInstancePfx, suffix) + paramGroup := fmt.Sprintf("%s-params-%d", dbInstancePfx, suffix) + id := fmt.Sprintf("%s-groups-%d", dbInstancePfx, suffix) + + vpcID, _, _ := harness.DiscoverDefaultVPC(t, f.AWS) + subnets := subnetsInVPC(t, f.AWS, vpcID) + require.NotEmpty(t, subnets, "the default VPC %s has no subnets to build a DB subnet group from", vpcID) + + harness.Phase(t, "Creating DB subnet group %q over %d subnet(s) in %s", subnetGroup, len(subnets), vpcID) + subnetIDs := make([]string, 0, len(subnets)) + for _, s := range subnets { + subnetIDs = append(subnetIDs, aws.StringValue(s.SubnetId)) + } + createdSubnets, err := f.AWS.RDS.CreateDBSubnetGroup(&rds.CreateDBSubnetGroupInput{ + DBSubnetGroupName: aws.String(subnetGroup), + DBSubnetGroupDescription: aws.String("rds e2e subnet group"), + SubnetIds: aws.StringSlice(subnetIDs), + }) + require.NoError(t, err, "create-db-subnet-group") + require.NotNil(t, createdSubnets.DBSubnetGroup) + assert.Equal(t, vpcID, aws.StringValue(createdSubnets.DBSubnetGroup.VpcId)) + assert.Equal(t, "Complete", aws.StringValue(createdSubnets.DBSubnetGroup.SubnetGroupStatus)) + assert.ElementsMatch(t, subnetIDs, groupSubnetIDs(createdSubnets.DBSubnetGroup), + "every subnet supplied must be stored on the group") + for _, s := range createdSubnets.DBSubnetGroup.Subnets { + require.NotNil(t, s.SubnetAvailabilityZone) + assert.NotEmpty(t, aws.StringValue(s.SubnetAvailabilityZone.Name), + "subnet %s must report the zone it is actually in", aws.StringValue(s.SubnetIdentifier)) + } + + harness.Phase(t, "Creating DB parameter group %q", paramGroup) + _, err = f.AWS.RDS.CreateDBParameterGroup(&rds.CreateDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + DBParameterGroupFamily: aws.String(dbParameterGroupFamily), + Description: aws.String("rds e2e parameter group"), + }) + require.NoError(t, err, "create-db-parameter-group") + + _, err = f.AWS.RDS.ModifyDBParameterGroup(&rds.ModifyDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + Parameters: []*rds.Parameter{{ + ParameterName: aws.String("work_mem"), + ParameterValue: aws.String(workMemKiB), + ApplyMethod: aws.String("immediate"), + }}, + }) + require.NoError(t, err, "modify-db-parameter-group") + + // D20: the size-derived defaults are formulas internally, but a customer only + // ever sees the literal they resolved to. + t.Run("DescribeDBParametersReportsLiterals", func(t *testing.T) { + params := describeAllParameters(t, f, paramGroup) + + workMem, ok := params["work_mem"] + require.True(t, ok, "the modified parameter must be reported") + assert.Equal(t, workMemKiB, aws.StringValue(workMem.ParameterValue)) + assert.Equal(t, "user", aws.StringValue(workMem.Source)) + + shared, ok := params["shared_buffers"] + require.True(t, ok, "a parameter the group does not set must still be reported") + assert.Equal(t, "engine-default", aws.StringValue(shared.Source)) + assert.NotContains(t, aws.StringValue(shared.ParameterValue), "{", + "a size-derived default must reach the customer as a literal") + assert.Equal(t, "static", aws.StringValue(shared.ApplyType)) + }) + + // The value the engine would refuse has to be refused here, not handed to a + // cluster that then fails to start. + t.Run("RejectsAValueTheEngineWouldRefuse", func(t *testing.T) { + harness.ExpectError(t, "InvalidParameterValue", func() error { + _, err := f.AWS.RDS.ModifyDBParameterGroup(&rds.ModifyDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + Parameters: []*rds.Parameter{{ + ParameterName: aws.String("max_connections"), + ParameterValue: aws.String("{DBInstanceClassMemory/9531392}"), + }}, + }) + return err + }) + }) + + harness.Phase(t, "Creating DB instance %q against both groups", id) + created, err := f.AWS.RDS.CreateDBInstance(&rds.CreateDBInstanceInput{ //nolint:staticcheck // e2e:allow-create — the instance under test + DBInstanceIdentifier: aws.String(id), + Engine: aws.String(dbEngine), + DBInstanceClass: aws.String(dbClass), + AllocatedStorage: aws.Int64(dbStorageGiB), + DBName: aws.String(dbName), + MasterUsername: aws.String(dbMasterUser), + MasterUserPassword: aws.String(dbMasterPassword), + DBSubnetGroupName: aws.String(subnetGroup), + DBParameterGroupName: aws.String(paramGroup), + }) + require.NoError(t, err, "create-db-instance") + require.NotNil(t, created.DBInstance) + + // Registered before the waits below, so a failure part-way still frees the + // groups the later deletes need. + t.Cleanup(func() { + teardownGroups(t, f, id, subnetGroup, paramGroup) + }) + + assert.Equal(t, subnetGroup, dbSubnetGroupName(created.DBInstance)) + assert.Equal(t, paramGroup, dbParameterGroupName(created.DBInstance)) + + var instance *rds.DBInstance + t.Run("BecomesAvailableOnTheResolvedParameters", func(t *testing.T) { + instance = harness.WaitForDBInstanceAvailable(t, f.AWS, id) + assert.Equal(t, subnetGroup, dbSubnetGroupName(instance), + "the describe must report the group the instance was placed from") + assert.Equal(t, paramGroup, dbParameterGroupName(instance)) + }) + + // The only assertion that proves the resolved set reached the engine rather + // than merely being stored: the running cluster reports the override. + t.Run("TheEngineRunsTheOverride", func(t *testing.T) { + requireAvailable(t, instance) + out := runSQL(t, instance, "SHOW work_mem;") + assert.Equal(t, "8MB", strings.TrimSpace(out), "the engine is not running the group's work_mem") + }) + + t.Run("GroupsInUseCannotBeDeleted", func(t *testing.T) { + requireAvailable(t, instance) + harness.ExpectError(t, "InvalidDBSubnetGroupStateFault", func() error { + _, err := f.AWS.RDS.DeleteDBSubnetGroup(&rds.DeleteDBSubnetGroupInput{ + DBSubnetGroupName: aws.String(subnetGroup), + }) + return err + }) + harness.ExpectError(t, "InvalidDBParameterGroupState", func() error { + _, err := f.AWS.RDS.DeleteDBParameterGroup(&rds.DeleteDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + }) + return err + }) + }) + + t.Run("GroupsFreeUpOnceTheInstanceIsGone", func(t *testing.T) { + requireAvailable(t, instance) + harness.Phase(t, "Deleting DB instance %q to free its groups", id) + _, err := f.AWS.RDS.DeleteDBInstance(&rds.DeleteDBInstanceInput{ + DBInstanceIdentifier: aws.String(id), + SkipFinalSnapshot: aws.Bool(true), + }) + require.NoError(t, err, "delete-db-instance") + harness.WaitForDBInstanceGone(t, f.AWS, id) + + _, err = f.AWS.RDS.DeleteDBSubnetGroup(&rds.DeleteDBSubnetGroupInput{ + DBSubnetGroupName: aws.String(subnetGroup), + }) + require.NoError(t, err, "delete-db-subnet-group after the instance is gone") + + _, err = f.AWS.RDS.DeleteDBParameterGroup(&rds.DeleteDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + }) + require.NoError(t, err, "delete-db-parameter-group after the instance is gone") + + harness.ExpectError(t, "DBSubnetGroupNotFoundFault", func() error { + _, err := f.AWS.RDS.DescribeDBSubnetGroups(&rds.DescribeDBSubnetGroupsInput{ + DBSubnetGroupName: aws.String(subnetGroup), + }) + return err + }) + }) + + // default.postgres18 is implicit rather than stored, so it has to be + // describable and undeletable without ever having been created. + t.Run("TheDefaultParameterGroupIsImplicitAndUndeletable", func(t *testing.T) { + out, err := f.AWS.RDS.DescribeDBParameterGroups(&rds.DescribeDBParameterGroupsInput{ + DBParameterGroupName: aws.String("default." + dbParameterGroupFamily), + }) + require.NoError(t, err, "describe-db-parameter-groups") + require.Len(t, out.DBParameterGroups, 1) + assert.Equal(t, dbParameterGroupFamily, aws.StringValue(out.DBParameterGroups[0].DBParameterGroupFamily)) + + harness.ExpectError(t, "InvalidDBParameterGroupState", func() error { + _, err := f.AWS.RDS.DeleteDBParameterGroup(&rds.DeleteDBParameterGroupInput{ + DBParameterGroupName: aws.String("default." + dbParameterGroupFamily), + }) + return err + }) + }) +} + +// Best-effort teardown: the groups can only go once the instance does, and every +// step is already the assertion of some subtest, so a failure here is logged +// rather than failing a test that otherwise passed. +func teardownGroups(t *testing.T, f *Fixture, id, subnetGroup, paramGroup string) { + t.Helper() + if _, err := harness.DescribeDBInstance(f.AWS, id); err == nil { + if _, err := f.AWS.RDS.DeleteDBInstance(&rds.DeleteDBInstanceInput{ + DBInstanceIdentifier: aws.String(id), + SkipFinalSnapshot: aws.Bool(true), + }); err != nil && !harness.ErrorCodeIs(err, "DBInstanceNotFound") { + t.Logf("cleanup: delete DB instance %s: %v", id, err) + } + harness.WaitForDBInstanceGone(t, f.AWS, id) + } + if _, err := f.AWS.RDS.DeleteDBSubnetGroup(&rds.DeleteDBSubnetGroupInput{ + DBSubnetGroupName: aws.String(subnetGroup), + }); err != nil && !harness.ErrorCodeIs(err, "DBSubnetGroupNotFoundFault") { + t.Logf("cleanup: delete DB subnet group %s: %v", subnetGroup, err) + } + if _, err := f.AWS.RDS.DeleteDBParameterGroup(&rds.DeleteDBParameterGroupInput{ + DBParameterGroupName: aws.String(paramGroup), + }); err != nil && !harness.ErrorCodeIs(err, "DBParameterGroupNotFound") { + t.Logf("cleanup: delete DB parameter group %s: %v", paramGroup, err) + } +} + +// The catalog is larger than one page, so a describe that stopped at the first +// response would miss whatever sorts late. +func describeAllParameters(t *testing.T, f *Fixture, group string) map[string]*rds.Parameter { + t.Helper() + params := map[string]*rds.Parameter{} + var marker *string + for { + out, err := f.AWS.RDS.DescribeDBParameters(&rds.DescribeDBParametersInput{ + DBParameterGroupName: aws.String(group), + Marker: marker, + }) + require.NoError(t, err, "describe-db-parameters") + for _, p := range out.Parameters { + params[aws.StringValue(p.ParameterName)] = p + } + if marker = out.Marker; aws.StringValue(marker) == "" { + return params + } + } +} + +func subnetsInVPC(t *testing.T, c *harness.AWSClient, vpcID string) []*ec2.Subnet { + t.Helper() + out, err := c.EC2.DescribeSubnets(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{{Name: aws.String("vpc-id"), Values: []*string{aws.String(vpcID)}}}, + }) + require.NoError(t, err, "describe-subnets") + return out.Subnets +} + +func groupSubnetIDs(group *rds.DBSubnetGroup) []string { + ids := make([]string, 0, len(group.Subnets)) + for _, s := range group.Subnets { + ids = append(ids, aws.StringValue(s.SubnetIdentifier)) + } + return ids +} + +func dbSubnetGroupName(instance *rds.DBInstance) string { + if instance.DBSubnetGroup == nil { + return "" + } + return aws.StringValue(instance.DBSubnetGroup.DBSubnetGroupName) +} + +func dbParameterGroupName(instance *rds.DBInstance) string { + if len(instance.DBParameterGroups) == 0 { + return "" + } + return aws.StringValue(instance.DBParameterGroups[0].DBParameterGroupName) +} + +// Shells to psql for the same reason the smoke test does: no production driver +// dependency for the suite. Skips when psql is absent. +func runSQL(t *testing.T, instance *rds.DBInstance, statement string) string { + t.Helper() + psql, err := exec.LookPath("psql") + if err != nil { + t.Skip("psql not on PATH; skipping the in-engine assertion") + } + + host := aws.StringValue(instance.Endpoint.Address) + port := aws.Int64Value(instance.Endpoint.Port) + cmd := exec.Command(psql, //nolint:gosec // psql is LookPath-resolved, args test-controlled + "--no-psqlrc", "--quiet", "--tuples-only", "--no-align", + "--set", "ON_ERROR_STOP=1", + "--host", host, "--port", fmt.Sprint(port), + "--username", dbMasterUser, "--dbname", dbName, + "--command", statement, + ) + cmd.Env = append(cmd.Environ(), "PGPASSWORD="+dbMasterPassword, "PGCONNECT_TIMEOUT=30") + + output, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "psql against %s:%d: %s", host, port, output) + return string(output) +}