diff --git a/spinifex/gateway/ec2.go b/spinifex/gateway/ec2.go index 4373293ca..442a1ac66 100644 --- a/spinifex/gateway/ec2.go +++ b/spinifex/gateway/ec2.go @@ -117,7 +117,7 @@ func ec2HandlerWithReq[In any](handler func(ctx context.Context, input *In, gw * var ec2Actions = map[string]EC2Handler{ "DescribeInstances": ec2Handler(func(ctx context.Context, input *ec2.DescribeInstancesInput, gw *GatewayConfig, accountID string) (any, error) { - out, err := gateway_ec2_instance.DescribeInstances(ctx, input, gw.NATSConn, gw.DiscoverActiveNodes(ctx), accountID) + out, err := gateway_ec2_instance.DescribeInstancesChecked(ctx, input, gw.NATSConn, gw.DiscoverActiveNodes(ctx), accountID) if err != nil { return out, err } diff --git a/spinifex/gateway/ec2/instance/DescribeInstances.go b/spinifex/gateway/ec2/instance/DescribeInstances.go index 571343d17..50488d7c5 100644 --- a/spinifex/gateway/ec2/instance/DescribeInstances.go +++ b/spinifex/gateway/ec2/instance/DescribeInstances.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" @@ -18,8 +19,15 @@ import ( // DescribeInstances fans out to all nodes via NATS and aggregates the results. // It is lenient: partial results from a slow or unreachable node are returned -// without error, which is correct for user-facing describes. Callers that must -// not act on a partial view (the quota reconcile) use DescribeInstancesForReconcile. +// without error, and an explicitly named instance ID that is simply absent +// from the aggregate is not an error either — the caller sees an empty list. +// That silence is required by internal callers (the IMDS instance lookup, the +// RDS instance-state resolver, the EKS control-plane reconciler) which treat +// "not found" as "not currently running/present" rather than a fault. +// The customer-facing gateway action uses DescribeInstancesChecked instead, +// which adds the InvalidInstanceID.NotFound assertion AWS callers expect. +// Callers that must not act on a partial view at all (the quota reconcile) +// use DescribeInstancesForReconcile. func DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, natsConn *nats.Conn, expectedNodes int, accountID string) (*ec2.DescribeInstancesOutput, error) { reservations, _, firstClient4xx, err := gatherInstances(ctx, input, natsConn, expectedNodes, accountID) if err != nil { @@ -32,6 +40,43 @@ func DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, n return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil } +// DescribeInstancesChecked is the customer-facing variant served by the +// gateway's DescribeInstances action. On top of DescribeInstances' aggregation, +// it asserts InvalidInstanceID.NotFound for any explicitly named instance ID +// absent from the result — but only when the sweep is provably complete +// (every expected node answered, both KV buckets succeeded). A partial sweep +// stays silent, the same as DescribeInstances: a node timing out during the +// fan-out must never turn into a false NotFound for an instance that actually +// exists on that node. A --filters-only query (no explicit IDs) is never +// affected and keeps returning an empty list with no error. +func DescribeInstancesChecked(ctx context.Context, input *ec2.DescribeInstancesInput, natsConn *nats.Conn, expectedNodes int, accountID string) (*ec2.DescribeInstancesOutput, error) { + reservations, complete, firstClient4xx, err := gatherInstances(ctx, input, natsConn, expectedNodes, accountID) + if err != nil { + return nil, err + } + if firstClient4xx != "" && len(reservations) == 0 { + return nil, errors.New(firstClient4xx) + } + + if len(input.InstanceIds) > 0 && complete { + found := make(map[string]bool) + for _, res := range reservations { + for _, inst := range res.Instances { + if inst != nil && inst.InstanceId != nil { + found[*inst.InstanceId] = true + } + } + } + for _, id := range input.InstanceIds { + if id != nil && !found[*id] { + return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound) + } + } + } + + return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil +} + // DescribeInstancesForReconcile is the strict variant the quota reconcile uses. // complete is true only when the sweep observed every expected node and both // instance buckets, so reconcile may lower a counter only from a provably diff --git a/spinifex/gateway/ec2/instance/DescribeInstances_test.go b/spinifex/gateway/ec2/instance/DescribeInstances_test.go index 0c92f8dfb..153b9bf82 100644 --- a/spinifex/gateway/ec2/instance/DescribeInstances_test.go +++ b/spinifex/gateway/ec2/instance/DescribeInstances_test.go @@ -246,6 +246,156 @@ func TestDescribeInstances_EarlyExitWithExpectedNodes(t *testing.T) { assert.Less(t, duration, 2*time.Second) } +// subscribeEmptyInstanceBuckets makes the stopped/terminated KV bucket +// queries in gatherInstances succeed with no instances, so a fan-out that +// resolves fully still isn't marked incomplete just because the buckets have +// nothing to say. +func subscribeEmptyInstanceBuckets(t *testing.T, nc *nats.Conn) { + t.Helper() + for _, topic := range []string{"ec2.DescribeStoppedInstances", "ec2.DescribeTerminatedInstances"} { + _, err := nc.Subscribe(topic, func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}}) + msg.Respond(data) + }) + require.NoError(t, err) + } + nc.Flush() +} + +// TestDescribeInstancesChecked_ExplicitIDNotFound_CompleteSweep covers the +// bead's primary criterion: naming a nonexistent instance ID with a +// fully-answered fan-out returns InvalidInstanceID.NotFound instead of a +// silent empty list. +func TestDescribeInstancesChecked_ExplicitIDNotFound_CompleteSweep(t *testing.T) { + _, nc := startTestNATSServer(t) + subscribeEmptyInstanceBuckets(t, nc) + + _, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{ + Reservations: []*ec2.Reservation{ + { + ReservationId: aws.String("r-1"), + Instances: []*ec2.Instance{{InstanceId: aws.String("i-real")}}, + }, + }, + }) + msg.Respond(data) + }) + require.NoError(t, err) + nc.Flush() + + input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-doesnotexist0000000")}} + _, err = DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012") + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error()) +} + +// TestDescribeInstances_ExplicitIDNotFound_StaysSilent guards the internal +// callers (IMDS lookup, RDS instance-state resolver, EKS control-plane +// reconciler) that call the plain DescribeInstances and rely on "not found" +// meaning an empty list, not an error. Only DescribeInstancesChecked — used +// solely by the gateway's customer-facing action — may assert NotFound. +func TestDescribeInstances_ExplicitIDNotFound_StaysSilent(t *testing.T) { + _, nc := startTestNATSServer(t) + subscribeEmptyInstanceBuckets(t, nc) + + // Mirrors a real node: it filters by the requested InstanceIds itself, so + // a query for an ID it doesn't hold gets an empty reservations list back. + _, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}}) + msg.Respond(data) + }) + require.NoError(t, err) + nc.Flush() + + input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-doesnotexist0000000")}} + output, err := DescribeInstances(context.Background(), input, nc, 1, "123456789012") + + require.NoError(t, err) + require.NotNil(t, output) + assert.Empty(t, output.Reservations) +} + +// TestDescribeInstancesChecked_ExplicitIDFound_CompleteSweep is the positive +// counterpart: the same complete sweep, but the requested ID is present, so +// no error is raised. +func TestDescribeInstancesChecked_ExplicitIDFound_CompleteSweep(t *testing.T) { + _, nc := startTestNATSServer(t) + subscribeEmptyInstanceBuckets(t, nc) + + _, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{ + Reservations: []*ec2.Reservation{ + { + ReservationId: aws.String("r-1"), + Instances: []*ec2.Instance{{InstanceId: aws.String("i-real")}}, + }, + }, + }) + msg.Respond(data) + }) + require.NoError(t, err) + nc.Flush() + + input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-real")}} + output, err := DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012") + + require.NoError(t, err) + require.Len(t, output.Reservations, 1) +} + +// TestDescribeInstancesChecked_FilterOnlyNoMatch_ReturnsEmptyNotNotFound +// covers the bead's second criterion: a --filters query that matches nothing +// must still return rc=0 with an empty list, never NotFound (no instance IDs +// were named). +func TestDescribeInstancesChecked_FilterOnlyNoMatch_ReturnsEmptyNotNotFound(t *testing.T) { + _, nc := startTestNATSServer(t) + subscribeEmptyInstanceBuckets(t, nc) + + _, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}}) + msg.Respond(data) + }) + require.NoError(t, err) + nc.Flush() + + input := &ec2.DescribeInstancesInput{ + Filters: []*ec2.Filter{{Name: aws.String("tag:nope"), Values: []*string{aws.String("nothing")}}}, + } + output, err := DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012") + + require.NoError(t, err) + require.NotNil(t, output) + assert.Empty(t, output.Reservations) +} + +// TestDescribeInstancesChecked_ExplicitIDNotFound_NodeTimeout is the bead's +// subtle third criterion: a node timing out during the fan-out must not +// produce a false NotFound. Only one of two expected nodes answers, so the +// sweep is provably incomplete and the naive found-set check must be +// suppressed. +func TestDescribeInstancesChecked_ExplicitIDNotFound_NodeTimeout(t *testing.T) { + _, nc := startTestNATSServer(t) + subscribeEmptyInstanceBuckets(t, nc) + + // Only one of the two expected nodes has a subscriber; the second never + // answers, so the fan-out hits its 3s deadline with sum.TimedOut=true. + _, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) { + data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}}) + msg.Respond(data) + }) + require.NoError(t, err) + nc.Flush() + + input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-on-the-slow-node")}} + output, err := DescribeInstancesChecked(context.Background(), input, nc, 2, "123456789012") + + require.NoError(t, err, "an incomplete sweep must never assert a false NotFound") + require.NotNil(t, output) + assert.Empty(t, output.Reservations) +} + func TestDescribeInstances_ClosedConnection(t *testing.T) { _, nc := startTestNATSServer(t)