r/aws_backup_plan: Adding resource for managing AWS Backup plans #7350

slapula · 2019-01-28T06:02:57Z

Reference: #7166

Changes proposed in this pull request:

Adding resource to manage AWS Backup plans: aws_backup_plan

Output from acceptance testing:

$  make testacc TESTARGS='-run=TestAccAwsBackupPlan_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupPlan_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupPlan_basic
=== PAUSE TestAccAwsBackupPlan_basic
=== CONT  TestAccAwsBackupPlan_basic
--- PASS: TestAccAwsBackupPlan_basic (11.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	11.637s

$ make testacc TESTARGS='-run=TestAccAwsBackupPlan_withRules'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupPlan_withRules -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupPlan_withRules
=== PAUSE TestAccAwsBackupPlan_withRules
=== CONT  TestAccAwsBackupPlan_withRules
--- PASS: TestAccAwsBackupPlan_withRules (11.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	11.461s

$ make testacc TESTARGS='-run=TestAccAwsBackupPlan_withRuleRemove'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupPlan_withRuleRemove -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupPlan_withRuleRemove
=== PAUSE TestAccAwsBackupPlan_withRuleRemove
=== CONT  TestAccAwsBackupPlan_withRuleRemove
--- PASS: TestAccAwsBackupPlan_withRuleRemove (19.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	19.574s

$ make testacc TESTARGS='-run=TestAccAwsBackupPlan_withRuleAdd'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupPlan_withRuleAdd -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupPlan_withRuleAdd
=== PAUSE TestAccAwsBackupPlan_withRuleAdd
=== CONT  TestAccAwsBackupPlan_withRuleAdd
--- PASS: TestAccAwsBackupPlan_withRuleAdd (19.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	19.225s

slapula · 2019-01-28T06:04:37Z

This is still a WIP. This PR's test rely on #7207. I also need to write up a few more tests before this is ready for final review.

slapula · 2019-02-02T18:39:07Z

I think this ready for review now

bflad · 2019-02-08T08:34:28Z

Can you please rebase this with master please? Thanks so much.

slapula · 2019-02-09T02:04:49Z

@bflad Done

denniswebb · 2019-02-19T19:32:14Z

@bflad Really need this resource added. Can it get some ❤️ ?

mooseracer · 2019-03-11T21:05:15Z

I get a type-related panic when trying to use Lifecycle.

`resource "aws_backup_plan" "my_backup_plan" {
name = "my_backup_plan"

rule {
rule_name = "my_backup_plan_rule"
target_vault_name = "${aws_backup_vault.my_vault.name}"
schedule = "cron(0 10 * * ? *)"

lifecycle = {
  cold_storage_after = 90
  delete_after       = 120
}

}
}`

Passes a terraform plan, but apply crashes:

panic: interface conversion: interface {} is string, not int

2019-03-12T10:00:53.081+1300 [DEBUG] plugin.terraform-provider-aws.exe: C:/Users/username/Documents/tf_aws_backup/terraform-provider-aws-backup/aws/resource_aws_backup_plan.go:267 +0xc6c

slapula · 2019-03-13T22:49:24Z

@mooseracer thanks for catching that! I'm looking into that right now. Should be an easy fix.

slapula · 2019-03-13T23:43:49Z

@mooseracer I think I fixed your issue. Could you try that again?

mooseracer · 2019-03-14T21:04:53Z

@slapula Sorry, tried it again but it's throwing the same crash errors.

bflad

Hi @slapula 👋 Thanks for submitting this. Some initial feedback provided below. Please reach out with any questions or if you do not have time to address them.

Regarding the trouble folks are seeing with lifecycle, its because the schema is incorrectly defined and is not being error checked in the Read function as noted below.

bflad · 2019-03-15T02:10:57Z

aws/resource_aws_backup_plan.go

+							Default:  180,
+						},
+						"lifecycle": {
+							Type:     schema.TypeMap,


The Terraform Provider SDK does not currently support TypeMap configuration blocks. Did you mean TypeList and MaxItems: 1?

Suggested change

Type: schema.TypeMap,

Type: schema.TypeList,

MaxItems: 1,

Is this something that has been depreciated? I seem to see TypeMap used elsewhere in this provider.

A TypeMap attribute can only currently[1] be used with a single primative type element (TypeString, TypeInt, or TypeFloat). Any Elem usage outside of that with TypeMap is an implementation bug and/or a quirk that just happens to work in the current Terraform Provider SDK and the usage may not be supported in the future.

In Terraform 0.12 core, we can start supporting a TypeMap configuration block syntax, such as:

my_configuration_block "key1" { child_argument = "" }

However its implementation is still in its design phases and is not present in the Terraform Provider SDK. See also: https://github.com/hashicorp/terraform/issues/19749#issuecomment-450256714

[1]: The underlying type system in Terraform 0.12 removes this restriction. When the Terraform Provider SDK supports dynamic elements in a future update and we upgrade to that version of the Terraform Provider SDK (requires removing Terraform 0.11 support in a later major version of the Terraform AWS Provider) we can support multiple value types in TypeMap.

Oh wow, TIL. Thanks for the explanation! 😁

aws/resource_aws_backup_plan.go

aws/resource_aws_backup_plan_test.go

bflad · 2019-03-15T02:55:05Z

aws/resource_aws_backup_plan_test.go

+		schedule			= "cron(0 12 * * ? *)"
+	}
+}
+`, randInt, randInt, randInt)


You can avoid fmt argument sprawl by using indexed formatter references, e.g.

fmt.Sprintf(` thing1 = %[1]d thing2 = %[1]d `, randInt)

website/docs/r/backup_plan.html.markdown

slapula · 2019-03-16T23:09:55Z

@bflad Thanks for the review! I believe I've addressed all of your comments:

$ make testacc TESTARGS='-run=TestAccAwsBackupPlan_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupPlan_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupPlan_basic
=== PAUSE TestAccAwsBackupPlan_basic
=== RUN   TestAccAwsBackupPlan_withRules
=== PAUSE TestAccAwsBackupPlan_withRules
=== RUN   TestAccAwsBackupPlan_withRuleRemove
=== PAUSE TestAccAwsBackupPlan_withRuleRemove
=== RUN   TestAccAwsBackupPlan_withRuleAdd
=== PAUSE TestAccAwsBackupPlan_withRuleAdd
=== RUN   TestAccAwsBackupPlan_disappears
=== PAUSE TestAccAwsBackupPlan_disappears
=== CONT  TestAccAwsBackupPlan_basic
=== CONT  TestAccAwsBackupPlan_withRuleAdd
=== CONT  TestAccAwsBackupPlan_disappears
=== CONT  TestAccAwsBackupPlan_withRuleRemove
=== CONT  TestAccAwsBackupPlan_withRules
--- PASS: TestAccAwsBackupPlan_disappears (14.37s)
--- PASS: TestAccAwsBackupPlan_withRules (15.00s)
--- PASS: TestAccAwsBackupPlan_basic (15.14s)
--- PASS: TestAccAwsBackupPlan_withRuleRemove (26.08s)
--- PASS: TestAccAwsBackupPlan_withRuleAdd (27.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	27.823s

bflad

I think two more updates with testing and this should be good to go. Thanks so much for the work here, this functionality will make a bunch of folks happy.

bflad · 2019-03-17T01:23:56Z

aws/resource_aws_backup_plan.go

+							Optional: true,
+							Default:  180,
+						},
+						"lifecycle": {


Can you please add an acceptance test that exercises this configuration block? Thanks! I think line 156 should be a something like m["lifecycle"] = []interface{}{l}

One question that's been nagging me as I've been working on these resources... How do I test for attributes in a list where the identifier appears to be a random number instead of an ordered number? For example:

rule.# = 1 rule.160206105.completion_window = 180 rule.160206105.lifecycle.# = 1 rule.160206105.lifecycle.0.cold_storage_after = 30 rule.160206105.lifecycle.0.delete_after = 160 rule.160206105.recovery_point_tags.% = 0 rule.160206105.rule_name = tf_acc_test_backup_rule_7563947093289689272 rule.160206105.schedule = cron(0 12 * * ? *) rule.160206105.start_window = 60 rule.160206105.target_vault_name = tf_acc_test_backup_vault_7563947093289689272

How do I test the attributes of rule when 160206105 changes with each test run?

Unfortunately in these cases, the randomization gets complicated for the acceptance testing. You'll probably want to make the rule names static (unless the randomization there actually matters). While it is technically possible you could try to calculate the hash of the TypeSet, once you're in this situation it is generally easier to take the resultant API object returned by the Exists TestCheckFunc and check the values within it directly, e.g.

func testAccCheckBackupPlanRules(plan *backup.GetBackupPlanOutput, expectedRules []*backup.Rule) resource.TestCheckFunc { return func(s *terraform.State) error { if a, e := len(plan.Plan.Rules), len(expectedRules); a != e { return fmt.Errorf("expected %d Backup Plan Rules, got: %d", e, a) } for _, expectedRule := range expectedRules { for _, rule := range plan.Plan.Rules { if aws.StringValue(rule.RuleName) != aws.StringValue(expectedRule.RuleName) { continue } ruleName := aws.StringValue(rule.RuleName) if a, e := aws.StringValue(rule.Schedule), aws.StringValue(expectedRule.Schedule); a != e { return fmt.Errorf("expected Backup Plan Rule (%s) schedule to match (%s), got: %s", ruleName, e, a) } // Check other various attributes as appropriate break } } return nil } } // In a TestStep, filling in rule details as necessary testAccCheckAwsBackupPlanExists("aws_backup_plan.test", &plan), testAccCheckBackupPlanRules(&plan, []*backup.Rule{ { RuleName: aws.String("test"), Schedule: aws.String("cron(0 12 * * ? *)"), }, })

As an added assurance to the above, acceptance tests that implement TestSteps with ImportStateVerify: true will automatically ensure that the resource read function is correctly able to re-read the API object and generate the same Terraform state.

Ah ok, that makes sense. There really isn't a requirement for the plan to be unique other than to help me avoid running into dangling resources while testing. For this test, I'm going to opt to not make it use the random int so I can reliably test for the existence of the lifecycle.

aws/resource_aws_backup_plan.go

slapula · 2019-03-17T22:12:36Z

@bflad I believe I've addressed your last round of comments. I've implemented the acceptance test for lifecycle blocks. I've also added the missing read/update functionality for tags as well as adding the related acceptance tests for that too. Let me know if there's anything else I need to look at.

bflad

Thanks so much, @slapula 🚀 (TravisCI failures related to #7993)

--- PASS: TestAccAwsBackupPlan_disappears (11.86s)
--- PASS: TestAccAwsBackupPlan_basic (12.99s)
--- PASS: TestAccAwsBackupPlan_withRules (13.14s)
--- PASS: TestAccAwsBackupPlan_withLifecycle (13.26s)
--- PASS: TestAccAwsBackupPlan_withRuleRemove (20.08s)
--- PASS: TestAccAwsBackupPlan_withRuleAdd (20.34s)
--- PASS: TestAccAwsBackupPlan_withTags (31.76s)

mooseracer · 2019-03-21T00:49:47Z

Thanks for all the work on this! I'd just like to report a logical bug with the Lifecycle -- it should be able to accept values of "delete_after" when "cold_storage_after" is left unspecified. Right now it's always passing "cold_storage_after" to the API and it gets validated against their logic: lifecycle = { delete_after = 15 } Error in rule backup-plan-rule : Invalid lifecycle. DeleteAfterDays cannot be less than 90 days apart from MoveToColdStorageAfterDays status code: 400 This represents a simple scenario where you don't want to transition to cold storage and only keep X days of snapshots. It's supported by the API; you can set it in the GUI and the following JSON is accepted by AWS CLI: `{ "BackupPlan": { "BackupPlanName": "myplan", "Rules": [ { "RuleName": "myrule", "TargetBackupVaultName": "Default", "ScheduleExpression": "cron(0 10 * * ? *)", "Lifecycle": { "DeleteAfterDays": 15 } } ] } }`

slapula · 2019-03-21T05:20:23Z

@mooseracer Can you submit this as an issue? I noticed some weirdness with these API calls when I was working on this and if I remember correctly AWS wasn't letting me submit requests with any empty or zero-valued parameters. As you can see, I made them both required to work around the issue. I'll need to take another look to confirm all of this and think of a better solution.

bflad · 2019-03-21T20:50:54Z

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 · 2020-03-31T17:12:07Z

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!

slapula added 3 commits January 25, 2019 19:48

initial commit

d98fd21

fixing merge conflicts

912f5ea

resource working and basic test passing

5854329

removing double-quotes from regex

0acabf2

ewbankkit mentioned this pull request Jan 28, 2019

Support for AWS Backup #7166

Closed

slapula added 6 commits January 28, 2019 22:39

Merge branch 'master' into resource-aws-backup-plan

2fc7848

Merge branch 'master' into resource-aws-backup-plan

016df03

updating documentation

716fd40

fixing merge conflict

fe9a8fe

removing cruft

e709860

adding tests

1343426

bflad added new-resource Introduces a new resource. service/backup Issues and PRs that pertain to the backup service. labels Feb 4, 2019

Merge branch 'master' into resource-aws-backup-plan

087dc71

Merge branch 'master' into resource-aws-backup-plan

2e6be1b

fixing plan rule hash func

3fd0a66

bflad reviewed Mar 15, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 15, 2019

slapula added 2 commits March 16, 2019 11:13

Merge branch 'master' into resource-aws-backup-plan

f5cb223

changes based on review feedback

7d02977

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 16, 2019

bflad reviewed Mar 17, 2019

View reviewed changes

bflad added this to the v2.3.0 milestone Mar 17, 2019

bflad self-assigned this Mar 17, 2019

slapula added 2 commits March 17, 2019 13:16

more changes based on feedback

9562b2e

adding read/update tagging functions and tests

9c2bf32

bflad approved these changes Mar 18, 2019

View reviewed changes

bflad merged commit dcd6d1c into hashicorp:master Mar 18, 2019

bflad added a commit that referenced this pull request Mar 18, 2019

Update CHANGELOG for #7350

e83dd1a

sbutler mentioned this pull request Apr 2, 2019

aws_backup_plan lifecycle cold_storage_after should be optional #8151

Closed

atward mentioned this pull request Apr 10, 2019

Formatting fixes for backup_plan and backup_vault examples #8264

Merged

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_backup_plan: Adding resource for managing AWS Backup plans #7350

r/aws_backup_plan: Adding resource for managing AWS Backup plans #7350

slapula commented Jan 28, 2019 •

edited by bflad

Loading

slapula commented Jan 28, 2019

slapula commented Feb 2, 2019

bflad commented Feb 8, 2019

slapula commented Feb 9, 2019

denniswebb commented Feb 19, 2019

mooseracer commented Mar 11, 2019

slapula commented Mar 13, 2019

slapula commented Mar 13, 2019

mooseracer commented Mar 14, 2019

bflad left a comment

bflad Mar 15, 2019

slapula Mar 16, 2019 •

edited

Loading

bflad Mar 17, 2019

slapula Mar 17, 2019

bflad Mar 15, 2019

slapula commented Mar 16, 2019

bflad left a comment

bflad Mar 17, 2019

slapula Mar 17, 2019

bflad Mar 17, 2019

slapula Mar 17, 2019

slapula commented Mar 17, 2019

bflad left a comment

mooseracer commented Mar 21, 2019 via email •

edited

Loading

slapula commented Mar 21, 2019

bflad commented Mar 21, 2019

ghost commented Mar 31, 2020

r/aws_backup_plan: Adding resource for managing AWS Backup plans #7350

r/aws_backup_plan: Adding resource for managing AWS Backup plans #7350

Conversation

slapula commented Jan 28, 2019 • edited by bflad Loading

slapula commented Jan 28, 2019

slapula commented Feb 2, 2019

bflad commented Feb 8, 2019

slapula commented Feb 9, 2019

denniswebb commented Feb 19, 2019

mooseracer commented Mar 11, 2019

slapula commented Mar 13, 2019

slapula commented Mar 13, 2019

mooseracer commented Mar 14, 2019

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slapula Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slapula commented Mar 16, 2019

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slapula commented Mar 17, 2019

bflad left a comment

Choose a reason for hiding this comment

mooseracer commented Mar 21, 2019 via email • edited Loading

slapula commented Mar 21, 2019

bflad commented Mar 21, 2019

ghost commented Mar 31, 2020

slapula commented Jan 28, 2019 •

edited by bflad

Loading

slapula Mar 16, 2019 •

edited

Loading

mooseracer commented Mar 21, 2019 via email •

edited

Loading