Add new aws_vpc_endpoint_route_table_association resource #10137

ewbankkit · 2016-11-15T17:22:02Z

This PR follows on from #9244 and addresses #9138.
Here's the log from @kwilczynski's initial commit:

This commit adds a new resource which allows to a list of route tables to be
either added and/or removed from an existing VPC Endpoint. This resource would
also be complimentary to the existing `aws_vpc_endpoint` resource where the
route tables might not be specified (not a requirement for a VPC Endpoint to
be created successfully) during creation, especially where the workflow is
such where the route tables are not immediately known.

As it stands now the new aws_vpc_endpoint_route_table_association resource associates/dissociates as single route_table_id with a VPC endpoint.
Somehow I feel it's "better" Terraform to use a count attribute on the resource to associate multiple route tables with a VPC endpoint rather than accept a list of route tables in a single association, e.g.

resource "aws_vpc" "foo" {
    cidr_block = "10.0.0.0/16"
}

resource "aws_vpc_endpoint" "s3" {
    vpc_id = "${aws_vpc.foo.id}"
    service_name = "com.amazonaws.us-west-2.s3"
}

variable "c" {
    default = 2
}

resource "aws_route_table" "rt" {
    count = "${var.c}"

    vpc_id = "${aws_vpc.foo.id}"

    tags {
        Name = "test"
    }
}

resource "aws_vpc_endpoint_route_table_association" "a" {
    count = "${var.c}"

	vpc_endpoint_id = "${aws_vpc_endpoint.s3.id}"
	route_table_id  = "${element(aws_route_table.rt.*.id, count.index)}"
}

Since this is a "synthetic" resource (no underlying AWS resource maps exactly to this) this doesn't cause a cognitive disconnect, plus it greatly simplifies the implementation.
I welcome feedback on this, especially from @coen-hyde and @supergibbs who commented on the associated issue.

This commit adds a new resource which allows to a list of route tables to be either added and/or removed from an existing VPC Endpoint. This resource would also be complimentary to the existing `aws_vpc_endpoint` resource where the route tables might not be specified (not a requirement for a VPC Endpoint to be created successfully) during creation, especially where the workflow is such where the route tables are not immediately known. Signed-off-by: Krzysztof Wilczynski <[email protected]> Additions by Kit Ewbank <[email protected]>: * Add functionality * Add documentation * Add acceptance tests * Set VPC endpoint route_table_ids attribute to "Computed"

ewbankkit · 2016-11-15T17:22:25Z

Acceptance tests:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVpcEndpointRouteTableAssociation_'

jrnt30 · 2016-11-18T14:55:28Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+	if err != nil {
+		return fmt.Errorf("Error creating VPC Endpoint/Route Table association: %s", err.Error())
+	}
+


I believe generally you should be calling the d.SetId here to populate the state, the fact that you call Read immediately makes it a bit less necessary but is the pattern I've seen around the code base.

OK, I'll fix that.

ewbankkit · 2016-11-19T22:15:42Z

Removing the [WIP] marker.
I am considering this as ready for consideration.

kwilczynski · 2016-11-21T14:58:42Z

@ewbankkit hi there! Thank you for completing this when I could not do it. You are a ⭐️ !

I do agree that relying on count seem much more natural with Terraform - my idea of having the ability to pass multiple IDs definitely is not as flexible as using the count 👍 .

kwilczynski · 2016-11-21T14:59:44Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+	return &schema.Resource{
+		Create: resourceAwsVPCEndpointRouteTableAssociationCreate,
+		Read:   resourceAwsVPCEndpointRouteTableAssociationRead,
+		// "resource aws_vpc_endpoint_route_table_association: All fields are ForceNew or Computed w/out Optional, Update is superfluous"


I would get rid of the comment here. If we are never going to use UpdateFunc then I would not bother with clarification.

kwilczynski · 2016-11-21T20:34:23Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+			return fmt.Errorf("Error deleting VPC Endpoint/Route Table association: %s", err.Error())
+		}
+
+		switch ec2err.Code() {


Here, I would use fallthrough for both NotFound and simply log a simple message. Even more, perhaps catch all in the fallthrough case, and then issue a single message noting that it is gone (we do that in few other places). I could also be re-factored to catch the !ok case once, I believe.

kwilczynski · 2016-11-21T20:36:14Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+	return fmt.Sprintf("a-%s%d", endpointId, hashcode.String(rtId))
+}
+
+type vpcEndpointNotFound struct {


I am struggling with this a bit - do we need a custom type (also same in the code below) for two error messages?

I'll revisit. I based that on another resource and maybe it's overkill in this situation.

* Removed error types and simplified the error handling in 'resourceAwsVPCEndpointRouteTableAssociationRead' * Simplified logging in 'resourceAwsVPCEndpointRouteTableAssociationDelete'

ewbankkit · 2016-11-23T20:45:41Z

Pushed changes after @kwilczynski's code review:

Removed error types and simplified the error handling in resourceAwsVPCEndpointRouteTableAssociationRead
Simplified logging in resourceAwsVPCEndpointRouteTableAssociationDelete

stack72 · 2016-11-29T17:04:36Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+	}
+	if !found {
+		// The association no longer exists.
+		d.SetId("")


A quick follow up point for the future - we usually log a message here just to let the user know we have removed from state

OK. I won't change this now. Thanks for the tip.

stack72 · 2016-11-29T17:06:26Z

builtin/providers/aws/resource_aws_vpc_endpoint_route_table_association.go

+		return nil, err
+	}
+
+	return output.VpcEndpoints[0], nil


