diff --git a/docs/testing/e2e-coverage.md b/docs/testing/e2e-coverage.md index a23819ff3..bb37be01d 100644 --- a/docs/testing/e2e-coverage.md +++ b/docs/testing/e2e-coverage.md @@ -28,7 +28,7 @@ The largest suite, 25 top-level tests. Notable entry points: - `TestWANEgressSMTPBlock` — egress port blocking. - `TestInstanceMetadata` — IMDS from inside a guest. - `TestStopStart`, `TestEarlyRebootLiveness`, `TestGuestChurnDurability`, `TestSnapshotLifecycle`, `TestSnapshotBackedLaunch`, `TestCreateImage`, `TestVolumeLifecycle`, `TestSpotInstanceLifecycle`, `TestLaunchTemplateBoot`. -- `TestNegativeErrorPaths` — the `8a`–`8h` AWS error-shape cases (malformed AMI ID, invalid instance type, volume in use, detach-root forbidden, and so on). +- `TestNegativeErrorPaths` — the `8a`–`8t` AWS error-shape cases (malformed AMI ID, invalid instance type, volume in use, detach-root forbidden, unknown or malformed instance IDs on the lifecycle verbs, and so on). - **`TestKVReplication`** — the single-node half of the KV replication policy, and it is not the multi-node assertion with a smaller number in it. On one node one replica per bucket is the correct answer rather than the defect, so what it proves is the opposite: that nothing declared a replica count this deployment cannot place, that `spx admin kv replicas` exits 0 and reports `WANT 1`, and that `--repair` run twice against a cluster with nothing to repair changes no bucket's replica count and leaves every bucket serving. The multi-node suites cannot cover any of that, because on three nodes the unplaceable count is the right one. It lives here rather than in its own suite so it runs in every single-node nightly cell, across all three host and network permutations. ### `natuplink` — routed NAT (`external_mode = "nat"`), one node or three diff --git a/spinifex/accountteardown/reapers_ec2_test.go b/spinifex/accountteardown/reapers_ec2_test.go index e32754902..65c37074f 100644 --- a/spinifex/accountteardown/reapers_ec2_test.go +++ b/spinifex/accountteardown/reapers_ec2_test.go @@ -173,9 +173,9 @@ func TestInstanceReaperTerminates(t *testing.T) { reaper := &instanceReaper{nc: cluster.nc, expectedNodes: 1} require.NoError(t, reaper.Delete(testCtx(t), "000000000042", - Resource{Kind: "instance", ID: "i-abc"}, false)) + Resource{Kind: "instance", ID: "i-0abc0123456789def"}, false)) - assert.Contains(t, cluster.called(), "ec2.cmd.i-abc") + assert.Contains(t, cluster.called(), "ec2.cmd.i-0abc0123456789def") } // The attachment is the reason a volume will not go, so it has to reach the diff --git a/spinifex/gateway/ec2/instance/RebootInstances.go b/spinifex/gateway/ec2/instance/RebootInstances.go index 6bf137ea7..b9e487a4b 100644 --- a/spinifex/gateway/ec2/instance/RebootInstances.go +++ b/spinifex/gateway/ec2/instance/RebootInstances.go @@ -22,7 +22,8 @@ import ( var rebootInstancesTimeout = 5 * time.Second // ValidateRebootInstancesInput rejects a nil input with InvalidParameterValue and an empty -// InstanceIds list with MissingParameter. +// InstanceIds list with MissingParameter, and a malformed instance ID with +// InvalidInstanceID.Malformed before any NATS traffic. func ValidateRebootInstancesInput(input *ec2.RebootInstancesInput) error { if input == nil { return errors.New(awserrors.ErrorInvalidParameterValue) @@ -30,7 +31,7 @@ func ValidateRebootInstancesInput(input *ec2.RebootInstancesInput) error { if len(input.InstanceIds) == 0 { return errors.New(awserrors.ErrorMissingParameter) } - return nil + return validateInstanceIDFormat(input.InstanceIds) } // RebootInstances sends reboot commands to specified instances via NATS. @@ -79,19 +80,17 @@ func RebootInstances(ctx context.Context, input *ec2.RebootInstancesInput, natsC return nil, errors.New(awserrors.ErrorServerInternal) } - // No daemon subscription: check stopped-KV to return IncorrectInstanceState instead of NotFound. - describeInput := &ec2.DescribeInstancesInput{ - InstanceIds: []*string{&instanceID}, - } - describeData, err := json.Marshal(describeInput) - if err != nil { - return nil, fmt.Errorf("marshal describe input: %w", err) - } - if reservations, _ := queryInstanceBucket(ctx, natsConn, "ec2.DescribeStoppedInstances", describeData, accountID); len(reservations) > 0 { + // No daemon subscription: a stopped or terminated instance cannot be + // rebooted; only readable, empty buckets prove it does not exist. + state, queryOK := classifyUnownedInstance(ctx, natsConn, instanceID, accountID) + switch { + case state != bucketStateAbsent: return nil, errors.New(awserrors.ErrorIncorrectInstanceState) + case queryOK: + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) + default: + return nil, errors.New(awserrors.ErrorServerInternal) } - - return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) } if responseError, parseErr := utils.ValidateErrorPayload(msg.Data); parseErr != nil { diff --git a/spinifex/gateway/ec2/instance/RebootInstances_test.go b/spinifex/gateway/ec2/instance/RebootInstances_test.go index d5efb95fa..69915aa7f 100644 --- a/spinifex/gateway/ec2/instance/RebootInstances_test.go +++ b/spinifex/gateway/ec2/instance/RebootInstances_test.go @@ -47,7 +47,7 @@ func TestRebootInstances_MultipleInstances(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - ids := []string{"i-001", "i-002"} + ids := []string{"i-06794c450e770fbc0", "i-70aee04c61ac4d963"} for _, id := range ids { nc.Subscribe("ec2.cmd."+id, func(msg *nats.Msg) { @@ -78,12 +78,13 @@ func TestRebootInstances_EmptyInstanceIds(t *testing.T) { assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) } -func TestRebootInstances_NATSRequestFails(t *testing.T) { +func TestRebootInstances_NeverExistedIsNotFound(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) + serveEmptyBuckets(t, nc) input := &ec2.RebootInstancesInput{ - InstanceIds: []*string{aws.String("i-nosubscriber")}, + InstanceIds: []*string{aws.String("i-d2d5fe795b6b57882")}, } _, err := RebootInstances(context.Background(), input, nc, "123456789012") @@ -91,6 +92,36 @@ func TestRebootInstances_NATSRequestFails(t *testing.T) { assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) } +func TestRebootInstances_BucketsUnreachableIsServerError(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + input := &ec2.RebootInstancesInput{ + InstanceIds: []*string{aws.String("i-0cc1dd2ee3ff40516")}, + } + + _, err := RebootInstances(context.Background(), input, nc, "123456789012") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +func TestRebootInstances_TerminatedInstanceIsIncorrectState(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + instanceID := "i-0bb1cc2dd3ee4ff50" + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances", instanceID) + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + + input := &ec2.RebootInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + } + + _, err := RebootInstances(context.Background(), input, nc, "123456789012") + require.Error(t, err) + assert.Equal(t, awserrors.ErrorIncorrectInstanceState, err.Error()) +} + // TestRebootInstances_SlowDaemonIsNotAMissingInstance covers a daemon that // holds the request open. Reporting that as NotFound names a fault the caller // does not have and hides the one it does: the instance is there, the node @@ -102,7 +133,7 @@ func TestRebootInstances_SlowDaemonIsNotAMissingInstance(t *testing.T) { rebootInstancesTimeout = 50 * time.Millisecond t.Cleanup(func() { rebootInstancesTimeout = previous }) - instanceID := "i-slow" + instanceID := "i-6ed2716331719b275" // A live subscriber that never replies forces a real ErrTimeout rather // than the immediate ErrNoResponders a missing subscriber would give. _, err := nc.Subscribe("ec2.cmd."+instanceID, func(*nats.Msg) {}) @@ -120,7 +151,7 @@ func TestRebootInstances_StoppedInstance(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-stopped" + instanceID := "i-42798f10e29985019" // No ec2.cmd subscription — simulates a stopped instance with no daemon listener. // Subscribe to the stopped-instances describe topic to return the instance. @@ -152,7 +183,7 @@ func TestRebootInstances_DaemonError(t *testing.T) { t.Run(code, func(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-error" + instanceID := "i-156a87e2dd55d1967" nc.Subscribe("ec2.cmd."+instanceID, func(msg *nats.Msg) { msg.Respond(utils.GenerateErrorPayload(code)) diff --git a/spinifex/gateway/ec2/instance/StartInstances.go b/spinifex/gateway/ec2/instance/StartInstances.go index 6d0c871ea..e7ef43276 100644 --- a/spinifex/gateway/ec2/instance/StartInstances.go +++ b/spinifex/gateway/ec2/instance/StartInstances.go @@ -21,7 +21,8 @@ type startStoppedInstanceRequest struct { } // ValidateStartInstancesInput rejects a nil input with InvalidParameterValue and an empty -// InstanceIds list with MissingParameter. +// InstanceIds list with MissingParameter, and a malformed instance ID with +// InvalidInstanceID.Malformed before any NATS traffic. func ValidateStartInstancesInput(input *ec2.StartInstancesInput) error { if input == nil { return errors.New(awserrors.ErrorInvalidParameterValue) @@ -29,7 +30,7 @@ func ValidateStartInstancesInput(input *ec2.StartInstancesInput) error { if len(input.InstanceIds) == 0 { return errors.New(awserrors.ErrorMissingParameter) } - return nil + return validateInstanceIDFormat(input.InstanceIds) } // StartInstances starts the requested instances via the per-instance ec2.cmd topic, @@ -128,8 +129,9 @@ func startStoppedInstance(ctx context.Context, natsConn *nats.Conn, instanceID, utils.InjectTraceContext(ctx, reqMsg.Header) msg, err := natsConn.RequestMsg(reqMsg, 30*time.Second) if err != nil { + // A daemon that answers reports NotFound itself; no answer proves nothing. slog.ErrorContext(ctx, "StartInstances: Failed to send start request", "instance_id", instanceID, "err", err) - return newStateChange(instanceID, 80, "stopped", 80, "stopped"), nil + return nil, errors.New(awserrors.ErrorServerInternal) } if responseError, parseErr := utils.ValidateErrorPayload(msg.Data); parseErr != nil { diff --git a/spinifex/gateway/ec2/instance/StartInstances_test.go b/spinifex/gateway/ec2/instance/StartInstances_test.go index 41f261fed..7f414fd7b 100644 --- a/spinifex/gateway/ec2/instance/StartInstances_test.go +++ b/spinifex/gateway/ec2/instance/StartInstances_test.go @@ -52,7 +52,7 @@ func TestStartInstances_MultipleInstances(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - ids := []string{"i-001", "i-002", "i-003"} + ids := []string{"i-06794c450e770fbc0", "i-70aee04c61ac4d963", "i-ed828a4ce70b0199c"} // Single subscriber handles all start requests via queue group nc.QueueSubscribe("ec2.start", "spinifex-workers", func(msg *nats.Msg) { @@ -94,7 +94,7 @@ func TestStartInstances_NilInstanceIdSkipped(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-valid" + instanceID := "i-476e190637b456de0" nc.QueueSubscribe("ec2.start", "spinifex-workers", func(msg *nats.Msg) { msg.Respond([]byte(`{"status":"running","instanceId":"` + instanceID + `"}`)) }) @@ -117,7 +117,7 @@ func TestStartInstances_NATSRequestFails(t *testing.T) { _, nc := startTestNATSServer(t) // No subscriber for ec2.start, so NATS request will fail - instanceID := "i-nosubscriber" + instanceID := "i-d2d5fe795b6b57882" input := &ec2.StartInstancesInput{ InstanceIds: []*string{aws.String(instanceID)}, @@ -125,25 +125,19 @@ func TestStartInstances_NATSRequestFails(t *testing.T) { output, err := StartInstances(context.Background(), input, nc, "123456789012") - require.NoError(t, err) // Function itself doesn't error, it records state change - require.NotNil(t, output) - require.Len(t, output.StartingInstances, 1) - - // On NATS failure, state should reflect "still stopped" - sc := output.StartingInstances[0] - assert.Equal(t, instanceID, *sc.InstanceId) - assert.Equal(t, int64(80), *sc.CurrentState.Code) - assert.Equal(t, "stopped", *sc.CurrentState.Name) - assert.Equal(t, int64(80), *sc.PreviousState.Code) - assert.Equal(t, "stopped", *sc.PreviousState.Name) + // No daemon answered, so nothing is known about the instance: neither a + // fabricated stopped/stopped change nor NotFound. + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Nil(t, output) } func TestStartInstances_MixedSuccessAndFailure(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - goodID := "i-good" - badID := "i-bad" + goodID := "i-fd00438116c1f2ce2" + badID := "i-587b21c4147f9d9d8" // Subscribe to ec2.start but only respond for goodID nc.QueueSubscribe("ec2.start", "spinifex-workers", func(msg *nats.Msg) { diff --git a/spinifex/gateway/ec2/instance/StopInstances.go b/spinifex/gateway/ec2/instance/StopInstances.go index a036e29f2..8f5a97598 100644 --- a/spinifex/gateway/ec2/instance/StopInstances.go +++ b/spinifex/gateway/ec2/instance/StopInstances.go @@ -4,9 +4,7 @@ import ( "context" "encoding/json" "errors" - "fmt" "log/slog" - "time" "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" @@ -16,7 +14,8 @@ import ( ) // ValidateStopInstancesInput rejects a nil input with InvalidParameterValue and an empty -// InstanceIds list with MissingParameter. +// InstanceIds list with MissingParameter, and a malformed instance ID with +// InvalidInstanceID.Malformed before any NATS traffic. func ValidateStopInstancesInput(input *ec2.StopInstancesInput) error { if input == nil { return errors.New(awserrors.ErrorInvalidParameterValue) @@ -24,7 +23,7 @@ func ValidateStopInstancesInput(input *ec2.StopInstancesInput) error { if len(input.InstanceIds) == 0 { return errors.New(awserrors.ErrorMissingParameter) } - return nil + return validateInstanceIDFormat(input.InstanceIds) } // StopInstances sends stop commands via NATS using system_powerdown with stop_instance @@ -58,16 +57,27 @@ func StopInstances(ctx context.Context, input *ec2.StopInstancesInput, natsConn continue } - subject := fmt.Sprintf("ec2.cmd.%s", instanceID) - reqMsg := nats.NewMsg(subject) - reqMsg.Data = jsonData - reqMsg.Header.Set(utils.AccountIDHeader, accountID) - utils.InjectTraceContext(ctx, reqMsg.Header) - msg, err := natsConn.RequestMsg(reqMsg, 5*time.Second) + msg, err := requestInstanceCmd(ctx, natsConn, instanceID, accountID, jsonData) if err != nil { slog.ErrorContext(ctx, "StopInstances: Failed to send command", "instance_id", instanceID, "err", err) - stateChanges = append(stateChanges, newStateChange(instanceID, 16, "running", 16, "running")) - continue + // A timeout is a slow or wedged owner, which says nothing about + // where the instance is. + if !errors.Is(err, nats.ErrNoResponders) { + return nil, errors.New(awserrors.ErrorServerInternal) + } + state, queryOK := classifyUnownedInstance(ctx, natsConn, instanceID, accountID) + switch { + case state == bucketStateStopped: + // Stopping a stopped instance succeeds as a no-op, as on AWS. + stateChanges = append(stateChanges, newStateChange(instanceID, 80, "stopped", 80, "stopped")) + continue + case state == bucketStateTerminated: + return nil, errors.New(awserrors.ErrorIncorrectInstanceState) + case queryOK: + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) + default: + return nil, errors.New(awserrors.ErrorServerInternal) + } } if responseError, parseErr := utils.ValidateErrorPayload(msg.Data); parseErr != nil { diff --git a/spinifex/gateway/ec2/instance/StopInstances_test.go b/spinifex/gateway/ec2/instance/StopInstances_test.go index 80ec4f9b9..66f2f142a 100644 --- a/spinifex/gateway/ec2/instance/StopInstances_test.go +++ b/spinifex/gateway/ec2/instance/StopInstances_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -54,7 +55,7 @@ func TestStopInstances_MultipleInstances(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - ids := []string{"i-001", "i-002"} + ids := []string{"i-06794c450e770fbc0", "i-70aee04c61ac4d963"} for _, id := range ids { nc.Subscribe("ec2.cmd."+id, func(msg *nats.Msg) { @@ -95,7 +96,7 @@ func TestStopInstances_NilInstanceIdSkipped(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-valid" + instanceID := "i-476e190637b456de0" nc.Subscribe("ec2.cmd."+instanceID, func(msg *nats.Msg) { msg.Respond([]byte(`{"return":{}}`)) }) @@ -111,55 +112,114 @@ func TestStopInstances_NilInstanceIdSkipped(t *testing.T) { assert.Equal(t, instanceID, *output.StoppingInstances[0].InstanceId) } -func TestStopInstances_NATSRequestFails(t *testing.T) { +// The reported bug: an id the system never issued was answered with a +// fabricated running/running state change instead of NotFound. +func TestStopInstances_NeverExistedIsNotFound(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveEmptyBuckets(t, nc) + + output, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ + InstanceIds: []*string{aws.String("i-d2d5fe795b6b57882")}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) + assert.Nil(t, output) +} + +// Stopping an already-stopped instance is a successful no-op on AWS. +func TestStopInstances_StoppedInstanceIsNoOp(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-nosubscriber" + instanceID := "i-0aa1bb2cc3dd4ee5f" + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances", instanceID) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances") - input := &ec2.StopInstancesInput{ + output, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ InstanceIds: []*string{aws.String(instanceID)}, - } - - output, err := StopInstances(context.Background(), input, nc, "123456789012") + }, nc, "123456789012") require.NoError(t, err) require.Len(t, output.StoppingInstances, 1) - - // On NATS failure, state should reflect "still running" sc := output.StoppingInstances[0] assert.Equal(t, instanceID, *sc.InstanceId) - assert.Equal(t, int64(16), *sc.CurrentState.Code) - assert.Equal(t, "running", *sc.CurrentState.Name) - assert.Equal(t, int64(16), *sc.PreviousState.Code) - assert.Equal(t, "running", *sc.PreviousState.Name) + assert.Equal(t, int64(80), *sc.CurrentState.Code) + assert.Equal(t, "stopped", *sc.CurrentState.Name) + assert.Equal(t, int64(80), *sc.PreviousState.Code) + assert.Equal(t, "stopped", *sc.PreviousState.Name) } -func TestStopInstances_MixedSuccessAndFailure(t *testing.T) { +func TestStopInstances_TerminatedInstanceIsIncorrectState(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - goodID := "i-good" - badID := "i-bad" + instanceID := "i-0bb1cc2dd3ee4ff50" + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances", instanceID) + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + + _, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorIncorrectInstanceState, err.Error()) +} + +func TestStopInstances_BucketsUnreachableIsServerError(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + output, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ + InstanceIds: []*string{aws.String("i-0cc1dd2ee3ff40516")}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Nil(t, output) +} + +// A live owner that does not answer in time is not evidence the instance is +// missing, and must not be reported as a state change either. +func TestStopInstances_SlowOwnerIsServerError(t *testing.T) { + _, nc := startTestNATSServer(t) + + previous := instanceCmdTimeout + instanceCmdTimeout = 50 * time.Millisecond + t.Cleanup(func() { instanceCmdTimeout = previous }) + + instanceID := "i-0dd1ee2ff30415263" + _, err := nc.Subscribe("ec2.cmd."+instanceID, func(*nats.Msg) {}) + require.NoError(t, err) + serveEmptyBuckets(t, nc) + + output, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) + assert.Nil(t, output) +} + +func TestStopInstances_MixedBatchWithUnknownIDFails(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + goodID := "i-fd00438116c1f2ce2" + unknownID := "i-587b21c4147f9d9d8" nc.Subscribe("ec2.cmd."+goodID, func(msg *nats.Msg) { msg.Respond([]byte(`{"return":{}}`)) }) + serveEmptyBuckets(t, nc) - input := &ec2.StopInstancesInput{ - InstanceIds: []*string{aws.String(goodID), aws.String(badID)}, - } + output, err := StopInstances(context.Background(), &ec2.StopInstancesInput{ + InstanceIds: []*string{aws.String(goodID), aws.String(unknownID)}, + }, nc, "123456789012") - output, err := StopInstances(context.Background(), input, nc, "123456789012") - - require.NoError(t, err) - require.Len(t, output.StoppingInstances, 2) - - // First: success → stopping - assert.Equal(t, goodID, *output.StoppingInstances[0].InstanceId) - assert.Equal(t, "stopping", *output.StoppingInstances[0].CurrentState.Name) - - // Second: failure → still running - assert.Equal(t, badID, *output.StoppingInstances[1].InstanceId) - assert.Equal(t, "running", *output.StoppingInstances[1].CurrentState.Name) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) + assert.Nil(t, output) } diff --git a/spinifex/gateway/ec2/instance/TerminateInstances.go b/spinifex/gateway/ec2/instance/TerminateInstances.go index 0a3b18cff..622a41aeb 100644 --- a/spinifex/gateway/ec2/instance/TerminateInstances.go +++ b/spinifex/gateway/ec2/instance/TerminateInstances.go @@ -21,11 +21,9 @@ type terminateStoppedInstanceRequest struct { InstanceID string `json:"instance_id"` } -// terminateRetrySleep is the backoff seam between NoResponders retries; tests override it. -var terminateRetrySleep = time.Sleep - // ValidateTerminateInstancesInput rejects a nil input with InvalidParameterValue and an empty -// InstanceIds list with MissingParameter. +// InstanceIds list with MissingParameter, and a malformed instance ID with +// InvalidInstanceID.Malformed before any NATS traffic. func ValidateTerminateInstancesInput(input *ec2.TerminateInstancesInput) error { if input == nil { return errors.New(awserrors.ErrorInvalidParameterValue) @@ -33,7 +31,7 @@ func ValidateTerminateInstancesInput(input *ec2.TerminateInstancesInput) error { if len(input.InstanceIds) == 0 { return errors.New(awserrors.ErrorMissingParameter) } - return nil + return validateInstanceIDFormat(input.InstanceIds) } // TerminateInstances sends terminate commands via NATS with stop_instance set to prevent restart. @@ -66,24 +64,7 @@ func TerminateInstances(ctx context.Context, input *ec2.TerminateInstancesInput, continue } - // Retry on ErrNoResponders: per-instance NATS subscription may not have propagated yet after a cluster restart. - subject := fmt.Sprintf("ec2.cmd.%s", instanceID) - var msg *nats.Msg - for attempt := range 3 { - reqMsg := nats.NewMsg(subject) - reqMsg.Data = jsonData - reqMsg.Header.Set(utils.AccountIDHeader, accountID) - utils.InjectTraceContext(ctx, reqMsg.Header) - msg, err = natsConn.RequestMsg(reqMsg, 5*time.Second) - if err == nil || !errors.Is(err, nats.ErrNoResponders) { - break - } - if attempt < 2 { - slog.DebugContext(ctx, "TerminateInstances: No responder on per-instance topic, retrying", - "instance_id", instanceID, "attempt", attempt+1) - terminateRetrySleep(time.Duration(attempt+1) * time.Second) - } - } + msg, err := requestInstanceCmd(ctx, natsConn, instanceID, accountID, jsonData) if err != nil { // If no daemon owns this instance, try the ec2.terminate topic for stopped instances if errors.Is(err, nats.ErrNoResponders) { @@ -99,6 +80,7 @@ func TerminateInstances(ctx context.Context, input *ec2.TerminateInstancesInput, terminateReqMsg.Header.Set(utils.AccountIDHeader, accountID) utils.InjectTraceContext(ctx, terminateReqMsg.Header) terminateMsg, terminateErr := natsConn.RequestMsg(terminateReqMsg, 30*time.Second) + daemonReportedAbsent := false if terminateErr == nil { responseError, parseErr := utils.ValidateErrorPayload(terminateMsg.Data) if parseErr == nil { @@ -107,39 +89,36 @@ func TerminateInstances(ctx context.Context, input *ec2.TerminateInstancesInput, continue } - // Only a genuine absence may fall through to the idempotent - // branch below. Any other refusal is a terminate that did not - // happen, so returning success for it strands a record the - // caller has been told is gone. + // Only a genuine absence may fall through to the bucket check + // below. Any other refusal is a terminate that did not happen, + // so returning success for it strands a record the caller has + // been told is gone. code := aws.StringValue(responseError.Code) if code != awserrors.ErrorInvalidInstanceIDNotFound { slog.ErrorContext(ctx, "TerminateInstances: ec2.terminate refused the instance", "instance_id", instanceID, "code", code) return nil, errors.New(code) } - slog.InfoContext(ctx, "TerminateInstances: instance is not a stopped instance, checking the terminated bucket", + daemonReportedAbsent = true + slog.InfoContext(ctx, "TerminateInstances: instance is not a stopped instance, checking the instance buckets", "instance_id", instanceID) } - // Idempotent terminate (rule #1): no daemon owns the instance and it - // is not a stopped instance. If the terminated-bucket query succeeds - // (KV reachable) the instance is already gone — return success so - // tofu destroy retries converge. KV-health gated: a failed query does - // NOT fabricate success (never trust an empty desired-state we cannot - // read — ADR-0003 §3). - found, queryOK := lookupTerminated(ctx, natsConn, instanceID, accountID) - if found { + // An instance still in the terminated bucket returns 48/48 so repeat + // terminates converge. NotFound needs the daemon's own NotFound plus + // both buckets readable and empty; anything less cannot prove absence. + state, queryOK := classifyUnownedInstance(ctx, natsConn, instanceID, accountID) + if state == bucketStateTerminated { slog.InfoContext(ctx, "TerminateInstances: Instance already terminated", "instance_id", instanceID) stateChanges = append(stateChanges, newStateChange(instanceID, 48, "terminated", 48, "terminated")) continue } - if queryOK { - slog.InfoContext(ctx, "TerminateInstances: Instance absent, terminate is idempotent", "instance_id", instanceID) - stateChanges = append(stateChanges, newStateChange(instanceID, 48, "terminated", 48, "terminated")) - continue + if queryOK && state == bucketStateAbsent && daemonReportedAbsent { + slog.InfoContext(ctx, "TerminateInstances: Instance does not exist", "instance_id", instanceID) + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) } - slog.ErrorContext(ctx, "TerminateInstances: terminated-bucket query failed, cannot confirm absence", - "instance_id", instanceID) + slog.ErrorContext(ctx, "TerminateInstances: cannot confirm instance state", + "instance_id", instanceID, "bucket_state", state, "query_ok", queryOK, "terminate_err", terminateErr) } slog.ErrorContext(ctx, "TerminateInstances: Failed to send command", "instance_id", instanceID, "err", err) @@ -163,40 +142,3 @@ func TerminateInstances(ctx context.Context, input *ec2.TerminateInstancesInput, slog.InfoContext(ctx, "TerminateInstances: Completed", "total_instances", len(stateChanges)) return output, nil } - -// lookupTerminated reports whether an instance exists in the terminated KV -// bucket. queryOK is false when the lookup itself failed (KV unreachable / -// malformed reply), so callers can KV-health gate any idempotent-absence -// decision rather than treating a failed query as "not found". -func lookupTerminated(ctx context.Context, natsConn *nats.Conn, instanceID, accountID string) (found, queryOK bool) { - describeInput := &ec2.DescribeInstancesInput{ - InstanceIds: []*string{&instanceID}, - } - reqData, err := json.Marshal(describeInput) - if err != nil { - slog.WarnContext(ctx, "lookupTerminated: failed to marshal request", "instanceId", instanceID, "err", err) - return false, false - } - reqMsg := nats.NewMsg("ec2.DescribeTerminatedInstances") - reqMsg.Data = reqData - reqMsg.Header.Set(utils.AccountIDHeader, accountID) - utils.InjectTraceContext(ctx, reqMsg.Header) - msg, err := natsConn.RequestMsg(reqMsg, 3*time.Second) - if err != nil { - slog.WarnContext(ctx, "lookupTerminated: failed to query terminated instances", "instanceId", instanceID, "err", err) - return false, false - } - var output ec2.DescribeInstancesOutput - if unmarshalErr := json.Unmarshal(msg.Data, &output); unmarshalErr != nil { - slog.WarnContext(ctx, "lookupTerminated: failed to unmarshal response", "instanceId", instanceID, "err", unmarshalErr) - return false, false - } - for _, res := range output.Reservations { - for _, inst := range res.Instances { - if inst.InstanceId != nil && *inst.InstanceId == instanceID { - return true, true - } - } - } - return false, true -} diff --git a/spinifex/gateway/ec2/instance/TerminateInstances_test.go b/spinifex/gateway/ec2/instance/TerminateInstances_test.go index 5005863e5..c041619de 100644 --- a/spinifex/gateway/ec2/instance/TerminateInstances_test.go +++ b/spinifex/gateway/ec2/instance/TerminateInstances_test.go @@ -54,7 +54,7 @@ func TestTerminateInstances_MultipleInstances(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - ids := []string{"i-001", "i-002", "i-003"} + ids := []string{"i-06794c450e770fbc0", "i-70aee04c61ac4d963", "i-ed828a4ce70b0199c"} for _, id := range ids { nc.Subscribe("ec2.cmd."+id, func(msg *nats.Msg) { @@ -95,7 +95,7 @@ func TestTerminateInstances_NilInstanceIdSkipped(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-valid" + instanceID := "i-476e190637b456de0" nc.Subscribe("ec2.cmd."+instanceID, func(msg *nats.Msg) { msg.Respond([]byte(`{"return":{}}`)) }) @@ -114,7 +114,7 @@ func TestTerminateInstances_NilInstanceIdSkipped(t *testing.T) { func TestTerminateInstances_NATSRequestFails(t *testing.T) { _, nc := startTestNATSServer(t) - instanceID := "i-nosubscriber" + instanceID := "i-d2d5fe795b6b57882" input := &ec2.TerminateInstancesInput{ InstanceIds: []*string{aws.String(instanceID)}, @@ -130,8 +130,8 @@ func TestTerminateInstances_NATSRequestFails(t *testing.T) { func TestTerminateInstances_MixedSuccessAndFailure(t *testing.T) { _, nc := startTestNATSServer(t) - goodID := "i-good" - badID := "i-bad" + goodID := "i-fd00438116c1f2ce2" + badID := "i-587b21c4147f9d9d8" nc.Subscribe("ec2.cmd."+goodID, func(msg *nats.Msg) { msg.Respond([]byte(`{"return":{}}`)) @@ -153,7 +153,7 @@ func TestTerminateInstances_VerifiesQMPAttributes(t *testing.T) { t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-verify" + instanceID := "i-aaca8ef21b7bcf3c1" var receivedCmd types.EC2InstanceCommand nc.Subscribe("ec2.cmd."+instanceID, func(msg *nats.Msg) { @@ -177,7 +177,7 @@ func TestTerminateInstances_VerifiesQMPAttributes(t *testing.T) { func TestTerminateInstances_StoppedInstanceFallback(t *testing.T) { _, nc := startTestNATSServer(t) - instanceID := "i-stopped-term" + instanceID := "i-b727f5c80b8852641" // No ec2.cmd. subscriber — simulate stopped instance with no daemon owning it. // Subscribe to ec2.terminate to handle the fallback. @@ -208,8 +208,8 @@ func TestTerminateInstances_StoppedInstanceFallback(t *testing.T) { func TestTerminateInstances_MixedRunningAndStopped(t *testing.T) { _, nc := startTestNATSServer(t) - runningID := "i-running-mix" - stoppedID := "i-stopped-mix" + runningID := "i-eedceec2e5f2de227" + stoppedID := "i-8d23f24b13d54a30c" // Running instance responds on ec2.cmd. nc.Subscribe("ec2.cmd."+runningID, func(msg *nats.Msg) { @@ -253,7 +253,7 @@ func TestTerminateInstances_MixedRunningAndStopped(t *testing.T) { func TestTerminateInstances_RefusalIsNotReportedAsTerminated(t *testing.T) { _, nc := startTestNATSServer(t) - instanceID := "i-refused-term" + instanceID := "i-7488538f97c0a8654" nc.QueueSubscribe("ec2.terminate", "spinifex-workers", func(msg *nats.Msg) { msg.Respond([]byte(`{"code":"` + awserrors.ErrorIncorrectInstanceState + `","message":"wrong state"}`)) @@ -273,20 +273,41 @@ func TestTerminateInstances_RefusalIsNotReportedAsTerminated(t *testing.T) { assert.Nil(t, output) } -// A genuine absence still converges: the stopped bucket does not hold the -// instance and neither does the terminated bucket, so the terminate is a no-op -// on something already gone. -func TestTerminateInstances_AbsentInstanceStaysIdempotent(t *testing.T) { +// The reported bug: an id the system never issued was answered with a +// fabricated terminated/terminated state change instead of NotFound. +func TestTerminateInstances_NeverExistedIsNotFound(t *testing.T) { + t.Parallel() _, nc := startTestNATSServer(t) - instanceID := "i-absent-term" + instanceID := "i-d419ad3c922169e6e" nc.QueueSubscribe("ec2.terminate", "spinifex-workers", func(msg *nats.Msg) { msg.Respond([]byte(`{"code":"` + awserrors.ErrorInvalidInstanceIDNotFound + `","message":"not found"}`)) }) - nc.Subscribe("ec2.DescribeTerminatedInstances", func(msg *nats.Msg) { - msg.Respond([]byte(`{"Reservations":[]}`)) + serveEmptyBuckets(t, nc) + + output, err := TerminateInstances(context.Background(), &ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) + assert.Nil(t, output) +} + +// A repeat terminate of an instance still in the terminated bucket converges, +// which is what lets a retried destroy finish. +func TestTerminateInstances_AlreadyTerminatedStaysIdempotent(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + instanceID := "i-0aa1bb2cc3dd4ee5f" + + nc.QueueSubscribe("ec2.terminate", "spinifex-workers", func(msg *nats.Msg) { + msg.Respond([]byte(`{"code":"` + awserrors.ErrorInvalidInstanceIDNotFound + `","message":"not found"}`)) }) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances", instanceID) + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") output, err := TerminateInstances(context.Background(), &ec2.TerminateInstancesInput{ InstanceIds: []*string{aws.String(instanceID)}, @@ -294,6 +315,71 @@ func TestTerminateInstances_AbsentInstanceStaysIdempotent(t *testing.T) { require.NoError(t, err) require.Len(t, output.TerminatingInstances, 1) - assert.Equal(t, int64(48), *output.TerminatingInstances[0].CurrentState.Code) - assert.Equal(t, "terminated", *output.TerminatingInstances[0].CurrentState.Name) + sc := output.TerminatingInstances[0] + assert.Equal(t, int64(48), *sc.CurrentState.Code) + assert.Equal(t, "terminated", *sc.CurrentState.Name) + assert.Equal(t, int64(48), *sc.PreviousState.Code) + assert.Equal(t, "terminated", *sc.PreviousState.Name) +} + +// With the buckets unreadable absence cannot be proved, so the call fails +// with neither a fabricated terminate nor a NotFound. +func TestTerminateInstances_BucketsUnreachableIsNeitherSuccessNorNotFound(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + instanceID := "i-0bb1cc2dd3ee4ff50" + + nc.QueueSubscribe("ec2.terminate", "spinifex-workers", func(msg *nats.Msg) { + msg.Respond([]byte(`{"code":"` + awserrors.ErrorInvalidInstanceIDNotFound + `","message":"not found"}`)) + }) + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + + output, err := TerminateInstances(context.Background(), &ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }, nc, "123456789012") + + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) + assert.Nil(t, output) +} + +// NotFound needs the stopped-instance path's own NotFound. When ec2.terminate +// does not answer, empty buckets alone could be hiding a stopped instance. +func TestTerminateInstances_NoStoppedPathAnswerIsNotNotFound(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + serveEmptyBuckets(t, nc) + + _, err := TerminateInstances(context.Background(), &ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String("i-0cc1dd2ee3ff40516")}, + }, nc, "123456789012") + + require.Error(t, err) + assert.NotEqual(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) +} + +func TestTerminateInstances_MixedBatchWithUnknownIDFails(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + runningID := "i-0dd1ee2ff30415263" + unknownID := "i-0ee1ff203142536a7" + + nc.Subscribe("ec2.cmd."+runningID, func(msg *nats.Msg) { + msg.Respond([]byte(`{}`)) + }) + nc.QueueSubscribe("ec2.terminate", "spinifex-workers", func(msg *nats.Msg) { + msg.Respond([]byte(`{"code":"` + awserrors.ErrorInvalidInstanceIDNotFound + `","message":"not found"}`)) + }) + serveEmptyBuckets(t, nc) + + output, err := TerminateInstances(context.Background(), &ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(runningID), aws.String(unknownID)}, + }, nc, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) + assert.Nil(t, output) } diff --git a/spinifex/gateway/ec2/instance/helpers_test.go b/spinifex/gateway/ec2/instance/helpers_test.go index 550a6298b..e6cda578e 100644 --- a/spinifex/gateway/ec2/instance/helpers_test.go +++ b/spinifex/gateway/ec2/instance/helpers_test.go @@ -18,12 +18,12 @@ func startTestNATSServer(t *testing.T) (*server.Server, *nats.Conn) { return testutil.StartTestNATS(t) } -// TestMain neuters the terminate NoResponders backoff for the whole binary, so +// TestMain neuters the ec2.cmd NoResponders backoff for the whole binary, so // retry paths do not burn real seconds. Installed once rather than swapped per // test: the seam is a package global and the tests that need it run in // parallel, so their restores raced each other. func TestMain(m *testing.M) { - terminateRetrySleep = func(time.Duration) {} + cmdRetrySleep = func(time.Duration) {} os.Exit(m.Run()) } diff --git a/spinifex/gateway/ec2/instance/instanceid.go b/spinifex/gateway/ec2/instance/instanceid.go new file mode 100644 index 000000000..823e901e3 --- /dev/null +++ b/spinifex/gateway/ec2/instance/instanceid.go @@ -0,0 +1,101 @@ +package gateway_ec2_instance + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "regexp" + "time" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats.go" +) + +// instanceIDPattern is the AWS instance ID shape: i- followed by 8 (legacy) +// or 17 lowercase hex characters. +var instanceIDPattern = regexp.MustCompile(`^i-([0-9a-f]{8}|[0-9a-f]{17})$`) + +// validateInstanceIDFormat rejects the whole batch with InvalidInstanceID.Malformed +// if any ID is not a well-formed instance ID. Nil entries are skipped, as the +// lifecycle handlers skip them. +func validateInstanceIDFormat(ids []*string) error { + for _, id := range ids { + if id != nil && !instanceIDPattern.MatchString(*id) { + return errors.New(awserrors.ErrorInvalidInstanceIDMalformed) + } + } + return nil +} + +// cmdRetrySleep is the backoff seam between NoResponders retries; tests override it. +var cmdRetrySleep = time.Sleep + +// instanceCmdTimeout bounds each ec2.cmd. request; tests shorten it to +// exercise ErrTimeout without a real wait. +var instanceCmdTimeout = 5 * time.Second + +// requestInstanceCmd sends a command on ec2.cmd., retrying ErrNoResponders +// because the owner's per-instance subscription may not have propagated yet +// after a cluster restart. Only an ErrNoResponders that survives every retry +// suggests no daemon owns the instance. +func requestInstanceCmd(ctx context.Context, natsConn *nats.Conn, instanceID, accountID string, data []byte) (*nats.Msg, error) { + subject := fmt.Sprintf("ec2.cmd.%s", instanceID) + var ( + msg *nats.Msg + err error + ) + for attempt := range 3 { + reqMsg := nats.NewMsg(subject) + reqMsg.Data = data + reqMsg.Header.Set(utils.AccountIDHeader, accountID) + utils.InjectTraceContext(ctx, reqMsg.Header) + msg, err = natsConn.RequestMsg(reqMsg, instanceCmdTimeout) + if err == nil || !errors.Is(err, nats.ErrNoResponders) { + return msg, err + } + if attempt < 2 { + slog.DebugContext(ctx, "No responder on per-instance topic, retrying", + "instance_id", instanceID, "attempt", attempt+1) + cmdRetrySleep(time.Duration(attempt+1) * time.Second) + } + } + return msg, err +} + +// Bucket states reported by classifyUnownedInstance. +const ( + bucketStateAbsent = "" + bucketStateStopped = "stopped" + bucketStateTerminated = "terminated" +) + +// classifyUnownedInstance looks up an instance no daemon answered for in the +// terminated and stopped KV buckets. A hit in either is returned with ok=true. +// Absence is reported (bucketStateAbsent, true) only when both queries succeeded, +// so a failed query never reads as "not found". +func classifyUnownedInstance(ctx context.Context, natsConn *nats.Conn, instanceID, accountID string) (state string, ok bool) { + reqData, err := json.Marshal(&ec2.DescribeInstancesInput{InstanceIds: []*string{&instanceID}}) + if err != nil { + slog.ErrorContext(ctx, "classifyUnownedInstance: failed to marshal request", "instance_id", instanceID, "err", err) + return bucketStateAbsent, false + } + allOK := true + for _, bucket := range []struct{ topic, state string }{ + {"ec2.DescribeTerminatedInstances", bucketStateTerminated}, + {"ec2.DescribeStoppedInstances", bucketStateStopped}, + } { + reservations, queryOK := queryInstanceBucket(ctx, natsConn, bucket.topic, reqData, accountID) + if !queryOK { + allOK = false + continue + } + if len(missingInstanceIDs(reservations, []*string{&instanceID})) == 0 { + return bucket.state, true + } + } + return bucketStateAbsent, allOK +} diff --git a/spinifex/gateway/ec2/instance/instanceid_test.go b/spinifex/gateway/ec2/instance/instanceid_test.go new file mode 100644 index 000000000..dab2ef598 --- /dev/null +++ b/spinifex/gateway/ec2/instance/instanceid_test.go @@ -0,0 +1,169 @@ +package gateway_ec2_instance + +import ( + "context" + "encoding/json" + "sync/atomic" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// serveInstanceBucket answers topic with a describe output holding ids, the +// shape the daemon's stopped/terminated bucket handlers reply with. +func serveInstanceBucket(t *testing.T, nc *nats.Conn, topic string, ids ...string) { + t.Helper() + var instances []*ec2.Instance + for _, id := range ids { + instances = append(instances, &ec2.Instance{InstanceId: aws.String(id)}) + } + out := ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}} + if len(instances) > 0 { + out.Reservations = append(out.Reservations, &ec2.Reservation{Instances: instances}) + } + data, err := json.Marshal(out) + require.NoError(t, err) + _, err = nc.Subscribe(topic, func(msg *nats.Msg) { _ = msg.Respond(data) }) + require.NoError(t, err) +} + +// serveEmptyBuckets makes both instance buckets reachable and empty, the only +// condition under which an unowned instance may be reported NotFound. +func serveEmptyBuckets(t *testing.T, nc *nats.Conn) { + t.Helper() + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances") +} + +func TestValidateInstanceIDFormat(t *testing.T) { + t.Parallel() + valid := []string{"i-0123456789abcdef0", "i-1a2b3c4d"} + for _, id := range valid { + assert.NoError(t, validateInstanceIDFormat([]*string{aws.String(id)}), id) + } + malformed := []string{ + "i-9decec9b6a07590efasdasdsd", + "i-abc", + "i-0123456789ABCDEF0", + "i-0123456789abcdef", + "ami-bb529e224722bd834", + "0123456789abcdef0", + "", + } + for _, id := range malformed { + err := validateInstanceIDFormat([]*string{aws.String(id)}) + require.Error(t, err, id) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, err.Error(), id) + } + err := validateInstanceIDFormat([]*string{aws.String("i-0123456789abcdef0"), aws.String("i-abc")}) + require.Error(t, err, "one malformed id fails the whole batch") + assert.NoError(t, validateInstanceIDFormat([]*string{nil, aws.String("i-0123456789abcdef0")})) +} + +// A malformed id must be rejected before the gateway asks any daemon about it. +func TestLifecycleVerbs_MalformedIDRejectedBeforeNATS(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + + var requests atomic.Int32 + _, err := nc.Subscribe("ec2.>", func(msg *nats.Msg) { + requests.Add(1) + _ = msg.Respond([]byte(`{}`)) + }) + require.NoError(t, err) + require.NoError(t, nc.Flush()) + + ids := []*string{aws.String("i-0123456789abcdef0"), aws.String("i-9decec9b6a07590efasdasdsd")} + ctx := context.Background() + calls := map[string]func() error{ + "terminate": func() error { + _, err := TerminateInstances(ctx, &ec2.TerminateInstancesInput{InstanceIds: ids}, nc, "123456789012") + return err + }, + "stop": func() error { + _, err := StopInstances(ctx, &ec2.StopInstancesInput{InstanceIds: ids}, nc, "123456789012") + return err + }, + "start": func() error { + _, err := StartInstances(ctx, &ec2.StartInstancesInput{InstanceIds: ids}, nc, "123456789012") + return err + }, + "reboot": func() error { + _, err := RebootInstances(ctx, &ec2.RebootInstancesInput{InstanceIds: ids}, nc, "123456789012") + return err + }, + } + for name, call := range calls { + err := call() + require.Error(t, err, name) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, err.Error(), name) + } + require.NoError(t, nc.Flush()) + assert.Zero(t, requests.Load(), "no NATS request may be sent for a batch holding a malformed id") +} + +func TestClassifyUnownedInstance(t *testing.T) { + t.Parallel() + const id = "i-0123456789abcdef0" + ctx := context.Background() + + t.Run("terminated", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances", id) + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + state, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.Equal(t, bucketStateTerminated, state) + assert.True(t, ok) + }) + t.Run("stopped", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances") + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances", id) + state, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.Equal(t, bucketStateStopped, state) + assert.True(t, ok) + }) + t.Run("absent", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveEmptyBuckets(t, nc) + state, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.Equal(t, bucketStateAbsent, state) + assert.True(t, ok) + }) + t.Run("another instance in the bucket is not a hit", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances", "i-0fedcba9876543210") + serveInstanceBucket(t, nc, "ec2.DescribeStoppedInstances") + state, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.Equal(t, bucketStateAbsent, state) + assert.True(t, ok) + }) + t.Run("one bucket unreachable cannot prove absence", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances") + state, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.Equal(t, bucketStateAbsent, state) + assert.False(t, ok) + }) + t.Run("bucket error payload cannot prove absence", func(t *testing.T) { + t.Parallel() + _, nc := startTestNATSServer(t) + serveInstanceBucket(t, nc, "ec2.DescribeTerminatedInstances") + _, err := nc.Subscribe("ec2.DescribeStoppedInstances", func(msg *nats.Msg) { + _ = msg.Respond([]byte(`{"code":"ServerInternal","message":"kv down"}`)) + }) + require.NoError(t, err) + _, ok := classifyUnownedInstance(ctx, nc, id, "123456789012") + assert.False(t, ok) + }) +} diff --git a/spinifex/gateway/ec2/instance/placement_spread_test.go b/spinifex/gateway/ec2/instance/placement_spread_test.go index faefcbd30..c1262104b 100644 --- a/spinifex/gateway/ec2/instance/placement_spread_test.go +++ b/spinifex/gateway/ec2/instance/placement_spread_test.go @@ -143,9 +143,9 @@ func TestDistributeInstancesSpread_OnePerNode(t *testing.T) { map[string]int{"node-1": 4, "node-2": 3, "node-3": 2}, []string{"node-1", "node-2", "node-3"}, map[string][]byte{ - "node-1": spreadReservation(t, "r-spread", "i-n1"), - "node-2": spreadReservation(t, "r-spread2", "i-n2"), - "node-3": spreadReservation(t, "r-spread3", "i-n3"), + "node-1": spreadReservation(t, "r-spread", "i-900cea8dd0184c1ea"), + "node-2": spreadReservation(t, "r-spread2", "i-4985b8a3ce18c08ef"), + "node-3": spreadReservation(t, "r-spread3", "i-4b976aa6829e7cc09"), }, "") reservation, err := distributeInstancesSpread(context.Background(), spreadInput(3, 3), nc, "test-account", "my-spread", 3) @@ -165,9 +165,9 @@ func TestDistributeInstancesSpread_OnePerNode(t *testing.T) { assert.Equal(t, 3, h.reserveInput.MaxCount) assert.Equal(t, map[string][]string{ - "node-1": {"i-n1"}, - "node-2": {"i-n2"}, - "node-3": {"i-n3"}, + "node-1": {"i-900cea8dd0184c1ea"}, + "node-2": {"i-4985b8a3ce18c08ef"}, + "node-3": {"i-4b976aa6829e7cc09"}, }, h.finalizeInput.NodeInstances) assert.Empty(t, h.releaseInputs, "no release when every reserved node launched") @@ -214,7 +214,7 @@ func TestDistributeInstancesSpread_ReserveErrorPropagates(t *testing.T) { launched := false daemonSub, err := nc.Subscribe("ec2.RunInstances.t3.micro.node-1", func(msg *nats.Msg) { launched = true - _ = msg.Respond(spreadReservation(t, "r-x", "i-x")) + _ = msg.Respond(spreadReservation(t, "r-x", "i-5d166bc283d57a85f")) }) require.NoError(t, err) defer daemonSub.Unsubscribe() @@ -236,8 +236,8 @@ func TestDistributeInstancesSpread_PartialSuccessMeetsMinCount(t *testing.T) { map[string]int{"node-1": 2, "node-2": 2, "node-3": 2}, []string{"node-1", "node-2", "node-3"}, map[string][]byte{ - "node-1": spreadReservation(t, "r-p1", "i-p1"), - "node-2": spreadReservation(t, "r-p2", "i-p2"), + "node-1": spreadReservation(t, "r-p1", "i-456a27c936ac90cbe"), + "node-2": spreadReservation(t, "r-p2", "i-a5ac4ef7131e1a178"), "node-3": utils.GenerateErrorPayload(awserrors.ErrorServerInternal), }, "") @@ -248,7 +248,7 @@ func TestDistributeInstancesSpread_PartialSuccessMeetsMinCount(t *testing.T) { h.mu.Lock() defer h.mu.Unlock() - assert.Equal(t, map[string][]string{"node-1": {"i-p1"}, "node-2": {"i-p2"}}, h.finalizeInput.NodeInstances) + assert.Equal(t, map[string][]string{"node-1": {"i-456a27c936ac90cbe"}, "node-2": {"i-a5ac4ef7131e1a178"}}, h.finalizeInput.NodeInstances) // Only the node that failed to launch gets its slot back. require.Len(t, h.releaseInputs, 1) @@ -265,7 +265,7 @@ func TestDistributeInstancesSpread_BelowMinCountRollsBackAndReleases(t *testing. map[string]int{"node-1": 2, "node-2": 2}, []string{"node-1", "node-2"}, map[string][]byte{ - "node-1": spreadReservation(t, "r-b1", "i-b1"), + "node-1": spreadReservation(t, "r-b1", "i-115789d2ae9c27206"), "node-2": utils.GenerateErrorPayload(awserrors.ErrorServerInternal), }, "") @@ -278,7 +278,7 @@ func TestDistributeInstancesSpread_BelowMinCountRollsBackAndReleases(t *testing. h.mu.Lock() defer h.mu.Unlock() assert.False(t, h.finalizeCalled, "a short launch must not be finalized") - assert.Equal(t, []string{"i-b1"}, h.terminatedIDs, "the one launched instance is rolled back") + assert.Equal(t, []string{"i-115789d2ae9c27206"}, h.terminatedIDs, "the one launched instance is rolled back") require.Len(t, h.releaseInputs, 1) assert.Equal(t, []string{"node-1", "node-2"}, h.releaseInputs[0].Nodes, "every reserved node is released, not just the failed one") @@ -314,8 +314,8 @@ func TestDistributeInstancesSpread_FinalizeFailureRollsBack(t *testing.T) { map[string]int{"node-1": 2, "node-2": 2}, []string{"node-1", "node-2"}, map[string][]byte{ - "node-1": spreadReservation(t, "r-f1", "i-f1"), - "node-2": spreadReservation(t, "r-f2", "i-f2"), + "node-1": spreadReservation(t, "r-f1", "i-2ba7fe6e5300e7c3a"), + "node-2": spreadReservation(t, "r-f2", "i-a95c8791805440ee5"), }, awserrors.ErrorServerInternal) _, err := distributeInstancesSpread(context.Background(), spreadInput(2, 2), nc, "test-account", "my-spread", 2) @@ -325,7 +325,7 @@ func TestDistributeInstancesSpread_FinalizeFailureRollsBack(t *testing.T) { h.mu.Lock() defer h.mu.Unlock() - assert.ElementsMatch(t, []string{"i-f1", "i-f2"}, h.terminatedIDs, + assert.ElementsMatch(t, []string{"i-2ba7fe6e5300e7c3a", "i-a95c8791805440ee5"}, h.terminatedIDs, "instances launched before a failed finalize must be terminated") require.Len(t, h.releaseInputs, 1) assert.Equal(t, []string{"node-1", "node-2"}, h.releaseInputs[0].Nodes) diff --git a/tests/e2e/single/negative_test.go b/tests/e2e/single/negative_test.go index 6f8074c9f..0182d3ffb 100644 --- a/tests/e2e/single/negative_test.go +++ b/tests/e2e/single/negative_test.go @@ -36,6 +36,8 @@ import ( // 8p (idempotent) — DeleteKeyPair for unknown key must succeed // 8q InvalidInstanceID.NotFound — ModifyInstanceAttribute on running inst // 8r InvalidInstanceID.NotFound — RebootInstances on unknown id +// 8s InvalidInstanceID.NotFound — Terminate/Stop/StartInstances on unknown id +// 8t InvalidInstanceID.Malformed — lifecycle verbs on a malformed id func runNegativeErrorPaths(t *testing.T, fix *Fixture) { harness.Phase(t, "Single — Negative / Error Path Tests") @@ -298,14 +300,49 @@ func runNegativeErrorPaths(t *testing.T, fix *Fixture) { // 8r: RebootInstances on a non-existent instance id. t.Run("8r_RebootInstanceNotFound", func(t *testing.T) { t.Parallel() - harness.Step(t, "reboot-instances i-nonexistent") + harness.Step(t, "reboot-instances %s", unknownInstanceID) _, err := fix.AWS.EC2.RebootInstances(&ec2.RebootInstancesInput{ - InstanceIds: []*string{aws.String("i-nonexistent")}, + InstanceIds: []*string{aws.String(unknownInstanceID)}, }) harness.AssertAWSError(t, err, "InvalidInstanceID.NotFound") }) + + // 8s: Terminate/Stop/Start on a well-formed id that was never issued must + // not report a state change. + t.Run("8s_LifecycleInstanceNotFound", func(t *testing.T) { + t.Parallel() + ids := []*string{aws.String(unknownInstanceID)} + harness.Step(t, "terminate-instances %s", unknownInstanceID) + _, err := fix.AWS.EC2.TerminateInstances(&ec2.TerminateInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.NotFound") + harness.Step(t, "stop-instances %s", unknownInstanceID) + _, err = fix.AWS.EC2.StopInstances(&ec2.StopInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.NotFound") + harness.Step(t, "start-instances %s", unknownInstanceID) + _, err = fix.AWS.EC2.StartInstances(&ec2.StartInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.NotFound") + }) + + // 8t: an id that is not i- plus 8 or 17 hex characters is malformed. + t.Run("8t_LifecycleInstanceIDMalformed", func(t *testing.T) { + t.Parallel() + const malformedID = "i-9decec9b6a07590efasdasdsd" + ids := []*string{aws.String(malformedID)} + harness.Step(t, "terminate/stop/start/reboot-instances %s", malformedID) + _, err := fix.AWS.EC2.TerminateInstances(&ec2.TerminateInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.Malformed") + _, err = fix.AWS.EC2.StopInstances(&ec2.StopInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.Malformed") + _, err = fix.AWS.EC2.StartInstances(&ec2.StartInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.Malformed") + _, err = fix.AWS.EC2.RebootInstances(&ec2.RebootInstancesInput{InstanceIds: ids}) + harness.AssertAWSError(t, err, "InvalidInstanceID.Malformed") + }) } +// unknownInstanceID is a well-formed instance id that no cluster issues. +const unknownInstanceID = "i-0123456789abcdef0" + // cleanupUnexpectedInstances registers a t.Cleanup that terminates any // instances the gateway accidentally launched from a negative RunInstances // call. Most failures return nil reservations, so this is a no-op in the