terraform support

[kapilt]

Issue: closes #1121

This adds in terraform support for chalice package. There's a few goals underlying.. one was to provide provisioning flexibility and extensibility instead of having to layer in multiple provisioning executions for an app (ie reference an s3 bucket, or use chalice lambda function for a step function, etc). Another was to enable chalice use more naturally in orgs that mandate/prefer terraform for infrastructure provisioning.

I've done a few manual e2e tests on a sample app (api gw, 5 lambdas, scheduled func, managed role). The end result is actually a bit faster than chalice's direct api calls for deploy/destroy.

https://gist.github.com/kapilt/1794c092cd2f17e0faadb3e00a44bc33

[codecov-io]

Codecov Report

Merging #1129 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   95.89%   95.98%   +0.09%     
==========================================
  Files          28       28              
  Lines        5068     5183     +115     
  Branches      641      660      +19     
==========================================
+ Hits         4860     4975     +115     
  Misses        135      135              
  Partials       73       73

Impacted Files	Coverage Δ
chalice/cli/__init__.py	87.17% <100%> (+0.19%)	⬆️
chalice/cli/factory.py	92.4% <100%> (ø)	⬆️
chalice/package.py	100% <100%> (ø)	⬆️
chalice/deploy/swagger.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf0cad0...53a55a8. Read the comment docs.

[kapilt]

Also worth noting.. This targets terraform versions under < 0.12 at the moment. Terraform just had a major release (0.12) last week which has some significant incompatibilities with previous versions.

https://www.terraform.io/upgrade-guides/0-12.html

[kapilt]

fwiw, tested against 0.12 seems to work fine, i've removed that restriction in the generated output.

to elaborate on some of the converged/combined provisioning capability, you can create a dynamodb table, sqs queue in terraform, and then direct reference them as env vars for the chalice app.. an example from a sample app i'm using (config.json).

        "environment_variables": {
            "TABLE_GITTER": "${aws_dynamodb_table.gitter.name}",
            "GITTER_TOKEN_PARAM": "${aws_ssm_parameter.gitter_token.name}",
            "ELASTICSEARCH_HOST": "${aws_elasticsearch_domain.gitter.endpoint}"
        }

at least for me this removes the need for post processing scripts, or splitting the app into multi stack deployments, etc. the one item i've been considering for enabling better/forward backward navigation of references is capturing the app name / chalice stage name as terraform variables for easier referencing, the extant generator interface doesn't allow for that at the moment though, so it will require some additional refactoring, perhaps as a followup.

[stealthycoin]

This would be really cool to add. What do you think about integration tests for terraform?

I will have to go through this in more detail since I'm not very familiar with terraform so I gave it a quick once over. But Ill have to block off some time to play around with it.

I wonder as we add more backends if we need to add some documentation matrix of features to backend support.

[kapilt]

I'm a little unfamiliar with the integration tests in chalice, and how one actually executes them or what the ci harness is for them to run, i've mostly been using make prcheck around test running. i'd be game for one, or even some additional tests for this on the generated output, but terraform itself is pretty stable in its usage. Is there any docs on how to run the integration tests?

Wrt to additional backends, i'm not sure if there's others that are meaningful to support (maybe cdk?), terraform integration covers off on a wide variety of use cases that cfn doesn't handle by itself well. there's definitely costs to each backend, as there will be differential feature support by backend, and new features need to be added to all of them. Your websocket branch is an example of one such, as terraform doesnt yet have support for that. update there's a pr open for websocket support in terraform now hashicorp/terraform-provider-aws#8881

One note, i do suspect it would be possible to enable automatic transition from deploy/destroy to terraform, just not sure its worth the effort, would require a separate script to transition the client side state from one to the other.

[haduart]

@kapilt Is this already merged? If I don't understand it wrong, with this terraform support feature, aws chalice should be able to not only generate a cloudformation template but also terraform scripts? is this correct?

[kapilt]

@haduart its not merged yet, pip has solid support for installing from a branch, example below. yes this branch effectively supports provisioning/deploying a chalice app with just terraform

$ virtualenv xyz
$ ./xyz/bin/pip install -e git+https://github.com/kapilt/chalice.git@terraform-package#egg=chalice
$ ./xyz/bin/chalice start-project mydev
$ cd mydev
... do some chalice dev

# now deploy with terraform

$ ./xyz/bin/chalice package --pkg-format terraform tfout
$ cd tfout
$ terraform init
$ terraform apply

There's a readme with some examples in this pr, but real world uses. Also fwiw, since its taking some time to merge, i've started to maintain an integration branch with most of my pull requests here integrated ( cloud watch events support ) etc called next albeit thats subject to change, but feedback appreciated re kicking the tires. i'm using it for some projects atm.

[kyleknap]

This is awesome. To test it out more fully, I converted one of my workshop examples that previously relied on deploying a separate CFN stack to use terraform: kyleknap/chalice-workshop@73bcfec. It worked out really well in the end. The ability to cross reference resources from other terraform modules is super useful. It definitely cleaned up my workshop application. I did run into a couple of issues trying to convert the application. I noted it in the review. I still need to go through most of the code. So I will have a follow up review with anything I find.

