provider/aws: Add aws_cloudformation_stack

[radeksimko]

Status

• Docs
• Creation of a stack
• Update of a stack
• Deletion of a stack
• Acceptance tests
• Proper events parsing & progress report - I'll see how easy/difficult it is, but this would be nice to have for UX - too difficult at this point, can be added later

Test plan

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=CloudFormation' 2>~/tf.log

go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=CloudFormation -timeout 90m
=== RUN   TestAccAWSCloudFormation_basic
--- PASS: TestAccAWSCloudFormation_basic (59.53s)
=== RUN   TestAccAWSCloudFormation_defaultParams
--- PASS: TestAccAWSCloudFormation_defaultParams (62.86s)
=== RUN   TestAccAWSCloudFormation_allAttributes
--- PASS: TestAccAWSCloudFormation_allAttributes (58.48s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    180.900s

[ctiwald]

This is an extraordinarily meta feature.

[JeanMertz]

Nice 👍 looking forward to this.

[bhourigan]

I'm experimenting with your feature branch and I'm running into an error when trying to update a CF stack. When this happens the state file becomes corrupt and terraform thinks it already updated the stack on subsequent runs. I spent some time debugging this but got nowhere.

aws_cloudformation_stack.consul: Modifying...
<...>
Error applying plan:

1 error(s) occurred:

* aws_cloudformation_stack.consul: ValidationError: Either Template URL or Template Body must be specified.
    status code: 400, request id: [xxxxxxxxxx-xxxx-xxxxxxxx-xxxxxxxxxx]

I do have template_body defined in my resource - which gets initially created just fine.

P.S. If you haven't seen it I think #1778 will help manage templates

[radeksimko]

@bhourigan Thanks for testing it! Would you mind sharing the template_body used so I can reproduce it?

I have seen template_file and I do expect people to use templates w/ CloudFormation, but I wanted to keep tests as isolated as possible -> e.g. if there was a bug in templates, I don't want CF tests to fail - that would be a false positive. Although I may add one test that will be using templates and make it obvious, that we're testing both, so whoever ends up debugging that doesn't think that there's an issue w/ CloudFormation resource.

[bhourigan]

No problem at all - I can reproduce this reliably. Here's my cloudformation.tf https://gist.github.com/bhourigan/1baa3ec39905eb016da6 which includes the template and all other resource parameters.

[bhourigan]

I didn't find issues enabled for http://github.com/timeincoss/terraform - so bug reports are going here. If there is a more appropriate location for these happy to oblige.

During testing I munged my tfstate file and was forced to remove the CF stack via the web console. When I went to re-apply the refresh didn't pick up that the stack needs to be re-created.

• aws_cloudformation_stack.consul: ValidationError: Stack with id consul-dev does not exist
  status code: 400, request id: [XXXXXXXX-XXXX-XXXXXXX-XXXX-XXXXXXXXXX]

[radeksimko]

This is the right place for bugs. I will have a look into that probably during the upcoming weekend or the next one. I wanted to finish some other bits & send PRs for Google provider-related stuff first.

Thanks for providing the gist though.

[radeksimko]

@bhourigan This was an issue of missing fields for Update, should be fixed now. I also slightly improved error reporting during rollbacks, but I'm still not sure if that's the best approach to this.

If there is *_FAILED status coming from update or create, it will pull down any failures from events log, but I need to find a way how to only get those which happened during the creation/update, not all.

[bhourigan]

I've been trying to test this - unsuccessfully. I don't know whether something is broken in my environment or if there is a regression introduced in your latest commits.

I'm on b7cd361 of f-aws-cloudformation.

Build steps:

$ mv ~/gocode ~/gocode.attic
$ go get github.com/TimeIncOSS/terraform
$ cd ~/gocode/src/github.com/TimeIncOSS/terraform
$ git checkout f-aws-cloudformation
$ make updatedeps
$ make dev

I'm getting the following error when running terraform plan

  * aws_cloudformation_stack.consul: Provider doesn't support resource: aws_cloudformation_stack

I run your tests and they seem to pass:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=CloudFormation'
<...>
ok      github.com/timeincoss/terraform/builtin/providers/aws   61.566s

I'm using the right binary in $PATH:

$ which terraform terraform-provider-aws
/Users/bhourigan/gocode/bin/terraform
/Users/bhourigan/gocode/bin/terraform-provider-aws

It matches the latest build in bin/:

$ md5 ~/gocode/bin/terraform ~/gocode/bin/terraform-provider-aws ~/gocode/src/github.com/TimeIncOSS/terraform/bin/terraform ~/gocode/src/github.com/TimeIncOSS/terraform/bin/terraform-provider-aws
MD5 (/Users/bhourigan/gocode/bin/terraform) = a0785a9eefd0950f0e805a10ea00ab92
MD5 (/Users/bhourigan/gocode/bin/terraform-provider-aws) = ef87f05674f366e89cef641776c843db
MD5 (/Users/bhourigan/gocode/src/github.com/TimeIncOSS/terraform/bin/terraform) = a0785a9eefd0950f0e805a10ea00ab92
MD5 (/Users/bhourigan/gocode/src/github.com/TimeIncOSS/terraform/bin/terraform-provider-aws) = ef87f05674f366e89cef641776c843db

When I run strings on the terraform binary I don't see any references to cloudformation, but I can see references to other resources.

Is this PEBKAC on my part?

[woodhull]

This PR has been open for a long time & is important to us. Is there anything I can do to move it forward?

I think progress report is the only thing not implemented, has anyone else already started on that?

[bhourigan]

I'd love to continue testing - but I'm unable to.

[radeksimko]

@woodhull Sorry for the silence here. I plan to get back to this PR probably this week. I want to add more acceptance tests with more complicated examples.

I'm afraid that proper progress reporting will require bigger changes in the core - ain't saying it's impossible, just a bit more difficult, than you may imagine. Calling fmt.Print* directly from the resource is not a good approach. Maybe an optional channel being passed into resource or so is the way.

I was more thinking of merging this without progress reporting - just check more thoroughly for bugs and issues & then let someone else review this. Feature-wise it should be complete.

Is progress reporting critical feature - i.e. if there was no detailed progress reporting, would you not be using this resource at all?

@bhourigan I'll try to reproduce that, but I have never been compiling custom build like this. I usually use git remotes:

cd $GOPATH/src/github.com/hashicorp/terraform
git remote add TimeIncOSS https://github.com/TimeIncOSS/terraform
git pull TimeIncOSS
git checkout f-aws-cloudformation
make updatedeps
make dev

[woodhull]

Progress reporting is not a critical feature for us.

[radeksimko]

@bhourigan

If the CF template has an invalid parameter and a rollback is initiated terraform can't keep up with the state:

That's now fixed. From now on any errors coming from _FAILED$ or ^ROLLBACK_ events will be displayed. I was only catching _FAILED$ previously.

After I fixed the error, I re-applied however terraform didn't detect that the stack was in ROLLBACK_COMPLETE state.

I'm not quite convinced Terraform should automate this. If you chose the CloudFormation workflow, I'd guess you did so for reason - i.e. if creation/update fails, you probably want to inspect the state.

The "clean way" of approaching that problem would be probably

terraform taint aws_cloudformation_stack.my_stack

Do you really think we should be tainting the resource automatically or even destroying+creating again? It feels too magical to me.

Current status

A few acceptance tests was added covering basic functionality, I'm just trying to find out how to parse events reliably due to NextToken and pagination that may occur in some cases.
It is not clear what is the limit per page, so I raised question to the AWS support and I'm awaiting response.

[bhourigan]

That's now fixed. From now on any errors coming from FAILED$ or ^ROLLBACK events will be displayed. I was only catching _FAILED$ previously.

👍

I'm not quite convinced Terraform should automate this. If you chose the CloudFormation workflow, I'd guess you did so for reason - i.e. if creation/update fails, you probably want to inspect the state.

I must confess that the only thing I want from CloudFormation is the AutoScalingRollingUpdate policy. Since this functionality is not exposed via AWS APIs I'm forced to consume CF instead. I admit I don't have enough experience with the CF paradigm to offer an informed opinion.

The "clean way" of approaching that problem would be probably

terraform taint aws_cloudformation_stack.my_stack
Do you really think we should be tainting the resource automatically or even destroying+creating again? It feels too magical to me.

I don't think terraform should ever destroy a resource unless you explicitly ask it to. I agree it's difficult to code around all possible CF policies but I think at the very least terraform should report deltas between my desired infrastructure and the currently applied infrastructure, without depending on catching errors from previous runs.

[radeksimko]

@bhourigan I must admit I'm slightly confused now 😃

I would like to depend on Terraform to let me know if a CF stack creation or update resulted in anything other than success. As part of a CI workflow this feedback is important.

but you're getting that feedback now, aren't you?

I don't think terraform should ever destroy a resource unless you explicitly ask it to.

Exactly, but AFAIK the only way out of failed creation is to destroy the stack and create it from scratch again, which you can actually even recognise from the AWS Console that has "Update" button greyed out when this happens.

How do you want to solve this then?

I think at the very least terraform should report deltas between my desired infrastructure and the currently applied infrastructure, without depending on catching errors from previous runs.

That should be happening since the last commit too. CF has a special API endpoint just for templates which I'm using there. The diff may not be as useful as you'd hoped, because it's minified - i.e. spaces removed etc., but you should have that functionality there.

Give it a try and let me know if there's something else you'd change/improve.
I think it's very close to review/merge.

[bhourigan]

Give it a try and let me know if there's something else you'd change/improve.
I think it's very close to review/merge.

NP - I should of tested before commenting.

[radeksimko]

So I finally managed to finish this PR. It's ready for review/merge. A few different acceptance tests is attached too.

[catsby]

Hey @radeksimko sorry for the delay, this looks good! Merge away 😄

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.