From 09c897db6a30fb56e79ce97ad54f971e041d7eaa Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Mon, 11 May 2026 10:01:40 +1000 Subject: [PATCH] refactor(daemon): extract ModifyInstanceAttribute to service (1b-pre phase 2a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse handleEC2ModifyInstanceAttribute to a thin Pattern A wrapper over InstanceServiceImpl.ModifyInstanceAttribute, mirroring the Describe extraction in 719ac63 (phase 1). The InstanceService interface and StoppedInstanceStore each gain one method (ModifyInstanceAttribute, WriteStoppedInstance); the daemon's JetStreamManager already implements WriteStoppedInstance, so no production code outside the daemon handler changes. Logic-level tests move from daemon/daemon_handlers_test.go to handlers/ec2/instance/service_impl_test.go (12 cases against an in-memory fakeStoppedStore). The daemon keeps one NATS round-trip wrapper smoke test. The error-injection cases in daemon_handlers_instance_test.go remain in place but now require daemonWithFakeStateStore to rewire d.instanceService with the fake store — the previous `d.stateStore = store` swap no longer reaches the new code path. No behaviour change: SourceDestCheck stays a state-agnostic no-op, the ownership/visibility check still gates after the stopped-state gate, and the success response payload remains `{}`. --- spinifex/daemon/daemon_handlers_instance.go | 105 +------ .../daemon/daemon_handlers_instance_test.go | 10 +- spinifex/daemon/daemon_handlers_test.go | 265 +----------------- spinifex/handlers/ec2/instance/service.go | 6 +- .../handlers/ec2/instance/service_impl.go | 80 ++++++ .../ec2/instance/service_impl_test.go | 243 +++++++++++++++- .../handlers/ec2/instance/service_mock.go | 4 + .../handlers/ec2/instance/service_nats.go | 4 + 8 files changed, 347 insertions(+), 370 deletions(-) diff --git a/spinifex/daemon/daemon_handlers_instance.go b/spinifex/daemon/daemon_handlers_instance.go index f8ff41ffe..6983b9b39 100644 --- a/spinifex/daemon/daemon_handlers_instance.go +++ b/spinifex/daemon/daemon_handlers_instance.go @@ -1,7 +1,6 @@ package daemon import ( - "encoding/base64" "encoding/json" "errors" "fmt" @@ -1162,110 +1161,8 @@ func (d *Daemon) describeInstancesFromKV(msg *nats.Msg, listFn func() ([]*vm.VM, slog.Info(handlerName+" completed", "count", len(reservations)) } -// handleEC2ModifyInstanceAttribute modifies attributes of a stopped instance in shared KV. -// All supported attributes (InstanceType, UserData) require the instance to be stopped. func (d *Daemon) handleEC2ModifyInstanceAttribute(msg *nats.Msg) { - var input ec2.ModifyInstanceAttributeInput - if err := json.Unmarshal(msg.Data, &input); err != nil { - slog.Error("handleEC2ModifyInstanceAttribute: failed to unmarshal request", "err", err) - respondWithError(msg, awserrors.ErrorServerInternal) - return - } - - if input.InstanceId == nil || *input.InstanceId == "" { - slog.Error("handleEC2ModifyInstanceAttribute: missing instance_id") - respondWithError(msg, awserrors.ErrorMissingParameter) - return - } - - instanceID := *input.InstanceId - - // SourceDestCheck is a networking concept that doesn't apply to bare-metal VMs. - // Accept the call as a no-op so Terraform and the AWS CLI don't error out. - // Unlike InstanceType/UserData, AWS allows this on running instances, so handle - // it before the stopped-state gate. - if input.SourceDestCheck != nil { - slog.Info("handleEC2ModifyInstanceAttribute: accepting SourceDestCheck (no-op on bare metal)", "instanceId", instanceID) - if err := msg.Respond([]byte(`{}`)); err != nil { - slog.Error("Failed to respond to NATS request", "err", err) - } - return - } - - if d.stateStore == nil { - slog.Error("handleEC2ModifyInstanceAttribute: state store not available") - respondWithError(msg, awserrors.ErrorServerInternal) - return - } - - instance, err := d.stateStore.LoadStoppedInstance(instanceID) - if err != nil { - slog.Error("handleEC2ModifyInstanceAttribute: failed to load stopped instance", "instanceId", instanceID, "err", err) - respondWithError(msg, awserrors.ErrorServerInternal) - return - } - if instance == nil { - slog.Warn("handleEC2ModifyInstanceAttribute: instance not found in shared KV", "instanceId", instanceID) - respondWithError(msg, awserrors.ErrorInvalidInstanceIDNotFound) - return - } - - if instance.Status != vm.StateStopped { - slog.Error("handleEC2ModifyInstanceAttribute: instance not in stopped state", "instanceId", instanceID, "status", instance.Status) - respondWithError(msg, awserrors.ErrorIncorrectInstanceState) - return - } - - // Verify the caller owns this instance - if !checkInstanceOwnership(msg, instanceID, instance.AccountID) { - return - } - - // Apply the requested attribute change - if input.InstanceType != nil && input.InstanceType.Value != nil { - newType := *input.InstanceType.Value - if newType == "" { - slog.Error("handleEC2ModifyInstanceAttribute: empty instance type value", "instanceId", instanceID) - respondWithError(msg, awserrors.ErrorInvalidInstanceAttributeValue) - return - } - if instance.Instance == nil { - slog.Error("handleEC2ModifyInstanceAttribute: instance.Instance is nil, data integrity issue", "instanceId", instanceID) - respondWithError(msg, awserrors.ErrorServerInternal) - return - } - slog.Info("handleEC2ModifyInstanceAttribute: changing instance type", - "instanceId", instanceID, "oldType", instance.InstanceType, "newType", newType) - - instance.InstanceType = newType - instance.Config.InstanceType = newType - instance.Instance.InstanceType = aws.String(newType) - // Clear StateReason — resolves capacity-unavailable state from instance-type-missing bug - instance.Instance.StateReason = nil - } - - if input.UserData != nil && input.UserData.Value != nil { - slog.Info("handleEC2ModifyInstanceAttribute: changing user data", "instanceId", instanceID) - - // Value arrives as decoded bytes (JSON unmarshal handles base64 → []byte automatically) - instance.UserData = string(input.UserData.Value) - if instance.RunInstancesInput != nil { - instance.RunInstancesInput.UserData = aws.String(base64.StdEncoding.EncodeToString(input.UserData.Value)) - } - } - - if err := d.stateStore.WriteStoppedInstance(instanceID, instance); err != nil { - slog.Error("handleEC2ModifyInstanceAttribute: failed to write modified instance to KV", - "instanceId", instanceID, "err", err) - respondWithError(msg, awserrors.ErrorServerInternal) - return - } - - slog.Info("handleEC2ModifyInstanceAttribute: completed successfully", "instanceId", instanceID) - - if err := msg.Respond([]byte(`{}`)); err != nil { - slog.Error("Failed to respond to NATS request", "err", err) - } + handleNATSRequest(msg, d.instanceService.ModifyInstanceAttribute) } // handleEC2DescribeInstanceAttribute returns a single requested attribute for an instance. diff --git a/spinifex/daemon/daemon_handlers_instance_test.go b/spinifex/daemon/daemon_handlers_instance_test.go index 1b4c6ae42..b6e238875 100644 --- a/spinifex/daemon/daemon_handlers_instance_test.go +++ b/spinifex/daemon/daemon_handlers_instance_test.go @@ -10,6 +10,8 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_instance "github.com/mulgadc/spinifex/spinifex/handlers/ec2/instance" + "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/spinifex/spinifex/vm" "github.com/nats-io/nats.go" @@ -142,11 +144,17 @@ var _ vm.StateStore = (*fakeStateStore)(nil) // 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. +// 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 { t.Helper() d := createTestDaemon(t, sharedNATSURL) d.stateStore = store + d.instanceService = handlers_ec2_instance.NewInstanceServiceImpl( + d.config, d.resourceMgr.instanceTypes, d.natsConn, + objectstore.NewMemoryObjectStore(), d.vmMgr, d.resourceMgr, store, + ) return d } diff --git a/spinifex/daemon/daemon_handlers_test.go b/spinifex/daemon/daemon_handlers_test.go index a3cd7d86f..173b9d91d 100644 --- a/spinifex/daemon/daemon_handlers_test.go +++ b/spinifex/daemon/daemon_handlers_test.go @@ -1653,9 +1653,14 @@ func TestAttachVolume_ZoneMismatch(t *testing.T) { assert.Contains(t, string(resp.Data), "InvalidVolume.ZoneMismatch") } -// --- handleEC2ModifyInstanceAttribute tests --- +// --- handleEC2ModifyInstanceAttribute wrapper smoke test --- +// +// Detailed logic coverage lives in +// handlers/ec2/instance/service_impl_test.go (TestModifyInstanceAttribute_*). +// This case keeps one end-to-end NATS → handleNATSRequest → service round-trip +// to confirm the daemon wiring stays intact. -func TestHandleEC2ModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { +func TestHandleEC2ModifyInstanceAttribute_WrapperRoundTrip(t *testing.T) { natsURL := sharedJSNATSURL daemon := createFullTestDaemonWithJetStream(t, natsURL) @@ -1664,7 +1669,7 @@ func TestHandleEC2ModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { require.NoError(t, err) defer sub.Unsubscribe() - instanceID := "i-modify-type-001" + instanceID := "i-modify-wrapper-001" instance := &vm.VM{ ID: instanceID, Status: vm.StateStopped, @@ -1693,260 +1698,6 @@ func TestHandleEC2ModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { require.NoError(t, err) require.NotNil(t, updated) assert.Equal(t, "t3.medium", updated.InstanceType) - assert.Equal(t, "t3.medium", updated.Config.InstanceType) - assert.Equal(t, "t3.medium", *updated.Instance.InstanceType) -} - -func TestHandleEC2ModifyInstanceAttribute_ChangeUserData(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - instanceID := "i-modify-ud-001" - instance := &vm.VM{ - ID: instanceID, - Status: vm.StateStopped, - InstanceType: "t3.micro", - AccountID: testAccountID, - UserData: "old data", - RunInstancesInput: &ec2.RunInstancesInput{ - UserData: aws.String("b2xkIGRhdGE="), - }, - Instance: &ec2.Instance{ - InstanceId: aws.String(instanceID), - }, - } - err = daemon.jsManager.WriteStoppedInstance(instanceID, instance) - require.NoError(t, err) - t.Cleanup(func() { _ = daemon.jsManager.DeleteStoppedInstance(instanceID) }) - - // Value holds decoded bytes (the gateway query parser decodes base64 from the CLI, - // then json.Marshal/Unmarshal round-trips []byte through base64 transparently) - newContent := "#!/bin/bash" - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String(instanceID), - UserData: &ec2.BlobAttributeValue{Value: []byte(newContent)}, - } - reqData, _ := json.Marshal(input) - reply, err := natsRequest(daemon.natsConn, "ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - assert.Equal(t, `{}`, string(reply.Data)) - - updated, err := daemon.jsManager.LoadStoppedInstance(instanceID) - require.NoError(t, err) - require.NotNil(t, updated) - assert.Equal(t, newContent, updated.UserData) - assert.Equal(t, "IyEvYmluL2Jhc2g=", *updated.RunInstancesInput.UserData) -} - -func TestHandleEC2ModifyInstanceAttribute_SourceDestCheck(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - // SourceDestCheck is a no-op that succeeds without requiring a stopped instance - // in KV — Terraform sends this on running instances right after creation. - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String("i-modify-sdc-001"), - SourceDestCheck: &ec2.AttributeBooleanValue{Value: aws.Bool(false)}, - } - reqData, _ := json.Marshal(input) - reply, err := natsRequest(daemon.natsConn, "ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - assert.Equal(t, `{}`, string(reply.Data)) -} - -func TestHandleEC2ModifyInstanceAttribute_InstanceNotFound(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String("i-nonexistent"), - InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, - } - reqData, _ := json.Marshal(input) - reply, err := daemon.natsConn.Request("ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - - var errResp map[string]any - err = json.Unmarshal(reply.Data, &errResp) - require.NoError(t, err) - assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, errResp["Code"]) -} - -func TestHandleEC2ModifyInstanceAttribute_NotStopped(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - instanceID := "i-modify-running-001" - instance := &vm.VM{ - ID: instanceID, - Status: vm.StateRunning, - InstanceType: "t3.micro", - AccountID: testAccountID, - } - err = daemon.jsManager.WriteStoppedInstance(instanceID, instance) - require.NoError(t, err) - t.Cleanup(func() { _ = daemon.jsManager.DeleteStoppedInstance(instanceID) }) - - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String(instanceID), - InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, - } - reqData, _ := json.Marshal(input) - reply, err := natsRequest(daemon.natsConn, "ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - - var errResp map[string]any - err = json.Unmarshal(reply.Data, &errResp) - require.NoError(t, err) - assert.Equal(t, awserrors.ErrorIncorrectInstanceState, errResp["Code"]) -} - -func TestHandleEC2ModifyInstanceAttribute_ClearsStateReason(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - instanceID := "i-modify-recovery-001" - instance := &vm.VM{ - ID: instanceID, - Status: vm.StateStopped, - InstanceType: "m7i.small", - AccountID: testAccountID, - Config: vm.Config{InstanceType: "m7i.small"}, - Instance: &ec2.Instance{ - InstanceId: aws.String(instanceID), - InstanceType: aws.String("m7i.small"), - StateReason: &ec2.StateReason{ - Code: aws.String("Server.InsufficientInstanceCapacity"), - Message: aws.String("Instance type not available on any node"), - }, - }, - } - err = daemon.jsManager.WriteStoppedInstance(instanceID, instance) - require.NoError(t, err) - t.Cleanup(func() { _ = daemon.jsManager.DeleteStoppedInstance(instanceID) }) - - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String(instanceID), - InstanceType: &ec2.AttributeValue{Value: aws.String("t3.micro")}, - } - reqData, _ := json.Marshal(input) - reply, err := natsRequest(daemon.natsConn, "ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - assert.Equal(t, `{}`, string(reply.Data)) - - updated, err := daemon.jsManager.LoadStoppedInstance(instanceID) - require.NoError(t, err) - require.NotNil(t, updated) - assert.Equal(t, "t3.micro", updated.InstanceType) - assert.Equal(t, "t3.micro", updated.Config.InstanceType) - assert.Equal(t, "t3.micro", *updated.Instance.InstanceType) - assert.Nil(t, updated.Instance.StateReason) -} - -func TestHandleEC2ModifyInstanceAttribute_InvalidTypeAccepted(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - instanceID := "i-modify-nonsense-001" - instance := &vm.VM{ - ID: instanceID, - Status: vm.StateStopped, - InstanceType: "t3.micro", - AccountID: testAccountID, - Config: vm.Config{InstanceType: "t3.micro"}, - Instance: &ec2.Instance{ - InstanceId: aws.String(instanceID), - InstanceType: aws.String("t3.micro"), - }, - } - err = daemon.jsManager.WriteStoppedInstance(instanceID, instance) - require.NoError(t, err) - t.Cleanup(func() { _ = daemon.jsManager.DeleteStoppedInstance(instanceID) }) - - // z99.mega is nonsense — modify does not pre-validate, matching AWS behavior - input := &ec2.ModifyInstanceAttributeInput{ - InstanceId: aws.String(instanceID), - InstanceType: &ec2.AttributeValue{Value: aws.String("z99.mega")}, - } - reqData, _ := json.Marshal(input) - reply, err := natsRequest(daemon.natsConn, "ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - assert.Equal(t, `{}`, string(reply.Data)) - - updated, err := daemon.jsManager.LoadStoppedInstance(instanceID) - require.NoError(t, err) - require.NotNil(t, updated) - assert.Equal(t, "z99.mega", updated.InstanceType) - assert.Equal(t, "z99.mega", updated.Config.InstanceType) - assert.Equal(t, "z99.mega", *updated.Instance.InstanceType) -} - -func TestHandleEC2ModifyInstanceAttribute_MissingInstanceID(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - input := &ec2.ModifyInstanceAttributeInput{} - reqData, _ := json.Marshal(input) - reply, err := daemon.natsConn.Request("ec2.ModifyInstanceAttribute", reqData, 5*time.Second) - require.NoError(t, err) - - var errResp map[string]any - err = json.Unmarshal(reply.Data, &errResp) - require.NoError(t, err) - assert.Equal(t, awserrors.ErrorMissingParameter, errResp["Code"]) -} - -func TestHandleEC2ModifyInstanceAttribute_InvalidJSON(t *testing.T) { - natsURL := sharedJSNATSURL - - daemon := createFullTestDaemonWithJetStream(t, natsURL) - - sub, err := daemon.natsConn.QueueSubscribe("ec2.ModifyInstanceAttribute", "spinifex-workers", daemon.handleEC2ModifyInstanceAttribute) - require.NoError(t, err) - defer sub.Unsubscribe() - - reply, err := daemon.natsConn.Request("ec2.ModifyInstanceAttribute", []byte(`{invalid`), 5*time.Second) - require.NoError(t, err) - - var errResp map[string]any - err = json.Unmarshal(reply.Data, &errResp) - require.NoError(t, err) - assert.Equal(t, awserrors.ErrorServerInternal, errResp["Code"]) } // --- DescribeInstanceAttribute daemon tests --- diff --git a/spinifex/handlers/ec2/instance/service.go b/spinifex/handlers/ec2/instance/service.go index 0c45e53e7..5f4baef06 100644 --- a/spinifex/handlers/ec2/instance/service.go +++ b/spinifex/handlers/ec2/instance/service.go @@ -13,6 +13,7 @@ type InstanceService interface { DescribeInstanceAttribute(input *ec2.DescribeInstanceAttributeInput, accountID string) (*ec2.DescribeInstanceAttributeOutput, error) DescribeStoppedInstances(input *ec2.DescribeInstancesInput, accountID string) (*ec2.DescribeInstancesOutput, error) DescribeTerminatedInstances(input *ec2.DescribeInstancesInput, accountID string) (*ec2.DescribeInstancesOutput, error) + ModifyInstanceAttribute(input *ec2.ModifyInstanceAttributeInput, accountID string) (*ec2.ModifyInstanceAttributeOutput, error) } // ResourceCapacityProvider exposes per-node instance-type availability used by @@ -21,10 +22,11 @@ type ResourceCapacityProvider interface { GetAvailableInstanceTypeInfos(showCapacity bool) []*ec2.InstanceTypeInfo } -// StoppedInstanceStore provides KV-backed read access to stopped/terminated -// instances. Implemented by daemon.JetStreamManager. +// StoppedInstanceStore provides KV-backed read/write access to stopped and +// read access to terminated instances. Implemented by daemon.JetStreamManager. type StoppedInstanceStore interface { LoadStoppedInstance(instanceID string) (*vm.VM, error) ListStoppedInstances() ([]*vm.VM, error) ListTerminatedInstances() ([]*vm.VM, error) + WriteStoppedInstance(instanceID string, instance *vm.VM) error } diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 46be34b67..7754ba445 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -1157,6 +1157,86 @@ func (s *InstanceServiceImpl) describeInstancesFromKV(input *ec2.DescribeInstanc return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil } +// ModifyInstanceAttribute applies a single attribute change to a stopped instance. +// All supported attributes (InstanceType, UserData) require the instance to be +// stopped. SourceDestCheck is a networking concept that does not apply to +// bare-metal VMs; it is accepted as a no-op on any instance state so Terraform +// and the AWS CLI do not error out. +func (s *InstanceServiceImpl) ModifyInstanceAttribute(input *ec2.ModifyInstanceAttributeInput, accountID string) (*ec2.ModifyInstanceAttributeOutput, error) { + if input.InstanceId == nil || *input.InstanceId == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + instanceID := *input.InstanceId + + if input.SourceDestCheck != nil { + slog.Info("ModifyInstanceAttribute: accepting SourceDestCheck (no-op on bare metal)", "instanceId", instanceID) + return &ec2.ModifyInstanceAttributeOutput{}, nil + } + + if s.stoppedStore == nil { + slog.Error("ModifyInstanceAttribute: stopped store not available") + return nil, errors.New(awserrors.ErrorServerInternal) + } + + instance, err := s.stoppedStore.LoadStoppedInstance(instanceID) + if err != nil { + slog.Error("ModifyInstanceAttribute: failed to load stopped instance", "instanceId", instanceID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + if instance == nil { + slog.Warn("ModifyInstanceAttribute: instance not found in shared KV", "instanceId", instanceID) + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) + } + + if instance.Status != vm.StateStopped { + slog.Error("ModifyInstanceAttribute: instance not in stopped state", "instanceId", instanceID, "status", instance.Status) + return nil, errors.New(awserrors.ErrorIncorrectInstanceState) + } + + if !IsInstanceVisible(accountID, instance.AccountID) { + slog.Warn("ModifyInstanceAttribute: instance not visible", + "instanceId", instanceID, "callerAccount", accountID, "ownerAccount", instance.AccountID) + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) + } + + if input.InstanceType != nil && input.InstanceType.Value != nil { + newType := *input.InstanceType.Value + if newType == "" { + slog.Error("ModifyInstanceAttribute: empty instance type value", "instanceId", instanceID) + return nil, errors.New(awserrors.ErrorInvalidInstanceAttributeValue) + } + if instance.Instance == nil { + slog.Error("ModifyInstanceAttribute: instance.Instance is nil, data integrity issue", "instanceId", instanceID) + return nil, errors.New(awserrors.ErrorServerInternal) + } + slog.Info("ModifyInstanceAttribute: changing instance type", + "instanceId", instanceID, "oldType", instance.InstanceType, "newType", newType) + + instance.InstanceType = newType + instance.Config.InstanceType = newType + instance.Instance.InstanceType = aws.String(newType) + // Clear StateReason — resolves capacity-unavailable state from instance-type-missing bug. + instance.Instance.StateReason = nil + } + + if input.UserData != nil && input.UserData.Value != nil { + slog.Info("ModifyInstanceAttribute: changing user data", "instanceId", instanceID) + instance.UserData = string(input.UserData.Value) + if instance.RunInstancesInput != nil { + instance.RunInstancesInput.UserData = aws.String(base64.StdEncoding.EncodeToString(input.UserData.Value)) + } + } + + if err := s.stoppedStore.WriteStoppedInstance(instanceID, instance); err != nil { + slog.Error("ModifyInstanceAttribute: failed to write modified instance to KV", + "instanceId", instanceID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("ModifyInstanceAttribute: completed successfully", "instanceId", instanceID) + return &ec2.ModifyInstanceAttributeOutput{}, nil +} + // DescribeInstanceAttribute returns a single requested attribute for an instance. // Checks running instances first (in-memory), then falls back to stopped instances in KV. func (s *InstanceServiceImpl) DescribeInstanceAttribute(input *ec2.DescribeInstanceAttributeInput, accountID string) (*ec2.DescribeInstanceAttributeOutput, error) { diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 613ae4f6f..af93a14ee 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -653,12 +653,14 @@ func (f *fakeResourceCapacityProvider) GetAvailableInstanceTypeInfos(showCapacit } type fakeStoppedStore struct { - stopped []*vm.VM - terminated []*vm.VM - loadByID map[string]*vm.VM - listErr error - listTermErr error - loadErr error + stopped []*vm.VM + terminated []*vm.VM + loadByID map[string]*vm.VM + wroteStopped map[string]*vm.VM + listErr error + listTermErr error + loadErr error + writeErr error } func (f *fakeStoppedStore) LoadStoppedInstance(id string) (*vm.VM, error) { @@ -682,6 +684,16 @@ func (f *fakeStoppedStore) ListTerminatedInstances() ([]*vm.VM, error) { } return f.terminated, nil } +func (f *fakeStoppedStore) WriteStoppedInstance(id string, instance *vm.VM) error { + if f.writeErr != nil { + return f.writeErr + } + if f.wroteStopped == nil { + f.wroteStopped = make(map[string]*vm.VM) + } + f.wroteStopped[id] = instance + return nil +} func TestDescribeInstanceTypes_NilResourceMgr(t *testing.T) { svc := &InstanceServiceImpl{} @@ -1036,3 +1048,222 @@ func TestIsInstanceVisible(t *testing.T) { }) } } + +func TestModifyInstanceAttribute_MissingInstanceID(t *testing.T) { + svc := &InstanceServiceImpl{} + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{}, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestModifyInstanceAttribute_SourceDestCheckNoOp(t *testing.T) { + // SourceDestCheck succeeds without touching KV or requiring stopped state. + svc := &InstanceServiceImpl{} + out, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String("i-sdc-001"), + SourceDestCheck: &ec2.AttributeBooleanValue{Value: aws.Bool(false)}, + }, "acc") + require.NoError(t, err) + require.NotNil(t, out) +} + +func TestModifyInstanceAttribute_NilStore(t *testing.T) { + svc := &InstanceServiceImpl{} + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String("i-1"), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +func TestModifyInstanceAttribute_InstanceNotFound(t *testing.T) { + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{}} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String("i-missing"), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) +} + +func TestModifyInstanceAttribute_NotStopped(t *testing.T) { + id := "i-running" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: {ID: id, Status: vm.StateRunning, AccountID: "acc"}, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorIncorrectInstanceState, err.Error()) +} + +func TestModifyInstanceAttribute_NotVisible(t *testing.T) { + id := "i-stopped" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: {ID: id, Status: vm.StateStopped, AccountID: "owner-acc"}, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "other-acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) +} + +func TestModifyInstanceAttribute_ChangeInstanceType(t *testing.T) { + id := "i-type" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: { + ID: id, + Status: vm.StateStopped, + AccountID: "acc", + InstanceType: "t3.micro", + Config: vm.Config{InstanceType: "t3.micro"}, + Instance: &ec2.Instance{ + InstanceId: aws.String(id), + InstanceType: aws.String("t3.micro"), + }, + }, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.NoError(t, err) + + updated := store.wroteStopped[id] + require.NotNil(t, updated) + assert.Equal(t, "t3.medium", updated.InstanceType) + assert.Equal(t, "t3.medium", updated.Config.InstanceType) + assert.Equal(t, "t3.medium", *updated.Instance.InstanceType) +} + +func TestModifyInstanceAttribute_ChangeInstanceType_EmptyValue(t *testing.T) { + id := "i-empty" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: {ID: id, Status: vm.StateStopped, AccountID: "acc", Instance: &ec2.Instance{}}, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceAttributeValue, err.Error()) +} + +func TestModifyInstanceAttribute_ChangeInstanceType_NilEmbeddedInstance(t *testing.T) { + id := "i-nil-inst" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: {ID: id, Status: vm.StateStopped, AccountID: "acc"}, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +func TestModifyInstanceAttribute_ChangeUserData(t *testing.T) { + id := "i-ud" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: { + ID: id, + Status: vm.StateStopped, + AccountID: "acc", + UserData: "old", + RunInstancesInput: &ec2.RunInstancesInput{ + UserData: aws.String("b2xk"), + }, + Instance: &ec2.Instance{InstanceId: aws.String(id)}, + }, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + newContent := "#!/bin/bash" + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + UserData: &ec2.BlobAttributeValue{Value: []byte(newContent)}, + }, "acc") + require.NoError(t, err) + + updated := store.wroteStopped[id] + require.NotNil(t, updated) + assert.Equal(t, newContent, updated.UserData) + assert.Equal(t, "IyEvYmluL2Jhc2g=", *updated.RunInstancesInput.UserData) +} + +func TestModifyInstanceAttribute_ClearsStateReason(t *testing.T) { + id := "i-recovery" + store := &fakeStoppedStore{loadByID: map[string]*vm.VM{ + id: { + ID: id, + Status: vm.StateStopped, + AccountID: "acc", + InstanceType: "m7i.small", + Config: vm.Config{InstanceType: "m7i.small"}, + Instance: &ec2.Instance{ + InstanceId: aws.String(id), + InstanceType: aws.String("m7i.small"), + StateReason: &ec2.StateReason{ + Code: aws.String("Server.InsufficientInstanceCapacity"), + Message: aws.String("Instance type not available on any node"), + }, + }, + }, + }} + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.micro")}, + }, "acc") + require.NoError(t, err) + + updated := store.wroteStopped[id] + require.NotNil(t, updated) + assert.Nil(t, updated.Instance.StateReason) +} + +func TestModifyInstanceAttribute_WriteError(t *testing.T) { + id := "i-werr" + store := &fakeStoppedStore{ + loadByID: map[string]*vm.VM{ + id: { + ID: id, + Status: vm.StateStopped, + AccountID: "acc", + InstanceType: "t3.micro", + Config: vm.Config{InstanceType: "t3.micro"}, + Instance: &ec2.Instance{ + InstanceId: aws.String(id), + InstanceType: aws.String("t3.micro"), + }, + }, + }, + writeErr: fmt.Errorf("kv write boom"), + } + svc := &InstanceServiceImpl{stoppedStore: store} + + _, err := svc.ModifyInstanceAttribute(&ec2.ModifyInstanceAttributeInput{ + InstanceId: aws.String(id), + InstanceType: &ec2.AttributeValue{Value: aws.String("t3.medium")}, + }, "acc") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} diff --git a/spinifex/handlers/ec2/instance/service_mock.go b/spinifex/handlers/ec2/instance/service_mock.go index be98d09f2..203688552 100644 --- a/spinifex/handlers/ec2/instance/service_mock.go +++ b/spinifex/handlers/ec2/instance/service_mock.go @@ -55,3 +55,7 @@ func (s *MockInstanceService) DescribeStoppedInstances(_ *ec2.DescribeInstancesI func (s *MockInstanceService) DescribeTerminatedInstances(_ *ec2.DescribeInstancesInput, _ string) (*ec2.DescribeInstancesOutput, error) { return &ec2.DescribeInstancesOutput{}, nil } + +func (s *MockInstanceService) ModifyInstanceAttribute(_ *ec2.ModifyInstanceAttributeInput, _ string) (*ec2.ModifyInstanceAttributeOutput, error) { + return &ec2.ModifyInstanceAttributeOutput{}, nil +} diff --git a/spinifex/handlers/ec2/instance/service_nats.go b/spinifex/handlers/ec2/instance/service_nats.go index 66023c709..a09d6ac45 100644 --- a/spinifex/handlers/ec2/instance/service_nats.go +++ b/spinifex/handlers/ec2/instance/service_nats.go @@ -60,3 +60,7 @@ func (s *NATSInstanceService) DescribeStoppedInstances(input *ec2.DescribeInstan func (s *NATSInstanceService) DescribeTerminatedInstances(input *ec2.DescribeInstancesInput, accountID string) (*ec2.DescribeInstancesOutput, error) { return utils.NATSRequest[ec2.DescribeInstancesOutput](s.natsConn, "ec2.DescribeTerminatedInstances", input, 30*time.Second, accountID) } + +func (s *NATSInstanceService) ModifyInstanceAttribute(input *ec2.ModifyInstanceAttributeInput, accountID string) (*ec2.ModifyInstanceAttributeOutput, error) { + return utils.NATSRequest[ec2.ModifyInstanceAttributeOutput](s.natsConn, "ec2.ModifyInstanceAttribute", input, 30*time.Second, accountID) +}