diff --git a/.changelog/38804.txt b/.changelog/38804.txt new file mode 100644 index 00000000000..2417486ac49 --- /dev/null +++ b/.changelog/38804.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_ecs_task_definition: Prevent lowercasing of the first character of JSON keys in `container_definitions.dockerLabels` +``` \ No newline at end of file diff --git a/internal/json/transform.go b/internal/json/transform.go deleted file mode 100644 index 34ffbe8d551..00000000000 --- a/internal/json/transform.go +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -package json - -import ( - "unicode" - "unicode/utf8" - - "github.com/hashicorp/terraform-provider-aws/internal/json/ujson" -) - -// KeyFirstLower converts the first letter of each key to lowercase. -func KeyFirstLower(in []byte) []byte { - out := make([]byte, 0, len(in)) - - err := ujson.Walk(in, func(_ int, key, value []byte) bool { - // Write to output. - if len(out) != 0 && ujson.ShouldAddComma(value, out[len(out)-1]) { - out = append(out, ',') - } - // key is the raw key of the current object or empty otherwise. - // It can be a double-quoted string or empty. - switch len(key) { - case 0: - case 1, 2: - // Empty key. - out = append(out, key...) - default: - } - if len(key) > 0 { - out = append(out, '"') - r, n := utf8.DecodeRune(key[1:]) - r = unicode.ToLower(r) - low := make([]byte, utf8.RuneLen(r)) - utf8.EncodeRune(low, r) - out = append(out, low...) - out = append(out, key[n+1:]...) - out = append(out, ':') - } - out = append(out, value...) - - return true - }) - - if err != nil { - return nil - } - - return out -} diff --git a/internal/json/transform_test.go b/internal/json/transform_test.go deleted file mode 100644 index e9b311f7f9e..00000000000 --- a/internal/json/transform_test.go +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -package json_test - -import ( - "testing" - - "github.com/hashicorp/terraform-provider-aws/internal/json" -) - -func TestKeyFirstLower(t *testing.T) { - t.Parallel() - - testCases := []struct { - testName string - input string - want string - }{ - { - testName: "empty JSON", - input: "{}", - want: "{}", - }, - { - testName: "single field, lowercase", - input: `{ "key": 42 }`, - want: `{"key":42}`, - }, - { - testName: "single field, uppercase", - input: `{ "Key": 42 }`, - want: `{"key":42}`, - }, - { - testName: "multiple fields", - input: ` -[ - { - "Name": "FIRST", - "Image": "alpine", - "Cpu": 10, - "Memory": 512, - "Essential": true, - "PortMappings": [ - { - "ContainerPort": 80, - "HostPort": 80 - } - ] - } -] - `, - want: `[{"name":"FIRST","image":"alpine","cpu":10,"memory":512,"essential":true,"portMappings":[{"containerPort":80,"hostPort":80}]}]`, - }, - } - - for _, testCase := range testCases { - testCase := testCase - t.Run(testCase.testName, func(t *testing.T) { - t.Parallel() - - if got, want := string(json.KeyFirstLower([]byte(testCase.input))), testCase.want; got != want { - t.Errorf("KeyFirstLower(%q) = %q, want %q", testCase.input, got, want) - } - }) - } -} diff --git a/internal/service/ecs/task_definition_equivalency.go b/internal/service/ecs/container_definitions.go similarity index 71% rename from internal/service/ecs/task_definition_equivalency.go rename to internal/service/ecs/container_definitions.go index 6947d99a884..1dbcbd2bc5d 100644 --- a/internal/service/ecs/task_definition_equivalency.go +++ b/internal/service/ecs/container_definitions.go @@ -4,11 +4,16 @@ package ecs import ( + "fmt" "sort" + _ "unsafe" // Required for go:linkname "github.com/aws/aws-sdk-go-v2/aws" + _ "github.com/aws/aws-sdk-go-v2/service/ecs" // Required for go:linkname 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" + itypes "github.com/hashicorp/terraform-provider-aws/internal/types" ) func containerDefinitionsAreEquivalent(def1, def2 string, isAWSVPC bool) (bool, error) { @@ -143,3 +148,36 @@ func (cd containerDefinitions) orderContainers() { return aws.ToString(cd[i].Name) < aws.ToString(cd[j].Name) }) } + +// 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. +// +//go:linkname serializeContainerDefinitions github.com/aws/aws-sdk-go-v2/service/ecs.awsAwsjson11_serializeDocumentContainerDefinitions +func serializeContainerDefinitions(v []awstypes.ContainerDefinition, value smithyjson.Value) error + +func flattenContainerDefinitions(apiObjects []awstypes.ContainerDefinition) (string, error) { + jsonEncoder := smithyjson.NewEncoder() + err := serializeContainerDefinitions(apiObjects, jsonEncoder.Value) + + if err != nil { + return "", err + } + + return jsonEncoder.String(), nil +} + +func expandContainerDefinitions(tfString string) ([]awstypes.ContainerDefinition, error) { + var apiObjects []awstypes.ContainerDefinition + + if err := tfjson.DecodeFromString(tfString, &apiObjects); err != nil { + return nil, err + } + + for i, apiObject := range apiObjects { + if itypes.IsZero(&apiObject) { + return nil, fmt.Errorf("invalid container definition supplied at index (%d)", i) + } + } + + return apiObjects, nil +} diff --git a/internal/service/ecs/task_definition_equivalency_test.go b/internal/service/ecs/container_definitions_test.go similarity index 100% rename from internal/service/ecs/task_definition_equivalency_test.go rename to internal/service/ecs/container_definitions_test.go diff --git a/internal/service/ecs/task_definition.go b/internal/service/ecs/task_definition.go index b087c41fa85..c4e8a50be4c 100644 --- a/internal/service/ecs/task_definition.go +++ b/internal/service/ecs/task_definition.go @@ -26,10 +26,8 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/errs" "github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag" "github.com/hashicorp/terraform-provider-aws/internal/flex" - tfjson "github.com/hashicorp/terraform-provider-aws/internal/json" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" - itypes "github.com/hashicorp/terraform-provider-aws/internal/types" "github.com/hashicorp/terraform-provider-aws/internal/verify" "github.com/hashicorp/terraform-provider-aws/names" ) @@ -1211,35 +1209,6 @@ func flattenFSxWinVolumeAuthorizationConfig(config *awstypes.FSxWindowsFileServe return items } -func flattenContainerDefinitions(apiObjects []awstypes.ContainerDefinition) (string, error) { - json, err := tfjson.EncodeToBytes(apiObjects) - if err != nil { - return "", err - } - - // Remove empty fields and convert first character of keys to lowercase. - json = tfjson.RemoveEmptyFields(json) - json = tfjson.KeyFirstLower(json) - - return string(json), nil -} - -func expandContainerDefinitions(tfString string) ([]awstypes.ContainerDefinition, error) { - var apiObjects []awstypes.ContainerDefinition - - if err := tfjson.DecodeFromString(tfString, &apiObjects); err != nil { - return nil, err - } - - for i, apiObject := range apiObjects { - if itypes.IsZero(&apiObject) { - return nil, fmt.Errorf("invalid container definition supplied at index (%d)", i) - } - } - - return apiObjects, nil -} - func expandTaskDefinitionEphemeralStorage(config []interface{}) *awstypes.EphemeralStorage { configMap := config[0].(map[string]interface{}) diff --git a/internal/service/ecs/task_definition_test.go b/internal/service/ecs/task_definition_test.go index d4b78e2b316..a8351554c32 100644 --- a/internal/service/ecs/task_definition_test.go +++ b/internal/service/ecs/task_definition_test.go @@ -1337,6 +1337,133 @@ func TestAccECSTaskDefinition_v5590ContainerDefinitionsRegression(t *testing.T) }) } +// https://github.com/hashicorp/terraform-provider-aws/issues/38779. +func TestAccECSTaskDefinition_containerDefinitionEmptyPortMappings(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), + CheckDestroy: testAccCheckTaskDefinitionDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.58.0", + }, + }, + Config: testAccTaskDefinitionConfig_containerDefinitionEmptyPortMappings(rName, "alpine"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].cpu", acctest.Ct10), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].essential", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].image", "alpine"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].memory", "512"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].name", "first"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length([0].portMappings)", acctest.Ct0), + ), + }, + // At v5.59.0 and v5.60.0, JSON keys were returned with leading capital letters. + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.60.0", + }, + }, + Config: testAccTaskDefinitionConfig_containerDefinitionEmptyPortMappings(rName, "jenkins"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].Cpu", acctest.Ct10), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].Essential", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].Image", "jenkins"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].Memory", "512"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].Name", "first"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length([0].PortMappings)", acctest.Ct0), + ), + }, + // At v5.61.0, all empty values were removed. + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.61.0", + }, + }, + Config: testAccTaskDefinitionConfig_containerDefinitionEmptyPortMappings(rName, "nginx"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].cpu", acctest.Ct10), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].essential", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].image", "nginx"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].memory", "512"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].name", "first"), + acctest.CheckResourceAttrJMESNotExists(resourceName, "container_definitions", "[0].portMappings"), + ), + }, + // At v5.62.0, fidelity with v5.58 was restored. + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccTaskDefinitionConfig_containerDefinitionEmptyPortMappings(rName, "alpine"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].cpu", acctest.Ct10), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].essential", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].image", "alpine"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].memory", "512"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].name", "first"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length([0].portMappings)", acctest.Ct0), + ), + }, + }, + }) +} + +// https://github.com/hashicorp/terraform-provider-aws/issues/38782. +func TestAccECSTaskDefinition_containerDefinitionDockerLabels(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_containerDefinitionDockerLabels(rName, "alpine"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckTaskDefinitionExists(ctx, resourceName, &def), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "length(@)", acctest.Ct1), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'PROMETHEUS_EXPORTER_JOB_NAME')", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].dockerLabels.PROMETHEUS_EXPORTER_JOB_NAME", "my-job"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'PROMETHEUS_EXPORTER_PATH')", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].dockerLabels.PROMETHEUS_EXPORTER_PATH", "/prometheus"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'PROMETHEUS_EXPORTER_PORT')", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].dockerLabels.PROMETHEUS_EXPORTER_PORT", "12345"), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'PROMETHEUS_TARGET')", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "[0].dockerLabels.PROMETHEUS_TARGET", acctest.CtTrue), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'pROMETHEUS_EXPORTER_JOB_NAME')", acctest.CtFalse), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'pROMETHEUS_EXPORTER_PATH')", acctest.CtFalse), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'pROMETHEUS_EXPORTER_PORT')", acctest.CtFalse), + acctest.CheckResourceAttrJMES(resourceName, "container_definitions", "keys([0].dockerLabels) | contains(@, 'pROMETHEUS_TARGET')", acctest.CtFalse), + 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 { @@ -3261,3 +3388,52 @@ resource "aws_ecs_task_definition" "test" { } `, rName, image) } + +func testAccTaskDefinitionConfig_containerDefinitionEmptyPortMappings(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 = [] + } + ]) +} +`, rName, image) +} + +func testAccTaskDefinitionConfig_containerDefinitionDockerLabels(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 = [ + { + containerPort = 80 + hostPort = 80 + } + ] + + dockerLabels = { + "PROMETHEUS_TARGET" = "true" + "PROMETHEUS_EXPORTER_PORT" = "12345" + "PROMETHEUS_EXPORTER_PATH" = "/prometheus" + "PROMETHEUS_EXPORTER_JOB_NAME" = "my-job" + } + } + ]) +} +`, rName, image) +}