diff --git a/cmd/rds-agent/agent.go b/cmd/rds-agent/agent.go index 77aee2f5e..55ac3dd8f 100644 --- a/cmd/rds-agent/agent.go +++ b/cmd/rds-agent/agent.go @@ -169,7 +169,7 @@ func newAgent(cfg config, cp controlPlane, probe *engineProbe) *Agent { EngineVersion: cfg.EngineVersion, } a := &Agent{cfg: cfg, id: id, cp: cp, probe: probe, handoffWriter: writeHandoff} - a.engine = newPostgresEngine(cfg, execCommandRunner) + a.engine = newPostgresEngine(cfg, execCommandRunner, execSessionRunner) 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) diff --git a/cmd/rds-agent/command.go b/cmd/rds-agent/command.go index 8bc529b7c..d9e2920ea 100644 --- a/cmd/rds-agent/command.go +++ b/cmd/rds-agent/command.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "strconv" "strings" "time" @@ -43,9 +44,32 @@ func newCommandRegistry(engine engineOps, storage storageOps) commandRegistry { handlers_rds.CommandGrowFilesystem: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { return storage.GrowFilesystem(ctx) }, + handlers_rds.CommandQuiesce: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { + params := commandParams(cmd) + hold, err := quiesceHoldFrom(params[handlers_rds.CommandParamQuiesceDeadlineSeconds]) + if err != nil { + return "", err + } + return "", engine.Quiesce(ctx, params[handlers_rds.CommandParamQuiesceLabel], hold) + }, + handlers_rds.CommandUnquiesce: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { + return "", engine.Unquiesce(ctx) + }, } } +// The control plane's hold deadline. A missing or unreadable one is refused +// rather than defaulted: a hold the guest chose the length of would not be the +// bound the control plane is relying on. +func quiesceHoldFrom(value string) (time.Duration, error) { + seconds, err := strconv.Atoi(strings.TrimSpace(value)) + if err != nil || seconds <= 0 { + return 0, fmt.Errorf("%s must be a positive number of seconds, got %q", + handlers_rds.CommandParamQuiesceDeadlineSeconds, value) + } + return time.Duration(seconds) * time.Second, nil +} + // A later value wins, so a malformed command carrying a parameter twice cannot // leave the handler acting on the first of two conflicting values. func commandParams(cmd handlers_rds.Command) map[string]string { diff --git a/cmd/rds-agent/engine.go b/cmd/rds-agent/engine.go index 426bf23ab..ae2565a6d 100644 --- a/cmd/rds-agent/engine.go +++ b/cmd/rds-agent/engine.go @@ -11,8 +11,10 @@ import ( "path/filepath" "strconv" "strings" + "sync" "sync/atomic" "syscall" + "time" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) @@ -30,6 +32,12 @@ type engineOps interface { // Shuts the engine down cleanly, so the data volume is checkpointed before // the VM is stopped or snapshotted. Stop(ctx context.Context) error + // Puts the engine into backup mode so the datadir can be snapshotted at a + // checkpoint. Releases itself after hold, whatever happens to the caller. + Quiesce(ctx context.Context, label string, hold time.Duration) error + // Takes the engine back out of backup mode. Fails when no hold is active, + // which is how a hold that expired mid-snapshot becomes visible. + Unquiesce(ctx context.Context) error } // The rollback half of the parameter path, kept off engineOps because it is not @@ -102,6 +110,7 @@ func lookupCredential(username string) (*syscall.Credential, error) { // agent drops to the postgres OS user rather than holding a password of its own. type postgresEngine struct { run commandRunner + startSess sessionRunner psql string rcService string service string @@ -111,13 +120,19 @@ type postgresEngine struct { // Set from the bootstrap config, on a different goroutine than the commands // that read it. port atomic.Int64 + + // The backup mode currently held, if any. Guarded because the expiry timer + // releases it on its own goroutine. + mu sync.Mutex + held *quiesceHold } var _ engineOps = (*postgresEngine)(nil) -func newPostgresEngine(cfg config, run commandRunner) *postgresEngine { +func newPostgresEngine(cfg config, run commandRunner, startSess sessionRunner) *postgresEngine { e := &postgresEngine{ run: run, + startSess: startSess, psql: filepath.Join(cfg.PGBin, "psql"), rcService: cfg.RCService, service: cfg.EngineService, @@ -360,22 +375,28 @@ const defaultGuestPath = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbi // visible in the process table. func (e *postgresEngine) psqlRun(ctx context.Context, sql string, env ...string) (string, error) { return e.run(ctx, command{ - Name: e.psql, - Args: []string{ - "--no-psqlrc", "--quiet", "--no-align", "--tuples-only", - "-v", "ON_ERROR_STOP=1", - "-h", e.socketDir, - "-p", strconv.FormatInt(e.port.Load(), 10), - "-U", e.osUser, - "-d", "postgres", - "-f", "-", - }, + Name: e.psql, + Args: e.psqlArgs(), Env: append([]string{"PATH=" + defaultGuestPath}, env...), Stdin: sql, User: e.osUser, }) } +// Reading the script from stdin is what lets one invocation serve both a +// one-shot run and a session held open across several statements. +func (e *postgresEngine) psqlArgs() []string { + return []string{ + "--no-psqlrc", "--quiet", "--no-align", "--tuples-only", + "-v", "ON_ERROR_STOP=1", + "-h", e.socketDir, + "-p", strconv.FormatInt(e.port.Load(), 10), + "-U", e.osUser, + "-d", "postgres", + "-f", "-", + } +} + func redact(s, secret string) string { if secret == "" { return s diff --git a/cmd/rds-agent/engine_test.go b/cmd/rds-agent/engine_test.go index ec1aabd77..d82ea52b1 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -9,6 +9,7 @@ import ( "slices" "strings" "testing" + "time" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) @@ -21,6 +22,9 @@ type fakeEngine struct { params []handlers_rds.Parameter pending []string stopped bool + label string + hold time.Duration + released bool err error } @@ -41,6 +45,16 @@ func (f *fakeEngine) Stop(context.Context) error { return f.err } +func (f *fakeEngine) Quiesce(_ context.Context, label string, hold time.Duration) error { + f.label, f.hold = label, hold + return f.err +} + +func (f *fakeEngine) Unquiesce(context.Context) error { + f.released = true + return f.err +} + func TestCommandRegistry_SetPasswordCarriesBothParameters(t *testing.T) { engine := &fakeEngine{} reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), handlers_rds.Command{ @@ -139,7 +153,7 @@ func newTestEngine(t *testing.T, run commandRunner) *postgresEngine { t.Fatalf("resolve the current user: %v", err) } cfg.PGUser = current.Username - return newPostgresEngine(cfg, run) + return newPostgresEngine(cfg, run, nil) } // D8: the password reaches psql through the environment and is re-quoted there, diff --git a/cmd/rds-agent/quiesce.go b/cmd/rds-agent/quiesce.go new file mode 100644 index 000000000..4353455a4 --- /dev/null +++ b/cmd/rds-agent/quiesce.go @@ -0,0 +1,259 @@ +package main + +import ( + "bufio" + "bytes" + "context" + "errors" + "fmt" + "io" + "log/slog" + "os/exec" + "strings" + "sync" + "syscall" + "time" +) + +// PostgreSQL's non-exclusive backup API ties the backup to the session that +// started it: when that session ends, for any reason, the backup is aborted. +// That is what makes the hold self-expiring — a control plane that dies mid +// snapshot cannot leave the engine in backup mode forever. +type engineSession interface { + // Runs sql and waits for the engine to finish it. + Exec(ctx context.Context, sql string) error + // Ends the session, releasing whatever the engine tied to it. + Close() error +} + +// Starts a child process whose lifetime the caller controls, rather than one +// that is run to completion. ctx governs the process, not one statement. +type sessionRunner func(ctx context.Context, c command) (engineSession, error) + +// Written after every statement so the reader knows the engine is done with it. +// The statements this carries return LSNs, so it cannot appear in a result. +const sessionSentinel = "--rds-agent-ready--" + +// The backup mode currently held, and the timer that ends it regardless of what +// happens to the control plane. +type quiesceHold struct { + label string + session engineSession + expiry *time.Timer +} + +// Puts the engine into backup mode: the datadir is checkpointed and the engine +// stops writing over the pages a snapshot is about to read. The hold is released +// by Unquiesce, or by its own deadline, whichever comes first. +func (e *postgresEngine) Quiesce(ctx context.Context, label string, hold time.Duration) error { + if label == "" { + return errors.New("quiesce requires a backup label") + } + if hold <= 0 { + return errors.New("quiesce requires a positive hold deadline") + } + + // Held across the whole start, so a second quiesce waits and then finds the + // first one's hold rather than opening a concurrent backup alongside it. + e.mu.Lock() + defer e.mu.Unlock() + if e.held != nil { + return fmt.Errorf("the engine is already quiesced for backup %s", e.held.label) + } + + // Deliberately not the command's context: the session has to outlive the call + // that started it, and its own deadline is what bounds it instead. + session, err := e.startSess(context.WithoutCancel(ctx), command{ + Name: e.psql, + Args: e.psqlArgs(), + Env: []string{"PATH=" + defaultGuestPath, "RDS_BACKUP_LABEL=" + label}, + Stdin: "", + User: e.osUser, + }) + if err != nil { + return fmt.Errorf("open a backup session: %w", err) + } + + // fast forces an immediate checkpoint rather than spreading it over the + // checkpoint interval, which would hold the snapshot open for minutes. + const sql = `\getenv label RDS_BACKUP_LABEL +SELECT pg_backup_start(:'label', fast => true); +` + if err := session.Exec(ctx, sql); err != nil { + if closeErr := session.Close(); closeErr != nil { + slog.Warn("rds-agent: closing a failed backup session", "err", closeErr) + } + return fmt.Errorf("put the engine into backup mode: %w", err) + } + + e.held = &quiesceHold{ + label: label, + session: session, + expiry: time.AfterFunc(hold, func() { e.expireQuiesce(label) }), + } + slog.Info("rds-agent: engine quiesced for backup", "label", label, "hold", hold) + return nil +} + +// Ends the backup cleanly. A missing hold is an error rather than a silent +// success: it means the deadline fired first, so the snapshot the control plane +// just took was not taken against a held checkpoint. +func (e *postgresEngine) Unquiesce(ctx context.Context) error { + e.mu.Lock() + held := e.held + e.held = nil + e.mu.Unlock() + + if held == nil { + return errors.New("the engine is not quiesced; the backup hold had already expired") + } + held.expiry.Stop() + + // The stop has to run on the session that started the backup, and the session + // is closed either way — the engine aborts an unstopped backup with it. + execErr := held.session.Exec(ctx, "SELECT pg_backup_stop();\n") + closeErr := held.session.Close() + if execErr != nil { + return fmt.Errorf("take the engine out of backup mode: %w", errors.Join(execErr, closeErr)) + } + if closeErr != nil { + return fmt.Errorf("close the backup session: %w", closeErr) + } + slog.Info("rds-agent: engine released from backup mode", "label", held.label) + return nil +} + +// The deadline. Ending the session is enough — the engine aborts the backup it +// was holding — so this never has to reach the engine itself. +func (e *postgresEngine) expireQuiesce(label string) { + e.mu.Lock() + held := e.held + if held == nil || held.label != label { + e.mu.Unlock() + return + } + e.held = nil + e.mu.Unlock() + + slog.Warn("rds-agent: backup hold expired; releasing the engine", "label", label) + if err := held.session.Close(); err != nil { + slog.Error("rds-agent: closing an expired backup session failed", "label", label, "err", err) + } +} + +// A psql child with its stdin held open, so statements can be fed to it one at a +// time and the engine keeps seeing a single session. +type psqlSession struct { + cmd *exec.Cmd + stdin io.WriteCloser + stdout *bufio.Reader + stderr *bytes.Buffer + + mu sync.Mutex + closed bool +} + +var _ engineSession = (*psqlSession)(nil) + +func execSessionRunner(ctx context.Context, c command) (engineSession, error) { + cmd := exec.CommandContext(ctx, c.Name, c.Args...) + cmd.Env = c.Env + if c.User != "" { + credential, err := lookupCredential(c.User) + if err != nil { + return nil, err + } + cmd.SysProcAttr = &syscall.SysProcAttr{Credential: credential} + } + + stdin, err := cmd.StdinPipe() + if err != nil { + return nil, fmt.Errorf("open stdin: %w", err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("open stdout: %w", err) + } + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("start %s: %w", c.Name, err) + } + return &psqlSession{cmd: cmd, stdin: stdin, stdout: bufio.NewReader(stdout), stderr: &stderr}, nil +} + +// Feeds sql and waits for the sentinel that follows it. ON_ERROR_STOP makes psql +// exit on a failed statement, so a sentinel that never arrives is the failure +// itself and the engine's own message is on stderr. +func (s *psqlSession) Exec(ctx context.Context, sql string) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.closed { + return errors.New("the backup session is closed") + } + if _, err := io.WriteString(s.stdin, sql+"\\echo "+sessionSentinel+"\n"); err != nil { + return fmt.Errorf("%w: %s", err, strings.TrimSpace(s.stderr.String())) + } + + // Read on its own goroutine so a psql that never answers costs the caller its + // deadline rather than blocking it forever. + done := make(chan error, 1) + go func() { done <- s.awaitSentinel() }() + select { + case err := <-done: + return err + case <-ctx.Done(): + return ctx.Err() + } +} + +func (s *psqlSession) awaitSentinel() error { + for { + line, err := s.stdout.ReadString('\n') + if strings.TrimSpace(line) == sessionSentinel { + return nil + } + if err != nil { + if message := strings.TrimSpace(s.stderr.String()); message != "" { + return errors.New(message) + } + return fmt.Errorf("the backup session ended: %w", err) + } + } +} + +// Closing stdin ends the script, and the wait reaps the child. A session that +// will not end is killed, because leaving it is what would keep the engine in +// backup mode. +func (s *psqlSession) Close() error { + s.mu.Lock() + defer s.mu.Unlock() + if s.closed { + return nil + } + s.closed = true + + if err := s.stdin.Close(); err != nil { + slog.Debug("rds-agent: closing the backup session stdin", "err", err) + } + done := make(chan error, 1) + go func() { done <- s.cmd.Wait() }() + select { + case err := <-done: + if err != nil { + return fmt.Errorf("%w: %s", err, strings.TrimSpace(s.stderr.String())) + } + return nil + case <-time.After(sessionCloseGrace): + if err := s.cmd.Process.Kill(); err != nil { + return fmt.Errorf("kill the backup session: %w", err) + } + <-done + return nil + } +} + +// How long a backup session gets to end on its own before it is killed. The +// engine releases the backup on either, so this only bounds the wait. +const sessionCloseGrace = 10 * time.Second diff --git a/cmd/rds-agent/quiesce_test.go b/cmd/rds-agent/quiesce_test.go new file mode 100644 index 000000000..f6d45ab27 --- /dev/null +++ b/cmd/rds-agent/quiesce_test.go @@ -0,0 +1,246 @@ +package main + +import ( + "context" + "errors" + "slices" + "strings" + "sync" + "testing" + "time" + + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" +) + +// fakeSession stands in for the held psql child. Closing it is what the engine +// treats as the end of the backup, so a test asserts on that rather than on any +// statement the release would have sent. +type fakeSession struct { + mu sync.Mutex + executed []string + closed int + execErr error + closeErr error +} + +var _ engineSession = (*fakeSession)(nil) + +func (f *fakeSession) Exec(_ context.Context, sql string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.executed = append(f.executed, sql) + return f.execErr +} + +func (f *fakeSession) Close() error { + f.mu.Lock() + defer f.mu.Unlock() + f.closed++ + return f.closeErr +} + +func (f *fakeSession) statements() []string { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.executed...) +} + +func (f *fakeSession) closes() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.closed +} + +// newQuiesceEngine wires an engine whose backup sessions are the fake, and +// returns the commands each session was started with. +func newQuiesceEngine(t *testing.T, session *fakeSession) (*postgresEngine, *[]command) { + t.Helper() + started := &[]command{} + engine := newTestEngine(t, (&recordingRunner{}).run) + engine.startSess = func(_ context.Context, c command) (engineSession, error) { + *started = append(*started, c) + return session, nil + } + return engine, started +} + +// The label rides the environment and is re-quoted by psql, so it never reaches +// a shell word or an argv another process can read. +func TestPostgresEngine_QuiesceStartsTheBackupOnAHeldSession(t *testing.T) { + session := &fakeSession{} + engine, started := newQuiesceEngine(t, session) + + if err := engine.Quiesce(context.Background(), "orders-db-pre-upgrade", time.Minute); err != nil { + t.Fatalf("Quiesce: %v", err) + } + + if len(*started) != 1 { + t.Fatalf("started %d sessions, want 1", len(*started)) + } + if !slices.Contains((*started)[0].Env, "RDS_BACKUP_LABEL=orders-db-pre-upgrade") { + t.Errorf("env %v does not carry the backup label", (*started)[0].Env) + } + statements := session.statements() + if len(statements) != 1 || !strings.Contains(statements[0], "pg_backup_start") { + t.Fatalf("statements = %q, want a pg_backup_start", statements) + } + // A spread checkpoint would hold the snapshot open for minutes. + if !strings.Contains(statements[0], "fast => true") { + t.Errorf("statement %q does not force an immediate checkpoint", statements[0]) + } + if session.closes() != 0 { + t.Error("the session was closed while the backup was meant to be held") + } +} + +func TestPostgresEngine_UnquiesceStopsTheBackupAndEndsTheSession(t *testing.T) { + session := &fakeSession{} + engine, _ := newQuiesceEngine(t, session) + if err := engine.Quiesce(context.Background(), "orders-db-pre-upgrade", time.Minute); err != nil { + t.Fatalf("Quiesce: %v", err) + } + + if err := engine.Unquiesce(context.Background()); err != nil { + t.Fatalf("Unquiesce: %v", err) + } + + statements := session.statements() + if len(statements) != 2 || !strings.Contains(statements[1], "pg_backup_stop") { + t.Fatalf("statements = %q, want a pg_backup_stop", statements) + } + if session.closes() != 1 { + t.Errorf("closed %d times, want the session ended exactly once", session.closes()) + } +} + +// A hold that expired mid-snapshot means the snapshot was not taken against a +// held checkpoint, so the release reports it rather than succeeding silently. +func TestPostgresEngine_UnquiesceWithoutAHoldIsAnError(t *testing.T) { + engine, _ := newQuiesceEngine(t, &fakeSession{}) + + err := engine.Unquiesce(context.Background()) + if err == nil { + t.Fatal("Unquiesce succeeded with no hold to release") + } + if !strings.Contains(err.Error(), "expired") { + t.Errorf("error %q does not say the hold had expired", err) + } +} + +// The engine aborts the backup when the session ends, which is what makes the +// hold self-expiring: a control plane that dies cannot leave it held forever. +func TestPostgresEngine_QuiesceReleasesItselfOnItsDeadline(t *testing.T) { + session := &fakeSession{} + engine, _ := newQuiesceEngine(t, session) + + if err := engine.Quiesce(context.Background(), "orders-db-pre-upgrade", 10*time.Millisecond); err != nil { + t.Fatalf("Quiesce: %v", err) + } + deadline := time.Now().Add(2 * time.Second) + for session.closes() == 0 && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + + if session.closes() != 1 { + t.Fatal("the backup session outlived its deadline") + } + // The hold is gone with it, so a later release is honest about that. + if err := engine.Unquiesce(context.Background()); err == nil { + t.Error("Unquiesce succeeded after the hold expired") + } +} + +// One backup at a time: a second start would open a concurrent backup alongside +// the first rather than extending it. +func TestPostgresEngine_QuiesceRefusesASecondHold(t *testing.T) { + session := &fakeSession{} + engine, started := newQuiesceEngine(t, session) + if err := engine.Quiesce(context.Background(), "first", time.Minute); err != nil { + t.Fatalf("Quiesce: %v", err) + } + + err := engine.Quiesce(context.Background(), "second", time.Minute) + if err == nil { + t.Fatal("a second quiesce was accepted while the first was held") + } + if !strings.Contains(err.Error(), "first") { + t.Errorf("error %q does not name the backup already holding the engine", err) + } + if len(*started) != 1 { + t.Errorf("started %d sessions, want the second refused before opening one", len(*started)) + } +} + +// A failed start leaves nothing held, so a retry is not refused by a hold that +// never took effect. +func TestPostgresEngine_QuiesceEndsTheSessionWhenTheBackupWillNotStart(t *testing.T) { + session := &fakeSession{execErr: errors.New(`ERROR: a backup is already in progress`)} + engine, _ := newQuiesceEngine(t, session) + + if err := engine.Quiesce(context.Background(), "orders-db-pre-upgrade", time.Minute); err == nil { + t.Fatal("Quiesce succeeded against an engine that refused the backup") + } + if session.closes() != 1 { + t.Errorf("closed %d times, want the failed session ended", session.closes()) + } + if engine.held != nil { + t.Error("a failed quiesce left a hold behind") + } +} + +func TestCommandRegistry_QuiesceCarriesTheLabelAndDeadline(t *testing.T) { + engine := &fakeEngine{} + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), + handlers_rds.Command{ + CommandID: "cmd-5", + Type: handlers_rds.CommandQuiesce, + Parameters: []handlers_rds.Parameter{ + {Name: handlers_rds.CommandParamQuiesceLabel, Value: "orders-db-pre-upgrade"}, + {Name: handlers_rds.CommandParamQuiesceDeadlineSeconds, Value: "360"}, + }, + }) + + if reply.Status != handlers_rds.CommandStatusSucceeded { + t.Fatalf("reply = %+v, want succeeded", reply) + } + if engine.label != "orders-db-pre-upgrade" || engine.hold != 6*time.Minute { + t.Errorf("engine got %q/%s, want the command's label and deadline", engine.label, engine.hold) + } +} + +// A hold the guest chose the length of would not be the bound the control plane +// is relying on, so a missing or unreadable deadline is refused. +func TestCommandRegistry_QuiesceRejectsAnUnusableDeadline(t *testing.T) { + for _, value := range []string{"", "soon", "0", "-30"} { + engine := &fakeEngine{} + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), + handlers_rds.Command{ + CommandID: "cmd-6", + Type: handlers_rds.CommandQuiesce, + Parameters: []handlers_rds.Parameter{ + {Name: handlers_rds.CommandParamQuiesceLabel, Value: "orders-db-pre-upgrade"}, + {Name: handlers_rds.CommandParamQuiesceDeadlineSeconds, Value: value}, + }, + }) + + if reply.Status != handlers_rds.CommandStatusFailed { + t.Errorf("deadline %q: reply = %+v, want failed", value, reply) + } + if engine.label != "" { + t.Errorf("deadline %q: the engine was quiesced anyway", value) + } + } +} + +func TestCommandRegistry_UnquiesceReachesTheEngine(t *testing.T) { + engine := &fakeEngine{} + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), + handlers_rds.Command{CommandID: "cmd-7", Type: handlers_rds.CommandUnquiesce}) + + if reply.Status != handlers_rds.CommandStatusSucceeded { + t.Fatalf("reply = %+v, want succeeded", reply) + } + if !engine.released { + t.Error("unquiesce did not reach the engine") + } +} diff --git a/spinifex/awserrors/awserrors.go b/spinifex/awserrors/awserrors.go index 19833c221..7036e211a 100644 --- a/spinifex/awserrors/awserrors.go +++ b/spinifex/awserrors/awserrors.go @@ -468,6 +468,8 @@ var ( ErrorDBInstanceAlreadyExists = "DBInstanceAlreadyExists" ErrorDBInstanceInvalidState = "InvalidDBInstanceState" ErrorDBSnapshotAlreadyExists = "DBSnapshotAlreadyExists" + ErrorDBSnapshotNotFound = "DBSnapshotNotFound" + ErrorDBSnapshotInvalidState = "InvalidDBSnapshotState" ErrorDBSubnetGroupNotFound = "DBSubnetGroupNotFoundFault" ErrorDBParameterGroupNotFound = "DBParameterGroupNotFound" ErrorDBInvalidVPCNetworkState = "InvalidVPCNetworkStateFault" @@ -1057,6 +1059,8 @@ var ErrorLookup = map[string]ErrorMessage{ ErrorDBInstanceAlreadyExists: {HTTPCode: 400, Message: "The user already has a DB instance with the given identifier."}, ErrorDBInstanceInvalidState: {HTTPCode: 400, Message: "The DB instance is not in a state that allows the requested operation."}, ErrorDBSnapshotAlreadyExists: {HTTPCode: 400, Message: "The user already has a DB snapshot with the given identifier."}, + ErrorDBSnapshotNotFound: {HTTPCode: 404, Message: "DBSnapshotIdentifier does not refer to an existing DB snapshot."}, + ErrorDBSnapshotInvalidState: {HTTPCode: 400, Message: "The DB snapshot is not in a state that allows the requested operation."}, ErrorDBSubnetGroupNotFound: {HTTPCode: 404, Message: "DBSubnetGroupName does not refer to an existing DB subnet group."}, 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."}, diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 15bcc22e8..5547eab43 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1063,6 +1063,10 @@ func (d *Daemon) subscribeAll() error { natsSub{handlers_rds.SubjectStopDBInstance, handleNATSRequest(d.rdsService.StopDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectModifyDBInstance, handleNATSRequest(d.rdsService.ModifyDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectDeleteDBInstance, handleNATSRequest(d.rdsService.DeleteDBInstance), "spinifex-workers"}, + natsSub{handlers_rds.SubjectCreateDBSnapshot, handleNATSRequest(d.rdsService.CreateDBSnapshot), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDescribeDBSnapshots, handleNATSRequest(d.rdsService.DescribeDBSnapshots), "spinifex-workers"}, + natsSub{handlers_rds.SubjectDeleteDBSnapshot, handleNATSRequest(d.rdsService.DeleteDBSnapshot), "spinifex-workers"}, + natsSub{handlers_rds.SubjectRestoreDBInstanceFromDBSnapshot, handleNATSRequest(d.rdsService.RestoreDBInstanceFromDBSnapshot), "spinifex-workers"}, natsSub{handlers_rds.SubjectDescribeEvents, handleNATSRequest(d.rdsService.DescribeEvents), "spinifex-workers"}, natsSub{handlers_rds.SubjectAddTagsToResource, handleNATSRequest(d.rdsService.AddTagsToResource), "spinifex-workers"}, natsSub{handlers_rds.SubjectRemoveTagsFromResource, handleNATSRequest(d.rdsService.RemoveTagsFromResource), "spinifex-workers"}, diff --git a/spinifex/gateway/rds/handler.go b/spinifex/gateway/rds/handler.go index fd8e12624..9e287538b 100644 --- a/spinifex/gateway/rds/handler.go +++ b/spinifex/gateway/rds/handler.go @@ -88,10 +88,10 @@ var actions = map[string]Handler{ "StopDBInstance": typed(StopDBInstance), // Snapshots. - "CreateDBSnapshot": pending(), - "DescribeDBSnapshots": pending(), - "DeleteDBSnapshot": pending(), - "RestoreDBInstanceFromDBSnapshot": pending(), + "CreateDBSnapshot": typed(CreateDBSnapshot), + "DescribeDBSnapshots": typed(DescribeDBSnapshots), + "DeleteDBSnapshot": typed(DeleteDBSnapshot), + "RestoreDBInstanceFromDBSnapshot": typed(RestoreDBInstanceFromDBSnapshot), // Automated backups. "DescribeDBInstanceAutomatedBackups": pending(), diff --git a/spinifex/gateway/rds/handler_test.go b/spinifex/gateway/rds/handler_test.go index 1ea325528..380a212dc 100644 --- a/spinifex/gateway/rds/handler_test.go +++ b/spinifex/gateway/rds/handler_test.go @@ -113,6 +113,8 @@ var liveActions = []string{ "CreateDBSubnetGroup", "DescribeDBSubnetGroups", "DeleteDBSubnetGroup", "CreateDBParameterGroup", "DescribeDBParameterGroups", "ModifyDBParameterGroup", "DescribeDBParameters", "DeleteDBParameterGroup", + "CreateDBSnapshot", "DescribeDBSnapshots", "DeleteDBSnapshot", + "RestoreDBInstanceFromDBSnapshot", } // What is under test in this file is the action table and the XML envelope, not @@ -144,6 +146,12 @@ func newStubbedNATS(t *testing.T) *nats.Conn { respondWith(t, nc, handlers_rds.SubjectModifyDBParameterGroup, &rds.DBParameterGroupNameMessage{}) respondWith(t, nc, handlers_rds.SubjectDescribeDBParameters, &rds.DescribeDBParametersOutput{}) respondWith(t, nc, handlers_rds.SubjectDeleteDBParameterGroup, &rds.DeleteDBParameterGroupOutput{}) + respondWith(t, nc, handlers_rds.SubjectCreateDBSnapshot, + &rds.CreateDBSnapshotOutput{DBSnapshot: &rds.DBSnapshot{DBSnapshotIdentifier: aws.String("orders-db-pre-upgrade")}}) + respondWith(t, nc, handlers_rds.SubjectDescribeDBSnapshots, &rds.DescribeDBSnapshotsOutput{}) + respondWith(t, nc, handlers_rds.SubjectDeleteDBSnapshot, &rds.DeleteDBSnapshotOutput{}) + respondWith(t, nc, handlers_rds.SubjectRestoreDBInstanceFromDBSnapshot, + &rds.RestoreDBInstanceFromDBSnapshotOutput{DBInstance: &rds.DBInstance{DBInstanceIdentifier: aws.String("orders-db-restored")}}) return nc } @@ -193,7 +201,8 @@ func TestDispatch_LiveActionsAreNotPending(t *testing.T) { } func TestDispatch_PendingActionIsNotImplemented(t *testing.T) { - _, err := Dispatch(t.Context(), "CreateDBSnapshot", map[string]string{"Action": "CreateDBSnapshot"}, nil, testCaller) + _, err := Dispatch(t.Context(), "DescribeDBInstanceAutomatedBackups", + map[string]string{"Action": "DescribeDBInstanceAutomatedBackups"}, nil, testCaller) require.Error(t, err) assert.Equal(t, awserrors.ErrorNotImplemented, err.Error()) } diff --git a/spinifex/gateway/rds/snapshot.go b/spinifex/gateway/rds/snapshot.go new file mode 100644 index 000000000..3d0e589b6 --- /dev/null +++ b/spinifex/gateway/rds/snapshot.go @@ -0,0 +1,28 @@ +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" +) + +// DB snapshots are addressed by their own identifier within the caller's +// account, so nothing here needs the EC2 snapshot the record points at — that +// stays system-owned and unreachable from the customer's EC2 surface. +func CreateDBSnapshot(ctx context.Context, input *rds.CreateDBSnapshotInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).CreateDBSnapshot(ctx, input, caller.AccountID) +} + +func DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDBSnapshotsInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DescribeDBSnapshots(ctx, input, caller.AccountID) +} + +func DeleteDBSnapshot(ctx context.Context, input *rds.DeleteDBSnapshotInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).DeleteDBSnapshot(ctx, input, caller.AccountID) +} + +func RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rds.RestoreDBInstanceFromDBSnapshotInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).RestoreDBInstanceFromDBSnapshot(ctx, input, caller.AccountID) +} diff --git a/spinifex/handlers/ec2/snapshot/service_impl.go b/spinifex/handlers/ec2/snapshot/service_impl.go index 3ddbd617f..590bc0b4a 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl.go +++ b/spinifex/handlers/ec2/snapshot/service_impl.go @@ -536,6 +536,18 @@ var describeSnapshotsValidFilters = map[string]bool{ // DescribeSnapshots lists snapshots matching the specified criteria, scoped to the caller's account. func (s *SnapshotServiceImpl) DescribeSnapshots(ctx context.Context, input *ec2.DescribeSnapshotsInput, accountID string) (*ec2.DescribeSnapshotsOutput, error) { + return s.describeSnapshots(ctx, input, accountID, false) +} + +// DescribeSnapshotsStrict is the control-plane lookup contract. A metadata read +// failure makes the result non-authoritative, so it is returned rather than +// hidden as an absent snapshot. +func (s *SnapshotServiceImpl) DescribeSnapshotsStrict(ctx context.Context, input *ec2.DescribeSnapshotsInput, accountID string) (*ec2.DescribeSnapshotsOutput, error) { + return s.describeSnapshots(ctx, input, accountID, true) +} + +func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.DescribeSnapshotsInput, + accountID string, strict bool) (*ec2.DescribeSnapshotsOutput, error) { s.mutex.RLock() defer s.mutex.RUnlock() @@ -594,6 +606,9 @@ func (s *SnapshotServiceImpl) DescribeSnapshots(ctx context.Context, input *ec2. cfg, err := s.getSnapshotConfig(snapshotID) if err != nil { slog.WarnContext(ctx, "DescribeSnapshots failed to get config", "snapshotId", snapshotID, "err", err) + if strict { + return nil, fmt.Errorf("describe snapshot %s metadata: %w", snapshotID, err) + } continue } diff --git a/spinifex/handlers/ec2/snapshot/service_impl_test.go b/spinifex/handlers/ec2/snapshot/service_impl_test.go index d55d89712..2a70c0f68 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl_test.go +++ b/spinifex/handlers/ec2/snapshot/service_impl_test.go @@ -264,6 +264,27 @@ func TestDescribeSnapshots(t *testing.T) { assert.True(t, snapshotIDs[*snap2.SnapshotId]) } +func TestDescribeSnapshotsStrict_RejectsPartialMetadataResults(t *testing.T) { + svc, store := setupTestSnapshotService(t) + _, err := store.PutObject(t.Context(), &s3.PutObjectInput{ + Bucket: aws.String("test-bucket"), + Key: aws.String(GetSnapshotKey("snap-corrupt")), + Body: strings.NewReader("not-json"), + }) + require.NoError(t, err) + + // The customer-facing list retains its historical best-effort behavior. + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{}, testAccountID) + require.NoError(t, err) + assert.Empty(t, out.Snapshots) + + // Reconciliation cannot interpret that partial result as authoritative + // absence, so its internal lookup surfaces the metadata failure. + _, err = svc.DescribeSnapshotsStrict(t.Context(), &ec2.DescribeSnapshotsInput{}, testAccountID) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCorruptSnapshotMetadata) +} + // TestDescribeSnapshots_ByID tests listing specific snapshots by ID. func TestDescribeSnapshots_ByID(t *testing.T) { svc, store := setupTestSnapshotService(t) diff --git a/spinifex/handlers/rds/agent_test.go b/spinifex/handlers/rds/agent_test.go index 25095b579..dbd747731 100644 --- a/spinifex/handlers/rds/agent_test.go +++ b/spinifex/handlers/rds/agent_test.go @@ -27,12 +27,14 @@ const ( testInstance = "i-0abc123" ) -// newTestCA builds a throwaway CA so cert minting is exercised without needing -// the cluster's real ca.key, which the daemon user cannot read. -func newTestCA(t *testing.T) CALoader { - t.Helper() +// Minted once per package rather than per harness. An RSA-2048 keygen costs +// ~140ms under GOFIPS140, and every fixture needs a CA, which dominated the +// package's runtime. The CA is immutable, so sharing it is safe. +var sharedTestCA = sync.OnceValues(func() (*x509.Certificate, *rsa.PrivateKey) { key, err := rsa.GenerateKey(rand.Reader, 2048) - require.NoError(t, err) + if err != nil { + panic(err) + } tmpl := x509.Certificate{ SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: "Test Cluster CA"}, @@ -43,9 +45,21 @@ func newTestCA(t *testing.T) CALoader { IsCA: true, } der, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, &key.PublicKey, key) - require.NoError(t, err) + if err != nil { + panic(err) + } cert, err := x509.ParseCertificate(der) - require.NoError(t, err) + if err != nil { + panic(err) + } + return cert, key +}) + +// newTestCA builds a throwaway CA so cert minting is exercised without needing +// the cluster's real ca.key, which the daemon user cannot read. +func newTestCA(t *testing.T) CALoader { + t.Helper() + cert, key := sharedTestCA() return func() (*x509.Certificate, *rsa.PrivateKey, error) { return cert, key, nil } } diff --git a/spinifex/handlers/rds/arn_parse_test.go b/spinifex/handlers/rds/arn_parse_test.go index 7ec2a5c73..1f2d68219 100644 --- a/spinifex/handlers/rds/arn_parse_test.go +++ b/spinifex/handlers/rds/arn_parse_test.go @@ -67,10 +67,9 @@ 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_CoverEveryResourceThatExists(t *testing.T) { - for _, kind := range []ResourceKind{ResourceKindDBInstance, ResourceKindDBSubnetGroup, ResourceKindDBParameterGroup} { + for _, kind := range []ResourceKind{ResourceKindDBInstance, ResourceKindDBSnapshot, + ResourceKindDBSubnetGroup, ResourceKindDBParameterGroup} { _, ok := taggableResources[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/commands.go b/spinifex/handlers/rds/commands.go index 08a20bdfa..eda3c6165 100644 --- a/spinifex/handlers/rds/commands.go +++ b/spinifex/handlers/rds/commands.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "strconv" "strings" "time" @@ -17,7 +18,6 @@ import ( // long poll over the gateway; this publishes on the bus subject that poll is // subscribed to and waits for the reply the agent carries back on its next one. -// Command types. rds-8 adds the snapshot quiesce. const ( CommandSetPassword = "set-password" CommandApplyParams = "apply-params" @@ -25,6 +25,11 @@ const ( // Extends the in-guest filesystem onto a data volume the control plane has // already grown (D12). CommandGrowFilesystem = "grow-filesystem" + // Holds the engine at a checkpoint for the length of a snapshot, and releases + // it again (D10). The hold is a session the agent keeps open, so the pair is + // two commands rather than one with a duration. + CommandQuiesce = "quiesce" + CommandUnquiesce = "unquiesce" ) // Parameter names carried on a command. They are AWS-shaped rather than @@ -32,6 +37,11 @@ const ( const ( CommandParamMasterUsername = "MasterUsername" CommandParamMasterUserPassword = "MasterUserPassword" + + // The backup label the engine records, and how long the agent holds the + // quiesce before releasing it unasked. + CommandParamQuiesceLabel = "QuiesceLabel" + CommandParamQuiesceDeadlineSeconds = "QuiesceDeadlineSeconds" ) // Per-command budgets. A password apply is one statement; a parameter apply @@ -46,6 +56,17 @@ const ( // rather than proportional to the grow. growFilesystemTimeout = 180 * time.Second + // A quiesce forces an immediate checkpoint, which is bounded by the dirty set + // the same way a graceful stop is; the release is one statement. + quiesceTimeout = 120 * time.Second + unquiesceTimeout = 60 * time.Second + + // How long the agent holds the backup session before releasing it unasked. It + // covers the drain and the whole ec2.CreateSnapshot round trip with room to + // spare, so a control plane that dies mid-snapshot costs the engine this much + // time in backup mode rather than the rest of its life. + quiesceHold = 6 * time.Minute + // The channel is a live subscription rather than a durable queue (D8), so a // command published between two of the agent's polls reaches nobody. // Re-publishing on this interval closes that gap without queueing anything. @@ -94,6 +115,26 @@ func (s *Service) growFilesystem(ctx context.Context, accountID, dbInstanceIdent return err } +// Holds the engine at a checkpoint so the data volume can be snapshotted at a +// consistent point. The agent keeps the backup session open until the release +// below, or until the deadline expires — so a control plane that dies here does +// not leave the engine in backup mode indefinitely (D10). +func (s *Service) quiesceEngine(ctx context.Context, accountID, dbInstanceIdentifier, label string) error { + _, err := s.issueCommand(ctx, accountID, dbInstanceIdentifier, CommandQuiesce, quiesceTimeout, []Parameter{ + {Name: CommandParamQuiesceLabel, Value: label}, + {Name: CommandParamQuiesceDeadlineSeconds, Value: strconv.Itoa(int(quiesceHold.Seconds()))}, + }) + return err +} + +// Releases the held backup session. Idempotent on the agent side, so a release +// that races the deadline is a success rather than an error the caller has to +// interpret. +func (s *Service) unquiesceEngine(ctx context.Context, accountID, dbInstanceIdentifier string) error { + _, err := s.issueCommand(ctx, accountID, dbInstanceIdentifier, CommandUnquiesce, unquiesceTimeout, nil) + return err +} + // Publishes one directive and blocks until the agent replies to that command // ID, the budget expires, or ctx is cancelled. A reply reporting failure is // returned as an error carrying the agent's message, so the caller does not diff --git a/spinifex/handlers/rds/create.go b/spinifex/handlers/rds/create.go index 3115b1242..c62ee9f2b 100644 --- a/spinifex/handlers/rds/create.go +++ b/spinifex/handlers/rds/create.go @@ -181,7 +181,11 @@ func (s *Service) recordLaunch(ctx context.Context, kv jetstream.KeyValue, key, rec.ENIID = launched.CustomerENIID rec.ENIPrivateIP = launched.CustomerENIIP rec.DataVolumeID = launched.DataVolumeID - rec.StorageEncrypted = launched.DataVolumeEncrypted + // Only a launch that made the volume can report its encryption; a restore + // attaches one it created itself, whose encryption the record already carries. + if launched.CreatedDataVolume { + rec.StorageEncrypted = launched.DataVolumeEncrypted + } rec.DNSName = s.dnsName(accountID, rec.DBInstanceIdentifier) // Without northstar there is no resolvable name, so the endpoint is the ENI // IP itself — stable per D5 and therefore as durable as a hostname (D6). diff --git a/spinifex/handlers/rds/delete.go b/spinifex/handlers/rds/delete.go index 7371acfaa..d91c780a9 100644 --- a/spinifex/handlers/rds/delete.go +++ b/spinifex/handlers/rds/delete.go @@ -18,11 +18,11 @@ import ( "github.com/nats-io/nats.go/jetstream" ) -// Takes the D18 final snapshot and answers whether a volume is still -// referenced by one, which is what decides between deleting and retaining it. -type snapshotProvider interface { - CreateSnapshot(ctx context.Context, input *ec2.CreateSnapshotInput, accountID string) (*ec2.Snapshot, error) - DescribeSnapshots(ctx context.Context, input *ec2.DescribeSnapshotsInput, accountID string) (*ec2.DescribeSnapshotsOutput, error) +var errFinalSnapshotInProgress = errors.New("rds: final DB snapshot creation is still in progress") + +type finalSnapshotReservation struct { + creator bool + revision uint64 } // Tears the DB instance down. AWS requires the caller to choose explicitly @@ -63,6 +63,14 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta "DB instance %s is already being deleted with %s; a retry must repeat that choice", id, snapshotChoice(rec.FinalSnapshotIdentifier)) } + // Reserve the final snapshot before the instance enters deleting. A collision + // therefore fails while the VM and data volume are still intact, and a crash + // afterwards leaves a creating record the reconciler can resolve. + reservation, err := s.reserveFinalSnapshot(ctx, kv, accountID, rec, finalSnapshot) + if err != nil { + return nil, err + } + // A repeat of a delete already under way re-runs the teardown rather than // being rejected: the first call may have died partway through it. if rec.Status != StatusDeleting { @@ -73,6 +81,9 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta rec.TransitionStartedAt = &now rec.UpdatedAt = now if err := updateJSON(ctx, kv, DBInstanceKey(id), rev, rec); err != nil { + if reservation.creator { + s.rollbackFinalSnapshotReservation(ctx, kv, finalSnapshot, reservation.revision) + } if errors.Is(err, jetstream.ErrKeyExists) { return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, "DB instance %s changed state concurrently; retry the delete", id) @@ -82,7 +93,7 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, id, "DB instance deleted.", EventCategoryDeletion) } - if err := s.teardownDBInstance(ctx, kv, accountID, rec); err != nil { + if err := s.teardownDBInstance(ctx, kv, accountID, rec, reservation.creator); err != nil { return nil, err } @@ -112,7 +123,7 @@ func validateDeleteRequest(input *rds.DeleteDBInstanceInput) (string, error) { "FinalDBSnapshotIdentifier is required unless SkipFinalSnapshot is set") } if identifier != "" { - if err := validateDBInstanceIdentifier(identifier); err != nil { + if err := validateDBSnapshotIdentifier(identifier); err != nil { return "", err } } @@ -133,8 +144,11 @@ func snapshotChoice(identifier string) string { // earlier one. Only a record naming this same instance and its current data // volume is a resumed teardown's own work. func finalSnapshotIsOurs(existing *DBSnapshotRecord, rec *DBInstanceRecord) bool { - return existing.DBInstanceIdentifier == rec.DBInstanceIdentifier && + sameSource := existing.DBInstanceIdentifier == rec.DBInstanceIdentifier && existing.SourceVolumeID == rec.DataVolumeID + legacyFinal := rec.Status == StatusDeleting && + rec.FinalSnapshotIdentifier == existing.DBSnapshotIdentifier + return sameSource && (existing.FinalSnapshot || legacyFinal) } // Rejects a taken identifier before anything is torn down, as AWS does. Leaving @@ -162,7 +176,8 @@ func (s *Service) checkFinalSnapshotAvailable(ctx context.Context, kv jetstream. // // Every step treats a missing resource as done, so this is safe to re-run from // any point it stopped at. -func (s *Service) teardownDBInstance(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord) error { +func (s *Service) teardownDBInstance(ctx context.Context, kv jetstream.KeyValue, accountID string, + rec *DBInstanceRecord, finalSnapshotCreator bool) error { // Asked to stop cleanly even though the VM is about to go: it is what makes // the final snapshot a clean checkpoint rather than one needing WAL replay. if rec.FinalSnapshotIdentifier != "" { @@ -172,7 +187,7 @@ func (s *Service) teardownDBInstance(ctx context.Context, kv jetstream.KeyValue, if err := s.terminateInstanceVM(ctx, rec.InstanceID); err != nil { return fmt.Errorf("rds: terminate the VM behind %s: %w", rec.DBInstanceIdentifier, err) } - if err := s.takeFinalSnapshot(ctx, kv, accountID, rec); err != nil { + if err := s.takeFinalSnapshot(ctx, kv, accountID, rec, finalSnapshotCreator); err != nil { return err } if err := s.releaseDataVolume(ctx, kv, accountID, rec); err != nil { @@ -216,30 +231,119 @@ func (s *Service) terminateInstanceVM(ctx context.Context, instanceID string) er return nil } -// Records the D18 final snapshot under db-snapshots/{id}. The record is written -// with Create, so a resumed teardown that already took the snapshot does not -// take a second one. -func (s *Service) takeFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord) error { - if rec.FinalSnapshotIdentifier == "" || rec.DataVolumeID == "" { - return nil +// Reserves the final snapshot name before deletion becomes destructive. The +// creator is the only worker allowed to cut new data; retries may adopt data it +// already cut, but cannot race it into making a duplicate snapshot. +func (s *Service) reserveFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, accountID string, + rec *DBInstanceRecord, identifier string) (finalSnapshotReservation, error) { + if identifier == "" || rec.DataVolumeID == "" { + return finalSnapshotReservation{}, nil } - key := DBSnapshotKey(rec.FinalSnapshotIdentifier) + + record := newDBSnapshotRecord(accountID, rec, &validatedSnapshot{ + DBSnapshotIdentifier: identifier, + Tags: rec.Tags, + }) + record.FinalSnapshot = true + rev, err := createJSONRevision(ctx, kv, DBSnapshotKey(identifier), &record) + if err == nil { + return finalSnapshotReservation{creator: true, revision: rev}, nil + } + if !errors.Is(err, jetstream.ErrKeyExists) { + return finalSnapshotReservation{}, err + } + var existing DBSnapshotRecord - found, err := getJSON(ctx, kv, key, &existing) + _, found, err := getJSONRevision(ctx, kv, DBSnapshotKey(identifier), &existing) if err != nil { - return err + return finalSnapshotReservation{}, err } - if found { - if !finalSnapshotIsOurs(&existing, rec) { - return awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, - "DB snapshot %s already exists", rec.FinalSnapshotIdentifier) - } + if !found || !finalSnapshotIsOurs(&existing, rec) { + return finalSnapshotReservation{}, awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, + "DB snapshot %s already exists", identifier) + } + return finalSnapshotReservation{}, nil +} + +// Removes a reservation when the instance never entered deleting or the EC2 +// create failed. The revision guard prevents cleanup from deleting a record +// another worker has already completed. +func (s *Service) rollbackFinalSnapshotReservation(ctx context.Context, kv jetstream.KeyValue, + identifier string, rev uint64) { + if identifier == "" || rev == 0 { + return + } + rbCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), rollbackTimeout) + defer cancel() + if err := kv.Delete(rbCtx, DBSnapshotKey(identifier), jetstream.LastRevision(rev)); err != nil && + !errors.Is(err, jetstream.ErrKeyNotFound) { + slog.WarnContext(rbCtx, "rds: rollback of a final snapshot reservation failed", + "dbSnapshot", identifier, "err", err) + } +} + +// Records the final snapshot under the reservation written before teardown. A +// retry first adopts matching EC2 data left by a dead creator; it never cuts a +// second snapshot while that creator may still be running. +func (s *Service) takeFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, accountID string, + rec *DBInstanceRecord, creator bool) error { + if rec.FinalSnapshotIdentifier == "" || rec.DataVolumeID == "" { return nil } if s.deps.Snapshots == nil { return errors.New("rds: no snapshot service configured") } + key := DBSnapshotKey(rec.FinalSnapshotIdentifier) + var record DBSnapshotRecord + rev, found, err := getJSONRevision(ctx, kv, key, &record) + if err != nil { + return err + } + if !found { + reservation, err := s.reserveFinalSnapshot(ctx, kv, accountID, rec, rec.FinalSnapshotIdentifier) + if err != nil { + return err + } + creator = reservation.creator + rev, found, err = getJSONRevision(ctx, kv, key, &record) + if err != nil { + return err + } + if !found { + return fmt.Errorf("rds: final snapshot reservation %s is missing", rec.FinalSnapshotIdentifier) + } + } + if !finalSnapshotIsOurs(&record, rec) { + return awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, + "DB snapshot %s already exists", rec.FinalSnapshotIdentifier) + } + if record.Status == SnapshotStatusAvailable && record.SnapshotID != "" { + return nil + } + if record.Status != SnapshotStatusCreating { + return awserrors.Errorf(awserrors.ErrorDBSnapshotInvalidState, + "DB snapshot %s is %s; expected %s", rec.FinalSnapshotIdentifier, record.Status, SnapshotStatusCreating) + } + + if !creator { + snapshotID, err := s.findEC2SnapshotFor(ctx, accountID, rec.FinalSnapshotIdentifier) + if err != nil { + return err + } + if snapshotID == "" { + return errFinalSnapshotInProgress + } + completed, err := s.completeFinalSnapshot(ctx, kv, rec, snapshotID) + if err != nil { + return err + } + if completed { + s.recordFinalSnapshotCreated(ctx, accountID, rec, snapshotID) + } + return nil + } + // System-owned, like the volume it is taken from: the customer addresses it // by its RDS identifier, never by the EC2 snapshot underneath. snapshot, err := s.deps.Snapshots.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{ @@ -250,43 +354,80 @@ func (s *Service) takeFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, Tags: []*ec2.Tag{ {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, {Key: aws.String(rdsInstanceTagKey), Value: aws.String(rec.DBInstanceIdentifier)}, + {Key: aws.String(rdsSnapshotTagKey), Value: aws.String(rec.FinalSnapshotIdentifier)}, + {Key: aws.String(rdsSnapshotAccountTagKey), Value: aws.String(accountID)}, }, }}, }, utils.GlobalAccountID) if err != nil { + s.rollbackFinalSnapshotReservation(ctx, kv, rec.FinalSnapshotIdentifier, rev) return fmt.Errorf("rds: take the final snapshot of %s: %w", rec.DBInstanceIdentifier, err) } if snapshot == nil || aws.StringValue(snapshot.SnapshotId) == "" { + s.rollbackFinalSnapshotReservation(ctx, kv, rec.FinalSnapshotIdentifier, rev) return fmt.Errorf("rds: take the final snapshot of %s: empty snapshot id", rec.DBInstanceIdentifier) } - - // Everything a restore needs is copied here rather than referenced: the DB - // instance record is deleted moments later. - record := DBSnapshotRecord{ - DBSnapshotIdentifier: rec.FinalSnapshotIdentifier, - DBInstanceIdentifier: rec.DBInstanceIdentifier, - AccountID: accountID, - SnapshotType: SnapshotTypeManual, - Status: SnapshotStatusAvailable, - SnapshotID: aws.StringValue(snapshot.SnapshotId), - SourceVolumeID: rec.DataVolumeID, - Engine: rec.Engine, - EngineVersion: rec.EngineVersion, - AllocatedStorage: rec.AllocatedStorage, - StorageType: rec.StorageType, - StorageEncrypted: rec.StorageEncrypted, - MasterUsername: rec.MasterUsername, - Port: rec.Port, - VpcID: rec.VpcID, - Tags: rec.Tags, - CreatedAt: time.Now().UTC(), - } - if err := createJSON(ctx, kv, key, &record); err != nil && !errors.Is(err, jetstream.ErrKeyExists) { + snapshotID := aws.StringValue(snapshot.SnapshotId) + completed, err := s.completeFinalSnapshot(ctx, kv, rec, snapshotID) + if err != nil { return fmt.Errorf("rds: record the final snapshot of %s: %w", rec.DBInstanceIdentifier, err) } + if completed { + s.recordFinalSnapshotCreated(ctx, accountID, rec, snapshotID) + } + return nil +} + +// Completes only the snapshot fields under CAS, preserving tags changed while +// the EC2 snapshot was being cut. +func (s *Service) completeFinalSnapshot(ctx context.Context, kv jetstream.KeyValue, + source *DBInstanceRecord, snapshotID string) (bool, error) { + key := DBSnapshotKey(source.FinalSnapshotIdentifier) + for range tagWriteAttempts { + var record DBSnapshotRecord + rev, found, err := getJSONRevision(ctx, kv, key, &record) + if err != nil { + return false, err + } + if !found { + return false, fmt.Errorf("final snapshot reservation %s is missing", source.FinalSnapshotIdentifier) + } + if !finalSnapshotIsOurs(&record, source) { + return false, awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, + "DB snapshot %s already exists", source.FinalSnapshotIdentifier) + } + if record.Status == SnapshotStatusAvailable { + if record.SnapshotID != snapshotID { + return false, fmt.Errorf("final snapshot %s already points to %s, not %s", + source.FinalSnapshotIdentifier, record.SnapshotID, snapshotID) + } + return false, nil + } + if record.Status != SnapshotStatusCreating { + return false, awserrors.Errorf(awserrors.ErrorDBSnapshotInvalidState, + "DB snapshot %s is %s; expected %s", source.FinalSnapshotIdentifier, + record.Status, SnapshotStatusCreating) + } + + record.SnapshotID = snapshotID + record.Status = SnapshotStatusAvailable + if err := updateJSON(ctx, kv, key, rev, &record); err == nil { + return true, nil + } else if !errors.Is(err, jetstream.ErrKeyExists) { + return false, err + } + } + return false, fmt.Errorf("rds: completing final snapshot %s contended after %d attempts", + source.FinalSnapshotIdentifier, tagWriteAttempts) +} + +func (s *Service) recordFinalSnapshotCreated(ctx context.Context, accountID string, + rec *DBInstanceRecord, snapshotID string) { s.RecordEvent(ctx, accountID, EventSourceTypeDBSnapshot, rec.FinalSnapshotIdentifier, "Final DB snapshot created.", EventCategoryBackup, EventCategoryCreation) - return nil + slog.InfoContext(ctx, "rds: final DB snapshot created", + "dbSnapshot", rec.FinalSnapshotIdentifier, "dbInstance", rec.DBInstanceIdentifier, + "snapshotId", snapshotID) } // A viperblock snapshot references its source volume's chunk files rather than diff --git a/spinifex/handlers/rds/delete_test.go b/spinifex/handlers/rds/delete_test.go index 51525d325..9366a74f7 100644 --- a/spinifex/handlers/rds/delete_test.go +++ b/spinifex/handlers/rds/delete_test.go @@ -137,6 +137,8 @@ func TestDeleteDBInstance_TakesTheFinalSnapshotAfterTheEngineAndVMAreDown(t *tes assert.Equal(t, CommandStopEngine, issued[0].Type) require.Len(t, h.snaps.created, 1) assert.Equal(t, "vol-rdsdata01", aws.StringValue(h.snaps.created[0].VolumeId)) + assert.Equal(t, testAccountID, + tagOf(h.snaps.created[0].TagSpecifications, rdsSnapshotAccountTagKey)) // Everything a restore needs is copied onto the snapshot record: the DB // instance record is deleted moments later. @@ -149,6 +151,62 @@ func TestDeleteDBInstance_TakesTheFinalSnapshotAfterTheEngineAndVMAreDown(t *tes assert.Equal(t, "vol-rdsdata01", snapshot.SourceVolumeID) } +func TestDeleteDBInstance_ReservesTheFinalSnapshotBeforeCuttingIt(t *testing.T) { + h := newLifecycleHarness(t, false) + seedInstance(t, h.svc, availableRecord()) + + h.snaps.beforeCreate = func() { + record, found := h.snapshotRecord(t, "orders-db-final") + require.True(t, found) + assert.Equal(t, SnapshotStatusCreating, record.Status) + assert.Empty(t, record.SnapshotID) + } + + _, err := h.svc.DeleteDBInstance(t.Context(), &rds.DeleteDBInstanceInput{ + DBInstanceIdentifier: aws.String(testDBID), + FinalDBSnapshotIdentifier: aws.String("orders-db-final"), + }, testAccountID) + require.NoError(t, err) +} + +// A dead worker can leave the EC2 snapshot cut while its RDS record still says +// creating. A retry adopts that exact account-scoped snapshot instead of +// cutting another one and permanently pinning the source volume twice. +func TestDeleteDBInstance_AdoptsAnInterruptedFinalSnapshot(t *testing.T) { + h := newLifecycleHarness(t, false) + rec := availableRecord() + rec.Status = StatusDeleting + rec.FinalSnapshotIdentifier = "orders-db-final" + seedInstance(t, h.svc, rec) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + creating := newDBSnapshotRecord(testAccountID, &rec, &validatedSnapshot{ + DBSnapshotIdentifier: rec.FinalSnapshotIdentifier, + Tags: rec.Tags, + }) + require.NoError(t, createJSON(t.Context(), kv, DBSnapshotKey(rec.FinalSnapshotIdentifier), &creating)) + h.snaps.holding = []string{"snap-cut-before-crash"} + h.snaps.tagged = map[string]map[string]string{ + "snap-cut-before-crash": { + rdsSnapshotTagKey: rec.FinalSnapshotIdentifier, + rdsSnapshotAccountTagKey: testAccountID, + }, + } + + _, err = h.svc.DeleteDBInstance(t.Context(), &rds.DeleteDBInstanceInput{ + DBInstanceIdentifier: aws.String(testDBID), + FinalDBSnapshotIdentifier: aws.String(rec.FinalSnapshotIdentifier), + }, testAccountID) + require.NoError(t, err) + + assert.Empty(t, h.snaps.created, "the retry must adopt the cut snapshot, not create another") + stored, found := h.snapshotRecord(t, rec.FinalSnapshotIdentifier) + require.True(t, found) + assert.Equal(t, SnapshotStatusAvailable, stored.Status) + assert.Equal(t, "snap-cut-before-crash", stored.SnapshotID) +} + // D10: a snapshot references its source volume's chunks, so the volume cannot // be deleted while the final snapshot survives. It is retained and recorded // with the snapshots holding it. diff --git a/spinifex/handlers/rds/launch.go b/spinifex/handlers/rds/launch.go index 0832281ef..8090b4a08 100644 --- a/spinifex/handlers/rds/launch.go +++ b/spinifex/handlers/rds/launch.go @@ -132,10 +132,11 @@ type LaunchOutput struct { DataDevice string MgmtIP string // The volume's own reported state, not an echo of the request, so - // DescribeDBInstances reports encryption the way EC2 does. Only set when - // this launch created the volume: a replace re-attaches one whose - // encryption the record already carries. + // DescribeDBInstances reports encryption the way EC2 does. Meaningful only + // when this launch created the volume; a replace or restore attaches one + // whose encryption the record already carries. DataVolumeEncrypted bool + CreatedDataVolume bool // Tears down everything this launch created, for a caller that fails after // it returned. The launch runs it itself on its own failures, so it is only // ever invoked once. @@ -308,6 +309,7 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o DataDevice: device, MgmtIP: sysOut.MgmtIP, DataVolumeEncrypted: volumeEncrypted, + CreatedDataVolume: in.ExistingDataVolume == "", Unwind: unwind, }, nil } diff --git a/spinifex/handlers/rds/lifecycle_test.go b/spinifex/handlers/rds/lifecycle_test.go index 80ceaa9b7..c30863a88 100644 --- a/spinifex/handlers/rds/lifecycle_test.go +++ b/spinifex/handlers/rds/lifecycle_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "fmt" + "slices" "strings" "sync" "testing" @@ -64,37 +66,95 @@ func (f *fakeInstanceCommander) StartStoppedInstance(_ context.Context, instance // DescribeSnapshots reports against the data volume, which is what decides // between deleting the volume and retaining it. type fakeSnapshots struct { - created []*ec2.CreateSnapshotInput - holding []string - createErr error - describeErr error + created []*ec2.CreateSnapshotInput + holding []string + deleted []string + createErr error + describeErr error + deleteErr error + beforeCreate func() + afterDescribe func() + // The tags each created snapshot carries, which is what a tag-filtered + // describe resolves a DB snapshot identifier through. + tagged map[string]map[string]string } var _ snapshotProvider = (*fakeSnapshots)(nil) func (f *fakeSnapshots) CreateSnapshot(_ context.Context, input *ec2.CreateSnapshotInput, _ string) (*ec2.Snapshot, error) { + if f.beforeCreate != nil { + f.beforeCreate() + } if f.createErr != nil { return nil, f.createErr } f.created = append(f.created, input) - id := "snap-0001" + id := fmt.Sprintf("snap-%04d", len(f.created)) + if f.tagged == nil { + f.tagged = map[string]map[string]string{} + } + f.tagged[id] = map[string]string{} + for _, spec := range input.TagSpecifications { + for _, tag := range spec.Tags { + f.tagged[id][aws.StringValue(tag.Key)] = aws.StringValue(tag.Value) + } + } // A snapshot pins the chunks of the volume it was taken from, so taking one // is also what makes that volume undeletable. f.holding = append(f.holding, id) return &ec2.Snapshot{SnapshotId: aws.String(id), VolumeId: input.VolumeId}, nil } -func (f *fakeSnapshots) DescribeSnapshots(_ context.Context, _ *ec2.DescribeSnapshotsInput, _ string) (*ec2.DescribeSnapshotsOutput, error) { +// A tag filter is answered from the recorded tags; anything else reports every +// holder, which is what the volume-release path asks for. +func (f *fakeSnapshots) DescribeSnapshots(_ context.Context, input *ec2.DescribeSnapshotsInput, _ string) (*ec2.DescribeSnapshotsOutput, error) { if f.describeErr != nil { return nil, f.describeErr } out := &ec2.DescribeSnapshotsOutput{} for _, id := range f.holding { + if !f.matchesFilters(id, input) { + continue + } out.Snapshots = append(out.Snapshots, &ec2.Snapshot{SnapshotId: aws.String(id)}) } + if f.afterDescribe != nil { + f.afterDescribe() + } return out, nil } +func (f *fakeSnapshots) DescribeSnapshotsStrict(ctx context.Context, input *ec2.DescribeSnapshotsInput, + accountID string) (*ec2.DescribeSnapshotsOutput, error) { + return f.DescribeSnapshots(ctx, input, accountID) +} + +func (f *fakeSnapshots) matchesFilters(id string, input *ec2.DescribeSnapshotsInput) bool { + if input == nil { + return true + } + for _, filter := range input.Filters { + key, ok := strings.CutPrefix(aws.StringValue(filter.Name), "tag:") + if !ok { + continue + } + if !slices.Contains(aws.StringValueSlice(filter.Values), f.tagged[id][key]) { + return false + } + } + return true +} + +func (f *fakeSnapshots) DeleteSnapshot(_ context.Context, input *ec2.DeleteSnapshotInput, _ string) (*ec2.DeleteSnapshotOutput, error) { + if f.deleteErr != nil { + return nil, f.deleteErr + } + id := aws.StringValue(input.SnapshotId) + f.deleted = append(f.deleted, id) + f.holding = slices.DeleteFunc(f.holding, func(held string) bool { return held == id }) + return &ec2.DeleteSnapshotOutput{}, nil +} + // stubAgent answers the command channel the way the in-guest agent does: it // replies to whatever lands on the command subject, correlated by command ID. type stubAgent struct { diff --git a/spinifex/handlers/rds/reconciler.go b/spinifex/handlers/rds/reconciler.go index faa96e357..58d0ac658 100644 --- a/spinifex/handlers/rds/reconciler.go +++ b/spinifex/handlers/rds/reconciler.go @@ -33,6 +33,11 @@ const ( // reconciler calls it failed. It covers a cold boot plus a WAL replay, which // is the longest a restart can honestly take. transitionTimeout = 10 * time.Minute + + // How long a DB snapshot record may sit in creating before the reconciler + // settles it against EC2. Comfortably past the snapshot request's own budget, + // so a record still being written by a live worker is never touched. + snapshotResolveTimeout = 15 * time.Minute ) // The EC2 lifecycle states a DB VM may be in and still be on its way up. The @@ -198,6 +203,9 @@ func (r *Reconciler) reconcileAccount(ctx context.Context, kv jetstream.KeyValue failures = append(failures, awserrors.Errorf(id, "%w", err)) } } + if err := r.reconcileSnapshots(ctx, kv, accountID); err != nil { + failures = append(failures, err) + } return errors.Join(failures...) } @@ -221,6 +229,8 @@ func (r *Reconciler) reconcileInstance(ctx context.Context, kv jetstream.KeyValu return r.reconcileStopping(ctx, kv, rev, accountID, &rec) case StatusDeleting: return r.reconcileDeleting(ctx, kv, rev, accountID, &rec) + case StatusBackingUp: + return r.reconcileBackingUp(ctx, kv, rev, accountID, &rec) case StatusAvailable, StatusFailed: return r.reconcileHealth(ctx, kv, rev, accountID, &rec) default: @@ -368,8 +378,8 @@ func (r *Reconciler) reconcileStopping(ctx context.Context, kv jetstream.KeyValu // did; only a teardown still stuck past the bound is called failed, which is // what lets the customer retry the delete. func (r *Reconciler) reconcileDeleting(ctx context.Context, kv jetstream.KeyValue, rev uint64, accountID string, rec *DBInstanceRecord) error { - err := r.svc.teardownDBInstance(ctx, kv, accountID, rec) - if err == nil { + err := r.svc.teardownDBInstance(ctx, kv, accountID, rec, false) + if err == nil || errors.Is(err, errFinalSnapshotInProgress) { return nil } if time.Since(transitionStarted(rec)) > transitionTimeout { @@ -381,6 +391,145 @@ func (r *Reconciler) reconcileDeleting(ctx context.Context, kv jetstream.KeyValu return nil } +// A snapshot holds its instance in backing-up for the length of the snapshot and +// no longer, so an instance still there past the bound belongs to a worker that +// died. It is returned to where the snapshot found it; the quiesce needs no +// undoing, because the agent's own hold deadline is far shorter than this bound +// and has already released the engine. +func (r *Reconciler) reconcileBackingUp(ctx context.Context, kv jetstream.KeyValue, rev uint64, accountID string, rec *DBInstanceRecord) error { + if time.Since(transitionStarted(rec)) <= transitionTimeout { + return nil + } + // The operation and the backing-up status are written in one CAS, so a record + // without one predates this phase; the health classifier settles the guess. + resume := StatusAvailable + if rec.SnapshotOperation != nil { + resume = rec.SnapshotOperation.ResumeStatus + } + rec.SnapshotOperation = nil + slog.WarnContext(ctx, "rds reconciler: a snapshot left its instance in backing-up; returning it", + "dbInstance", rec.DBInstanceIdentifier, "accountId", accountID, "resume", resume) + return r.transition(ctx, kv, rev, rec, resume, "") +} + +// Settles the DB snapshot records a create never finished writing. The EC2 +// snapshot is the authority: it was tagged with the DB snapshot identifier before +// the record was flipped, so its presence says the data exists and its absence +// says the create died before cutting it. +func (r *Reconciler) reconcileSnapshots(ctx context.Context, kv jetstream.KeyValue, accountID string) error { + ids, err := ListDBSnapshotIDs(ctx, kv) + if err != nil { + return fmt.Errorf("list DB snapshots: %w", err) + } + var failures []error + for _, id := range ids { + var rec DBSnapshotRecord + rev, found, err := getJSONRevision(ctx, kv, DBSnapshotKey(id), &rec) + if err != nil { + failures = append(failures, fmt.Errorf("read DB snapshot %s: %w", id, err)) + continue + } + if !found || rec.Status != SnapshotStatusCreating || + time.Since(rec.CreatedAt) <= snapshotResolveTimeout { + continue + } + if err := r.resolveCreatingSnapshot(ctx, kv, rev, accountID, &rec); err != nil { + failures = append(failures, fmt.Errorf("resolve DB snapshot %s: %w", id, err)) + } + } + return errors.Join(failures...) +} + +// Either adopts the EC2 snapshot the dead worker cut, or withdraws the record so +// its identifier is usable again. A record left in creating would otherwise hold +// the name forever while naming nothing a customer can restore. +func (r *Reconciler) resolveCreatingSnapshot(ctx context.Context, kv jetstream.KeyValue, rev uint64, + accountID string, rec *DBSnapshotRecord) error { + snapshotID, err := r.svc.findEC2SnapshotFor(ctx, accountID, rec.DBSnapshotIdentifier) + if err != nil { + return err + } + if snapshotID == "" { + if err := kv.Delete(ctx, DBSnapshotKey(rec.DBSnapshotIdentifier), jetstream.LastRevision(rev)); err != nil { + switch { + case errors.Is(err, jetstream.ErrKeyNotFound), errors.Is(err, jetstream.ErrKeyExists): + // A concurrent completion or delete owns the newer revision. + return nil + default: + return err + } + } + r.svc.RecordEvent(ctx, accountID, EventSourceTypeDBSnapshot, rec.DBSnapshotIdentifier, + "The DB snapshot could not be completed and has been removed.", + EventCategoryFailure, EventCategoryBackup) + slog.WarnContext(ctx, "rds reconciler: withdrew a DB snapshot whose data was never cut", + "dbSnapshot", rec.DBSnapshotIdentifier, "accountId", accountID) + return nil + } + + rec.SnapshotID = snapshotID + rec.Status = SnapshotStatusAvailable + // The worker died before it could report whether the quiesce held, so the + // conservative reading is the one that never overstates the snapshot. + rec.CrashConsistent = true + if err := updateJSON(ctx, kv, DBSnapshotKey(rec.DBSnapshotIdentifier), rev, rec); err != nil { + if errors.Is(err, jetstream.ErrKeyExists) { + return nil + } + return err + } + r.svc.RecordEvent(ctx, accountID, EventSourceTypeDBSnapshot, rec.DBSnapshotIdentifier, + "DB snapshot created.", EventCategoryBackup, EventCategoryCreation) + slog.InfoContext(ctx, "rds reconciler: adopted the EC2 snapshot of an unfinished DB snapshot", + "dbSnapshot", rec.DBSnapshotIdentifier, "snapshotId", snapshotID, "accountId", accountID) + return nil +} + +// The EC2 snapshot an account-scoped DB snapshot identifier owns, empty when +// none was cut. Strict lookup is required because a partial metadata scan cannot +// prove absence and must never cause reconciliation to withdraw the RDS record. +func (s *Service) findEC2SnapshotFor(ctx context.Context, accountID, dbSnapshotIdentifier string) (string, error) { + if s.deps.Snapshots == nil { + return "", errors.New("rds: no snapshot service configured") + } + out, err := s.deps.Snapshots.DescribeSnapshotsStrict(ctx, &ec2.DescribeSnapshotsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:" + rdsSnapshotTagKey), + Values: aws.StringSlice([]string{dbSnapshotIdentifier}), + }, + { + Name: aws.String("tag:" + rdsSnapshotAccountTagKey), + Values: aws.StringSlice([]string{accountID}), + }, + }, + }, utils.GlobalAccountID) + if err != nil { + if awserrors.IsNotFound(err) { + return "", nil + } + return "", fmt.Errorf("rds: find the EC2 snapshot of %s in account %s: %w", + dbSnapshotIdentifier, accountID, err) + } + if out == nil { + return "", nil + } + + var found string + for _, snapshot := range out.Snapshots { + id := aws.StringValue(snapshot.SnapshotId) + if id == "" { + continue + } + if found != "" { + return "", fmt.Errorf("rds: DB snapshot %s in account %s has multiple EC2 snapshots (%s and %s)", + dbSnapshotIdentifier, accountID, found, id) + } + found = id + } + return found, nil +} + // When the transition began. A record written by an older control plane carries // no stamp, so its last write stands in — it is never earlier than the // transition, so the bound cannot be under-counted. diff --git a/spinifex/handlers/rds/reconciler_test.go b/spinifex/handlers/rds/reconciler_test.go index b022346ee..02d5a2703 100644 --- a/spinifex/handlers/rds/reconciler_test.go +++ b/spinifex/handlers/rds/reconciler_test.go @@ -163,25 +163,21 @@ func TestReconciler_LeavesASlowBootstrapAloneInsideTheWindow(t *testing.T) { assert.Equal(t, StatusCreating, status) } -// A deliberately stopped instance, and a transition owned by a later phase, are -// both left alone: touching one would race its owner. available and failed are -// the classifier's, and covered in recovery_test.go. +// A deliberately stopped instance is left alone: touching it would race its +// owner. available and failed are the classifier's, and covered in +// recovery_test.go. func TestReconciler_LeavesSettledInstancesAlone(t *testing.T) { - for _, status := range []Status{StatusStopped, StatusBackingUp} { - t.Run(string(status), func(t *testing.T) { - h := newReconcileHarness(t) - rec := healthyRecord() - rec.Status = status - rec.CreatedAt = time.Now().UTC().Add(-24 * time.Hour) - seedInstance(t, h.svc, rec) + h := newReconcileHarness(t) + rec := healthyRecord() + rec.Status = StatusStopped + rec.CreatedAt = time.Now().UTC().Add(-24 * time.Hour) + seedInstance(t, h.svc, rec) - require.NoError(t, h.rec.reconcileOnce(t.Context())) + require.NoError(t, h.rec.reconcileOnce(t.Context())) - got, _ := h.statusOf(t, testDBID) - assert.Equal(t, status, got) - assert.Empty(t, h.state.calls, "a settled instance is not even inspected") - }) - } + got, _ := h.statusOf(t, testDBID) + assert.Equal(t, StatusStopped, got) + assert.Empty(t, h.state.calls, "a settled instance is not even inspected") } // A VM-state lookup that fails is not evidence the instance is unhealthy, so the diff --git a/spinifex/handlers/rds/records.go b/spinifex/handlers/rds/records.go index cf4b71e30..76a633082 100644 --- a/spinifex/handlers/rds/records.go +++ b/spinifex/handlers/rds/records.go @@ -89,6 +89,14 @@ type DBInstanceRecord struct { // resumed delete takes the same final snapshot rather than none. FinalSnapshotIdentifier string `json:"finalSnapshotIdentifier,omitempty"` + // The snapshot currently being taken of this instance. Nil when none is. + SnapshotOperation *SnapshotOperation `json:"snapshotOperation,omitempty"` + + // The DB snapshot this instance was restored from, if any. Kept so a + // DeleteDBSnapshot the restored volume still blocks can name what to remove + // first rather than reporting an opaque in-use fault. + RestoredFromDBSnapshot string `json:"restoredFromDbSnapshot,omitempty"` + // Static parameters written to the engine's config but not yet in effect. // Cleared by the reboot that applies them (D16). PendingRebootParameters []string `json:"pendingRebootParameters,omitempty"` @@ -183,9 +191,24 @@ const ( SnapshotTypeManual = "manual" SnapshotTypeAutomated = "automated" + // The record is written creating before the EC2 snapshot is taken, so a crash + // in between leaves a reconcilable trace rather than an orphaned EC2 snapshot. + SnapshotStatusCreating = "creating" SnapshotStatusAvailable = "available" ) +// The snapshot operation holding a DB instance, written under the same CAS that +// moves it to backing-up so a second request is rejected rather than queued. An +// rds-9 automated snapshot and a manual one serialise against each other here. +type SnapshotOperation struct { + DBSnapshotIdentifier string `json:"dbSnapshotIdentifier"` + // Where the instance goes when the snapshot finishes. Recorded rather than + // assumed, because snapshotting a stopped instance must not leave it looking + // available. + ResumeStatus Status `json:"resumeStatus"` + StartedAt time.Time `json:"startedAt"` +} + // The db-snapshots/{id} record. The EC2 snapshot holds the data; this is the // RDS-level metadata a restore needs and DescribeDBSnapshots projects, captured // at snapshot time because the DB instance it describes may be gone by then. @@ -195,6 +218,10 @@ type DBSnapshotRecord struct { AccountID string `json:"accountId"` SnapshotType string `json:"snapshotType"` Status string `json:"status"` + // Distinguishes a delete reservation from a concurrent manual snapshot of + // the same instance and volume. Older final snapshots are inferred while + // their source instance remains in deleting. + FinalSnapshot bool `json:"finalSnapshot,omitempty"` // The EC2 snapshot the data lives in, and the volume whose chunks it // references — which is why that volume is retained rather than deleted. @@ -203,12 +230,25 @@ type DBSnapshotRecord struct { Engine string `json:"engine"` EngineVersion string `json:"engineVersion"` + DBInstanceClass string `json:"dbInstanceClass,omitempty"` AllocatedStorage int64 `json:"allocatedStorage"` StorageType string `json:"storageType,omitempty"` StorageEncrypted bool `json:"storageEncrypted,omitempty"` + DBName string `json:"dbName,omitempty"` MasterUsername string `json:"masterUsername"` Port int64 `json:"port"` - VpcID string `json:"vpcId,omitempty"` + + // The placement the source instance had, which a restore falls back to for + // every field the request leaves unspecified. + VpcID string `json:"vpcId,omitempty"` + VpcSecurityGroupIDs []string `json:"vpcSecurityGroupIds,omitempty"` + DBSubnetGroupName string `json:"dbSubnetGroupName,omitempty"` + DBParameterGroupName string `json:"dbParameterGroupName,omitempty"` + + // Copied from the source instance so a restore keeps its rotation history: a + // later ModifyDBInstance --master-user-password still works, and the restored + // instance keeps the credentials the datadir was written with. + MasterPasswordUpdatedAt *time.Time `json:"masterPasswordUpdatedAt,omitempty"` // True when the engine was still writing as it was taken, so a restore // replays WAL. A final snapshot is taken with the engine already down, so it @@ -220,6 +260,12 @@ type DBSnapshotRecord struct { CreatedAt time.Time `json:"createdAt"` } +var _ TaggedRecord = (*DBSnapshotRecord)(nil) + +func (r *DBSnapshotRecord) GetTags() map[string]string { return r.Tags } + +func (r *DBSnapshotRecord) SetTags(tags map[string]string) { r.Tags = tags } + // A data volume that outlived its DB instance because a COW snapshot still // references its chunks (D10). The last DeleteDBSnapshot to empty Snapshots // deletes it; rds-9's reaper is the backstop for a crash in between. @@ -229,7 +275,9 @@ type RetainedVolumeRecord struct { // The instance it belonged to, so an operator can attribute the footprint // after the DB instance record is gone. DBInstanceIdentifier string `json:"dbInstanceIdentifier"` - // The DB snapshot identifiers holding it alive. + // The EC2 snapshot IDs holding it alive, read from the volume store's own + // index rather than from the RDS key space: that index is what DeleteVolume + // enforces against, so it is the only list a release can trust. Snapshots []string `json:"snapshots"` // Set when the volume store refused the delete without naming a holder, so a // release must re-check rather than read the empty list as "nothing holds it". diff --git a/spinifex/handlers/rds/restore.go b/spinifex/handlers/rds/restore.go new file mode 100644 index 000000000..e2138c97e --- /dev/null +++ b/spinifex/handlers/rds/restore.go @@ -0,0 +1,404 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + "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" + handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" + "github.com/mulgadc/spinifex/spinifex/tags" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats.go/jetstream" +) + +// Builds a new DB instance on a volume created from the snapshot. It is a fresh +// instance in every respect but its data: a new identifier, customer ENI, DNS +// name, serving certificate and VM. What it inherits is the datadir — which +// already holds the database, its roles and their password hashes, so the engine +// starts on it rather than running initdb (D10). +func (s *Service) RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rds.RestoreDBInstanceFromDBSnapshotInput, accountID string) (out *rds.RestoreDBInstanceFromDBSnapshotOutput, err error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + if err := validateDBInstanceIdentifier(aws.StringValue(input.DBInstanceIdentifier)); err != nil { + return nil, err + } + if err := validateDBSnapshotIdentifier(aws.StringValue(input.DBSnapshotIdentifier)); err != nil { + return nil, err + } + if err := rejectUnimplementedRestore(input); err != nil { + return nil, err + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + snapshot, _, err := s.getDBSnapshot(ctx, kv, aws.StringValue(input.DBSnapshotIdentifier)) + if err != nil { + return nil, err + } + if snapshot.Status != SnapshotStatusAvailable || snapshot.SnapshotID == "" { + return nil, awserrors.Errorf(awserrors.ErrorDBSnapshotInvalidState, + "DB snapshot %s is %s; it must be %s to restore from", + snapshot.DBSnapshotIdentifier, snapshot.Status, SnapshotStatusAvailable) + } + + req, err := resolveRestoreRequest(input, snapshot) + if err != nil { + return nil, err + } + placement, err := s.resolvePlacement(ctx, kv, accountID, req) + if err != nil { + return nil, err + } + parameters, err := s.resolveGroupParameters(ctx, kv, accountID, req.DBParameterGroupName, req.InstanceClass) + if err != nil { + return nil, err + } + + // The record is the identifier's reservation, exactly as at create, and is + // withdrawn on any failure below. + key := DBInstanceKey(req.Identifier) + rec := newRestoredDBInstanceRecord(accountID, req, placement, parameters, snapshot) + if createErr := createJSON(ctx, kv, key, &rec); createErr != nil { + if errors.Is(createErr, jetstream.ErrKeyExists) { + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceAlreadyExists, + "DB instance %s already exists", req.Identifier) + } + return nil, createErr + } + defer func() { + if err == nil { + return + } + rbCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), rollbackTimeout) + defer cancel() + if delErr := kv.Delete(rbCtx, key); delErr != nil { + slog.WarnContext(rbCtx, "rds: rollback delete of reserved restored DB instance record failed", + "dbInstance", req.Identifier, "err", delErr) + } + }() + + // The datadir the restored engine comes up on. Created before the VM because + // the launch attaches an existing volume rather than making one — and unwound + // here, since only a launch that created a volume unwinds it. + volume, err := s.createRestoreVolume(ctx, req, snapshot) + if err != nil { + return nil, err + } + defer func() { + if err != nil { + s.discardRestoreVolume(ctx, volume.id) + } + }() + // Read from the volume rather than echoed from the snapshot: a cluster whose + // storage key has gone would otherwise report encryption it is not giving. + if snapshot.StorageEncrypted && !volume.encrypted { + return nil, fmt.Errorf("rds: volume %s restored from %s came back unencrypted; the cluster storage key is not configured", + volume.id, snapshot.DBSnapshotIdentifier) + } + + launched, err := LaunchDBInstanceVM(ctx, s.deps.Launch, LaunchInput{ + DBInstanceIdentifier: req.Identifier, + AccountID: accountID, + SubnetID: placement.SubnetID, + SecurityGroupIDs: placement.SecurityGroupIDs, + Engine: req.Engine.Name, + EngineVersion: req.EngineVersion, + InstanceType: req.InstanceType, + AllocatedStorage: req.AllocatedStorage, + ExistingDataVolume: volume.id, + UserData: buildAgentUserData(agentUserDataInput{ + GatewayURL: s.deps.GatewayURL, + GatewayCACert: s.deps.GatewayCACert, + Region: s.region, + DBInstanceIdentifier: req.Identifier, + EngineVersion: req.EngineVersion, + EnginePort: req.Port, + }), + IamInstanceProfileArn: ensureInstanceProfile(s.deps.IAM, utils.GlobalAccountID), + }) + if err != nil { + return nil, err + } + + stored, err := s.recordLaunch(ctx, kv, key, accountID, launched) + if err != nil { + s.unwindLaunched(ctx, launched) + return nil, err + } + if err = s.PutInstanceIndex(ctx, launched.InstanceID, InstanceIndexEntry{ + AccountID: accountID, + DBInstanceIdentifier: req.Identifier, + VMGeneration: firstVMGeneration, + }); err != nil { + s.unwindLaunched(ctx, launched) + return nil, fmt.Errorf("rds: write instance index for %s: %w", req.Identifier, err) + } + s.publishDNS(ctx, accountID, stored, handlers_dns.ActionUpsert) + + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, req.Identifier, + fmt.Sprintf("Restored from DB snapshot %s.", snapshot.DBSnapshotIdentifier), + EventCategoryCreation, EventCategoryRecovery) + slog.InfoContext(ctx, "rds: DB instance restored from snapshot", + "dbInstance", req.Identifier, "dbSnapshot", snapshot.DBSnapshotIdentifier, + "accountId", accountID, "instanceId", launched.InstanceID, "dataVolumeId", volume.id) + + return &rds.RestoreDBInstanceFromDBSnapshotOutput{DBInstance: s.projectDBInstance(stored)}, nil +} + +// Every field the request left out comes from the snapshot's own recorded +// configuration, so a bare restore reproduces the instance the snapshot was +// taken from rather than defaulting away from it. +func resolveRestoreRequest(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord) (*validatedCreate, error) { + // The snapshot's engine, never the request's: the datadir is written in one + // engine's on-disk format and no other can read it. + engine, err := LookupEngine(snapshot.Engine) + if err != nil { + return nil, err + } + if requested := aws.StringValue(input.Engine); requested != "" && !strings.EqualFold(requested, engine.Name) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DB snapshot %s holds %s data; restoring it as %s is not offered", + snapshot.DBSnapshotIdentifier, engine.Name, requested) + } + + instanceClass := aws.StringValue(input.DBInstanceClass) + if instanceClass == "" { + instanceClass = snapshot.DBInstanceClass + } + instanceType, err := InstanceTypeForClass(instanceClass) + if err != nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBInstanceClass %q is not supported; supported classes are %s", instanceClass, strings.Join(SupportedInstanceClasses(), ", ")) + } + + storage, err := resolveRestoreStorage(input, snapshot) + if err != nil { + return nil, err + } + storageType, err := resolveRestoreStorageType(input, snapshot) + if err != nil { + return nil, err + } + port, err := resolveRestorePort(input, snapshot, engine) + if err != nil { + return nil, err + } + + if name := aws.StringValue(input.DBName); name != "" && name != snapshot.DBName { + return nil, unimplemented("DBName", + "a restore starts on the snapshot's datadir, so it cannot create a database the snapshot does not hold") + } + paramGroup := aws.StringValue(input.DBParameterGroupName) + if paramGroup == "" { + paramGroup = snapshot.DBParameterGroupName + } + if paramGroup == "" { + paramGroup = engine.DefaultParameterGroupName() + } + subnetGroup := aws.StringValue(input.DBSubnetGroupName) + if subnetGroup == "" { + subnetGroup = snapshot.DBSubnetGroupName + } + securityGroups := aws.StringValueSlice(input.VpcSecurityGroupIds) + if len(securityGroups) == 0 { + securityGroups = snapshot.VpcSecurityGroupIDs + } + + tagMap, err := validateTags(input.Tags) + if err != nil { + return nil, err + } + + return &validatedCreate{ + Identifier: aws.StringValue(input.DBInstanceIdentifier), + Engine: engine, + EngineVersion: snapshot.EngineVersion, + InstanceClass: instanceClass, + InstanceType: instanceType, + AllocatedStorage: storage, + StorageType: storageType, + Port: port, + MasterUsername: snapshot.MasterUsername, + // Deliberately empty: the datadir already holds the master role and its + // password hash, so there is nothing for a bootstrap to set (D8). + DBName: snapshot.DBName, + SecurityGroupIDs: securityGroups, + DBSubnetGroupName: subnetGroup, + DBParameterGroupName: paramGroup, + DeletionProtection: aws.BoolValue(input.DeletionProtection), + Tags: tagMap, + }, nil +} + +// Storage may only be grown: CreateVolume refuses a size below the snapshot's, +// and a shrink has nowhere to put the data the snapshot already holds. +func resolveRestoreStorage(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord) (int64, error) { + if input.AllocatedStorage == nil { + return snapshot.AllocatedStorage, nil + } + requested := aws.Int64Value(input.AllocatedStorage) + if requested < snapshot.AllocatedStorage { + return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "AllocatedStorage may not be below the %d GiB DB snapshot %s holds", + snapshot.AllocatedStorage, snapshot.DBSnapshotIdentifier) + } + if requested > maxAllocatedStorageGiB { + return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "AllocatedStorage must be between %d and %d GiB", minAllocatedStorageGiB, maxAllocatedStorageGiB) + } + return requested, nil +} + +func resolveRestoreStorageType(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord) (string, error) { + storageType := strings.ToLower(strings.TrimSpace(aws.StringValue(input.StorageType))) + if storageType == "" { + storageType = snapshot.StorageType + } + if storageType == "" { + return storageTypeGP3, nil + } + if storageType != storageTypeGP3 { + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "StorageType %q is not supported; only %q is offered", storageType, storageTypeGP3) + } + return storageType, nil +} + +func resolveRestorePort(input *rds.RestoreDBInstanceFromDBSnapshotInput, snapshot *DBSnapshotRecord, engine Engine) (int64, error) { + if input.Port != nil { + port := aws.Int64Value(input.Port) + if port < minDBPort || port > maxDBPort { + return 0, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Port must be between %d and %d", minDBPort, maxDBPort) + } + return port, nil + } + if snapshot.Port > 0 { + return snapshot.Port, nil + } + return engine.DefaultPort, nil +} + +// D19: a parameter that would create a false safety, security or availability +// guarantee is rejected rather than silently dropped. The list mirrors the one +// at create, plus what only a restore can be asked for. +func rejectUnimplementedRestore(input *rds.RestoreDBInstanceFromDBSnapshotInput) error { + if aws.BoolValue(input.MultiAZ) { + return unimplemented("MultiAZ", "this platform is single-AZ; a standby would not exist") + } + if aws.BoolValue(input.PubliclyAccessible) { + return unimplemented("PubliclyAccessible", + "the endpoint is a private VPC address; a public one would not be reachable") + } + if aws.StringValue(input.DBClusterSnapshotIdentifier) != "" { + return unimplemented("DBClusterSnapshotIdentifier", "clustered engines are not offered") + } + if aws.Int64Value(input.Iops) > 0 { + return unimplemented("Iops", "provisioned IOPS are not implemented; storage is gp3") + } + if aws.Int64Value(input.StorageThroughput) > 0 { + return unimplemented("StorageThroughput", "provisioned throughput is not implemented; storage is gp3") + } + if aws.StringValue(input.AvailabilityZone) != "" { + return unimplemented("AvailabilityZone", "this platform exposes a single zone") + } + if aws.BoolValue(input.EnableIAMDatabaseAuthentication) { + return unimplemented("EnableIAMDatabaseAuthentication", "IAM database authentication is not implemented") + } + if len(input.EnableCloudwatchLogsExports) > 0 { + return unimplemented("EnableCloudwatchLogsExports", "log export is not implemented") + } + if aws.StringValue(input.OptionGroupName) != "" { + return unimplemented("OptionGroupName", "option groups are not offered") + } + if aws.StringValue(input.Domain) != "" { + return unimplemented("Domain", "Active Directory domain joining is not implemented") + } + if aws.StringValue(input.CustomIamInstanceProfile) != "" { + return unimplemented("CustomIamInstanceProfile", "the DB VM's instance profile is platform-owned") + } + if aws.StringValue(input.TdeCredentialArn) != "" { + return unimplemented("TdeCredentialArn", "storage is encrypted with the cluster key, not a TDE credential") + } + if aws.BoolValue(input.EnableCustomerOwnedIp) { + return unimplemented("EnableCustomerOwnedIp", "customer-owned IP addressing is an Outposts feature") + } + return nil +} + +// The reserved record for a restored instance: everything the request and the +// snapshot decided, with the bootstrap marked consumed so the agent's first +// fetch returns attach rather than a password it would use to run initdb (D8). +func newRestoredDBInstanceRecord(accountID string, req *validatedCreate, placement *endpointPlacement, + parameters []Parameter, snapshot *DBSnapshotRecord) DBInstanceRecord { + rec := newDBInstanceRecord(accountID, req, placement, parameters) + now := time.Now().UTC() + rec.Bootstrap.Consumed = true + rec.Bootstrap.ConsumedAt = &now + // Carried over so a later ModifyDBInstance --master-user-password still reads + // as a rotation of the credentials the datadir was written with. + rec.MasterPasswordUpdatedAt = snapshot.MasterPasswordUpdatedAt + rec.StorageEncrypted = snapshot.StorageEncrypted + rec.RestoredFromDBSnapshot = snapshot.DBSnapshotIdentifier + return rec +} + +// The restored data volume and what it reported about itself. +type restoredVolume struct { + id string + encrypted bool +} + +// System-account owned like every RDS data volume, so the customer reaches it +// only through the DB instance. +func (s *Service) createRestoreVolume(ctx context.Context, req *validatedCreate, snapshot *DBSnapshotRecord) (*restoredVolume, error) { + if s.deps.Launch.Volume == nil || s.deps.Launch.Config == nil { + return nil, errors.New("rds: no volume service configured") + } + volume, err := s.deps.Launch.Volume.CreateVolume(ctx, &ec2.CreateVolumeInput{ + AvailabilityZone: aws.String(s.deps.Launch.Config.AZ), + Size: aws.Int64(req.AllocatedStorage), + VolumeType: aws.String(storageTypeGP3), + SnapshotId: aws.String(snapshot.SnapshotID), + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String("volume"), + Tags: []*ec2.Tag{ + {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, + {Key: aws.String(rdsInstanceTagKey), Value: aws.String(req.Identifier)}, + }, + }}, + }, utils.GlobalAccountID) + if err != nil { + return nil, fmt.Errorf("rds: create the data volume for %s from %s: %w", + req.Identifier, snapshot.DBSnapshotIdentifier, err) + } + if volume == nil || aws.StringValue(volume.VolumeId) == "" { + return nil, fmt.Errorf("rds: create the data volume for %s from %s: empty volume id", + req.Identifier, snapshot.DBSnapshotIdentifier) + } + return &restoredVolume{id: aws.StringValue(volume.VolumeId), encrypted: aws.BoolValue(volume.Encrypted)}, nil +} + +// A volume left behind would hold its source snapshot undeletable for a restore +// that never produced an instance, so the delete runs on a context detached from +// the caller's. +func (s *Service) discardRestoreVolume(ctx context.Context, volumeID string) { + rbCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), rollbackTimeout) + defer cancel() + if _, err := s.deps.Launch.Volume.DeleteVolume(rbCtx, &ec2.DeleteVolumeInput{ + VolumeId: aws.String(volumeID), + }, utils.GlobalAccountID); err != nil && !awserrors.IsNotFound(err) { + slog.WarnContext(rbCtx, "rds: rollback delete of a restored data volume failed", + "volumeId", volumeID, "err", err) + } +} diff --git a/spinifex/handlers/rds/restore_test.go b/spinifex/handlers/rds/restore_test.go new file mode 100644 index 000000000..f70eac4da --- /dev/null +++ b/spinifex/handlers/rds/restore_test.go @@ -0,0 +1,253 @@ +package handlers_rds + +import ( + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/tags" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testRestoredID = "orders-db-restored" + +// seedSnapshot takes a real snapshot of a seeded instance, so a restore reads +// the record CreateDBSnapshot actually writes rather than a hand-built one. +func (h *snapshotHarness) seedSnapshot(t *testing.T) DBSnapshotRecord { + t.Helper() + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + rec, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + return rec +} + +func restoreInput() *rds.RestoreDBInstanceFromDBSnapshotInput { + return &rds.RestoreDBInstanceFromDBSnapshotInput{ + DBInstanceIdentifier: aws.String(testRestoredID), + DBSnapshotIdentifier: aws.String(testSnapshotID), + } +} + +func TestRestoreDBInstanceFromDBSnapshot_BuildsANewInstanceOnTheSnapshotsData(t *testing.T) { + h := newSnapshotHarness(t, false) + snapshot := h.seedSnapshot(t) + + out, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.NoError(t, err) + + require.NotNil(t, out.DBInstance) + assert.Equal(t, testRestoredID, aws.StringValue(out.DBInstance.DBInstanceIdentifier)) + assert.Equal(t, string(StatusCreating), aws.StringValue(out.DBInstance.DBInstanceStatus)) + + // The datadir comes from the snapshot rather than from a fresh volume, so the + // launch attaches a volume the restore created instead of making one. + created := h.launch.volumes.created + require.Len(t, created, 1) + assert.Equal(t, snapshot.SnapshotID, aws.StringValue(created[0].SnapshotId)) + assert.Equal(t, int64(20), aws.Int64Value(created[0].Size)) + assert.Equal(t, tags.ManagedByRDS, tagOf(created[0].TagSpecifications, tags.ManagedByKey)) + // System-owned like every RDS data volume, so the customer reaches it only + // through the DB instance. + assert.Equal(t, []string{utils.GlobalAccountID}, h.launch.volumes.accts) + + stored := h.instance(t, testRestoredID) + assert.Equal(t, "vol-rdsdata01", stored.DataVolumeID) + assert.Equal(t, testSnapshotID, stored.RestoredFromDBSnapshot) + assert.True(t, stored.StorageEncrypted) + + // D8: the datadir already holds the master role and its password hash, so + // the agent's first bootstrap fetch has to attach rather than run initdb. + assert.True(t, stored.Bootstrap.Consumed) + require.NotNil(t, stored.Bootstrap.ConsumedAt) + assert.Empty(t, stored.Bootstrap.MasterUserPassword) +} + +// Everything the request left out comes from the snapshot, so a bare restore +// reproduces the instance it was taken from rather than defaulting away from it. +func TestRestoreDBInstanceFromDBSnapshot_FallsBackToTheSnapshotsConfiguration(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.NoError(t, err) + + stored := h.instance(t, testRestoredID) + assert.Equal(t, "db.t3.medium", stored.DBInstanceClass) + assert.Equal(t, int64(20), stored.AllocatedStorage) + assert.Equal(t, "postgres", stored.Engine) + assert.Equal(t, "18.1", stored.EngineVersion) + assert.Equal(t, "postgres", stored.MasterUsername) + assert.Equal(t, "orders", stored.DBName) + assert.Equal(t, int64(5432), stored.Port) + assert.Equal(t, []string{testDefaultSG}, stored.VpcSecurityGroupIDs) +} + +func TestRestoreDBInstanceFromDBSnapshot_HonoursTheRequestedOverrides(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + input := restoreInput() + input.DBInstanceClass = aws.String("db.t3.large") + input.AllocatedStorage = aws.Int64(50) + input.Port = aws.Int64(6432) + input.Tags = []*rds.Tag{{Key: aws.String("env"), Value: aws.String("staging")}} + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + require.NoError(t, err) + + stored := h.instance(t, testRestoredID) + assert.Equal(t, "db.t3.large", stored.DBInstanceClass) + assert.Equal(t, int64(50), stored.AllocatedStorage) + assert.Equal(t, int64(6432), stored.Port) + assert.Equal(t, map[string]string{"env": "staging"}, stored.Tags) + assert.Equal(t, int64(50), aws.Int64Value(h.launch.volumes.created[0].Size)) +} + +// CreateVolume refuses a size below the snapshot's, and a shrink has nowhere to +// put the data the snapshot already holds. +func TestRestoreDBInstanceFromDBSnapshot_RejectsStorageBelowTheSnapshots(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + input := restoreInput() + input.AllocatedStorage = aws.Int64(10) + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Empty(t, h.launch.volumes.created) + assert.False(t, h.instanceExists(t, testRestoredID)) +} + +// The volume holds its source snapshot undeletable, so a restore that never +// produced an instance must not leave one behind. +func TestRestoreDBInstanceFromDBSnapshot_UnwindsTheVolumeAndTheRecordWhenTheLaunchFails(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + h.launch.launcher.err = errors.New("no node had capacity") + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.Error(t, err) + + assert.Contains(t, h.launch.volumes.deleted, "vol-rdsdata01") + assert.False(t, h.instanceExists(t, testRestoredID), + "the reserved identifier is withdrawn with everything else") + // The ENIs the launch made are its own to unwind; what matters here is that + // none survives the failure. + assert.NotEmpty(t, h.launch.unwind) +} + +func TestRestoreDBInstanceFromDBSnapshot_RejectsATakenIdentifier(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + input := restoreInput() + input.DBInstanceIdentifier = aws.String(testDBID) + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceAlreadyExists) + assert.Empty(t, h.launch.volumes.created) +} + +func TestRestoreDBInstanceFromDBSnapshot_RejectsASnapshotThatDoesNotExist(t *testing.T) { + h := newSnapshotHarness(t, false) + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotNotFound) +} + +// A snapshot still being taken has no data to restore from. +func TestRestoreDBInstanceFromDBSnapshot_RejectsASnapshotStillBeingTaken(t *testing.T) { + h := newSnapshotHarness(t, false) + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + creating := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + Engine: "postgres", + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &creating)) + + _, err = h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotInvalidState) +} + +// D19: a parameter that would create a false safety, security or availability +// guarantee is rejected rather than silently dropped. +func TestRestoreDBInstanceFromDBSnapshot_RejectsUnimplementedParameters(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + cases := map[string]func(*rds.RestoreDBInstanceFromDBSnapshotInput){ + "MultiAZ": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { in.MultiAZ = aws.Bool(true) }, + "PubliclyAccessible": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { in.PubliclyAccessible = aws.Bool(true) }, + "Iops": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { in.Iops = aws.Int64(3000) }, + "AvailabilityZone": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { + in.AvailabilityZone = aws.String("ap-southeast-2b") + }, + "OptionGroupName": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { in.OptionGroupName = aws.String("custom") }, + "EnableIAMDatabaseAuthentication": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { + in.EnableIAMDatabaseAuthentication = aws.Bool(true) + }, + "DBClusterSnapshotIdentifier": func(in *rds.RestoreDBInstanceFromDBSnapshotInput) { + in.DBClusterSnapshotIdentifier = aws.String("cluster-snap") + }, + } + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + input := restoreInput() + mutate(input) + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), name) + }) + } +} + +// The datadir is written in one engine's on-disk format and no other can read +// it, so a request naming a different engine is refused rather than honoured. +func TestRestoreDBInstanceFromDBSnapshot_RejectsAnEngineChange(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + + input := restoreInput() + input.Engine = aws.String("mysql") + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), input, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) +} + +// Read from the volume rather than echoed from the snapshot: a cluster whose +// storage key has gone would otherwise report encryption it is not giving. +func TestRestoreDBInstanceFromDBSnapshot_RefusesAnUnencryptedVolumeForAnEncryptedSnapshot(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshot(t) + h.launch.volumes.encrypted = false + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "unencrypted") + assert.Contains(t, h.launch.volumes.deleted, "vol-rdsdata01") + assert.False(t, h.instanceExists(t, testRestoredID)) +} + +func (h *snapshotHarness) instanceExists(t *testing.T, id string) bool { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var rec DBInstanceRecord + found, err := getJSON(t.Context(), kv, DBInstanceKey(id), &rec) + require.NoError(t, err) + return found +} diff --git a/spinifex/handlers/rds/service.go b/spinifex/handlers/rds/service.go index 426de3ce5..4e318ea3c 100644 --- a/spinifex/handlers/rds/service.go +++ b/spinifex/handlers/rds/service.go @@ -183,14 +183,22 @@ func putJSON(ctx context.Context, kv jetstream.KeyValue, key string, v any) erro // Writes v at key only if nothing is stored there, so the key doubles as a // cluster-wide reservation. Returns jetstream.ErrKeyExists when it is taken. func createJSON(ctx context.Context, kv jetstream.KeyValue, key string, v any) error { + _, err := createJSONRevision(ctx, kv, key, v) + return err +} + +// createJSON plus the created entry's revision, for callers that must undo only +// the reservation they created rather than a concurrent replacement. +func createJSONRevision(ctx context.Context, kv jetstream.KeyValue, key string, v any) (uint64, error) { data, err := json.Marshal(v) if err != nil { - return err + return 0, err } - if _, err := kv.Create(ctx, key, data); err != nil { - return err + rev, err := kv.Create(ctx, key, data) + if err != nil { + return 0, err } - return nil + return rev, nil } // Writes v at key only if the stored entry is still at rev. diff --git a/spinifex/handlers/rds/service_nats.go b/spinifex/handlers/rds/service_nats.go index 6f93c0a64..1e50b8393 100644 --- a/spinifex/handlers/rds/service_nats.go +++ b/spinifex/handlers/rds/service_nats.go @@ -26,6 +26,16 @@ const ( modifyTimeout = 6 * time.Minute ) +// A snapshot create holds the engine quiesced while the COW snapshot is cut, and +// a delete may have to release the retained source volume behind it. A restore +// creates a volume from the snapshot and then launches a VM on it, so it costs a +// create plus the volume. +const ( + snapshotTimeout = 6 * time.Minute + snapshotDeleteTimeout = 4 * time.Minute + restoreTimeout = 8 * time.Minute +) + // The gateway-side adapter that forwards each agent action as a NATS request to // the daemon's matching subscriber. type NATSService struct { @@ -81,6 +91,26 @@ func (s *NATSService) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBI SubjectDeleteDBInstance, input, deleteTimeout, accountID) } +func (s *NATSService) CreateDBSnapshot(ctx context.Context, input *rds.CreateDBSnapshotInput, accountID string) (*rds.CreateDBSnapshotOutput, error) { + return utils.NATSRequest[rds.CreateDBSnapshotOutput](ctx, s.nc, + SubjectCreateDBSnapshot, input, snapshotTimeout, accountID) +} + +func (s *NATSService) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDBSnapshotsInput, accountID string) (*rds.DescribeDBSnapshotsOutput, error) { + return utils.NATSRequest[rds.DescribeDBSnapshotsOutput](ctx, s.nc, + SubjectDescribeDBSnapshots, input, defaultTimeout, accountID) +} + +func (s *NATSService) DeleteDBSnapshot(ctx context.Context, input *rds.DeleteDBSnapshotInput, accountID string) (*rds.DeleteDBSnapshotOutput, error) { + return utils.NATSRequest[rds.DeleteDBSnapshotOutput](ctx, s.nc, + SubjectDeleteDBSnapshot, input, snapshotDeleteTimeout, accountID) +} + +func (s *NATSService) RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rds.RestoreDBInstanceFromDBSnapshotInput, accountID string) (*rds.RestoreDBInstanceFromDBSnapshotOutput, error) { + return utils.NATSRequest[rds.RestoreDBInstanceFromDBSnapshotOutput](ctx, s.nc, + SubjectRestoreDBInstanceFromDBSnapshot, input, restoreTimeout, accountID) +} + func (s *NATSService) DescribeEvents(ctx context.Context, input *rds.DescribeEventsInput, accountID string) (*rds.DescribeEventsOutput, error) { return utils.NATSRequest[rds.DescribeEventsOutput](ctx, s.nc, SubjectDescribeEvents, input, defaultTimeout, accountID) diff --git a/spinifex/handlers/rds/snapshot.go b/spinifex/handlers/rds/snapshot.go new file mode 100644 index 000000000..b00df7202 --- /dev/null +++ b/spinifex/handlers/rds/snapshot.go @@ -0,0 +1,630 @@ +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/mulgadc/spinifex/spinifex/tags" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats.go/jetstream" +) + +// The EC2 snapshot surface the RDS control plane drives. A DB snapshot is an +// ec2.CreateSnapshot of the instance's data volume wrapped in an agent quiesce +// (D10); DescribeSnapshots is also what answers whether a volume is still +// referenced, which is what decides between deleting and retaining it. +type snapshotProvider interface { + CreateSnapshot(ctx context.Context, input *ec2.CreateSnapshotInput, accountID string) (*ec2.Snapshot, error) + DescribeSnapshots(ctx context.Context, input *ec2.DescribeSnapshotsInput, accountID string) (*ec2.DescribeSnapshotsOutput, error) + // Unlike the customer-facing describe, this fails on any unreadable snapshot + // metadata so reconciliation never mistakes a partial result for absence. + DescribeSnapshotsStrict(ctx context.Context, input *ec2.DescribeSnapshotsInput, accountID string) (*ec2.DescribeSnapshotsOutput, error) + DeleteSnapshot(ctx context.Context, input *ec2.DeleteSnapshotInput, accountID string) (*ec2.DeleteSnapshotOutput, error) +} + +// AWS's bound. The character rules match a DB instance identifier's so a name +// accepted here is also accepted as a FinalDBSnapshotIdentifier at delete. +const maxDBSnapshotIdentifierLen = 255 + +// Links the EC2 snapshot back to the account-scoped RDS identity that owns it, +// so a record left stuck in creating cannot adopt another tenant's snapshot. +const ( + rdsSnapshotTagKey = "spinifex:rds-db-snapshot" + rdsSnapshotAccountTagKey = "spinifex:rds-db-snapshot-account" +) + +// Takes a customer-requested snapshot of the instance's data volume. The engine +// is held at a checkpoint for the length of it, so the captured datadir is a +// checkpoint rather than a mid-write state — and if it cannot be, the snapshot +// is still taken and reported as crash consistent (D10). +func (s *Service) CreateDBSnapshot(ctx context.Context, input *rds.CreateDBSnapshotInput, accountID string) (*rds.CreateDBSnapshotOutput, error) { + req, err := validateCreateSnapshotRequest(input) + if err != nil { + return nil, err + } + if s.deps.Snapshots == nil { + return nil, errors.New("rds: no snapshot service configured") + } + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + + rec, rev, err := s.getDBInstance(ctx, kv, req.DBInstanceIdentifier) + if err != nil { + return nil, err + } + if rec.DataVolumeID == "" { + return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s has no data volume to snapshot", req.DBInstanceIdentifier) + } + // Checked before the instance is moved to backing-up, so a request naming a + // taken identifier leaves the instance where it was. + if err := s.checkDBSnapshotAvailable(ctx, kv, req.DBSnapshotIdentifier); err != nil { + return nil, err + } + + // The same CAS that moves the instance to backing-up records the snapshot + // holding it, so a second snapshot request or a lifecycle op is rejected + // rather than entering the same window. + resume, err := s.beginSnapshotOperation(ctx, kv, rev, rec, req.DBSnapshotIdentifier) + if err != nil { + return nil, err + } + defer s.endSnapshotOperation(ctx, kv, rec.DBInstanceIdentifier, resume) + + // Written before the EC2 call, so a crash in between leaves a reconcilable + // record rather than an untracked EC2 snapshot. + record := newDBSnapshotRecord(accountID, rec, req) + key := DBSnapshotKey(req.DBSnapshotIdentifier) + if err := createJSON(ctx, kv, key, &record); err != nil { + if errors.Is(err, jetstream.ErrKeyExists) { + return nil, awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, + "DB snapshot %s already exists", req.DBSnapshotIdentifier) + } + return nil, err + } + + snapshotID, crashConsistent, err := s.snapshotDataVolume(ctx, accountID, rec, resume, req.DBSnapshotIdentifier) + if err != nil { + // The record only ever described a snapshot that now does not exist, so it + // goes with it rather than being left for the reconciler to puzzle over. + s.discardSnapshotRecord(ctx, kv, req.DBSnapshotIdentifier) + return nil, err + } + + record.SnapshotID = snapshotID + record.Status = SnapshotStatusAvailable + record.CrashConsistent = crashConsistent + if err := putJSON(ctx, kv, key, &record); err != nil { + return nil, err + } + + s.RecordEvent(ctx, accountID, EventSourceTypeDBSnapshot, req.DBSnapshotIdentifier, + "DB snapshot created.", EventCategoryBackup, EventCategoryCreation) + slog.InfoContext(ctx, "rds: DB snapshot created", + "dbSnapshot", req.DBSnapshotIdentifier, "dbInstance", rec.DBInstanceIdentifier, + "snapshotId", snapshotID, "crashConsistent", crashConsistent) + + return &rds.CreateDBSnapshotOutput{DBSnapshot: s.projectDBSnapshot(&record)}, nil +} + +// Quiesces the engine, snapshots the data volume, and releases the engine again +// however the snapshot went. Reports the EC2 snapshot and whether the engine was +// still writing when it was taken. +func (s *Service) snapshotDataVolume(ctx context.Context, accountID string, rec *DBInstanceRecord, + resume Status, dbSnapshotIdentifier string) (snapshotID string, crashConsistent bool, err error) { + // A stopped instance has no engine to hold: its datadir was sealed by the + // graceful stop, which is the checkpoint a quiesce would be forcing. + if resume == StatusAvailable { + if quiesceErr := s.quiesceEngine(ctx, accountID, rec.DBInstanceIdentifier, dbSnapshotIdentifier); quiesceErr != nil { + // D10: still a restorable backup, so it is taken and reported honestly + // rather than refused. The engine replays WAL on restore. + crashConsistent = true + slog.WarnContext(ctx, "rds: the engine could not be quiesced; taking a crash-consistent snapshot", + "dbInstance", rec.DBInstanceIdentifier, "dbSnapshot", dbSnapshotIdentifier, "err", quiesceErr) + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, + "The database engine could not be quiesced before the snapshot; the snapshot is crash consistent and will recover from its write-ahead log when it is restored.", + EventCategoryNotification, EventCategoryBackup) + } else { + defer s.releaseQuiesce(ctx, accountID, rec.DBInstanceIdentifier) + } + } + + // System-owned, like the volume it is taken from: the customer addresses it by + // its RDS identifier and can neither see nor delete the EC2 snapshot. + snapshot, err := s.deps.Snapshots.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{ + VolumeId: aws.String(rec.DataVolumeID), + Description: aws.String("RDS snapshot " + dbSnapshotIdentifier + " of " + rec.DBInstanceIdentifier), + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String("snapshot"), + Tags: []*ec2.Tag{ + {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, + {Key: aws.String(rdsInstanceTagKey), Value: aws.String(rec.DBInstanceIdentifier)}, + {Key: aws.String(rdsSnapshotTagKey), Value: aws.String(dbSnapshotIdentifier)}, + {Key: aws.String(rdsSnapshotAccountTagKey), Value: aws.String(accountID)}, + }, + }}, + }, utils.GlobalAccountID) + if err != nil { + return "", crashConsistent, fmt.Errorf("rds: snapshot the data volume of %s: %w", rec.DBInstanceIdentifier, err) + } + if snapshot == nil || aws.StringValue(snapshot.SnapshotId) == "" { + return "", crashConsistent, fmt.Errorf("rds: snapshot the data volume of %s: empty snapshot id", rec.DBInstanceIdentifier) + } + return aws.StringValue(snapshot.SnapshotId), crashConsistent, nil +} + +// Releases the quiesce on a context detached from the caller's, so a snapshot +// that failed because the request deadline expired still lets the engine out of +// backup mode rather than leaving it to the agent's own deadline. +func (s *Service) releaseQuiesce(ctx context.Context, accountID, dbInstanceIdentifier string) { + releaseCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), unquiesceTimeout) + defer cancel() + if err := s.unquiesceEngine(releaseCtx, accountID, dbInstanceIdentifier); err != nil { + slog.ErrorContext(ctx, "rds: releasing the engine quiesce failed; the agent will release it on its own deadline", + "dbInstance", dbInstanceIdentifier, "err", err) + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, dbInstanceIdentifier, + fmt.Sprintf("The database engine could not be released from backup mode after the snapshot; the instance agent releases it automatically within %s.", quiesceHold), + EventCategoryNotification, EventCategoryBackup) + } +} + +// The customer's view of one account's snapshots, filtered the three ways AWS +// scopes them. A named snapshot that does not exist is an error rather than an +// empty list, matching AWS: a client polling a create would otherwise read +// "gone" for "not ready". +func (s *Service) DescribeDBSnapshots(ctx context.Context, input *rds.DescribeDBSnapshotsInput, accountID string) (*rds.DescribeDBSnapshotsOutput, error) { + snapshotType, err := validateDescribeSnapshotsRequest(input) + if err != nil { + return nil, err + } + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + + if id := aws.StringValue(input.DBSnapshotIdentifier); id != "" { + rec, _, err := s.getDBSnapshot(ctx, kv, id) + if err != nil { + return nil, err + } + if !snapshotMatches(rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType) { + return &rds.DescribeDBSnapshotsOutput{DBSnapshots: []*rds.DBSnapshot{}}, nil + } + return &rds.DescribeDBSnapshotsOutput{DBSnapshots: []*rds.DBSnapshot{s.projectDBSnapshot(rec)}}, nil + } + + ids, err := ListDBSnapshotIDs(ctx, kv) + if err != nil { + return nil, err + } + slices.Sort(ids) + + snapshots := make([]*rds.DBSnapshot, 0, len(ids)) + for _, id := range ids { + var rec DBSnapshotRecord + found, err := getJSON(ctx, kv, DBSnapshotKey(id), &rec) + if err != nil { + return nil, err + } + // A snapshot deleted between the key listing and this read is simply gone, + // which is what a describe one tick later would report too. + if !found || !snapshotMatches(&rec, aws.StringValue(input.DBInstanceIdentifier), snapshotType) { + continue + } + snapshots = append(snapshots, s.projectDBSnapshot(&rec)) + } + return &rds.DescribeDBSnapshotsOutput{DBSnapshots: snapshots}, nil +} + +// An empty filter matches everything, as AWS does. +func snapshotMatches(rec *DBSnapshotRecord, dbInstanceIdentifier, snapshotType string) bool { + if dbInstanceIdentifier != "" && rec.DBInstanceIdentifier != dbInstanceIdentifier { + return false + } + return snapshotType == "" || rec.SnapshotType == snapshotType +} + +// Removes the snapshot and, when it was the last thing holding a data volume +// its DB instance already released, that volume too. A snapshot a restored +// instance still reads from is refused with the instance named, so the customer +// knows what to remove first. +func (s *Service) DeleteDBSnapshot(ctx context.Context, input *rds.DeleteDBSnapshotInput, accountID string) (*rds.DeleteDBSnapshotOutput, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + id := aws.StringValue(input.DBSnapshotIdentifier) + if err := validateDBSnapshotIdentifier(id); err != nil { + return nil, err + } + if s.deps.Snapshots == nil { + return nil, errors.New("rds: no snapshot service configured") + } + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + rec, _, err := s.getDBSnapshot(ctx, kv, id) + if err != nil { + return nil, err + } + // A snapshot still being taken has no data to remove and an in-flight writer + // that would recreate the record; the reconciler resolves it either way. + if rec.Status != SnapshotStatusAvailable { + return nil, awserrors.Errorf(awserrors.ErrorDBSnapshotInvalidState, + "DB snapshot %s is %s; it must be %s to be deleted", id, rec.Status, SnapshotStatusAvailable) + } + + if err := s.deleteEC2Snapshot(ctx, kv, accountID, rec); err != nil { + return nil, err + } + // After the EC2 snapshot, so the volume store no longer sees this reference + // when it decides whether the volume is still held. + if err := s.releaseRetainedVolume(ctx, kv, rec); err != nil { + return nil, err + } + // Last: while it exists the snapshot is still nameable, and a retry re-runs + // the steps above, each of which tolerates work it has already done. + if err := kv.Delete(ctx, DBSnapshotKey(id)); err != nil && !errors.Is(err, jetstream.ErrKeyNotFound) { + return nil, fmt.Errorf("rds: delete the DB snapshot record for %s: %w", id, err) + } + + s.RecordEvent(ctx, accountID, EventSourceTypeDBSnapshot, id, "DB snapshot deleted.", EventCategoryDeletion) + slog.InfoContext(ctx, "rds: DB snapshot deleted", "dbSnapshot", id, "snapshotId", rec.SnapshotID) + + // AWS answers with the snapshot as it last stood, the way a delete of a DB + // instance answers with the record it just removed. + return &rds.DeleteDBSnapshotOutput{DBSnapshot: s.projectDBSnapshot(rec)}, nil +} + +// A snapshot with a volume still reading through it cannot go, which is exactly +// the case where an instance restored from it is still running. That is +// translated into an RDS fault naming the instance rather than surfaced as the +// raw EC2 code, which says nothing a customer can act on. +func (s *Service) deleteEC2Snapshot(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBSnapshotRecord) error { + if rec.SnapshotID == "" { + return nil + } + _, err := s.deps.Snapshots.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{ + SnapshotId: aws.String(rec.SnapshotID), + }, utils.GlobalAccountID) + switch { + case err == nil || awserrors.IsNotFound(err): + return nil + case awserrors.IsErrorCode(err, awserrors.ErrorInvalidSnapshotInUse): + return awserrors.Errorf(awserrors.ErrorDBSnapshotInvalidState, + "DB snapshot %s cannot be deleted while %s reads from it", rec.DBSnapshotIdentifier, + s.describeSnapshotDependents(ctx, kv, rec.DBSnapshotIdentifier)) + default: + return fmt.Errorf("rds: delete the EC2 snapshot behind %s: %w", rec.DBSnapshotIdentifier, err) + } +} + +// What the customer has to remove before the snapshot can go. A restored +// instance is named; anything else is a volume outside RDS's own bookkeeping, +// which is reported as such rather than guessed at. +func (s *Service) describeSnapshotDependents(ctx context.Context, kv jetstream.KeyValue, dbSnapshotIdentifier string) string { + ids, err := ListDBInstanceIDs(ctx, kv) + if err != nil { + slog.WarnContext(ctx, "rds: listing the instances restored from a snapshot failed", + "dbSnapshot", dbSnapshotIdentifier, "err", err) + return "a volume created from it" + } + var dependents []string + for _, id := range ids { + var rec DBInstanceRecord + found, err := getJSON(ctx, kv, DBInstanceKey(id), &rec) + if err == nil && found && rec.RestoredFromDBSnapshot == dbSnapshotIdentifier { + dependents = append(dependents, id) + } + } + if len(dependents) == 0 { + return "a volume created from it" + } + slices.Sort(dependents) + return "DB instance " + joinNames(dependents) + ", restored from it," +} + +// Deletes a data volume its DB instance already gave up, once this was the last +// snapshot holding its chunks. The holders are re-read from the volume store +// rather than taken from the retained record: that index is what DeleteVolume +// enforces against, and a record written before another snapshot was taken would +// otherwise read as "nothing holds it". +func (s *Service) releaseRetainedVolume(ctx context.Context, kv jetstream.KeyValue, rec *DBSnapshotRecord) error { + if rec.SourceVolumeID == "" { + return nil + } + var retained RetainedVolumeRecord + found, err := getJSON(ctx, kv, RetainedVolumeKey(rec.SourceVolumeID), &retained) + if err != nil || !found { + return err + } + + holders, err := s.snapshotsHolding(ctx, rec.SourceVolumeID) + if err != nil { + return err + } + if len(holders) > 0 { + retained.Snapshots = holders + retained.HoldersUnresolved = false + return putJSON(ctx, kv, RetainedVolumeKey(rec.SourceVolumeID), &retained) + } + + if s.deps.Launch.Volume == nil { + return errors.New("rds: no volume service configured") + } + _, err = s.deps.Launch.Volume.DeleteVolume(ctx, &ec2.DeleteVolumeInput{ + VolumeId: aws.String(rec.SourceVolumeID), + }, utils.GlobalAccountID) + if err != nil && !awserrors.IsNotFound(err) { + return fmt.Errorf("rds: delete the retained data volume %s: %w", rec.SourceVolumeID, err) + } + if err := kv.Delete(ctx, RetainedVolumeKey(rec.SourceVolumeID)); err != nil && !errors.Is(err, jetstream.ErrKeyNotFound) { + return fmt.Errorf("rds: clear the retained-volume record for %s: %w", rec.SourceVolumeID, err) + } + slog.InfoContext(ctx, "rds: retained data volume reclaimed; its last snapshot is gone", + "volumeId", rec.SourceVolumeID, "dbSnapshot", rec.DBSnapshotIdentifier) + return nil +} + +// The request as CreateDBSnapshot resolved it. +type validatedSnapshot struct { + DBSnapshotIdentifier string + DBInstanceIdentifier string + Tags map[string]string +} + +func validateCreateSnapshotRequest(input *rds.CreateDBSnapshotInput) (*validatedSnapshot, error) { + if input == nil { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "empty request") + } + if err := validateDBSnapshotIdentifier(aws.StringValue(input.DBSnapshotIdentifier)); err != nil { + return nil, err + } + if err := validateDBInstanceIdentifier(aws.StringValue(input.DBInstanceIdentifier)); err != nil { + return nil, err + } + // Rejected before the instance is touched, so a snapshot with bad tags does + // not leave it in backing-up. + tagMap, err := validateTags(input.Tags) + if err != nil { + return nil, err + } + return &validatedSnapshot{ + DBSnapshotIdentifier: aws.StringValue(input.DBSnapshotIdentifier), + DBInstanceIdentifier: aws.StringValue(input.DBInstanceIdentifier), + Tags: tagMap, + }, nil +} + +// Returns the snapshot type to filter on, empty for "any". D19: a filter this +// phase cannot honour is rejected rather than dropped, because a silently +// unfiltered list reads as a complete answer. +func validateDescribeSnapshotsRequest(input *rds.DescribeDBSnapshotsInput) (string, error) { + if input == nil { + return "", nil + } + if len(input.Filters) > 0 { + return "", unimplemented("Filters", "DescribeDBSnapshots filters on DBSnapshotIdentifier, DBInstanceIdentifier and SnapshotType only") + } + if aws.StringValue(input.DbiResourceId) != "" { + return "", unimplemented("DbiResourceId", "a DB instance has no resource ID distinct from its identifier here") + } + if aws.BoolValue(input.IncludeShared) || aws.BoolValue(input.IncludePublic) { + return "", unimplemented("IncludeShared/IncludePublic", "cross-account snapshot sharing is not offered") + } + switch snapshotType := aws.StringValue(input.SnapshotType); snapshotType { + case "", SnapshotTypeManual, SnapshotTypeAutomated: + return snapshotType, nil + default: + return "", awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "SnapshotType %q is not offered; use %q or %q", snapshotType, SnapshotTypeManual, SnapshotTypeAutomated) + } +} + +// AWS's own rules. The character set is deliberately the DB instance +// identifier's rather than AWS's laxer one, so every name a snapshot can be +// created under is also a name DeleteDBInstance accepts for a final snapshot. +func validateDBSnapshotIdentifier(id string) error { + switch { + case id == "": + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBSnapshotIdentifier is required") + case len(id) > maxDBSnapshotIdentifierLen: + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBSnapshotIdentifier must be at most %d characters", maxDBSnapshotIdentifierLen) + case !isLetter(rune(id[0])): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBSnapshotIdentifier must begin with a letter") + case strings.HasSuffix(id, "-"): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBSnapshotIdentifier may not end with a hyphen") + case strings.Contains(id, "--"): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBSnapshotIdentifier may not contain consecutive hyphens") + } + for _, r := range id { + if !isDigit(r) && r != '-' && (r < 'a' || r > 'z') { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBSnapshotIdentifier may contain only lowercase letters, digits and hyphens") + } + } + return nil +} + +// Everything a restore needs, copied rather than referenced: the DB instance +// this describes may be gone by the time the snapshot is used. +func newDBSnapshotRecord(accountID string, rec *DBInstanceRecord, req *validatedSnapshot) DBSnapshotRecord { + return DBSnapshotRecord{ + DBSnapshotIdentifier: req.DBSnapshotIdentifier, + DBInstanceIdentifier: rec.DBInstanceIdentifier, + AccountID: accountID, + SnapshotType: SnapshotTypeManual, + Status: SnapshotStatusCreating, + SourceVolumeID: rec.DataVolumeID, + Engine: rec.Engine, + EngineVersion: rec.EngineVersion, + DBInstanceClass: rec.DBInstanceClass, + AllocatedStorage: rec.AllocatedStorage, + StorageType: rec.StorageType, + StorageEncrypted: rec.StorageEncrypted, + DBName: rec.DBName, + MasterUsername: rec.MasterUsername, + Port: rec.Port, + VpcID: rec.VpcID, + VpcSecurityGroupIDs: rec.VpcSecurityGroupIDs, + DBSubnetGroupName: rec.DBSubnetGroupName, + DBParameterGroupName: rec.DBParameterGroupName, + MasterPasswordUpdatedAt: rec.MasterPasswordUpdatedAt, + Tags: req.Tags, + CreatedAt: time.Now().UTC(), + } +} + +// Returns the record plus its revision, for callers that follow with a CAS. +func (s *Service) getDBSnapshot(ctx context.Context, kv jetstream.KeyValue, id string) (*DBSnapshotRecord, uint64, error) { + var rec DBSnapshotRecord + rev, found, err := getJSONRevision(ctx, kv, DBSnapshotKey(id), &rec) + if err != nil { + return nil, 0, err + } + if !found { + return nil, 0, awserrors.Errorf(awserrors.ErrorDBSnapshotNotFound, "DB snapshot %s not found", id) + } + return &rec, rev, nil +} + +// Rejects a taken identifier before any work starts. A snapshot record outlives +// the instance it came from, so a name can be held by a snapshot of an instance +// that no longer exists. +func (s *Service) checkDBSnapshotAvailable(ctx context.Context, kv jetstream.KeyValue, id string) error { + var existing DBSnapshotRecord + found, err := getJSON(ctx, kv, DBSnapshotKey(id), &existing) + if err != nil { + return err + } + if found { + return awserrors.Errorf(awserrors.ErrorDBSnapshotAlreadyExists, "DB snapshot %s already exists", id) + } + return nil +} + +// Withdraws a creating record whose snapshot never happened, on a context +// detached from the caller's so an expired request deadline still clears it. +func (s *Service) discardSnapshotRecord(ctx context.Context, kv jetstream.KeyValue, id string) { + rbCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), rollbackTimeout) + defer cancel() + if err := kv.Delete(rbCtx, DBSnapshotKey(id)); err != nil && !errors.Is(err, jetstream.ErrKeyNotFound) { + slog.WarnContext(rbCtx, "rds: withdrawing the record of a snapshot that failed left it behind", + "dbSnapshot", id, "err", err) + } +} + +// Moves the instance into backing-up and records the snapshot holding it in one +// CAS, returning the status the instance goes back to. That write is the +// per-instance guard: a second snapshot, an rds-9 automated one or a lifecycle +// op finds the instance already backing-up and is rejected rather than queued. +func (s *Service) beginSnapshotOperation(ctx context.Context, kv jetstream.KeyValue, rev uint64, + rec *DBInstanceRecord, dbSnapshotIdentifier string) (Status, error) { + legal := []Status{StatusAvailable, StatusStopped} + if !slices.Contains(legal, rec.Status) { + return "", awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s is %s; a snapshot requires it to be %s", + rec.DBInstanceIdentifier, rec.Status, joinStatuses(legal)) + } + + now := time.Now().UTC() + resume := rec.Status + rec.SnapshotOperation = &SnapshotOperation{ + DBSnapshotIdentifier: dbSnapshotIdentifier, + ResumeStatus: resume, + StartedAt: now, + } + rec.Status = StatusBackingUp + rec.TransitionStartedAt = &now + rec.UpdatedAt = now + + if err := updateJSON(ctx, kv, DBInstanceKey(rec.DBInstanceIdentifier), rev, rec); err != nil { + if errors.Is(err, jetstream.ErrKeyExists) { + return "", awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState, + "DB instance %s changed state concurrently; retry the snapshot", rec.DBInstanceIdentifier) + } + return "", err + } + return resume, nil +} + +// Returns the instance to where the snapshot found it, so a failed snapshot +// leaves a usable instance rather than one stuck in backing-up. A delete +// accepted while the snapshot ran owns the record by now, so the resume only +// applies to an instance still in backing-up. +func (s *Service) endSnapshotOperation(ctx context.Context, kv jetstream.KeyValue, id string, resume Status) { + endCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), defaultTimeout) + defer cancel() + err := s.updateInstance(endCtx, kv, id, func(stored *DBInstanceRecord) { + if stored.Status == StatusBackingUp { + stored.Status = resume + stored.TransitionStartedAt = nil + } + stored.SnapshotOperation = nil + }) + if err != nil { + slog.ErrorContext(ctx, "rds: returning an instance out of backing-up failed; the reconciler will finish it", + "dbInstance", id, "resume", resume, "err", err) + } +} + +// The customer-facing view. Only fields this phase actually backs are set, so an +// unset one is honestly absent rather than a fabricated default. +func (s *Service) projectDBSnapshot(rec *DBSnapshotRecord) *rds.DBSnapshot { + if rec == nil { + return nil + } + out := &rds.DBSnapshot{ + DBSnapshotIdentifier: aws.String(rec.DBSnapshotIdentifier), + DBSnapshotArn: aws.String(DBSnapshotARN(s.region, rec.AccountID, rec.DBSnapshotIdentifier)), + DBInstanceIdentifier: aws.String(rec.DBInstanceIdentifier), + SnapshotType: aws.String(rec.SnapshotType), + Status: aws.String(rec.Status), + Engine: aws.String(rec.Engine), + EngineVersion: aws.String(rec.EngineVersion), + AllocatedStorage: aws.Int64(rec.AllocatedStorage), + StorageType: aws.String(rec.StorageType), + Encrypted: aws.Bool(rec.StorageEncrypted), + MasterUsername: aws.String(rec.MasterUsername), + Port: aws.Int64(rec.Port), + SnapshotCreateTime: aws.Time(rec.CreatedAt), + // The Terraform provider reads tags from the describe as well as from + // ListTagsForResource, so the two have to agree. + TagList: tagsToAWS(rec.Tags), + } + if rec.VpcID != "" { + out.VpcId = aws.String(rec.VpcID) + } + // A snapshot still being taken has no data yet, so reporting full progress + // would have a client restore from something that does not exist. + if rec.Status == SnapshotStatusAvailable { + out.PercentProgress = aws.Int64(100) + } + return out +} + +// Reads as a sentence: the fault this feeds names what the customer has to +// remove first, and one dependent is the overwhelmingly common case. +func joinNames(names []string) string { + switch len(names) { + case 1: + return names[0] + case 2: + return names[0] + " and " + names[1] + default: + return strings.Join(names[:len(names)-1], ", ") + " and " + names[len(names)-1] + } +} diff --git a/spinifex/handlers/rds/snapshot_test.go b/spinifex/handlers/rds/snapshot_test.go new file mode 100644 index 000000000..27de8416f --- /dev/null +++ b/spinifex/handlers/rds/snapshot_test.go @@ -0,0 +1,809 @@ +package handlers_rds + +import ( + "encoding/json" + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" + "github.com/mulgadc/spinifex/spinifex/tags" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// snapshotHarness is a Service wired for both halves of this phase: the EC2 +// snapshot and volume services a create and a delete drive, and the customer +// VPC and launch fakes a restore needs on top. +type snapshotHarness struct { + svc *Service + launch *launchHarness + network *fakeNetwork + snaps *fakeSnapshots + agent *stubAgent + nc *nats.Conn +} + +func newSnapshotHarness(t *testing.T, agentFails bool) *snapshotHarness { + t.Helper() + _, nc, _ := testutil.StartTestJetStream(t) + + h := &snapshotHarness{ + launch: newLaunchHarness(), + network: newFakeNetwork(), + snaps: &fakeSnapshots{}, + nc: nc, + } + h.launch.volumes.encrypted = true + h.agent = newStubAgent(t, nc, testAccountID, testDBID, agentFails) + // Without a responder the best-effort endpoint publish would sit out its own + // timeout on every restore. + stubDNSWriter(t, nc) + + h.svc = NewService(nc, testRegion).WithDeps(Deps{ + LoadCA: newTestCA(t), + Launch: h.launch.deps(), + Network: h.network, + Snapshots: h.snaps, + }) + return h +} + +func stubDNSWriter(t *testing.T, nc *nats.Conn) { + t.Helper() + sub, err := nc.Subscribe(handlers_dns.SubjectRecordsetChange, func(msg *nats.Msg) { + if err := msg.Respond([]byte(`{}`)); err != nil { + t.Logf("respond on %s: %v", handlers_dns.SubjectRecordsetChange, err) + } + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) +} + +func (h *snapshotHarness) instance(t *testing.T, id string) DBInstanceRecord { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var rec DBInstanceRecord + found, err := getJSON(t.Context(), kv, DBInstanceKey(id), &rec) + require.NoError(t, err) + require.True(t, found, "DB instance %s should be stored", id) + return rec +} + +func (h *snapshotHarness) snapshot(t *testing.T, id string) (DBSnapshotRecord, bool) { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var rec DBSnapshotRecord + found, err := getJSON(t.Context(), kv, DBSnapshotKey(id), &rec) + require.NoError(t, err) + return rec, found +} + +func (h *snapshotHarness) events(t *testing.T, sourceType, id string) []Event { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var ring eventRing + found, err := getJSON(t.Context(), kv, EventRingKey(sourceType, id), &ring) + require.NoError(t, err) + if !found { + return nil + } + return ring.Events +} + +// seedSnapshotSource stores an available instance with a data volume, carrying +// the configuration a restore has to be able to reproduce from the snapshot +// alone. +func (h *snapshotHarness) seedSnapshotSource(t *testing.T) DBInstanceRecord { + t.Helper() + rec := availableRecord() + rec.DBInstanceClass = "db.t3.medium" + rec.AllocatedStorage = 20 + rec.StorageType = storageTypeGP3 + rec.StorageEncrypted = true + rec.VpcID = testDefaultVPC + rec.VpcSecurityGroupIDs = []string{testDefaultSG} + seedInstance(t, h.svc, rec) + return rec +} + +const testSnapshotID = "orders-db-pre-upgrade" + +func snapshotInput() *rds.CreateDBSnapshotInput { + return &rds.CreateDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + DBInstanceIdentifier: aws.String(testDBID), + } +} + +func TestCreateDBSnapshot_QuiescesTheEngineAndRecordsTheSnapshot(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + + out, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + require.NotNil(t, out.DBSnapshot) + assert.Equal(t, testSnapshotID, aws.StringValue(out.DBSnapshot.DBSnapshotIdentifier)) + assert.Equal(t, SnapshotStatusAvailable, aws.StringValue(out.DBSnapshot.Status)) + assert.Equal(t, SnapshotTypeManual, aws.StringValue(out.DBSnapshot.SnapshotType)) + assert.Equal(t, DBSnapshotARN(testRegion, testAccountID, testSnapshotID), + aws.StringValue(out.DBSnapshot.DBSnapshotArn)) + + // The engine is held at a checkpoint for the length of the EC2 call and let + // go again, so the captured datadir is a checkpoint rather than a mid-write + // state. + issued := make([]string, 0, 2) + for _, cmd := range h.agent.received() { + issued = append(issued, cmd.Type) + } + assert.Equal(t, []string{CommandQuiesce, CommandUnquiesce}, issued) + + require.Len(t, h.snaps.created, 1) + assert.Equal(t, "vol-rdsdata01", aws.StringValue(h.snaps.created[0].VolumeId)) + assert.Equal(t, tags.ManagedByRDS, tagOf(h.snaps.created[0].TagSpecifications, tags.ManagedByKey)) + assert.Equal(t, testSnapshotID, tagOf(h.snaps.created[0].TagSpecifications, rdsSnapshotTagKey)) + assert.Equal(t, testAccountID, tagOf(h.snaps.created[0].TagSpecifications, rdsSnapshotAccountTagKey)) + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, "snap-0001", stored.SnapshotID) + assert.False(t, stored.CrashConsistent) + // Everything a restore needs is copied, because the instance it describes may + // be gone by the time the snapshot is used. + assert.Equal(t, "vol-rdsdata01", stored.SourceVolumeID) + assert.Equal(t, "db.t3.medium", stored.DBInstanceClass) + assert.Equal(t, "orders", stored.DBName) + assert.Equal(t, int64(5432), stored.Port) +} + +// The hold has to be released before the instance settles, so the deadline the +// agent enforces is never what ends a successful snapshot. +func TestCreateDBSnapshot_ReturnsTheInstanceToWhereItWasFound(t *testing.T) { + for _, resume := range []Status{StatusAvailable, StatusStopped} { + t.Run(string(resume), func(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := availableRecord() + rec.Status = resume + seedInstance(t, h.svc, rec) + + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + stored := h.instance(t, testDBID) + assert.Equal(t, resume, stored.Status) + assert.Nil(t, stored.SnapshotOperation) + assert.Nil(t, stored.TransitionStartedAt) + }) + } +} + +// A stopped instance's datadir was sealed by the graceful stop, which is the +// checkpoint a quiesce would be forcing — so there is no engine to hold. +func TestCreateDBSnapshot_DoesNotQuiesceAStoppedInstance(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := availableRecord() + rec.Status = StatusStopped + seedInstance(t, h.svc, rec) + + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + assert.Empty(t, h.agent.received(), "a stopped engine has nothing to quiesce") + stored, _ := h.snapshot(t, testSnapshotID) + assert.False(t, stored.CrashConsistent) +} + +// D10: a snapshot that could not be quiesced is still a restorable backup, so it +// is taken and reported honestly rather than refused. +func TestCreateDBSnapshot_FallsBackToCrashConsistentWhenTheQuiesceFails(t *testing.T) { + h := newSnapshotHarness(t, true) + h.seedSnapshotSource(t) + + out, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + assert.Equal(t, SnapshotStatusAvailable, aws.StringValue(out.DBSnapshot.Status)) + + stored, _ := h.snapshot(t, testSnapshotID) + assert.True(t, stored.CrashConsistent) + require.Len(t, h.snaps.created, 1) + + // The customer is told, because a crash-consistent snapshot recovers rather + // than restores instantly. + assert.Contains(t, eventMessages(h.events(t, EventSourceTypeDBInstance, testDBID)), + "The database engine could not be quiesced before the snapshot; the snapshot is crash consistent and will recover from its write-ahead log when it is restored.") +} + +// The record only ever described a snapshot that now does not exist, so it goes +// with it rather than holding the identifier for a reconciler to puzzle over. +func TestCreateDBSnapshot_WithdrawsTheRecordWhenTheSnapshotFails(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + h.snaps.createErr = awserrors.Errorf(awserrors.ErrorInternalError, "the volume store refused the snapshot") + + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.Error(t, err) + + _, found := h.snapshot(t, testSnapshotID) + assert.False(t, found, "a snapshot that was never cut must not hold its identifier") + assert.Equal(t, StatusAvailable, h.instance(t, testDBID).Status) +} + +func TestCreateDBSnapshot_RejectsATakenIdentifier(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + _, err = h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotAlreadyExists) + // Checked before the instance is moved, so a duplicate leaves it where it was. + assert.Equal(t, StatusAvailable, h.instance(t, testDBID).Status) +} + +// One snapshot per instance at a time: the CAS into backing-up is the guard, so +// an instance already there is refused rather than entering the same window. +func TestCreateDBSnapshot_RefusesAnInstanceAlreadyBackingUp(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := availableRecord() + rec.Status = StatusBackingUp + rec.SnapshotOperation = &SnapshotOperation{ + DBSnapshotIdentifier: "orders-db-nightly", + ResumeStatus: StatusAvailable, + StartedAt: time.Now().UTC(), + } + seedInstance(t, h.svc, rec) + + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceInvalidState) + assert.Empty(t, h.snaps.created) +} + +func TestCreateDBSnapshot_RejectsAnUnusableIdentifier(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + + // Rejected because a name that cannot also be a FinalDBSnapshotIdentifier at + // delete would make the two paths disagree. + for _, id := range []string{"", "9-leading-digit", "trailing-", "double--hyphen", "UPPER"} { + input := snapshotInput() + input.DBSnapshotIdentifier = aws.String(id) + _, err := h.svc.CreateDBSnapshot(t.Context(), input, testAccountID) + require.Error(t, err, "identifier %q", id) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue, "identifier %q", id) + } +} + +func TestDescribeDBSnapshots_FiltersByInstanceAndType(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + // A snapshot of another instance, which the instance filter must exclude. It + // is stopped so the snapshot needs no quiesce: only testDBID has an agent. + other := availableRecord() + other.DBInstanceIdentifier = "reports-db" + other.Status = StatusStopped + seedInstance(t, h.svc, other) + second := snapshotInput() + second.DBSnapshotIdentifier = aws.String("reports-db-daily") + second.DBInstanceIdentifier = aws.String("reports-db") + _, err = h.svc.CreateDBSnapshot(t.Context(), second, testAccountID) + require.NoError(t, err) + + all, err := h.svc.DescribeDBSnapshots(t.Context(), &rds.DescribeDBSnapshotsInput{}, testAccountID) + require.NoError(t, err) + assert.Len(t, all.DBSnapshots, 2) + + byInstance, err := h.svc.DescribeDBSnapshots(t.Context(), &rds.DescribeDBSnapshotsInput{ + DBInstanceIdentifier: aws.String(testDBID), + }, testAccountID) + require.NoError(t, err) + require.Len(t, byInstance.DBSnapshots, 1) + assert.Equal(t, testSnapshotID, aws.StringValue(byInstance.DBSnapshots[0].DBSnapshotIdentifier)) + + byType, err := h.svc.DescribeDBSnapshots(t.Context(), &rds.DescribeDBSnapshotsInput{ + SnapshotType: aws.String(SnapshotTypeAutomated), + }, testAccountID) + require.NoError(t, err) + assert.Empty(t, byType.DBSnapshots, "nothing in this phase is automated") +} + +// A client polling a create would otherwise read "gone" for "not ready". +func TestDescribeDBSnapshots_NamedSnapshotThatDoesNotExistIsAnError(t *testing.T) { + h := newSnapshotHarness(t, false) + + _, err := h.svc.DescribeDBSnapshots(t.Context(), &rds.DescribeDBSnapshotsInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotNotFound) +} + +// D19: a filter this phase cannot honour is rejected, because a silently +// unfiltered list reads as a complete answer. +func TestDescribeDBSnapshots_RejectsUnhonouredScoping(t *testing.T) { + h := newSnapshotHarness(t, false) + + cases := map[string]*rds.DescribeDBSnapshotsInput{ + "Filters": {Filters: []*rds.Filter{{Name: aws.String("engine")}}}, + "DbiResourceId": {DbiResourceId: aws.String("db-ABC123")}, + "IncludeShared": {IncludeShared: aws.Bool(true)}, + "SnapshotType": {SnapshotType: aws.String("shared")}, + } + for name, input := range cases { + t.Run(name, func(t *testing.T) { + _, err := h.svc.DescribeDBSnapshots(t.Context(), input, testAccountID) + require.Error(t, err) + }) + } +} + +func TestDeleteDBSnapshot_RemovesTheSnapshotAndItsRecord(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + out, err := h.svc.DeleteDBSnapshot(t.Context(), &rds.DeleteDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.NoError(t, err) + + // AWS answers with the snapshot as it last stood, the way a DB instance + // delete answers with the record it just removed. + require.NotNil(t, out.DBSnapshot) + assert.Equal(t, testSnapshotID, aws.StringValue(out.DBSnapshot.DBSnapshotIdentifier)) + + assert.Equal(t, []string{"snap-0001"}, h.snaps.deleted) + _, found := h.snapshot(t, testSnapshotID) + assert.False(t, found) + // The instance still owns its data volume, so nothing releases it here. + assert.Empty(t, h.launch.volumes.deleted) +} + +// A COW snapshot references its source volume's chunks, so a deleted instance's +// volume is retained until the last snapshot holding it goes. +func TestDeleteDBSnapshot_ReleasesARetainedVolumeWhenItWasTheLastHolder(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + retained := RetainedVolumeRecord{ + VolumeID: "vol-rdsdata01", + DBInstanceIdentifier: testDBID, + Snapshots: []string{"snap-0001"}, + RetainedAt: time.Now().UTC(), + } + require.NoError(t, putJSON(t.Context(), kv, RetainedVolumeKey("vol-rdsdata01"), &retained)) + + _, err = h.svc.DeleteDBSnapshot(t.Context(), &rds.DeleteDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.NoError(t, err) + + assert.Equal(t, []string{"vol-rdsdata01"}, h.launch.volumes.deleted) + var stillRetained RetainedVolumeRecord + found, err := getJSON(t.Context(), kv, RetainedVolumeKey("vol-rdsdata01"), &stillRetained) + require.NoError(t, err) + assert.False(t, found, "the retained-volume record goes with the volume") +} + +// The volume store's own index is the authority, not the list the retained +// record was written with: another snapshot taken since would otherwise read as +// "nothing holds it". +func TestDeleteDBSnapshot_KeepsARetainedVolumeAnotherSnapshotStillHolds(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + retained := RetainedVolumeRecord{ + VolumeID: "vol-rdsdata01", + Snapshots: []string{"snap-0001"}, + HoldersUnresolved: true, + RetainedAt: time.Now().UTC(), + } + require.NoError(t, putJSON(t.Context(), kv, RetainedVolumeKey("vol-rdsdata01"), &retained)) + // A holder the record does not name, taken after it was written. + h.snaps.holding = append(h.snaps.holding, "snap-9999") + + _, err = h.svc.DeleteDBSnapshot(t.Context(), &rds.DeleteDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.NoError(t, err) + + assert.Empty(t, h.launch.volumes.deleted, "a volume another snapshot reads through must not go") + var stillRetained RetainedVolumeRecord + found, err := getJSON(t.Context(), kv, RetainedVolumeKey("vol-rdsdata01"), &stillRetained) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, []string{"snap-9999"}, stillRetained.Snapshots) + assert.False(t, stillRetained.HoldersUnresolved, "the holders are resolved now") +} + +// The raw EC2 code says nothing a customer can act on, so the fault names the +// restored instance they have to remove first. +func TestDeleteDBSnapshot_NamesTheRestoredInstanceStillReadingFromIt(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + _, err := h.svc.CreateDBSnapshot(t.Context(), snapshotInput(), testAccountID) + require.NoError(t, err) + + restored := availableRecord() + restored.DBInstanceIdentifier = "orders-db-restored" + restored.RestoredFromDBSnapshot = testSnapshotID + seedInstance(t, h.svc, restored) + h.snaps.deleteErr = awserrors.Errorf(awserrors.ErrorInvalidSnapshotInUse, "snap-0001 is in use") + + _, err = h.svc.DeleteDBSnapshot(t.Context(), &rds.DeleteDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotInvalidState) + assert.Contains(t, err.Error(), "orders-db-restored") + + _, found := h.snapshot(t, testSnapshotID) + assert.True(t, found, "a refused delete leaves the snapshot addressable") +} + +// A snapshot still being taken has no data to remove and an in-flight writer +// that would recreate the record. +func TestDeleteDBSnapshot_RefusesASnapshotStillBeingTaken(t *testing.T) { + h := newSnapshotHarness(t, false) + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + creating := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC(), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &creating)) + + _, err = h.svc.DeleteDBSnapshot(t.Context(), &rds.DeleteDBSnapshotInput{ + DBSnapshotIdentifier: aws.String(testSnapshotID), + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotInvalidState) +} + +// A snapshot still being taken has no PercentProgress, because reporting full +// progress would have a client restore from something that does not exist. +func TestProjectDBSnapshot_ReportsProgressOnlyWhenTheDataExists(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + } + assert.Nil(t, h.svc.projectDBSnapshot(&rec).PercentProgress) + + rec.Status = SnapshotStatusAvailable + assert.Equal(t, int64(100), aws.Int64Value(h.svc.projectDBSnapshot(&rec).PercentProgress)) +} + +// The EC2 snapshot is tagged with the DB snapshot identifier before the record +// is flipped, so its presence is what says the data exists. +func TestReconciler_AdoptsTheEC2SnapshotOfAnUnfinishedDBSnapshot(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + // The snapshot the dead worker cut, carrying the identifier's tag. + h.snaps.holding = append(h.snaps.holding, "snap-orphan") + h.snaps.tagged = map[string]map[string]string{"snap-orphan": { + rdsSnapshotTagKey: testSnapshotID, + rdsSnapshotAccountTagKey: testAccountID, + }} + + require.NoError(t, rec.reconcileOnce(t.Context())) + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusAvailable, stored.Status) + assert.Equal(t, "snap-orphan", stored.SnapshotID) + // The worker died before it could report whether the quiesce held. + assert.True(t, stored.CrashConsistent) +} + +// A record left in creating would hold its name forever while naming nothing a +// customer can restore. +func TestReconciler_DoesNotAdoptAnotherAccountsSnapshot(t *testing.T) { + h := newSnapshotHarness(t, false) + reconciler := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + h.snaps.holding = []string{"snap-other-account", "snap-this-account"} + h.snaps.tagged = map[string]map[string]string{ + "snap-other-account": { + rdsSnapshotTagKey: testSnapshotID, + rdsSnapshotAccountTagKey: "444455556666", + }, + "snap-this-account": { + rdsSnapshotTagKey: testSnapshotID, + rdsSnapshotAccountTagKey: testAccountID, + }, + } + + require.NoError(t, reconciler.reconcileOnce(t.Context())) + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusAvailable, stored.Status) + assert.Equal(t, "snap-this-account", stored.SnapshotID) +} + +func TestReconciler_KeepsCreatingSnapshotWhenEC2LookupIsIncomplete(t *testing.T) { + h := newSnapshotHarness(t, false) + h.snaps.describeErr = errors.New("snapshot metadata temporarily unavailable") + reconciler := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + + err = reconciler.reconcileOnce(t.Context()) + require.Error(t, err) + assert.Contains(t, err.Error(), "metadata temporarily unavailable") + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusCreating, stored.Status) +} + +func TestReconciler_DoesNotDeleteAConcurrentlyCompletedSnapshot(t *testing.T) { + h := newSnapshotHarness(t, false) + reconciler := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + h.snaps.afterDescribe = func() { + h.snaps.afterDescribe = nil + var current DBSnapshotRecord + rev, found, getErr := getJSONRevision(t.Context(), kv, DBSnapshotKey(testSnapshotID), ¤t) + require.NoError(t, getErr) + require.True(t, found) + current.Status = SnapshotStatusAvailable + current.SnapshotID = "snap-completed-concurrently" + require.NoError(t, updateJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), rev, ¤t)) + } + + require.NoError(t, reconciler.reconcileOnce(t.Context())) + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusAvailable, stored.Status) + assert.Equal(t, "snap-completed-concurrently", stored.SnapshotID) +} + +func TestReconciler_RejectsAmbiguousEC2Snapshots(t *testing.T) { + h := newSnapshotHarness(t, false) + reconciler := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + matchingTags := map[string]string{ + rdsSnapshotTagKey: testSnapshotID, + rdsSnapshotAccountTagKey: testAccountID, + } + h.snaps.holding = []string{"snap-duplicate-a", "snap-duplicate-b"} + h.snaps.tagged = map[string]map[string]string{ + "snap-duplicate-a": matchingTags, + "snap-duplicate-b": matchingTags, + } + + err = reconciler.reconcileOnce(t.Context()) + require.Error(t, err) + assert.Contains(t, err.Error(), "multiple EC2 snapshots") + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusCreating, stored.Status) +} + +// A completed authoritative lookup is the only evidence that no EC2 snapshot +// was cut. Once it succeeds with no match, the stale name can be released. +func TestReconciler_WithdrawsADBSnapshotWhoseDataWasNeverCut(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + stale := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC().Add(-2 * snapshotResolveTimeout), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &stale)) + + require.NoError(t, rec.reconcileOnce(t.Context())) + + _, found := h.snapshot(t, testSnapshotID) + assert.False(t, found) + assert.Contains(t, eventMessages(h.events(t, EventSourceTypeDBSnapshot, testSnapshotID)), + "The DB snapshot could not be completed and has been removed.") +} + +// Inside the window the record still belongs to a live worker, so touching it +// would race the write that is about to land. +func TestReconciler_LeavesAFreshCreatingSnapshotAlone(t *testing.T) { + h := newSnapshotHarness(t, false) + rec := NewReconciler(h.svc, "node-a") + + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + fresh := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + AccountID: testAccountID, + Status: SnapshotStatusCreating, + CreatedAt: time.Now().UTC(), + } + require.NoError(t, putJSON(t.Context(), kv, DBSnapshotKey(testSnapshotID), &fresh)) + + require.NoError(t, rec.reconcileOnce(t.Context())) + + stored, found := h.snapshot(t, testSnapshotID) + require.True(t, found) + assert.Equal(t, SnapshotStatusCreating, stored.Status) +} + +// A snapshot holds its instance in backing-up for the length of the snapshot and +// no longer, so one still there past the bound belongs to a worker that died. +func TestReconciler_ReturnsAnInstanceStuckInBackingUp(t *testing.T) { + for _, resume := range []Status{StatusAvailable, StatusStopped} { + t.Run(string(resume), func(t *testing.T) { + h := newSnapshotHarness(t, false) + reconciler := NewReconciler(h.svc, "node-a") + + started := time.Now().UTC().Add(-2 * transitionTimeout) + rec := availableRecord() + rec.Status = StatusBackingUp + rec.TransitionStartedAt = &started + rec.SnapshotOperation = &SnapshotOperation{ + DBSnapshotIdentifier: testSnapshotID, + ResumeStatus: resume, + StartedAt: started, + } + seedInstance(t, h.svc, rec) + + require.NoError(t, reconciler.reconcileOnce(t.Context())) + + stored := h.instance(t, testDBID) + assert.Equal(t, resume, stored.Status) + assert.Nil(t, stored.SnapshotOperation) + }) + } +} + +// Inside the bound the snapshot is still running, and cutting it short would +// leave the engine quiesced with nobody to release it. +func TestReconciler_LeavesAnInFlightSnapshotAlone(t *testing.T) { + h := newSnapshotHarness(t, false) + reconciler := NewReconciler(h.svc, "node-a") + + started := time.Now().UTC() + rec := availableRecord() + rec.Status = StatusBackingUp + rec.TransitionStartedAt = &started + rec.SnapshotOperation = &SnapshotOperation{ + DBSnapshotIdentifier: testSnapshotID, + ResumeStatus: StatusAvailable, + StartedAt: started, + } + seedInstance(t, h.svc, rec) + + require.NoError(t, reconciler.reconcileOnce(t.Context())) + + stored := h.instance(t, testDBID) + assert.Equal(t, StatusBackingUp, stored.Status) + assert.NotNil(t, stored.SnapshotOperation) +} + +// The Terraform provider reads tags from the describe as well as from +// ListTagsForResource, so the two have to agree. +func TestDBSnapshotTags_ReadBackThroughBothPaths(t *testing.T) { + h := newSnapshotHarness(t, false) + h.seedSnapshotSource(t) + + input := snapshotInput() + input.Tags = []*rds.Tag{{Key: aws.String("env"), Value: aws.String("prod")}} + out, err := h.svc.CreateDBSnapshot(t.Context(), input, testAccountID) + require.NoError(t, err) + require.Len(t, out.DBSnapshot.TagList, 1) + + listed, err := h.svc.ListTagsForResource(t.Context(), &rds.ListTagsForResourceInput{ + ResourceName: aws.String(DBSnapshotARN(testRegion, testAccountID, testSnapshotID)), + }, testAccountID) + require.NoError(t, err) + require.Len(t, listed.TagList, 1) + assert.Equal(t, "prod", aws.StringValue(listed.TagList[0].Value)) + + _, err = h.svc.AddTagsToResource(t.Context(), &rds.AddTagsToResourceInput{ + ResourceName: aws.String(DBSnapshotARN(testRegion, testAccountID, testSnapshotID)), + Tags: []*rds.Tag{{Key: aws.String("team"), Value: aws.String("payments")}}, + }, testAccountID) + require.NoError(t, err) + + stored, _ := h.snapshot(t, testSnapshotID) + assert.Equal(t, map[string]string{"env": "prod", "team": "payments"}, stored.Tags) +} + +// The record has to survive a round trip through KV, because that is the only +// copy a restore reads once the instance it came from is gone. +func TestDBSnapshotRecord_SurvivesARoundTrip(t *testing.T) { + updated := time.Now().UTC().Truncate(time.Second) + rec := DBSnapshotRecord{ + DBSnapshotIdentifier: testSnapshotID, + DBInstanceIdentifier: testDBID, + AccountID: testAccountID, + SnapshotType: SnapshotTypeManual, + Status: SnapshotStatusAvailable, + SnapshotID: "snap-0001", + SourceVolumeID: "vol-rdsdata01", + DBInstanceClass: "db.t3.medium", + VpcSecurityGroupIDs: []string{"sg-app01"}, + DBSubnetGroupName: "prod-db", + DBParameterGroupName: "custom-pg18", + MasterPasswordUpdatedAt: &updated, + } + payload, err := json.Marshal(&rec) + require.NoError(t, err) + + var back DBSnapshotRecord + require.NoError(t, json.Unmarshal(payload, &back)) + assert.Equal(t, rec, back) +} diff --git a/spinifex/handlers/rds/status.go b/spinifex/handlers/rds/status.go index fbe6b96bc..9c60dd94e 100644 --- a/spinifex/handlers/rds/status.go +++ b/spinifex/handlers/rds/status.go @@ -30,13 +30,15 @@ const ( // Every non-terminal state can reach failed and deleting, since a delete is // accepted at any point and any step can fail. var transitions = map[Status][]Status{ - StatusCreating: {StatusAvailable, StatusFailed, StatusDeleting}, - StatusAvailable: {StatusModifying, StatusBackingUp, StatusRebooting, StatusStopping, StatusRecovering, StatusFailed, StatusDeleting}, - StatusModifying: {StatusAvailable, StatusFailed, StatusDeleting}, - StatusBackingUp: {StatusAvailable, StatusFailed, StatusDeleting}, + StatusCreating: {StatusAvailable, StatusFailed, StatusDeleting}, + StatusAvailable: {StatusModifying, StatusBackingUp, StatusRebooting, StatusStopping, StatusRecovering, StatusFailed, StatusDeleting}, + StatusModifying: {StatusAvailable, StatusFailed, StatusDeleting}, + // A snapshot of a stopped instance needs no quiesce and leaves it stopped, so + // backing-up has to be reachable from and returnable to both settled states. + StatusBackingUp: {StatusAvailable, StatusStopped, StatusFailed, StatusDeleting}, StatusRebooting: {StatusAvailable, StatusFailed, StatusDeleting}, StatusStopping: {StatusStopped, StatusFailed, StatusDeleting}, - StatusStopped: {StatusStarting, StatusModifying, StatusFailed, StatusDeleting}, + StatusStopped: {StatusStarting, StatusModifying, StatusBackingUp, StatusFailed, StatusDeleting}, StatusStarting: {StatusAvailable, StatusFailed, StatusDeleting}, StatusRecovering: {StatusAvailable, StatusFailed, StatusDeleting}, // A failed instance is retried by the reconciler rather than being stuck: @@ -86,3 +88,28 @@ func CanTransition(from, to Status) bool { } return slices.Contains(transitions[from], to) } + +// The snapshot's own lifecycle, separate from the instance's backing-up state +// (D4): the instance is derived from an in-flight control-plane operation, while +// the snapshot record moves creating → available once the data exists. There is +// no failed state — a snapshot that never completed is removed rather than kept +// as an unrestorable record. +var snapshotTransitions = map[string][]string{ + SnapshotStatusCreating: {SnapshotStatusAvailable}, + SnapshotStatusAvailable: nil, +} + +func ValidSnapshotStatus(status string) bool { + _, ok := snapshotTransitions[status] + return ok +} + +func CanTransitionSnapshot(from, to string) bool { + if !ValidSnapshotStatus(from) || !ValidSnapshotStatus(to) { + return false + } + if from == to { + return true + } + return slices.Contains(snapshotTransitions[from], to) +} diff --git a/spinifex/handlers/rds/status_test.go b/spinifex/handlers/rds/status_test.go index d1079c149..b8490bfa3 100644 --- a/spinifex/handlers/rds/status_test.go +++ b/spinifex/handlers/rds/status_test.go @@ -69,8 +69,9 @@ func TestCanTransition_Lifecycle(t *testing.T) { {StatusAvailable, StatusAvailable, true}, {StatusDeleted, StatusDeleted, true}, - // A stopped instance has no VM, so it cannot be backing up or rebooting. - {StatusStopped, StatusBackingUp, false}, + // A stopped instance has no VM to reboot, but its datadir was sealed by the + // stop, which is exactly what a snapshot wants to read. + {StatusStopped, StatusBackingUp, true}, {StatusStopped, StatusRebooting, false}, {StatusStopped, StatusAvailable, false}, diff --git a/spinifex/handlers/rds/store.go b/spinifex/handlers/rds/store.go index f40f6f4fc..2aa783f9e 100644 --- a/spinifex/handlers/rds/store.go +++ b/spinifex/handlers/rds/store.go @@ -231,6 +231,12 @@ func ListDBInstanceIDs(ctx context.Context, kv jetstream.KeyValue) ([]string, er return listNames(ctx, kv, DBInstancesPrefix()) } +// Manual and automated snapshots alike: the type is on the record, so a listing +// filtered by it still has to read every one. +func ListDBSnapshotIDs(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { + return listNames(ctx, kv, DBSnapshotsPrefix()) +} + func ListDBSubnetGroupNames(ctx context.Context, kv jetstream.KeyValue) ([]string, error) { return listNames(ctx, kv, DBSubnetGroupsPrefix()) } diff --git a/spinifex/handlers/rds/subjects.go b/spinifex/handlers/rds/subjects.go index 0307199cb..15355e5e9 100644 --- a/spinifex/handlers/rds/subjects.go +++ b/spinifex/handlers/rds/subjects.go @@ -32,6 +32,13 @@ const ( SubjectDescribeEvents = "rds.DescribeEvents" + // Backup and recovery. A create quiesces the engine and a restore launches a + // VM, so both are as much orchestration as the lifecycle actions above. + SubjectCreateDBSnapshot = "rds.CreateDBSnapshot" + SubjectDescribeDBSnapshots = "rds.DescribeDBSnapshots" + SubjectDeleteDBSnapshot = "rds.DeleteDBSnapshot" + SubjectRestoreDBInstanceFromDBSnapshot = "rds.RestoreDBInstanceFromDBSnapshot" + // 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" diff --git a/spinifex/handlers/rds/tags.go b/spinifex/handlers/rds/tags.go index 9b515aa25..332b6295a 100644 --- a/spinifex/handlers/rds/tags.go +++ b/spinifex/handlers/rds/tags.go @@ -45,15 +45,20 @@ type taggableResource struct { } // 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. +// resource. 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, }, + ResourceKindDBSnapshot: { + key: DBSnapshotKey, + newRecord: func() TaggedRecord { return &DBSnapshotRecord{} }, + notFound: awserrors.ErrorDBSnapshotNotFound, + }, ResourceKindDBSubnetGroup: { key: DBSubnetGroupKey, newRecord: func() TaggedRecord { return &DBSubnetGroupRecord{} }, diff --git a/spinifex/handlers/rds/tags_test.go b/spinifex/handlers/rds/tags_test.go index cdad7c92c..dd86b3f5f 100644 --- a/spinifex/handlers/rds/tags_test.go +++ b/spinifex/handlers/rds/tags_test.go @@ -219,17 +219,16 @@ func TestTagActions_MissingResourceIsTheResourcesOwnFault(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceNotFound) } -// Snapshots are a valid ARN kind that no phase has built yet. Answering with an -// empty list would be indistinguishable from an untagged snapshot and would let -// an apply appear to tag something that does not exist. -func TestTagActions_UnregisteredResourceTypeIsRejected(t *testing.T) { +// Answering with an empty list would be indistinguishable from an untagged +// snapshot and would let an apply appear to tag something that does not exist. +func TestTagActions_MissingSnapshotIsRejected(t *testing.T) { h := newCreateHarness(t, "") _, err := h.svc.ListTagsForResource(t.Context(), &rds.ListTagsForResourceInput{ ResourceName: aws.String(DBSnapshotARN(testRegion, testAccountID, "orders-db-snap")), }, testAccountID) require.Error(t, err) - assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Contains(t, err.Error(), awserrors.ErrorDBSnapshotNotFound) } func TestTagActions_ForeignAccountARNIsRejected(t *testing.T) {