From ccd38f24d40533331ae2810c22873ca54920911e Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 6 Jan 2020 10:21:24 -0500 Subject: [PATCH] resource/aws_batch_job_definition: Properly set container_properties into Terraform state and perform drift detection Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/9954 Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/11038 Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`: ``` /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties ``` Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered: ``` --- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s) testing.go:640: Step 0 error: After applying this step, the plan was not empty: DIFF: DESTROY/CREATE: aws_batch_job_definition.test ... omitted for clarity ... container_properties: "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource) ... omitted for clarity ... ``` Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well. Output from acceptance testing: ``` --- PASS: TestAccAWSBatchJobDefinition_basic (14.45s) --- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s) ``` --- .../batch/equivalency/container_properties.go | 76 +++++++ .../equivalency/container_properties_test.go | 203 ++++++++++++++++++ aws/resource_aws_batch_job_definition.go | 33 ++- 3 files changed, 309 insertions(+), 3 deletions(-) create mode 100644 aws/internal/service/batch/equivalency/container_properties.go create mode 100644 aws/internal/service/batch/equivalency/container_properties_test.go diff --git a/aws/internal/service/batch/equivalency/container_properties.go b/aws/internal/service/batch/equivalency/container_properties.go new file mode 100644 index 00000000000..a08d8f2b52c --- /dev/null +++ b/aws/internal/service/batch/equivalency/container_properties.go @@ -0,0 +1,76 @@ +package equivalency + +import ( + "bytes" + "encoding/json" + "log" + "sort" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/private/protocol/json/jsonutil" + "github.com/aws/aws-sdk-go/service/batch" +) + +type containerProperties batch.ContainerProperties + +func (cp *containerProperties) Reduce() error { + // Deal with Environment objects which may be re-ordered in the API + sort.Slice(cp.Environment, func(i, j int) bool { + return aws.StringValue(cp.Environment[i].Name) < aws.StringValue(cp.Environment[j].Name) + }) + + if len(cp.ResourceRequirements) == 0 { + cp.ResourceRequirements = nil + } + + return nil +} + +// EquivalentBatchContainerPropertiesJSON determines equality between two Batch ContainerProperties JSON strings +func EquivalentBatchContainerPropertiesJSON(str1, str2 string) (bool, error) { + if str1 == "" { + str1 = "{}" + } + + if str2 == "" { + str2 = "{}" + } + + var cp1, cp2 containerProperties + + if err := json.Unmarshal([]byte(str1), &cp1); err != nil { + return false, err + } + + if err := cp1.Reduce(); err != nil { + return false, err + } + + canonicalJson1, err := jsonutil.BuildJSON(cp1) + + if err != nil { + return false, err + } + + if err := json.Unmarshal([]byte(str2), &cp2); err != nil { + return false, err + } + + if err := cp2.Reduce(); err != nil { + return false, err + } + + canonicalJson2, err := jsonutil.BuildJSON(cp2) + + if err != nil { + return false, err + } + + equal := bytes.Equal(canonicalJson1, canonicalJson2) + + if !equal { + log.Printf("[DEBUG] Canonical Batch Container Properties JSON are not equal.\nFirst: %s\nSecond: %s\n", canonicalJson1, canonicalJson2) + } + + return equal, nil +} diff --git a/aws/internal/service/batch/equivalency/container_properties_test.go b/aws/internal/service/batch/equivalency/container_properties_test.go new file mode 100644 index 00000000000..16aa6fc9ef2 --- /dev/null +++ b/aws/internal/service/batch/equivalency/container_properties_test.go @@ -0,0 +1,203 @@ +package equivalency + +import ( + "testing" +) + +func TestEquivalentBatchContainerPropertiesJSON(t *testing.T) { + testCases := []struct { + Name string + ApiJson string + ConfigurationJson string + ExpectEquivalent bool + ExpectError bool + }{ + { + Name: "empty", + ApiJson: ``, + ConfigurationJson: ``, + ExpectEquivalent: true, + }, + { + Name: "empty ResourceRequirements", + ApiJson: ` +{ + "command": ["ls", "-la"], + "environment": [ + { + "name": "VARNAME", + "value": "VARVAL" + } + ], + "image": "busybox", + "memory":512, + "mountPoints": [ + { + "containerPath": "/tmp", + "readOnly": false, + "sourceVolume": "tmp" + } + ], + "resourceRequirements": [], + "ulimits": [ + { + "hardLimit": 1024, + "name": "nofile", + "softLimit": 1024 + } + ], + "vcpus": 1, + "volumes": [ + { + "host": { + "sourcePath": "/tmp" + }, + "name": "tmp" + } + ] +} +`, + ConfigurationJson: ` +{ + "command": ["ls", "-la"], + "environment": [ + { + "name": "VARNAME", + "value": "VARVAL" + } + ], + "image": "busybox", + "memory":512, + "mountPoints": [ + { + "containerPath": "/tmp", + "readOnly": false, + "sourceVolume": "tmp" + } + ], + "ulimits": [ + { + "hardLimit": 1024, + "name": "nofile", + "softLimit": 1024 + } + ], + "vcpus": 1, + "volumes": [ + { + "host": { + "sourcePath": "/tmp" + }, + "name": "tmp" + } + ] +} +`, + ExpectEquivalent: true, + }, + { + Name: "reordered Environment", + ApiJson: ` +{ + "command": ["ls", "-la"], + "environment": [ + { + "name": "VARNAME1", + "value": "VARVAL1" + }, + { + "name": "VARNAME2", + "value": "VARVAL2" + } + ], + "image": "busybox", + "memory":512, + "mountPoints": [ + { + "containerPath": "/tmp", + "readOnly": false, + "sourceVolume": "tmp" + } + ], + "resourceRequirements": [], + "ulimits": [ + { + "hardLimit": 1024, + "name": "nofile", + "softLimit": 1024 + } + ], + "vcpus": 1, + "volumes": [ + { + "host": { + "sourcePath": "/tmp" + }, + "name": "tmp" + } + ] +} +`, + ConfigurationJson: ` +{ + "command": ["ls", "-la"], + "environment": [ + { + "name": "VARNAME2", + "value": "VARVAL2" + }, + { + "name": "VARNAME1", + "value": "VARVAL1" + } + ], + "image": "busybox", + "memory":512, + "mountPoints": [ + { + "containerPath": "/tmp", + "readOnly": false, + "sourceVolume": "tmp" + } + ], + "resourceRequirements": [], + "ulimits": [ + { + "hardLimit": 1024, + "name": "nofile", + "softLimit": 1024 + } + ], + "vcpus": 1, + "volumes": [ + { + "host": { + "sourcePath": "/tmp" + }, + "name": "tmp" + } + ] +} +`, + ExpectEquivalent: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + got, err := EquivalentBatchContainerPropertiesJSON(testCase.ConfigurationJson, testCase.ApiJson) + + if err != nil && !testCase.ExpectError { + t.Errorf("got unexpected error: %s", err) + } + + if err == nil && testCase.ExpectError { + t.Errorf("expected error, but received none") + } + + if got != testCase.ExpectEquivalent { + t.Errorf("got %t, expected %t", got, testCase.ExpectEquivalent) + } + }) + } +} diff --git a/aws/resource_aws_batch_job_definition.go b/aws/resource_aws_batch_job_definition.go index f2b5d049fbc..f94bd7573fc 100644 --- a/aws/resource_aws_batch_job_definition.go +++ b/aws/resource_aws_batch_job_definition.go @@ -6,10 +6,12 @@ import ( "encoding/json" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/private/protocol/json/jsonutil" "github.com/aws/aws-sdk-go/service/batch" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/helper/validation" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency" ) func resourceAwsBatchJobDefinition() *schema.Resource { @@ -33,8 +35,12 @@ func resourceAwsBatchJobDefinition() *schema.Resource { json, _ := structure.NormalizeJsonString(v) return json }, - DiffSuppressFunc: suppressEquivalentJsonDiffs, - ValidateFunc: validateAwsBatchJobContainerProperties, + DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { + equal, _ := equivalency.EquivalentBatchContainerPropertiesJSON(old, new) + + return equal + }, + ValidateFunc: validateAwsBatchJobContainerProperties, }, "parameters": { Type: schema.TypeMap, @@ -142,7 +148,17 @@ func resourceAwsBatchJobDefinitionRead(d *schema.ResourceData, meta interface{}) return nil } d.Set("arn", job.JobDefinitionArn) - d.Set("container_properties", job.ContainerProperties) + + containerProperties, err := flattenBatchContainerProperties(job.ContainerProperties) + + if err != nil { + return fmt.Errorf("error converting Batch Container Properties to JSON: %s", err) + } + + if err := d.Set("container_properties", containerProperties); err != nil { + return fmt.Errorf("error setting container_properties: %s", err) + } + d.Set("parameters", aws.StringValueMap(job.Parameters)) if err := d.Set("retry_strategy", flattenBatchRetryStrategy(job.RetryStrategy)); err != nil { @@ -215,6 +231,17 @@ func expandBatchJobContainerProperties(rawProps string) (*batch.ContainerPropert return props, nil } +// Convert batch.ContainerProperties object into its JSON representation +func flattenBatchContainerProperties(containerProperties *batch.ContainerProperties) (string, error) { + b, err := jsonutil.BuildJSON(containerProperties) + + if err != nil { + return "", err + } + + return string(b), nil +} + func expandJobDefinitionParameters(params map[string]interface{}) map[string]*string { var jobParams = make(map[string]*string) for k, v := range params {