Add top-level ELB Attachment resource #6879

jbardin · 2016-05-25T21:03:24Z

Add an aws_elb_attachment resource so that the attment of instances to
an ELB can be managed separately from an aws_elb and prevent dependency
cycles.

fixes #3999

radeksimko · 2016-05-28T20:57:34Z

This LGTM 👍 with a few nitpicks:

Link to the new resource page in docs is missing in menu - see website/source/layouts/aws.erb
I'm thinking that not only aws_elb & aws_elb_attachment are mutually exclusive, but also multiple aws_elb_attachment resources for the same ELB would conflict with each other since aws_elb_attachment inevitably manages all attached instances - shouldn't that be also mentioned in the docs?

radeksimko · 2016-05-28T21:00:06Z

Also I reckon enhancement to this resource will be requested soon - support for ASG/ELB attachment, but I think that can be part of another PR.

jbardin · 2016-05-31T14:02:15Z

@radeksimko thanks for the feedback!

Yes, we were discussing making aws_elb_attachment resources additive rather than exclusive so that attachments could be defined in multiple places.

Do we have the same dependency cycle problems with ASG attachments? the ELB and and LC are both defined in the ASG, so I'm not sure if there's a clear benefit there.

radeksimko · 2016-06-03T09:00:05Z

Yes, we were discussing making aws_elb_attachment resources additive rather than exclusive so that attachments could be defined in multiple places.

I think that anyone wanting to do blue/green deployments in a way that each "colour" is managed in a separate Terraform context/directory may benefit from the additive approach.

I think it can be still a list of instances to attach - i.e. aws_elb_attachment can still manage more than 1 relationship (more than 1 instance) - so the schema can remain the same, but it would only remove attachments it has created.

It also makes sense in the light of Terraform being nice to existing infrastructure outside of Terraform - imagine a situation where user had existing ELB and wanted to use TF only for managing the relationship EC2/ELB.

If you still believe that the exclusive approach is better, then I think the resource may be renamed to aws_elb_attachments to make it clear that it's managing all attachments.

Do we have the same dependency cycle problems with ASG attachments?

What kind of dependency cycles do you mean? I'm not sure I follow.

jbardin · 2016-06-03T20:09:44Z

Ok, this just got a refactor and simplification. After discussing this more with @phinze, we're going to leave this as a single instance per attachment resource, rather than a list. The aligns with other resources we've broken out in this manner, and works better within in the limitations of the terraform internals. We can work on refactoring them all when we have a way to effectively manage this type of interdependent set of resources.

@radeksimko the resource cycle I was referring to was what's described in issue #3999, but that isn't the only reason to add other things as separate resources either.

radeksimko · 2016-06-03T21:10:36Z

builtin/providers/aws/resource_aws_elb_attachment.go

+		return fmt.Errorf("Failure registering instances with ELB: %s", err)
+	}
+
+	d.SetId(resource.PrefixedUniqueId(fmt.Sprintf("%s-", elbName)))


Is there any reason why not just make this elbName + instance ID?

An instance attached to the same ELB could change to a new aws_elb_attachment resource, creating a new resource with the same ID (I'm not sure if that's a problem in practice, but was being cautious).

I see what do you mean, but both fields are ForceNew: true anyway, so it's going to happen anyway.

My only concern about this is that it may make future resource importing slightly tricker. I haven't looked at the import functionality yet though, so maybe I'm wrong - in which case feel free to ignore this concern.

The import won't work anyway since the lookup is not based on ID anyway, so it doesn't matter - see my other note about Importer.

radeksimko · 2016-06-03T21:20:06Z

we're going to leave this as a single instance per attachment resource, rather than a list

ok, I didn't have a strong opinion about this as much as I had about exclusive/additive approach, so I think we're pretty much on the same page here. 👌

I left you some other comments/questions in the code.

radeksimko · 2016-06-05T12:28:40Z

builtin/providers/aws/resource_aws_elb_attachment.go

+		Read:   resourceAwsElbAttachmentRead,
+		Delete: resourceAwsElbAttachmentDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,


I don't think this will actually work. The passthrough function is only supposed to work for ID-only imports as far as I know (and was able to read from the code I skimmed through).

It would be probably better to omit Importer completely for now, until we figure out how to deal with import for such resources.

