resource/aws_appautoscaling_policy: Recreate resource for `resource_i…

…d` updates and ignore `ObjectNotFoundException` on deletion

References:

* #7963
* #5747
* #538
* #486
* #427
* #404

Previously the documentation recommended an ECS setup that used `depends_on` combined with an updateable `resource_id` attribute, that could introduce very subtle bugs in the operation of the `aws_appautoscaling_policy` resource when the either underlying Application AutoScaling Target or target resource (e.g. ECS service) was updated or recreated.

Given the scenario with an `aws_appautoscaling_policy` configuration:

* No direct attributes references to its `aws_appautoscaling_target` parent (usage with or without `depends_on` is inconsequential except without its usage in this case, it would generate errors that the target does not exist due to lack of proper ordering)
* `resource_id` directly references the target resource (e.g. an ECS service)
* The underlying `resource_id` target resource (e.g. an ECS service) is pointed to a new location or the resource is recreated

The `aws_appautoscaling_policy` resource would plan as an resource update of just the `resource_id` attribute instead of resource recreation. Several consquences could occur in this situation depending on the exact ordering and Terraform configuration:

* Since the Application AutoScaling Policy API only supports a `PUT` type operation for creation and update, a new policy would create successfully (given the Application AutoScaling Target was already in place), hiding any coding errors that might have been found if it was attempting to update a non-created policy
* Usage of only `depends_on` to reference the Application AutoScaling Target could miss creating the Application AutoScaling Policy in a single apply since `depends_on` is purely for ordering
* The lack of Application AutoScaling Policy deletion could leave dangling policies on the previous Application AutoScaling Target unless it was updated (which it correctly recreates the resource in Terraform) or otherwise deleted
* The Terraform resource would not know to properly update the value of other computed attributes during plan, such as `arn`, potentially only noticing these attribute values as a new applied value different from the planned value

These situations could surface as Terraform bugs in multiple ways:

* In friendlier cases, a second apply would be required to create the missing policy or update downstream computed references
* In worse cases, Terraform would report errors (depending on the Terraform version) such as `Resource 'aws_appautoscaling_policy.example' does not have attribute 'arn'` and `diffs didn't match during apply` for downstream attribute references to those computed attributes

To prevent these situations, the `ResourceId` of the Application AutoScaling Policy needs be treated as part of the API object ID, similar to Application AutoScaling Targets, and marked `ForceNew: true` in the Terraform resource schema. We also ensure the documentation examples always recommend direct references to the upstream `aws_appautoscaling_target` instead of using `depends_on` so Terraform properly handles recreations when necessary, e.g.

```hcl
resource "aws_appautoscaling_target" "example" {
  # ... other configuration ...
}

resource "aws_appautoscaling_policy" "example" {
 # ... other configuration ...

  resource_id        = "${aws_appautoscaling_target.example.resource_id}"
  scalable_dimension = "${aws_appautoscaling_target.example.scalable_dimension}"
  service_namespace  = "${aws_appautoscaling_target.example.service_namespace}"
}
```

During research of this bug, it was also similarly discovered that the `aws_appautoscaling_policy` resource did not gracefully handle external deletions of the Application AutoScaling Policy without a refresh or potential deletion race conditions with the following error:

```
ObjectNotFoundException: No scaling policy found for service namespace: ecs, resource ID: service/tf-acc-test-9190521664283069857/tf-acc-test-9190521664283069857, scalable dimension: ecs:service:DesiredCount, policy name: tf-acc-test-9190521664283069857
```

We include ignoring this potential error on deletion as part of the comprehesive solution to ensuring resource recreations are successful.

Output from acceptance testing before code update:

```
--- FAIL: TestAccAWSAppautoScalingPolicy_ResourceId_ForceNew (54.69s)
    testing.go:538: Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: aws_cloudwatch_metric_alarm.test
          alarm_actions.3359603714: "arn:aws:autoscaling:us-west-2:--OMITTED--:scalingPolicy:065d03ea-a7a4-4047-9a43-c92ec1871170:resource/ecs/service/tf-acc-test-2456603151506624334/tf-acc-test-2456603151506624334-1:policyName/tf-acc-test-2456603151506624334" => ""
          alarm_actions.4257611624: "" => "arn:aws:autoscaling:us-west-2:--OMITTED--:scalingPolicy:cdc6d280-8a93-4c67-9790-abb47fd167c6:resource/ecs/service/tf-acc-test-2456603151506624334/tf-acc-test-2456603151506624334-2:policyName/tf-acc-test-2456603151506624334"
```

Output from acceptance testing:

