New Resource: aws_dx_connection_association #2360

atsushi-ishibashi · 2017-11-18T14:15:53Z

make testacc TEST=./aws TESTARGS='-run=TestAccAwsDxConnectionAssociation_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsDxConnectionAssociation_basic -timeout 120m
=== RUN   TestAccAwsDxConnectionAssociation_basic
--- PASS: TestAccAwsDxConnectionAssociation_basic (100.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	100.725s

radeksimko

@atsushi-ishibashi Thanks for another PR!

This looks overall good, I just left you some questions. Please keep in mind those are questions - I am not implying that it should be one way or another, I'm opening a conversation. 😉

radeksimko · 2017-12-02T02:24:19Z

aws/resource_aws_dx_connection_association.go

+		d.SetId("")
+		return nil
+	}
+	if *resp.Connections[0].LagId != d.Get("lag_id").(string) {


As far as I understand there's 1-to-1 relationship between a connection and a LAG which this resource should represent, so shouldn't we instead treat this as if the resource (i.e. unique pair) is gone?

radeksimko · 2017-12-02T02:25:49Z

aws/resource_aws_dx_connection_association.go

+		return nil
+	}
+	if len(resp.Connections) != 1 {
+		return fmt.Errorf("Number of DX Connection (%s) isn't one, got %d", connectionId, len(resp.Connections))


Just grammar nitpick - do you mind changing this to Found %d DX connections for %s, expected 1?

radeksimko · 2017-12-02T02:28:15Z

aws/resource_aws_dx_connection_association.go

+	if err != nil {
+		return err
+	}
+	if resp.LagId != nil {


Have you ever experienced this in reality? Is it just a matter of time until it becomes actually disassociated or is it an erroneous/unrecoverable state? If it's the former then I'd suggest we just put a retry logic in place and wait until it's disassociated. If it's the latter then I guess this is a correct behaviour - I'm just trying to ensure that we know what we're doing here.

radeksimko · 2017-12-02T02:30:38Z

website/docs/r/dx_connection_association.html.markdown

+page_title: "AWS: aws_dx_connection_association"
+sidebar_current: "docs-aws-resource-dx-connection-association"
+description: |-
+  Associates a Connection with LAG.


Nitpick, but do you mind changing this to Associates a Direct Connect Connection with a LAG.?

radeksimko · 2017-12-02T02:31:17Z

website/docs/r/dx_connection_association.html.markdown

+
+# aws_dx_connection_association
+
+Associates a Connection with LAG.


Nitpick, but do you mind changing this to Associates a Direct Connect Connection with a LAG.?

atsushi-ishibashi · 2017-12-02T14:30:11Z

I removed some extra error handling.

radeksimko · 2017-12-04T10:08:23Z

aws/resource_aws_dx_connection_association.go

+	if len(resp.Connections) < 1 {
+		d.SetId("")
+		return nil
+	}


I think there was a bit of misunderstanding about what I meant by my question about LAG ID mismatch - I wasn't implying it should be removed. I meant we should treat it the same way as if there is no connection - i.e. d.SetId(""); return nil.

Oh, I got it👍

radeksimko · 2017-12-04T10:09:56Z

aws/resource_aws_dx_connection_association_test.go

+		}
+		for _, v := range resp.Connections {
+			if *v.ConnectionId == rs.Primary.ID && v.LagId != nil {
+				return fmt.Errorf("Dx Connection (%s) is not diasociated with Lag", rs.Primary.ID)


nitpick: is not dissociated from Lag

radeksimko · 2017-12-04T10:34:46Z

So I took a deeper look into how Direct Connect works with LAGs & connections and I think we'll need to add a for loop to read through all connections for the given connection ID as we cannot assume that there's only 1 connection per lag.

Basically we need to make the following config work:

resource "aws_dx_connection" "example" {
  name = "example"
  bandwidth = "1Gbps"
  location = "EqSe2"
}

resource "aws_dx_lag" "example" {
  name = "example"
  connections_bandwidth = "1Gbps"
  location = "EqSe2"
  number_of_connections = 1
}

resource "aws_dx_lag" "example2" {
  name = "example2"
  connections_bandwidth = "1Gbps"
  location = "EqSe2"
  number_of_connections = 1
}

resource "aws_dx_connection_association" "example" {
  connection_id = "${aws_dx_connection.example.id}"
  lag_id = "${aws_dx_lag.example.id}"
}

resource "aws_dx_connection_association" "example2" {
  connection_id = "${aws_dx_connection.example.id}"
  lag_id = "${aws_dx_lag.example2.id}"
}

can you also add it as an acceptance test and make it pass, please?

atsushi-ishibashi · 2017-12-04T11:12:17Z

Could I understand what you meant? @radeksimko

TF_ACC=1 go test ./aws -v -run=TestAccAwsDxConnectionAssociation_multiConns -timeout 120m
=== RUN   TestAccAwsDxConnectionAssociation_multiConns
--- PASS: TestAccAwsDxConnectionAssociation_multiConns (41.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.386s

radeksimko · 2017-12-04T15:12:28Z

Having a test for multiple connections is certainly helpful (thanks 👍 ), but I was talking about multiple LAGs. I just committed a failing test to you branch which hopefully explains what I mean.

You should see the following failure:

make testacc TEST=./aws TESTARGS='-run=TestAccAwsDxConnectionAssociation_multiLags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsDxConnectionAssociation_multiLags -timeout 120m
=== RUN   TestAccAwsDxConnectionAssociation_multiLags
--- FAIL: TestAccAwsDxConnectionAssociation_multiLags (44.62s)
	testing.go:503: Step 0 error: After applying this step and refreshing, the plan was not empty:

		DIFF:

		CREATE: aws_dx_connection_association.test2
		  connection_id: "" => "dxcon-ffo66i6d" (forces new resource)
		  lag_id:        "" => "dxlag-fhdhkf6q" (forces new resource)
...

Let me know if you need any further help in reproducing it.

atsushi-ishibashi · 2017-12-04T15:32:03Z

@radeksimko
At first, I added acctest which you provided above but I had the same error. On web console, the connection which is associated with a LAG doesn't show available connection list of another LAG.
LAG can have many connections but a connection can belong to only one LAG

radeksimko · 2017-12-04T16:04:21Z

@atsushi-ishibashi I see, thanks for the explanation.

It looks like the AWS API is behaving unexpectedly in the sense that it just accepts any association and overrides the existing one. 😞

I assume there isn't much we can do in this case - other than ask AWS to change/fix the API.

I'll merge it as is then - after removing my test.

* New Resource: aws_dx_connection_association * Reflect reviews * fix * fix * Reflect 2nd reviews * Add acctest

ghost · 2020-04-10T17:18:40Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

New Resource: aws_dx_connection_association

159e21f

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 20, 2017

radeksimko added new-resource Introduces a new resource. and removed enhancement Requests to existing resources that expand the functionality or scope. labels Dec 2, 2017

radeksimko reviewed Dec 2, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 2, 2017

Reflect reviews

87dd343

atsushi-ishibashi added 2 commits December 3, 2017 12:13

fix

e8fad39

fix

52fcd8b

radeksimko reviewed Dec 4, 2017

View reviewed changes

Reflect 2nd reviews

67ea88a

Add acctest

53df933

radeksimko force-pushed the aws_dx_connection_association branch from c0fc1fe to 53df933 Compare December 4, 2017 16:04

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 4, 2017

radeksimko approved these changes Dec 4, 2017

View reviewed changes

radeksimko merged commit dcb44db into hashicorp:master Dec 4, 2017

mdlavin pushed a commit to mdlavin/terraform-provider-aws that referenced this pull request Dec 9, 2017

New Resource: aws_dx_connection_association (hashicorp#2360)

72a776a

* New Resource: aws_dx_connection_association * Reflect reviews * fix * fix * Reflect 2nd reviews * Add acctest

atsushi-ishibashi deleted the aws_dx_connection_association branch December 13, 2017 15:16

ghost locked and limited conversation to collaborators Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_dx_connection_association #2360

New Resource: aws_dx_connection_association #2360

atsushi-ishibashi commented Nov 18, 2017

radeksimko left a comment

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi commented Dec 2, 2017

radeksimko Dec 4, 2017

atsushi-ishibashi Dec 4, 2017

radeksimko Dec 4, 2017

radeksimko commented Dec 4, 2017

atsushi-ishibashi commented Dec 4, 2017

radeksimko commented Dec 4, 2017

atsushi-ishibashi commented Dec 4, 2017 •

edited

Loading

radeksimko commented Dec 4, 2017

ghost commented Apr 10, 2020


		# aws_dx_connection_association

		Associates a Connection with LAG.

New Resource: aws_dx_connection_association #2360

New Resource: aws_dx_connection_association #2360

Conversation

atsushi-ishibashi commented Nov 18, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Dec 4, 2017

atsushi-ishibashi commented Dec 4, 2017

radeksimko commented Dec 4, 2017

atsushi-ishibashi commented Dec 4, 2017 • edited Loading

radeksimko commented Dec 4, 2017

ghost commented Apr 10, 2020

atsushi-ishibashi commented Dec 4, 2017 •

edited

Loading