provider/aws: aws_autoscaling_attachment resource

[jrnt30]

Fixes #8755

• Introduces a new resource for attaching an ASG to an ELB.

Context:
Adds support for associating an ASG with an ELB without having to declare this when the ASG is created. The primary use case we wanted this for is to allow dynamically allocating ASGs to an ELB for Blue/Green deployments. With the current process, there is not a clean way for us to do this without some hacking of variables.

NOTE: I really struggled with the decision of keeping this as a unique resource or trying to branch the logic in the aws_elb_attachment resource. In the end, I opted for the later because it seemed cleaner, as the internals of the two resources are fairly different and it made the implementation and testing cleaner. That being said, if you would prefer it merged with the other resource, I can take a stab at that.

@kwilczynski This is what we were discussing in slack the other day. Would love your 2 cents

[kwilczynski]

@jrnt30 hi there! Thank you for this! Looks really good.

Few little comments.

Then, we would need @stack72 to look it through for us.

[kwilczynski]

I would call the attribute autoscaling_group_name to be consistent with other resources in the same group.

[kwilczynski]

Extra new line, I think.

[kwilczynski]

We are endorsing usage of errwrap.Wrapf as much as possible, thus this could be replaced with e.g.

return errwrap.Wrapf(fmt.Sprintf(
    "Failure attaching Elastic Load Balancer %q to Auto Scaling Group %q: {{err}}",
    elbName, asgName), err)

You can keep it simple too (like the original message).

[kwilczynski]

Often, as a customary thing, we would invoke the ReadFunc so for example it would be:

return resourceAwsAutoscalingAttachmentRead(d, meta)

This is so that it would re-read the resource back ensuring that AWS already created it, etc. Primarily as a precaution (or guard, if you wish) against the eventually consistent nature of the AWS.

[kwilczynski]

I would break here, so that you don't check rest of them, as there will be no need at this point, I believe.

[kwilczynski]

Extra new line, I think.

[kwilczynski]

See comment above about the errwrap.Wrapf.

[kwilczynski]

You can inline this to:

if _, err := asgconn.DetachLoadBalancers(detachOpts); err != nil {

Since the err variable is not used below this line.

[kwilczynski]

The good thing is, there isn't much in terms of needing to pool for state changes, which makes a lot of this much easier.

[kwilczynski]

@jrnt30 as per the conversation on Slack, we might need to update the name of the resource to be something like e.g. aws_autoscaling_elb_attachment, or something similar that would allow people to get an idea what it does at a first glance.

[jrnt30]

I held off on the renaming of the resource due to the conversation and potential expansion to also support an AutoScale group's attachment to the ALB Target Group resource. Before I make those changes, I need to read up on that resource more so I am going to leave the name and resource attributes alone for right now.

If it's decided that we would prefer to use two distinct resources for the attachment (which as a user I'm not sure I would expect) I can make the name more explicit.

[simonvanderveldt]

I do think aws_autoscaling_attachment is a bit confusing, especially in relation to aws_elb_attachment.
Would it be problematic to use aws_autoscaling_elb_attachment?

[jrnt30]

@simonvanderveldt I was a bit on the fence about this and am open to change. There are several resources that also provide the ability to attach to an Application Load Balancer. The idea was, since the resource's "api" footprint is essentially the same we may want to have those be the resource.

Looking back at the other resources which support both of these (the autoscaling resource itself as an example), one common thread I see between them is that they seem to all call the same AWS API call. That might be a good enough litmus test for me to say "ALB and ELB resource associations should be two distinct resources, since they require different underlying AWS API calls for the attachment and detachment.

Honestly I don't feel very strongly one way or another, but would appreciate if there were some guidelines. In general I think the question is, is it more explicit/maintainable/easier to approach if we break out the resources (alb vs. elb attachments being different in this case) or to keep them as a single resource?

[simonvanderveldt]

@jrnt30 afaik Terraform's basic way of working is to mimic the API's it wraps as closely as possible. I guess that should be documented somewhere, but can't really find it.
I do know one example offhand, which is #3851 (comment)

[stack72]

So I have been looking through this. I believe this is the resource name we want. This will allow us the flexibility to use ELB and ALB target groups (going forward!)

Tests are looking good:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAwsAutoscaling'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/03 12:29:03 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAwsAutoscaling -timeout 120m
=== RUN   TestAccAwsAutoscalingAttachment_basic
--- PASS: TestAccAwsAutoscalingAttachment_basic (111.94s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    111.954s

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAutoScalingGroup_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/03 12:31:51 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAutoScalingGroup_ -timeout 120m
=== RUN   TestAccAWSAutoScalingGroup_importBasic
--- PASS: TestAccAWSAutoScalingGroup_importBasic (174.43s)
=== RUN   TestAccAWSAutoScalingGroup_basic
--- PASS: TestAccAWSAutoScalingGroup_basic (301.94s)
=== RUN   TestAccAWSAutoScalingGroup_autoGeneratedName
--- PASS: TestAccAWSAutoScalingGroup_autoGeneratedName (56.33s)
=== RUN   TestAccAWSAutoScalingGroup_terminationPolicies
--- PASS: TestAccAWSAutoScalingGroup_terminationPolicies (83.79s)
=== RUN   TestAccAWSAutoScalingGroup_tags
--- PASS: TestAccAWSAutoScalingGroup_tags (268.13s)
=== RUN   TestAccAWSAutoScalingGroup_VpcUpdates
--- PASS: TestAccAWSAutoScalingGroup_VpcUpdates (106.32s)
=== RUN   TestAccAWSAutoScalingGroup_WithLoadBalancer
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer (381.09s)
=== RUN   TestAccAWSAutoScalingGroup_withPlacementGroup
--- PASS: TestAccAWSAutoScalingGroup_withPlacementGroup (154.24s)
=== RUN   TestAccAWSAutoScalingGroup_enablingMetrics
--- PASS: TestAccAWSAutoScalingGroup_enablingMetrics (169.05s)
=== RUN   TestAccAWSAutoScalingGroup_withMetrics
--- PASS: TestAccAWSAutoScalingGroup_withMetrics (46.26s)
=== RUN   TestAccAWSAutoScalingGroup_ALB_TargetGroups
--- PASS: TestAccAWSAutoScalingGroup_ALB_TargetGroups (181.88s)
=== RUN   TestAccAWSAutoScalingGroup_initialLifecycleHook
--- PASS: TestAccAWSAutoScalingGroup_initialLifecycleHook (252.94s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    2176.410s

Thanks for all the work here @jrnt30 :)

P.

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