From 903597479c453e4e9f1ed6765638c41fc6841009 Mon Sep 17 00:00:00 2001 From: nywilken Date: Sat, 6 Apr 2019 14:03:31 -0400 Subject: [PATCH] r/aws_backup_plan: Prevents the sending of empty lifecycle attributes 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 ``` --- aws/resource_aws_backup_plan.go | 20 ++++-- aws/resource_aws_backup_plan_test.go | 88 ++++++++++++++++++++++++ website/docs/r/backup_plan.html.markdown | 6 +- 3 files changed, 105 insertions(+), 9 deletions(-) diff --git a/aws/resource_aws_backup_plan.go b/aws/resource_aws_backup_plan.go index 8c7f052b4d08..c319585c2d86 100644 --- a/aws/resource_aws_backup_plan.go +++ b/aws/resource_aws_backup_plan.go @@ -151,8 +151,14 @@ func resourceAwsBackupPlanRead(d *schema.ResourceData, meta interface{}) error { if r.Lifecycle != nil { l := make(map[string]interface{}) - l["delete_after"] = aws.Int64Value(r.Lifecycle.DeleteAfterDays) - l["cold_storage_after"] = aws.Int64Value(r.Lifecycle.MoveToColdStorageAfterDays) + + if v := r.Lifecycle.DeleteAfterDays; v != nil { + l["delete_after"] = aws.Int64Value(v) + } + + if v := r.Lifecycle.MoveToColdStorageAfterDays; v != nil { + l["cold_storage_after"] = aws.Int64Value(v) + } m["lifecycle"] = []interface{}{l} } @@ -297,11 +303,13 @@ func expandBackupPlanRules(l []interface{}) []*backup.RuleInput { if len(lifecycleRaw) == 1 { lifecycle = lifecycleRaw[0].(map[string]interface{}) lcValues := &backup.Lifecycle{} - if lifecycle["delete_after"] != nil { - lcValues.DeleteAfterDays = aws.Int64(int64(lifecycle["delete_after"].(int))) + + if v, ok := lifecycle["delete_after"]; ok && v.(int) > 0 { + lcValues.DeleteAfterDays = aws.Int64(int64(v.(int))) } - if lifecycle["cold_storage_after"] != nil { - lcValues.MoveToColdStorageAfterDays = aws.Int64(int64(lifecycle["cold_storage_after"].(int))) + + if v, ok := lifecycle["cold_storage_after"]; ok && v.(int) > 0 { + lcValues.MoveToColdStorageAfterDays = aws.Int64(int64(v.(int))) } rule.Lifecycle = lcValues } diff --git a/aws/resource_aws_backup_plan_test.go b/aws/resource_aws_backup_plan_test.go index 4f99b39affc3..25a1a5d09682 100644 --- a/aws/resource_aws_backup_plan_test.go +++ b/aws/resource_aws_backup_plan_test.go @@ -27,6 +27,8 @@ func TestAccAwsBackupPlan_basic(t *testing.T) { testAccCheckAwsBackupPlanExists("aws_backup_plan.test", &plan), testAccMatchResourceAttrRegionalARN("aws_backup_plan.test", "arn", "backup", regexp.MustCompile(`backup-plan:.+`)), resource.TestCheckResourceAttrSet("aws_backup_plan.test", "version"), + resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.#", "1"), + resource.TestCheckNoResourceAttr("aws_backup_plan.test", "rule.712706565.lifecycle.#"), ), }, }, @@ -165,6 +167,50 @@ func TestAccAwsBackupPlan_withLifecycle(t *testing.T) { }) } +func TestAccAwsBackupPlan_withLifecycleDeleteAfterOnly(t *testing.T) { + var plan backup.GetBackupPlanOutput + rStr := "lifecycle_policy_two" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsBackupPlanDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBackupPlanWithLifecycleDeleteAfterOnly(rStr), + Check: resource.ComposeTestCheckFunc( + 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"), + ), + }, + }, + }) +} + +func TestAccAwsBackupPlan_withLifecycleColdStorageAfterOnly(t *testing.T) { + var plan backup.GetBackupPlanOutput + rStr := "lifecycle_policy_three" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAwsBackupPlanDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBackupPlanWithLifecycleColdStorageAfterOnly(rStr), + Check: resource.ComposeTestCheckFunc( + testAccCheckAwsBackupPlanExists("aws_backup_plan.test", &plan), + resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.1300859512.lifecycle.#", "1"), + resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.1300859512.lifecycle.0.delete_after", "0"), + resource.TestCheckResourceAttr("aws_backup_plan.test", "rule.1300859512.lifecycle.0.cold_storage_after", "7"), + ), + }, + }, + }) +} + func TestAccAwsBackupPlan_disappears(t *testing.T) { var plan backup.GetBackupPlanOutput rInt := acctest.RandInt() @@ -338,6 +384,48 @@ resource "aws_backup_plan" "test" { `, stringID, stringID, stringID) } +func testAccBackupPlanWithLifecycleDeleteAfterOnly(stringID string) string { + return fmt.Sprintf(` +resource "aws_backup_vault" "test" { + name = "tf_acc_test_backup_vault_%s" +} + +resource "aws_backup_plan" "test" { + name = "tf_acc_test_backup_plan_%s" + + rule { + rule_name = "tf_acc_test_backup_rule_%s" + target_vault_name = "${aws_backup_vault.test.name}" + schedule = "cron(0 12 * * ? *)" + lifecycle { + delete_after = "7" + } + } +} +`, stringID, stringID, stringID) +} + +func testAccBackupPlanWithLifecycleColdStorageAfterOnly(stringID string) string { + return fmt.Sprintf(` +resource "aws_backup_vault" "test" { + name = "tf_acc_test_backup_vault_%s" +} + +resource "aws_backup_plan" "test" { + name = "tf_acc_test_backup_plan_%s" + + rule { + rule_name = "tf_acc_test_backup_rule_%s" + target_vault_name = "${aws_backup_vault.test.name}" + schedule = "cron(0 12 * * ? *)" + lifecycle { + cold_storage_after = "7" + } + } +} +`, stringID, stringID, stringID) +} + func testAccBackupPlanWithRules(randInt int) string { return fmt.Sprintf(` resource "aws_backup_vault" "test" { diff --git a/website/docs/r/backup_plan.html.markdown b/website/docs/r/backup_plan.html.markdown index c98f96188e59..a37d88d631f3 100644 --- a/website/docs/r/backup_plan.html.markdown +++ b/website/docs/r/backup_plan.html.markdown @@ -46,12 +46,12 @@ For **rule** the following attributes are supported: ### Lifecycle Arguments For **lifecycle** the following attributes are supported: -* `cold_storage_after` - (Required) Specifies the number of days after creation that a recovery point is moved to cold storage. -* `delete_after` (Required) - Specifies the number of days after creation that a recovery point is deleted. Must be greater than `cold_storage_after`. +* `cold_storage_after` - (Optional) Specifies the number of days after creation that a recovery point is moved to cold storage. +* `delete_after` (Optional) - Specifies the number of days after creation that a recovery point is deleted. Must be 90 days greater than `cold_storage_after`. ## Attributes Reference In addition to all arguments above, the following attributes are exported: * `arn` - The ARN of the backup plan. -* `version` - Unique, randomly generated, Unicode, UTF-8 encoded string that serves as the version ID of the backup plan. \ No newline at end of file +* `version` - Unique, randomly generated, Unicode, UTF-8 encoded string that serves as the version ID of the backup plan.