From 7f723f78ecf63cd72d1139ad903bd8bc1f8be529 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:04:58 -0400 Subject: [PATCH 1/7] batch/job_definition: Fix for ARN updating --- internal/service/batch/job_definition.go | 137 ++++++++++++++++++++++- 1 file changed, 134 insertions(+), 3 deletions(-) diff --git a/internal/service/batch/job_definition.go b/internal/service/batch/job_definition.go index d725226c9d0..f754290b888 100644 --- a/internal/service/batch/job_definition.go +++ b/internal/service/batch/job_definition.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "log" + "reflect" "strings" "github.com/YakDriver/regexache" @@ -16,6 +17,7 @@ import ( "github.com/aws/aws-sdk-go/service/batch" "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" @@ -439,10 +441,139 @@ func ResourceJobDefinition() *schema.Resource { }, }, - CustomizeDiff: verify.SetTagsDiff, + CustomizeDiff: customdiff.Sequence( + jobDefinitionCustomizeDiff, + verify.SetTagsDiff, + ), } } +func jobDefinitionCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { + if d.Id() != "" && needsJobDefUpdate(d) && d.Get("arn").(string) != "" { + d.SetNewComputed("arn") + d.SetNewComputed("revision") + d.SetNewComputed("id") + } + + return nil +} + +// needsJobDefUpdate determines if the Job Definition needs to be updated. This is the +// cost of not forcing new when updates to one argument (eg, container_properties) +// simultaneously impact a computed attribute (eg, arn). The real challenge here is that +// we have to figure out if a change is **GOING** to cause a new revision to be created, +// without the benefit of AWS just telling us. This is necessary because waiting until +// after AWS tells us is too late, and practitioners will need to refresh or worse, get +// an inconsistent plan. BUT, if we SetNewComputed **without** a change, we'll get a +// testing error: "the non-refresh plan was not empty". +func needsJobDefUpdate(d *schema.ResourceDiff) bool { + if d.HasChange("container_properties") { + o, n := d.GetChange("container_properties") + + equivalent, err := EquivalentContainerPropertiesJSON(o.(string), n.(string)) + if err != nil { + return false + } + + if !equivalent { + return true + } + } + + if d.HasChange("node_properties") { + o, n := d.GetChange("node_properties") + + equivalent, err := EquivalentNodePropertiesJSON(o.(string), n.(string)) + if err != nil { + return false + } + + if !equivalent { + return true + } + } + + if d.HasChange("eks_properties") { + o, n := d.GetChange("eks_properties") + if len(o.([]interface{})) == 0 && len(n.([]interface{})) == 0 { + return false + } + + if d.Get("type").(string) != batch.JobDefinitionTypeContainer { + return false + } + + var oeks, neks *batch.EksPodProperties + if len(o.([]interface{})) > 0 { + oProps := o.([]interface{})[0].(map[string]interface{}) + if opodProps, ok := oProps["pod_properties"].([]interface{}); ok && len(opodProps) > 0 { + oeks = expandEKSPodProperties(opodProps[0].(map[string]interface{})) + } + } + + if len(n.([]interface{})) > 0 { + nProps := n.([]interface{})[0].(map[string]interface{}) + if npodProps, ok := nProps["pod_properties"].([]interface{}); ok && len(npodProps) > 0 { + neks = expandEKSPodProperties(npodProps[0].(map[string]interface{})) + } + } + + return !reflect.DeepEqual(oeks, neks) + } + + if d.HasChange("retry_strategy") { + o, n := d.GetChange("retry_strategy") + if len(o.([]interface{})) == 0 && len(n.([]interface{})) == 0 { + return false + } + + var ors, nrs *batch.RetryStrategy + if len(o.([]interface{})) > 0 { + oProps := o.([]interface{})[0].(map[string]interface{}) + ors = expandRetryStrategy(oProps) + } + + if len(n.([]interface{})) > 0 { + nProps := n.([]interface{})[0].(map[string]interface{}) + nrs = expandRetryStrategy(nProps) + } + + return !reflect.DeepEqual(ors, nrs) + } + + if d.HasChange("timeout") { + o, n := d.GetChange("timeout") + if len(o.([]interface{})) == 0 && len(n.([]interface{})) == 0 { + return false + } + + var ors, nrs *batch.JobTimeout + if len(o.([]interface{})) > 0 { + oProps := o.([]interface{})[0].(map[string]interface{}) + ors = expandJobTimeout(oProps) + } + + if len(n.([]interface{})) > 0 { + nProps := n.([]interface{})[0].(map[string]interface{}) + nrs = expandJobTimeout(nProps) + } + + return !reflect.DeepEqual(ors, nrs) + } + + if d.HasChanges( + "propagate_tags", + "parameters", + "platform_capabilities", + "scheduling_priority", + "type", + ) { + return true + } + + return false +} + func resourceJobDefinitionCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).BatchConn(ctx) @@ -684,7 +815,6 @@ func resourceJobDefinitionUpdate(ctx context.Context, d *schema.ResourceData, me } jd, err := conn.RegisterJobDefinitionWithContext(ctx, input) - if err != nil { return sdkdiag.AppendErrorf(diags, "updating Batch Job Definition (%s): %s", name, err) } @@ -693,6 +823,7 @@ func resourceJobDefinitionUpdate(ctx context.Context, d *schema.ResourceData, me currentARN := d.Get("arn").(string) d.SetId(aws.StringValue(jd.JobDefinitionArn)) d.Set("revision", jd.Revision) + d.Set("arn", jd.JobDefinitionArn) if v := d.Get("deregister_on_new_revision"); v == true { log.Printf("[DEBUG] Deleting Previous Batch Job Definition: %s", currentARN) @@ -889,7 +1020,7 @@ func expandEvaluateOnExit(tfMap map[string]interface{}) *batch.EvaluateOnExit { apiObject := &batch.EvaluateOnExit{} if v, ok := tfMap["action"].(string); ok && v != "" { - apiObject.Action = aws.String(v) + apiObject.Action = aws.String(strings.ToLower(v)) } if v, ok := tfMap["on_exit_code"].(string); ok && v != "" { From 380c887681ea3c485bb462dc8f1733cc8d35b1c5 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:16:12 -0400 Subject: [PATCH 2/7] Add changelog --- .changelog/37111.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/37111.txt diff --git a/.changelog/37111.txt b/.changelog/37111.txt new file mode 100644 index 00000000000..5dadef759f5 --- /dev/null +++ b/.changelog/37111.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_batch_job_definition: Fix issues where changes causing a new `revision` do not trigger changes in dependent resources and/or cause an error, "Provider produced inconsistent final plan" +``` From 6a5866260ba295e50a7f8e64b29b2499fb84b57f Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:16:52 -0400 Subject: [PATCH 3/7] batch: Add fixture for Lambda related testing --- .../service/batch/test-fixtures/lambdatest.zip | Bin 0 -> 342 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 internal/service/batch/test-fixtures/lambdatest.zip diff --git a/internal/service/batch/test-fixtures/lambdatest.zip b/internal/service/batch/test-fixtures/lambdatest.zip new file mode 100644 index 0000000000000000000000000000000000000000..5c636e955b2cccd992ac213993798acfdc39d6aa GIT binary patch literal 342 zcmWIWW@Zs#U|`^2_)xpcjj3pP&N2{>k%57iL53kGF*hkCu_U#)L@%p2G=!6ZndL`H zXdn=mR&X;gvU~-q18Xlm&k&i8#+QN6XCT<*-b`*@ggZ%;WfQT_nWzO%+$*O&s=9G|AR z&f>yR^AA6>@7sK}irLaRiOHq^)$z9dpQ3v8avCCzNC$W`GRZOH@~#BX;|vTyA2BRx d1hLRO&kFH8n#TjYS=m5}8G$euNWTSf7yyUqbSVG; literal 0 HcmV?d00001 From e6f555dcaeb6517604b7432389609fd047430249 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:17:52 -0400 Subject: [PATCH 4/7] batch/job_definition: Minor documentation updates --- website/docs/r/batch_job_definition.html.markdown | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/website/docs/r/batch_job_definition.html.markdown b/website/docs/r/batch_job_definition.html.markdown index be69314ffce..cc2af1d03a4 100644 --- a/website/docs/r/batch_job_definition.html.markdown +++ b/website/docs/r/batch_job_definition.html.markdown @@ -200,17 +200,14 @@ The following arguments are required: The following arguments are optional: -* `container_properties` - (Optional) A valid [container properties](http://docs.aws.amazon.com/batch/latest/APIReference/API_RegisterJobDefinition.html) - provided as a single valid JSON document. This parameter is only valid if the `type` parameter is `container`. +* `container_properties` - (Optional) A valid [container properties](http://docs.aws.amazon.com/batch/latest/APIReference/API_RegisterJobDefinition.html) provided as a single valid JSON document. This parameter is only valid if the `type` parameter is `container`. * `deregister_on_new_revision` - (Optional) When updating a job definition a new revision is created. This parameter determines if the previous version is `deregistered` (`INACTIVE`) or left `ACTIVE`. Defaults to `true`. -* `node_properties` - (Optional) A valid [node properties](http://docs.aws.amazon.com/batch/latest/APIReference/API_RegisterJobDefinition.html) - provided as a single valid JSON document. This parameter is required if the `type` parameter is `multinode`. +* `node_properties` - (Optional) A valid [node properties](http://docs.aws.amazon.com/batch/latest/APIReference/API_RegisterJobDefinition.html) provided as a single valid JSON document. This parameter is required if the `type` parameter is `multinode`. * `eks_properties` - (Optional) A valid [eks properties](#eks_properties). This parameter is only valid if the `type` parameter is `container`. * `parameters` - (Optional) Specifies the parameter substitution placeholders to set in the job definition. * `platform_capabilities` - (Optional) The platform capabilities required by the job definition. If no value is specified, it defaults to `EC2`. To run the job on Fargate resources, specify `FARGATE`. * `propagate_tags` - (Optional) Specifies whether to propagate the tags from the job definition to the corresponding Amazon ECS task. Default is `false`. -* `retry_strategy` - (Optional) Specifies the retry strategy to use for failed jobs that are submitted with this job definition. - Maximum number of `retry_strategy` is `1`. Defined below. +* `retry_strategy` - (Optional) Specifies the retry strategy to use for failed jobs that are submitted with this job definition. Maximum number of `retry_strategy` is `1`. Defined below. * `scheduling_priority` - (Optional) The scheduling priority of the job definition. This only affects jobs in job queues with a fair share policy. Jobs with a higher scheduling priority are scheduled before jobs with a lower scheduling priority. Allowed values `0` through `9999`. * `tags` - (Optional) Key-value map of resource tags. If configured with a provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block) present, tags with matching keys will overwrite those defined at the provider-level. * `timeout` - (Optional) Specifies the timeout for jobs so that if a job runs longer, AWS Batch terminates the job. Maximum number of `timeout` is `1`. Defined below. @@ -266,7 +263,7 @@ The following arguments are optional: #### `evaluate_on_exit` -* `action` - (Required) Specifies the action to take if all of the specified conditions are met. The values are not case sensitive. Valid values: `RETRY`, `EXIT`. +* `action` - (Required) Specifies the action to take if all of the specified conditions are met. The values are not case sensitive. Valid values: `retry`, `exit`. * `on_exit_code` - (Optional) A glob pattern to match against the decimal representation of the exit code returned for a job. * `on_reason` - (Optional) A glob pattern to match against the reason returned for a job. * `on_status_reason` - (Optional) A glob pattern to match against the status reason returned for a job. From 0b892633cd2457c4d2abb3fb1ceed20464c8b56e Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:18:44 -0400 Subject: [PATCH 5/7] batch/job_definition: Update data source test mistake --- internal/service/batch/job_definition_data_source_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/service/batch/job_definition_data_source_test.go b/internal/service/batch/job_definition_data_source_test.go index 322c09be82c..35478ef642d 100644 --- a/internal/service/batch/job_definition_data_source_test.go +++ b/internal/service/batch/job_definition_data_source_test.go @@ -67,7 +67,6 @@ func TestAccBatchJobDefinitionDataSource_basicARN(t *testing.T) { resource.TestCheckResourceAttr(dataSourceName, "retry_strategy.0.attempts", "10"), resource.TestCheckResourceAttr(dataSourceName, "revision", "1"), resource.TestCheckResourceAttr(dataSourceName, "revision", "1"), - resource.TestCheckResourceAttr(dataSourceName, "retry_strategy.attempts", "10"), acctest.MatchResourceAttrRegionalARN(dataSourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:\d+`, rName))), ), }, From 6931b9f42be53c8ba70bc4645a63babc3975600b Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 25 Apr 2024 19:19:11 -0400 Subject: [PATCH 6/7] batch/job_definition: Update testing to account for new functionality --- internal/service/batch/job_definition_test.go | 174 +++++++++++++++++- 1 file changed, 165 insertions(+), 9 deletions(-) diff --git a/internal/service/batch/job_definition_test.go b/internal/service/batch/job_definition_test.go index 8eca5f35a28..6ca57aa55ee 100644 --- a/internal/service/batch/job_definition_test.go +++ b/internal/service/batch/job_definition_test.go @@ -92,7 +92,7 @@ func TestAccBatchJobDefinition_attributes(t *testing.T) { Config: testAccJobDefinitionConfig_attributes(rName, 2, true, 3, 120, false), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckJobDefinitionExists(ctx, resourceName, &jd), - acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:\d+`, rName))), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:1`, rName))), acctest.MatchResourceAttrRegionalARN(resourceName, "arn_prefix", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s`, rName))), resource.TestCheckResourceAttr(resourceName, "name", rName), resource.TestCheckResourceAttr(resourceName, "parameters.%", "0"), @@ -113,6 +113,7 @@ func TestAccBatchJobDefinition_attributes(t *testing.T) { Config: testAccJobDefinitionConfig_attributes(rName, 2, true, 4, 120, false), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:2`, rName))), testAccCheckJobDefinitionPreviousRegistered(ctx, resourceName), resource.TestCheckResourceAttr(resourceName, "revision", "2"), ), @@ -122,7 +123,7 @@ func TestAccBatchJobDefinition_attributes(t *testing.T) { Check: resource.ComposeAggregateTestCheckFunc( testAccCheckJobDefinitionExists(ctx, resourceName, &jd), testAccCheckJobDefinitionPreviousDeregistered(ctx, resourceName), - acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:\d+`, rName))), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:3`, rName))), acctest.MatchResourceAttrRegionalARN(resourceName, "arn_prefix", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s`, rName))), resource.TestCheckResourceAttr(resourceName, "name", rName), resource.TestCheckResourceAttr(resourceName, "parameters.%", "0"), @@ -391,7 +392,7 @@ func TestAccBatchJobDefinition_ContainerProperties_advanced(t *testing.T) { CheckDestroy: testAccCheckJobDefinitionDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName), + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val2", 1, 60), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckJobDefinitionExists(ctx, resourceName, &jd), testAccCheckJobDefinitionAttributes(&jd, &compare), @@ -405,14 +406,90 @@ func TestAccBatchJobDefinition_ContainerProperties_advanced(t *testing.T) { "deregister_on_new_revision", }, }, + { + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val3", 1, 60), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + resource.TestCheckResourceAttr(resourceName, "revision", "2"), + ), + }, + { + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val3", 1, 60), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + resource.TestCheckResourceAttr(resourceName, "revision", "2"), + ), + }, + { + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val3", 3, 60), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + resource.TestCheckResourceAttr(resourceName, "revision", "3"), + ), + }, + { + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val3", 3, 60), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + resource.TestCheckResourceAttr(resourceName, "revision", "3"), + ), + }, + { + Config: testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, "val3", 3, 120), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + resource.TestCheckResourceAttr(resourceName, "revision", "4"), + ), + }, { Config: testAccJobDefinitionConfig_containerPropertiesAdvancedUpdate(rName), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckJobDefinitionExists(ctx, resourceName, &jd), testAccCheckJobDefinitionPreviousDeregistered(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "revision", "5"), + ), + }, + }, + }) +} + +func TestAccBatchJobDefinition_ContainerProperties_minorUpdate(t *testing.T) { + ctx := acctest.Context(t) + var jd batch.JobDefinition + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_batch_job_definition.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.BatchServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckJobDefinitionDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccJobDefinitionConfig_containerProperties(rName, "-la"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:1`, rName))), + ), + }, + { + Config: testAccJobDefinitionConfig_containerProperties(rName, "-lah"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:2`, rName))), + testAccCheckJobDefinitionPreviousDeregistered(ctx, resourceName), resource.TestCheckResourceAttr(resourceName, "revision", "2"), ), }, + { + Config: testAccJobDefinitionConfig_containerProperties(rName, "-hal"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckJobDefinitionExists(ctx, resourceName, &jd), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "batch", regexache.MustCompile(fmt.Sprintf(`job-definition/%s:3`, rName))), + testAccCheckJobDefinitionPreviousDeregistered(ctx, resourceName), + resource.TestCheckResourceAttr(resourceName, "revision", "3"), + ), + }, }, }) } @@ -970,7 +1047,12 @@ func testAccCheckJobDefinitionDestroy(ctx context.Context) resource.TestCheckFun } re := regexache.MustCompile(`job-definition/(.*?):`) - name := re.FindStringSubmatch(rs.Primary.ID)[1] + m := re.FindStringSubmatch(rs.Primary.ID) + if len(m) < 1 { + continue + } + + name := m[1] jds, err := tfbatch.ListActiveJobDefinitionByName(ctx, conn, name) @@ -988,17 +1070,17 @@ func testAccCheckJobDefinitionDestroy(ctx context.Context) resource.TestCheckFun } } -func testAccJobDefinitionConfig_containerPropertiesAdvanced(rName string) string { +func testAccJobDefinitionConfig_containerPropertiesAdvanced(rName, param string, retries, timeout int) string { return fmt.Sprintf(` resource "aws_batch_job_definition" "test" { name = %[1]q type = "container" parameters = { param1 = "val1" - param2 = "val2" + param2 = %[2]q } retry_strategy { - attempts = 1 + attempts = %[3]d evaluate_on_exit { action = "RETRY" on_status_reason = "Host EC2*" @@ -1009,7 +1091,7 @@ resource "aws_batch_job_definition" "test" { } } timeout { - attempt_duration_seconds = 60 + attempt_duration_seconds = %[4]d } container_properties = < Date: Thu, 25 Apr 2024 19:23:41 -0400 Subject: [PATCH 7/7] batch/job_definition: Tests fix --- internal/service/batch/job_definition_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/batch/job_definition_test.go b/internal/service/batch/job_definition_test.go index 6ca57aa55ee..b391c5d0e82 100644 --- a/internal/service/batch/job_definition_test.go +++ b/internal/service/batch/job_definition_test.go @@ -1196,7 +1196,7 @@ resource "aws_batch_job_definition" "test" { name = %[1]q type = "container" container_properties = jsonencode({ - command = ["ls", %[2]q], + command = ["ls", "%[2]s"], image = "busybox" resourceRequirements = [