[kyleknap]

Went through the code more. It's looking pretty good. I had some more comments that I wanted to capture to keep the feedback process moving. I think in general like the interface and how the template gets filled out. The two more things that I want to spend more time on are: 1) Double checking the resource names currently being used are the ones we want. Specifically, "logical" resource names are something we can't change given you can cross-reference resources across terraform files. 2) Try deploying more chalice resources of different types and configuration to make sure we did not miss anything.

[kyleknap]

It's looking good to me. I just had a few more comments based on feedback. The last thing I still wanted to do is test more of the various decorators/resources/configurations chalice supports.

[kyleknap]

Just making a general comment for future development. I went through the resource names we are generating. They look fine too me. The one thing that we should address (but do not need to now) is add tests to make sure that we do not accidentally change resource_names and have that cause a change in the resource name in the generated template. This is especially true for terraform as people will be able to reference resources from the generated chalice template to other templates. The backwards compatibility guarantee will be needed for this PR as well: #1195

[kyleknap]

Looks good. I just finished testing some of the sample applications with terraform. Overall, it worked pretty well. There were a couple of issues that I ran into that I posted in the review, but I say we are good to merge this once:

1. Fix (if possible) the issues I ran into
2. Add a change log entry
3. Squash all of the commits into a single commit to clean up the history a bit

[kapilt]

re 3. squashing commits should be done ideally on merge within the GitHub ui, and can be setup as mandatory for pull requests merge on GitHub project settings. The chalice project has not historically done this to date, and rather than doing so arbitrarily on some pull requests it would be better to do so uniformly by doing this via a project setting. Generally doing rebases often on pull requests is typically considered an anti-pattern by project reviewers once they've commented/reviewed as they have to reconsider the whole diff on changes as to opposed post review changes.

currently looking into 1 & 2

[kapilt]

I’ve been noodling again if we should be outputting the stage name as a variable here, so that other resources can use it as a prefix when provisioning to have isolated stacks. It effectively it would become a terraform variable, but we’d have to note that it can’t be user supplied via terraform in the chalice docs though.

[kyleknap]

Could you elaborate or give an example on how the stage variable would look in the generated terraform template?

[kyleknap]

Otherwise. I don't think I have any more feedback on the code. Confirmed that the updates resolve the two issues I ran into.

[kapilt]

re stage-name, so when provisioning physical assets with terraform its useful to prefix them with app-name, stage-name, so that multiple chalice stages can be deployed in the same account.

Could you elaborate or give an example on how the stage variable would look in the generated terraform template?

as an example in one of my terraform'd chalice apps i have a kinesis delivery stream (non relevant parts omitted), atm its currently using static names, the change would be to introduce chalice stage and app name variables so they can be referenced when constructing physical asset names as below.

resource "aws_kinesis_firehose_delivery_stream" "gitter_index" {
  name        = "${var.chalice_app_name}-{var.chalice_stage_name}-gitter-s3-archive"
  destination = "elasticsearch"

  # omit various non germane configuration
}

To support this we would add terraform variables with default values to the terraform module.

Generators don't have access to the app name or chalice stage name atm (though they can infer from rest api stage name or lambda info tags). The clean way of doing this would be updating generator class constructor signature to also take a chalice Config.

The caveat is that these chalice prefix'd variables can't be user defined via the cli, because other chalice generate assets embeds the literal values of those variables into output'd resources. The description for the variables and the chalice documentation would both note this restriction.

--

One other item in looking at the chalice package cli, it feels like this needs a bit more help text.

$ chalice package --help
Usage: chalice package [OPTIONS] OUT

Options:
  --single-file            Create a single packaged file. By default,
                                  the 'out' argument specifies a directory in
                                  which the package assets will be placed.  If
                                  this argument is specified, a single zip
                                  file will be created instead.
  --stage TEXT
  --pkg-format [cloudformation|terraform]
  --merge-template TEXT           Specify a JSON template to be merged into
                                  the generated template. This is useful for
                                  adding resources to a Chalice template or
                                  modify values in the template.
  --help                          Show this message and exit.

ie pkg-format Specify Provisioning Engine to be used for generated template output

and also note for --single-file and --merge-template options that they only apply to cloudformation engine.

[kapilt]

i've gone ahead and added in terraform variables for chalice app_name and stage_name, and updated the cli help.

[kapilt]

added cli compatibility with via local addition of deployed_resources json stub to support subcommands logs/invoke/url post terraform apply.

[jamesls]

Nice! I've been playing around with this. I noticed that the deployed/dev.json changes are causing issues for me. I'd like to do more testing on that before we support writing out that json file. What do you think about holding off on this for now? We can always add this later given there's no backwards compatibility issues, and I think this PR without the deployed/dev.json changes are already useful as is, and I'd like to get an initial version of this merged.

[kyleknap]

@kapilt Thanks for the updates. I had a some comments/ideas about the updates:

1. I agree with James we should hold off on the deployed.json in this PR so we can get this merged. I think for me, I'd like to see if we can refactor the code so the chalice deployer and the chalice packager can leverage the same abstractions so we can ensure the deployed.json file will be close to the same for the chalice deployer backend and the terraform backend.

