From 77c0e337a65986db77ab2569fc2e5af92c2fd28d Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 22 Jul 2026 10:58:47 +1000 Subject: [PATCH] fix: return Placement and Spot lineage for stopped and terminated instances (#558) --- spinifex/daemon/daemon_handlers_instance.go | 158 ++++--------- spinifex/handlers/ec2/instance/projection.go | 146 ++++++++++++ .../handlers/ec2/instance/projection_test.go | 207 ++++++++++++++++++ .../handlers/ec2/instance/service_impl.go | 92 ++++---- .../ec2/instance/service_impl_test.go | 12 +- 5 files changed, 438 insertions(+), 177 deletions(-) create mode 100644 spinifex/handlers/ec2/instance/projection.go create mode 100644 spinifex/handlers/ec2/instance/projection_test.go diff --git a/spinifex/daemon/daemon_handlers_instance.go b/spinifex/daemon/daemon_handlers_instance.go index 9b5df2632..00a3a0167 100644 --- a/spinifex/daemon/daemon_handlers_instance.go +++ b/spinifex/daemon/daemon_handlers_instance.go @@ -13,7 +13,6 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/filterutil" - handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" handlers_ec2_instance "github.com/mulgadc/spinifex/spinifex/handlers/ec2/instance" "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/utils" @@ -313,18 +312,12 @@ func (d *Daemon) handleEC2DescribeInstances(msg *nats.Msg) { slog.Info("Processing DescribeInstances request from this node", "accountID", accountID) - // Validate and filter instances if specific instance IDs were requested - instanceIDFilter := make(map[string]bool) - if len(describeInstancesInput.InstanceIds) > 0 { - for _, id := range describeInstancesInput.InstanceIds { - if id != nil && *id != "" { - if !strings.HasPrefix(*id, "i-") { - respondWithError(msg, awserrors.ErrorInvalidInstanceIDMalformed) - return - } - instanceIDFilter[*id] = true - } - } + // Validate the requested instance IDs, rejecting malformed ones so this path + // and the stopped/terminated path below enforce the same rule. + instanceIDFilter, err := handlers_ec2_instance.ParseInstanceIDFilter(describeInstancesInput.InstanceIds) + if err != nil { + respondWithError(msg, awserrors.ErrorInvalidInstanceIDMalformed) + return } // Parse filters (returns error for unknown filter names) @@ -373,92 +366,32 @@ func (d *Daemon) handleEC2DescribeInstances(msg *nats.Msg) { reservationMap[resID] = reservation } - // Update the instance state to current state - instanceCopy := *instance.Instance - instanceCopy.State = &ec2.InstanceState{} - - // Populate PublicIpAddress from VM if stored - if instance.PublicIP != "" && instanceCopy.PublicIpAddress == nil { - instanceCopy.PublicIpAddress = aws.String(instance.PublicIP) - } - - // Project the AWS-shaped DNS names from the current IPs, the same - // way PublicIpAddress/State are derived here. Mirrors the records - // the control-plane writer publishes to northstar. - publicDNS, privateDNS := handlers_dns.EC2DNSNames( - d.config.Region, d.dnsBaseDomain, d.dnsInternalDomain, - aws.StringValue(instanceCopy.PublicIpAddress), aws.StringValue(instanceCopy.PrivateIpAddress), - ) - if publicDNS != "" { - instanceCopy.PublicDnsName = aws.String(publicDNS) - } - if privateDNS != "" { - instanceCopy.PrivateDnsName = aws.String(privateDNS) - } - - // Map internal status to AWS state, projecting Spinifex-only states - // (e.g. error -> stopped) so SDK/UI clients see a valid label. - if info, ok := vm.EC2APIState(instance.Status); ok { - instanceCopy.State.SetCode(info.Code) - instanceCopy.State.SetName(info.Name) - } else { + // Project every vm.VM-sourced field onto an API-shaped copy via the + // shared projection, so this path and the stopped/terminated path + // below define the field set exactly once and cannot drift. Running + // instances carry their runtime network (public IP, DNS) and + // capacity reservation; an unmapped status falls back to pending/0. + projected, stateMapped := handlers_ec2_instance.ProjectInstance(instance, handlers_ec2_instance.InstanceProjection{ + Region: d.config.Region, + DNSBaseDomain: d.dnsBaseDomain, + DNSInternalDomain: d.dnsInternalDomain, + AZ: d.config.AZ, + IncludeRuntimeNetwork: true, + FallbackStateCode: 0, + FallbackStateName: "pending", + }) + if !stateMapped { slog.Warn("Instance has unmapped status, reporting as pending", "instanceId", instance.ID, "status", string(instance.Status)) - instanceCopy.State.SetCode(0) - instanceCopy.State.SetName("pending") - } - - // Project IamInstanceProfile from vm.VM (single source of truth - // across Associate/Disassociate/Replace lifecycle). Id is left - // nil — the gateway resolves it via IAMService post-aggregation - // since daemons have no IAM access. Empty Arn clears any stale - // reference left on stored instance.Instance (e.g. after - // Disassociate or auto-clear on terminate). - if instance.IamInstanceProfileArn != "" { - instanceCopy.IamInstanceProfile = &ec2.IamInstanceProfile{ - Arn: aws.String(instance.IamInstanceProfileArn), - } - } else { - instanceCopy.IamInstanceProfile = nil - } - - // Populate Placement if instance belongs to a placement group - if instance.PlacementGroupName != "" { - instanceCopy.Placement = &ec2.Placement{ - GroupName: aws.String(instance.PlacementGroupName), - AvailabilityZone: aws.String(d.config.AZ), - } - } - - // Echo the consumed capacity reservation so targeted-launch - // Terraform converges — without it the instance reports no - // reservation and the plan never settles. - if instance.CapacityReservationId != "" { - instanceCopy.CapacityReservationId = aws.String(instance.CapacityReservationId) - instanceCopy.CapacityReservationSpecification = &ec2.CapacityReservationSpecificationResponse{ - CapacityReservationPreference: aws.String(ec2.CapacityReservationPreferenceOpen), - CapacityReservationTarget: &ec2.CapacityReservationTargetResponse{ - CapacityReservationId: aws.String(instance.CapacityReservationId), - }, - } - } - - // Project spot lineage stamped by the post-launch write-back. - // Both empty for on-demand, so the fields stay absent there. - if instance.InstanceLifecycle != "" { - instanceCopy.InstanceLifecycle = aws.String(instance.InstanceLifecycle) - } - if instance.SpotInstanceRequestId != "" { - instanceCopy.SpotInstanceRequestId = aws.String(instance.SpotInstanceRequestId) } // Apply filters against the fully-built instance copy - if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, &instanceCopy, parsedFilters) { + if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, projected, parsedFilters) { continue } // Add instance to its reservation - reservationMap[resID].Instances = append(reservationMap[resID].Instances, &instanceCopy) + reservationMap[resID].Instances = append(reservationMap[resID].Instances, projected) } } }) @@ -576,11 +509,12 @@ func (d *Daemon) describeInstancesFromKV(msg *nats.Msg, listFn func() ([]*vm.VM, } } - instanceIDFilter := make(map[string]bool) - for _, id := range describeInput.InstanceIds { - if id != nil { - instanceIDFilter[*id] = true - } + // Validate instance IDs identically to the running path — the gateway fans + // out to both, so a malformed ID must fail the same way whichever responds. + instanceIDFilter, err := handlers_ec2_instance.ParseInstanceIDFilter(describeInput.InstanceIds) + if err != nil { + respondWithError(msg, awserrors.ErrorInvalidInstanceIDMalformed) + return } parsedFilters, filterErr := filterutil.ParseFilters(describeInput.Filters, describeInstancesValidFilters) @@ -627,31 +561,23 @@ func (d *Daemon) describeInstancesFromKV(msg *nats.Msg, listFn func() ([]*vm.VM, reservationMap[resID] = reservation } - instanceCopy := *instance.Instance - instanceCopy.State = &ec2.InstanceState{} - if info, ok := vm.EC2APIState(instance.Status); ok { - instanceCopy.State.SetCode(info.Code) - instanceCopy.State.SetName(info.Name) - } else { - instanceCopy.State.SetCode(fallbackCode) - instanceCopy.State.SetName(fallbackName) - } + // Reuse the shared projection so stopped/terminated instances carry the + // same vm.VM-sourced fields as the running path — this is the fix for the + // dropped Placement/Spot lineage. Runtime network (public IP, DNS) and the + // capacity reservation are released on stop, so IncludeRuntimeNetwork stays + // false and they remain absent. The caller's fallback labels the state when + // a stored status has no EC2 mapping. + projected, _ := handlers_ec2_instance.ProjectInstance(instance, handlers_ec2_instance.InstanceProjection{ + AZ: d.config.AZ, + FallbackStateCode: fallbackCode, + FallbackStateName: fallbackName, + }) - // Project IamInstanceProfile from vm.VM (cleared on terminate; - // preserved across stop/start). Mirrors handleEC2DescribeInstances. - if instance.IamInstanceProfileArn != "" { - instanceCopy.IamInstanceProfile = &ec2.IamInstanceProfile{ - Arn: aws.String(instance.IamInstanceProfileArn), - } - } else { - instanceCopy.IamInstanceProfile = nil - } - - if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, &instanceCopy, parsedFilters) { + if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, projected, parsedFilters) { continue } - reservationMap[resID].Instances = append(reservationMap[resID].Instances, &instanceCopy) + reservationMap[resID].Instances = append(reservationMap[resID].Instances, projected) } reservations := make([]*ec2.Reservation, 0, len(reservationMap)) diff --git a/spinifex/handlers/ec2/instance/projection.go b/spinifex/handlers/ec2/instance/projection.go new file mode 100644 index 000000000..d2eed19a4 --- /dev/null +++ b/spinifex/handlers/ec2/instance/projection.go @@ -0,0 +1,146 @@ +package handlers_ec2_instance + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns" + "github.com/mulgadc/spinifex/spinifex/vm" +) + +// InstanceProjection carries the caller-scoped values ProjectInstance needs. +// All fields are plain data so the projection stays pure and table-testable. +type InstanceProjection struct { + Region string + DNSBaseDomain string + DNSInternalDomain string + AZ string + + // IncludeRuntimeNetwork projects the fields that only hold while an instance + // runs: the public IP, its derived DNS names, and the consumed capacity + // reservation. AWS releases all three when an instance stops, so the + // stopped/terminated path leaves IncludeRuntimeNetwork false and they stay + // unset. Placement and Spot lineage survive a stop and are projected either + // way. + IncludeRuntimeNetwork bool + + // FallbackStateCode/Name label the state when vm.EC2APIState has no mapping + // for the instance's internal status. + FallbackStateCode int64 + FallbackStateName string +} + +// ProjectInstance returns the API-shaped copy of v.Instance with every +// vm.VM-sourced field projected onto it. It is pure: it reads only v and cfg and +// mutates a fresh copy, so both live describe paths share one field set and +// cannot drift apart. v.Instance must be non-nil. +// +// stateMapped is false when v.Status has no EC2 equivalent; the caller's +// fallback code/name is applied either way, and the flag lets the running path +// log the gap without the stopped path having to. +func ProjectInstance(v *vm.VM, cfg InstanceProjection) (inst *ec2.Instance, stateMapped bool) { + instanceCopy := *v.Instance + instanceCopy.State = &ec2.InstanceState{} + + // Public IP and its derived DNS names exist only while the instance runs; a + // stopped instance has released the public IP, so the KV path leaves them + // unset. Mirrors the records the control-plane writer publishes to northstar. + if cfg.IncludeRuntimeNetwork { + if v.PublicIP != "" && instanceCopy.PublicIpAddress == nil { + instanceCopy.PublicIpAddress = aws.String(v.PublicIP) + } + + publicDNS, privateDNS := handlers_dns.EC2DNSNames( + cfg.Region, cfg.DNSBaseDomain, cfg.DNSInternalDomain, + aws.StringValue(instanceCopy.PublicIpAddress), aws.StringValue(instanceCopy.PrivateIpAddress), + ) + if publicDNS != "" { + instanceCopy.PublicDnsName = aws.String(publicDNS) + } + if privateDNS != "" { + instanceCopy.PrivateDnsName = aws.String(privateDNS) + } + } + + // Map internal status to AWS state, projecting Spinifex-only states + // (e.g. error -> stopped) so SDK/UI clients see a valid label. An unmapped + // status falls back to the caller's code/name and reports stateMapped false. + if info, ok := vm.EC2APIState(v.Status); ok { + instanceCopy.State.SetCode(info.Code) + instanceCopy.State.SetName(info.Name) + stateMapped = true + } else { + instanceCopy.State.SetCode(cfg.FallbackStateCode) + instanceCopy.State.SetName(cfg.FallbackStateName) + } + + // Project IamInstanceProfile from vm.VM (single source of truth across the + // Associate/Disassociate/Replace lifecycle). Id is left nil — the gateway + // resolves it via IAMService post-aggregation since daemons have no IAM + // access. Empty Arn clears any stale reference left on the stored + // instance.Instance (e.g. after Disassociate or auto-clear on terminate). + if v.IamInstanceProfileArn != "" { + instanceCopy.IamInstanceProfile = &ec2.IamInstanceProfile{ + Arn: aws.String(v.IamInstanceProfileArn), + } + } else { + instanceCopy.IamInstanceProfile = nil + } + + // Placement survives a stop in AWS, so project it regardless of runtime state + // whenever the instance belongs to a placement group. + if v.PlacementGroupName != "" { + instanceCopy.Placement = &ec2.Placement{ + GroupName: aws.String(v.PlacementGroupName), + AvailabilityZone: aws.String(cfg.AZ), + } + } + + // Echo the consumed capacity reservation so targeted-launch Terraform + // converges — without it the instance reports no reservation and the plan + // never settles. Gated to running instances: AWS releases the reservation + // when an instance stops, so a stopped instance must not echo it. + if cfg.IncludeRuntimeNetwork && v.CapacityReservationId != "" { + instanceCopy.CapacityReservationId = aws.String(v.CapacityReservationId) + instanceCopy.CapacityReservationSpecification = &ec2.CapacityReservationSpecificationResponse{ + CapacityReservationPreference: aws.String(ec2.CapacityReservationPreferenceOpen), + CapacityReservationTarget: &ec2.CapacityReservationTargetResponse{ + CapacityReservationId: aws.String(v.CapacityReservationId), + }, + } + } + + // Spot lineage stamped by the post-launch write-back survives a stop, so + // project it regardless of runtime state. Both empty for on-demand, so the + // fields stay absent there. + if v.InstanceLifecycle != "" { + instanceCopy.InstanceLifecycle = aws.String(v.InstanceLifecycle) + } + if v.SpotInstanceRequestId != "" { + instanceCopy.SpotInstanceRequestId = aws.String(v.SpotInstanceRequestId) + } + + return &instanceCopy, stateMapped +} + +// ParseInstanceIDFilter builds the set of instance IDs to filter on, rejecting +// any ID without the "i-" prefix (including the empty string) as malformed. Nil +// entries carry no constraint and are skipped. Empty input yields an empty set, +// which callers treat as "no filter". It is pure so both describe paths validate +// identically. +func ParseInstanceIDFilter(ids []*string) (map[string]bool, error) { + filter := make(map[string]bool) + for _, id := range ids { + if id == nil { + continue + } + if !strings.HasPrefix(*id, "i-") { + return nil, errors.New(awserrors.ErrorInvalidInstanceIDMalformed) + } + filter[*id] = true + } + return filter, nil +} diff --git a/spinifex/handlers/ec2/instance/projection_test.go b/spinifex/handlers/ec2/instance/projection_test.go new file mode 100644 index 000000000..ed60cbc6b --- /dev/null +++ b/spinifex/handlers/ec2/instance/projection_test.go @@ -0,0 +1,207 @@ +package handlers_ec2_instance + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fullVM returns a VM whose every vm.VM-sourced projection field is populated, +// so a projection either carries each through or deliberately drops it. +func fullVM(status vm.InstanceState) *vm.VM { + return &vm.VM{ + ID: "i-abc123", + Status: status, + Instance: &ec2.Instance{ + InstanceId: aws.String("i-abc123"), + PrivateIpAddress: aws.String("10.0.0.5"), + }, + PublicIP: "203.0.113.7", + PlacementGroupName: "pg-1", + IamInstanceProfileArn: "arn:aws:iam::123456789012:instance-profile/role", + CapacityReservationId: "cr-1", + InstanceLifecycle: "spot", + SpotInstanceRequestId: "sir-1", + } +} + +// runningCfg is the projection the running describe path passes. +func runningCfg() InstanceProjection { + return InstanceProjection{ + Region: "us-east-1", + DNSBaseDomain: "example.com", + DNSInternalDomain: "compute.internal", + AZ: "us-east-1a", + IncludeRuntimeNetwork: true, + FallbackStateName: "pending", + } +} + +// stoppedCfg is the projection the stopped/terminated KV path passes. +func stoppedCfg(code int64, name string) InstanceProjection { + return InstanceProjection{ + AZ: "us-east-1a", + FallbackStateCode: code, + FallbackStateName: name, + } +} + +func TestProjectInstance_RunningProjectsAllFields(t *testing.T) { + got, mapped := ProjectInstance(fullVM(vm.StateRunning), runningCfg()) + + require.True(t, mapped) + assert.Equal(t, int64(16), aws.Int64Value(got.State.Code)) + assert.Equal(t, "running", aws.StringValue(got.State.Name)) + + // Runtime network is present for a running instance. + assert.Equal(t, "203.0.113.7", aws.StringValue(got.PublicIpAddress)) + assert.NotEmpty(t, aws.StringValue(got.PublicDnsName)) + assert.NotEmpty(t, aws.StringValue(got.PrivateDnsName)) + + require.NotNil(t, got.Placement) + assert.Equal(t, "pg-1", aws.StringValue(got.Placement.GroupName)) + assert.Equal(t, "us-east-1a", aws.StringValue(got.Placement.AvailabilityZone)) + + require.NotNil(t, got.IamInstanceProfile) + assert.Equal(t, "arn:aws:iam::123456789012:instance-profile/role", aws.StringValue(got.IamInstanceProfile.Arn)) + // Id is left for the gateway to resolve post-aggregation. + assert.Nil(t, got.IamInstanceProfile.Id) + + assert.Equal(t, "cr-1", aws.StringValue(got.CapacityReservationId)) + require.NotNil(t, got.CapacityReservationSpecification) + + assert.Equal(t, "spot", aws.StringValue(got.InstanceLifecycle)) + assert.Equal(t, "sir-1", aws.StringValue(got.SpotInstanceRequestId)) +} + +// TestProjectInstance_StoppedRetainsPlacementAndSpot is the direct bug fix: +// Placement and Spot lineage survive a stop, while the runtime network and the +// capacity reservation — all released by AWS on stop — do not. +func TestProjectInstance_StoppedRetainsPlacementAndSpot(t *testing.T) { + got, mapped := ProjectInstance(fullVM(vm.StateStopped), stoppedCfg(80, "stopped")) + + require.True(t, mapped) + assert.Equal(t, "stopped", aws.StringValue(got.State.Name)) + + // Retained across the stop. + require.NotNil(t, got.Placement) + assert.Equal(t, "pg-1", aws.StringValue(got.Placement.GroupName)) + assert.Equal(t, "spot", aws.StringValue(got.InstanceLifecycle)) + assert.Equal(t, "sir-1", aws.StringValue(got.SpotInstanceRequestId)) + + // Released on stop, so never projected onto a stopped instance. + assert.Nil(t, got.PublicIpAddress) + assert.Nil(t, got.PublicDnsName) + assert.Nil(t, got.PrivateDnsName) + assert.Nil(t, got.CapacityReservationId) + assert.Nil(t, got.CapacityReservationSpecification) +} + +// TestProjectInstance_EmptyIamArnClearsStaleProfile guards the auto-clear: an +// empty ARN on the VM record must wipe any profile left on the stored instance. +func TestProjectInstance_EmptyIamArnClearsStaleProfile(t *testing.T) { + v := fullVM(vm.StateRunning) + v.IamInstanceProfileArn = "" + v.Instance.IamInstanceProfile = &ec2.IamInstanceProfile{Arn: aws.String("arn:aws:iam::123456789012:instance-profile/stale")} + + got, _ := ProjectInstance(v, runningCfg()) + + assert.Nil(t, got.IamInstanceProfile) +} + +// TestProjectInstance_UnmappedStatusUsesFallback covers a stored status with no +// EC2 equivalent: the caller's fallback labels it and stateMapped reports the gap. +func TestProjectInstance_UnmappedStatusUsesFallback(t *testing.T) { + got, mapped := ProjectInstance(fullVM(vm.InstanceState("weird-status")), stoppedCfg(48, "terminated")) + + assert.False(t, mapped) + assert.Equal(t, int64(48), aws.Int64Value(got.State.Code)) + assert.Equal(t, "terminated", aws.StringValue(got.State.Name)) +} + +// TestProjectInstance_DoesNotMutateSource confirms the projection writes only to +// its fresh copy, never back to the stored vm.VM.Instance shared under the lock. +func TestProjectInstance_DoesNotMutateSource(t *testing.T) { + v := fullVM(vm.StateRunning) + + _, _ = ProjectInstance(v, runningCfg()) + + assert.Nil(t, v.Instance.PublicIpAddress) + assert.Nil(t, v.Instance.State) + assert.Nil(t, v.Instance.Placement) +} + +// TestProjectInstance_PathsAgreeOnRetainedFields is the regression guard that +// would have caught the original drift: for one vm.VM, the fields AWS keeps +// across a stop must be identical whether projected by the running path or the +// stopped path. The running-only fields must, conversely, differ. +func TestProjectInstance_PathsAgreeOnRetainedFields(t *testing.T) { + v := fullVM(vm.StateStopped) + + running, _ := ProjectInstance(v, runningCfg()) + stopped, _ := ProjectInstance(v, stoppedCfg(80, "stopped")) + + // Retained-field set must match across both paths. + assert.Equal(t, running.Placement, stopped.Placement) + assert.Equal(t, running.InstanceLifecycle, stopped.InstanceLifecycle) + assert.Equal(t, running.SpotInstanceRequestId, stopped.SpotInstanceRequestId) + assert.Equal(t, running.IamInstanceProfile, stopped.IamInstanceProfile) + + // Runtime-only fields are intentionally path-specific. + assert.NotEqual(t, running.PublicIpAddress, stopped.PublicIpAddress) + assert.NotEqual(t, running.CapacityReservationId, stopped.CapacityReservationId) +} + +func TestParseInstanceIDFilter(t *testing.T) { + tests := []struct { + name string + ids []*string + want map[string]bool + wantErr bool + }{ + { + name: "valid i- IDs", + ids: []*string{aws.String("i-aaa"), aws.String("i-bbb")}, + want: map[string]bool{"i-aaa": true, "i-bbb": true}, + }, + { + name: "malformed ID rejected", + ids: []*string{aws.String("not-an-id")}, + wantErr: true, + }, + { + name: "empty string rejected as malformed", + ids: []*string{aws.String("")}, + wantErr: true, + }, + { + name: "nil entries skipped", + ids: []*string{nil, aws.String("i-aaa"), nil}, + want: map[string]bool{"i-aaa": true}, + }, + { + name: "empty input yields empty filter", + ids: nil, + want: map[string]bool{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseInstanceIDFilter(tt.ids) + if tt.wantErr { + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidInstanceIDMalformed, err.Error()) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index aa315cbf2..418fe0625 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -1754,14 +1754,9 @@ func matchTagValue(tags []*ec2.Tag, values []string) bool { func (s *InstanceServiceImpl) DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, accountID string) (*ec2.DescribeInstancesOutput, error) { slog.InfoContext(ctx, "Processing DescribeInstances request from this node", "accountID", accountID) - instanceIDFilter := make(map[string]bool) - for _, id := range input.InstanceIds { - if id != nil && *id != "" { - if !strings.HasPrefix(*id, "i-") { - return nil, errors.New(awserrors.ErrorInvalidInstanceIDMalformed) - } - instanceIDFilter[*id] = true - } + instanceIDFilter, err := ParseInstanceIDFilter(input.InstanceIds) + if err != nil { + return nil, err } parsedFilters, err := filterutil.ParseFilters(input.Filters, DescribeInstancesValidFilters) @@ -1807,35 +1802,29 @@ func (s *InstanceServiceImpl) DescribeInstances(ctx context.Context, input *ec2. reservationMap[resID] = reservation } - instanceCopy := *instance.Instance - instanceCopy.State = &ec2.InstanceState{} - - if instance.PublicIP != "" && instanceCopy.PublicIpAddress == nil { - instanceCopy.PublicIpAddress = aws.String(instance.PublicIP) - } - - if info, ok := vm.EC2APIState(instance.Status); ok { - instanceCopy.State.SetCode(info.Code) - instanceCopy.State.SetName(info.Name) - } else { + // Project every vm.VM-sourced field via the shared projection, the + // same call the daemon's running path makes, so both define the field + // set once. Running instances include their runtime network and + // capacity reservation; an unmapped status falls back to pending/0. + projected, stateMapped := ProjectInstance(instance, InstanceProjection{ + Region: s.config.Region, + DNSBaseDomain: s.dnsBaseDomain, + DNSInternalDomain: s.dnsInternalDomain, + AZ: s.config.AZ, + IncludeRuntimeNetwork: true, + FallbackStateCode: 0, + FallbackStateName: "pending", + }) + if !stateMapped { slog.WarnContext(ctx, "Instance has unmapped status, reporting as pending", "instanceId", instance.ID, "status", string(instance.Status)) - instanceCopy.State.SetCode(0) - instanceCopy.State.SetName("pending") } - if instance.PlacementGroupName != "" { - instanceCopy.Placement = &ec2.Placement{ - GroupName: aws.String(instance.PlacementGroupName), - AvailabilityZone: aws.String(s.config.AZ), - } - } - - if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, &instanceCopy, parsedFilters) { + if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, projected, parsedFilters) { continue } - reservationMap[resID].Instances = append(reservationMap[resID].Instances, &instanceCopy) + reservationMap[resID].Instances = append(reservationMap[resID].Instances, projected) } }) @@ -1897,11 +1886,9 @@ func (s *InstanceServiceImpl) DescribeTerminatedInstances(ctx context.Context, i } func (s *InstanceServiceImpl) describeInstancesFromKV(ctx context.Context, input *ec2.DescribeInstancesInput, accountID string, listFn func() ([]*vm.VM, error), fallbackCode int64, fallbackName, opName string) (*ec2.DescribeInstancesOutput, error) { - instanceIDFilter := make(map[string]bool) - for _, id := range input.InstanceIds { - if id != nil { - instanceIDFilter[*id] = true - } + instanceIDFilter, err := ParseInstanceIDFilter(input.InstanceIds) + if err != nil { + return nil, err } parsedFilters, filterErr := filterutil.ParseFilters(input.Filters, DescribeInstancesValidFilters) @@ -1946,21 +1933,22 @@ func (s *InstanceServiceImpl) describeInstancesFromKV(ctx context.Context, input reservationMap[resID] = reservation } - instanceCopy := *instance.Instance - instanceCopy.State = &ec2.InstanceState{} - if info, ok := vm.EC2APIState(instance.Status); ok { - instanceCopy.State.SetCode(info.Code) - instanceCopy.State.SetName(info.Name) - } else { - instanceCopy.State.SetCode(fallbackCode) - instanceCopy.State.SetName(fallbackName) - } + // Reuse the shared projection so stopped/terminated instances carry the + // same fields as the running path. Runtime network and the capacity + // reservation are released on stop, so IncludeRuntimeNetwork stays false; + // Placement and Spot lineage survive and are projected. The caller's + // fallback labels the state when a stored status has no EC2 mapping. + projected, _ := ProjectInstance(instance, InstanceProjection{ + AZ: s.config.AZ, + FallbackStateCode: fallbackCode, + FallbackStateName: fallbackName, + }) - if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, &instanceCopy, parsedFilters) { + if len(parsedFilters) > 0 && !instanceMatchesFilters(instance, projected, parsedFilters) { continue } - reservationMap[resID].Instances = append(reservationMap[resID].Instances, &instanceCopy) + reservationMap[resID].Instances = append(reservationMap[resID].Instances, projected) } reservations := make([]*ec2.Reservation, 0, len(reservationMap)) @@ -2814,15 +2802,9 @@ func instanceStatusMatchesFilters(v *vm.VM, is *ec2.InstanceStatus, filters map[ func (s *InstanceServiceImpl) DescribeInstanceStatus(ctx context.Context, input *ec2.DescribeInstanceStatusInput, accountID string) (*ec2.DescribeInstanceStatusOutput, error) { slog.InfoContext(ctx, "Processing DescribeInstanceStatus request from this node", "accountID", accountID) - instanceIDFilter := make(map[string]bool) - for _, id := range input.InstanceIds { - if id == nil || *id == "" { - continue - } - if !strings.HasPrefix(*id, "i-") { - return nil, errors.New(awserrors.ErrorInvalidInstanceIDMalformed) - } - instanceIDFilter[*id] = true + instanceIDFilter, err := ParseInstanceIDFilter(input.InstanceIds) + if err != nil { + return nil, err } parsedFilters, err := filterutil.ParseFilters(input.Filters, DescribeInstanceStatusValidFilters) diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 1e306672f..526cf514f 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -828,7 +828,7 @@ func TestDescribeInstances_OneVisibleInstance(t *testing.T) { }, Instance: &ec2.Instance{InstanceId: aws.String(id), InstanceType: aws.String("t3.micro")}, } - svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{id: v})} + svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{id: v}), config: &config.Config{}} out, err := svc.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{}, owner) require.NoError(t, err) @@ -889,7 +889,7 @@ func TestDescribeInstances_RootSeesManagedSystemVM(t *testing.T) { }, Instance: &ec2.Instance{InstanceId: aws.String("i-lb")}, } - svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{v.ID: v})} + svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{v.ID: v}), config: &config.Config{}} out, err := svc.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{}, utils.GlobalAccountID) require.NoError(t, err) @@ -911,7 +911,7 @@ func TestDescribeInstances_CustomerWorkloadStaysVisible(t *testing.T) { }, Instance: &ec2.Instance{InstanceId: aws.String("i-worker")}, } - svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{v.ID: v})} + svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{v.ID: v}), config: &config.Config{}} out, err := svc.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{}, owner) require.NoError(t, err) @@ -947,7 +947,7 @@ func TestDescribeInstances_FilterByInstanceID(t *testing.T) { svc := &InstanceServiceImpl{vmMgr: mgrWith(map[string]*vm.VM{ keep.ID: keep, drop.ID: drop, - })} + }), config: &config.Config{}} input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-keep")}} out, err := svc.DescribeInstances(context.Background(), input, utils.GlobalAccountID) @@ -1191,7 +1191,7 @@ func TestDescribeStoppedInstances_HappyPath(t *testing.T) { }, }, } - svc := &InstanceServiceImpl{stoppedStore: store} + svc := &InstanceServiceImpl{stoppedStore: store, config: &config.Config{}} out, err := svc.DescribeStoppedInstances(context.Background(), &ec2.DescribeInstancesInput{}, owner) require.NoError(t, err) @@ -1221,7 +1221,7 @@ func TestDescribeTerminatedInstances_HappyPath(t *testing.T) { }, }, } - svc := &InstanceServiceImpl{stoppedStore: store} + svc := &InstanceServiceImpl{stoppedStore: store, config: &config.Config{}} out, err := svc.DescribeTerminatedInstances(context.Background(), &ec2.DescribeInstancesInput{}, owner) require.NoError(t, err)