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 ```
hashicorp · Apr 7, 2019 · 9035974 · 9035974
1 parent 72b780b
commit 9035974
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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. 
+* `version` - Unique, randomly generated, Unicode, UTF-8 encoded string that serves as the version ID of the backup plan.