Yes, thanks for catching that, it was left from an earlier attempt.

I think importing would have to rely on the ELB importing them as a single list, which I think is probably fine. Either way we don't need Importer here, and can sort it out later if there are other options.

phinze · 2016-06-06T21:59:16Z

Looking great! A few final spacing / docs nits and this LGTM after those are addressed. 👍

Add an aws_elb_attachment resource so that the attment of instances to an ELB can be managed separately from an aws_elb and prevent dependency cycles.

jbardin · 2016-06-07T13:55:49Z

$ TF_ACC=1 go test -v -run ELBAttachment
=== RUN   TestAccAWSELBAttachment_basic
--- PASS: TestAccAWSELBAttachment_basic (131.55s)
=== RUN   TestAccAWSELBAttachment_drift
--- PASS: TestAccAWSELBAttachment_drift (95.12s)
PASS

phinze · 2016-06-07T14:18:06Z

LGTM

ghost · 2020-04-25T02:11:54Z

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.

jbardin force-pushed the jbardin/GH-3999 branch from 7565125 to 3a2ba50 Compare May 26, 2016 01:14

jbardin changed the title ~~WIP Add top-level ELB Attachment resource~~ Add top-level ELB Attachment resource May 26, 2016

jbardin force-pushed the jbardin/GH-3999 branch 2 times, most recently from 21de896 to 725f1c2 Compare May 26, 2016 13:34

radeksimko added provider/aws new-resource labels May 28, 2016

jbardin force-pushed the jbardin/GH-3999 branch from 725f1c2 to 6c3f81e Compare June 3, 2016 19:53

radeksimko reviewed Jun 3, 2016
View reviewed changes

radeksimko reviewed Jun 5, 2016
View reviewed changes

Add top-level ELB Attachment resource

e4d8c69

Add an aws_elb_attachment resource so that the attment of instances to an ELB can be managed separately from an aws_elb and prevent dependency cycles.

jbardin force-pushed the jbardin/GH-3999 branch from 5d38112 to e4d8c69 Compare June 7, 2016 13:55

jbardin merged commit 4c7a31d into master Jun 7, 2016

jbardin deleted the jbardin/GH-3999 branch June 7, 2016 14:19

jbardin mentioned this pull request Jun 7, 2016

add sidebar link for elb_attachment #7051

Merged

ghost locked and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add top-level ELB Attachment resource #6879

Add top-level ELB Attachment resource #6879

jbardin commented May 25, 2016

radeksimko commented May 28, 2016

radeksimko commented May 28, 2016

jbardin commented May 31, 2016 •

edited

Loading

radeksimko commented Jun 3, 2016

jbardin commented Jun 3, 2016

radeksimko Jun 3, 2016

jbardin Jun 4, 2016

radeksimko Jun 5, 2016

radeksimko Jun 5, 2016 •

edited

Loading

radeksimko commented Jun 3, 2016

radeksimko Jun 5, 2016

jbardin Jun 6, 2016

phinze commented Jun 6, 2016

jbardin commented Jun 7, 2016

phinze commented Jun 7, 2016

ghost commented Apr 25, 2020

Add top-level ELB Attachment resource #6879

Add top-level ELB Attachment resource #6879

Conversation

jbardin commented May 25, 2016

radeksimko commented May 28, 2016

radeksimko commented May 28, 2016

jbardin commented May 31, 2016 • edited Loading

radeksimko commented Jun 3, 2016

jbardin commented Jun 3, 2016

radeksimko Jun 3, 2016

Choose a reason for hiding this comment

jbardin Jun 4, 2016

Choose a reason for hiding this comment

radeksimko Jun 5, 2016

Choose a reason for hiding this comment

radeksimko Jun 5, 2016 • edited Loading

Choose a reason for hiding this comment

radeksimko commented Jun 3, 2016

radeksimko Jun 5, 2016

Choose a reason for hiding this comment

jbardin Jun 6, 2016

Choose a reason for hiding this comment

phinze commented Jun 6, 2016

jbardin commented Jun 7, 2016

phinze commented Jun 7, 2016

ghost commented Apr 25, 2020

jbardin commented May 31, 2016 •

edited

Loading

radeksimko Jun 5, 2016 •

edited

Loading