Can we guarantee that it's the first endpoint in the collection?

Yes. The desired VPC endpoint ID is passed as an input parameter and the API call will return either that VPC endpoint or a client error such as InvalidVpcEndpointId.Malformed or InvalidVpcEndpointId.NotFound.
We should be good here.

stack72 · 2016-11-29T17:07:47Z

Hi @ewbankkit

this LGTM! I left a few small questions inline - if you are happy with those then we can push forward

Thanks to @kwilczynski for the review as well

P.

ewbankkit · 2016-12-04T21:57:21Z

Sorry, at re:Invent all last week. I'll address these now...

stack72 · 2016-12-05T12:55:35Z

Hi @ewbankkit

Thanks for the work here (and the responses to my nit questions)

This LGTM!

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSVpcEndpointRouteTableAssociation_'                                                                                               ✹
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/05 12:53:35 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSVpcEndpointRouteTableAssociation_ -timeout 120m
=== RUN   TestAccAWSVpcEndpointRouteTableAssociation_basic
--- PASS: TestAccAWSVpcEndpointRouteTableAssociation_basic (50.68s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	50.700s

@kwilczynski

…10137) * Add new aws_vpc_endpoint_route_table_association resource. This commit adds a new resource which allows to a list of route tables to be either added and/or removed from an existing VPC Endpoint. This resource would also be complimentary to the existing `aws_vpc_endpoint` resource where the route tables might not be specified (not a requirement for a VPC Endpoint to be created successfully) during creation, especially where the workflow is such where the route tables are not immediately known. Signed-off-by: Krzysztof Wilczynski <[email protected]> Additions by Kit Ewbank <[email protected]>: * Add functionality * Add documentation * Add acceptance tests * Set VPC endpoint route_table_ids attribute to "Computed" * Changes after review - Set resource ID in create function. * Changes after code review by @kwilczynski: * Removed error types and simplified the error handling in 'resourceAwsVPCEndpointRouteTableAssociationRead' * Simplified logging in 'resourceAwsVPCEndpointRouteTableAssociationDelete'

ghost · 2020-04-19T01:52:42Z

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.

jrnt30 reviewed Nov 18, 2016

View reviewed changes

Changes after review - Set resource ID in create function.

b2e368a

ewbankkit changed the title ~~[WIP] Add new aws_vpc_endpoint_route_table_association resource~~ Add new aws_vpc_endpoint_route_table_association resource Nov 19, 2016

kwilczynski reviewed Nov 21, 2016

View reviewed changes

Changes after code review by @kwilczynski:

9fb5521

* Removed error types and simplified the error handling in 'resourceAwsVPCEndpointRouteTableAssociationRead' * Simplified logging in 'resourceAwsVPCEndpointRouteTableAssociationDelete'

stack72 added enhancement new-resource provider/aws labels Nov 29, 2016

stack72 reviewed Nov 29, 2016

View reviewed changes

stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Nov 29, 2016

stack72 removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 5, 2016

stack72 merged commit 26d0525 into hashicorp:master Dec 5, 2016

stack72 mentioned this pull request Dec 5, 2016

Feature Request: VPC Endpoint route table association resource #9138

Closed

ewbankkit deleted the issue-9138 branch December 8, 2016 21:06

ewbankkit mentioned this pull request Oct 10, 2019

import error: aws_vpc_endpoint_route_table_association: InvalidVpcEndpointId.Malformed: Invalid Id: '' (expecting 'vpce-...; hashicorp/terraform-provider-aws#9693

Closed

ghost locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new aws_vpc_endpoint_route_table_association resource #10137

Add new aws_vpc_endpoint_route_table_association resource #10137

ewbankkit commented Nov 15, 2016

ewbankkit commented Nov 15, 2016

jrnt30 Nov 18, 2016

ewbankkit Nov 19, 2016

ewbankkit commented Nov 19, 2016

kwilczynski commented Nov 21, 2016

kwilczynski Nov 21, 2016

ewbankkit Nov 23, 2016

kwilczynski Nov 21, 2016

ewbankkit Nov 23, 2016

kwilczynski Nov 21, 2016

ewbankkit Nov 23, 2016

ewbankkit commented Nov 23, 2016

stack72 Nov 29, 2016

ewbankkit Dec 4, 2016

stack72 Nov 29, 2016

ewbankkit Dec 4, 2016

stack72 commented Nov 29, 2016

ewbankkit commented Dec 4, 2016

stack72 commented Dec 5, 2016

ghost commented Apr 19, 2020

Add new aws_vpc_endpoint_route_table_association resource #10137

Add new aws_vpc_endpoint_route_table_association resource #10137

Conversation

ewbankkit commented Nov 15, 2016

ewbankkit commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewbankkit commented Nov 19, 2016

kwilczynski commented Nov 21, 2016

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

ewbankkit commented Nov 23, 2016

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

stack72 commented Nov 29, 2016

ewbankkit commented Dec 4, 2016

stack72 commented Dec 5, 2016

ghost commented Apr 19, 2020