New resource: aws_dlm_lifecycle_policy

[tomelliff]

Fixes #5176

Changes proposed in this pull request:

Adds the aws_dlm_lifecycle_policy resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSDlmLifecyclePolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSDlmLifecyclePolicy -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSDlmLifecyclePolicyBasic
--- PASS: TestAccAWSDlmLifecyclePolicyBasic (27.65s)
=== RUN   TestAccAWSDlmLifecyclePolicyFull
--- PASS: TestAccAWSDlmLifecyclePolicyFull (45.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	73.213s
...

This PR is currently WIP for a few reasons:

1. The AWS SDK appears to be failing to parse the timestamp for the created/modified time so falls over in a big heap. I've raised an issue against the SDK in case there's a way to work around it that I'm unaware of but in the meantime I locally patched the SDK in the vendor folder (not pushed) with the diff mentioned in the aforementioned issue.
2. The resource is failing to read state back from AWS properly for some reason so changes made outside of Terraform don't end up being refreshed back into the state. This is immediately evident with the commented out test for the policy_details.0.schedule.0.create_rule.0.times attribute in the basic test as AWS provides a value if you omit it but this doesn't end up back in the state currently. I'm not sure what I'm doing wrong here because there's nothing in the logs and no panics when refreshing state so would love a pointer here. The heavily nested structure of the API is much more complicated than other resources I've worked on so I guess I've ballsed something up here.
3. Need to add docs for the resource.
4. There's a couple of outstanding TODOs on the resource, mostly around validation but also for creating the default service role if it doesn't exist and it's not specified. This could maybe be done in a separate PR but would be a nice thing to do here.

I'll rebase the PR into a single commit after

[bflad]

Sorry I don't have time to give in depth review at the moment but to unblock you (hopefully) you can get a better handle on what's wrong with d.Set("policy_details", ...), if return an error from it. 😄

if err := d.Set("policy_details", flattenDlmPolicyDetails(out.Policy.PolicyDetails)); err != nil {
  return fmt.Errorf("error setting policy_details: %s", err)
}

At very quick glance of the flatten functions, here are a few hints:

• You'll need to be returning a []map[string]interface{} not just map[string]interface{} to account for the "container" attribute that is a schema.TypeList
• You'll need to dereference the API response pointer values (recommended with the SDK functions like aws.StringValue(), aws.Int64Value(), etc.)
• You'll want to return []map[string]interface{}{} if the API response object is nil before trying to reference attributes within the object

As an example:

func flattenDlmCreateRule(createRule *dlm.CreateRule) []map[string]interface{} {
 	if createRule == nil {
 	 	return []map[string]interface{}{}
 	}

 	result := make(map[string]interface{})
 	result["interval"] = aws.Int64Value(createRule.Interval)
 	result["internal_unit"] = aws.StringValue(createRule.IntervalUnit)
 	result["times"] = flattenStringList(createRule.Times)

 	return []map[string]interface{}{result}
}

You can also help verify the resource read function by adding import support and using ImportStateVerify testing:

// in resource schema definition
Importer: &schema.ResourceImporter{
	State: schema.ImportStatePassthrough,
},

// adding an acceptance testing TestStep
{
	ResourceName:      resourceName,
	ImportState:       true,
	ImportStateVerify: true,
},

[tomelliff]

@bflad thanks for the pointers, think I've got it working now.

I need to rerun the tests (not had a chance to run them since adding the import test) and then squash the mess of commits here.

We also still need to wait for AWS to fix the API response as per the previously linked issue on the AWS SDK before this can be merged.

[tomelliff]

Does this want doing in first pass or should it be done later? I'm tempted to do the work for this if AWS take a long time fixing the API response which is blocking this.

[bflad]

I think for starters that we can omit the handling in the code (so people can try this out) and document the solution on the website.

[ryan-dyer-sp]

Any update on this?

[tomelliff]

I'm still waiting for AWS to fix the response of the API. I've not checked again recently but the linked issue is still open so I'm leaving it until it's closed.

After that I'll fix up the remaining TODOs on this PR and then it should be good for review.

[tomelliff]

I've been really busy at work lately so not had any chance to come back and look at the review comments until now and no incentive to do so either with the DLM SDK/API response still being broken (chased AWS about it recently but it's still not fixed).

