provider/aws: elastic beanstalk invalid setting crash

[dharrisio]

This fixes the crash in #6624/#6707/#7197. It also adds functionality to relay any failures from the elastic beanstalk environment events. As an example, the config

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

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

resource "aws_elastic_beanstalk_environment" "tftest" {
  name = "tf-test-name"
  application = "${aws_elastic_beanstalk_application.tftest.name}"
  solution_stack_name = "64bit Amazon Linux 2016.03 v2.1.0 running Go 1.4"

setting {
    namespace = "aws:elb:loadbalancer"
    name = "SecurityGroups"
    value = "{\"Fn::GetAtt\":[\"AWSEBLoadBalancerSecurityGroup\",\"GroupId\"]},{\"Ref\":\"AWSEBLoadBalancerSecurityGroup\"}"
  }
}

Results in the following output

aws_elastic_beanstalk_application.tftest: Creating...
  description: "" => "tf-test-desc"
  name:        "" => "tf-test-name"
aws_elastic_beanstalk_application.tftest: Creation complete
aws_elastic_beanstalk_environment.tftest: Creating...
  all_settings.#:               "" => "<computed>"
  application:                  "" => "tf-test-name"
  autoscaling_groups.#:         "" => "<computed>"
  cname:                        "" => "<computed>"
  cname_prefix:                 "" => "<computed>"
  instances.#:                  "" => "<computed>"
  launch_configurations.#:      "" => "<computed>"
  load_balancers.#:             "" => "<computed>"
  name:                         "" => "tf-test-name"
  queues.#:                     "" => "<computed>"
  setting.#:                    "" => "1"
  setting.2468988437.name:      "" => "SecurityGroups"
  setting.2468988437.namespace: "" => "aws:elb:loadbalancer"
  setting.2468988437.value:     "" => "{\"Fn::GetAtt\":[\"AWSEBLoadBalancerSecurityGroup\",\"GroupId\"]},{\"Ref\":\"AWSEBLoadBalancerSecurityGroup\"}"
  solution_stack_name:          "" => "64bit Amazon Linux 2016.03 v2.1.0 running Go 1.4"
  tier:                         "" => "WebServer"
  triggers.#:                   "" => "<computed>"
  wait_for_ready_timeout:       "" => "10m"
aws_elastic_beanstalk_environment.tftest: Still creating... (10s elapsed)
aws_elastic_beanstalk_environment.tftest: Still creating... (20s elapsed)
aws_elastic_beanstalk_environment.tftest: Still creating... (30s elapsed)
Error applying plan:

1 error(s) occurred:

* aws_elastic_beanstalk_environment.tftest: 
2016-06-18 00:45:09.529 +0000 UTC: Stack named 'awseb-e-5s3a3gisjc-stack' aborted operation. Current state: 'CREATE_FAILED'  Reason: The following resource(s) failed to create: [AWSEBLoadBalancer]. 
2016-06-18 00:44:57.692 +0000 UTC: Creating load balancer failed Reason: Security group(s) can be applied to only an ELB in VPC.

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

Most of the tests on this pass, with one exception. The worker tier test fails, I was only able to reproduce the issue in us-west-2, so it might be temporary.

[catsby]

Thanks! I appreciate the work you did digging in and fixing these!

[jbardin]

A better name might be describeBeanstalkErrors since this only checks for ERRORs.

I'd also prefer a []string of the errors, (or even a []*elasticbeanstalk.EventDescription rather than concatenating them into an error value. A function returning an error generally indicated the call failed.

[dharrisio]

@jbardin Thanks for the feedback here, sorry it took a bit of time for me to get around to this.

1. I agree with changing the name to describeBeanstalkErrors. Originally, I was contemplating having a parameter to set the level of events returned, but I think error is all we really need.
2. I tried two approaches here, but ran into issues with both. Can you elaborate on how to do this? I tried both returning a slice of errors and a slice of strings. I pushed the slice of errors example to branch on my fork: dharrisio@950de0f
   What I ran into were type errors about returning []error as type error. There was a similar issue with using a slice of strings: []string as type string in argument to fmt.Errorf.

[jbardin]

oops, just missed the merge ;) My comments were only stylistic, so nothing that can't be cleaned up later if need be.

[jbardin]

Hi @dharrisio, sorry for the delayed reply.

This is mostly a style comment, and really only has an impact if someone else tries to reuse this function elsewhere in the code, so don't worry about it too much.

re: #2, This makes more sense once you change the name to describeBeanstalkErrors. The only error that indicates the call failed is the error from DescribeEvents, which is only logged. Since the purpose of the function is to "describe errors", those errors would be the expected return value, and any problem encountered while describing those would be returned as a separate error (if at all).

As for the return type, (IMHO) I think it shouldn't return an error type if the call succeeds (though error may be OK if the function were called say checkBeanstalkEvents, this is all about style and self-documenting APIs after all).

Though if you do ever need to bundle multiple errors like this, all you need is to define an Error() method on a container for those errors. Internally we use these for added functionality:

• github.com/hashicorp/go-multierror
• github.com/hashicorp/errwrap

[dharrisio]

@jbardin Thanks for the explanation on the errors, that helps a lot!

My reasoning for having this return an error is that the Beanstalk API doesn't return an error in some cases. When this happens we don't know if the resource was not created or updated until we get the errors from describe events. When this happens we don't want Terraform to continue creating/updating other resources in the graph. From my understanding, in that case, we would want this to be returned as an error?

As an example, expanded on from #6624

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

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

resource "aws_elastic_beanstalk_environment" "tftest" {
  name = "tf-test-name"
  application = "${aws_elastic_beanstalk_application.tftest.name}"
  solution_stack_name = "64bit Amazon Linux 2016.03 v2.1.0 running Go 1.4"

  setting {
    namespace = "aws:elb:loadbalancer"
    name = "SecurityGroups"
    value = "{\"Fn::GetAtt\":[\"AWSEBLoadBalancerSecurityGroup\",\"GroupId\"]},{\"Ref\":\"AWSEBLoadBalancerSecurityGroup\"}"
  }
}

resource "aws_route53_record" "default" {
  zone_id = "${var.zone_id}"
  name    = "${aws_elastic_beanstalk_environment.tftest.name}"
  type    = "${var.record_type}"
  ttl     = "${var.record_ttl}"
  records = ["${aws_elastic_beanstalk_environment.tftest.cname}"]
}

In this case, the environment creation will fail, and we would not want to create/update the r53 record to point to the bad environment.

To further complicate and confuse things, we probably do not want errors when deleting the resource because it is possible to get some false positives. In this case I had to run terraform destroy twice, because of errors trying to delete an autoscaling group that doesn't exist.

Example output using multierror: https://gist.github.com/dharrisio/8024591dfaabb0bfdd73ee39d9ff17e8

[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.