2. I like adding the idea of exposing the stage name and app name in the Terraform template. While the package command does not expose the stage name and app name for CloudFormation, I think it makes sense for terraform as users are put in a situation where they need to the name the resources themselves (which is typically handled for your behalf) and the app name along with stage name will help preserve uniqueness if they have multiple stages. But I'm wondering if this is another addition that we should consider holding off for in this PR and do it in a follow up PR? It would be backwards compatible to add the variables in the future and it seems weird right now to have variables that act more like outputs and do not actually affect the generation of the resources in the template.

3. I think we should be having CLI runtime validations for the --merge-template and --single-file parameters when the --pkg-format is set to terraform. Having it in the help documentation is nice, but it really should raise an error to better help the user when they did not thoroughly read the documentation.

In general, I think it would be nice to address the variable and deployed.json changes in a subsequent PRs as an incremental updates. Even before those two updates, we were really close to getting this merged and into a release, and I'd like to see that happen sooner rather than later especially since those two updates would not introduce any backwards incompatible changes.

[kapilt]

1. happy to back out the deployed/$stage.json, will file as a separate pull request once this pr gets merged, although curious what issues were seen.
2. exposing of the app and stage names is important imo, and there isn't any other way to do this. they are not weird imo, they are used in the exact way variables in terraform are intended to be use, ie for controlling resource generation, the reality is that anyone using the terraform is likely provisioning the rest of their app (buckets, DynamoDB, sqs, sns) alongside. adding later, is only backwards compatible if we ignore that everyone who needs this has to rewrite their terraform config, ie I consider this pretty basic functionality for real world usage, and its usage/constraint is documented.
3. sounds good re validation.

[kyleknap]

Just replying back...

1. I did not try it out but just skimming over the code for what would have been generated there were a couple of things I noticed: It made the assumption that there was always a route() in the app so if you had a chalice app with a single lambda_function, my guess is that would fail. Also, I believe we capture more than just lambda functions and the api gateway resources in the deployed.json such as iam roles and event sources, and while it is not necessarily needed for the logs, url, and invoke commands, it would be nice to output relatively the same file in case we add a command that needs the value of these deployed resources.

2. I guess it may be weird to me because I don't have too much experience with Terraform. I guess is it an accepted practice in Terraform to have read-only input variables, that are really only used for referencing? I looked more into the available functionality such as data sources, local values, or even if there was a resource available for it, but have not really found anything that fit the bill for being a grab bag of read-only data. I'll defer to you as you have more experience with Terraform, but I want to make sure that we explored our options here (if there are any other).

[kapilt]

1. There is no compatibility intended for deploy commands and package, so its unclear what value/intent rendering additional bits here serve, the engine attribute serves to denote which system set those values, and ideally should be used as guards. This was intended to preserve the minimal semantics needed to preserve cli compatibility, doing additional is a non goal feature because its non functional/useful. Per your initial comment, Trying to reuse logic from planner around recorded values, breaks encapsulation of features, and frankly is a large refactoring for minimal value chain, given this already establishes the feature (cli compatibility) for the least amount of code, the packager is already operating on deployer models which is the best degree of reuse out of this imo.

2. its very common in both cloudformation and terraform to have parameters/variables with default values that don't need to be set. terraform has a secondary notion of here dynamically sourced data values, i think i see another option here that removes the ambiguity of variables that cant be set by using static data values from a null data provider (https://www.terraform.io/docs/providers/null/data_source.html) . the ergonomics feel worse then using a variable imo, ${var.chalice_app_name} vs ${data.null_data_source.chalice.outputs.app} but it removes the ambiguity. i can make the change if that feels preferable.

[kyleknap]

Despite the longer name, I think I'd prefer the null data source provider here to remove the ambiguity.

[kapilt]

switched to null data provider, i'm still of the opinion that variables are better for this usage. the documentation on the null provider reinforces that perspective https://www.terraform.io/docs/providers/null/index.html

i think there's option here for future improvements to make app and stage actual variables by unwinding all the chalice app-stage prefixing in the deployer when packaging (aka regenerating resource names), but that can be done in a backwards compatible way to this null data provider.

also travis seems to be experiencing issues today, i've had numerous build failures do to network issues https://www.traviscistatus.com/incidents/hl4vqb7hvv5n

[kyleknap]

i think there's option here for future improvements to make app and stage actual variables by unwinding all the chalice app-stage prefixing in the deployer when packaging (aka regenerating resource names), but that can be done in a backwards compatible way to this null data provider.

Yep. I agree.

Looking over the diff. I think the only outstanding point was removing the deployed state file generation for now.

[kapilt]

Looking over the diff. I think the only outstanding point was removing the deployed state file generation for now.

hmm.. sorry thought i had gotten that removed, switching laptops when force pushing is leading to some oddities where it was reintroduced. just pushed the removal of that again.

[kyleknap]

Looks good to me! Thanks again for all of the work on this!

[jamesls]

Cool, I'll go ahead and squash/rebase and merge.