diff --git a/spinifex/handlers/rds/backup_test.go b/spinifex/handlers/rds/backup_test.go index f3c0426ea..9959a5ddf 100644 --- a/spinifex/handlers/rds/backup_test.go +++ b/spinifex/handlers/rds/backup_test.go @@ -58,10 +58,15 @@ func instanceRevision(t *testing.T, svc *Service, id string) (*DBInstanceRecord, // Runs one backup pass the way the reconciler does: re-read at its revision, // then fire. func (h *snapshotHarness) runBackupPass(t *testing.T) bool { + t.Helper() + return h.runBackupPassFor(t, testDBID) +} + +func (h *snapshotHarness) runBackupPassFor(t *testing.T, id string) bool { t.Helper() kv, err := h.svc.bucket(t.Context(), testAccountID) require.NoError(t, err) - rec, rev := instanceRevision(t, h.svc, testDBID) + rec, rev := instanceRevision(t, h.svc, id) fired, err := h.svc.runBackupWindow(t.Context(), kv, rev, testAccountID, rec) require.NoError(t, err) return fired diff --git a/spinifex/handlers/rds/restore.go b/spinifex/handlers/rds/restore.go index 4b3e77fac..04b4bd7d8 100644 --- a/spinifex/handlers/rds/restore.go +++ b/spinifex/handlers/rds/restore.go @@ -51,7 +51,7 @@ func (s *Service) RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rd snapshot.DBSnapshotIdentifier, snapshot.Status, SnapshotStatusAvailable) } - req, err := resolveRestoreRequest(input, snapshot) + req, err := s.resolveRestoreRequest(input, snapshot) if err != nil { return nil, err } @@ -161,10 +161,9 @@ func (s *Service) RestoreDBInstanceFromDBSnapshot(ctx context.Context, input *rd 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) { +// Request-overridable configuration comes from the snapshot when omitted. +// Platform-owned settings use the defaults in force when the restore runs. +func (s *Service) 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) @@ -244,6 +243,7 @@ func resolveRestoreRequest(input *rds.RestoreDBInstanceFromDBSnapshotInput, snap DeletionProtection: aws.BoolValue(input.DeletionProtection), AutoMinorVersionUpgrade: input.AutoMinorVersionUpgrade == nil || aws.BoolValue(input.AutoMinorVersionUpgrade), + BackupRetentionPeriod: s.defaultRetentionDays(), Tags: tagMap, }, nil diff --git a/spinifex/handlers/rds/restore_test.go b/spinifex/handlers/rds/restore_test.go index d178b2b99..dfbed6a19 100644 --- a/spinifex/handlers/rds/restore_test.go +++ b/spinifex/handlers/rds/restore_test.go @@ -3,6 +3,7 @@ package handlers_rds import ( "errors" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" @@ -87,8 +88,8 @@ func TestRestoreDBInstanceFromDBSnapshot_IAMFailurePrecedesReservationAndVolume( assert.Nil(t, h.launch.launcher.input) } -// 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. +// Source-instance configuration comes from the snapshot when the restore +// request does not override it; platform-owned settings use current defaults. func TestRestoreDBInstanceFromDBSnapshot_FallsBackToTheSnapshotsConfiguration(t *testing.T) { h := newSnapshotHarness(t, false) h.seedSnapshot(t) @@ -107,6 +108,49 @@ func TestRestoreDBInstanceFromDBSnapshot_FallsBackToTheSnapshotsConfiguration(t assert.Equal(t, []string{testDefaultSG}, stored.VpcSecurityGroupIDs) } +func TestRestoreDBInstanceFromDBSnapshot_DefaultsRetentionAndTakesAnAutomatedBackup(t *testing.T) { + h := newSnapshotHarness(t, false) + now := time.Now().UTC() + h.svc.deps.Backup = BackupPolicy{ + RetentionDays: 3, + BackupWindowBlock: openBackupWindow(now), + } + h.seedSnapshot(t) + + _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) + require.NoError(t, err) + + stored := h.instance(t, testRestoredID) + assert.Equal(t, int64(3), h.svc.defaultRetentionDays()) + assert.Equal(t, int64(3), stored.BackupRetentionPeriod) + assert.Empty(t, stored.PreferredBackupWindow) + assert.Empty(t, stored.PreferredMaintenanceWindow) + window, err := h.svc.resolvedBackupWindow(&stored) + require.NoError(t, err) + require.True(t, window.contains(now), "the lazily assigned window should be open") + + newStubAgent(t, h.nc, testAccountID, testRestoredID, false) + stored.Status = StatusAvailable + seedInstance(t, h.svc, stored) + require.True(t, h.runBackupPassFor(t, testRestoredID)) + + stamps := h.automatedStamps(t, testRestoredID) + require.Len(t, stamps, 1) + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var entry AutomatedBackupRecord + found, err := getJSON(t.Context(), kv, AutomatedBackupKey(testRestoredID, stamps[0]), &entry) + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, testRestoredID, entry.DBInstanceIdentifier) + backup, found := h.snapshot(t, entry.DBSnapshotIdentifier) + require.True(t, found) + assert.Equal(t, testRestoredID, backup.DBInstanceIdentifier) + assert.Equal(t, SnapshotTypeAutomated, backup.SnapshotType) + assert.Equal(t, SnapshotStatusAvailable, backup.Status) + assert.NotNil(t, h.instance(t, testRestoredID).LastAutomatedBackupAt) +} + func TestRestoreDBInstanceFromDBSnapshot_HonoursTheRequestedOverrides(t *testing.T) { h := newSnapshotHarness(t, false) h.seedSnapshot(t) diff --git a/spinifex/handlers/rds/validate.go b/spinifex/handlers/rds/validate.go index 1a7ceee17..63462dc58 100644 --- a/spinifex/handlers/rds/validate.go +++ b/spinifex/handlers/rds/validate.go @@ -53,9 +53,8 @@ type validatedCreate struct { // to true and so does the Terraform provider, so an unset request must not // read back as false. AutoMinorVersionUpgrade bool - // Automated backups as they will be in force: the retention defaulted when the - // request names none, and both windows in canonical text — assigned from the - // instance identifier when unnamed, never left empty. + // Automated backup settings in force. Create assigns unnamed windows, while + // restore leaves them empty for lazy deterministic assignment. BackupRetentionPeriod int64 PreferredBackupWindow string PreferredMaintenanceWindow string