resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes #8236

nywilken · 2019-04-06T18:24:19Z

Lifecycle policies contain settings for deleting backups, and for moving them to cold storage. A backup with cold storage enabled can not have a deletion value lower then 90 days. So to prevent this AWS allows setting ColdStorageAfter to Never by not setting a value for ColdStorageAfter.

This change adds logic to ensure ColdStorageAfter and DeleteAfter only get sent within the API request if the values are not empty and greater than 0.

Acceptance Test before change

=== RUN   TestAccAwsBackupPlan_withLifecycle
=== PAUSE TestAccAwsBackupPlan_withLifecycle
=== RUN   TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
=== PAUSE TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
=== RUN   TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== PAUSE TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== CONT  TestAccAwsBackupPlan_withLifecycle
=== CONT  TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== CONT  TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
--- FAIL: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly (10.86s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
                * aws_backup_plan.test: 1 error occurred:
                * aws_backup_plan.test: error creating Backup Plan: InvalidParameterValueException: Error in rule tf_acc_test_backup_rule_lifecycle_policy_three : Invalid lifecycle. DeleteAfterDays cannot be less than one day
                status code: 400, request id: 757544c3-4628-4a7e-95fa-416098fe2594

--- FAIL: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (11.00s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
                * aws_backup_plan.test: 1 error occurred:
                * aws_backup_plan.test: error creating Backup Plan: InvalidParameterValueException: Error in rule tf_acc_test_backup_rule_lifecycle_policy_two : Invalid lifecycle. DeleteAfterDays cannot be less than 90 days apart from MoveToColdStorageAfterDays
                status code: 400, request id: e37c0d06-0cd7-4783-b01a-40e1dc5758f0

--- PASS: TestAccAwsBackupPlan_withLifecycle (18.92s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       18.948s
GNUmakefile:20: recipe for target 'testacc' failed

Acceptance Test after change

=== RUN   TestAccAwsBackupPlan_withLifecycle
=== PAUSE TestAccAwsBackupPlan_withLifecycle
=== RUN   TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
=== PAUSE TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
=== RUN   TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== PAUSE TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== CONT  TestAccAwsBackupPlan_withLifecycle
=== CONT  TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
=== CONT  TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly
--- PASS: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (18.70s)
--- PASS: TestAccAwsBackupPlan_withLifecycle (19.75s)
--- PASS: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly
(20.25s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws
20.266s

bflad

LGTM with some optional nitpicks below.

Output from acceptance testing:

--- PASS: TestAccAwsBackupPlan_disappears (13.73s)
--- PASS: TestAccAwsBackupPlan_withLifecycle (14.79s)
--- PASS: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (14.93s)
--- PASS: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly (14.94s)
--- PASS: TestAccAwsBackupPlan_withRules (15.16s)
--- PASS: TestAccAwsBackupPlan_basic (15.21s)
--- PASS: TestAccAwsBackupPlan_withRuleRemove (22.22s)
--- PASS: TestAccAwsBackupPlan_withRuleAdd (22.31s)
--- PASS: TestAccAwsBackupPlan_withTags (32.90s)

bflad · 2019-04-08T15:44:11Z

aws/resource_aws_backup_plan.go

-			l["delete_after"] = aws.Int64Value(r.Lifecycle.DeleteAfterDays)
-			l["cold_storage_after"] = aws.Int64Value(r.Lifecycle.MoveToColdStorageAfterDays)
+
+			if v := r.Lifecycle.DeleteAfterDays; v != nil {


Nit: The nil checks here and below are extraneous because aws.Int64Value() will automatically convert nil to 0 and TypeInt defaults to 0.

bflad · 2019-04-08T15:46:15Z

aws/resource_aws_backup_plan_test.go

+    target_vault_name  = "${aws_backup_vault.test.name}"
+    schedule           = "cron(0 12 * * ? *)"
+    lifecycle {
+        delete_after       = "7"


Nit: Formatting here and below

Suggested change

delete_after = "7"

delete_after = "7"

bflad · 2019-04-08T15:47:48Z

aws/resource_aws_backup_plan_test.go

+    }
+  }
+}
+`, stringID, stringID, stringID)


Nit: Can use formatter indexing to reduce fmt.Sprintf() argument sprawl here and below, e.g.

return fmt.Sprintf(` resource "aws_backup_vault" "test" { name = "tf_acc_test_backup_vault_%[1]s" } resource "aws_backup_plan" "test" { name = "tf_acc_test_backup_plan_%[1]s" rule { rule_name = "tf_acc_test_backup_rule_%[1]s" ... `, stringID)

dud225 · 2019-04-10T14:33:54Z

aws/resource_aws_backup_plan_test.go

+					testAccCheckAwsBackupPlanExists("aws_backup_plan.test", &plan),
+					resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.2156287050.lifecycle.#", "1"),
+					resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.2156287050.lifecycle.0.delete_after", "7"),
+					resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.2156287050.lifecycle.0.cold_storage_after", "0"),


I don't speak Go at all so I'm not sure to understand properly what you did.
AFAICT if MoveToColdStorageAfterDays is not sent to the AWS API, then this field won't ever appear on the backup plan. Here is an excerpt from a backup plan :

"Lifecycle": { "DeleteAfterDays": 90 },

that has been created with this command :

$ aws backup update-backup-plan --backup-plan-id <id> --backup-plan '{"BackupPlanName":"my- backup", "Rules": [{"RuleName": "dailybackups", "TargetBackupVaultName": "my-vault", "Lifecycle": {"DeleteAfterDays": 90}}]}'

So if I understand your tests correctly you shouldn't test if the value is zero, but just ignore the field.

The same thing is true for DeleteAfterDays

OK I think I got it, it is only the TF representation of the backup plan that holds the zero values, the zero values are then filtered out when the plan is sent to the AWS API.

OK I think I got it, it is only the TF representation of the backup plan that holds the zero values, the zero values are then filtered out when the plan is sent to the AWS API.

Hi @dud225 thanks for the review and the question. Your understanding is correct by default the zero value of any Terraform configuration would be assigned within Terraform but we would only send the value to the API if it is a non zero value, in this case some an int greater than 0. FYI when I say zero value I am referring to the zero values of Go types https://golang.org/ref/spec#The_zero_value

…ributes Closes #8151 Lifecycle policies contain settings for deleting backups, and for moving them to cold storage. A backup with cold storage enabled can not have a deletion value lower then 90 days. So to prevent this AWS allows setting ColdStorageAfter to Never by not setting a value for ColdStorageAfter. This change adds logic to ensure ColdStorageAfter and DeleteAfter only get sent within the API request if the values are not empty and greater than 0. Acceptance Test before change ``` === RUN TestAccAwsBackupPlan_withLifecycle === PAUSE TestAccAwsBackupPlan_withLifecycle === RUN TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly === PAUSE TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly === RUN TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === PAUSE TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === CONT TestAccAwsBackupPlan_withLifecycle === CONT TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === CONT TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly --- FAIL: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly (10.86s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_plan.test: 1 error occurred: * aws_backup_plan.test: error creating Backup Plan: InvalidParameterValueException: Error in rule tf_acc_test_backup_rule_lifecycle_policy_three : Invalid lifecycle. DeleteAfterDays cannot be less than one day status code: 400, request id: 757544c3-4628-4a7e-95fa-416098fe2594 --- FAIL: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (11.00s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_plan.test: 1 error occurred: * aws_backup_plan.test: error creating Backup Plan: InvalidParameterValueException: Error in rule tf_acc_test_backup_rule_lifecycle_policy_two : Invalid lifecycle. DeleteAfterDays cannot be less than 90 days apart from MoveToColdStorageAfterDays status code: 400, request id: e37c0d06-0cd7-4783-b01a-40e1dc5758f0 --- PASS: TestAccAwsBackupPlan_withLifecycle (18.92s) FAIL FAIL github.com/terraform-providers/terraform-provider-aws/aws 18.948s GNUmakefile:20: recipe for target 'testacc' failed ``` Acceptance Test after change ``` === RUN TestAccAwsBackupPlan_withLifecycle === PAUSE TestAccAwsBackupPlan_withLifecycle === RUN TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly === PAUSE TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly === RUN TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === PAUSE TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === CONT TestAccAwsBackupPlan_withLifecycle === CONT TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly === CONT TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly --- PASS: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (18.70s) --- PASS: TestAccAwsBackupPlan_withLifecycle (19.75s) --- PASS: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly (20.25s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 20.266s ```

bflad

Still LGTM 😄

--- PASS: TestAccAwsBackupPlan_disappears (13.50s)
--- PASS: TestAccAwsBackupPlan_basic (14.91s)
--- PASS: TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly (14.99s)
--- PASS: TestAccAwsBackupPlan_withRules (15.47s)
--- PASS: TestAccAwsBackupPlan_withLifecycle (15.71s)
--- PASS: TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly (16.08s)
--- PASS: TestAccAwsBackupPlan_withRuleAdd (23.31s)
--- PASS: TestAccAwsBackupPlan_withRuleRemove (25.91s)
--- PASS: TestAccAwsBackupPlan_withTags (35.52s)

bflad · 2019-04-11T01:12:36Z

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

ghost · 2020-03-30T17:45:14Z

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 added size/M 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 Apr 6, 2019

slapula mentioned this pull request Apr 6, 2019

Support for AWS Backup #7166

Closed

nywilken force-pushed the f-aws_backup_plan-lifecycle branch from 7d98306 to 9035974 Compare April 7, 2019 11:06

nywilken requested a review from a team April 7, 2019 11:06

bflad added bug Addresses a defect in current functionality. service/backup Issues and PRs that pertain to the backup service. labels Apr 8, 2019

bflad approved these changes Apr 8, 2019

View reviewed changes

dud225 reviewed Apr 10, 2019

View reviewed changes

nywilken force-pushed the f-aws_backup_plan-lifecycle branch from 9035974 to 6732c19 Compare April 10, 2019 15:52

nywilken force-pushed the f-aws_backup_plan-lifecycle branch from 6732c19 to 530a198 Compare April 10, 2019 15:54

nywilken changed the title ~~r/aws_backup_plan: Prevents the sending of empty lifecycle attributes~~ resource/aws_backup_plan: Prevents the sending of empty lifecycle attributes Apr 10, 2019

bflad added this to the v2.6.0 milestone Apr 10, 2019

bflad approved these changes Apr 10, 2019

View reviewed changes

nywilken changed the title ~~resource/aws_backup_plan: Prevents the sending of empty lifecycle attributes~~ resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes Apr 10, 2019

nywilken merged commit f7f2f8f into master Apr 10, 2019

nywilken deleted the f-aws_backup_plan-lifecycle branch April 10, 2019 18:02

nywilken mentioned this pull request Apr 10, 2019

Do not send cold_storage_after or delete_after if not present in configuration #8235

Closed

nywilken added a commit that referenced this pull request Apr 10, 2019

Update CHANGELOG for #8236

88055e8

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes #8236

resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes #8236

nywilken commented Apr 6, 2019 •

edited

Loading

bflad left a comment

bflad Apr 8, 2019

bflad Apr 8, 2019

bflad Apr 8, 2019

dud225 Apr 10, 2019

dud225 Apr 10, 2019

nywilken Apr 10, 2019 •

edited

Loading

bflad left a comment

bflad commented Apr 11, 2019

ghost commented Mar 30, 2020

resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes #8236

resource/aws_backup_plan: Prevent the sending of empty lifecycle attributes #8236

Conversation

nywilken commented Apr 6, 2019 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Apr 8, 2019

Choose a reason for hiding this comment

bflad Apr 8, 2019

Choose a reason for hiding this comment

bflad Apr 8, 2019

Choose a reason for hiding this comment

dud225 Apr 10, 2019

Choose a reason for hiding this comment

dud225 Apr 10, 2019

Choose a reason for hiding this comment

nywilken Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 11, 2019

ghost commented Mar 30, 2020

nywilken commented Apr 6, 2019 •

edited

Loading

nywilken Apr 10, 2019 •

edited

Loading