```
--- PASS: TestAccAWSAppautoScalingPolicy_disappears (26.48s)
--- PASS: TestAccAWSAppautoScalingPolicy_scaleOutAndIn (28.53s)
--- PASS: TestAccAWSAppautoScalingPolicy_ResourceId_ForceNew (43.25s)
--- PASS: TestAccAWSAppautoScalingPolicy_basic (46.47s)
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (61.26s)
--- PASS: TestAccAWSAppautoScalingPolicy_dynamoDb (115.02s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameResource (116.06s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameName (116.80s)
```

aws/resource_aws_appautoscaling_policy.go

@@ -40,6 +40,7 @@ func resourceAwsAppautoscalingPolicy() *schema.Resource {
  "resource_id": {
  Type: schema.TypeString,
  Required: true,
+ ForceNew: true,
  },
  "scalable_dimension": {
  Type: schema.TypeString,
@@ -368,17 +369,29 @@ func resourceAwsAppautoscalingPolicyDelete(d *schema.ResourceData, meta interfac
  log.Printf("[DEBUG] Deleting Application AutoScaling Policy opts: %#v", params)
  err = resource.Retry(2*time.Minute, func() *resource.RetryError {
  _, err = conn.DeleteScalingPolicy(&params)
+
+ if isAWSErr(err, applicationautoscaling.ErrCodeFailedResourceAccessException, "") {
+ return resource.RetryableError(err)
+ }
+
+ if isAWSErr(err, applicationautoscaling.ErrCodeObjectNotFoundException, "") {
+ return nil
+ }
+
  if err != nil {
- if isAWSErr(err, applicationautoscaling.ErrCodeFailedResourceAccessException, "") {
- return resource.RetryableError(err)
- }
  return resource.NonRetryableError(err)
  }
  return nil
  })
+
+ if isResourceTimeoutError(err) {
+ _, err = conn.DeleteScalingPolicy(&params)
+ }
+
  if err != nil {
  return fmt.Errorf("Failed to delete scaling policy: %s", err)
  }
+
  return nil
 }
 

aws/resource_aws_appautoscaling_policy_test.go

@@ -43,6 +43,28 @@ func TestAccAWSAppautoScalingPolicy_basic(t *testing.T) {
  })
 }
 
+func TestAccAWSAppautoScalingPolicy_disappears(t *testing.T) {
+ var policy applicationautoscaling.ScalingPolicy
+ resourceName := "aws_appautoscaling_policy.test"
+ rName := acctest.RandomWithPrefix("tf-acc-test")
+
+ resource.ParallelTest(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckAWSAppautoscalingPolicyDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccAWSAppautoscalingPolicyConfig(rName),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckAWSAppautoscalingPolicyExists(resourceName, &policy),
+ testAccCheckAWSAppautoscalingPolicyDisappears(&policy),
+ ),
+ ExpectNonEmptyPlan: true,
+ },
+ },
+ })
+}
+
 func TestAccAWSAppautoScalingPolicy_scaleOutAndIn(t *testing.T) {
  var policy applicationautoscaling.ScalingPolicy
 
@@ -212,6 +234,40 @@ func TestAccAWSAppautoScalingPolicy_multiplePoliciesSameResource(t *testing.T) {
  })
 }
 
+// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/7963
+func TestAccAWSAppautoScalingPolicy_ResourceId_ForceNew(t *testing.T) {
+ var policy applicationautoscaling.ScalingPolicy
+ appAutoscalingTargetResourceName := "aws_appautoscaling_target.test"
+ resourceName := "aws_appautoscaling_policy.test"
+ rName := acctest.RandomWithPrefix("tf-acc-test")
+
+ resource.ParallelTest(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckAWSAppautoscalingPolicyDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccAWSAppautoscalingPolicyConfigResourceIdForceNew1(rName),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckAWSAppautoscalingPolicyExists(resourceName, &policy),
+ resource.TestCheckResourceAttrPair(resourceName, "resource_id", appAutoscalingTargetResourceName, "resource_id"),
+ resource.TestCheckResourceAttrPair(resourceName, "scalable_dimension", appAutoscalingTargetResourceName, "scalable_dimension"),
+ resource.TestCheckResourceAttrPair(resourceName, "service_namespace", appAutoscalingTargetResourceName, "service_namespace"),
+ ),
+ },
+ {
+ Config: testAccAWSAppautoscalingPolicyConfigResourceIdForceNew2(rName),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckAWSAppautoscalingPolicyExists(resourceName, &policy),
+ resource.TestCheckResourceAttrPair(resourceName, "resource_id", appAutoscalingTargetResourceName, "resource_id"),
+ resource.TestCheckResourceAttrPair(resourceName, "scalable_dimension", appAutoscalingTargetResourceName, "scalable_dimension"),
+ resource.TestCheckResourceAttrPair(resourceName, "service_namespace", appAutoscalingTargetResourceName, "service_namespace"),
+ ),
+ },
+ },
+ })
+}
+
 func testAccCheckAWSAppautoscalingPolicyExists(n string, policy *applicationautoscaling.ScalingPolicy) resource.TestCheckFunc {
  return func(s *terraform.State) error {
  rs, ok := s.RootModule().Resources[n]
@@ -221,8 +277,10 @@ func testAccCheckAWSAppautoscalingPolicyExists(n string, policy *applicationauto
 
  conn := testAccProvider.Meta().(*AWSClient).appautoscalingconn
  params := &applicationautoscaling.DescribeScalingPoliciesInput{
- ServiceNamespace: aws.String(rs.Primary.Attributes["service_namespace"]),
- PolicyNames: []*string{aws.String(rs.Primary.ID)},
+ PolicyNames: []*string{aws.String(rs.Primary.ID)},
+ ResourceId: aws.String(rs.Primary.Attributes["resource_id"]),
+ ScalableDimension: aws.String(rs.Primary.Attributes["scalable_dimension"]),
+ ServiceNamespace: aws.String(rs.Primary.Attributes["service_namespace"]),
  }
  resp, err := conn.DescribeScalingPolicies(params)
  if err != nil {
@@ -232,6 +290,8 @@ func testAccCheckAWSAppautoscalingPolicyExists(n string, policy *applicationauto
  return fmt.Errorf("ScalingPolicy %s not found", rs.Primary.ID)
  }
 
+ *policy = *resp.ScalingPolicies[0]
+
  return nil
  }
 }
@@ -258,6 +318,23 @@ func testAccCheckAWSAppautoscalingPolicyDestroy(s *terraform.State) error {
  return nil
 }
 
+func testAccCheckAWSAppautoscalingPolicyDisappears(policy *applicationautoscaling.ScalingPolicy) resource.TestCheckFunc {
+ return func(s *terraform.State) error {
+ conn := testAccProvider.Meta().(*AWSClient).appautoscalingconn
+
+ input := &applicationautoscaling.DeleteScalingPolicyInput{
+ PolicyName: policy.PolicyName,
+ ResourceId: policy.ResourceId,
+ ScalableDimension: policy.ScalableDimension,
+ ServiceNamespace: policy.ServiceNamespace,
+ }
+
+ _, err := conn.DeleteScalingPolicy(input)
+
+ return err
+ }
+}
+
 func testAccAWSAppautoscalingPolicyConfig(rName string) string {
  return fmt.Sprintf(`
 resource "aws_ecs_cluster" "test" {
@@ -676,3 +753,142 @@ resource "aws_appautoscaling_policy" "foobar_in" {
 }
 `, randClusterName, randPolicyNamePrefix, randPolicyNamePrefix)
 }
+
+func testAccAWSAppautoscalingPolicyConfigResourceIdForceNewBase(rName string) string {
+ return fmt.Sprintf(`
+resource "aws_ecs_cluster" "test" {
+ name = %[1]q
+}
+
+resource "aws_ecs_task_definition" "test" {
+ family = %[1]q
+ container_definitions = <<EOF
+[
+ {
+ "name": "busybox",
+ "image": "busybox:latest",
+ "cpu": 10,
+ "memory": 128,
+ "essential": true
+ }
+]
+EOF
+}
+
+resource "aws_ecs_service" "test1" {
+ cluster = "${aws_ecs_cluster.test.id}"
+ deployment_maximum_percent = 200
+ deployment_minimum_healthy_percent = 50
+ desired_count = 0
+ name = "%[1]s-1"
+ task_definition = "${aws_ecs_task_definition.test.arn}"
+}
+
+resource "aws_ecs_service" "test2" {
+ cluster = "${aws_ecs_cluster.test.id}"
+ deployment_maximum_percent = 200
+ deployment_minimum_healthy_percent = 50
+ desired_count = 0
+ name = "%[1]s-2"
+ task_definition = "${aws_ecs_task_definition.test.arn}"
+}
+`, rName)
+}
+
+func testAccAWSAppautoscalingPolicyConfigResourceIdForceNew1(rName string) string {
+ return testAccAWSAppautoscalingPolicyConfigResourceIdForceNewBase(rName) + fmt.Sprintf(`
+resource "aws_appautoscaling_target" "test" {
+ max_capacity = 4
+ min_capacity = 0
+ resource_id = "service/${aws_ecs_cluster.test.name}/${aws_ecs_service.test1.name}"
+ scalable_dimension = "ecs:service:DesiredCount"
+ service_namespace = "ecs"
+}
+
+resource "aws_appautoscaling_policy" "test" {
+ # The usage of depends_on here is intentional as this used to be a documented example
+ depends_on = ["aws_appautoscaling_target.test"]
+
+ name = %[1]q
+ resource_id = "service/${aws_ecs_cluster.test.name}/${aws_ecs_service.test1.name}"
+ scalable_dimension = "ecs:service:DesiredCount"
+ service_namespace = "ecs"
+
+ step_scaling_policy_configuration {
+ adjustment_type = "ChangeInCapacity"
+ cooldown = 60
+ metric_aggregation_type = "Average"
+
+ step_adjustment {
+ metric_interval_lower_bound = 0
+ scaling_adjustment = 1
+ }
+ }
+}
+
+resource "aws_cloudwatch_metric_alarm" "test" {
+ alarm_actions = ["${aws_appautoscaling_policy.test.arn}"]
+ alarm_name = %[1]q
+ comparison_operator = "LessThanOrEqualToThreshold"
+ evaluation_periods = "5"
+ metric_name = "CPUReservation"
+ namespace = "AWS/ECS"
+ period = "60"
+ statistic = "Average"
+ threshold = "0"
+
+ dimensions = {
+ ClusterName = "${aws_ecs_cluster.test.name}"
+ }
+}
+`, rName)
+}
+
+func testAccAWSAppautoscalingPolicyConfigResourceIdForceNew2(rName string) string {
+ return testAccAWSAppautoscalingPolicyConfigResourceIdForceNewBase(rName) + fmt.Sprintf(`
+resource "aws_appautoscaling_target" "test" {
+ max_capacity = 4
+ min_capacity = 0
+ resource_id = "service/${aws_ecs_cluster.test.name}/${aws_ecs_service.test2.name}"
+ scalable_dimension = "ecs:service:DesiredCount"
+ service_namespace = "ecs"
+}
+
+resource "aws_appautoscaling_policy" "test" {
+ # The usage of depends_on here is intentional as this used to be a documented example
+ depends_on = ["aws_appautoscaling_target.test"]
+
+ name = %[1]q
+ resource_id = "service/${aws_ecs_cluster.test.name}/${aws_ecs_service.test2.name}"
+ scalable_dimension = "ecs:service:DesiredCount"
+ service_namespace = "ecs"
+
+ step_scaling_policy_configuration {
+ adjustment_type = "ChangeInCapacity"
+ cooldown = 60
+ metric_aggregation_type = "Average"
+
+ step_adjustment {
+ metric_interval_lower_bound = 0
+ scaling_adjustment = 1
+ }
+ }
+}
+
+resource "aws_cloudwatch_metric_alarm" "test" {
+ alarm_actions = ["${aws_appautoscaling_policy.test.arn}"]
+ alarm_name = %[1]q
+ comparison_operator = "LessThanOrEqualToThreshold"
+ evaluation_periods = "5"
+ metric_name = "CPUReservation"
+ namespace = "AWS/ECS"
+ period = "60"
+ statistic = "Average"
+ threshold = "0"
+
+ dimensions = {
+ ClusterName = "${aws_ecs_cluster.test.name}"
+ }
+}
+`, rName)
+}

website/docs/r/appautoscaling_policy.html.markdown

@@ -56,9 +56,9 @@ resource "aws_appautoscaling_target" "ecs_target" {
 resource "aws_appautoscaling_policy" "ecs_policy" {
  name = "scale-down"
  policy_type = "StepScaling"
- resource_id = "service/clusterName/serviceName"
- scalable_dimension = "ecs:service:DesiredCount"
- service_namespace = "ecs"
+ resource_id = "${aws_appautoscaling_target.ecs_target.resource_id}"
+ scalable_dimension = "${aws_appautoscaling_target.ecs_target.scalable_dimension}"
+ service_namespace = "${aws_appautoscaling_target.ecs_target.service_namespace}"
 
  step_scaling_policy_configuration {
  adjustment_type = "ChangeInCapacity"
@@ -70,8 +70,6 @@ resource "aws_appautoscaling_policy" "ecs_policy" {
  scaling_adjustment = -1
  }
  }
-
- depends_on = ["aws_appautoscaling_target.ecs_target"]
 }
 ```