Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Elastic Beanstalk Application Version #5770

Merged

Conversation

dharrisio
Copy link
Contributor

This is a rebased version of #3871.

This adds support for Elastic Beanstalk Application Versions.

Example Terraform document

provider "aws" {
  region = "us-east-1"
}

resource "aws_s3_bucket" "default" {
  bucket = "tftest.applicationversion.bucket"
}

resource "aws_s3_bucket_object" "default" {
  bucket = "${aws_s3_bucket.default.id}"
  key = "beanstalk/go-v1.zip"
  source = "go-v1.zip"
}

resource "aws_elastic_beanstalk_application" "default" {
  name = "tf-test-name"
  description = "tf-test-desc"
}

resource "aws_elastic_beanstalk_environment" "default" {
  name = "tf-test-name"
  application = "${aws_elastic_beanstalk_application.default.name}"
  version_label = "${aws_elastic_beanstalk_application_version.default.name}"
  solution_stack_name = "64bit Amazon Linux 2015.09 v2.0.4 running Go 1.4"
}

resource "aws_elastic_beanstalk_application_version" "default" {
  application = "tf-test-name"
  name = "tf-test-version-label"
  bucket = "${aws_s3_bucket.default.id}"
  key = "${aws_s3_bucket_object.default.id}"
}

Tests

  TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSBeanstalk -timeout 120m
  === RUN   TestAccAWSBeanstalkApp_basic
  --- PASS: TestAccAWSBeanstalkApp_basic (7.83s)
  === RUN   TestAccAWSBeanstalkAppVersion_basic
  --- PASS: TestAccAWSBeanstalkAppVersion_basic (18.15s)
  === RUN   TestAccAWSBeanstalkConfigurationTemplate_basic
  --- PASS: TestAccAWSBeanstalkConfigurationTemplate_basic (11.68s)
  === RUN   TestAccAWSBeanstalkEnv_basic
  --- PASS: TestAccAWSBeanstalkEnv_basic (339.23s)
  === RUN   TestAccAWSBeanstalkEnv_tier
  --- PASS: TestAccAWSBeanstalkEnv_tier (493.23s)
  === RUN   TestAccAWSBeanstalkEnv_version_label
  --- PASS: TestAccAWSBeanstalkEnv_version_label (350.19s)
  PASS
  ok    github.com/hashicorp/terraform/builtin/providers/aws    1220.309s

The changes to resourceAwsElasticBeanstalkEnvironmentUpdate might not appear to be related to the Application Version resource. The reason I made those changes is that the current resourceAwsElasticBeanstalkEnvironmentUpdate function fails when trying to update the version_label and description in the same Terraform plan. UpdateEnvironment will return immediately, set the environment state to 'Updating' and move on the next update. This
second update will fail, because UpdateEnvironment can only be called when the environment state is 'Ready'. I Updated the resourceAwsElasticBeanstalkEnvironmentUpdate func to put all of the updates in one UpdateEnvironmentInput struct. Instead of making multiple update calls, this updates all the resource attributes in one call. The alternative is to wait for the Beanstalk Environment state to be 'Ready' after updating each attribute.

While working on this, I've been thinking about the tests for the Beanstalk resources. As you can see above the tests took about 20 minutes, I expect this to get even longer as I have a few other changes I'll be submitting soon (with more tests).
There are a few ways to speed them up.

  • Reduce the number of TestAcc functions and use more checks in the tests. Essentially, create environments that can test multiple attributes in the same environment.
  • Change the instance type in the test configs. Using larger instances should result in faster tests, because the environments won't take so long to create.
  • Run tests in parallel. At a quick glance I don't see any of the acceptance tests doing this in the aws package. There may be a few other reasons for not doing these in parallel, but at the very least the config documents would need to be more dynamic.

Any thoughts on that would be appreciated!

