Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Resource: aws_api_gateway_vpc_link #2512

Merged
merged 8 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ func Provider() terraform.ResourceProvider {
"aws_api_gateway_stage": resourceAwsApiGatewayStage(),
"aws_api_gateway_usage_plan": resourceAwsApiGatewayUsagePlan(),
"aws_api_gateway_usage_plan_key": resourceAwsApiGatewayUsagePlanKey(),
"aws_api_gateway_vpc_link": resourceAwsApiGatewayVpcLink(),
"aws_app_cookie_stickiness_policy": resourceAwsAppCookieStickinessPolicy(),
"aws_appautoscaling_target": resourceAwsAppautoscalingTarget(),
"aws_appautoscaling_policy": resourceAwsAppautoscalingPolicy(),
Expand Down
200 changes: 200 additions & 0 deletions aws/resource_aws_api_gateway_vpc_link.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
package aws

import (
"fmt"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/apigateway"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsApiGatewayVpcLink() *schema.Resource {
return &schema.Resource{
Create: resourceAwsApiGatewayVpcLinkCreate,
Read: resourceAwsApiGatewayVpcLinkRead,
Update: resourceAwsApiGatewayVpcLinkUpdate,
Delete: resourceAwsApiGatewayVpcLinkDelete,

Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
},
"description": {
Type: schema.TypeString,
Optional: true,
},
"target_arn": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

},
}
}

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

input := &apigateway.CreateVpcLinkInput{
Name: aws.String(d.Get("name").(string)),
TargetArns: []*string{aws.String(d.Get("target_arn").(string))},
}
if v, ok := d.GetOk("description"); ok {
input.Description = aws.String(v.(string))
}

resp, err := conn.CreateVpcLink(input)
if err != nil {
return err
}

stateConf := &resource.StateChangeConf{
Pending: []string{apigateway.VpcLinkStatusPending},
Target: []string{apigateway.VpcLinkStatusAvailable},
Refresh: apigatewayVpcLinkRefreshStatusFunc(conn, *resp.Id),
Timeout: 5 * time.Minute,
Copy link

@lukehoban lukehoban Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your advice!

MinTimeout: 3 * time.Second,
}

_, err = stateConf.WaitForState()
if err != nil {
return fmt.Errorf("[WARN] Error waiting for APIGateway Vpc Link status to be \"%s\": %s", apigateway.VpcLinkStatusAvailable, err)
}

d.SetId(*resp.Id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return nil
}

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}
return err
}

d.Set("name", resp.Name)
d.Set("description", resp.Description)
d.Set("target_arn", resp.TargetArns[0])
return nil
}

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

operations := make([]*apigateway.PatchOperation, 0)

if d.HasChange("name") {
operations = append(operations, &apigateway.PatchOperation{
Op: aws.String("replace"),
Path: aws.String("/name"),
Value: aws.String(d.Get("name").(string)),
})
}

if d.HasChange("description") {
operations = append(operations, &apigateway.PatchOperation{
Op: aws.String("replace"),
Path: aws.String("/description"),
Value: aws.String(d.Get("description").(string)),
})
}

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

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

stateConf := &resource.StateChangeConf{
Pending: []string{apigateway.VpcLinkStatusPending},
Target: []string{apigateway.VpcLinkStatusAvailable},
Refresh: apigatewayVpcLinkRefreshStatusFunc(conn, d.Id()),
Timeout: 5 * time.Minute,
MinTimeout: 3 * time.Second,
}

_, err = stateConf.WaitForState()
if err != nil {
return fmt.Errorf("[WARN] Error waiting for APIGateway Vpc Link status to be \"%s\": %s", apigateway.VpcLinkStatusAvailable, err)
}

return nil
}

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
}

stateConf := resource.StateChangeConf{
Pending: []string{apigateway.VpcLinkStatusPending,
apigateway.VpcLinkStatusAvailable,
apigateway.VpcLinkStatusDeleting},
Target: []string{"deleted"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Timeout: 3 * time.Minute,
MinTimeout: 1 * time.Second,
Refresh: func() (interface{}, string, error) {
resp, err := conn.GetVpcLink(&apigateway.GetVpcLinkInput{
VpcLinkId: aws.String(d.Id()),
})
if err != nil {
if isAWSErr(err, apigateway.ErrCodeNotFoundException, "") {
return nil, "deleted", nil
}
return nil, "failed", err
}
return resp, *resp.Status, nil
},
}

if _, err := stateConf.WaitForState(); err != nil {
return err
}

d.SetId("")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}

func apigatewayVpcLinkRefreshStatusFunc(conn *apigateway.APIGateway, vl string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
input := &apigateway.GetVpcLinkInput{
VpcLinkId: aws.String(vl),
}
resp, err := conn.GetVpcLink(input)
if err != nil {
return nil, "failed", err
}
return resp, *resp.Status, nil
}
}
132 changes: 132 additions & 0 deletions aws/resource_aws_api_gateway_vpc_link_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package aws

