From fbc94cb28caf308cf7116617d5b43a550d8eea35 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 12 Aug 2026 23:28:03 +1000 Subject: [PATCH] refactor(test): share the IAM ensurer and instance store test doubles (#777) * refactor(test): share the IAM ensurer and instance store test doubles The IAM system-instance-role ensurer was hand-faked in seven packages and a stopped-instance store in eight types across three, so one contract had many independently drifting doubles. Add mock packages for both, following the shape of network/ovn/mock, and use them where behaviour matches. handlers/iam's own tests move to package handlers_iam_test: the mock imports handlers_iam, so an in-package test importing the mock would cycle. The vm package keeps its local state store fakes. Moving them would mean converting shutdown_test.go, manager_deps_test.go and restore_test.go, which lean on unexported Manager internals, for no gain to other packages. The gateway IAM tests keep theirs too: they mock the ~50-method IAMService, a genuinely different contract. * fix: keep mock writes out of the claimable stopped pool WriteStoppedInstance was inserting into Stopped, the map ClaimStoppedInstance draws from. A caller that claimed an instance, failed downstream and rolled back by writing the record made it claimable again, so a concurrent caller could claim the same instance and both would reach vmMgr.Run. This broke TestStartStoppedInstance_ConcurrentClaimRace, which the local fake it replaced had passed: that fake recorded writes only. Writes now land in WroteStopped alone. Also flips both mock test files to external test packages to satisfy test-package-check. --- .../daemon_handlers_instance_tags_test.go | 63 +-- .../daemon/daemon_handlers_instance_test.go | 308 +++------------ spinifex/daemon/daemon_handlers_tag_test.go | 8 +- spinifex/daemon/daemon_handlers_test.go | 3 +- spinifex/daemon/daemon_test.go | 5 +- .../ec2/instance/instance_tags_test.go | 29 +- .../ec2/instance/metadata_options_test.go | 11 +- .../ec2/instance/service_impl_test.go | 359 +++++------------- spinifex/handlers/ecs/capacity_test.go | 51 +-- spinifex/handlers/eks/k3s_server_role_test.go | 20 +- .../eks/nodegroup_instance_profile_test.go | 117 +----- spinifex/handlers/elbv2/lb_role_test.go | 57 +-- spinifex/handlers/iam/mock/mock.go | 145 +++++++ spinifex/handlers/iam/mock/mock_test.go | 85 +++++ spinifex/handlers/iam/system_role_test.go | 120 +----- spinifex/handlers/rds/create_test.go | 9 +- spinifex/handlers/rds/iam_test.go | 111 +----- spinifex/handlers/rds/modify_test.go | 5 +- spinifex/handlers/rds/replace_test.go | 4 +- spinifex/handlers/rds/restore_test.go | 4 +- spinifex/handlers/rds/snapshot_test.go | 5 +- spinifex/vm/mock/mock.go | 264 +++++++++++++ spinifex/vm/mock/mock_test.go | 97 +++++ 23 files changed, 895 insertions(+), 985 deletions(-) create mode 100644 spinifex/handlers/iam/mock/mock.go create mode 100644 spinifex/handlers/iam/mock/mock_test.go create mode 100644 spinifex/vm/mock/mock.go create mode 100644 spinifex/vm/mock/mock_test.go diff --git a/spinifex/daemon/daemon_handlers_instance_tags_test.go b/spinifex/daemon/daemon_handlers_instance_tags_test.go index e3b921615..306a73f79 100644 --- a/spinifex/daemon/daemon_handlers_instance_tags_test.go +++ b/spinifex/daemon/daemon_handlers_instance_tags_test.go @@ -13,68 +13,11 @@ import ( "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/vm" - "github.com/nats-io/nats.go/jetstream" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// memStoppedStore is a map-backed StoppedInstanceStore standing in for the -// shared JetStream KV in tag fallback tests. -type memStoppedStore struct { - instances map[string]*vm.VM -} - -var _ handlers_ec2_instance.StoppedInstanceStore = (*memStoppedStore)(nil) - -func (m *memStoppedStore) LoadStoppedInstance(id string) (*vm.VM, error) { - return m.instances[id], nil -} - -func (m *memStoppedStore) ListStoppedInstances() ([]*vm.VM, error) { - var out []*vm.VM - for _, v := range m.instances { - out = append(out, v) - } - return out, nil -} - -func (m *memStoppedStore) ListTerminatedInstances() ([]*vm.VM, error) { return nil, nil } - -func (m *memStoppedStore) WriteStoppedInstance(id string, instance *vm.VM) error { - if m.instances == nil { - m.instances = make(map[string]*vm.VM) - } - m.instances[id] = instance - return nil -} - -func (m *memStoppedStore) DeleteStoppedInstance(id string) error { - delete(m.instances, id) - return nil -} - -// UpdateStoppedInstance mimics the real CAS semantics: a missing record -// returns jetstream.ErrKeyNotFound instead of resurrecting it. -func (m *memStoppedStore) UpdateStoppedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { - v, ok := m.instances[id] - if !ok { - return nil, jetstream.ErrKeyNotFound - } - mutate(v) - return v, nil -} - -func (m *memStoppedStore) ClaimStoppedInstance(id string) (*vm.VM, error) { - v, ok := m.instances[id] - if !ok { - return nil, vm.ErrStoppedInstanceClaimed - } - delete(m.instances, id) - return v, nil -} - -func (m *memStoppedStore) WriteTerminatedInstance(string, *vm.VM) error { return nil } - // tagTestDaemon returns a test daemon with an in-memory central tag store, an // empty stopped store, and a running VM carrying the given initial tags. func tagTestDaemon(t *testing.T, instanceID string, initial map[string]string) *Daemon { @@ -84,12 +27,12 @@ func tagTestDaemon(t *testing.T, instanceID string, initial map[string]string) * // tagTestDaemonWithStopped is tagTestDaemon exposing the stopped store, so // tests can seed stopped instances for the no-running-owner fallback. -func tagTestDaemonWithStopped(t *testing.T, instanceID string, initial map[string]string) (*Daemon, *memStoppedStore) { +func tagTestDaemonWithStopped(t *testing.T, instanceID string, initial map[string]string) (*Daemon, *vmmock.StateStore) { t.Helper() d := createTestDaemon(t, sharedNATSURL) d.tagsService = handlers_ec2_tags.NewTagsServiceImplWithStore(d.config, objectstore.NewMemoryObjectStore()) - stopped := &memStoppedStore{instances: map[string]*vm.VM{}} + stopped := vmmock.New() d.instanceService = handlers_ec2_instance.NewInstanceServiceImpl( d.config, d.resourceMgr.instanceTypes, d.natsConn, objectstore.NewMemoryObjectStore(), d.vmMgr, d.resourceMgr, stopped) diff --git a/spinifex/daemon/daemon_handlers_instance_test.go b/spinifex/daemon/daemon_handlers_instance_test.go index 4a58fd466..dd2add803 100644 --- a/spinifex/daemon/daemon_handlers_instance_test.go +++ b/spinifex/daemon/daemon_handlers_instance_test.go @@ -3,7 +3,6 @@ package daemon import ( "encoding/json" "errors" - "sync" "testing" "time" @@ -14,199 +13,24 @@ import ( "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/nats-io/nats.go" - "github.com/nats-io/nats.go/jetstream" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) // Tests in this file exercise the stopped/terminated daemon handlers in -// daemon_handlers_instance.go against an in-memory vm.StateStore fake. -// They cover error-injection paths (KV write/delete failures, retry, list -// errors) that the JetStream-backed integration tests in daemon_handlers_test.go -// cannot reach with a real backing bucket. - -// fakeStateStore is an in-memory vm.StateStore for daemon-handler unit tests. -// Each method has an optional error knob so a test can drive specific failure -// branches without standing up an embedded JetStream server. -type fakeStateStore struct { - mu sync.Mutex - - saveRunningErr error - - stopped map[string]*vm.VM - loadStoppedErr error - writeStoppedErr error - updateStoppedErr error - listStoppedErr error - deleteStoppedErr error - - // deleteStoppedFailFirst makes the first DeleteStoppedInstance call fail - // and the second succeed — exercises the handler's single retry. - deleteStoppedFailFirst bool - deleteStoppedAttempts int - - claimStoppedErr error - - terminated map[string]*vm.VM - writeTerminatedErr error - listTerminatedErr error -} - -func newFakeStateStore() *fakeStateStore { - return &fakeStateStore{ - stopped: map[string]*vm.VM{}, - terminated: map[string]*vm.VM{}, - } -} - -func (f *fakeStateStore) SaveRunningState(_ string, _ map[string]*vm.VM) error { - return f.saveRunningErr -} - -func (f *fakeStateStore) LoadRunningState(_ string) (map[string]*vm.VM, error) { - return map[string]*vm.VM{}, nil -} - -func (f *fakeStateStore) WriteStoppedInstance(id string, v *vm.VM) error { - if f.writeStoppedErr != nil { - return f.writeStoppedErr - } - f.mu.Lock() - defer f.mu.Unlock() - f.stopped[id] = v - return nil -} - -func (f *fakeStateStore) LoadStoppedInstance(id string) (*vm.VM, error) { - if f.loadStoppedErr != nil { - return nil, f.loadStoppedErr - } - f.mu.Lock() - defer f.mu.Unlock() - if v, ok := f.stopped[id]; ok { - return v, nil - } - return nil, nil -} - -func (f *fakeStateStore) DeleteStoppedInstance(id string) error { - f.mu.Lock() - f.deleteStoppedAttempts++ - attempt := f.deleteStoppedAttempts - f.mu.Unlock() - - if f.deleteStoppedErr != nil { - return f.deleteStoppedErr - } - if f.deleteStoppedFailFirst && attempt == 1 { - return errors.New("simulated transient delete failure") - } - f.mu.Lock() - defer f.mu.Unlock() - delete(f.stopped, id) - return nil -} - -// ClaimStoppedInstance mimics the real atomic-delete claim for tests: under -// the store lock, remove and return the entry, or claimStoppedErr / -// vm.ErrStoppedInstanceClaimed if it is already gone or a test forced an error. -func (f *fakeStateStore) ClaimStoppedInstance(id string) (*vm.VM, error) { - if f.claimStoppedErr != nil { - return nil, f.claimStoppedErr - } - f.mu.Lock() - defer f.mu.Unlock() - v, ok := f.stopped[id] - if !ok { - return nil, vm.ErrStoppedInstanceClaimed - } - delete(f.stopped, id) - return v, nil -} - -// UpdateStoppedInstance mimics the real CAS semantics for tests: mutate runs -// under the store lock against the stored value, and a missing record -// returns jetstream.ErrKeyNotFound (matching JetStreamManager's createIfAbsent=false -// behavior) rather than resurrecting it. -func (f *fakeStateStore) UpdateStoppedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { - f.mu.Lock() - defer f.mu.Unlock() - if f.updateStoppedErr != nil { - return nil, f.updateStoppedErr - } - v, ok := f.stopped[id] - if !ok { - return nil, jetstream.ErrKeyNotFound - } - mutate(v) - return v, nil -} - -func (f *fakeStateStore) ListStoppedInstances() ([]*vm.VM, error) { - if f.listStoppedErr != nil { - return nil, f.listStoppedErr - } - f.mu.Lock() - defer f.mu.Unlock() - out := make([]*vm.VM, 0, len(f.stopped)) - for _, v := range f.stopped { - out = append(out, v) - } - return out, nil -} - -func (f *fakeStateStore) WriteTerminatedInstance(id string, v *vm.VM) error { - if f.writeTerminatedErr != nil { - return f.writeTerminatedErr - } - f.mu.Lock() - defer f.mu.Unlock() - f.terminated[id] = v - return nil -} - -func (f *fakeStateStore) ListTerminatedInstances() ([]*vm.VM, error) { - if f.listTerminatedErr != nil { - return nil, f.listTerminatedErr - } - f.mu.Lock() - defer f.mu.Unlock() - out := make([]*vm.VM, 0, len(f.terminated)) - for _, v := range f.terminated { - out = append(out, v) - } - return out, nil -} - -func (f *fakeStateStore) DeleteTerminatedInstance(id string) error { - f.mu.Lock() - defer f.mu.Unlock() - delete(f.terminated, id) - return nil -} - -// UpdateTerminatedInstance mimics the real CAS semantics for tests: mutate -// runs under the store lock against the stored value. -func (f *fakeStateStore) UpdateTerminatedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { - f.mu.Lock() - defer f.mu.Unlock() - v, ok := f.terminated[id] - if !ok { - return nil, errors.New("terminated instance not found") - } - mutate(v) - return v, nil -} - -var _ vm.StateStore = (*fakeStateStore)(nil) +// daemon_handlers_instance.go against the shared in-memory vm/mock.StateStore +// fake. They cover error-injection paths (KV write/delete failures, retry, +// list errors) that the JetStream-backed integration tests in +// daemon_handlers_test.go cannot reach with a real backing bucket. // daemonWithFakeStateStore returns a daemon wired with an in-memory NATS // connection (via createTestDaemon) and the supplied fake StateStore. // The daemon does not have JetStream initialized. Rewires d.instanceService // to point at the fake store so handlers that delegate to InstanceService // (e.g. ModifyInstanceAttribute) see the injected state. -func daemonWithFakeStateStore(t *testing.T, store *fakeStateStore) *Daemon { +func daemonWithFakeStateStore(t *testing.T, store *vmmock.StateStore) *Daemon { t.Helper() d := createTestDaemon(t, sharedNATSURL) d.stateStore = store @@ -262,8 +86,8 @@ func stoppedVMFixture(id, accountID string) *vm.VM { // --- handleEC2StartStoppedInstance --- func TestHandleEC2StartStoppedInstance_LoadError(t *testing.T) { - store := newFakeStateStore() - store.loadStoppedErr = errors.New("kv unavailable") + store := vmmock.New() + store.LoadStoppedErr = errors.New("kv unavailable") d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.StartStoppedInstanceInput{InstanceID: "i-load-fail"}) @@ -281,8 +105,8 @@ func TestHandleEC2StartStoppedInstance_StateStoreNil(t *testing.T) { } func TestHandleEC2StartStoppedInstance_CrossTenantRejected(t *testing.T) { - store := newFakeStateStore() - store.stopped["i-foreign"] = stoppedVMFixture("i-foreign", "999988887777") + store := vmmock.New() + store.Stopped["i-foreign"] = stoppedVMFixture("i-foreign", "999988887777") d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.StartStoppedInstanceInput{InstanceID: "i-foreign"}) @@ -291,17 +115,15 @@ func TestHandleEC2StartStoppedInstance_CrossTenantRejected(t *testing.T) { // The instance must remain in shared KV — cross-tenant rejection cannot // also remove it (would be a leak across accounts). - store.mu.Lock() - _, stillStopped := store.stopped["i-foreign"] - store.mu.Unlock() + _, stillStopped := store.Stopped["i-foreign"] assert.True(t, stillStopped, "cross-tenant rejection must not delete the stopped instance") } func TestHandleEC2StartStoppedInstance_InstanceTypeUnknown(t *testing.T) { - store := newFakeStateStore() + store := vmmock.New() v := stoppedVMFixture("i-unknown-type", testAccountID) v.InstanceType = "definitely.not.a.real.type" - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.StartStoppedInstanceInput{InstanceID: v.ID}) @@ -330,11 +152,11 @@ func withShortForwardTimeout(t *testing.T, d time.Duration) { func TestHandleEC2StartStoppedInstance_ForwardTimeoutFallsBackLocally(t *testing.T) { withShortForwardTimeout(t, 50*time.Millisecond) - store := newFakeStateStore() + store := vmmock.New() v := stoppedVMFixture("i-timeout-fallback", testAccountID) v.InstanceType = "definitely.not.a.real.type" v.LastNode = "node-other" - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) // Simulate a live-but-unresponsive original node: subscribed, so no @@ -358,10 +180,10 @@ func TestHandleEC2StartStoppedInstance_ForwardTimeoutFallsBackLocally(t *testing func TestHandleEC2StartStoppedInstance_ForwardTimeoutAfterRemoteClaim_NoDoubleStart(t *testing.T) { withShortForwardTimeout(t, 50*time.Millisecond) - store := newFakeStateStore() + store := vmmock.New() v := stoppedVMFixture("i-race-claim", testAccountID) v.LastNode = "node-other" - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) // Simulate the original node winning the claim (atomically removing the @@ -387,8 +209,8 @@ func TestHandleEC2StartStoppedInstance_ForwardTimeoutAfterRemoteClaim_NoDoubleSt // --- handleEC2TerminateStoppedInstance --- func TestHandleEC2TerminateStoppedInstance_LoadError(t *testing.T) { - store := newFakeStateStore() - store.loadStoppedErr = errors.New("kv unavailable") + store := vmmock.New() + store.LoadStoppedErr = errors.New("kv unavailable") d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.TerminateStoppedInstanceInput{InstanceID: "i-load-fail"}) @@ -407,21 +229,19 @@ func TestHandleEC2TerminateStoppedInstance_StateStoreNil(t *testing.T) { // WriteTerminatedInstance failure must abort BEFORE the stopped-bucket // delete — otherwise an instance can vanish from both buckets. func TestHandleEC2TerminateStoppedInstance_WriteTerminatedFailureAborts(t *testing.T) { - store := newFakeStateStore() - store.writeTerminatedErr = errors.New("terminated bucket write failed") + store := vmmock.New() + store.WriteTerminatedErr = errors.New("terminated bucket write failed") v := stoppedVMFixture("i-write-term-fail", testAccountID) - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.TerminateStoppedInstanceInput{InstanceID: v.ID}) reply := requestHandler(t, d.natsConn, "ec2.terminate.test3", handleNATSRequest(d.instanceService.TerminateStoppedInstance), testAccountID, body) assert.Equal(t, awserrors.ErrorServerInternal, decodeError(t, reply.Data)["Code"]) - store.mu.Lock() - _, stillStopped := store.stopped[v.ID] - _, inTerminated := store.terminated[v.ID] - attempts := store.deleteStoppedAttempts - store.mu.Unlock() + _, stillStopped := store.Stopped[v.ID] + _, inTerminated := store.Terminated[v.ID] + attempts := store.DeleteAttempts assert.True(t, stillStopped, "stopped entry must remain when terminated write fails (caller can retry)") assert.False(t, inTerminated, "no terminated entry should exist after write failure") assert.Equal(t, 0, attempts, "DeleteStoppedInstance must not be called when terminated write fails") @@ -430,10 +250,10 @@ func TestHandleEC2TerminateStoppedInstance_WriteTerminatedFailureAborts(t *testi // First stopped-bucket delete fails, second succeeds — instance must end up // only in the terminated bucket and the handler must still respond success. func TestHandleEC2TerminateStoppedInstance_DeleteRetrySucceeds(t *testing.T) { - store := newFakeStateStore() - store.deleteStoppedFailFirst = true + store := vmmock.New() + store.DeleteFailFirst = true v := stoppedVMFixture("i-retry-success", testAccountID) - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.TerminateStoppedInstanceInput{InstanceID: v.ID}) @@ -443,11 +263,9 @@ func TestHandleEC2TerminateStoppedInstance_DeleteRetrySucceeds(t *testing.T) { require.NoError(t, json.Unmarshal(reply.Data, &resp)) assert.Equal(t, "terminated", resp["status"]) - store.mu.Lock() - _, stillStopped := store.stopped[v.ID] - _, inTerminated := store.terminated[v.ID] - attempts := store.deleteStoppedAttempts - store.mu.Unlock() + _, stillStopped := store.Stopped[v.ID] + _, inTerminated := store.Terminated[v.ID] + attempts := store.DeleteAttempts assert.False(t, stillStopped, "stopped entry must be removed after retry success") assert.True(t, inTerminated, "terminated entry must be present") assert.Equal(t, 2, attempts, "DeleteStoppedInstance must be retried exactly once") @@ -457,10 +275,10 @@ func TestHandleEC2TerminateStoppedInstance_DeleteRetrySucceeds(t *testing.T) { // (the terminated-bucket write is the source of truth) and must NOT roll back // the terminated write. func TestHandleEC2TerminateStoppedInstance_DeleteAlwaysFailsKeepsTerminated(t *testing.T) { - store := newFakeStateStore() - store.deleteStoppedErr = errors.New("delete persistently broken") + store := vmmock.New() + store.DeleteStoppedErr = errors.New("delete persistently broken") v := stoppedVMFixture("i-retry-fail", testAccountID) - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.TerminateStoppedInstanceInput{InstanceID: v.ID}) @@ -470,25 +288,21 @@ func TestHandleEC2TerminateStoppedInstance_DeleteAlwaysFailsKeepsTerminated(t *t require.NoError(t, json.Unmarshal(reply.Data, &resp)) assert.Equal(t, "terminated", resp["status"], "handler must report success — terminated write succeeded") - store.mu.Lock() - _, inTerminated := store.terminated[v.ID] - store.mu.Unlock() + _, inTerminated := store.Terminated[v.ID] assert.True(t, inTerminated, "terminated entry must NOT be rolled back when stopped delete fails") } func TestHandleEC2TerminateStoppedInstance_CrossTenantRejected(t *testing.T) { - store := newFakeStateStore() - store.stopped["i-foreign-term"] = stoppedVMFixture("i-foreign-term", "999988887777") + store := vmmock.New() + store.Stopped["i-foreign-term"] = stoppedVMFixture("i-foreign-term", "999988887777") d := daemonWithFakeStateStore(t, store) body, _ := json.Marshal(handlers_ec2_instance.TerminateStoppedInstanceInput{InstanceID: "i-foreign-term"}) reply := requestHandler(t, d.natsConn, "ec2.terminate.test6", handleNATSRequest(d.instanceService.TerminateStoppedInstance), testAccountID, body) assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, decodeError(t, reply.Data)["Code"]) - store.mu.Lock() - _, inTerminated := store.terminated["i-foreign-term"] - _, stillStopped := store.stopped["i-foreign-term"] - store.mu.Unlock() + _, inTerminated := store.Terminated["i-foreign-term"] + _, stillStopped := store.Stopped["i-foreign-term"] assert.False(t, inTerminated, "foreign-tenant terminate must not write to terminated bucket") assert.True(t, stillStopped, "foreign-tenant terminate must not delete the stopped entry") } @@ -496,10 +310,10 @@ func TestHandleEC2TerminateStoppedInstance_CrossTenantRejected(t *testing.T) { // --- handleEC2ModifyInstanceAttribute --- func TestHandleEC2ModifyInstanceAttribute_WriteFailureReturnsServerInternal(t *testing.T) { - store := newFakeStateStore() - store.updateStoppedErr = errors.New("kv write failed") + store := vmmock.New() + store.UpdateStoppedErr = errors.New("kv write failed") v := stoppedVMFixture("i-mod-write-fail", testAccountID) - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) input := &ec2.ModifyInstanceAttributeInput{ @@ -512,8 +326,8 @@ func TestHandleEC2ModifyInstanceAttribute_WriteFailureReturnsServerInternal(t *t } func TestHandleEC2ModifyInstanceAttribute_LoadFailureReturnsServerInternal(t *testing.T) { - store := newFakeStateStore() - store.loadStoppedErr = errors.New("kv unavailable") + store := vmmock.New() + store.LoadStoppedErr = errors.New("kv unavailable") d := daemonWithFakeStateStore(t, store) input := &ec2.ModifyInstanceAttributeInput{ @@ -528,7 +342,7 @@ func TestHandleEC2ModifyInstanceAttribute_LoadFailureReturnsServerInternal(t *te func TestHandleEC2ModifyInstanceAttribute_NilInstanceFieldGuard(t *testing.T) { // Stored VM with a valid status but a nil Instance pointer — the handler // must reject this as a data-integrity violation rather than NPE. - store := newFakeStateStore() + store := vmmock.New() v := &vm.VM{ ID: "i-mod-nil-inst", Status: vm.StateStopped, @@ -537,7 +351,7 @@ func TestHandleEC2ModifyInstanceAttribute_NilInstanceFieldGuard(t *testing.T) { Reservation: &ec2.Reservation{ReservationId: aws.String("r-x"), OwnerId: aws.String(testAccountID)}, Instance: nil, } - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) input := &ec2.ModifyInstanceAttributeInput{ @@ -550,9 +364,9 @@ func TestHandleEC2ModifyInstanceAttribute_NilInstanceFieldGuard(t *testing.T) { } func TestHandleEC2ModifyInstanceAttribute_EmptyInstanceTypeRejected(t *testing.T) { - store := newFakeStateStore() + store := vmmock.New() v := stoppedVMFixture("i-mod-empty-type", testAccountID) - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) input := &ec2.ModifyInstanceAttributeInput{ @@ -565,8 +379,8 @@ func TestHandleEC2ModifyInstanceAttribute_EmptyInstanceTypeRejected(t *testing.T } func TestHandleEC2ModifyInstanceAttribute_CrossTenantRejected(t *testing.T) { - store := newFakeStateStore() - store.stopped["i-mod-foreign"] = stoppedVMFixture("i-mod-foreign", "999988887777") + store := vmmock.New() + store.Stopped["i-mod-foreign"] = stoppedVMFixture("i-mod-foreign", "999988887777") d := daemonWithFakeStateStore(t, store) input := &ec2.ModifyInstanceAttributeInput{ @@ -581,8 +395,8 @@ func TestHandleEC2ModifyInstanceAttribute_CrossTenantRejected(t *testing.T) { // --- handleEC2DescribeInstanceAttribute --- func TestHandleEC2DescribeInstanceAttribute_StoppedFallback_LoadError(t *testing.T) { - store := newFakeStateStore() - store.loadStoppedErr = errors.New("kv unavailable") + store := vmmock.New() + store.LoadStoppedErr = errors.New("kv unavailable") d := daemonWithFakeStateStore(t, store) // d.vmMgr has no running instance, so the handler falls through to the // stopped KV branch — which now errors. @@ -597,11 +411,11 @@ func TestHandleEC2DescribeInstanceAttribute_StoppedFallback_LoadError(t *testing } func TestHandleEC2DescribeInstanceAttribute_StoppedFallback_HitsKV(t *testing.T) { - store := newFakeStateStore() + store := vmmock.New() v := stoppedVMFixture("i-describe-stopped", testAccountID) v.InstanceType = "t3.medium" v.Instance.InstanceType = aws.String("t3.medium") - store.stopped[v.ID] = v + store.Stopped[v.ID] = v d := daemonWithFakeStateStore(t, store) input := &ec2.DescribeInstanceAttributeInput{ @@ -635,8 +449,8 @@ func TestHandleEC2DescribeInstanceAttribute_StateStoreNil(t *testing.T) { // --- handleEC2DescribeStoppedInstances / handleEC2DescribeTerminatedInstances --- func TestHandleEC2DescribeStoppedInstances_ListError(t *testing.T) { - store := newFakeStateStore() - store.listStoppedErr = errors.New("list failed") + store := vmmock.New() + store.ListStoppedErr = errors.New("list failed") d := daemonWithFakeStateStore(t, store) reply := requestHandler(t, d.natsConn, "ec2.DescribeStoppedInstances.test1", handleNATSRequest(d.instanceService.DescribeStoppedInstances), testAccountID, []byte("{}")) @@ -644,8 +458,8 @@ func TestHandleEC2DescribeStoppedInstances_ListError(t *testing.T) { } func TestHandleEC2DescribeTerminatedInstances_ListError(t *testing.T) { - store := newFakeStateStore() - store.listTerminatedErr = errors.New("list failed") + store := vmmock.New() + store.ListTerminatedErr = errors.New("list failed") d := daemonWithFakeStateStore(t, store) reply := requestHandler(t, d.natsConn, "ec2.DescribeTerminatedInstances.test1", handleNATSRequest(d.instanceService.DescribeTerminatedInstances), testAccountID, []byte("{}")) @@ -653,9 +467,9 @@ func TestHandleEC2DescribeTerminatedInstances_ListError(t *testing.T) { } func TestHandleEC2DescribeStoppedInstances_CrossAccountIsolation(t *testing.T) { - store := newFakeStateStore() - store.stopped["i-mine"] = stoppedVMFixture("i-mine", testAccountID) - store.stopped["i-yours"] = stoppedVMFixture("i-yours", "999988887777") + store := vmmock.New() + store.Stopped["i-mine"] = stoppedVMFixture("i-mine", testAccountID) + store.Stopped["i-yours"] = stoppedVMFixture("i-yours", "999988887777") d := daemonWithFakeStateStore(t, store) reply := requestHandler(t, d.natsConn, "ec2.DescribeStoppedInstances.test3", handleNATSRequest(d.instanceService.DescribeStoppedInstances), testAccountID, []byte("{}")) diff --git a/spinifex/daemon/daemon_handlers_tag_test.go b/spinifex/daemon/daemon_handlers_tag_test.go index a83cf0308..f5a76d7e1 100644 --- a/spinifex/daemon/daemon_handlers_tag_test.go +++ b/spinifex/daemon/daemon_handlers_tag_test.go @@ -116,7 +116,7 @@ func TestDeleteTags_NoOwnerNotFound(t *testing.T) { func TestCreateTags_StoppedFallback(t *testing.T) { const id = "i-tag-stopfall" d, stopped := tagTestDaemonWithStopped(t, "i-tag-unrelated3", nil) - stopped.instances[id] = &vm.VM{ + stopped.Stopped[id] = &vm.VM{ ID: id, Status: vm.StateStopped, AccountID: testAccountID, @@ -131,7 +131,7 @@ func TestCreateTags_StoppedFallback(t *testing.T) { require.NoError(t, err) want := map[string]string{"Name": "web", "env": "prod"} - assert.Equal(t, want, tagsAsMap(stopped.instances[id].Instance.Tags)) + assert.Equal(t, want, tagsAsMap(stopped.Stopped[id].Instance.Tags)) assert.Equal(t, want, centralTags(t, d, testAccountID, id)) } @@ -141,7 +141,7 @@ func TestDeleteTags_StoppedCrossAccountRejected(t *testing.T) { const id = "i-tag-stopcross" const attacker = "999999999999" d, stopped := tagTestDaemonWithStopped(t, "i-tag-unrelated4", nil) - stopped.instances[id] = &vm.VM{ + stopped.Stopped[id] = &vm.VM{ ID: id, Status: vm.StateStopped, AccountID: testAccountID, @@ -155,7 +155,7 @@ func TestDeleteTags_StoppedCrossAccountRejected(t *testing.T) { require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) - assert.Equal(t, map[string]string{"Name": "web"}, tagsAsMap(stopped.instances[id].Instance.Tags)) + assert.Equal(t, map[string]string{"Name": "web"}, tagsAsMap(stopped.Stopped[id].Instance.Tags)) assert.Empty(t, centralTags(t, d, attacker, id)) } diff --git a/spinifex/daemon/daemon_handlers_test.go b/spinifex/daemon/daemon_handlers_test.go index ee5f5df51..958b0df78 100644 --- a/spinifex/daemon/daemon_handlers_test.go +++ b/spinifex/daemon/daemon_handlers_test.go @@ -39,6 +39,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/testutil/ebsfake" "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1939,7 +1940,7 @@ func TestDelegateHandlers_RoundTrip(t *testing.T) { daemon.instanceService = handlers_ec2_instance.NewInstanceServiceImpl( daemon.config, daemon.resourceMgr.instanceTypes, daemon.natsConn, objectstore.NewMemoryObjectStore(), daemon.vmMgr, daemon.resourceMgr, - &memStoppedStore{}) + vmmock.New()) daemon.instanceService.SetRunInstancesDeps(daemon.imageService, daemon.keyService, nil, nil) tests := []struct { diff --git a/spinifex/daemon/daemon_test.go b/spinifex/daemon/daemon_test.go index 3af83a27b..85c9df47c 100644 --- a/spinifex/daemon/daemon_test.go +++ b/spinifex/daemon/daemon_test.go @@ -52,6 +52,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" "github.com/stretchr/testify/assert" @@ -1764,7 +1765,7 @@ func TestTerminatedTeardownReaper_SelfHealsFailedVolumeTeardown(t *testing.T) { func TestTerminatedTeardownReaper_SelfHealsFailedVolumeDetach(t *testing.T) { daemon, store := createFullTestDaemonWithStore(t, sharedNATSURL) - fakeStore := newFakeStateStore() + fakeStore := vmmock.New() daemon.stateStore = fakeStore volumeID := "vol-data-self-heal" @@ -1838,7 +1839,7 @@ func TestStuckTerminateReaper_DetachesNonDoTVolumeWithoutUnmount(t *testing.T) { AttachedInstance: instanceID, // never cleared: Unmount never ran on this path }) - fakeStore := newFakeStateStore() + fakeStore := vmmock.New() mgr := vm.NewManagerWithDeps(vm.Deps{ NodeID: daemon.node, StateStore: fakeStore, diff --git a/spinifex/handlers/ec2/instance/instance_tags_test.go b/spinifex/handlers/ec2/instance/instance_tags_test.go index a3c5c09e2..4f975ad89 100644 --- a/spinifex/handlers/ec2/instance/instance_tags_test.go +++ b/spinifex/handlers/ec2/instance/instance_tags_test.go @@ -10,6 +10,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/awserrors" spxtypes "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -141,7 +142,7 @@ func stoppedTagInstance(id, accountID string, tags []*ec2.Tag) *vm.VM { func TestTagStoppedInstance_WritesRecordAndCentral(t *testing.T) { stored := stoppedTagInstance("i-123", "111122223333", tagList("Name", "web")) - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{"i-123": stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{"i-123": stored}} writer := &fakeTagWriter{} svc := &InstanceServiceImpl{stoppedStore: store} @@ -151,18 +152,18 @@ func TestTagStoppedInstance_WritesRecordAndCentral(t *testing.T) { require.NoError(t, err) want := map[string]string{"Name": "web", "env": "prod"} - written := store.loadByID["i-123"] + written := store.Stopped["i-123"] require.NotNil(t, written) assert.Equal(t, want, tagsAsMap(written.Instance.Tags)) assert.Equal(t, want, writer.tags) assert.Equal(t, "i-123", writer.resourceID) assert.Equal(t, "111122223333", writer.accountID) - assert.Equal(t, 1, store.updateStoppedCalls) + assert.Equal(t, 1, store.UpdateStoppedCalls) } func TestTagStoppedInstance_RemoveKeys(t *testing.T) { stored := stoppedTagInstance("i-123", "111122223333", tagList("Name", "web", "env", "dev")) - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{"i-123": stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{"i-123": stored}} writer := &fakeTagWriter{} svc := &InstanceServiceImpl{stoppedStore: store} @@ -172,12 +173,12 @@ func TestTagStoppedInstance_RemoveKeys(t *testing.T) { require.NoError(t, err) want := map[string]string{"Name": "web"} - assert.Equal(t, want, tagsAsMap(store.loadByID["i-123"].Instance.Tags)) + assert.Equal(t, want, tagsAsMap(store.Stopped["i-123"].Instance.Tags)) assert.Equal(t, want, writer.tags) } func TestTagStoppedInstance_NotFound(t *testing.T) { - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{}} writer := &fakeTagWriter{} svc := &InstanceServiceImpl{stoppedStore: store} @@ -187,12 +188,12 @@ func TestTagStoppedInstance_NotFound(t *testing.T) { require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) assert.Zero(t, writer.calls) - assert.Empty(t, store.wroteStopped) + assert.Empty(t, store.WroteStopped) } func TestTagStoppedInstance_CrossAccountRejected(t *testing.T) { stored := stoppedTagInstance("i-123", "999988887777", tagList("Name", "web")) - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{"i-123": stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{"i-123": stored}} writer := &fakeTagWriter{} svc := &InstanceServiceImpl{stoppedStore: store} @@ -202,7 +203,7 @@ func TestTagStoppedInstance_CrossAccountRejected(t *testing.T) { require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) assert.Zero(t, writer.calls) - assert.Empty(t, store.wroteStopped) + assert.Empty(t, store.WroteStopped) } // TestTagStoppedInstance_ConcurrentClaimDoesNotResurrect covers a @@ -212,7 +213,7 @@ func TestTagStoppedInstance_CrossAccountRejected(t *testing.T) { // stale stopped entry, and the caller must see a NotFound-shaped error. func TestTagStoppedInstance_ConcurrentClaimDoesNotResurrect(t *testing.T) { stored := stoppedTagInstance("i-123", "111122223333", tagList("Name", "web")) - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{"i-123": stored}, claimAfterLoad: true} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{"i-123": stored}, ClaimAfterLoad: true} writer := &fakeTagWriter{} svc := &InstanceServiceImpl{stoppedStore: store} @@ -221,13 +222,13 @@ func TestTagStoppedInstance_ConcurrentClaimDoesNotResurrect(t *testing.T) { }, false, writer, "111122223333") require.Error(t, err) assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) - assert.Empty(t, store.loadByID, "the claimed record must not be resurrected") - assert.Equal(t, []string{"i-123"}, store.claimedStopped) + assert.Empty(t, store.Stopped, "the claimed record must not be resurrected") + assert.Equal(t, []string{"i-123"}, store.ClaimedStopped) } func TestTagStoppedInstance_CentralWriteErrorSkipsRecordWrite(t *testing.T) { stored := stoppedTagInstance("i-123", "111122223333", tagList("Name", "web")) - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{"i-123": stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{"i-123": stored}} writer := &fakeTagWriter{err: errors.New("s3 down")} svc := &InstanceServiceImpl{stoppedStore: store} @@ -236,5 +237,5 @@ func TestTagStoppedInstance_CentralWriteErrorSkipsRecordWrite(t *testing.T) { }, false, writer, "111122223333") require.Error(t, err) assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) - assert.Empty(t, store.wroteStopped) + assert.Empty(t, store.WroteStopped) } diff --git a/spinifex/handlers/ec2/instance/metadata_options_test.go b/spinifex/handlers/ec2/instance/metadata_options_test.go index ca4713eb7..ca0c20151 100644 --- a/spinifex/handlers/ec2/instance/metadata_options_test.go +++ b/spinifex/handlers/ec2/instance/metadata_options_test.go @@ -11,6 +11,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/ebsmetadata" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -124,7 +125,7 @@ func TestModifyInstanceMetadataOptions_HopLimitStopped(t *testing.T) { ID: id, AccountID: owner, Status: vm.StateStopped, Instance: &ec2.Instance{InstanceId: aws.String(id), MetadataOptions: buildMetadataOptions(nil, "")}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: stored}} svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{}), stoppedStore: store} out, err := svc.ModifyInstanceMetadataOptions(context.Background(), &ec2.ModifyInstanceMetadataOptionsInput{ @@ -133,15 +134,15 @@ func TestModifyInstanceMetadataOptions_HopLimitStopped(t *testing.T) { }, owner) require.NoError(t, err) assert.Equal(t, int64(3), aws.Int64Value(out.InstanceMetadataOptions.HttpPutResponseHopLimit)) - require.Contains(t, store.wroteStopped, id) - assert.Equal(t, int64(3), aws.Int64Value(store.wroteStopped[id].Instance.MetadataOptions.HttpPutResponseHopLimit)) + require.Contains(t, store.WroteStopped, id) + assert.Equal(t, int64(3), aws.Int64Value(store.WroteStopped[id].Instance.MetadataOptions.HttpPutResponseHopLimit)) } // An unknown instance is reported absent, not a server error. func TestModifyInstanceMetadataOptions_NotFound(t *testing.T) { svc := &InstanceServiceImpl{ vmMgr: mgrWith(map[string]*vm.VM{}), - stoppedStore: &fakeStoppedStore{loadByID: map[string]*vm.VM{}}, + stoppedStore: &vmmock.StateStore{Stopped: map[string]*vm.VM{}}, } _, err := svc.ModifyInstanceMetadataOptions(context.Background(), &ec2.ModifyInstanceMetadataOptionsInput{ InstanceId: aws.String("i-ghost"), @@ -171,7 +172,7 @@ func TestModifyInstanceMetadataOptions_NilInstance(t *testing.T) { t.Run("stopped", func(t *testing.T) { id := "i-nil-stop" stored := &vm.VM{ID: id, AccountID: owner, Status: vm.StateStopped, Instance: nil} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: stored}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: stored}} svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{}), stoppedStore: store} _, err := svc.ModifyInstanceMetadataOptions(context.Background(), &ec2.ModifyInstanceMetadataOptionsInput{ InstanceId: aws.String(id), HttpPutResponseHopLimit: aws.Int64(2), diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 8079f5138..417aeb814 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -21,8 +21,8 @@ import ( spxtypes "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" + vmmock "github.com/mulgadc/spinifex/spinifex/vm/mock" "github.com/nats-io/nats.go" - "github.com/nats-io/nats.go/jetstream" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -602,157 +602,6 @@ func (f *fakeResourceCapacityProvider) InstanceTypes() map[string]*ec2.InstanceT return f.instanceTypes } -type fakeStoppedStore struct { - mu sync.Mutex - stopped []*vm.VM - terminated []*vm.VM - loadByID map[string]*vm.VM - wroteStopped map[string]*vm.VM - wroteTerminated map[string]*vm.VM - deletedStopped []string - listErr error - listTermErr error - loadErr error - writeErr error - updateErr error - writeTermErr error - deleteErr error - deleteFailFirst bool - deleteAttempts int - - // claimedStopped / claimErr drive ClaimStoppedInstance, the atomic - // claim used by StartStoppedInstance. Claiming removes the entry from - // loadByID (mirroring the real KV delete-as-claim), guarded by mu so - // concurrent StartStoppedInstance callers race exactly like production. - claimedStopped []string - claimErr error - claimAttempts int - - // updateStoppedCalls counts UpdateStoppedInstance invocations for tests - // that assert on retry/race behavior. - updateStoppedCalls int - - // claimAfterLoad simulates a winning ClaimStoppedInstance landing between - // a caller's Load and its later UpdateStoppedInstance: the record is - // removed right after LoadStoppedInstance returns it, so the subsequent - // UpdateStoppedInstance sees a missing entry exactly like the real CAS. - claimAfterLoad bool -} - -func (f *fakeStoppedStore) LoadStoppedInstance(id string) (*vm.VM, error) { - if f.loadErr != nil { - return nil, f.loadErr - } - f.mu.Lock() - defer f.mu.Unlock() - v, ok := f.loadByID[id] - if !ok { - return nil, nil - } - if f.claimAfterLoad { - delete(f.loadByID, id) - f.claimedStopped = append(f.claimedStopped, id) - } - return v, nil -} -func (f *fakeStoppedStore) ListStoppedInstances() ([]*vm.VM, error) { - if f.listErr != nil { - return nil, f.listErr - } - f.mu.Lock() - defer f.mu.Unlock() - return f.stopped, nil -} -func (f *fakeStoppedStore) ListTerminatedInstances() ([]*vm.VM, error) { - if f.listTermErr != nil { - return nil, f.listTermErr - } - f.mu.Lock() - defer f.mu.Unlock() - return f.terminated, nil -} -func (f *fakeStoppedStore) WriteStoppedInstance(id string, instance *vm.VM) error { - if f.writeErr != nil { - return f.writeErr - } - f.mu.Lock() - defer f.mu.Unlock() - if f.wroteStopped == nil { - f.wroteStopped = make(map[string]*vm.VM) - } - f.wroteStopped[id] = instance - return nil -} -func (f *fakeStoppedStore) WriteTerminatedInstance(id string, instance *vm.VM) error { - if f.writeTermErr != nil { - return f.writeTermErr - } - f.mu.Lock() - defer f.mu.Unlock() - if f.wroteTerminated == nil { - f.wroteTerminated = make(map[string]*vm.VM) - } - f.wroteTerminated[id] = instance - return nil -} -func (f *fakeStoppedStore) DeleteStoppedInstance(id string) error { - f.mu.Lock() - f.deleteAttempts++ - attempt := f.deleteAttempts - f.mu.Unlock() - if f.deleteFailFirst && attempt == 1 { - return errors.New("transient delete failure") - } - if f.deleteErr != nil { - return f.deleteErr - } - f.mu.Lock() - defer f.mu.Unlock() - f.deletedStopped = append(f.deletedStopped, id) - return nil -} -func (f *fakeStoppedStore) DeleteTerminatedInstance(string) error { return nil } - -// UpdateStoppedInstance mimics the real CAS semantics: mutate runs under the -// store lock against loadByID's entry, and a missing record (e.g. removed by -// a concurrent ClaimStoppedInstance) returns jetstream.ErrKeyNotFound instead of -// resurrecting it — the same createIfAbsent=false contract as JetStreamManager. -func (f *fakeStoppedStore) UpdateStoppedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { - f.mu.Lock() - defer f.mu.Unlock() - f.updateStoppedCalls++ - if f.updateErr != nil { - return nil, f.updateErr - } - v, ok := f.loadByID[id] - if !ok { - return nil, jetstream.ErrKeyNotFound - } - mutate(v) - return v, nil -} - -// ClaimStoppedInstance mimics the real atomic delete-as-claim: under the -// store lock, remove and return loadByID[id], or vm.ErrStoppedInstanceClaimed -// if it is already gone — mirroring the real KV's "revision conflict or -// not-found" outcome for a losing racer. The lock makes this genuinely -// exclusive under concurrent callers, matching the production guarantee. -func (f *fakeStoppedStore) ClaimStoppedInstance(id string) (*vm.VM, error) { - f.mu.Lock() - defer f.mu.Unlock() - f.claimAttempts++ - if f.claimErr != nil { - return nil, f.claimErr - } - v, ok := f.loadByID[id] - if !ok { - return nil, vm.ErrStoppedInstanceClaimed - } - delete(f.loadByID, id) - f.claimedStopped = append(f.claimedStopped, id) - return v, nil -} - func TestDescribeInstanceTypes_NilResourceMgr(t *testing.T) { svc := &InstanceServiceImpl{} _, err := svc.DescribeInstanceTypes(context.Background(), &ec2.DescribeInstanceTypesInput{}, "") @@ -1092,7 +941,7 @@ func TestDescribeInstanceAttribute_NotRunning_NoStore(t *testing.T) { func TestDescribeInstanceAttribute_FoundInStoppedStore(t *testing.T) { id := "i-stopped1" owner := utils.GlobalAccountID - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, InstanceType: "t3.medium", AccountID: owner}, }} svc := &InstanceServiceImpl{ @@ -1111,7 +960,7 @@ func TestDescribeInstanceAttribute_FoundInStoppedStore(t *testing.T) { func TestDescribeInstanceAttribute_NotFound(t *testing.T) { svc := &InstanceServiceImpl{ vmMgr: mgrWith(map[string]*vm.VM{}), - stoppedStore: &fakeStoppedStore{loadByID: map[string]*vm.VM{}}, + stoppedStore: &vmmock.StateStore{Stopped: map[string]*vm.VM{}}, } _, err := svc.DescribeInstanceAttribute(context.Background(), &ec2.DescribeInstanceAttributeInput{ InstanceId: aws.String("i-ghost"), @@ -1200,9 +1049,9 @@ func TestDescribeStoppedInstances_NilStore(t *testing.T) { func TestDescribeStoppedInstances_HappyPath(t *testing.T) { owner := utils.GlobalAccountID - store := &fakeStoppedStore{ - stopped: []*vm.VM{ - { + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{ + "i-stop1": { ID: "i-stop1", AccountID: owner, Reservation: &ec2.Reservation{ @@ -1230,9 +1079,9 @@ func TestDescribeTerminatedInstances_NilStore(t *testing.T) { func TestDescribeTerminatedInstances_HappyPath(t *testing.T) { owner := utils.GlobalAccountID - store := &fakeStoppedStore{ - terminated: []*vm.VM{ - { + store := &vmmock.StateStore{ + Terminated: map[string]*vm.VM{ + "i-term1": { ID: "i-term1", AccountID: owner, Reservation: &ec2.Reservation{ @@ -1267,9 +1116,9 @@ func TestDescribeKVInstances_FilteringGroupingAndIsolation(t *testing.T) { Reservation: &ec2.Reservation{ReservationId: aws.String("r-other")}, Instance: &ec2.Instance{InstanceId: aws.String("i-other")}, } - store := &fakeStoppedStore{ - stopped: []*vm.VM{first, second, other}, - terminated: []*vm.VM{first, second, other}, + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{first.ID: first, second.ID: second, other.ID: other}, + Terminated: map[string]*vm.VM{first.ID: first, second.ID: second, other.ID: other}, } svc := &InstanceServiceImpl{stoppedStore: store, config: &config.Config{}} @@ -1361,7 +1210,7 @@ func TestModifyInstanceAttribute_NilStore(t *testing.T) { } func TestModifyInstanceAttribute_InstanceNotFound(t *testing.T) { - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{}} svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.ModifyInstanceAttribute(context.Background(), &ec2.ModifyInstanceAttributeInput{ @@ -1374,7 +1223,7 @@ func TestModifyInstanceAttribute_InstanceNotFound(t *testing.T) { func TestModifyInstanceAttribute_NotStopped(t *testing.T) { id := "i-running" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateRunning, AccountID: "acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1389,7 +1238,7 @@ func TestModifyInstanceAttribute_NotStopped(t *testing.T) { func TestModifyInstanceAttribute_NotVisible(t *testing.T) { id := "i-stopped" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "owner-acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1404,7 +1253,7 @@ func TestModifyInstanceAttribute_NotVisible(t *testing.T) { func TestModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { id := "i-type" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, @@ -1425,7 +1274,7 @@ func TestModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { }, "acc") require.NoError(t, err) - updated := store.loadByID[id] + updated := store.Stopped[id] require.NotNil(t, updated) assert.Equal(t, "t3.medium", updated.InstanceType) assert.Equal(t, "t3.medium", updated.Config.InstanceType) @@ -1434,7 +1283,7 @@ func TestModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { func TestModifyInstanceAttribute_ChangeInstanceType_EmptyValue(t *testing.T) { id := "i-empty" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", Instance: &ec2.Instance{}}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1449,7 +1298,7 @@ func TestModifyInstanceAttribute_ChangeInstanceType_EmptyValue(t *testing.T) { func TestModifyInstanceAttribute_ChangeInstanceType_NilEmbeddedInstance(t *testing.T) { id := "i-nil-inst" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1464,7 +1313,7 @@ func TestModifyInstanceAttribute_ChangeInstanceType_NilEmbeddedInstance(t *testi func TestModifyInstanceAttribute_ChangeUserData(t *testing.T) { id := "i-ud" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, @@ -1484,14 +1333,14 @@ func TestModifyInstanceAttribute_ChangeUserData(t *testing.T) { }, "acc") require.NoError(t, err) - updated := store.loadByID[id] + updated := store.Stopped[id] require.NotNil(t, updated) assert.Equal(t, "IyEvYmluL2Jhc2g=", *updated.RunInstancesInput.UserData) } func TestModifyInstanceAttribute_ClearsStateReason(t *testing.T) { id := "i-recovery" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, @@ -1516,7 +1365,7 @@ func TestModifyInstanceAttribute_ClearsStateReason(t *testing.T) { }, "acc") require.NoError(t, err) - updated := store.loadByID[id] + updated := store.Stopped[id] require.NotNil(t, updated) assert.Nil(t, updated.Instance.StateReason) } @@ -1573,7 +1422,7 @@ func TestModifyInstanceAttribute_DisableApiTermination_Running_NotVisible(t *tes func TestModifyInstanceAttribute_DisableApiTermination_Stopped(t *testing.T) { id := "i-stop-prot" owner := "acc" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, AccountID: owner, Instance: &ec2.Instance{InstanceId: aws.String(id)}, @@ -1590,7 +1439,7 @@ func TestModifyInstanceAttribute_DisableApiTermination_Stopped(t *testing.T) { }, owner) require.NoError(t, err) - updated := store.loadByID[id] + updated := store.Stopped[id] require.NotNil(t, updated) require.NotNil(t, updated.RunInstancesInput) assert.True(t, *updated.RunInstancesInput.DisableApiTermination) @@ -1598,8 +1447,8 @@ func TestModifyInstanceAttribute_DisableApiTermination_Stopped(t *testing.T) { func TestModifyInstanceAttribute_WriteError(t *testing.T) { id := "i-werr" - store := &fakeStoppedStore{ - loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, @@ -1612,7 +1461,7 @@ func TestModifyInstanceAttribute_WriteError(t *testing.T) { }, }, }, - updateErr: fmt.Errorf("kv write boom"), + UpdateStoppedErr: fmt.Errorf("kv write boom"), } svc := &InstanceServiceImpl{stoppedStore: store} @@ -1631,8 +1480,8 @@ func TestModifyInstanceAttribute_WriteError(t *testing.T) { // stale stopped entry. func TestModifyInstanceAttribute_ConcurrentClaimDoesNotResurrect(t *testing.T) { id := "i-raced" - store := &fakeStoppedStore{ - loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, @@ -1645,7 +1494,7 @@ func TestModifyInstanceAttribute_ConcurrentClaimDoesNotResurrect(t *testing.T) { }, }, }, - claimAfterLoad: true, + ClaimAfterLoad: true, } svc := &InstanceServiceImpl{stoppedStore: store} @@ -1655,8 +1504,8 @@ func TestModifyInstanceAttribute_ConcurrentClaimDoesNotResurrect(t *testing.T) { }, "acc") require.Error(t, err) assert.Equal(t, awserrors.ErrorIncorrectInstanceState, err.Error()) - assert.Empty(t, store.loadByID, "the claimed record must not be resurrected") - assert.Equal(t, []string{id}, store.claimedStopped) + assert.Empty(t, store.Stopped, "the claimed record must not be resurrected") + assert.Equal(t, []string{id}, store.ClaimedStopped) } // --- TerminateStoppedInstance tests --- @@ -1752,7 +1601,7 @@ func TestTerminateStoppedInstance_NilStore(t *testing.T) { } func TestTerminateStoppedInstance_LoadError(t *testing.T) { - store := &fakeStoppedStore{loadErr: errors.New("kv down")} + store := &vmmock.StateStore{LoadStoppedErr: errors.New("kv down")} svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: "i-1"}, "acc") @@ -1761,7 +1610,7 @@ func TestTerminateStoppedInstance_LoadError(t *testing.T) { } func TestTerminateStoppedInstance_NotFound(t *testing.T) { - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{}} svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: "i-missing"}, "acc") @@ -1771,7 +1620,7 @@ func TestTerminateStoppedInstance_NotFound(t *testing.T) { func TestTerminateStoppedInstance_NotStopped(t *testing.T) { id := "i-running" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateRunning, AccountID: "acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1783,7 +1632,7 @@ func TestTerminateStoppedInstance_NotStopped(t *testing.T) { func TestTerminateStoppedInstance_NotVisible(t *testing.T) { id := "i-stopped" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "owner-acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1804,7 +1653,7 @@ func TestTerminateStoppedInstance_TerminationProtected(t *testing.T) { v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-001", DeleteOnTermination: true}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} vd := &fakeVolumeDeleter{} svc := &InstanceServiceImpl{stoppedStore: store, volumeDeleter: vd} @@ -1813,13 +1662,13 @@ func TestTerminateStoppedInstance_TerminationProtected(t *testing.T) { assert.Equal(t, awserrors.ErrorOperationNotPermitted, err.Error()) assert.Empty(t, vd.calls, "volumes must not be deleted when termination protected") - assert.Empty(t, store.wroteTerminated, "must not write to terminated bucket when protected") - assert.Empty(t, store.deletedStopped, "must not remove from stopped bucket when protected") + assert.Empty(t, store.WroteTerminated, "must not write to terminated bucket when protected") + assert.Empty(t, store.DeletedStopped, "must not remove from stopped bucket when protected") } func TestTerminateStoppedInstance_HappyPath(t *testing.T) { id := "i-term-001" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}, }} svc := &InstanceServiceImpl{stoppedStore: store} @@ -1830,14 +1679,14 @@ func TestTerminateStoppedInstance_HappyPath(t *testing.T) { assert.Equal(t, "terminated", out.Status) assert.Equal(t, id, out.InstanceID) - require.NotNil(t, store.wroteTerminated[id]) - assert.Equal(t, vm.StateTerminated, store.wroteTerminated[id].Status) - assert.Contains(t, store.deletedStopped, id) + require.NotNil(t, store.WroteTerminated[id]) + assert.Equal(t, vm.StateTerminated, store.WroteTerminated[id].Status) + assert.Contains(t, store.DeletedStopped, id) } func TestTerminateStoppedInstance_CentralTagsDeleted(t *testing.T) { id := "i-term-tags" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}, }} tw := &fakeTagWriter{} @@ -1853,7 +1702,7 @@ func TestTerminateStoppedInstance_CentralTagsDeleted(t *testing.T) { func TestTerminateStoppedInstance_CentralTagDeleteFailureIsBestEffort(t *testing.T) { id := "i-term-tagerr" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}, }} tw := &fakeTagWriter{err: errors.New("s3 down")} @@ -1862,13 +1711,13 @@ func TestTerminateStoppedInstance_CentralTagDeleteFailureIsBestEffort(t *testing out, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.NoError(t, err, "terminate already succeeded; central delete failure must not surface") assert.Equal(t, "terminated", out.Status) - assert.Contains(t, store.deletedStopped, id) + assert.Contains(t, store.DeletedStopped, id) } func TestTerminateStoppedInstance_ProtectedSkipsCentralTagDelete(t *testing.T) { id := "i-prot-tags" tw := &fakeTagWriter{} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: { ID: id, Status: vm.StateStopped, AccountID: "acc", RunInstancesInput: &ec2.RunInstancesInput{DisableApiTermination: aws.Bool(true)}, @@ -1883,30 +1732,30 @@ func TestTerminateStoppedInstance_ProtectedSkipsCentralTagDelete(t *testing.T) { func TestTerminateStoppedInstance_WriteTerminatedError_Aborts(t *testing.T) { id := "i-werr" - store := &fakeStoppedStore{ - loadByID: map[string]*vm.VM{id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}}, - writeTermErr: errors.New("kv write boom"), + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}}, + WriteTerminatedErr: errors.New("kv write boom"), } svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.Error(t, err) assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) - assert.Empty(t, store.deletedStopped, "stopped delete must not run when terminated write fails") + assert.Empty(t, store.DeletedStopped, "stopped delete must not run when terminated write fails") } func TestTerminateStoppedInstance_RetriesStoppedDelete(t *testing.T) { id := "i-retry" - store := &fakeStoppedStore{ - loadByID: map[string]*vm.VM{id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}}, - deleteFailFirst: true, + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}}, + DeleteFailFirst: true, } svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.NoError(t, err) - assert.Equal(t, 2, store.deleteAttempts, "first delete fails, retry succeeds") - assert.Contains(t, store.deletedStopped, id) + assert.Equal(t, 2, store.DeleteAttempts, "first delete fails, retry succeeds") + assert.Contains(t, store.DeletedStopped, id) } func TestTerminateStoppedInstance_UserVolumeDeleted(t *testing.T) { @@ -1916,7 +1765,7 @@ func TestTerminateStoppedInstance_UserVolumeDeleted(t *testing.T) { {Name: "vol-user-001", DeleteOnTermination: true}, {Name: "vol-keep-001", DeleteOnTermination: false}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} vd := &fakeVolumeDeleter{} svc := &InstanceServiceImpl{stoppedStore: store, volumeDeleter: vd} @@ -1938,15 +1787,15 @@ func TestTerminateStoppedInstance_StampsTeardownVolumesDone(t *testing.T) { v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-root-001", Boot: true, DeleteOnTermination: true}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} vd := &fakeVolumeDeleter{} svc := &InstanceServiceImpl{stoppedStore: store, volumeDeleter: vd} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.NoError(t, err) - require.NotNil(t, store.wroteTerminated[id]) - assert.Equal(t, string(vm.TeardownDone), store.wroteTerminated[id].Teardown[vm.TeardownVolumes], + require.NotNil(t, store.WroteTerminated[id]) + assert.Equal(t, string(vm.TeardownDone), store.WroteTerminated[id].Teardown[vm.TeardownVolumes], "ADR-0003 §1 semantics: a successful volume delete must stamp Teardown[volumes]=done") assert.Equal(t, []string{"vol-root-001"}, vd.deleted, "the still-attached root volume must actually be deleted") } @@ -1962,15 +1811,15 @@ func TestTerminateStoppedInstance_StampsTeardownVolumesFailedOnDeleteError(t *te v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-root-002", Boot: true, DeleteOnTermination: true}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} vd := &fakeVolumeDeleter{err: errors.New("delete forced failure")} svc := &InstanceServiceImpl{stoppedStore: store, volumeDeleter: vd} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.NoError(t, err, "terminate itself stays best-effort; the failure is tracked via Teardown, not returned") - require.NotNil(t, store.wroteTerminated[id]) - assert.Equal(t, string(vm.TeardownFailed), store.wroteTerminated[id].Teardown[vm.TeardownVolumes], + require.NotNil(t, store.WroteTerminated[id]) + assert.Equal(t, string(vm.TeardownFailed), store.WroteTerminated[id].Teardown[vm.TeardownVolumes], "a DeleteVolumeOnTerminate failure must be surfaced via Teardown, not silently swallowed") assert.Empty(t, vd.deleted, "the forced failure must mean nothing was actually deleted") } @@ -1987,7 +1836,7 @@ func TestTerminateStoppedInstance_NonDoTBootVolumeDetachedNotDeleted(t *testing. v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-root-nondot", Boot: true, DeleteOnTermination: false}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} vd := &fakeVolumeDeleter{} svc := &InstanceServiceImpl{stoppedStore: store, volumeDeleter: vd} @@ -1996,8 +1845,8 @@ func TestTerminateStoppedInstance_NonDoTBootVolumeDetachedNotDeleted(t *testing. assert.Equal(t, []string{"vol-root-nondot"}, vd.detached, "the still-attached non-DoT boot volume must be detached") assert.Empty(t, vd.deleted, "a DeleteOnTermination=false volume must never be deleted") - require.NotNil(t, store.wroteTerminated[id]) - assert.Equal(t, string(vm.TeardownDone), store.wroteTerminated[id].Teardown[vm.TeardownVolumes]) + require.NotNil(t, store.WroteTerminated[id]) + assert.Equal(t, string(vm.TeardownDone), store.WroteTerminated[id].Teardown[vm.TeardownVolumes]) } func TestTerminateStoppedInstance_NoVolumeDeleterSkipsGracefully(t *testing.T) { @@ -2006,12 +1855,12 @@ func TestTerminateStoppedInstance_NoVolumeDeleterSkipsGracefully(t *testing.T) { v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-user-001", DeleteOnTermination: true}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} svc := &InstanceServiceImpl{stoppedStore: store} _, err := svc.TerminateStoppedInstance(context.Background(), &TerminateStoppedInstanceInput{InstanceID: id}, "acc") require.NoError(t, err, "missing VolumeDeleter must not abort termination") - require.NotNil(t, store.wroteTerminated[id]) + require.NotNil(t, store.WroteTerminated[id]) } func TestTerminateStoppedInstance_InternalVolumesViaNATS(t *testing.T) { @@ -2020,7 +1869,7 @@ func TestTerminateStoppedInstance_InternalVolumesViaNATS(t *testing.T) { v.EBSRequests.Requests = []spxtypes.EBSRequest{ {Name: "vol-efi-001", EFI: true}, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} nc := embeddedNATS(t) var ebsDeleted []string @@ -2055,7 +1904,7 @@ func TestTerminateStoppedInstance_PublicIPReleased(t *testing.T) { PrivateIpAddress: aws.String("10.0.0.5"), }, } - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} pr := &fakePublicIPReleaser{} svc := &InstanceServiceImpl{stoppedStore: store, ipReleaser: pr} @@ -2068,7 +1917,7 @@ func TestTerminateStoppedInstance_PublicIPReleased(t *testing.T) { func TestTerminateStoppedInstance_ENIDeleted(t *testing.T) { id := "i-eni" v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc", ENIId: "eni-1234"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{} ec := &fakeENICreator{} svc := &InstanceServiceImpl{stoppedStore: store, eniDeleter: ed, eniCreator: ec} @@ -2086,7 +1935,7 @@ func TestTerminateStoppedInstance_ENIDeleteNotFoundTolerated(t *testing.T) { // instance must still finalize to the terminated bucket, not error out. id := "i-eni-gone" v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc", ENIId: "eni-gone"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{err: errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound)} svc := &InstanceServiceImpl{stoppedStore: store, eniDeleter: ed, eniCreator: &fakeENICreator{}} @@ -2102,7 +1951,7 @@ func TestTerminateStoppedInstance_ENIDeleteNotFoundTolerated(t *testing.T) { func TestTerminateStoppedInstance_PrimaryENIDetachFailureContinuesToDelete(t *testing.T) { id := "i-eni-detach-fail" v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc", ENIId: "eni-stuck"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{} ec := &fakeENICreator{detachErr: errors.New("detach unavailable")} svc := &InstanceServiceImpl{stoppedStore: store, eniDeleter: ed, eniCreator: ec} @@ -2120,7 +1969,7 @@ func TestTerminateStoppedInstance_PrimaryENIDetachFailureContinuesToDelete(t *te func TestTerminateStoppedInstance_PrimaryENIDeleteUnexpectedError(t *testing.T) { id := "i-eni-delete-fail" v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc", ENIId: "eni-broken"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{err: errors.New("server unavailable")} svc := &InstanceServiceImpl{stoppedStore: store, eniDeleter: ed, eniCreator: &fakeENICreator{}} @@ -2137,7 +1986,7 @@ func TestTerminateStoppedInstance_PrimaryENIDeleteUnexpectedError(t *testing.T) func TestTerminateStoppedInstance_ReleaseAttachedENIs_ListErrorLogsAndReturns(t *testing.T) { id := "i-list-err" v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{} ec := &fakeENICreator{listENIsErr: errors.New("kv unreachable")} svc := &InstanceServiceImpl{stoppedStore: store, eniDeleter: ed, eniCreator: ec} @@ -2198,7 +2047,7 @@ func TestTerminateStoppedInstance_ReleaseAttachedENIs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { id := "i-sweep-" + tt.name v := &vm.VM{ID: id, Status: vm.StateStopped, AccountID: "acc"} - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{id: v}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{id: v}} ed := &fakeENIDeleter{err: tt.deleteErr} ec := &fakeENICreator{ detachErr: tt.detachErr, @@ -2253,7 +2102,7 @@ func TestStartStoppedInstance_NilStore(t *testing.T) { } func TestStartStoppedInstance_NilResourceMgr(t *testing.T) { - svc := &InstanceServiceImpl{stoppedStore: &fakeStoppedStore{loadByID: map[string]*vm.VM{}}} + svc := &InstanceServiceImpl{stoppedStore: &vmmock.StateStore{Stopped: map[string]*vm.VM{}}} _, err := svc.StartStoppedInstance(context.Background(), &StartStoppedInstanceInput{InstanceID: "i-1"}, "acc") require.Error(t, err) assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) @@ -2261,7 +2110,7 @@ func TestStartStoppedInstance_NilResourceMgr(t *testing.T) { func TestStartStoppedInstance_NilVMMgr(t *testing.T) { svc := &InstanceServiceImpl{ - stoppedStore: &fakeStoppedStore{loadByID: map[string]*vm.VM{}}, + stoppedStore: &vmmock.StateStore{Stopped: map[string]*vm.VM{}}, resourceMgr: &fakeResourceCapacityProvider{}, } _, err := svc.StartStoppedInstance(context.Background(), &StartStoppedInstanceInput{InstanceID: "i-1"}, "acc") @@ -2270,7 +2119,7 @@ func TestStartStoppedInstance_NilVMMgr(t *testing.T) { } func TestStartStoppedInstance_LoadError(t *testing.T) { - store := &fakeStoppedStore{loadErr: errors.New("kv down")} + store := &vmmock.StateStore{LoadStoppedErr: errors.New("kv down")} svc := &InstanceServiceImpl{ stoppedStore: store, resourceMgr: &fakeResourceCapacityProvider{}, @@ -2282,7 +2131,7 @@ func TestStartStoppedInstance_LoadError(t *testing.T) { } func TestStartStoppedInstance_NotFound(t *testing.T) { - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{}} svc := &InstanceServiceImpl{ stoppedStore: store, resourceMgr: &fakeResourceCapacityProvider{}, @@ -2302,7 +2151,7 @@ func TestStartStoppedInstance_NotFound(t *testing.T) { // returns NotFound (TestStartStoppedInstance_NotFound). func TestStartStoppedInstance_NotFoundButRunningLocally(t *testing.T) { id := "i-won-locally" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{}} mgr := vm.NewManager() mgr.Insert(&vm.VM{ID: id, Status: vm.StateRunning, AccountID: "acc"}) svc := &InstanceServiceImpl{ @@ -2317,7 +2166,7 @@ func TestStartStoppedInstance_NotFoundButRunningLocally(t *testing.T) { func TestStartStoppedInstance_NotStopped(t *testing.T) { id := "i-running" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateRunning, AccountID: "acc"}, }} svc := &InstanceServiceImpl{ @@ -2332,7 +2181,7 @@ func TestStartStoppedInstance_NotStopped(t *testing.T) { func TestStartStoppedInstance_NotVisible(t *testing.T) { id := "i-foreign" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "owner-acc", InstanceType: "t3.micro"}, }} svc := &InstanceServiceImpl{ @@ -2345,12 +2194,12 @@ func TestStartStoppedInstance_NotVisible(t *testing.T) { assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) // Cross-tenant rejection must not delete from KV. - assert.Empty(t, store.deletedStopped, "cross-tenant rejection must not delete stopped instance") + assert.Empty(t, store.DeletedStopped, "cross-tenant rejection must not delete stopped instance") } func TestStartStoppedInstance_InstanceTypeUnknown(t *testing.T) { id := "i-badtype" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", InstanceType: "z99.nope"}, }} prov := &fakeResourceCapacityProvider{instanceTypes: map[string]*ec2.InstanceTypeInfo{}} @@ -2366,15 +2215,15 @@ func TestStartStoppedInstance_InstanceTypeUnknown(t *testing.T) { // The claim removed the entry from KV before the instance-type check; // the rollback must write it back so the instance is not lost. - assert.Contains(t, store.claimedStopped, id, "claim must run before the instance-type check") - require.NotNil(t, store.wroteStopped[id], "claimed instance must be restored to KV after a downstream failure") - assert.Equal(t, vm.StateStopped, store.wroteStopped[id].Status) + assert.Contains(t, store.ClaimedStopped, id, "claim must run before the instance-type check") + require.NotNil(t, store.WroteStopped[id], "claimed instance must be restored to KV after a downstream failure") + assert.Equal(t, vm.StateStopped, store.WroteStopped[id].Status) } func TestStartStoppedInstance_AllocateFails(t *testing.T) { id := "i-alloc-fail" itype := "t3.micro" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", InstanceType: itype}, }} prov := &fakeResourceCapacityProvider{ @@ -2395,8 +2244,8 @@ func TestStartStoppedInstance_AllocateFails(t *testing.T) { // A failed Allocate must not leave the instance stranded — it was // already claimed (removed from KV) by this point, so it must be // restored. - require.NotNil(t, store.wroteStopped[id], "claimed instance must be restored to KV after Allocate failure") - assert.Equal(t, vm.StateStopped, store.wroteStopped[id].Status) + require.NotNil(t, store.WroteStopped[id], "claimed instance must be restored to KV after Allocate failure") + assert.Equal(t, vm.StateStopped, store.WroteStopped[id].Status) } // GPU claim failure must roll back the resource allocation and remove the VM @@ -2404,7 +2253,7 @@ func TestStartStoppedInstance_AllocateFails(t *testing.T) { func TestStartStoppedInstance_GPUClaimFailureRollsBack(t *testing.T) { id := "i-gpu-fail" itype := "g5.xlarge" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", InstanceType: itype}, }} prov := &fakeResourceCapacityProvider{ @@ -2430,12 +2279,12 @@ func TestStartStoppedInstance_GPUClaimFailureRollsBack(t *testing.T) { require.Len(t, prov.deallocated, 1, "GPU claim failure must trigger deallocate") _, stillInMgr := mgr.Get(id) assert.False(t, stillInMgr, "GPU claim failure must remove the VM from the manager map") - assert.Empty(t, store.deletedStopped, "stopped-KV entry must remain on rollback") + assert.Empty(t, store.DeletedStopped, "stopped-KV entry must remain on rollback") // The atomic claim removed the entry from KV before Allocate/GPU-claim // ran; the rollback must write it back so it is not lost. - require.NotNil(t, store.wroteStopped[id], "claimed instance must be restored to KV after GPU claim failure") - assert.Equal(t, vm.StateStopped, store.wroteStopped[id].Status) + require.NotNil(t, store.WroteStopped[id], "claimed instance must be restored to KV after GPU claim failure") + assert.Equal(t, vm.StateStopped, store.WroteStopped[id].Status) } // TestStartStoppedInstance_ClaimConflict proves a lost claim race is @@ -2443,11 +2292,11 @@ func TestStartStoppedInstance_GPUClaimFailureRollsBack(t *testing.T) { func TestStartStoppedInstance_ClaimConflict(t *testing.T) { id := "i-claim-conflict" itype := "t3.micro" - store := &fakeStoppedStore{ - loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{ + Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", InstanceType: itype}, }, - claimErr: vm.ErrStoppedInstanceClaimed, + ClaimStoppedErr: vm.ErrStoppedInstanceClaimed, } prov := &fakeResourceCapacityProvider{ instanceTypes: map[string]*ec2.InstanceTypeInfo{itype: {InstanceType: aws.String(itype)}}, @@ -2503,7 +2352,7 @@ func TestStartStoppedInstance_ConcurrentClaimRace(t *testing.T) { id := "i-race" itype := "t3.micro" - store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + store := &vmmock.StateStore{Stopped: map[string]*vm.VM{ id: {ID: id, Status: vm.StateStopped, AccountID: "acc", InstanceType: itype}, }} prov := &fakeResourceCapacityProvider{ @@ -2549,11 +2398,11 @@ func TestStartStoppedInstance_ConcurrentClaimRace(t *testing.T) { // Exactly one caller ever wins the atomic claim, regardless of whether // the loser lost at the claim itself or earlier at the preliminary Load // (see comment above) — it may not always attempt the claim at all. - assert.Len(t, store.claimedStopped, 1, "the atomic claim must succeed exactly once") - assert.GreaterOrEqual(t, store.claimAttempts, 1, "the winner must have attempted the claim") + assert.Len(t, store.ClaimedStopped, 1, "the atomic claim must succeed exactly once") + assert.GreaterOrEqual(t, store.ClaimAttempts, 1, "the winner must have attempted the claim") - require.NotNil(t, store.wroteStopped[id], "the winner's downstream Run failure must restore the instance to KV") - assert.Equal(t, vm.StateStopped, store.wroteStopped[id].Status) + require.NotNil(t, store.WroteStopped[id], "the winner's downstream Run failure must restore the instance to KV") + assert.Equal(t, vm.StateStopped, store.WroteStopped[id].Status) _, stillInMgr := mgr.Get(id) assert.False(t, stillInMgr, "a failed Run must not leave the VM in the manager map") diff --git a/spinifex/handlers/ecs/capacity_test.go b/spinifex/handlers/ecs/capacity_test.go index 07101c396..ac300225e 100644 --- a/spinifex/handlers/ecs/capacity_test.go +++ b/spinifex/handlers/ecs/capacity_test.go @@ -2,7 +2,6 @@ package handlers_ecs import ( "context" - "errors" "slices" "testing" @@ -10,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/spinifex/spinifex/awserrors" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -66,8 +66,16 @@ func imageMatchesFilters(img *ec2.Image, filters []*ec2.Filter) bool { return true } -// stubIAM converges to find-or-create: Get* report NoSuchEntity, Create* succeed. -type stubIAM struct{} +// stubIAM embeds the shared SystemInstanceRoleEnsurer mock for the Get*/Create* +// find-or-create behaviour, and adds CreatePolicy/AttachRolePolicy locally +// since ecsIAM needs both beyond what the ensurer interface covers. +type stubIAM struct { + *iammock.SystemInstanceRoleEnsurer +} + +func newStubIAM() stubIAM { + return stubIAM{SystemInstanceRoleEnsurer: iammock.New()} +} var ( _ ecsIAM = stubIAM{} @@ -75,14 +83,6 @@ var ( _ ecsImageResolver = (*filteringStubImages)(nil) ) -func (stubIAM) GetRole(_ string, _ *iam.GetRoleInput) (*iam.GetRoleOutput, error) { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) -} - -func (stubIAM) CreateRole(_ string, _ *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { - return &iam.CreateRoleOutput{Role: &iam.Role{RoleName: aws.String(ecsInstanceRoleName)}}, nil -} - func (stubIAM) CreatePolicy(accountID string, _ *iam.CreatePolicyInput) (*iam.CreatePolicyOutput, error) { arn := "arn:aws:iam::" + accountID + ":policy/" + ecsInstanceRolePolicyName return &iam.CreatePolicyOutput{Policy: &iam.Policy{Arn: aws.String(arn)}}, nil @@ -92,19 +92,6 @@ func (stubIAM) AttachRolePolicy(_ string, _ *iam.AttachRolePolicyInput) (*iam.At return &iam.AttachRolePolicyOutput{}, nil } -func (stubIAM) GetInstanceProfile(_ string, _ *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) -} - -func (stubIAM) CreateInstanceProfile(accountID string, _ *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { - arn := "arn:aws:iam::" + accountID + ":instance-profile/" + ecsInstanceRoleName - return &iam.CreateInstanceProfileOutput{InstanceProfile: &iam.InstanceProfile{Arn: aws.String(arn)}}, nil -} - -func (stubIAM) AddRoleToInstanceProfile(_ string, _ *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { - return &iam.AddRoleToInstanceProfileOutput{}, nil -} - func ecsNodeImage() []*ec2.Image { return []*ec2.Image{{ ImageId: aws.String("ami-ecs"), @@ -120,7 +107,7 @@ func TestProvisionCapacity_BuildsRunInstancesInput(t *testing.T) { svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ GatewayBaseURL: "https://10.0.0.1:9999", GatewayCACert: "-----BEGIN CERTIFICATE-----\nx\n-----END CERTIFICATE-----", - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &stubImages{images: ecsNodeImage()}, RunInstances: func(_ context.Context, in *ec2.RunInstancesInput, _ string) (*ec2.Reservation, error) { captured = in @@ -159,7 +146,7 @@ func TestProvisionCapacity_BuildsRunInstancesInput(t *testing.T) { func TestProvisionCapacity_DefaultsAndCount(t *testing.T) { var captured *ec2.RunInstancesInput svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &stubImages{images: ecsNodeImage()}, RunInstances: func(_ context.Context, in *ec2.RunInstancesInput, _ string) (*ec2.Reservation, error) { captured = in @@ -190,7 +177,7 @@ func TestProvisionCapacity_DefaultsAndCount(t *testing.T) { func TestProvisionCapacity_ExplicitDiskSize(t *testing.T) { var captured *ec2.RunInstancesInput svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &stubImages{images: ecsNodeImage()}, RunInstances: func(_ context.Context, in *ec2.RunInstancesInput, _ string) (*ec2.Reservation, error) { captured = in @@ -212,7 +199,7 @@ func TestProvisionCapacity_ExplicitDiskSize(t *testing.T) { func TestProvisionCapacity_MissingRequired(t *testing.T) { svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, Images: &stubImages{images: ecsNodeImage()}, + IAM: newStubIAM(), Images: &stubImages{images: ecsNodeImage()}, RunInstances: func(context.Context, *ec2.RunInstancesInput, string) (*ec2.Reservation, error) { return nil, nil }, }) _, err := svc.ProvisionCapacity(context.Background(), &ProvisionCapacityInput{Cluster: "web"}, testAccountID) @@ -276,7 +263,7 @@ func TestLookupECSNodeAMI_ExcludesGPUTaggedAMI(t *testing.T) { func TestProvisionCapacity_GPUInstanceTypeUsesGPUAMI(t *testing.T) { var captured *ec2.RunInstancesInput svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &filteringStubImages{images: []*ec2.Image{ecsGPUNodeImage("ami-ecs-gpu", "2026-06-01T00:00:00.000Z")}}, RunInstances: func(_ context.Context, in *ec2.RunInstancesInput, _ string) (*ec2.Reservation, error) { captured = in @@ -298,7 +285,7 @@ func TestProvisionCapacity_GPUInstanceTypeUsesGPUAMI(t *testing.T) { func TestProvisionCapacity_NonGPUInstanceTypeUsesPlainAMI(t *testing.T) { var captured *ec2.RunInstancesInput svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &filteringStubImages{images: ecsNodeImage()}, RunInstances: func(_ context.Context, in *ec2.RunInstancesInput, _ string) (*ec2.Reservation, error) { captured = in @@ -322,7 +309,7 @@ func TestProvisionCapacity_NonGPUInstanceTypeUsesPlainAMI(t *testing.T) { // lookup must fail rather than silently reuse the driverless image. func TestProvisionCapacity_GPUInstanceTypeNoGPUAMIErrors(t *testing.T) { svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &filteringStubImages{images: ecsNodeImage()}, // only the plain (non-GPU) AMI resolves RunInstances: func(context.Context, *ec2.RunInstancesInput, string) (*ec2.Reservation, error) { t.Fatal("RunInstances must not be called when no GPU AMI resolves") @@ -342,7 +329,7 @@ func TestProvisionCapacity_GPUInstanceTypeNoGPUAMIErrors(t *testing.T) { func TestProvisionCapacity_AMINotFound(t *testing.T) { svc := NewService(nil, testRegion, "internal").WithDeps(Deps{ - IAM: stubIAM{}, + IAM: newStubIAM(), Images: &stubImages{images: nil}, RunInstances: func(context.Context, *ec2.RunInstancesInput, string) (*ec2.Reservation, error) { t.Fatal("RunInstances must not be called when no AMI resolves") diff --git a/spinifex/handlers/eks/k3s_server_role_test.go b/spinifex/handlers/eks/k3s_server_role_test.go index 28ed7ae78..a06f39f34 100644 --- a/spinifex/handlers/eks/k3s_server_role_test.go +++ b/spinifex/handlers/eks/k3s_server_role_test.go @@ -22,11 +22,11 @@ func TestEnsureK3sServerInstanceProfile_CreatesRolePolicyProfile(t *testing.T) { arn := s.ensureCPInstanceProfile(testSysAcct) assert.Equal(t, "arn:aws:iam::"+testSysAcct+":instance-profile/"+eksServerSystemRoleName, arn) - assert.Equal(t, 1, f.createRoleCalls) - assert.Contains(t, f.lastTrustDoc, "ec2.amazonaws.com") - assert.Contains(t, f.lastTrustDoc, "sts:AssumeRole") + assert.Equal(t, 1, f.CreateRoleCalls) + assert.Contains(t, f.LastTrustDoc, "ec2.amazonaws.com") + assert.Contains(t, f.LastTrustDoc, "sts:AssumeRole") - policy := f.rolePolicies[eksServerSystemRoleName] + policy := f.RolePolicies[eksServerSystemRoleName] assert.Contains(t, policy, "eks:PublishInternal") assert.Contains(t, policy, "eks:ListInternalAddons") // The eks-token-webhook relays get-token bearer tokens to the token-review @@ -38,8 +38,8 @@ func TestEnsureK3sServerInstanceProfile_CreatesRolePolicyProfile(t *testing.T) { // and a wedged control plane can never be auto-reset. assert.Contains(t, policy, "eks:GetRecoveryDirective") - require.NotNil(t, f.profiles[eksServerSystemRoleName]) - assert.Len(t, f.profiles[eksServerSystemRoleName].Roles, 1) + require.NotNil(t, f.Profiles[eksServerSystemRoleName]) + assert.Len(t, f.Profiles[eksServerSystemRoleName].Roles, 1) } // TestEnsureK3sServerInstanceProfile_ExistingRoleConverges asserts a re-launch @@ -47,7 +47,7 @@ func TestEnsureK3sServerInstanceProfile_CreatesRolePolicyProfile(t *testing.T) { // inline policy so a stale role converges onto the current permissions. func TestEnsureK3sServerInstanceProfile_ExistingRoleConverges(t *testing.T) { f := newFakeEnsurer() - f.roles[eksServerSystemRoleName] = &iam.Role{ + f.Roles[eksServerSystemRoleName] = &iam.Role{ RoleName: aws.String(eksServerSystemRoleName), Arn: aws.String("arn:aws:iam::" + testSysAcct + ":role/" + eksServerSystemRoleName), } @@ -55,8 +55,8 @@ func TestEnsureK3sServerInstanceProfile_ExistingRoleConverges(t *testing.T) { arn := s.ensureCPInstanceProfile(testSysAcct) assert.NotEmpty(t, arn) - assert.Zero(t, f.createRoleCalls) - assert.Contains(t, f.rolePolicies[eksServerSystemRoleName], "eks:PublishInternal") + assert.Zero(t, f.CreateRoleCalls) + assert.Contains(t, f.RolePolicies[eksServerSystemRoleName], "eks:PublishInternal") } // TestEnsureCPInstanceProfile_NilIAMFallsBack asserts an unwired IAM service @@ -76,7 +76,7 @@ func TestEnsureCPInstanceProfile_UsesLazyProvider(t *testing.T) { }} arn := s.ensureCPInstanceProfile(testSysAcct) assert.Equal(t, "arn:aws:iam::"+testSysAcct+":instance-profile/"+eksServerSystemRoleName, arn) - assert.Equal(t, 1, f.createRoleCalls) + assert.Equal(t, 1, f.CreateRoleCalls) } // TestEnsureCPInstanceProfile_ProviderNotReadyFallsBack asserts a provider that diff --git a/spinifex/handlers/eks/nodegroup_instance_profile_test.go b/spinifex/handlers/eks/nodegroup_instance_profile_test.go index 368082bbc..99faa8ef9 100644 --- a/spinifex/handlers/eks/nodegroup_instance_profile_test.go +++ b/spinifex/handlers/eks/nodegroup_instance_profile_test.go @@ -7,102 +7,15 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/spinifex/spinifex/awserrors" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// fakeInstanceProfileEnsurer records calls and serves a small in-memory profile -// store keyed by name. -type fakeInstanceProfileEnsurer struct { - profiles map[string]*iam.InstanceProfile - createErr error - addRoleErr error - createCalls int - addCalls int - - // Role store + capture for the system-role path (ensureSystemRole). - roles map[string]*iam.Role - createRoleCalls int - lastTrustDoc string - rolePolicies map[string]string // roleName -> last PutRolePolicy document -} - -func newFakeEnsurer() *fakeInstanceProfileEnsurer { - return &fakeInstanceProfileEnsurer{ - profiles: map[string]*iam.InstanceProfile{}, - roles: map[string]*iam.Role{}, - rolePolicies: map[string]string{}, - } -} - -func (f *fakeInstanceProfileEnsurer) GetRole(accountID string, in *iam.GetRoleInput) (*iam.GetRoleOutput, error) { - r, ok := f.roles[aws.StringValue(in.RoleName)] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetRoleOutput{Role: r}, nil -} - -func (f *fakeInstanceProfileEnsurer) CreateRole(accountID string, in *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { - f.createRoleCalls++ - f.lastTrustDoc = aws.StringValue(in.AssumeRolePolicyDocument) - name := aws.StringValue(in.RoleName) - if _, ok := f.roles[name]; ok { - return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) - } - r := &iam.Role{ - RoleName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + accountID + ":role/" + name), - } - f.roles[name] = r - return &iam.CreateRoleOutput{Role: r}, nil -} - -func (f *fakeInstanceProfileEnsurer) PutRolePolicy(_ string, in *iam.PutRolePolicyInput) (*iam.PutRolePolicyOutput, error) { - f.rolePolicies[aws.StringValue(in.RoleName)] = aws.StringValue(in.PolicyDocument) - return &iam.PutRolePolicyOutput{}, nil -} - -func (f *fakeInstanceProfileEnsurer) GetInstanceProfile(_ string, in *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { - p, ok := f.profiles[aws.StringValue(in.InstanceProfileName)] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetInstanceProfileOutput{InstanceProfile: p}, nil -} - -func (f *fakeInstanceProfileEnsurer) CreateInstanceProfile(accountID string, in *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { - f.createCalls++ - if f.createErr != nil { - return nil, f.createErr - } - name := aws.StringValue(in.InstanceProfileName) - if _, ok := f.profiles[name]; ok { - return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) - } - p := &iam.InstanceProfile{ - InstanceProfileName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + accountID + ":instance-profile/" + name), - } - f.profiles[name] = p - return &iam.CreateInstanceProfileOutput{InstanceProfile: p}, nil -} - -func (f *fakeInstanceProfileEnsurer) AddRoleToInstanceProfile(_ string, in *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { - f.addCalls++ - if f.addRoleErr != nil { - return nil, f.addRoleErr - } - name := aws.StringValue(in.InstanceProfileName) - p, ok := f.profiles[name] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - if len(p.Roles) > 0 { - return nil, errors.New(awserrors.ErrorIAMLimitExceeded) - } - p.Roles = []*iam.Role{{RoleName: in.RoleName}} - return &iam.AddRoleToInstanceProfileOutput{}, nil +// newFakeEnsurer returns the shared SystemInstanceRoleEnsurer mock, used +// across this package's tests wherever an EKSServiceDeps.IAM is needed. +func newFakeEnsurer() *iammock.SystemInstanceRoleEnsurer { + return iammock.New() } const testNodeRoleARN = "arn:aws:iam::000000000001:role/eks-quickstart-node-role" @@ -114,14 +27,14 @@ func TestEnsureNodeInstanceProfile_CreatesAndAttaches(t *testing.T) { arn, err := s.ensureNodeInstanceProfile("000000000001", testNodeRoleARN) require.NoError(t, err) assert.Equal(t, "arn:aws:iam::000000000001:instance-profile/eks-quickstart-node-role", arn) - assert.Equal(t, 1, f.createCalls) - assert.Equal(t, 1, f.addCalls) - assert.Len(t, f.profiles["eks-quickstart-node-role"].Roles, 1) + assert.Equal(t, 1, f.CreateInstanceProfileCalls) + assert.Equal(t, 1, f.AddRoleToInstanceProfileCalls) + assert.Len(t, f.Profiles["eks-quickstart-node-role"].Roles, 1) } func TestEnsureNodeInstanceProfile_ExistingWithRoleIsNoop(t *testing.T) { f := newFakeEnsurer() - f.profiles["eks-quickstart-node-role"] = &iam.InstanceProfile{ + f.Profiles["eks-quickstart-node-role"] = &iam.InstanceProfile{ InstanceProfileName: aws.String("eks-quickstart-node-role"), Arn: aws.String("arn:aws:iam::000000000001:instance-profile/eks-quickstart-node-role"), Roles: []*iam.Role{{RoleName: aws.String("eks-quickstart-node-role")}}, @@ -131,13 +44,13 @@ func TestEnsureNodeInstanceProfile_ExistingWithRoleIsNoop(t *testing.T) { arn, err := s.ensureNodeInstanceProfile("000000000001", testNodeRoleARN) require.NoError(t, err) assert.Equal(t, "arn:aws:iam::000000000001:instance-profile/eks-quickstart-node-role", arn) - assert.Zero(t, f.createCalls) - assert.Zero(t, f.addCalls) + assert.Zero(t, f.CreateInstanceProfileCalls) + assert.Zero(t, f.AddRoleToInstanceProfileCalls) } func TestEnsureNodeInstanceProfile_ExistingWithoutRoleAttaches(t *testing.T) { f := newFakeEnsurer() - f.profiles["eks-quickstart-node-role"] = &iam.InstanceProfile{ + f.Profiles["eks-quickstart-node-role"] = &iam.InstanceProfile{ InstanceProfileName: aws.String("eks-quickstart-node-role"), Arn: aws.String("arn:aws:iam::000000000001:instance-profile/eks-quickstart-node-role"), } @@ -145,13 +58,13 @@ func TestEnsureNodeInstanceProfile_ExistingWithoutRoleAttaches(t *testing.T) { _, err := s.ensureNodeInstanceProfile("000000000001", testNodeRoleARN) require.NoError(t, err) - assert.Zero(t, f.createCalls) - assert.Equal(t, 1, f.addCalls) + assert.Zero(t, f.CreateInstanceProfileCalls) + assert.Equal(t, 1, f.AddRoleToInstanceProfileCalls) } func TestEnsureNodeInstanceProfile_AddRoleLimitExceededIsSuccess(t *testing.T) { f := newFakeEnsurer() - f.addRoleErr = errors.New(awserrors.ErrorIAMLimitExceeded) + f.AddRoleToInstanceProfileErr = errors.New(awserrors.ErrorIAMLimitExceeded) s := &EKSServiceImpl{deps: EKSServiceDeps{IAM: f}} arn, err := s.ensureNodeInstanceProfile("000000000001", testNodeRoleARN) diff --git a/spinifex/handlers/elbv2/lb_role_test.go b/spinifex/handlers/elbv2/lb_role_test.go index beb0964e0..5ecb6d3b3 100644 --- a/spinifex/handlers/elbv2/lb_role_test.go +++ b/spinifex/handlers/elbv2/lb_role_test.go @@ -1,66 +1,19 @@ package handlers_elbv2 import ( - "errors" "testing" - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/stretchr/testify/assert" ) -// fakeLBEnsurer is a minimal SystemInstanceRoleEnsurer recording the account the -// role and instance profile are created under. GetRole/GetInstanceProfile always -// miss so EnsureSystemInstanceProfile takes the create path. -type fakeLBEnsurer struct { - roleAcct string - profileAcct string -} - -func (f *fakeLBEnsurer) GetRole(_ string, _ *iam.GetRoleInput) (*iam.GetRoleOutput, error) { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) -} - -func (f *fakeLBEnsurer) CreateRole(acct string, in *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { - f.roleAcct = acct - name := aws.StringValue(in.RoleName) - return &iam.CreateRoleOutput{Role: &iam.Role{ - RoleName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + acct + ":role/" + name), - }}, nil -} - -func (f *fakeLBEnsurer) PutRolePolicy(_ string, _ *iam.PutRolePolicyInput) (*iam.PutRolePolicyOutput, error) { - return &iam.PutRolePolicyOutput{}, nil -} - -func (f *fakeLBEnsurer) GetInstanceProfile(_ string, _ *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) -} - -func (f *fakeLBEnsurer) CreateInstanceProfile(acct string, in *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { - f.profileAcct = acct - name := aws.StringValue(in.InstanceProfileName) - return &iam.CreateInstanceProfileOutput{InstanceProfile: &iam.InstanceProfile{ - InstanceProfileName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + acct + ":instance-profile/" + name), - }}, nil -} - -func (f *fakeLBEnsurer) AddRoleToInstanceProfile(_ string, _ *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { - return &iam.AddRoleToInstanceProfileOutput{}, nil -} - -var _ handlers_iam.SystemInstanceRoleEnsurer = (*fakeLBEnsurer)(nil) - // TestEnsureLBInstanceProfile_UsesLazyProvider asserts the LB profile resolves via // the lazy IAMProvider when the eager IAM field is unset — the daemon wires only // the provider so the IAM build cannot race the NATS KV backend at startup. func TestEnsureLBInstanceProfile_UsesLazyProvider(t *testing.T) { - f := &fakeLBEnsurer{} + f := iammock.New() s := &ELBv2ServiceImpl{IAMProvider: func() handlers_iam.SystemInstanceRoleEnsurer { return f }} arn := s.ensureLBInstanceProfile(utils.GlobalAccountID) @@ -72,12 +25,12 @@ func TestEnsureLBInstanceProfile_UsesLazyProvider(t *testing.T) { // resolves the profile under the instance's account, so a role created in the LB // owner account would be invisible to it and the lb-agent would get no creds. func TestEnsureLBInstanceProfile_CreatesInSystemAccount(t *testing.T) { - f := &fakeLBEnsurer{} + f := iammock.New() s := &ELBv2ServiceImpl{IAM: f} s.ensureLBInstanceProfile(utils.GlobalAccountID) - assert.Equal(t, utils.GlobalAccountID, f.roleAcct) - assert.Equal(t, utils.GlobalAccountID, f.profileAcct) + assert.Equal(t, utils.GlobalAccountID, f.LastRoleAcct) + assert.Equal(t, utils.GlobalAccountID, f.LastProfileAcct) } // TestEnsureLBInstanceProfile_NotReadyFallsBack asserts an unwired IAM (no field, diff --git a/spinifex/handlers/iam/mock/mock.go b/spinifex/handlers/iam/mock/mock.go new file mode 100644 index 000000000..93f1c0fab --- /dev/null +++ b/spinifex/handlers/iam/mock/mock.go @@ -0,0 +1,145 @@ +// Package mock provides an in-memory implementation of +// handlers_iam.SystemInstanceRoleEnsurer for tests. +package mock + +import ( + "errors" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" +) + +// SystemInstanceRoleEnsurer is an in-memory +// handlers_iam.SystemInstanceRoleEnsurer. Roles and instance profiles are +// stored keyed by name; Get* miss with NoSuchEntity until the matching +// Create* call, and a repeat Create* reports EntityAlreadyExists, mirroring +// the live IAM API. +type SystemInstanceRoleEnsurer struct { + Roles map[string]*iam.Role + Profiles map[string]*iam.InstanceProfile + + // RolePolicies holds the most recent PutRolePolicy document per role + // name; PolicyCalls logs every call in order for tests asserting a + // policy is re-asserted on every launch. + RolePolicies map[string]string + PolicyCalls []iam.PutRolePolicyInput + + // Call counters. + CreateRoleCalls int + CreateInstanceProfileCalls int + AddRoleToInstanceProfileCalls int + + // LastTrustDoc captures the AssumeRolePolicyDocument of the most recent + // CreateRole call. + LastTrustDoc string + // LastRoleAcct / LastProfileAcct capture the accountID of the most + // recent CreateRole / CreateInstanceProfile call. + LastRoleAcct string + LastProfileAcct string + + // PutRolePolicyErr, when set, is returned by PutRolePolicy after the + // call is recorded, letting tests inject a mid-flow failure. + PutRolePolicyErr error + // CreateInstanceProfileErr, when set, is returned by + // CreateInstanceProfile instead of creating a row. + CreateInstanceProfileErr error + // AddRoleToInstanceProfileErr, when set, is returned by + // AddRoleToInstanceProfile instead of attaching. + AddRoleToInstanceProfileErr error + // EmptyInstanceProfileARN makes CreateInstanceProfile return an empty + // ARN, for exercising callers that must reject it. + EmptyInstanceProfileARN bool +} + +var _ handlers_iam.SystemInstanceRoleEnsurer = (*SystemInstanceRoleEnsurer)(nil) + +// New returns an empty SystemInstanceRoleEnsurer ready for use. +func New() *SystemInstanceRoleEnsurer { + return &SystemInstanceRoleEnsurer{ + Roles: make(map[string]*iam.Role), + Profiles: make(map[string]*iam.InstanceProfile), + RolePolicies: make(map[string]string), + } +} + +func (e *SystemInstanceRoleEnsurer) GetRole(_ string, in *iam.GetRoleInput) (*iam.GetRoleOutput, error) { + r, ok := e.Roles[aws.StringValue(in.RoleName)] + if !ok { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + return &iam.GetRoleOutput{Role: r}, nil +} + +func (e *SystemInstanceRoleEnsurer) CreateRole(accountID string, in *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { + e.CreateRoleCalls++ + e.LastTrustDoc = aws.StringValue(in.AssumeRolePolicyDocument) + e.LastRoleAcct = accountID + name := aws.StringValue(in.RoleName) + if _, ok := e.Roles[name]; ok { + return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) + } + r := &iam.Role{ + RoleName: aws.String(name), + Arn: aws.String("arn:aws:iam::" + accountID + ":role/" + name), + } + e.Roles[name] = r + return &iam.CreateRoleOutput{Role: r}, nil +} + +func (e *SystemInstanceRoleEnsurer) PutRolePolicy(_ string, in *iam.PutRolePolicyInput) (*iam.PutRolePolicyOutput, error) { + e.PolicyCalls = append(e.PolicyCalls, *in) + e.RolePolicies[aws.StringValue(in.RoleName)] = aws.StringValue(in.PolicyDocument) + if e.PutRolePolicyErr != nil { + return nil, e.PutRolePolicyErr + } + return &iam.PutRolePolicyOutput{}, nil +} + +func (e *SystemInstanceRoleEnsurer) GetInstanceProfile(_ string, in *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { + p, ok := e.Profiles[aws.StringValue(in.InstanceProfileName)] + if !ok { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + return &iam.GetInstanceProfileOutput{InstanceProfile: p}, nil +} + +func (e *SystemInstanceRoleEnsurer) CreateInstanceProfile(accountID string, in *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { + e.CreateInstanceProfileCalls++ + e.LastProfileAcct = accountID + if e.CreateInstanceProfileErr != nil { + return nil, e.CreateInstanceProfileErr + } + name := aws.StringValue(in.InstanceProfileName) + if _, ok := e.Profiles[name]; ok { + return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) + } + arn := "arn:aws:iam::" + accountID + ":instance-profile/" + name + if e.EmptyInstanceProfileARN { + arn = "" + } + p := &iam.InstanceProfile{ + InstanceProfileName: aws.String(name), + Arn: aws.String(arn), + } + e.Profiles[name] = p + return &iam.CreateInstanceProfileOutput{InstanceProfile: p}, nil +} + +func (e *SystemInstanceRoleEnsurer) AddRoleToInstanceProfile(_ string, in *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { + e.AddRoleToInstanceProfileCalls++ + if e.AddRoleToInstanceProfileErr != nil { + return nil, e.AddRoleToInstanceProfileErr + } + name := aws.StringValue(in.InstanceProfileName) + p, ok := e.Profiles[name] + if !ok { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + if len(p.Roles) > 0 { + return nil, errors.New(awserrors.ErrorIAMLimitExceeded) + } + p.Roles = []*iam.Role{{RoleName: in.RoleName}} + return &iam.AddRoleToInstanceProfileOutput{}, nil +} diff --git a/spinifex/handlers/iam/mock/mock_test.go b/spinifex/handlers/iam/mock/mock_test.go new file mode 100644 index 000000000..e91cdc8c1 --- /dev/null +++ b/spinifex/handlers/iam/mock/mock_test.go @@ -0,0 +1,85 @@ +package mock_test + +import ( + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" +) + +func TestGetRole_MissingReturnsNoSuchEntity(t *testing.T) { + e := mock.New() + _, err := e.GetRole("acct", &iam.GetRoleInput{RoleName: aws.String("r1")}) + if err == nil || err.Error() != awserrors.ErrorIAMNoSuchEntity { + t.Fatalf("err = %v, want ErrorIAMNoSuchEntity", err) + } +} + +func TestCreateRole_DuplicateReturnsEntityAlreadyExists(t *testing.T) { + e := mock.New() + in := &iam.CreateRoleInput{RoleName: aws.String("r1"), AssumeRolePolicyDocument: aws.String("doc")} + if _, err := e.CreateRole("acct", in); err != nil { + t.Fatalf("first CreateRole: %v", err) + } + if _, err := e.CreateRole("acct", in); err == nil || err.Error() != awserrors.ErrorIAMEntityAlreadyExists { + t.Fatalf("second CreateRole err = %v, want EntityAlreadyExists", err) + } + if e.CreateRoleCalls != 2 { + t.Fatalf("CreateRoleCalls = %d, want 2", e.CreateRoleCalls) + } + if e.LastTrustDoc != "doc" { + t.Fatalf("LastTrustDoc = %q, want %q", e.LastTrustDoc, "doc") + } +} + +// AddRoleToInstanceProfile must report LimitExceeded on a second attach, +// mirroring the live API rejecting a second role on one profile. +func TestAddRoleToInstanceProfile_SecondAttachIsLimitExceeded(t *testing.T) { + e := mock.New() + if _, err := e.CreateInstanceProfile("acct", &iam.CreateInstanceProfileInput{ + InstanceProfileName: aws.String("p1"), + }); err != nil { + t.Fatalf("CreateInstanceProfile: %v", err) + } + in := &iam.AddRoleToInstanceProfileInput{InstanceProfileName: aws.String("p1"), RoleName: aws.String("r1")} + if _, err := e.AddRoleToInstanceProfile("acct", in); err != nil { + t.Fatalf("first AddRoleToInstanceProfile: %v", err) + } + if _, err := e.AddRoleToInstanceProfile("acct", in); err == nil || err.Error() != awserrors.ErrorIAMLimitExceeded { + t.Fatalf("second AddRoleToInstanceProfile err = %v, want LimitExceeded", err) + } +} + +func TestCreateInstanceProfile_EmptyARNOverride(t *testing.T) { + e := mock.New() + e.EmptyInstanceProfileARN = true + out, err := e.CreateInstanceProfile("acct", &iam.CreateInstanceProfileInput{ + InstanceProfileName: aws.String("p1"), + }) + if err != nil { + t.Fatalf("CreateInstanceProfile: %v", err) + } + if aws.StringValue(out.InstanceProfile.Arn) != "" { + t.Fatalf("Arn = %q, want empty", aws.StringValue(out.InstanceProfile.Arn)) + } +} + +func TestPutRolePolicy_InjectedErrorStillRecordsCall(t *testing.T) { + e := mock.New() + injected := errors.New("boom") + e.PutRolePolicyErr = injected + in := &iam.PutRolePolicyInput{RoleName: aws.String("r1"), PolicyName: aws.String("p"), PolicyDocument: aws.String("doc")} + _, err := e.PutRolePolicy("acct", in) + if !errors.Is(err, injected) { + t.Fatalf("err = %v, want injected error", err) + } + if len(e.PolicyCalls) != 1 { + t.Fatalf("PolicyCalls len = %d, want 1", len(e.PolicyCalls)) + } + if e.RolePolicies["r1"] != "doc" { + t.Fatalf("RolePolicies[r1] = %q, want %q", e.RolePolicies["r1"], "doc") + } +} diff --git a/spinifex/handlers/iam/system_role_test.go b/spinifex/handlers/iam/system_role_test.go index d844d7fe8..8485b8f94 100644 --- a/spinifex/handlers/iam/system_role_test.go +++ b/spinifex/handlers/iam/system_role_test.go @@ -1,93 +1,17 @@ -package handlers_iam +// package handlers_iam_test, not handlers_iam: an in-package test importing +// the shared mock package would create an import cycle, since the mock +// package itself imports handlers_iam for the interface compile check. +package handlers_iam_test import ( - "errors" "testing" - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// fakeSystemRoleEnsurer is an in-memory SystemInstanceRoleEnsurer that records -// calls and serves role/profile stores keyed by name. -type fakeSystemRoleEnsurer struct { - roles map[string]*iam.Role - profiles map[string]*iam.InstanceProfile - policies map[string]string - createRoleCalls int - createProfCalls int - addRoleCalls int - lastTrustDoc string -} - -func newFakeSystemRoleEnsurer() *fakeSystemRoleEnsurer { - return &fakeSystemRoleEnsurer{ - roles: map[string]*iam.Role{}, - profiles: map[string]*iam.InstanceProfile{}, - policies: map[string]string{}, - } -} - -func (f *fakeSystemRoleEnsurer) GetRole(_ string, in *iam.GetRoleInput) (*iam.GetRoleOutput, error) { - r, ok := f.roles[aws.StringValue(in.RoleName)] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetRoleOutput{Role: r}, nil -} - -func (f *fakeSystemRoleEnsurer) CreateRole(acct string, in *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { - f.createRoleCalls++ - f.lastTrustDoc = aws.StringValue(in.AssumeRolePolicyDocument) - name := aws.StringValue(in.RoleName) - if _, ok := f.roles[name]; ok { - return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) - } - r := &iam.Role{RoleName: aws.String(name), Arn: aws.String("arn:aws:iam::" + acct + ":role/" + name)} - f.roles[name] = r - return &iam.CreateRoleOutput{Role: r}, nil -} - -func (f *fakeSystemRoleEnsurer) PutRolePolicy(_ string, in *iam.PutRolePolicyInput) (*iam.PutRolePolicyOutput, error) { - f.policies[aws.StringValue(in.RoleName)] = aws.StringValue(in.PolicyDocument) - return &iam.PutRolePolicyOutput{}, nil -} - -func (f *fakeSystemRoleEnsurer) GetInstanceProfile(_ string, in *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { - p, ok := f.profiles[aws.StringValue(in.InstanceProfileName)] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetInstanceProfileOutput{InstanceProfile: p}, nil -} - -func (f *fakeSystemRoleEnsurer) CreateInstanceProfile(acct string, in *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { - f.createProfCalls++ - name := aws.StringValue(in.InstanceProfileName) - if _, ok := f.profiles[name]; ok { - return nil, errors.New(awserrors.ErrorIAMEntityAlreadyExists) - } - p := &iam.InstanceProfile{ - InstanceProfileName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + acct + ":instance-profile/" + name), - } - f.profiles[name] = p - return &iam.CreateInstanceProfileOutput{InstanceProfile: p}, nil -} - -func (f *fakeSystemRoleEnsurer) AddRoleToInstanceProfile(_ string, in *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { - f.addRoleCalls++ - p, ok := f.profiles[aws.StringValue(in.InstanceProfileName)] - if !ok { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - p.Roles = []*iam.Role{{RoleName: in.RoleName}} - return &iam.AddRoleToInstanceProfileOutput{}, nil -} - const ( testRoleAcct = "000000000007" testRoleName = "spinifex-system-role" @@ -98,32 +22,30 @@ const ( // TestEnsureSystemInstanceProfile_CreatesAll asserts the role (EC2 trust), inline // policy, and instance profile are all created and the profile ARN is returned. func TestEnsureSystemInstanceProfile_CreatesAll(t *testing.T) { - f := newFakeSystemRoleEnsurer() - arn, err := EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + f := iammock.New() + arn, err := handlers_iam.EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) require.NoError(t, err) assert.Equal(t, "arn:aws:iam::"+testRoleAcct+":instance-profile/"+testRoleName, arn) - assert.Equal(t, 1, f.createRoleCalls) - assert.Equal(t, 1, f.createProfCalls) - assert.Equal(t, 1, f.addRoleCalls) - assert.Contains(t, f.lastTrustDoc, "ec2.amazonaws.com") - assert.Equal(t, testPolicyDoc, f.policies[testRoleName]) - require.NotNil(t, f.profiles[testRoleName]) - assert.Len(t, f.profiles[testRoleName].Roles, 1) + assert.Equal(t, 1, f.CreateRoleCalls) + assert.Equal(t, 1, f.CreateInstanceProfileCalls) + assert.Equal(t, 1, f.AddRoleToInstanceProfileCalls) + assert.Contains(t, f.LastTrustDoc, "ec2.amazonaws.com") + assert.Equal(t, testPolicyDoc, f.RolePolicies[testRoleName]) + require.NotNil(t, f.Profiles[testRoleName]) + assert.Len(t, f.Profiles[testRoleName].Roles, 1) } // TestEnsureSystemInstanceProfile_Idempotent asserts a re-run against existing // role+profile creates nothing new but still re-asserts the inline policy. func TestEnsureSystemInstanceProfile_Idempotent(t *testing.T) { - f := newFakeSystemRoleEnsurer() - _, err := EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + f := iammock.New() + _, err := handlers_iam.EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) require.NoError(t, err) - _, err = EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) + _, err = handlers_iam.EnsureSystemInstanceProfile(f, testRoleAcct, testRoleName, testPolicyName, testPolicyDoc) require.NoError(t, err) - assert.Equal(t, 1, f.createRoleCalls, "role created once") - assert.Equal(t, 1, f.createProfCalls, "profile created once") - assert.Equal(t, 1, f.addRoleCalls, "role attached once") + assert.Equal(t, 1, f.CreateRoleCalls, "role created once") + assert.Equal(t, 1, f.CreateInstanceProfileCalls, "profile created once") + assert.Equal(t, 1, f.AddRoleToInstanceProfileCalls, "role attached once") } - -var _ SystemInstanceRoleEnsurer = (*fakeSystemRoleEnsurer)(nil) diff --git a/spinifex/handlers/rds/create_test.go b/spinifex/handlers/rds/create_test.go index 3f71b3545..9b6319502 100644 --- a/spinifex/handlers/rds/create_test.go +++ b/spinifex/handlers/rds/create_test.go @@ -14,6 +14,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" @@ -122,7 +123,7 @@ type createHarness struct { svc *Service launch *launchHarness network *fakeNetwork - iam *fakeRDSEnsurer + iam *iammock.SystemInstanceRoleEnsurer nc *nats.Conn // dnsChanges collects what the endpoint publish put on the bus. @@ -136,7 +137,7 @@ func newCreateHarness(t *testing.T, baseDomain string) *createHarness { h := &createHarness{ launch: newLaunchHarness(), network: newFakeNetwork(), - iam: &fakeRDSEnsurer{}, + iam: iammock.New(), nc: nc, dnsChanges: make(chan handlers_dns.ChangeBatch, 4), } @@ -285,13 +286,13 @@ func TestCreateDBInstance_ProvisionsAndRecordsTheInstance(t *testing.T) { assert.Equal(t, int64(firstVMGeneration), entry.VMGeneration) require.NotNil(t, h.launch.launcher.input) - assert.Equal(t, h.iam.profileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) + assert.Equal(t, rdsInstanceProfileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) } func TestCreateDBInstance_IAMFailurePrecedesReservationAndLaunch(t *testing.T) { h := newCreateHarness(t, "") iamErr := errors.New("IAM store unavailable") - h.iam.policyErr = iamErr + h.iam.PutRolePolicyErr = iamErr _, err := h.svc.CreateDBInstance(t.Context(), validCreateInput(), testAccountID) diff --git a/spinifex/handlers/rds/iam_test.go b/spinifex/handlers/rds/iam_test.go index 358fd106c..3f91d35ec 100644 --- a/spinifex/handlers/rds/iam_test.go +++ b/spinifex/handlers/rds/iam_test.go @@ -7,94 +7,23 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// fakeRDSEnsurer records every call so both the grant and its idempotency can be -// asserted. The role and profile exist from the second call onward, which is the -// state a re-launch finds. -type fakeRDSEnsurer struct { - roleAcct string - profileAcct string - trust string - policies []iam.PutRolePolicyInput - roleCreates int - roleExists bool - profileNames []string - policyErr error - emptyProfileARN bool -} - -func (f *fakeRDSEnsurer) GetRole(_ string, _ *iam.GetRoleInput) (*iam.GetRoleOutput, error) { - if !f.roleExists { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetRoleOutput{Role: &iam.Role{RoleName: aws.String(InstanceRoleName)}}, nil -} - -func (f *fakeRDSEnsurer) CreateRole(acct string, in *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) { - f.roleAcct = acct - f.roleCreates++ - f.roleExists = true - f.trust = aws.StringValue(in.AssumeRolePolicyDocument) - name := aws.StringValue(in.RoleName) - return &iam.CreateRoleOutput{Role: &iam.Role{ - RoleName: aws.String(name), - Arn: aws.String("arn:aws:iam::" + acct + ":role/" + name), - }}, nil -} - -func (f *fakeRDSEnsurer) PutRolePolicy(_ string, in *iam.PutRolePolicyInput) (*iam.PutRolePolicyOutput, error) { - f.policies = append(f.policies, *in) - if f.policyErr != nil { - return nil, f.policyErr - } - return &iam.PutRolePolicyOutput{}, nil -} - -func (f *fakeRDSEnsurer) GetInstanceProfile(_ string, _ *iam.GetInstanceProfileInput) (*iam.GetInstanceProfileOutput, error) { - if len(f.profileNames) == 0 { - return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) - } - return &iam.GetInstanceProfileOutput{InstanceProfile: &iam.InstanceProfile{ - InstanceProfileName: aws.String(InstanceRoleName), - Arn: aws.String(f.profileARN(f.profileAcct)), - Roles: []*iam.Role{{RoleName: aws.String(InstanceRoleName)}}, - }}, nil -} - -func (f *fakeRDSEnsurer) CreateInstanceProfile(acct string, in *iam.CreateInstanceProfileInput) (*iam.CreateInstanceProfileOutput, error) { - f.profileAcct = acct - name := aws.StringValue(in.InstanceProfileName) - f.profileNames = append(f.profileNames, name) - return &iam.CreateInstanceProfileOutput{InstanceProfile: &iam.InstanceProfile{ - InstanceProfileName: aws.String(name), - Arn: aws.String(f.profileARN(acct)), - }}, nil -} - -func (f *fakeRDSEnsurer) profileARN(accountID string) string { - if f.emptyProfileARN { - return "" - } - return "arn:aws:iam::" + accountID + ":instance-profile/" + InstanceRoleName -} - -func (f *fakeRDSEnsurer) AddRoleToInstanceProfile(_ string, _ *iam.AddRoleToInstanceProfileInput) (*iam.AddRoleToInstanceProfileOutput, error) { - return &iam.AddRoleToInstanceProfileOutput{}, nil -} - -var _ handlers_iam.SystemInstanceRoleEnsurer = (*fakeRDSEnsurer)(nil) - -func testIAMProvider(f *fakeRDSEnsurer) IAMProvider { +func testIAMProvider(f *iammock.SystemInstanceRoleEnsurer) IAMProvider { return func() handlers_iam.SystemInstanceRoleEnsurer { return f } } +// rdsInstanceProfileARN mirrors the ARN ensureInstanceProfile computes, for +// assertions comparing against a launch input rather than the mock directly. +func rdsInstanceProfileARN(accountID string) string { + return "arn:aws:iam::" + accountID + ":instance-profile/" + InstanceRoleName +} + // A Postgres RCE on a DB VM inherits this role, so the grant must be the four // internal actions and nothing wildcarded that could reach the customer surface. func TestInstanceRolePolicy_GrantsOnlyTheInternalActions(t *testing.T) { @@ -123,21 +52,21 @@ func TestInstanceRolePolicy_GrantsOnlyTheInternalActions(t *testing.T) { // under the VM's own account, so a role in the customer account would be // invisible to the agent and it would get no credentials at all. func TestEnsureInstanceProfile_CreatesInSystemAccount(t *testing.T) { - f := &fakeRDSEnsurer{} + f := iammock.New() arn, err := ensureInstanceProfile(func() handlers_iam.SystemInstanceRoleEnsurer { return f }, utils.GlobalAccountID) require.NoError(t, err) assert.Equal(t, "arn:aws:iam::"+utils.GlobalAccountID+":instance-profile/"+InstanceRoleName, arn) - assert.Equal(t, utils.GlobalAccountID, f.roleAcct) - assert.Equal(t, utils.GlobalAccountID, f.profileAcct) - assert.Equal(t, handlers_iam.EC2InstanceTrustPolicy, f.trust, + assert.Equal(t, utils.GlobalAccountID, f.LastRoleAcct) + assert.Equal(t, utils.GlobalAccountID, f.LastProfileAcct) + assert.Equal(t, handlers_iam.EC2InstanceTrustPolicy, f.LastTrustDoc, "AssumeRoleForInstance only mints credentials for the EC2 trust document") } // Every launch calls this, so a second call must converge on the existing role // rather than create a second one, and must re-assert the same grant. func TestEnsureInstanceProfile_IsIdempotent(t *testing.T) { - f := &fakeRDSEnsurer{} + f := iammock.New() provider := func() handlers_iam.SystemInstanceRoleEnsurer { return f } first, err := ensureInstanceProfile(provider, utils.GlobalAccountID) @@ -146,10 +75,10 @@ func TestEnsureInstanceProfile_IsIdempotent(t *testing.T) { require.NoError(t, err) assert.Equal(t, first, second) - assert.Equal(t, 1, f.roleCreates, "the second launch must find the role, not create it") - assert.Len(t, f.profileNames, 1, "the second launch must find the instance profile") - require.Len(t, f.policies, 2, "the grant is re-asserted on every launch") - for _, put := range f.policies { + assert.Equal(t, 1, f.CreateRoleCalls, "the second launch must find the role, not create it") + assert.Equal(t, 1, f.CreateInstanceProfileCalls, "the second launch must find the instance profile") + require.Len(t, f.PolicyCalls, 2, "the grant is re-asserted on every launch") + for _, put := range f.PolicyCalls { assert.Equal(t, instanceRoleInlinePolicyName, aws.StringValue(put.PolicyName)) assert.Equal(t, instanceRoleInlinePolicy, aws.StringValue(put.PolicyDocument)) } @@ -174,7 +103,8 @@ func TestEnsureInstanceProfile_RejectsUnavailableIAM(t *testing.T) { func TestEnsureInstanceProfile_PreservesEnsureFailure(t *testing.T) { ensureErr := errors.New("IAM storage unavailable") - f := &fakeRDSEnsurer{policyErr: ensureErr} + f := iammock.New() + f.PutRolePolicyErr = ensureErr arn, err := ensureInstanceProfile( func() handlers_iam.SystemInstanceRoleEnsurer { return f }, utils.GlobalAccountID) @@ -186,7 +116,8 @@ func TestEnsureInstanceProfile_PreservesEnsureFailure(t *testing.T) { } func TestEnsureInstanceProfile_RejectsEmptyARN(t *testing.T) { - f := &fakeRDSEnsurer{emptyProfileARN: true} + f := iammock.New() + f.EmptyInstanceProfileARN = true arn, err := ensureInstanceProfile( func() handlers_iam.SystemInstanceRoleEnsurer { return f }, utils.GlobalAccountID) diff --git a/spinifex/handlers/rds/modify_test.go b/spinifex/handlers/rds/modify_test.go index a0a35baeb..12f16ee6f 100644 --- a/spinifex/handlers/rds/modify_test.go +++ b/spinifex/handlers/rds/modify_test.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/spinifex/awserrors" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" @@ -30,7 +31,7 @@ type modifyHarness struct { nc *nats.Conn launch *launchHarness network *fakeNetwork - iam *fakeRDSEnsurer + iam *iammock.SystemInstanceRoleEnsurer cmdr *fakeInstanceCommander storage *fakeVolumeResizer vmState *fakeInstanceState @@ -52,7 +53,7 @@ func newModifyHarnessWithAgent(t *testing.T, agentFails bool) *modifyHarness { nc: nc, launch: newLaunchHarness(), network: newFakeNetwork(), - iam: &fakeRDSEnsurer{}, + iam: iammock.New(), cmdr: &fakeInstanceCommander{vm: vmState}, storage: newFakeVolumeResizer(testDataVolume, 20), vmState: vmState, diff --git a/spinifex/handlers/rds/replace_test.go b/spinifex/handlers/rds/replace_test.go index 0ed1fe83f..86738aa9d 100644 --- a/spinifex/handlers/rds/replace_test.go +++ b/spinifex/handlers/rds/replace_test.go @@ -77,7 +77,7 @@ func TestReplaceInstanceVM_IAMFailureLeavesTheCurrentVMServing(t *testing.T) { h := newModifyHarness(t) rec := seedReplaceable(t, h, modifiableRecord()) iamErr := errors.New("IAM store unavailable") - h.iam.policyErr = iamErr + h.iam.PutRolePolicyErr = iamErr err := h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{ GrowStorageToGiB: 80, @@ -338,5 +338,5 @@ func TestReplaceInstanceVM_LaunchesWithTheInstancesOwnIdentity(t *testing.T) { assert.Equal(t, testAccountID, h.launch.launcher.input.ExtraENIs[0].AccountID) assert.NotEqual(t, testAccountID, h.launch.launcher.input.AccountID, "the VM and its management NIC live in the system account") - assert.Equal(t, h.iam.profileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) + assert.Equal(t, rdsInstanceProfileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) } diff --git a/spinifex/handlers/rds/restore_test.go b/spinifex/handlers/rds/restore_test.go index 630bd5a42..11dfe52f8 100644 --- a/spinifex/handlers/rds/restore_test.go +++ b/spinifex/handlers/rds/restore_test.go @@ -71,14 +71,14 @@ func TestRestoreDBInstanceFromDBSnapshot_BuildsANewInstanceOnTheSnapshotsData(t assert.Empty(t, stored.Bootstrap.MasterUserPassword) require.NotNil(t, h.launch.launcher.input) - assert.Equal(t, h.iam.profileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) + assert.Equal(t, rdsInstanceProfileARN(utils.GlobalAccountID), h.launch.launcher.input.IamInstanceProfileArn) } func TestRestoreDBInstanceFromDBSnapshot_IAMFailurePrecedesReservationAndVolume(t *testing.T) { h := newSnapshotHarness(t, false) h.seedSnapshot(t) iamErr := errors.New("IAM store unavailable") - h.iam.policyErr = iamErr + h.iam.PutRolePolicyErr = iamErr _, err := h.svc.RestoreDBInstanceFromDBSnapshot(t.Context(), restoreInput(), testAccountID) diff --git a/spinifex/handlers/rds/snapshot_test.go b/spinifex/handlers/rds/snapshot_test.go index 44b6e0852..44409e300 100644 --- a/spinifex/handlers/rds/snapshot_test.go +++ b/spinifex/handlers/rds/snapshot_test.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" + iammock "github.com/mulgadc/spinifex/spinifex/handlers/iam/mock" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/nats-io/nats.go" @@ -24,7 +25,7 @@ type snapshotHarness struct { svc *Service launch *launchHarness network *fakeNetwork - iam *fakeRDSEnsurer + iam *iammock.SystemInstanceRoleEnsurer snaps *fakeSnapshots agent *stubAgent nc *nats.Conn @@ -37,7 +38,7 @@ func newSnapshotHarness(t *testing.T, agentFails bool) *snapshotHarness { h := &snapshotHarness{ launch: newLaunchHarness(), network: newFakeNetwork(), - iam: &fakeRDSEnsurer{}, + iam: iammock.New(), snaps: &fakeSnapshots{}, nc: nc, } diff --git a/spinifex/vm/mock/mock.go b/spinifex/vm/mock/mock.go new file mode 100644 index 000000000..16b7b494d --- /dev/null +++ b/spinifex/vm/mock/mock.go @@ -0,0 +1,264 @@ +// Package mock provides a single shared in-memory fake for vm.StateStore, +// reused by every package that stands up a stopped/terminated instance +// store in its tests. It also structurally satisfies the narrower +// handlers/ec2/instance.StoppedInstanceStore (see mock_test.go for the +// compile-time check, kept out of this file to avoid importing that +// package from here). +package mock + +import ( + "errors" + "maps" + "sync" + + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/nats-io/nats.go/jetstream" +) + +// StateStore is an in-memory vm.StateStore. Per-method error fields let a +// test drive a specific failure branch without a real KV backend; the +// Wrote*/Deleted*/Claimed* fields record every successful call, so a test +// can assert on call history independent of final state. Stopped is the +// claimable pool: Delete/Claim remove from it, but WriteStoppedInstance +// records only, it never puts a record back. +type StateStore struct { + mu sync.Mutex + + Saved map[string]map[string]*vm.VM + Stopped map[string]*vm.VM + Terminated map[string]*vm.VM + + WroteStopped map[string]*vm.VM + WroteTerminated map[string]*vm.VM + DeletedStopped []string + ClaimedStopped []string + + SaveRunningErr error + LoadStoppedErr error + WriteStoppedErr error + UpdateStoppedErr error + ListStoppedErr error + DeleteStoppedErr error + ClaimStoppedErr error + WriteTerminatedErr error + ListTerminatedErr error + + // DeleteFailFirst makes the first DeleteStoppedInstance call fail and the + // second succeed, exercising a caller's single retry. + DeleteFailFirst bool + DeleteAttempts int + + // ClaimAfterLoad simulates a winning ClaimStoppedInstance landing between + // a caller's Load and its later mutation: the record is removed right + // after LoadStoppedInstance returns it. + ClaimAfterLoad bool + ClaimAttempts int + + UpdateStoppedCalls int + + // OnSaveRunning fires after a successful SaveRunningState, outside the + // lock, so a test can observe a save without polling a racy shared map. + OnSaveRunning func() + + // OnWriteStoppedInstance fires after a successful WriteStoppedInstance, + // outside the lock, so a test can inject a concurrent mutation (e.g. a + // slot reclaim) at the same point production code would race it. + OnWriteStoppedInstance func(id string, v *vm.VM) +} + +// New returns a StateStore with all maps initialized. +func New() *StateStore { + return &StateStore{ + Saved: map[string]map[string]*vm.VM{}, + Stopped: map[string]*vm.VM{}, + Terminated: map[string]*vm.VM{}, + } +} + +var _ vm.StateStore = (*StateStore)(nil) + +func (s *StateStore) SaveRunningState(nodeID string, snap map[string]*vm.VM) error { + s.mu.Lock() + if s.SaveRunningErr != nil { + s.mu.Unlock() + return s.SaveRunningErr + } + cp := make(map[string]*vm.VM, len(snap)) + maps.Copy(cp, snap) + if s.Saved == nil { + s.Saved = map[string]map[string]*vm.VM{} + } + s.Saved[nodeID] = cp + hook := s.OnSaveRunning + s.mu.Unlock() + if hook != nil { + hook() + } + return nil +} + +func (s *StateStore) LoadRunningState(nodeID string) (map[string]*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + if v, ok := s.Saved[nodeID]; ok { + return v, nil + } + return map[string]*vm.VM{}, nil +} + +func (s *StateStore) WriteStoppedInstance(id string, v *vm.VM) error { + s.mu.Lock() + if s.WriteStoppedErr != nil { + s.mu.Unlock() + return s.WriteStoppedErr + } + // Records the write without repopulating Stopped: that map is the pool + // ClaimStoppedInstance draws from, and a rollback write must not make an + // already-claimed instance claimable a second time. + if s.WroteStopped == nil { + s.WroteStopped = map[string]*vm.VM{} + } + s.WroteStopped[id] = v + hook := s.OnWriteStoppedInstance + s.mu.Unlock() + if hook != nil { + hook(id, v) + } + return nil +} + +func (s *StateStore) LoadStoppedInstance(id string) (*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.LoadStoppedErr != nil { + return nil, s.LoadStoppedErr + } + v, ok := s.Stopped[id] + if !ok { + return nil, nil + } + if s.ClaimAfterLoad { + delete(s.Stopped, id) + s.ClaimedStopped = append(s.ClaimedStopped, id) + } + return v, nil +} + +// DeleteStoppedInstance mimics a transient-then-success retry (DeleteFailFirst) +// and a hard injected error (DeleteStoppedErr); the two knobs are mutually +// exclusive in practice, DeleteFailFirst is checked first. +func (s *StateStore) DeleteStoppedInstance(id string) error { + s.mu.Lock() + defer s.mu.Unlock() + s.DeleteAttempts++ + attempt := s.DeleteAttempts + if s.DeleteFailFirst && attempt == 1 { + return errors.New("simulated transient delete failure") + } + if s.DeleteStoppedErr != nil { + return s.DeleteStoppedErr + } + delete(s.Stopped, id) + s.DeletedStopped = append(s.DeletedStopped, id) + return nil +} + +func (s *StateStore) ListStoppedInstances() ([]*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.ListStoppedErr != nil { + return nil, s.ListStoppedErr + } + out := make([]*vm.VM, 0, len(s.Stopped)) + for _, v := range s.Stopped { + out = append(out, v) + } + return out, nil +} + +// ClaimStoppedInstance mimics the real atomic delete-as-claim: under the +// store lock, remove and return Stopped[id], or ClaimStoppedErr / +// vm.ErrStoppedInstanceClaimed if it is already gone. +func (s *StateStore) ClaimStoppedInstance(id string) (*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.ClaimAttempts++ + if s.ClaimStoppedErr != nil { + return nil, s.ClaimStoppedErr + } + v, ok := s.Stopped[id] + if !ok { + return nil, vm.ErrStoppedInstanceClaimed + } + delete(s.Stopped, id) + s.ClaimedStopped = append(s.ClaimedStopped, id) + return v, nil +} + +// UpdateStoppedInstance mimics the real CAS semantics: mutate runs under the +// store lock against the stored value, and a missing record returns +// jetstream.ErrKeyNotFound rather than resurrecting it. +func (s *StateStore) UpdateStoppedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.UpdateStoppedCalls++ + if s.UpdateStoppedErr != nil { + return nil, s.UpdateStoppedErr + } + v, ok := s.Stopped[id] + if !ok { + return nil, jetstream.ErrKeyNotFound + } + mutate(v) + return v, nil +} + +func (s *StateStore) WriteTerminatedInstance(id string, v *vm.VM) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.WriteTerminatedErr != nil { + return s.WriteTerminatedErr + } + if s.Terminated == nil { + s.Terminated = map[string]*vm.VM{} + } + s.Terminated[id] = v + if s.WroteTerminated == nil { + s.WroteTerminated = map[string]*vm.VM{} + } + s.WroteTerminated[id] = v + return nil +} + +func (s *StateStore) ListTerminatedInstances() ([]*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.ListTerminatedErr != nil { + return nil, s.ListTerminatedErr + } + out := make([]*vm.VM, 0, len(s.Terminated)) + for _, v := range s.Terminated { + out = append(out, v) + } + return out, nil +} + +func (s *StateStore) DeleteTerminatedInstance(id string) error { + s.mu.Lock() + defer s.mu.Unlock() + delete(s.Terminated, id) + return nil +} + +// UpdateTerminatedInstance mimics the real CAS semantics: mutate runs under +// the store lock against the stored value. +func (s *StateStore) UpdateTerminatedInstance(id string, mutate func(*vm.VM)) (*vm.VM, error) { + s.mu.Lock() + defer s.mu.Unlock() + v, ok := s.Terminated[id] + if !ok { + return nil, errors.New("terminated instance not found") + } + mutate(v) + return v, nil +} diff --git a/spinifex/vm/mock/mock_test.go b/spinifex/vm/mock/mock_test.go new file mode 100644 index 000000000..f86a4fd59 --- /dev/null +++ b/spinifex/vm/mock/mock_test.go @@ -0,0 +1,97 @@ +package mock_test + +import ( + "errors" + "testing" + + handlers_ec2_instance "github.com/mulgadc/spinifex/spinifex/handlers/ec2/instance" + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/mulgadc/spinifex/spinifex/vm/mock" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This check lives here rather than in mock.go so mock.go's own import set +// stays minimal (vm only): handlers/ec2/instance imports vm, and pulling it +// into mock.go would make its own internal tests importing this package a +// cycle. This external test package imports both, so it can assert the +// same thing without either production package importing the other. +var _ handlers_ec2_instance.StoppedInstanceStore = (*mock.StateStore)(nil) + +func TestSaveAndLoadRunningState_RoundTrips(t *testing.T) { + s := mock.New() + require.NoError(t, s.SaveRunningState("node-1", map[string]*vm.VM{"i-1": {ID: "i-1"}})) + + got, err := s.LoadRunningState("node-1") + require.NoError(t, err) + require.Contains(t, got, "i-1") + + empty, err := s.LoadRunningState("node-2") + require.NoError(t, err) + assert.Empty(t, empty) +} + +func TestClaimStoppedInstance_MissingReturnsErrStoppedInstanceClaimed(t *testing.T) { + s := mock.New() + _, err := s.ClaimStoppedInstance("i-missing") + assert.ErrorIs(t, err, vm.ErrStoppedInstanceClaimed) + assert.Equal(t, 1, s.ClaimAttempts) +} + +func TestClaimAfterLoad_RemovesEntryOnLoad(t *testing.T) { + s := mock.New() + s.Stopped["i-1"] = &vm.VM{ID: "i-1"} + s.ClaimAfterLoad = true + + got, err := s.LoadStoppedInstance("i-1") + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, []string{"i-1"}, s.ClaimedStopped) + + _, err = s.ClaimStoppedInstance("i-1") + assert.ErrorIs(t, err, vm.ErrStoppedInstanceClaimed) +} + +func TestDeleteStoppedInstance_FailFirstThenSucceeds(t *testing.T) { + s := mock.New() + s.Stopped["i-1"] = &vm.VM{ID: "i-1"} + s.DeleteFailFirst = true + + require.Error(t, s.DeleteStoppedInstance("i-1")) + require.NoError(t, s.DeleteStoppedInstance("i-1")) + assert.Equal(t, 2, s.DeleteAttempts) + assert.Contains(t, s.DeletedStopped, "i-1") +} + +func TestUpdateStoppedInstance_MissingReturnsErrKeyNotFound(t *testing.T) { + s := mock.New() + _, err := s.UpdateStoppedInstance("i-missing", func(*vm.VM) {}) + assert.ErrorIs(t, err, jetstream.ErrKeyNotFound) + assert.Equal(t, 1, s.UpdateStoppedCalls) +} + +func TestWriteStoppedInstance_ErrInjectionAndHook(t *testing.T) { + s := mock.New() + var hookCalls int + s.OnWriteStoppedInstance = func(id string, v *vm.VM) { hookCalls++ } + + require.NoError(t, s.WriteStoppedInstance("i-1", &vm.VM{ID: "i-1"})) + assert.Equal(t, 1, hookCalls) + assert.NotNil(t, s.WroteStopped["i-1"]) + // A write records only; it must not add to the claimable pool, or a + // rollback would let a second caller claim the same instance. + assert.Nil(t, s.Stopped["i-1"]) + _, err := s.ClaimStoppedInstance("i-1") + assert.ErrorIs(t, err, vm.ErrStoppedInstanceClaimed) + + s.WriteStoppedErr = errors.New("kv down") + err = s.WriteStoppedInstance("i-2", &vm.VM{ID: "i-2"}) + assert.EqualError(t, err, "kv down") +} + +func TestUpdateTerminatedInstance_MissingErrors(t *testing.T) { + s := mock.New() + _, err := s.UpdateTerminatedInstance("i-missing", func(*vm.VM) {}) + require.Error(t, err) +}