New Resource: aws_api_gateway_vpc_link #2512

atsushi-ishibashi · 2017-12-01T15:33:02Z

test
docs

atsushi-ishibashi · 2017-12-02T05:32:15Z

BadRequestException: Target arns are not allowed to be changed/added.
    status code: 400, request id: c45e681f-d6b7-11e7-b794-51b5b296ccda

BadRequestException: More than one target arn specified for vpc link.

So I set target_arn as ForceNew and TypeString.

atsushi-ishibashi · 2017-12-02T05:32:49Z

😕

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAPIGatewayVpcLink_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAPIGatewayVpcLink_basic -timeout 120m
=== RUN   TestAccAwsAPIGatewayVpcLink_basic
--- FAIL: TestAccAwsAPIGatewayVpcLink_basic (515.16s)
	testing.go:563: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error applying: 1 error(s) occurred:
		
		* aws_lb.test_a (destroy): 1 error(s) occurred:
		
		* aws_lb.test_a: Error deleting ALB: OperationNotPermitted: Load balancer 'arn:aws:elasticloadbalancing:us-west-2:1111111111111:loadbalancer/net/tf-lb-a-sa2n7/25a8919dc2841c43' cannot be deleted because deletion protection is enabled
			status code: 400, request id: 7fe9a326-d721-11e7-a8d8-3922343a3b38
		
		State: aws_lb.test_a:
		  ID = arn:aws:elasticloadbalancing:us-west-2:11111111111:loadbalancer/net/tf-lb-a-sa2n7/25a8919dc2841c43
		  access_logs.# = 1
		  arn = arn:aws:elasticloadbalancing:us-west-2:1111111111111:loadbalancer/net/tf-lb-a-sa2n7/25a8919dc2841c43
		  arn_suffix = net/tf-lb-a-sa2n7/25a8919dc2841c43
		  dns_name = tf-lb-a-sa2n7-25a8919dc2841c43.elb.us-west-2.amazonaws.com
		  enable_deletion_protection = false
		  idle_timeout = 60
		  internal = true
		  ip_address_type = ipv4
		  load_balancer_type = network

Ninir

Hi @atsushi-ishibashi

Thanks for the work here! This is looking really good :)

Just left a few comments to address before merging this! 👍 🚀

Ninir · 2017-12-04T08:59:06Z

aws/resource_aws_api_gateway_vpc_link.go