I've just pushed fixes for all the feedback now and think it's good to go once the DLM API response is fixed (which should be announced in the linked issue or I can respond here when my support ticket is finally closed). When the DLM API response is fixed I'll rebase the branch to fix conflicts and squash the commits.

I've implemented the IntInSlice validation func here but think it probably should go upstream. I'll raise a PR for that over in core and if it gets merged we can remove it from here.

[tomelliff]

Just got a reply from AWS support saying the DLM API fix is being deployed across all regions now and they'll let me know when it's fully rolled out. After that I'll test it without my patch to the SDK, rebase the branch to fix merge conflicts and squash the commits.

[bflad]

@tomelliff let us know if you need anything!

[tomelliff]

Had a bit of a monster rebase there, let me know if I screwed anything up.

Acceptance test output after rebase:

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

[goochi1]

has this been merged? cant see it on the site
https://www.terraform.io/docs/providers/aws

[bflad]

Fantastic work, @tomelliff! Thanks so much for working with AWS on the SDK issue. A few little minor things, but those were easy enough to quickly fix on merge so we can get this released today or tomorrow! 🚀

--- PASS: TestAccAWSDlmLifecyclePolicy_Basic (15.16s)
--- PASS: TestAccAWSDlmLifecyclePolicy_Full (20.28s)

[bflad]

Nit: We should return context about errors so operators and code maintainers can more easily troubleshoot issues, e.g.

return fmt.Errorf("error creating DLM Lifecycle Policy: %s", err)

Same with other similar messaging during Read/Update/Delete. 👍

[bflad]

Two things here:

• We have a helper for handling this type of logic, e.g. isAWSErr(err, dlm.ErrCodeResourceNotFoundException, "")
• We should not create a special case during Read after Create that returns an error -- instead if this resource has eventual consistency issues, we should implement a resource.Retry() loop that retries for a minute or two (then can use d.IsNewResource() to determine if its a retryable error).

[bflad]

Nit: When triggering resource recreation, we should return a warning message in the logs so operators and code maintainers can more easily diagnose why it occurred, e.g.

log.Printf("[WARN] DLM Lifecycle Policy (%s) not found, removing from state", d.Id())
d.SetId("")

[bflad]

To prevent potential panics, we should length and nil check the slice before accessing its first element, e.g.

if len(cfg) == 0 || cfg[0] == nil {
  return nil
}
m := cfg[0].(map[string]interface{})

[tomelliff]

Ack yeah, I keep missing these. Is there a linting rule or something we can use to check for this?

[bflad]

To prevent potential panics, we should prefer to use the SDK helpers in this case, e.g.

result[aws.StringValue(t.Key)] = aws.StringValue(t.Value)

[bflad]

This function should implement a similar (but opposite) check to the CheckDestroy, where it verifies that the expected resource does actually exist in the API. Admittedly, this is a little buried in the provider development documentation, here: https://www.terraform.io/docs/extend/testing/acceptance-tests/teststep.html#custom-check-functions

return func(s *terraform.State) error {
  rs, ok := s.RootModule().Resources[name]

  if !ok {
    return fmt.Errorf("Not found: %s", name)
  }

  if rs.Primary.ID == "" {
    return fmt.Errorf("Resource %s ID not found", name)
  }

  input := dlm.GetLifecyclePolicyInput{
    PolicyId: aws.String(rs.Primary.ID),
  }

  out, err := conn.GetLifecyclePolicy(&input)

  if err != nil {
    return fmt.Errorf("error getting DLM Lifecycle Policy (%s): %s", rs.Primary.ID, err)
  }

   return nil
}

[bflad]

Nit: This was written before resource.ParallelTest() was available so we'll update these. 👍

[bflad]

Nit: This would've been found with resource.ParallelTest() (had it existed back then!), but resource names like these need to be randomized (check out other testing with rName variables).

[bflad]

Merge note: Missing sidebar link in website/aws.erb

[tomelliff]

@bflad thanks for the review, fixes and merge :)

This has been the largest PR I've raised for Terraform and I had a lot of issues with the (unnecessarily in some places IMO) deeply nested API of DLM. I don't get any other opportunities other than PRs to Terraform to write any Go so I learned a ton from this.

[bflad]

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

[ghost]

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!