From fd4f3bcb495709e1b6b7455ea8f4094354a76405 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 16 Sep 2026 07:39:41 +1000 Subject: [PATCH] fix(ecs): complete Describe read-back for task definitions, services and clusters (#1038) DescribeTaskDefinition now echoes portMappings[].name and runtimePlatform, DescribeServices projects the stored networkConfiguration and enableECSManagedTags, DescribeClusters returns the four instance, task and service counts, and the five list operations return their ARN key as a present empty list instead of omitting it. UpdateService now honours networkConfiguration and enableECSManagedTags so a changed value converges instead of producing a permanent diff. The eleven container runtime fields the task definition also drops are deliberately left absent: nothing between the daemon and the on-host agent carries them, and reporting a container as privileged or read-only-root when nothing enforces it is worse than reporting nothing. Closes: mulga-0t9q2 Closes: mulga-7thn7 Closes: mulga-d8kif Closes: mulga-5z8z9 --- spinifex/handlers/ecs/bus/messages.go | 7 +- spinifex/handlers/ecs/conv.go | 16 +++ spinifex/handlers/ecs/conv_test.go | 18 ++- spinifex/handlers/ecs/instances.go | 6 +- spinifex/handlers/ecs/records.go | 24 +++- spinifex/handlers/ecs/records_test.go | 62 ++++++++++ spinifex/handlers/ecs/service.go | 72 +++++++++-- spinifex/handlers/ecs/service_test.go | 81 +++++++++++++ spinifex/handlers/ecs/services.go | 86 ++++++++----- spinifex/handlers/ecs/services_test.go | 113 ++++++++++++++++++ .../handlers/ecs/taskdef_validation_test.go | 46 +++++++ spinifex/handlers/ecs/tasks.go | 6 +- 12 files changed, 485 insertions(+), 52 deletions(-) create mode 100644 spinifex/handlers/ecs/records_test.go diff --git a/spinifex/handlers/ecs/bus/messages.go b/spinifex/handlers/ecs/bus/messages.go index 668ff0547..4ba50db0e 100644 --- a/spinifex/handlers/ecs/bus/messages.go +++ b/spinifex/handlers/ecs/bus/messages.go @@ -53,11 +53,16 @@ const ( TaskStatusStopped = "STOPPED" ) -// PortMapping is a container port exposed on the host (bridge mode v1). +// PortMapping is a container port exposed on the host (bridge mode v1). Shared +// by the persisted ContainerDef and the AssignContainer wire payload below; Name +// is a pure echo the agent does not read. type PortMapping struct { ContainerPort int `json:"containerPort"` HostPort int `json:"hostPort,omitempty"` Protocol string `json:"protocol,omitempty"` // tcp | udp (default tcp) + // Name is the Service Connect port-mapping name; inert until Service Connect + // is implemented, but harmless to echo (the agent ignores it). + Name string `json:"name,omitempty"` } // AssignContainer is one container the agent must pull and run for a task. diff --git a/spinifex/handlers/ecs/conv.go b/spinifex/handlers/ecs/conv.go index 0532a9cc2..920041b50 100644 --- a/spinifex/handlers/ecs/conv.go +++ b/spinifex/handlers/ecs/conv.go @@ -116,6 +116,7 @@ func containerDefsFromAWS(in []*ecs.ContainerDefinition) []ContainerDef { ContainerPort: int(aws.Int64Value(p.ContainerPort)), HostPort: int(aws.Int64Value(p.HostPort)), Protocol: aws.StringValue(p.Protocol), + Name: aws.StringValue(p.Name), }) } if lc := c.LogConfiguration; lc != nil { @@ -132,6 +133,18 @@ func containerDefsFromAWS(in []*ecs.ContainerDefinition) []ContainerDef { return out } +// runtimePlatformFromAWS maps the SDK runtimePlatform to the persisted subset. +// Pure echo: nothing in v1 enforces CPU architecture or OS family selection. +func runtimePlatformFromAWS(in *ecs.RuntimePlatform) *RuntimePlatformRecord { + if in == nil { + return nil + } + return &RuntimePlatformRecord{ + CPUArchitecture: aws.StringValue(in.CpuArchitecture), + OperatingSystemFamily: aws.StringValue(in.OperatingSystemFamily), + } +} + func (c ContainerDef) toAWS() *ecs.ContainerDefinition { cd := &ecs.ContainerDefinition{ Name: aws.String(c.Name), @@ -164,6 +177,9 @@ func (c ContainerDef) toAWS() *ecs.ContainerDefinition { if p.Protocol != "" { pm.Protocol = aws.String(p.Protocol) } + if p.Name != "" { + pm.Name = aws.String(p.Name) + } cd.PortMappings = append(cd.PortMappings, pm) } if c.LogDriver != "" { diff --git a/spinifex/handlers/ecs/conv_test.go b/spinifex/handlers/ecs/conv_test.go index 96fab0747..3f5e09b44 100644 --- a/spinifex/handlers/ecs/conv_test.go +++ b/spinifex/handlers/ecs/conv_test.go @@ -56,7 +56,7 @@ func TestContainerDefRoundTrip(t *testing.T) { Essential: aws.Bool(true), Command: []*string{aws.String("/bin/sh"), aws.String("-c")}, Environment: []*ecs.KeyValuePair{{Name: aws.String("FOO"), Value: aws.String("bar")}}, - PortMappings: []*ecs.PortMapping{{ContainerPort: aws.Int64(80), HostPort: aws.Int64(8080), Protocol: aws.String("tcp")}}, + PortMappings: []*ecs.PortMapping{{ContainerPort: aws.Int64(80), HostPort: aws.Int64(8080), Protocol: aws.String("tcp"), Name: aws.String("http")}}, }} defs := containerDefsFromAWS(in) require.Len(t, defs, 1) @@ -67,10 +67,12 @@ func TestContainerDefRoundTrip(t *testing.T) { assert.Equal(t, "bar", d.Environment["FOO"]) require.Len(t, d.PortMappings, 1) assert.Equal(t, 8080, d.PortMappings[0].HostPort) + assert.Equal(t, "http", d.PortMappings[0].Name) back := d.toAWS() assert.Equal(t, "registry/web:1", aws.StringValue(back.Image)) assert.Equal(t, int64(80), aws.Int64Value(back.PortMappings[0].ContainerPort)) + assert.Equal(t, "http", aws.StringValue(back.PortMappings[0].Name)) ac := d.toAssignContainer() assert.Equal(t, "web", ac.Name) @@ -78,6 +80,20 @@ func TestContainerDefRoundTrip(t *testing.T) { assert.Equal(t, "bar", ac.Environment["FOO"]) } +// TestContainerDefRoundTrip_PortNameOmittedWhenUnset covers the AWS-parity +// path: a port mapping with no name must not grow one on the way back out. +func TestContainerDefRoundTrip_PortNameOmittedWhenUnset(t *testing.T) { + in := []*ecs.ContainerDefinition{{ + Name: aws.String("web"), Image: aws.String("registry/web:1"), Essential: aws.Bool(true), + PortMappings: []*ecs.PortMapping{{ContainerPort: aws.Int64(80)}}, + }} + defs := containerDefsFromAWS(in) + require.Len(t, defs, 1) + assert.Empty(t, defs[0].PortMappings[0].Name) + back := defs[0].toAWS() + assert.Nil(t, back.PortMappings[0].Name) +} + func TestContainerDefsFromAWS_SkipsNil(t *testing.T) { assert.Empty(t, containerDefsFromAWS([]*ecs.ContainerDefinition{nil})) } diff --git a/spinifex/handlers/ecs/instances.go b/spinifex/handlers/ecs/instances.go index 3afe7d4c3..decd66628 100644 --- a/spinifex/handlers/ecs/instances.go +++ b/spinifex/handlers/ecs/instances.go @@ -114,11 +114,11 @@ func (s *Service) ListContainerInstances(ctx context.Context, input *ecs.ListCon if err != nil { return nil, err } - out := &ecs.ListContainerInstancesOutput{} + arns := make([]string, 0, len(recs)) for i := range recs { - out.ContainerInstanceArns = append(out.ContainerInstanceArns, aws.String(recs[i].ARN)) + arns = append(arns, recs[i].ARN) } - return out, nil + return &ecs.ListContainerInstancesOutput{ContainerInstanceArns: aws.StringSlice(arns)}, nil } // upsertInstance reads-or-creates the instance record, applies mutate, and writes diff --git a/spinifex/handlers/ecs/records.go b/spinifex/handlers/ecs/records.go index b0b2e4ec1..da6fd5282 100644 --- a/spinifex/handlers/ecs/records.go +++ b/spinifex/handlers/ecs/records.go @@ -206,6 +206,14 @@ type ContainerDef struct { // lands in the host journal/file, retrievable per ecs-logging.md. const LogDriverJSONFile = "json-file" +// RuntimePlatformRecord is the persisted subset of an ecs.RuntimePlatform. +// Pure echo, same as RequiresCompatibilities: nothing selects capacity by CPU +// architecture or OS family, but a caller that sets it must read it back. +type RuntimePlatformRecord struct { + CPUArchitecture string `json:"cpuArchitecture,omitempty"` + OperatingSystemFamily string `json:"operatingSystemFamily,omitempty"` +} + // TaskDefRecord is the persisted task definition revision at TaskDefRevKey. type TaskDefRecord struct { Family string `json:"family"` @@ -219,11 +227,12 @@ type TaskDefRecord struct { // Persisted purely so Describe echoes back what Register was given. Only // the EC2 launch type is implemented, but a client that sets this and // reads back an empty list sees permanent drift. - RequiresCompatibilities []string `json:"requiresCompatibilities,omitempty"` - Containers []ContainerDef `json:"containers"` - Status string `json:"status"` - Tags map[string]string `json:"tags,omitempty"` - RegisteredAt time.Time `json:"registeredAt"` + RequiresCompatibilities []string `json:"requiresCompatibilities,omitempty"` + RuntimePlatform *RuntimePlatformRecord `json:"runtimePlatform,omitempty"` + Containers []ContainerDef `json:"containers"` + Status string `json:"status"` + Tags map[string]string `json:"tags,omitempty"` + RegisteredAt time.Time `json:"registeredAt"` } // reservedCPU/reservedMemory sum the task definition's per-container reservations @@ -384,7 +393,10 @@ type ServiceRecord struct { DeploymentID string `json:"deploymentId"` RunningCount int `json:"runningCount"` PendingCount int `json:"pendingCount"` - Tags map[string]string `json:"tags,omitempty"` + // EnableECSManagedTags is persisted purely so Describe echoes back what + // Create was given; nothing in v1 propagates ECS-managed tags onto tasks. + EnableECSManagedTags bool `json:"enableEcsManagedTags,omitempty"` + Tags map[string]string `json:"tags,omitempty"` // Rolling-update configuration (deploymentConfiguration) and its live state. // MinimumHealthyPercent / MaximumPercent gate the rollout; the circuit breaker // trips a failing deployment and optionally rolls back to LastGoodTaskDefARN. diff --git a/spinifex/handlers/ecs/records_test.go b/spinifex/handlers/ecs/records_test.go new file mode 100644 index 000000000..366751669 --- /dev/null +++ b/spinifex/handlers/ecs/records_test.go @@ -0,0 +1,62 @@ +package handlers_ecs + +//test:in-package — these decode persisted records straight into the unexported +// record types and call their unexported toAWS projection, which is the whole +// point: the assertion is that an older KV entry still reads. + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTaskDefRecord_DecodesPreExistingJSON verifies that a TaskDefRecord +// persisted before portMappings[].name and runtimePlatform existed still +// decodes cleanly: every KV entry written by an older daemon build must stay +// readable without a migration. +func TestTaskDefRecord_DecodesPreExistingJSON(t *testing.T) { + legacy := `{ + "family": "app", + "revision": 1, + "arn": "arn:aws:ecs:ap-southeast-2:123456789012:task-definition/app:1", + "status": "ACTIVE", + "containers": [{ + "name": "app", + "image": "registry/app:1", + "essential": true, + "portMappings": [{"containerPort": 80, "hostPort": 8080, "protocol": "tcp"}] + }] + }` + var rec TaskDefRecord + require.NoError(t, json.Unmarshal([]byte(legacy), &rec)) + assert.Nil(t, rec.RuntimePlatform) + require.Len(t, rec.Containers, 1) + require.Len(t, rec.Containers[0].PortMappings, 1) + assert.Empty(t, rec.Containers[0].PortMappings[0].Name) + + // toAWS must not panic or invent a RuntimePlatform for the missing field. + td := rec.toAWS() + assert.Nil(t, td.RuntimePlatform) + require.Len(t, td.ContainerDefinitions, 1) + assert.Nil(t, td.ContainerDefinitions[0].PortMappings[0].Name) +} + +// TestServiceRecord_DecodesPreExistingJSON verifies that a ServiceRecord +// persisted before enableEcsManagedTags existed still decodes cleanly, and +// defaults the new field to false rather than failing to unmarshal. +func TestServiceRecord_DecodesPreExistingJSON(t *testing.T) { + legacy := `{ + "name": "web", + "arn": "arn:aws:ecs:ap-southeast-2:123456789012:service/web/web", + "cluster": "web", + "status": "ACTIVE", + "schedulingStrategy": "REPLICA", + "deploymentId": "d-1" + }` + var rec ServiceRecord + require.NoError(t, json.Unmarshal([]byte(legacy), &rec)) + assert.False(t, rec.EnableECSManagedTags) + assert.Empty(t, rec.Subnets) +} diff --git a/spinifex/handlers/ecs/service.go b/spinifex/handlers/ecs/service.go index c22b49e38..3bd9011c0 100644 --- a/spinifex/handlers/ecs/service.go +++ b/spinifex/handlers/ecs/service.go @@ -282,13 +282,58 @@ func (s *Service) DescribeClusters(ctx context.Context, input *ecs.DescribeClust if err != nil { return nil, err } - if found { - out.Clusters = append(out.Clusters, rec.toAWS()) + if !found { + continue } + c := rec.toAWS() + if err := s.applyClusterCounts(ctx, kv, name, c); err != nil { + return nil, err + } + out.Clusters = append(out.Clusters, c) } return out, nil } +// applyClusterCounts fills the four count fields AWS always returns on a +// cluster, tallied at read time from the existing listers. The separate +// Statistics key/value list, which Include=STATISTICS adds, stays unset. +func (s *Service) applyClusterCounts(ctx context.Context, kv jetstream.KeyValue, cluster string, c *ecs.Cluster) error { + instances, err := s.listInstanceRecords(ctx, kv, cluster) + if err != nil { + return err + } + c.RegisteredContainerInstancesCount = aws.Int64(int64(len(instances))) + + tasks, err := s.listTaskRecords(ctx, kv, cluster) + if err != nil { + return err + } + var running, pending int64 + for i := range tasks { + switch tasks[i].LastStatus { + case TaskStatusRunning: + running++ + case TaskStatusPending: + pending++ + } + } + c.RunningTasksCount = aws.Int64(running) + c.PendingTasksCount = aws.Int64(pending) + + services, err := s.listServiceRecords(ctx, kv, cluster) + if err != nil { + return err + } + var active int64 + for i := range services { + if services[i].Status == ServiceStatusActive { + active++ + } + } + c.ActiveServicesCount = aws.Int64(active) + return nil +} + // ListClusters returns the ARNs of all clusters in the account. func (s *Service) ListClusters(ctx context.Context, _ *ecs.ListClustersInput, accountID string) (*ecs.ListClustersOutput, error) { kv, err := s.bucket(ctx, accountID) @@ -299,7 +344,7 @@ func (s *Service) ListClusters(ctx context.Context, _ *ecs.ListClustersInput, ac if err != nil { return nil, err } - out := &ecs.ListClustersOutput{} + arns := make([]string, 0, len(keys)) for _, k := range keys { if !strings.HasSuffix(k, "/meta") { continue @@ -310,10 +355,10 @@ func (s *Service) ListClusters(ctx context.Context, _ *ecs.ListClustersInput, ac return nil, err } if found { - out.ClusterArns = append(out.ClusterArns, aws.String(rec.ARN)) + arns = append(arns, rec.ARN) } } - return out, nil + return &ecs.ListClustersOutput{ClusterArns: aws.StringSlice(arns)}, nil } func (r *ClusterRecord) toAWS() *ecs.Cluster { @@ -402,6 +447,7 @@ func (s *Service) RegisterTaskDefinition(ctx context.Context, input *ecs.Registe Containers: containerDefsFromAWS(input.ContainerDefinitions), RequiresCompatibilities: aws.StringValueSlice(input.RequiresCompatibilities), + RuntimePlatform: runtimePlatformFromAWS(input.RuntimePlatform), } if err := putJSON(ctx, kv, TaskDefRevKey(family, rev), &rec); err != nil { return nil, err @@ -514,7 +560,7 @@ func (s *Service) ListTaskDefinitions(ctx context.Context, input *ecs.ListTaskDe if err != nil { return nil, err } - out := &ecs.ListTaskDefinitionsOutput{} + arns := make([]string, 0, len(keys)) for _, k := range keys { if !strings.Contains(k, "/revs/") { continue @@ -525,10 +571,10 @@ func (s *Service) ListTaskDefinitions(ctx context.Context, input *ecs.ListTaskDe return nil, err } if found && rec.Status == wantStatus { - out.TaskDefinitionArns = append(out.TaskDefinitionArns, aws.String(rec.ARN)) + arns = append(arns, rec.ARN) } } - return out, nil + return &ecs.ListTaskDefinitionsOutput{TaskDefinitionArns: aws.StringSlice(arns)}, nil } // resolveTaskDef loads the TaskDefRecord named by ref ("family", "family:rev", @@ -603,6 +649,16 @@ func (r *TaskDefRecord) toAWS() *ecs.TaskDefinition { if len(r.RequiresCompatibilities) > 0 { td.RequiresCompatibilities = aws.StringSlice(r.RequiresCompatibilities) } + if r.RuntimePlatform != nil { + rp := &ecs.RuntimePlatform{} + if r.RuntimePlatform.CPUArchitecture != "" { + rp.CpuArchitecture = aws.String(r.RuntimePlatform.CPUArchitecture) + } + if r.RuntimePlatform.OperatingSystemFamily != "" { + rp.OperatingSystemFamily = aws.String(r.RuntimePlatform.OperatingSystemFamily) + } + td.RuntimePlatform = rp + } for _, c := range r.Containers { td.ContainerDefinitions = append(td.ContainerDefinitions, c.toAWS()) } diff --git a/spinifex/handlers/ecs/service_test.go b/spinifex/handlers/ecs/service_test.go index ca56f1754..80e3c6532 100644 --- a/spinifex/handlers/ecs/service_test.go +++ b/spinifex/handlers/ecs/service_test.go @@ -694,6 +694,87 @@ func TestRecordRegister_GPU_CarriesDeviceUUIDs(t *testing.T) { assert.Equal(t, []string{"GPU-aaa", "GPU-bbb"}, rec.GPUIDs) } +// TestService_DescribeClusters_CountsAlwaysPresent verifies the four count +// fields come back on every DescribeClusters, not only under +// Include=STATISTICS, and that they are tallied from the live +// instance/task/service records rather than any persisted counter. AWS returns +// them unconditionally; Include=STATISTICS adds a separate key/value list. +func TestService_DescribeClusters_CountsAlwaysPresent(t *testing.T) { + svc, _, _ := serviceTestRig(t) + _, err := svc.CreateService(context.Background(), &ecs.CreateServiceInput{ + Cluster: aws.String("web"), ServiceName: aws.String("web"), TaskDefinition: aws.String("app"), + DesiredCount: aws.Int64(1), + }, testAccountID) + require.NoError(t, err) + + assertCounts := func(t *testing.T, c *ecs.Cluster) { + t.Helper() + assert.Equal(t, int64(1), aws.Int64Value(c.RegisteredContainerInstancesCount)) + assert.Equal(t, int64(1), aws.Int64Value(c.ActiveServicesCount)) + assert.Equal(t, int64(1), aws.Int64Value(c.PendingTasksCount)) + assert.Equal(t, int64(0), aws.Int64Value(c.RunningTasksCount)) + } + + // No Include at all: this is what the Terraform data source sends, and the + // counts must still be populated. + out, err := svc.DescribeClusters(context.Background(), &ecs.DescribeClustersInput{ + Clusters: []*string{aws.String("web")}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.Clusters, 1) + assertCounts(t, out.Clusters[0]) + + stats, err := svc.DescribeClusters(context.Background(), &ecs.DescribeClustersInput{ + Clusters: []*string{aws.String("web")}, + Include: aws.StringSlice([]string{ecs.ClusterFieldStatistics}), + }, testAccountID) + require.NoError(t, err) + require.Len(t, stats.Clusters, 1) + assertCounts(t, stats.Clusters[0]) +} + +// The following cover the ECS list operations that must return the ARNs key +// as a present, empty list rather than omit it: the SDK's JSON marshaller +// drops a nil slice unconditionally, regardless of the "omitempty" AWS itself +// documents for these fields, so this must build the slice with make(..., 0, +// n) rather than append into a nil field. + +func TestService_ListClusters_EmptyIsPresentNotAbsent(t *testing.T) { + svc, _ := newTestService(t) + out, err := svc.ListClusters(context.Background(), &ecs.ListClustersInput{}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out.ClusterArns) + assert.Empty(t, out.ClusterArns) +} + +func TestService_ListTaskDefinitions_EmptyIsPresentNotAbsent(t *testing.T) { + svc, _ := newTestService(t) + out, err := svc.ListTaskDefinitions(context.Background(), &ecs.ListTaskDefinitionsInput{}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out.TaskDefinitionArns) + assert.Empty(t, out.TaskDefinitionArns) +} + +func TestService_ListTasks_EmptyIsPresentNotAbsent(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + out, err := svc.ListTasks(context.Background(), &ecs.ListTasksInput{Cluster: aws.String("web")}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out.TaskArns) + assert.Empty(t, out.TaskArns) +} + +func TestService_ListContainerInstances_EmptyIsPresentNotAbsent(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + out, err := svc.ListContainerInstances(context.Background(), &ecs.ListContainerInstancesInput{Cluster: aws.String("web")}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out.ContainerInstanceArns) + assert.Empty(t, out.ContainerInstanceArns) +} + func TestAccountIDFromBucket(t *testing.T) { id, ok := accountIDFromBucket(AccountBucketName(testAccountID)) assert.True(t, ok) diff --git a/spinifex/handlers/ecs/services.go b/spinifex/handlers/ecs/services.go index 3978245e5..aed7ec90b 100644 --- a/spinifex/handlers/ecs/services.go +++ b/spinifex/handlers/ecs/services.go @@ -166,23 +166,24 @@ func (s *Service) CreateService(ctx context.Context, input *ecs.CreateServiceInp now := time.Now().UTC() rec := ServiceRecord{ - Name: name, - ARN: ServiceARN(s.region, accountID, cluster, name), - Cluster: cluster, - TaskDefFamily: taskDef.Family, - TaskDefRevision: taskDef.Revision, - TaskDefARN: taskDef.ARN, - DesiredCount: int(aws.Int64Value(input.DesiredCount)), - Status: ServiceStatusActive, - SchedulingStrategy: SchedulingStrategyReplica, - LaunchType: aws.StringValue(input.LaunchType), - NetworkMode: resolveNetworkMode(taskDef), - PlacementStrategy: placementStrategyFromAWS(input.PlacementStrategy), - LoadBalancers: loadBalancersFromAWS(input.LoadBalancers), - DeploymentID: uuid.NewV4().String(), - Tags: tagsToMap(input.Tags), - CreatedAt: now, - UpdatedAt: now, + Name: name, + ARN: ServiceARN(s.region, accountID, cluster, name), + Cluster: cluster, + TaskDefFamily: taskDef.Family, + TaskDefRevision: taskDef.Revision, + TaskDefARN: taskDef.ARN, + DesiredCount: int(aws.Int64Value(input.DesiredCount)), + Status: ServiceStatusActive, + SchedulingStrategy: SchedulingStrategyReplica, + LaunchType: aws.StringValue(input.LaunchType), + NetworkMode: resolveNetworkMode(taskDef), + PlacementStrategy: placementStrategyFromAWS(input.PlacementStrategy), + LoadBalancers: loadBalancersFromAWS(input.LoadBalancers), + DeploymentID: uuid.NewV4().String(), + EnableECSManagedTags: aws.BoolValue(input.EnableECSManagedTags), + Tags: tagsToMap(input.Tags), + CreatedAt: now, + UpdatedAt: now, } if input.NetworkConfiguration != nil && input.NetworkConfiguration.AwsvpcConfiguration != nil { v := input.NetworkConfiguration.AwsvpcConfiguration @@ -224,6 +225,18 @@ func (s *Service) UpdateService(ctx context.Context, input *ecs.UpdateServiceInp if input.DeploymentConfiguration != nil { applyDeploymentConfig(&rec, input.DeploymentConfiguration) } + // Persisted so the next Describe reflects it; only tasks placed after this + // point pick it up, as AWS does not move an existing awsvpc ENI. Without + // this a subnet change is a diff no apply can converge. + if input.NetworkConfiguration != nil && input.NetworkConfiguration.AwsvpcConfiguration != nil { + v := input.NetworkConfiguration.AwsvpcConfiguration + rec.Subnets = awsStringSlice(v.Subnets) + rec.SecurityGroups = awsStringSlice(v.SecurityGroups) + rec.AssignPublicIP = aws.StringValue(v.AssignPublicIp) + } + if input.EnableECSManagedTags != nil { + rec.EnableECSManagedTags = aws.BoolValue(input.EnableECSManagedTags) + } if input.DesiredCount != nil { rec.DesiredCount = int(aws.Int64Value(input.DesiredCount)) } @@ -319,11 +332,11 @@ func (s *Service) ListServices(ctx context.Context, input *ecs.ListServicesInput if err != nil { return nil, err } - out := &ecs.ListServicesOutput{} + arns := make([]string, 0, len(recs)) for i := range recs { - out.ServiceArns = append(out.ServiceArns, aws.String(recs[i].ARN)) + arns = append(arns, recs[i].ARN) } - return out, nil + return &ecs.ListServicesOutput{ServiceArns: aws.StringSlice(arns)}, nil } // --- Reconciliation --- @@ -661,20 +674,33 @@ func loadBalancersFromAWS(in []*ecs.LoadBalancer) []LoadBalancerTarget { func (s *Service) serviceToAWS(accountID string, r *ServiceRecord) *ecs.Service { svc := &ecs.Service{ - ServiceName: aws.String(r.Name), - ServiceArn: aws.String(r.ARN), - ClusterArn: aws.String(ClusterARN(s.region, accountID, r.Cluster)), - Status: aws.String(r.Status), - DesiredCount: aws.Int64(int64(r.DesiredCount)), - RunningCount: aws.Int64(int64(r.RunningCount)), - PendingCount: aws.Int64(int64(r.PendingCount)), - SchedulingStrategy: aws.String(r.SchedulingStrategy), - TaskDefinition: aws.String(r.TaskDefARN), - Tags: tagsToAWS(r.Tags), + ServiceName: aws.String(r.Name), + ServiceArn: aws.String(r.ARN), + ClusterArn: aws.String(ClusterARN(s.region, accountID, r.Cluster)), + Status: aws.String(r.Status), + DesiredCount: aws.Int64(int64(r.DesiredCount)), + RunningCount: aws.Int64(int64(r.RunningCount)), + PendingCount: aws.Int64(int64(r.PendingCount)), + SchedulingStrategy: aws.String(r.SchedulingStrategy), + TaskDefinition: aws.String(r.TaskDefARN), + EnableECSManagedTags: aws.Bool(r.EnableECSManagedTags), + Tags: tagsToAWS(r.Tags), } if r.LaunchType != "" { svc.LaunchType = aws.String(r.LaunchType) } + // Only awsvpc tasks carry a subnet/security-group selection; other modes + // never populate one (RunTask's parseAwsvpcConfig gates it the same way). + if r.NetworkMode == NetworkModeAwsvpc { + awsvpc := &ecs.AwsVpcConfiguration{ + Subnets: aws.StringSlice(r.Subnets), + SecurityGroups: aws.StringSlice(r.SecurityGroups), + } + if r.AssignPublicIP != "" { + awsvpc.AssignPublicIp = aws.String(r.AssignPublicIP) + } + svc.NetworkConfiguration = &ecs.NetworkConfiguration{AwsvpcConfiguration: awsvpc} + } for _, lb := range r.LoadBalancers { svc.LoadBalancers = append(svc.LoadBalancers, &ecs.LoadBalancer{ TargetGroupArn: aws.String(lb.TargetGroupARN), diff --git a/spinifex/handlers/ecs/services_test.go b/spinifex/handlers/ecs/services_test.go index 20739cefd..fabce590d 100644 --- a/spinifex/handlers/ecs/services_test.go +++ b/spinifex/handlers/ecs/services_test.go @@ -198,6 +198,119 @@ func TestService_DescribeAndList(t *testing.T) { assert.Len(t, miss.Failures, 1) } +// TestService_ListServices_EmptyIsPresentNotAbsent covers the ARN-list D-fix +// for ListServices: the key must marshal as an empty list, not vanish. +func TestService_ListServices_EmptyIsPresentNotAbsent(t *testing.T) { + svc, _, _ := serviceTestRig(t) + out, err := svc.ListServices(context.Background(), &ecs.ListServicesInput{Cluster: aws.String("web")}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out.ServiceArns) + assert.Empty(t, out.ServiceArns) +} + +// TestService_CreateService_ProjectsNetworkConfigurationAndManagedTags covers +// the two DescribeServices projection gaps: networkConfiguration is already +// stored and used internally to place tasks, but serviceToAWS never echoed +// it; enableECSManagedTags was not stored at all. +func TestService_CreateService_ProjectsNetworkConfigurationAndManagedTags(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + _, err = svc.RegisterTaskDefinition(context.Background(), &ecs.RegisterTaskDefinitionInput{ + Family: aws.String("app"), + NetworkMode: aws.String(NetworkModeAwsvpc), + ContainerDefinitions: []*ecs.ContainerDefinition{{ + Name: aws.String("app"), Image: aws.String("registry/app:1"), Essential: aws.Bool(true), + }}, + }, testAccountID) + require.NoError(t, err) + + out, err := svc.CreateService(context.Background(), &ecs.CreateServiceInput{ + Cluster: aws.String("web"), ServiceName: aws.String("web"), TaskDefinition: aws.String("app"), + DesiredCount: aws.Int64(0), + EnableECSManagedTags: aws.Bool(true), + NetworkConfiguration: &ecs.NetworkConfiguration{ + AwsvpcConfiguration: &ecs.AwsVpcConfiguration{ + Subnets: aws.StringSlice([]string{"subnet-1", "subnet-2"}), + SecurityGroups: aws.StringSlice([]string{"sg-1"}), + AssignPublicIp: aws.String("ENABLED"), + }, + }, + }, testAccountID) + require.NoError(t, err) + require.True(t, aws.BoolValue(out.Service.EnableECSManagedTags)) + require.NotNil(t, out.Service.NetworkConfiguration) + nc := out.Service.NetworkConfiguration.AwsvpcConfiguration + require.NotNil(t, nc) + assert.ElementsMatch(t, []string{"subnet-1", "subnet-2"}, aws.StringValueSlice(nc.Subnets)) + assert.ElementsMatch(t, []string{"sg-1"}, aws.StringValueSlice(nc.SecurityGroups)) + assert.Equal(t, "ENABLED", aws.StringValue(nc.AssignPublicIp)) +} + +// TestService_UpdateService_ChangesNetworkConfiguration covers the +// cross-cutting constraint: once networkConfiguration is echoed back, +// UpdateService must actually persist a changed value, or a caller changing +// subnets would see a diff no apply could ever converge. +func TestService_UpdateService_ChangesNetworkConfiguration(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.CreateCluster(context.Background(), &ecs.CreateClusterInput{ClusterName: aws.String("web")}, testAccountID) + require.NoError(t, err) + _, err = svc.RegisterTaskDefinition(context.Background(), &ecs.RegisterTaskDefinitionInput{ + Family: aws.String("app"), + NetworkMode: aws.String(NetworkModeAwsvpc), + ContainerDefinitions: []*ecs.ContainerDefinition{{ + Name: aws.String("app"), Image: aws.String("registry/app:1"), Essential: aws.Bool(true), + }}, + }, testAccountID) + require.NoError(t, err) + _, err = svc.CreateService(context.Background(), &ecs.CreateServiceInput{ + Cluster: aws.String("web"), ServiceName: aws.String("web"), TaskDefinition: aws.String("app"), + DesiredCount: aws.Int64(0), + NetworkConfiguration: &ecs.NetworkConfiguration{ + AwsvpcConfiguration: &ecs.AwsVpcConfiguration{Subnets: aws.StringSlice([]string{"subnet-1"})}, + }, + }, testAccountID) + require.NoError(t, err) + + out, err := svc.UpdateService(context.Background(), &ecs.UpdateServiceInput{ + Cluster: aws.String("web"), Service: aws.String("web"), + NetworkConfiguration: &ecs.NetworkConfiguration{ + AwsvpcConfiguration: &ecs.AwsVpcConfiguration{Subnets: aws.StringSlice([]string{"subnet-2"})}, + }, + EnableECSManagedTags: aws.Bool(true), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out.Service.NetworkConfiguration) + assert.Equal(t, []string{"subnet-2"}, + aws.StringValueSlice(out.Service.NetworkConfiguration.AwsvpcConfiguration.Subnets)) + assert.True(t, aws.BoolValue(out.Service.EnableECSManagedTags)) + + // The change persisted, not just the response of this one call. + desc, err := svc.DescribeServices(context.Background(), &ecs.DescribeServicesInput{ + Cluster: aws.String("web"), Services: []*string{aws.String("web")}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.Services, 1) + assert.Equal(t, []string{"subnet-2"}, + aws.StringValueSlice(desc.Services[0].NetworkConfiguration.AwsvpcConfiguration.Subnets)) + assert.True(t, aws.BoolValue(desc.Services[0].EnableECSManagedTags)) + + // An update naming neither field leaves both alone rather than resetting + // them to their zero values. + _, err = svc.UpdateService(context.Background(), &ecs.UpdateServiceInput{ + Cluster: aws.String("web"), Service: aws.String("web"), DesiredCount: aws.Int64(0), + }, testAccountID) + require.NoError(t, err) + desc, err = svc.DescribeServices(context.Background(), &ecs.DescribeServicesInput{ + Cluster: aws.String("web"), Services: []*string{aws.String("web")}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.Services, 1) + assert.Equal(t, []string{"subnet-2"}, + aws.StringValueSlice(desc.Services[0].NetworkConfiguration.AwsvpcConfiguration.Subnets)) + assert.True(t, aws.BoolValue(desc.Services[0].EnableECSManagedTags)) +} + // StartTask places one task per named container instance and assigns it. func TestService_StartTask_PlacesPerInstance(t *testing.T) { svc, _, kv := serviceTestRig(t) diff --git a/spinifex/handlers/ecs/taskdef_validation_test.go b/spinifex/handlers/ecs/taskdef_validation_test.go index 5293f0f86..6827c918e 100644 --- a/spinifex/handlers/ecs/taskdef_validation_test.go +++ b/spinifex/handlers/ecs/taskdef_validation_test.go @@ -77,6 +77,52 @@ func TestRegisterTaskDefinition_EchoesRequiresCompatibilities(t *testing.T) { assert.Equal(t, []string{"EC2"}, aws.StringValueSlice(d.TaskDefinition.RequiresCompatibilities)) } +// TestRegisterTaskDefinition_EchoesRuntimePlatform: runtimePlatform is inert +// (nothing selects capacity by CPU architecture or OS family in v1), but the +// same drift trap as RequiresCompatibilities applies if it is not echoed back. +func TestRegisterTaskDefinition_EchoesRuntimePlatform(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.RegisterTaskDefinition(context.Background(), &ecs.RegisterTaskDefinitionInput{ + Family: aws.String("app"), + RuntimePlatform: &ecs.RuntimePlatform{ + CpuArchitecture: aws.String("ARM64"), + OperatingSystemFamily: aws.String("LINUX"), + }, + ContainerDefinitions: []*ecs.ContainerDefinition{{ + Name: aws.String("app"), Image: aws.String("registry/app:1"), Essential: aws.Bool(true), + }}, + }, testAccountID) + require.NoError(t, err) + + d, err := svc.DescribeTaskDefinition(context.Background(), &ecs.DescribeTaskDefinitionInput{ + TaskDefinition: aws.String("app"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, d.TaskDefinition.RuntimePlatform) + assert.Equal(t, "ARM64", aws.StringValue(d.TaskDefinition.RuntimePlatform.CpuArchitecture)) + assert.Equal(t, "LINUX", aws.StringValue(d.TaskDefinition.RuntimePlatform.OperatingSystemFamily)) +} + +// TestRegisterTaskDefinition_RuntimePlatformOmittedWhenUnset verifies that a +// task definition registered without runtimePlatform describes with a nil +// RuntimePlatform rather than an invented empty struct. +func TestRegisterTaskDefinition_RuntimePlatformOmittedWhenUnset(t *testing.T) { + svc, _ := newTestService(t) + _, err := svc.RegisterTaskDefinition(context.Background(), &ecs.RegisterTaskDefinitionInput{ + Family: aws.String("app"), + ContainerDefinitions: []*ecs.ContainerDefinition{{ + Name: aws.String("app"), Image: aws.String("registry/app:1"), Essential: aws.Bool(true), + }}, + }, testAccountID) + require.NoError(t, err) + + d, err := svc.DescribeTaskDefinition(context.Background(), &ecs.DescribeTaskDefinitionInput{ + TaskDefinition: aws.String("app"), + }, testAccountID) + require.NoError(t, err) + assert.Nil(t, d.TaskDefinition.RuntimePlatform) +} + // TestRunTask_AssignCarriesExecutionRoleAndLogDriver verifies that execution // role plumbed to the agent) and the log driver reaches the assign. func TestRunTask_AssignCarriesExecutionRoleAndLogDriver(t *testing.T) { diff --git a/spinifex/handlers/ecs/tasks.go b/spinifex/handlers/ecs/tasks.go index 6dbe091f4..070d8dead 100644 --- a/spinifex/handlers/ecs/tasks.go +++ b/spinifex/handlers/ecs/tasks.go @@ -252,7 +252,7 @@ func (s *Service) ListTasks(ctx context.Context, input *ecs.ListTasksInput, acco if err != nil { return nil, err } - out := &ecs.ListTasksOutput{} + arns := make([]string, 0, len(keys)) for _, k := range keys { var rec TaskRecord found, err := getJSON(ctx, kv, k, &rec) @@ -260,10 +260,10 @@ func (s *Service) ListTasks(ctx context.Context, input *ecs.ListTasksInput, acco return nil, err } if found { - out.TaskArns = append(out.TaskArns, aws.String(rec.ARN)) + arns = append(arns, rec.ARN) } } - return out, nil + return &ecs.ListTasksOutput{TaskArns: aws.StringSlice(arns)}, nil } func (s *Service) taskToAWS(accountID string, r *TaskRecord) *ecs.Task {