+
+	resp, err := conn.GetVpcLink(input)
+	if err != nil {
+		if ecrerr, ok := err.(awserr.Error); ok {


We can rewrite a part of this block to:

if isAWSErr(err, "ErrCodeNotFoundException", "some message returned by the API") { ... }

Where some message returned by the API is actually a message when this error happens! 😄

(the same should be applied to the update & delete parts)

Ninir · 2017-12-04T09:08:56Z

aws/resource_aws_api_gateway_vpc_link_test.go

+  target_arn = "${aws_lb.test_a.arn}"
+}
+`, rName, rName)
+}


We should also add a test with an actual API Gateway and a HTTP endpoint, so that we ensure it's all ok.
I'm actually wondering whether we need a hard dependency (using depends_on) on the HTTP integration since this resource is just creating a link between the API Gateway service & the VPC targets... 🤔
Not sure if this could result in race dependencies! 🤷‍♂️

Sorry, I couldn't understand what you meant so cloud you describe more details:bow: ? @Ninir

I guess what @Ninir means is that we should add a test which tests the full setup including API Gateway integration leveraging the VPC Link.

It seems we'd also need to update other API Gateway resources to make that happen, namely to add few more attributes to aws_api_gateway_integration per http://docs.aws.amazon.com/apigateway/api-reference/resource/integration/#connectionType

I'm personally ok with shipping this resource as is and addressing the above in a separate PR to keep diffs small and easier to review - what do you think @Ninir ?

Ninir · 2017-12-04T09:10:15Z

aws/resource_aws_api_gateway_vpc_link.go

+		Pending:    []string{apigateway.VpcLinkStatusPending},
+		Target:     []string{apigateway.VpcLinkStatusAvailable},
+		Refresh:    apigatewayVpcLinkRefreshStatusFunc(conn, *resp.Id),
+		Timeout:    10 * time.Minute,


Does the timeout really need to be this big? (same goes for the update)

Docs says

Creates a VPC link, under the caller's account in a selected region, in an asynchronous operation that typically takes 2-4 minutes to complete and become operational.

So I set 5 minutes:bow:

Ninir · 2017-12-04T09:11:44Z

website/docs/r/api_gateway_vpc_link.html.markdown

+  Provides an API Gateway VPC Link.
+---
+
+# aws_api_gateway_usage_plan


Copy/pasta typo? 😄

SmithKevin · 2018-01-03T00:06:52Z

Just checking on the status of this as I am really looking forward to this feature. Looks like it has stalled a bit over the holidays.

Ninir · 2018-01-04T11:06:47Z

@atsushi-ishibashi Just encountering an issue with the tests:

$ TF_LOG=DEBUG TF_LOG_PATH=terraform.log make testacc TEST=./aws TESTARGS='-run=TestAccAwsAPIGatewayVpcLink_'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAPIGatewayVpcLink_ -timeout 120m
=== RUN   TestAccAwsAPIGatewayVpcLink_basic
--- FAIL: TestAccAwsAPIGatewayVpcLink_basic (582.98s)
	testing.go:563: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error applying: 1 error(s) occurred:
		* aws_lb.test_a (destroy): 1 error(s) occurred:
		* aws_lb.test_a: Error deleting ALB: ResourceInUse: Load balancer 'arn:aws:elasticloadbalancing:us-west-2:XXXXXXXXX:loadbalancer/net/tf-lb-a-esy6k/c86aa43c1258c6ce' cannot be deleted because it is currently associated with another service
			status code: 400, request id: b55115ef-f0bb-11e7-8cc6-6b0dd342af2b

Do you have time to investigate?

Thanks!

atsushi-ishibashi · 2018-01-04T16:35:25Z

@Ninir Uhhh.. There may be possibility that vpc link was deleting when api to delete nlb was called.

atsushi-ishibashi · 2018-01-04T16:56:31Z

After DeleteVpcLink was called, waited for a minute then test passed.

func resourceAwsApiGatewayVpcLinkDelete(d *schema.ResourceData, meta interface{}) error {
	conn := meta.(*AWSClient).apigateway

	input := &apigateway.DeleteVpcLinkInput{
		VpcLinkId: aws.String(d.Id()),
	}

	_, err := conn.DeleteVpcLink(input)
	if err != nil {
		if isAWSErr(err, apigateway.ErrCodeNotFoundException, "") {
			d.SetId("")
			return nil
		}
		return err
	}

	time.Sleep(60 * time.Second)

	d.SetId("")
	return nil
}

What is the best way to wait until completely deleted?

atsushi-ishibashi · 2018-01-05T02:30:10Z

VpcLinkStatus doesn't have DELETED so we cannot apply WaitForState 😕

const (
    // VpcLinkStatusAvailable is a VpcLinkStatus enum value
    VpcLinkStatusAvailable = "AVAILABLE"

    // VpcLinkStatusPending is a VpcLinkStatus enum value
    VpcLinkStatusPending = "PENDING"

    // VpcLinkStatusDeleting is a VpcLinkStatus enum value
    VpcLinkStatusDeleting = "DELETING"

    // VpcLinkStatusFailed is a VpcLinkStatus enum value
    VpcLinkStatusFailed = "FAILED"
)

bflad · 2018-01-05T14:20:50Z

I have it's possible to use resource.StateChangeConf in these cases where you set Pending to all the mentioned status constants and Target to "" which you can return when you receive the NotFoundException

atsushi-ishibashi · 2018-01-05T14:40:50Z

Good idea👍 Thanks!

lukehoban · 2018-01-06T05:29:04Z

aws/resource_aws_api_gateway_vpc_link.go

+		Pending:    []string{apigateway.VpcLinkStatusPending},
+		Target:     []string{apigateway.VpcLinkStatusAvailable},
+		Refresh:    apigatewayVpcLinkRefreshStatusFunc(conn, *resp.Id),
+		Timeout:    5 * time.Minute,


I was trying this PR out, and hit a case where this operation timed out after 5 minutes. The VPC Link did end up being successfully created. The AWS docs note that this can take "2-4 minutes", so it may be worth allowing more buffer here (and on update), to ensure these don't time out under normal conditions?

https://docs.aws.amazon.com/cli/latest/reference/apigateway/create-vpc-link.html

Thank you for your advice!

atsushi-ishibashi · 2018-01-06T07:07:15Z

@Ninir Ok, the test passed

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

a-h · 2018-01-12T15:03:53Z

website/docs/r/api_gateway_vpc_link.html.markdown

+
+* `name` - (Required) The name used to label and identify the VPC link.
+* `description` - (Optional) The description of the VPC link.
+* `target_arn` - (Required, ForceNew) The ARN of network load balancer of the VPC targeted by the VPC link.


(Required, ForceNew) The ARN of a network load balancer in the VPC targeted by the VPC link.

Thank you for correct me!

christophwitzko · 2018-01-23T10:21:34Z

Hey folks, this looks very promising. What is the status of this pull request? Would love to see this resource in terraform. ✌️

mattvv · 2018-02-06T23:44:04Z

Also checking in on this.

I'm also thinking that aws_api_gateway_integration will need to be updated to allow resources with type VPC

radeksimko

@atsushi-ishibashi thanks for the PR and @Ninir for earlier review.

I left you some (hopefully final) questions/comments.

radeksimko · 2018-02-08T14:13:14Z

aws/resource_aws_api_gateway_vpc_link.go

+		return fmt.Errorf("[WARN] Error waiting for APIGateway Vpc Link status to be \"%s\": %s", apigateway.VpcLinkStatusAvailable, err)
+	}
+
+	d.SetId(*resp.Id)


We usually set the ID as soon as we can after receiving it from the API, before making any further requests so that it's recorded in the state in case any future API requests fail for any reason. Do you mind moving it above the StateChangeConf block?

radeksimko · 2018-02-08T14:14:16Z

aws/resource_aws_api_gateway_vpc_link.go

+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,
+			},


Is there any particular reason this should be implemented as 1-1 relationship instead of reflecting the API, i.e. allowing users to assign multiple LBs to one VPC link?

radeksimko · 2018-02-08T14:14:39Z

aws/resource_aws_api_gateway_vpc_link.go

+	resp, err := conn.GetVpcLink(input)
+	if err != nil {
+		if isAWSErr(err, apigateway.ErrCodeNotFoundException, "") {
+			d.SetId("")


Nitpick: Do you mind adding our usual WARN log message here?

radeksimko · 2018-02-08T14:16:21Z

aws/resource_aws_api_gateway_vpc_link.go

+		Pending: []string{apigateway.VpcLinkStatusPending,
+			apigateway.VpcLinkStatusAvailable,
+			apigateway.VpcLinkStatusDeleting},
+		Target:     []string{"deleted"},


Using empty string here is equally valid and in fact preferred, over making up our own statuses 😉

radeksimko · 2018-02-08T14:17:16Z

aws/resource_aws_api_gateway_vpc_link.go

+		return err
+	}
+
+	d.SetId("")


Nitpick: I know we still have it in many resources, but it's redundant. The ID is emptied automatically in the core.

See https://github.com/hashicorp/terraform/blob/096847c9f774bfb946de7413260b30f9f6041241/helper/schema/resource.go#L197-L206

radeksimko · 2018-02-08T14:20:05Z

aws/resource_aws_api_gateway_vpc_link_test.go

+			return err
+		}
+
+		if *resp.Status != apigateway.VpcLinkStatusDeleting {


I think we should treat any positive response here as a failure - the VPC link should be already deleted by the time test finishes and practically have no status (i.e. it shouldn't be in deleting status).

radeksimko · 2018-02-08T14:20:24Z

aws/resource_aws_api_gateway_vpc_link_test.go

+		if !ok {
+			return fmt.Errorf("Not found: %s", name)
+		}
+


Do you mind checking the API here also?

atsushi-ishibashi · 2018-02-10T08:18:07Z

@radeksimko I remembered that the interface could accept a multiple of lb but you can apply only one lb. Refer to web console as well.

TF_ACC=1 go test ./aws -v -run=TestAccAwsAPIGatewayVpcLink_basic -timeout 120m
=== RUN   TestAccAwsAPIGatewayVpcLink_basic
--- FAIL: TestAccAwsAPIGatewayVpcLink_basic (589.96s)
	testing.go:503: Step 0 error: Check failed: Check 4/4 error: aws_api_gateway_vpc_link.test: Attribute 'target_arns.#' expected "2", got "1"
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	589.996s
make: *** [testacc] Error 1

However in the future, aws may support multiple so it's better to use MaxItem for now. What do you think?

radeksimko

However in the future, aws may support multiple so it's better to use MaxItem for now. What do you think?

I agree - see my inline comments. We can add MaxItems: 1 for now and eventually remove it once/if Amazon decides to raise the limit.

radeksimko · 2018-02-12T13:31:17Z

aws/resource_aws_api_gateway_vpc_link_test.go

+  cidr_block = "10.10.0.0/21"
+  availability_zone = "${data.aws_availability_zones.test.names[0]}"
+}
+`, rName, rName)


