From 4356f739becde063a4c81260e826d5e17decb02d Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:19:10 -0400 Subject: [PATCH 1/7] Add 'containerDefinitions.compactSparseLists'. --- internal/service/ecs/container_definitions.go | 33 +++++++++++++ .../service/ecs/container_definitions_test.go | 47 +++++++++++++++++++ internal/service/ecs/task_definition.go | 1 + 3 files changed, 81 insertions(+) diff --git a/internal/service/ecs/container_definitions.go b/internal/service/ecs/container_definitions.go index 1dbcbd2bc5d5..02703edb6bf7 100644 --- a/internal/service/ecs/container_definitions.go +++ b/internal/service/ecs/container_definitions.go @@ -13,6 +13,7 @@ import ( awstypes "github.com/aws/aws-sdk-go-v2/service/ecs/types" smithyjson "github.com/aws/smithy-go/encoding/json" tfjson "github.com/hashicorp/terraform-provider-aws/internal/json" + tfslices "github.com/hashicorp/terraform-provider-aws/internal/slices" itypes "github.com/hashicorp/terraform-provider-aws/internal/types" ) @@ -50,6 +51,9 @@ func (cd containerDefinitions) reduce(isAWSVPC bool) { cd.orderEnvironmentVariables() cd.orderSecrets() + // Compact any sparse lists. + cd.compactSparseLists() + for i, def := range cd { // Deal with special fields which have defaults. if def.Essential == nil { @@ -149,6 +153,35 @@ func (cd containerDefinitions) orderContainers() { }) } +func (cd containerDefinitions) compactSparseLists() { + for i, def := range cd { + cd[i].Command = compactSparseList(def.Command) + cd[i].CredentialSpecs = compactSparseList(def.CredentialSpecs) + cd[i].DependsOn = compactSparseList(def.DependsOn) + cd[i].DnsSearchDomains = compactSparseList(def.DnsSearchDomains) + cd[i].DnsServers = compactSparseList(def.DnsServers) + cd[i].DockerSecurityOptions = compactSparseList(def.DockerSecurityOptions) + cd[i].EntryPoint = compactSparseList(def.EntryPoint) + cd[i].Environment = compactSparseList(def.Environment) + cd[i].EnvironmentFiles = compactSparseList(def.EnvironmentFiles) + cd[i].ExtraHosts = compactSparseList(def.ExtraHosts) + cd[i].Links = compactSparseList(def.Links) + cd[i].MountPoints = compactSparseList(def.MountPoints) + cd[i].PortMappings = compactSparseList(def.PortMappings) + cd[i].ResourceRequirements = compactSparseList(def.ResourceRequirements) + cd[i].Secrets = compactSparseList(def.Secrets) + cd[i].SystemControls = compactSparseList(def.SystemControls) + cd[i].Ulimits = compactSparseList(def.Ulimits) + cd[i].VolumesFrom = compactSparseList(def.VolumesFrom) + } +} + +func compactSparseList[S ~[]E, E any](s S) S { + return tfslices.Filter(s, func(e E) bool { + return !itypes.IsZero(&e) + }) +} + // Dirty hack to avoid any backwards compatibility issues with the AWS SDK for Go v2 migration. // Reach down into the SDK and use the same serialization function that the SDK uses. // diff --git a/internal/service/ecs/container_definitions_test.go b/internal/service/ecs/container_definitions_test.go index 2e75d80a1c62..28823822a1cf 100644 --- a/internal/service/ecs/container_definitions_test.go +++ b/internal/service/ecs/container_definitions_test.go @@ -576,3 +576,50 @@ func TestContainerDefinitionsAreEquivalent_missingEnvironmentName(t *testing.T) t.Fatal("Expected definitions to be equal.") } } + +func TestContainerDefinitionsAreEquivalent_sparseArrays(t *testing.T) { + t.Parallel() + + cfgRepresention := ` +[ + { + "name": "wordpress", + "links": [ + "mysql" + ], + "image": "wordpress", + "essential": true, + "portMappings": [ + {} + ], + "memory": 500, + "cpu": 10, + "environment": [null], + "mountPoints": [{"containerPath": null}], + "command": [""] + } +]` + + apiRepresentation := ` +[ + { + "name": "wordpress", + "image": "wordpress", + "cpu": 10, + "memory": 500, + "links": [ + "mysql" + ], + "portMappings": [], + "essential": true + } +]` + + equal, err := containerDefinitionsAreEquivalent(cfgRepresention, apiRepresentation, false) + if err != nil { + t.Fatal(err) + } + if !equal { + t.Fatal("Expected definitions to be equal.") + } +} diff --git a/internal/service/ecs/task_definition.go b/internal/service/ecs/task_definition.go index 88698563c2cb..f4b4012e244d 100644 --- a/internal/service/ecs/task_definition.go +++ b/internal/service/ecs/task_definition.go @@ -93,6 +93,7 @@ func resourceTaskDefinition() *schema.Resource { containerDefinitions(orderedCDs).orderContainers() containerDefinitions(orderedCDs).orderEnvironmentVariables() containerDefinitions(orderedCDs).orderSecrets() + containerDefinitions(orderedCDs).compactSparseLists() unnormalizedJson, _ := flattenContainerDefinitions(orderedCDs) json, _ := structure.NormalizeJsonString(unnormalizedJson) return json From 475e83f7a7810856357693db75bb93fa23b87456 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:28:48 -0400 Subject: [PATCH 2/7] 'compactSparseLists' -> 'compactArrays'. --- internal/service/ecs/container_definitions.go | 36 ++++++++----------- .../service/ecs/container_definitions_test.go | 3 +- internal/service/ecs/task_definition.go | 2 +- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/internal/service/ecs/container_definitions.go b/internal/service/ecs/container_definitions.go index 02703edb6bf7..a7abdd95b9c7 100644 --- a/internal/service/ecs/container_definitions.go +++ b/internal/service/ecs/container_definitions.go @@ -52,7 +52,7 @@ func (cd containerDefinitions) reduce(isAWSVPC bool) { cd.orderSecrets() // Compact any sparse lists. - cd.compactSparseLists() + cd.compactArrays() for i, def := range cd { // Deal with special fields which have defaults. @@ -153,30 +153,24 @@ func (cd containerDefinitions) orderContainers() { }) } -func (cd containerDefinitions) compactSparseLists() { +// compactArrays removes any zero values from the object arrays in the container definitions. +func (cd containerDefinitions) compactArrays() { for i, def := range cd { - cd[i].Command = compactSparseList(def.Command) - cd[i].CredentialSpecs = compactSparseList(def.CredentialSpecs) - cd[i].DependsOn = compactSparseList(def.DependsOn) - cd[i].DnsSearchDomains = compactSparseList(def.DnsSearchDomains) - cd[i].DnsServers = compactSparseList(def.DnsServers) - cd[i].DockerSecurityOptions = compactSparseList(def.DockerSecurityOptions) - cd[i].EntryPoint = compactSparseList(def.EntryPoint) - cd[i].Environment = compactSparseList(def.Environment) - cd[i].EnvironmentFiles = compactSparseList(def.EnvironmentFiles) - cd[i].ExtraHosts = compactSparseList(def.ExtraHosts) - cd[i].Links = compactSparseList(def.Links) - cd[i].MountPoints = compactSparseList(def.MountPoints) - cd[i].PortMappings = compactSparseList(def.PortMappings) - cd[i].ResourceRequirements = compactSparseList(def.ResourceRequirements) - cd[i].Secrets = compactSparseList(def.Secrets) - cd[i].SystemControls = compactSparseList(def.SystemControls) - cd[i].Ulimits = compactSparseList(def.Ulimits) - cd[i].VolumesFrom = compactSparseList(def.VolumesFrom) + cd[i].DependsOn = compactArray(def.DependsOn) + cd[i].Environment = compactArray(def.Environment) + cd[i].EnvironmentFiles = compactArray(def.EnvironmentFiles) + cd[i].ExtraHosts = compactArray(def.ExtraHosts) + cd[i].MountPoints = compactArray(def.MountPoints) + cd[i].PortMappings = compactArray(def.PortMappings) + cd[i].ResourceRequirements = compactArray(def.ResourceRequirements) + cd[i].Secrets = compactArray(def.Secrets) + cd[i].SystemControls = compactArray(def.SystemControls) + cd[i].Ulimits = compactArray(def.Ulimits) + cd[i].VolumesFrom = compactArray(def.VolumesFrom) } } -func compactSparseList[S ~[]E, E any](s S) S { +func compactArray[S ~[]E, E any](s S) S { return tfslices.Filter(s, func(e E) bool { return !itypes.IsZero(&e) }) diff --git a/internal/service/ecs/container_definitions_test.go b/internal/service/ecs/container_definitions_test.go index 28823822a1cf..abd43d8de9cb 100644 --- a/internal/service/ecs/container_definitions_test.go +++ b/internal/service/ecs/container_definitions_test.go @@ -611,7 +611,8 @@ func TestContainerDefinitionsAreEquivalent_sparseArrays(t *testing.T) { "mysql" ], "portMappings": [], - "essential": true + "essential": true, + "command": [""] } ]` diff --git a/internal/service/ecs/task_definition.go b/internal/service/ecs/task_definition.go index f4b4012e244d..d7e5ca880669 100644 --- a/internal/service/ecs/task_definition.go +++ b/internal/service/ecs/task_definition.go @@ -93,7 +93,7 @@ func resourceTaskDefinition() *schema.Resource { containerDefinitions(orderedCDs).orderContainers() containerDefinitions(orderedCDs).orderEnvironmentVariables() containerDefinitions(orderedCDs).orderSecrets() - containerDefinitions(orderedCDs).compactSparseLists() + containerDefinitions(orderedCDs).compactArrays() unnormalizedJson, _ := flattenContainerDefinitions(orderedCDs) json, _ := structure.NormalizeJsonString(unnormalizedJson) return json From 0b7809bf37d2554ca5d8fbc068de1ec848f42801 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:31:19 -0400 Subject: [PATCH 3/7] Add CHANGELOG entry. --- .changelog/#####.txt | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/#####.txt diff --git a/.changelog/#####.txt b/.changelog/#####.txt new file mode 100644 index 000000000000..0d2d1b938fb6 --- /dev/null +++ b/.changelog/#####.txt @@ -0,0 +1,2 @@ +```release-note:bug +resource/aws_ecs_task_definition: Remove `null`s from `container_definition` array fields \ No newline at end of file From 819de9c9e1261da23d538b6206cef1fcbf554724 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:37:12 -0400 Subject: [PATCH 4/7] Add 'TestAccECSTaskDefinition_containerDefinitionNullPortMapping'. --- internal/service/ecs/task_definition_test.go | 47 ++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/internal/service/ecs/task_definition_test.go b/internal/service/ecs/task_definition_test.go index a8351554c323..c334bc506661 100644 --- a/internal/service/ecs/task_definition_test.go +++ b/internal/service/ecs/task_definition_test.go @@ -1464,6 +1464,32 @@ func TestAccECSTaskDefinition_containerDefinitionDockerLabels(t *testing.T) { }) } +// https://github.com/hashicorp/terraform-provider-aws/issues/38779. +func TestAccECSTaskDefinition_containerDefinitionNullPortMapping(t *testing.T) { + ctx := acctest.Context(t) + var def awstypes.TaskDefinition + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_ecs_task_definition.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.ECSServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckTaskDefinitionDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccTaskDefinitionConfig_containerDefinitionNullPortMapping(rName, "alpine"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length([0].portMappings)", acctest.Ct0), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].image", "alpine"), + ), + }, + }, + }) +} + func testAccCheckTaskDefinitionProxyConfiguration(after *awstypes.TaskDefinition, containerName string, proxyType string, ignoredUid string, ignoredGid string, appPorts string, proxyIngressPort string, proxyEgressPort string, egressIgnoredPorts string, egressIgnoredIPs string) resource.TestCheckFunc { @@ -3437,3 +3463,24 @@ resource "aws_ecs_task_definition" "test" { } `, rName, image) } + +func testAccTaskDefinitionConfig_containerDefinitionNullPortMapping(rName, image string) string { + return fmt.Sprintf(` +resource "aws_ecs_task_definition" "test" { + family = %[1]q + container_definitions = jsonencode([ + { + name = "first" + image = %[2]q + cpu = 10 + memory = 512 + essential = true + + portMappings = [ + null + ] + } + ]) +} +`, rName, image) +} From 919661031c514785b7f27aebe01d5a28f229643d Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:46:14 -0400 Subject: [PATCH 5/7] Call 'containerDefinitions.compactArrays' in 'expandContainerDefinitions'. --- internal/service/ecs/container_definitions.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/service/ecs/container_definitions.go b/internal/service/ecs/container_definitions.go index a7abdd95b9c7..7c71b88841da 100644 --- a/internal/service/ecs/container_definitions.go +++ b/internal/service/ecs/container_definitions.go @@ -171,6 +171,10 @@ func (cd containerDefinitions) compactArrays() { } func compactArray[S ~[]E, E any](s S) S { + if len(s) == 0 { + return s + } + return tfslices.Filter(s, func(e E) bool { return !itypes.IsZero(&e) }) @@ -206,5 +210,7 @@ func expandContainerDefinitions(tfString string) ([]awstypes.ContainerDefinition } } + containerDefinitions(apiObjects).compactArrays() + return apiObjects, nil } From 9c7b045239e7d5dd826e1500638409a10c9efa2d Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 13:57:44 -0400 Subject: [PATCH 6/7] Use 'names.USGovCloudPartitionID'. --- internal/service/ecs/task_definition_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/ecs/task_definition_test.go b/internal/service/ecs/task_definition_test.go index c334bc506661..3ba586fe0456 100644 --- a/internal/service/ecs/task_definition_test.go +++ b/internal/service/ecs/task_definition_test.go @@ -609,7 +609,7 @@ func TestAccECSTaskDefinition_fsxWinFileSystem(t *testing.T) { t.Skip("skipping long-running test in short mode") } - if acctest.Partition() == "aws-us-gov" { + if acctest.Partition() == names.USGovCloudPartitionID { t.Skip("Amazon FSx for Windows File Server volumes for ECS tasks are not supported in GovCloud partition") } From 6e422702af131e6dc958a8dfc1119164ccdebc35 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 14 Aug 2024 14:05:35 -0400 Subject: [PATCH 7/7] Correct CHANGELOG entry file name. --- .changelog/{#####.txt => 38870.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{#####.txt => 38870.txt} (100%) diff --git a/.changelog/#####.txt b/.changelog/38870.txt similarity index 100% rename from .changelog/#####.txt rename to .changelog/38870.txt