Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_appautoscaling_policy: Recreate resource for resource_id updates and ignore ObjectNotFoundException on deletion #7982

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Mar 17, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #7963
Closes #5747
Closes #538
Closes #486
Closes #427
Closes #404

Previously the documentation recommended an ECS setup that used depends_on combined with an updatable 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 a resource update of just the resource_id attribute instead of resource recreation. Several consequences 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.

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)

…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)
```
@bflad bflad added bug Addresses a defect in current functionality. service/applicationautoscaling labels Mar 17, 2019
@ghost ghost added size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Mar 17, 2019
@bflad bflad requested a review from a team March 18, 2019 15:00
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@bflad
Copy link
Contributor Author

bflad commented Mar 21, 2019

This has been released in version 2.3.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
2 participants