s3Location := elasticbeanstalk.S3Location{
S3Bucket: aws.String(bucket),
S3Key: aws.String(key),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If bucket is optional, what happens here if the User omits it from the config? I assume a failure in the CreateApplicationVersion call. We need to conditionally added this, if that is the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to make bucket and key required for the reasons discussed below on the documentation note.

@catsby
Copy link
Contributor

catsby commented Mar 22, 2016

This is a great start! I added some comments that need addressed before we merge.

Regarding your comments on tests: you're correct, we can do those things, however testing time is less of a focus for us now that we have our acceptance tests running nightly. We may cross that bridge in the future though, and I appreciate your thoughts/consideration there 😄

Thanks!

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Mar 22, 2016
@dharrisio
Copy link
Contributor Author

@catsby Thanks for the feedback! I should be able to get to all of this today.

@dharrisio
Copy link
Contributor Author

I may have come across a small issue for some common use cases. In our case we create a unique application version per release, so we aren't really sharing versions across multiple environments. In this case, we don't have any issues because after we create the new application version, the old one can get cleaned up by Terraform. Where this causes problems is a situation like this:

Version "A" is deployed to development and production - No issues here yet
Version "B" is deployed to development - Terraform will create version "b" and then delete version "a". But, "a" is still in use for the production environment. There aren't any immediate issues, but it could prevent the environment from scaling or being rebuilt. (I haven't looked into what happens in those cases yet).

A couple ideas on dealing with this issue:

  • Terraform doesn't delete the old application version resources. This leaves cleanup to the user. There are limits to the number of application versions an account can have, so it can't be ignored completely.
  • Some combination of retention period options are added to the resource. For example, keep all application versions less than 7 days old, and at least 3. This would be more complicated, but it would allow rolling back to older application versions without having to recreate them. This also sounds like it would be tricky to add tests to make sure this is working as expected.
  • Don't delete an application version if it is currently deployed to an environment.

@kevtainer
Copy link

kevtainer commented Jun 14, 2016

I've used Elastic Beanstalk extensively in our organization, personally authored a deployment framework for managing the packaging and deprecation of application versions. The challenge in our organization is that we have multiple environments per stack - so simply deleting the last n versions would not work (we'd end up deleting the last known good version for other environment stacks in that application).

The solution we came up with was programmatic, we had to derive the environment name from the application version name and generate a hash map for those environments to determine which application versions to delete. One might argue that this type of work is better suited for something other than terraform.

I would submit that managing the deprecation and removal of stale application versions is outside of the scope of the aws_elastic_beanstalk_application_version resource during a create. There is certainly room for a "delete" action, but imo that action should be explicit - requiring the user to define what exactly is being deleted. I'd prefer Terraform to not infer anything.

Also, Beanstalk won't let you delete a version which is actively deployed to a stack.

@bobbydeveaux
Copy link
Contributor

Looking forward to this. Agree with @KCrawley definitely need to ensure application versions are not removed each time. A similar comment could be made for the API Gateway deployments too.

@dharrisio
Copy link
Contributor Author

@KCrawley and @bobbydeveaux thanks for the feedback. I agree that cleaning up the application versions is beyond the current scope of Terraform. I tried a few different ways and was never really happy with the results. The issue comes down to differences between Terraform's and Elastic Beanstalk's "view" of the world. A Terraform state file is going to correlate to one environment(generally), while a Beanstalk Application can include multiple environments, and resources shared across those environments. So, the real problem here is that Terraform is trying to manage resources that aren't in its dependency graph, which gets really messy.

Until there is a better way to manage those types of resources we could make the delete function essentially a noop, and notify the user that old application versions are not cleaned up by Terraform. @catsby how does that sound to you?

@lestephane
Copy link

@dharrisio If the application version is not present yet, it gets created, and that is something simple enough to reason about.

Deleting versions, there be dragons. This can be better solved with a remove-unused-elastic-beanstalk-versions task in a delivery pipeline, which can be tweaked and customized to taste. Ie. not Terraform's responsibility, me thinks.

@karim-h
Copy link

karim-h commented Jun 29, 2016

Hi, when will we have this in the master?

@lestephane
Copy link

I provided an updated rebase PR for this: #7407 to speed up the inclusion of this into master, I also need it urgently.

@dharrisio
Copy link
Contributor Author

The current issue here is not really about rebasing. From the issues discussed above, I'm not sure that this should really be in master any time soon. Because there really isn't a good way to manage application versions I'm not sure that this resource is a good fit for Terraform.

From what has been discussed so far, there are two options for including this as a resource:

  1. Leave as is: Application versions are created/deleted just like any other resource. The problem is that this imposes a limitation of using a unique application version per environment. This is an assumption most people will not make, and I think will lead to poor user experience with this resource.

  2. Noop: Update the delete method to not delete application versions. This puts the burden of cleaning up old application versions to the user, goes against the CRUD model that all Terraform resources use and also causes problems with state management. What happens when a new version is created? The version resources would still need to be removed from state, but the resource doesn't actually get deleted. At the point, does storing the version resource in Terraform state even make sense?

I think uploading new application versions will be best handled outside of Terraform. For now, I think adding the application_version attribute to the environment resource is the most we can really do. I'll put together a better example of this shortly.