The main reason the test is failing the way it's failing is because two resources (aws_lb.test_a and aws_lb.test_b) both have the same name and the TypeSet will internally merge them into a single ARN since the ARN is the same.

Once you fix that you get more reasonable error which I think we can just present to the user:

* aws_api_gateway_vpc_link.test: BadRequestException: More than one target arn specified for vpc link. status code: 400, request id: d4eabe6c-0ff8-11e8-bea2-d9187e5e8757

As you rightly mentioned Amazon currently doesn't support more than 1 LB though, so we'll have to keep this test focused on single LB only.

That's right! Thank you for correct.

atsushi-ishibashi · 2018-02-16T13:41:51Z

@radeksimko Ok👍

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

Stale review

radeksimko

✅ 👍 Thanks @atsushi-ishibashi

bflad · 2018-02-27T12:40:18Z

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:55:34Z

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!

WIP

64134dd

radeksimko added the new-resource Introduces a new resource. label Dec 1, 2017

Add acctest, docs

93bf79d

atsushi-ishibashi changed the title ~~[WIP]New Resource: APIGateway VPC Link~~ New Resource: APIGateway VPC Link Dec 2, 2017

Ninir previously requested changes Dec 4, 2017

View reviewed changes

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

Reflect reviews except one

0bbcedc

