diff --git a/spinifex/accountteardown/reapers_ec2_account_settings_current_behaviour_test.go b/spinifex/accountteardown/reapers_ec2_account_settings_current_behaviour_test.go new file mode 100644 index 000000000..367712832 --- /dev/null +++ b/spinifex/accountteardown/reapers_ec2_account_settings_current_behaviour_test.go @@ -0,0 +1,50 @@ +package accountteardown + +//test:in-package — drives the real final-stage reapers against the real +// EC2 account-settings service to pin that teardown leaves the bucket alone. + +import ( + "testing" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/internal/testkit" + ec2account "github.com/mulgadc/spinifex/spinifex/domains/ec2/account" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// No reaper in this package references the EC2 account-settings bucket or +// calls a settings delete. Running the real engine and the real StageAccount +// reapers against an account with settings enabled shows the consequence: +// the settings record outlives the account record it belongs to. +func TestCurrentBehaviour_AccountTermination_EC2AccountSettingsSurviveTeardown(t *testing.T) { + _, nc, js := testutil.StartTestJetStream(t) + ctx := testCtx(t) + + settings, err := ec2account.NewAccountSettingsServiceImplWithNATS(ctx, nil, nc) + require.NoError(t, err) + _, err = settings.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + _, err = settings.EnableSerialConsoleAccess(ctx, &ec2.EnableSerialConsoleAccessInput{}, testAccountID) + require.NoError(t, err) + + accounts := newFakeAccounts() + iam := &accountIAM{account: &handlers_iam.Account{AccountID: testAccountID, AccountName: testAccountName}} + engine := quietEngine(accounts, AccountReapers(iam, nil, nil)...) + + result, err := engine.Teardown(ctx, request()) + require.NoError(t, err) + assert.True(t, result.AccountDeleted) + assert.True(t, accounts.deleted) + assert.Zero(t, result.StuckCount()) + + bucket, err := js.KeyValue(ctx, ec2account.KVBucketAccountSettings) + require.NoError(t, err) + _, err = bucket.Get(ctx, testAccountID) + require.NoError(t, err, "the settings key must still be present after the account is torn down") + + out, err := settings.GetEbsEncryptionByDefault(ctx, &ec2.GetEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + assert.True(t, *out.EbsEncryptionByDefault) +} diff --git a/spinifex/domains/ec2/account/cleanup.go b/spinifex/domains/ec2/account/cleanup.go new file mode 100644 index 000000000..183c6086b --- /dev/null +++ b/spinifex/domains/ec2/account/cleanup.go @@ -0,0 +1,90 @@ +package account + +import ( + "context" + "errors" + "fmt" + + awserrors "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + awsidentifiers "github.com/mulgadc/spinifex/spinifex/foundation/aws/identifiers" + "github.com/nats-io/nats.go/jetstream" +) + +// AccountSettingsCleanupDisposition is CleanupAccount's outcome for an +// account's settings key. +type AccountSettingsCleanupDisposition string + +const ( + AccountSettingsCleanupDeleted AccountSettingsCleanupDisposition = "deleted" + AccountSettingsCleanupAbsent AccountSettingsCleanupDisposition = "absent" + AccountSettingsCleanupFailed AccountSettingsCleanupDisposition = "failed" +) + +// evidenceRevisionGuardedDelete and evidenceObservedAbsent are the fixed +// evidence bases CleanupAccount reports; see kvstore/bucket.go:112-120 for why +// a plain read cannot support a stronger absence claim. +const ( + evidenceRevisionGuardedDelete = "revision-guarded-delete" + evidenceObservedAbsent = "observed-absent-at-scan" +) + +// cleanupMaxAttempts bounds retries against a revision that keeps moving +// under concurrent writes, so cleanup fails closed instead of spinning. +const cleanupMaxAttempts = 5 + +// AccountSettingsCleanup is CleanupAccount's result: what happened to an +// account's settings key, and the fixed evidence basis for that claim. +type AccountSettingsCleanup struct { + Disposition AccountSettingsCleanupDisposition + Evidence string + Revision uint64 +} + +// CleanupAccount removes accountID's settings key so a reused account ID does +// not inherit a deleted account's EBS-encryption-by-default or +// serial-console-access settings. It is unwired: nothing calls it yet. +func (s *AccountSettingsServiceImpl) CleanupAccount(ctx context.Context, accountID string) (*AccountSettingsCleanup, error) { + if accountID == "" { + return nil, awserrors.Errorf(awserrors.ErrorMissingParameter, "accountID is required for settings cleanup") + } + if !awsidentifiers.IsAccountID(accountID) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "accountID %q is not a valid AWS account ID", accountID) + } + if accountID == awsidentifiers.GlobalAccountID { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "settings cleanup must not target the global account") + } + + // settingsKey is never used here: cleanup must act on exactly accountID + // and never fall back to the global key. + for range cleanupMaxAttempts { + entry, err := s.settingsKV.Get(ctx, accountID) + if err != nil { + if errors.Is(err, jetstream.ErrKeyNotFound) { + return &AccountSettingsCleanup{Disposition: AccountSettingsCleanupAbsent, Evidence: evidenceObservedAbsent}, nil + } + return &AccountSettingsCleanup{Disposition: AccountSettingsCleanupFailed}, fmt.Errorf("get account settings for cleanup: %w", err) + } + rev := entry.Revision() + + err = s.settingsKV.Delete(ctx, accountID, jetstream.LastRevision(rev)) + switch { + case err == nil: + return &AccountSettingsCleanup{ + Disposition: AccountSettingsCleanupDeleted, + Evidence: evidenceRevisionGuardedDelete, + Revision: rev, + }, nil + case errors.Is(err, jetstream.ErrKeyNotFound): + return &AccountSettingsCleanup{Disposition: AccountSettingsCleanupAbsent, Evidence: evidenceObservedAbsent}, nil + case errors.Is(err, jetstream.ErrKeyRevisionMismatch): + // A concurrent write landed between the read and the guarded + // delete; retry against whatever revision now holds. + continue + default: + return &AccountSettingsCleanup{Disposition: AccountSettingsCleanupFailed}, fmt.Errorf("delete account settings for cleanup: %w", err) + } + } + + return &AccountSettingsCleanup{Disposition: AccountSettingsCleanupFailed}, + fmt.Errorf("settings cleanup for account: exhausted %d attempts against concurrent writes", cleanupMaxAttempts) +} diff --git a/spinifex/domains/ec2/account/cleanup_test.go b/spinifex/domains/ec2/account/cleanup_test.go new file mode 100644 index 000000000..f81ddc144 --- /dev/null +++ b/spinifex/domains/ec2/account/cleanup_test.go @@ -0,0 +1,181 @@ +package account + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go/service/ec2" + awsidentifiers "github.com/mulgadc/spinifex/spinifex/foundation/aws/identifiers" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// cleanupFakeKV wraps a real KeyValue so a test can inject a race between +// CleanupAccount's read and its guarded delete, or a store failure, +// deterministically rather than through goroutine timing. +type cleanupFakeKV struct { + jetstream.KeyValue + + deleteCalls int + beforeDelete func(attempt int) error + getErr error +} + +func (f *cleanupFakeKV) Get(ctx context.Context, key string) (jetstream.KeyValueEntry, error) { + if f.getErr != nil { + return nil, f.getErr + } + return f.KeyValue.Get(ctx, key) +} + +func (f *cleanupFakeKV) Delete(ctx context.Context, key string, opts ...jetstream.KVDeleteOpt) error { + f.deleteCalls++ + if f.beforeDelete != nil { + if err := f.beforeDelete(f.deleteCalls); err != nil { + return err + } + } + return f.KeyValue.Delete(ctx, key, opts...) +} + +func TestCleanupAccount_DeletesTheSettingsKey(t *testing.T) { + svc := setupTestAccountService(t) + ctx := context.Background() + _, err := svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + + result, err := svc.CleanupAccount(ctx, testAccountID) + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupDeleted, result.Disposition) + assert.Equal(t, evidenceRevisionGuardedDelete, result.Evidence) + assert.NotZero(t, result.Revision) + + _, err = svc.settingsKV.Get(ctx, testAccountID) + assert.ErrorIs(t, err, jetstream.ErrKeyNotFound) +} + +func TestCleanupAccount_RepeatCallIsIdempotent(t *testing.T) { + svc := setupTestAccountService(t) + ctx := context.Background() + _, err := svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + + first, err := svc.CleanupAccount(ctx, testAccountID) + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupDeleted, first.Disposition) + + second, err := svc.CleanupAccount(ctx, testAccountID) + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupAbsent, second.Disposition) + assert.Equal(t, evidenceObservedAbsent, second.Evidence) +} + +func TestCleanupAccount_AbsentKeyIsIdempotent(t *testing.T) { + svc := setupTestAccountService(t) + result, err := svc.CleanupAccount(context.Background(), "333333333333") + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupAbsent, result.Disposition) + assert.Equal(t, evidenceObservedAbsent, result.Evidence) +} + +func TestCleanupAccount_OtherAccountsUntouched(t *testing.T) { + svc := setupTestAccountService(t) + ctx := context.Background() + accountA, accountB := testAccountID, "222222222222" + _, err := svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, accountA) + require.NoError(t, err) + _, err = svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, accountB) + require.NoError(t, err) + + result, err := svc.CleanupAccount(ctx, accountA) + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupDeleted, result.Disposition) + + out, err := svc.GetEbsEncryptionByDefault(ctx, &ec2.GetEbsEncryptionByDefaultInput{}, accountB) + require.NoError(t, err) + assert.True(t, *out.EbsEncryptionByDefault) +} + +func TestCleanupAccount_RefusesGlobalAccountID(t *testing.T) { + svc := setupTestAccountService(t) + result, err := svc.CleanupAccount(context.Background(), awsidentifiers.GlobalAccountID) + assert.Error(t, err) + assert.Nil(t, result) +} + +func TestCleanupAccount_RefusesEmptyAccountID(t *testing.T) { + svc := setupTestAccountService(t) + result, err := svc.CleanupAccount(context.Background(), "") + assert.Error(t, err) + assert.Nil(t, result) +} + +func TestCleanupAccount_RefusesInvalidAccountID(t *testing.T) { + svc := setupTestAccountService(t) + result, err := svc.CleanupAccount(context.Background(), "not-an-account") + assert.Error(t, err) + assert.Nil(t, result) +} + +// A concurrent Put landing between CleanupAccount's read and its +// revision-guarded delete must not be lost silently: the delete is rejected +// and retried against the revision the write left behind. +func TestCleanupAccount_ConcurrentPutDuringCleanupRetriesAndDeletes(t *testing.T) { + svc := setupTestAccountService(t) + ctx := context.Background() + _, err := svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + + fake := &cleanupFakeKV{KeyValue: svc.settingsKV} + fake.beforeDelete = func(attempt int) error { + if attempt != 1 { + return nil + } + // Simulate a racing settings write landing after CleanupAccount's + // read but before its guarded delete, through the real KV so the + // delete's expected revision is now stale. + return svc.saveSettings(ctx, &AccountSettingsRecord{SerialConsoleAccess: true}, testAccountID) + } + svc.settingsKV = fake + + result, err := svc.CleanupAccount(ctx, testAccountID) + require.NoError(t, err) + assert.Equal(t, AccountSettingsCleanupDeleted, result.Disposition) + assert.Equal(t, 2, fake.deleteCalls, "the first delete is rejected by the race and retried once") +} + +// A revision that keeps moving under sustained concurrent writes must not +// spin forever: cleanup gives up and reports failed, bounded by attempts. +func TestCleanupAccount_PersistentConcurrentWritesFailsBounded(t *testing.T) { + svc := setupTestAccountService(t) + ctx := context.Background() + _, err := svc.EnableEbsEncryptionByDefault(ctx, &ec2.EnableEbsEncryptionByDefaultInput{}, testAccountID) + require.NoError(t, err) + + fake := &cleanupFakeKV{KeyValue: svc.settingsKV} + fake.beforeDelete = func(int) error { + return svc.saveSettings(ctx, &AccountSettingsRecord{SerialConsoleAccess: true}, testAccountID) + } + svc.settingsKV = fake + + result, err := svc.CleanupAccount(ctx, testAccountID) + assert.Error(t, err) + require.NotNil(t, result) + assert.Equal(t, AccountSettingsCleanupFailed, result.Disposition) + assert.Equal(t, cleanupMaxAttempts, fake.deleteCalls) +} + +// A store error must report failed, never absent: absence is a claim about +// what the store holds, which an error gives no basis to make. +func TestCleanupAccount_StoreErrorIsFailedNotAbsent(t *testing.T) { + svc := setupTestAccountService(t) + fake := &cleanupFakeKV{KeyValue: svc.settingsKV, getErr: errors.New("kv unavailable")} + svc.settingsKV = fake + + result, err := svc.CleanupAccount(context.Background(), testAccountID) + assert.Error(t, err) + require.NotNil(t, result) + assert.Equal(t, AccountSettingsCleanupFailed, result.Disposition) +}