@cwarden
Copy link
Contributor

cwarden commented Jul 8, 2016

I'm new to both terraform and elastic beanstalk, so please bear with me. Why does adding an application_version attribute to the environment not solve the problem of figuring out which application versions can be deleted?

If environment A is using version 1 and environment B is using version 2, terraform shouldn't delete either version. When environment A is updated to use version 2, terraform can delete version 1.

What am I missing?

@dharrisio
Copy link
Contributor Author

@cwarden When Terraform updates environment B to use version2, it will delete version1, create version2 (it can do this the other way around too with create_before_destroy) and then attach version2 to environment B. The problem is that version1 is deleted, but environment A is still dependent on version1.

So we would never really be able to get to a state you described, which as you mentioned doesn't really present the problem. Hope this helps clear that up for you.

@kevtainer
Copy link

@dharrisio beanstalk won't let you delete versions which are deployed to an environment.

the main issue which is addressed by this PR (for myself) is when creating a new beanstalk application and environment it is required to have an application version which is compatible with the stack (and will pass health checks if they're turned on).

even if the ability to create application versions is removed it should be, at a minimum, possible to attach a zip or s3 reference to the newly created resource.

@cwarden
Copy link
Contributor

cwarden commented Jul 8, 2016

My mental model must be wrong. I'm assuming terraform works like this:

  1. dependency graph is generated
  2. resources without dependencies are deleted
  3. new resources are created or updated (or swap 2 and 3)

Why, when terraform updates environment B to version 2, the dependency upon version 1 by environment A doesn't prevent version 1 from being deleted?

Using alternative resources as an example, if two aws_instance resources use the same aws_key_pair, and one of the aws_instance resources is updated to use a different, aws_key_pair, the first aws_key_pair is not deleted, right?

@mrooney
Copy link

mrooney commented Jul 11, 2016

if two aws_instance resources use the same aws_key_pair, and one of the aws_instance resources is updated to use a different, aws_key_pair, the first aws_key_pair is not deleted, right?

That seems like a great analogy if that's the case!

FWIW, a work-around we're using is an idempotent script to upload and deploy the application, and use a "local-exec" provisioner to run it. Here's an example tf and deploy script for Jenkins: https://gist.github.com/mrooney/79d80e262c6c9d484c5e1e2130f8b917

@dharrisio
Copy link
Contributor Author

Using alternative resources as an example, if two aws_instance resources use the same aws_key_pair, and one of the aws_instance resources is updated to use a different, aws_key_pair, the first aws_key_pair is not deleted, right?

If the modifed aws_key_pair resource is defined in the same document/state as the instance, then yes, Terraform will try to delete the old aws_key_pair when you update the public_key value.

FWIW, a work-around we're using is an idempotent script to upload and deploy the application, and use a "local-exec" provisioner to run it. Here's an example tf and deploy script for Jenkins: https://gist.github.com/mrooney/79d80e262c6c9d484c5e1e2130f8b917

This is along the lines of what I was thinking.

@dharrisio dharrisio changed the title [WIP] provider/aws: Elastic Beanstalk Application Version provider/aws: Elastic Beanstalk Application Version Aug 16, 2016
@dharrisio
Copy link
Contributor Author

@catsby Any chance we could get this looked at soon?

@dharrisio
Copy link
Contributor Author

@catsby / @stack72 Commenting on this again, feedback would be appreciated.

@joeharrison714
Copy link

Can this be merged soon please?

@ncrothe
Copy link

ncrothe commented Oct 18, 2016

Having only just dived into terraform and beanstalk and hit the block of initial app version, I would recommend keeping all versions the user wants to keep as explicit resources, i.e. have aws_elastic_beanstalk_application_version.version_a and aws_elastic_beanstalk_application_version.version_b if both are used. And have the environments refer to the ones you want to use. When you need a new version, create a new app_version resource. If you don't need a version anymore, remove it from the .tf file.
This is clearly not the most elegant way, but to me kind of bridges between the way AWS sees it and terraform sees it. Best practice could then see the versions handled in a central terraform project that is then referred to from individual environments.

And do I understand correctly that AWS does not stop you from deleting a app version still in use? That's a bummer, can we add a warning in the planning stage?

@mitchellh mitchellh removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 1, 2016
@marioharper
Copy link

Any update on the progress of this getting to master?

@dharrisio dharrisio force-pushed the f-aws-elastic-beanstalk-application-version branch from 3d71f5b to c2b9c0b Compare February 16, 2017 21:02
@dharrisio
Copy link
Contributor Author

@stack72 I added a check to return an error when an Application Version is in use by more than one environment, as well as an attribute to force delete the resource if necessary. Here is my output from the tests, the one that failed is due to a missing solution stack in my account.

go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/16 10:43:54 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSBeanstalk -timeout 120m
=== RUN   TestAccAWSBeanstalkApp_basic
--- PASS: TestAccAWSBeanstalkApp_basic (26.66s)
=== RUN   TestAccAWSBeanstalkAppVersion_basic
--- PASS: TestAccAWSBeanstalkAppVersion_basic (46.25s)
=== RUN   TestAccAWSBeanstalkConfigurationTemplate_basic
--- PASS: TestAccAWSBeanstalkConfigurationTemplate_basic (34.55s)
=== RUN   TestAccAWSBeanstalkConfigurationTemplate_VPC
--- PASS: TestAccAWSBeanstalkConfigurationTemplate_VPC (54.93s)
=== RUN   TestAccAWSBeanstalkConfigurationTemplate_Setting
--- PASS: TestAccAWSBeanstalkConfigurationTemplate_Setting (47.75s)
=== RUN   TestAccAWSBeanstalkEnv_basic
--- PASS: TestAccAWSBeanstalkEnv_basic (494.79s)
=== RUN   TestAccAWSBeanstalkEnv_tier
--- PASS: TestAccAWSBeanstalkEnv_tier (560.12s)
=== RUN   TestAccAWSBeanstalkEnv_outputs
--- PASS: TestAccAWSBeanstalkEnv_outputs (566.15s)
=== RUN   TestAccAWSBeanstalkEnv_cname_prefix
--- PASS: TestAccAWSBeanstalkEnv_cname_prefix (493.07s)
=== RUN   TestAccAWSBeanstalkEnv_config
--- PASS: TestAccAWSBeanstalkEnv_config (589.50s)
=== RUN   TestAccAWSBeanstalkEnv_resource
--- PASS: TestAccAWSBeanstalkEnv_resource (497.86s)
=== RUN   TestAccAWSBeanstalkEnv_vpc
--- PASS: TestAccAWSBeanstalkEnv_vpc (528.26s)
=== RUN   TestAccAWSBeanstalkEnv_template_change
--- FAIL: TestAccAWSBeanstalkEnv_template_change (552.55s)
	testing.go:265: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_elastic_beanstalk_configuration_template.template: 1 error(s) occurred:

		* aws_elastic_beanstalk_configuration_template.template: Error creating Elastic Beanstalk configuration template: InvalidParameterValue: No Solution Stack named '64bit Amazon Linux 2016.03 v2.1.3 running Go 1.5' found.
			status code: 400, request id: c72d829c-f478-11e6-b798-3bd5a504bd61
=== RUN   TestAccAWSBeanstalkEnv_basic_settings_update
--- PASS: TestAccAWSBeanstalkEnv_basic_settings_update (802.38s)
=== RUN   TestAccAWSBeanstalkEnv_version_label
--- PASS: TestAccAWSBeanstalkEnv_version_label (651.32s)
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/aws	5946.208s

@stack72
Copy link
Contributor

stack72 commented Feb 17, 2017

this LGTM! Thanks for all the work on this @dharrisio :)

Sorry it took so long to get merged

Paul

@stack72 stack72 merged commit 2ab6fcc into hashicorp:master Feb 17, 2017
stack72 pushed a commit that referenced this pull request Feb 17, 2017
* Added new resource aws_elastic_beanstalk_application_version.

* Changing bucket and key to required.

* Update to use d.Id() directly in DescribeApplicationVersions.

* Checking err to make sure that the application version is successfully deleted.

* Update `version_label` to `Computed: true`.

* provider/aws: Updating to python solution stack

* provider/aws: Beanstalk App Version delete source

The Elastic Beanstalk API call to delete `application_version` resource
should not delete the s3 bundle, as this object is managed by another
Terraform resource

* provider/aws: Update application version docs

* Fix application version test

* Add `version_label` update test

Adds test that fails after rebasing branch onto v0.8.x. `version_label`
changes do not update the `aws_elastic_beanstalk_environment` resource.

* `version_label` changes to update environment

* Prevent unintended delete of `application_version`

Prevents an `application_version` used by multiple environments from
being deleted.

* Add `force_delete` attribute

* Update documentation
@ghost
Copy link

ghost commented Apr 16, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.