import (
"fmt"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/apigateway"
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAwsAPIGatewayVpcLink_basic(t *testing.T) {
rName := acctest.RandString(5)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsAPIGatewayVpcLinkDestroy,
Steps: []resource.TestStep{
{
Config: testAccAPIGatewayVpcLinkConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsAPIGatewayVpcLinkExists("aws_api_gateway_vpc_link.test"),
resource.TestCheckResourceAttr("aws_api_gateway_vpc_link.test", "name", fmt.Sprintf("tf-apigateway-%s", rName)),
resource.TestCheckResourceAttr("aws_api_gateway_vpc_link.test", "description", "test"),
),
},
{
Config: testAccAPIGatewayVpcLinkConfig_Update(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsAPIGatewayVpcLinkExists("aws_api_gateway_vpc_link.test"),
resource.TestCheckResourceAttr("aws_api_gateway_vpc_link.test", "name", fmt.Sprintf("tf-apigateway-update-%s", rName)),
resource.TestCheckResourceAttr("aws_api_gateway_vpc_link.test", "description", "test update"),
),
},
},
})
}

func testAccCheckAwsAPIGatewayVpcLinkDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).apigateway

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_api_gateway_vpc_link" {
continue
}

input := &apigateway.GetVpcLinkInput{
VpcLinkId: aws.String(rs.Primary.ID),
}

resp, err := conn.GetVpcLink(input)
if err != nil {
if isAWSErr(err, apigateway.ErrCodeNotFoundException, "") {
return nil
}
return err
}

if *resp.Status != apigateway.VpcLinkStatusDeleting {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return fmt.Errorf("APIGateway VPC Link (%s) not deleted", rs.Primary.ID)
}
}

return nil
}

func testAccCheckAwsAPIGatewayVpcLinkExists(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
_, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("Not found: %s", name)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind checking the API here also?

return nil
}
}

func testAccAPIGatewayVpcLinkConfig(rName string) string {
return fmt.Sprintf(`
resource "aws_lb" "test_a" {
name = "tf-lb-a-%s"
internal = true
load_balancer_type = "network"
subnets = ["${aws_subnet.test.id}"]
}

resource "aws_vpc" "test" {
cidr_block = "10.10.0.0/16"
}

resource "aws_subnet" "test" {
vpc_id = "${aws_vpc.test.id}"
cidr_block = "10.10.0.0/21"
availability_zone = "us-west-2a"
}

resource "aws_api_gateway_vpc_link" "test" {
name = "tf-apigateway-%s"
description = "test"
target_arn = "${aws_lb.test_a.arn}"
}
`, rName, rName)
}

func testAccAPIGatewayVpcLinkConfig_Update(rName string) string {
return fmt.Sprintf(`
resource "aws_lb" "test_a" {
name = "tf-lb-a-%s"
internal = true
load_balancer_type = "network"
subnets = ["${aws_subnet.test.id}"]
}

resource "aws_vpc" "test" {
cidr_block = "10.10.0.0/16"
}

resource "aws_subnet" "test" {
vpc_id = "${aws_vpc.test.id}"
cidr_block = "10.10.0.0/21"
availability_zone = "us-west-2a"
}

resource "aws_api_gateway_vpc_link" "test" {
name = "tf-apigateway-update-%s"
description = "test update"
target_arn = "${aws_lb.test_a.arn}"
}
`, rName, rName)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

3 changes: 3 additions & 0 deletions website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@
<li<%= sidebar_current("docs-aws-resource-api-gateway-usage-plan-key") %>>
<a href="/docs/providers/aws/r/api_gateway_usage_plan_key.html">aws_api_gateway_usage_plan_key</a>
</li>
<li<%= sidebar_current("docs-aws-resource-api-gateway-vpc-link") %>>
<a href="/docs/providers/aws/r/api_gateway_vpc_link.html">aws_api_gateway_vpc_link</a>
</li>
</ul>
</li>

Expand Down
45 changes: 45 additions & 0 deletions website/docs/r/api_gateway_vpc_link.html.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
---
layout: "aws"
page_title: "AWS: aws_api_gateway_vpc_link"
sidebar_current: "docs-aws-resource-api-gateway-vpc-link"
description: |-
Provides an API Gateway VPC Link.
---

# aws_api_gateway_vpc_link

Provides an API Gateway VPC Link.

## Example Usage

```hcl
resource "aws_lb" "example" {
name = "example"
internal = true
load_balancer_type = "network"

subnet_mapping {
subnet_id = "12345"
}
}

resource "aws_api_gateway_vpc_link" "example" {
name = "example"
description = "example description"
target_arn = "${aws_lb.example.arn}"
}
```

## Argument Reference

The following arguments are supported:

* `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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for correct me!


## Attributes Reference

The following attributes are exported:

* `id` - The identifier of the VpcLink.