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 2 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
187 changes: 187 additions & 0 deletions aws/resource_aws_api_gateway_vpc_link.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
package aws

import (
"fmt"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"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: 10 * time.Minute,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

Delay: 10 * time.Second,
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 ecrerr, ok := err.(awserr.Error); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

switch ecrerr.Code() {
case apigateway.ErrCodeNotFoundException:
d.SetId("")
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 ecrerr, ok := err.(awserr.Error); ok {
switch ecrerr.Code() {
case 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: 10 * time.Minute,
Delay: 10 * time.Second,
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 ecrerr, ok := err.(awserr.Error); ok {
switch ecrerr.Code() {
case apigateway.ErrCodeNotFoundException:
d.SetId("")
return 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
}
}
144 changes: 144 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,144 @@
package aws

import (
"fmt"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"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 ecrerr, ok := err.(awserr.Error); ok {
switch ecrerr.Code() {
case 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"

subnet_mapping {
subnet_id = "${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"
map_public_ip_on_launch = true
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"

subnet_mapping {
subnet_id = "${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"
map_public_ip_on_launch = true
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_usage_plan
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy/pasta typo? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅


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.