lukehoban mentioned this pull request Dec 21, 2017

Update to 1.6.0 pulumi/terraform-provider-aws#13

Merged

This was referenced Dec 21, 2017

Feature request: support API Gateway VPC Links #2738

Closed

[FR] Support for VPC Link integration type on API GW #2511

Closed

radeksimko requested a review from Ninir January 3, 2018 12:46

wip

1b40524

wip

19ab262

lukehoban reviewed Jan 6, 2018

View reviewed changes

Modify

70a6355

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2018

a-h reviewed Jan 12, 2018

View reviewed changes

radeksimko added the service/apigateway Issues and PRs that pertain to the apigateway service. label Jan 16, 2018

radeksimko changed the title ~~New Resource: APIGateway VPC Link~~ New Resource: aws_api_gateway_vpc_link Jan 16, 2018

radeksimko reviewed Feb 8, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 8, 2018

updates

d18c6bf

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 10, 2018

radeksimko suggested changes Feb 12, 2018

View reviewed changes

reflect reviews

c91d313

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 16, 2018

radeksimko self-assigned this Feb 16, 2018

radeksimko removed the request for review from Ninir February 16, 2018 13:55

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 16, 2018

radeksimko added this to the v1.10.0 milestone Feb 16, 2018

radeksimko approved these changes Feb 16, 2018

View reviewed changes

radeksimko merged commit 564e006 into hashicorp:master Feb 16, 2018

loivis mentioned this pull request Feb 17, 2018

resource/api_gateway_integration: support vpc link #3428

Merged

ghost locked and limited conversation to collaborators Apr 7, 2020

New Resource: aws_api_gateway_vpc_link #2512

New Resource: aws_api_gateway_vpc_link #2512

Conversation

atsushi-ishibashi commented Dec 1, 2017 • edited Loading

atsushi-ishibashi commented Dec 2, 2017 • edited Loading

atsushi-ishibashi commented Dec 2, 2017 • edited Loading

Ninir 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmithKevin commented Jan 3, 2018

Ninir commented Jan 4, 2018

atsushi-ishibashi commented Jan 4, 2018

atsushi-ishibashi commented Jan 4, 2018 • edited Loading

atsushi-ishibashi commented Jan 5, 2018

bflad commented Jan 5, 2018

atsushi-ishibashi commented Jan 5, 2018

lukehoban Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophwitzko commented Jan 23, 2018

mattvv commented Feb 6, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Feb 10, 2018

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

atsushi-ishibashi commented Feb 16, 2018

radeksimko left a comment

Choose a reason for hiding this comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

atsushi-ishibashi commented Dec 1, 2017 •

edited

Loading

atsushi-ishibashi commented Dec 2, 2017 •

edited

Loading

atsushi-ishibashi commented Dec 2, 2017 •

edited

Loading

atsushi-ishibashi commented Jan 4, 2018 •

edited

Loading

lukehoban Jan 6, 2018 •

edited

Loading