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

Adding local as supported target for gateway_id #24507

Merged
merged 8 commits into from
Apr 17, 2023

Conversation

ddericco
Copy link
Contributor

@ddericco ddericco commented May 2, 2022

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #21350.
Relates #11455.

Output from acceptance testing: TBD - still need to create tests

@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. service/vpc Issues and PRs that pertain to the vpc service. size/XS Managed by automation to categorize the size of a PR. labels May 2, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @ddericco 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@deutmeyerbrianpfg
Copy link

Will this PR cover vpc_endpoint_id, as well?

@ddericco
Copy link
Contributor Author

Yes - the expected workflow would be to update an existing more-specific route (e.g. to a Gateway Load Balancer endpoint) to the local route, e.g.:

# Existing route to endpoint
resource "aws_route" "firewall_route" {
    route_table_id = "rtb-0123456789abcde"
    destination_cidr_block = "10.254.0.0/24"
    vpc_endpoint_id = "vpce-987654321dcba"
}
# Revert to local route
resource "aws_route" "firewall_route" {
    route_table_id = "rtb-0123456789abcde"
    destination_cidr_block = "10.254.0.0/24"
    gateway_id = "local"
}

Note that this deviates from the API behavior in that you can't call ReplaceRoute with gateway_id = "local" - as others have pointed out, this instead requires a bool value on the LocalTarget attribute1. This attribute isn't returned when calling DescribeRouteTables2, which means a resource that uses the LocalTarget bool will be flagged for an unnecessary update/replacement. Using the gateway_id = "local" syntax matches the API output and provides an easier experience for the end user.

I'm having a challenge creating appropriate acceptance tests, however. Specifically, creating an import function3 in acceptance tests that retains the imported resource ID in the state passed to the next step. This might be my lack of experience with Go or building for the Terraform provider.

The expected step-by-step would look like:

  • Create the VPC and other resources (GWLB, GWLBe, etc.)
  • Import the local route for the VPC CIDR block (created during VPC creation) into the state, and pass this state into future steps
  • Update the imported local route to point to an endpoint (GWLBe) target - currently this fails with an error that the route already exists
  • Update the endpoint route back to point to "local" target

This works when using standard Terraform config (plan, apply, etc.) but fails in acceptance testing, I suspect due to the reasons mentioned above. The goal is to be able to change to/from the local target to account for scenarios like Figure 2 in this architecture4.

If anyone has any links to other examples where state is altered with an import and then passed to the next test step, that would allow me to move forward.

Footnotes

  1. https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/ec2#Client.ReplaceRoute

  2. https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/ec2#DescribeRouteTablesOutput

  3. https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/helper/resource#TestStep

  4. https://aws.amazon.com/blogs/networking-and-content-delivery/deployment-models-for-aws-network-firewall-with-vpc-routing-enhancements/

@gdavison gdavison marked this pull request as ready for review June 1, 2022 19:04
@gdavison gdavison marked this pull request as draft June 1, 2022 19:05
@gdavison
Copy link
Contributor

gdavison commented Jun 1, 2022

Oops, meant to press the "Approve and Run" button for the workflows

@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 23, 2022
@mammlouk
Copy link

Is this change going to make it into a release soon or is there more work to be done? I'm working with infrastructure that uses a custom built NAT solution requiring a local route to be directed to the ENI of a NAT EC2.

Our current workaround is a local_exec to call aws ec2 replace-route ....

Obviously this is less than ideal. It would be great if we could handle this directly within terraform.

@ewbankkit ewbankkit marked this pull request as ready for review April 14, 2023 21:22
@github-actions github-actions bot added size/L Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. sweeper Pertains to changes to or issues with the sweeper. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Apr 14, 2023
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccVPCRoute_' PKG=ec2 ACCTEST_PARALLELISM=4
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 4  -run=TestAccVPCRoute_ -timeout 180m
=== RUN   TestAccVPCRoute_basic
=== PAUSE TestAccVPCRoute_basic
=== RUN   TestAccVPCRoute_disappears
=== PAUSE TestAccVPCRoute_disappears
=== RUN   TestAccVPCRoute_Disappears_routeTable
=== PAUSE TestAccVPCRoute_Disappears_routeTable
=== RUN   TestAccVPCRoute_ipv6ToEgressOnlyInternetGateway
=== PAUSE TestAccVPCRoute_ipv6ToEgressOnlyInternetGateway
=== RUN   TestAccVPCRoute_ipv6ToInternetGateway
=== PAUSE TestAccVPCRoute_ipv6ToInternetGateway
=== RUN   TestAccVPCRoute_ipv6ToInstance
=== PAUSE TestAccVPCRoute_ipv6ToInstance
=== RUN   TestAccVPCRoute_IPv6ToNetworkInterface_unattached
=== PAUSE TestAccVPCRoute_IPv6ToNetworkInterface_unattached
=== RUN   TestAccVPCRoute_ipv6ToVPCPeeringConnection
=== PAUSE TestAccVPCRoute_ipv6ToVPCPeeringConnection
=== RUN   TestAccVPCRoute_ipv6ToVPNGateway
=== PAUSE TestAccVPCRoute_ipv6ToVPNGateway
=== RUN   TestAccVPCRoute_ipv4ToVPNGateway
=== PAUSE TestAccVPCRoute_ipv4ToVPNGateway
=== RUN   TestAccVPCRoute_ipv4ToInstance
=== PAUSE TestAccVPCRoute_ipv4ToInstance
=== RUN   TestAccVPCRoute_IPv4ToNetworkInterface_unattached
=== PAUSE TestAccVPCRoute_IPv4ToNetworkInterface_unattached
=== RUN   TestAccVPCRoute_IPv4ToNetworkInterface_attached
=== PAUSE TestAccVPCRoute_IPv4ToNetworkInterface_attached
=== RUN   TestAccVPCRoute_IPv4ToNetworkInterface_twoAttachments
=== PAUSE TestAccVPCRoute_IPv4ToNetworkInterface_twoAttachments
=== RUN   TestAccVPCRoute_ipv4ToVPCPeeringConnection
=== PAUSE TestAccVPCRoute_ipv4ToVPCPeeringConnection
=== RUN   TestAccVPCRoute_ipv4ToNatGateway
=== PAUSE TestAccVPCRoute_ipv4ToNatGateway
=== RUN   TestAccVPCRoute_ipv6ToNatGateway
=== PAUSE TestAccVPCRoute_ipv6ToNatGateway
=== RUN   TestAccVPCRoute_doesNotCrashWithVPCEndpoint
=== PAUSE TestAccVPCRoute_doesNotCrashWithVPCEndpoint
=== RUN   TestAccVPCRoute_ipv4ToTransitGateway
=== PAUSE TestAccVPCRoute_ipv4ToTransitGateway
=== RUN   TestAccVPCRoute_ipv6ToTransitGateway
=== PAUSE TestAccVPCRoute_ipv6ToTransitGateway
=== RUN   TestAccVPCRoute_ipv4ToCarrierGateway
=== PAUSE TestAccVPCRoute_ipv4ToCarrierGateway
=== RUN   TestAccVPCRoute_ipv4ToLocalGateway
=== PAUSE TestAccVPCRoute_ipv4ToLocalGateway
=== RUN   TestAccVPCRoute_ipv6ToLocalGateway
=== PAUSE TestAccVPCRoute_ipv6ToLocalGateway
=== RUN   TestAccVPCRoute_conditionalCIDRBlock
=== PAUSE TestAccVPCRoute_conditionalCIDRBlock
=== RUN   TestAccVPCRoute_IPv4Update_target
=== PAUSE TestAccVPCRoute_IPv4Update_target
=== RUN   TestAccVPCRoute_IPv6Update_target
=== PAUSE TestAccVPCRoute_IPv6Update_target
=== RUN   TestAccVPCRoute_ipv4ToVPCEndpoint
=== PAUSE TestAccVPCRoute_ipv4ToVPCEndpoint
=== RUN   TestAccVPCRoute_ipv6ToVPCEndpoint
=== PAUSE TestAccVPCRoute_ipv6ToVPCEndpoint
=== RUN   TestAccVPCRoute_localRoute
=== PAUSE TestAccVPCRoute_localRoute
=== RUN   TestAccVPCRoute_localRouteUpdate
=== PAUSE TestAccVPCRoute_localRouteUpdate
=== RUN   TestAccVPCRoute_prefixListToInternetGateway
=== PAUSE TestAccVPCRoute_prefixListToInternetGateway
=== RUN   TestAccVPCRoute_prefixListToVPNGateway
=== PAUSE TestAccVPCRoute_prefixListToVPNGateway
=== RUN   TestAccVPCRoute_prefixListToInstance
=== PAUSE TestAccVPCRoute_prefixListToInstance
=== RUN   TestAccVPCRoute_PrefixListToNetworkInterface_unattached
=== PAUSE TestAccVPCRoute_PrefixListToNetworkInterface_unattached
=== RUN   TestAccVPCRoute_PrefixListToNetworkInterface_attached
=== PAUSE TestAccVPCRoute_PrefixListToNetworkInterface_attached
=== RUN   TestAccVPCRoute_prefixListToVPCPeeringConnection
=== PAUSE TestAccVPCRoute_prefixListToVPCPeeringConnection
=== RUN   TestAccVPCRoute_prefixListToNatGateway
=== PAUSE TestAccVPCRoute_prefixListToNatGateway
=== RUN   TestAccVPCRoute_prefixListToTransitGateway
=== PAUSE TestAccVPCRoute_prefixListToTransitGateway
=== RUN   TestAccVPCRoute_prefixListToCarrierGateway
=== PAUSE TestAccVPCRoute_prefixListToCarrierGateway
=== RUN   TestAccVPCRoute_prefixListToLocalGateway
=== PAUSE TestAccVPCRoute_prefixListToLocalGateway
=== RUN   TestAccVPCRoute_prefixListToEgressOnlyInternetGateway
=== PAUSE TestAccVPCRoute_prefixListToEgressOnlyInternetGateway
=== CONT  TestAccVPCRoute_basic
=== CONT  TestAccVPCRoute_prefixListToEgressOnlyInternetGateway
=== CONT  TestAccVPCRoute_prefixListToLocalGateway
=== CONT  TestAccVPCRoute_prefixListToCarrierGateway
=== CONT  TestAccVPCRoute_prefixListToLocalGateway
    acctest.go:1047: skipping since no Outposts found
--- SKIP: TestAccVPCRoute_prefixListToLocalGateway (1.47s)
=== CONT  TestAccVPCRoute_prefixListToTransitGateway
--- PASS: TestAccVPCRoute_basic (25.20s)
=== CONT  TestAccVPCRoute_prefixListToNatGateway
--- PASS: TestAccVPCRoute_prefixListToCarrierGateway (31.62s)
=== CONT  TestAccVPCRoute_prefixListToVPCPeeringConnection
--- PASS: TestAccVPCRoute_prefixListToEgressOnlyInternetGateway (42.23s)
=== CONT  TestAccVPCRoute_PrefixListToNetworkInterface_attached
--- PASS: TestAccVPCRoute_prefixListToVPCPeeringConnection (29.19s)
=== CONT  TestAccVPCRoute_PrefixListToNetworkInterface_unattached
--- PASS: TestAccVPCRoute_PrefixListToNetworkInterface_unattached (32.57s)
=== CONT  TestAccVPCRoute_prefixListToInstance
--- PASS: TestAccVPCRoute_PrefixListToNetworkInterface_attached (144.51s)
=== CONT  TestAccVPCRoute_prefixListToVPNGateway
--- PASS: TestAccVPCRoute_prefixListToInstance (129.80s)
=== CONT  TestAccVPCRoute_prefixListToInternetGateway
--- PASS: TestAccVPCRoute_prefixListToNatGateway (201.03s)
=== CONT  TestAccVPCRoute_localRouteUpdate
--- PASS: TestAccVPCRoute_prefixListToInternetGateway (29.62s)
=== CONT  TestAccVPCRoute_localRoute
--- PASS: TestAccVPCRoute_localRoute (21.94s)
=== CONT  TestAccVPCRoute_ipv6ToVPCEndpoint
--- PASS: TestAccVPCRoute_localRouteUpdate (54.91s)
=== CONT  TestAccVPCRoute_ipv4ToVPCEndpoint
--- PASS: TestAccVPCRoute_prefixListToVPNGateway (154.38s)
=== CONT  TestAccVPCRoute_IPv6Update_target
--- PASS: TestAccVPCRoute_prefixListToTransitGateway (345.24s)
=== CONT  TestAccVPCRoute_IPv4Update_target
=== CONT  TestAccVPCRoute_ipv6ToVPCEndpoint
    vpc_route_test.go:1580: Step 1/2 error: Error running apply: exit status 1
        
        Error: modifying EC2 VPC Endpoint Service (vpce-svc-09bb73de9731fd4ba) permissions: InvalidPrincipal: Invalid Principal: 'arn:aws:sts::123456789012:assumed-role/terraform_team1_dev-developer/kewbank@hashicorp.com'
        	status code: 400, request id: c70192a4-c736-49ef-89eb-c13ba7fe0a8c
        
          with aws_vpc_endpoint_service.test,
          on terraform_plugin_test.tf line 40, in resource "aws_vpc_endpoint_service" "test":
          40: resource "aws_vpc_endpoint_service" "test" {
        
=== CONT  TestAccVPCRoute_ipv4ToVPCEndpoint
    vpc_route_test.go:1529: Step 1/2 error: Error running apply: exit status 1
        
        Error: modifying EC2 VPC Endpoint Service (vpce-svc-016a04265334a1ee1) permissions: InvalidPrincipal: Invalid Principal: 'arn:aws:sts::123456789012:assumed-role/terraform_team1_dev-developer/kewbank@hashicorp.com'
        	status code: 400, request id: 09f198c6-115e-46f3-9603-093d7ddf1677
        
          with aws_vpc_endpoint_service.test,
          on terraform_plugin_test.tf line 40, in resource "aws_vpc_endpoint_service" "test":
          40: resource "aws_vpc_endpoint_service" "test" {
        
--- FAIL: TestAccVPCRoute_ipv6ToVPCEndpoint (174.24s)
=== CONT  TestAccVPCRoute_conditionalCIDRBlock
--- FAIL: TestAccVPCRoute_ipv4ToVPCEndpoint (173.61s)
=== CONT  TestAccVPCRoute_ipv6ToLocalGateway
    acctest.go:1047: skipping since no Outposts found
--- SKIP: TestAccVPCRoute_ipv6ToLocalGateway (0.47s)
=== CONT  TestAccVPCRoute_ipv4ToLocalGateway
    acctest.go:1047: skipping since no Outposts found
--- SKIP: TestAccVPCRoute_ipv4ToLocalGateway (0.20s)
=== CONT  TestAccVPCRoute_ipv4ToCarrierGateway
--- PASS: TestAccVPCRoute_ipv4ToCarrierGateway (24.47s)
=== CONT  TestAccVPCRoute_ipv6ToTransitGateway
--- PASS: TestAccVPCRoute_conditionalCIDRBlock (48.59s)
=== CONT  TestAccVPCRoute_ipv4ToTransitGateway
=== CONT  TestAccVPCRoute_IPv4Update_target
    vpc_route_test.go:1145: Step 1/9 error: Error running apply: exit status 1
        
        Error: modifying EC2 VPC Endpoint Service (vpce-svc-0f4545486bdd7c8b3) permissions: InvalidPrincipal: Invalid Principal: 'arn:aws:sts::123456789012:assumed-role/terraform_team1_dev-developer/kewbank@hashicorp.com'
        	status code: 400, request id: ff0b77f9-9197-4b6a-aa7c-22c71d29a6f2
        
          with aws_vpc_endpoint_service.test,
          on terraform_plugin_test.tf line 165, in resource "aws_vpc_endpoint_service" "test":
         165: resource "aws_vpc_endpoint_service" "test" {
        
--- PASS: TestAccVPCRoute_IPv6Update_target (305.60s)
=== CONT  TestAccVPCRoute_doesNotCrashWithVPCEndpoint
--- PASS: TestAccVPCRoute_doesNotCrashWithVPCEndpoint (34.54s)
=== CONT  TestAccVPCRoute_ipv6ToNatGateway
--- FAIL: TestAccVPCRoute_IPv4Update_target (408.92s)
=== CONT  TestAccVPCRoute_ipv4ToNatGateway
--- PASS: TestAccVPCRoute_ipv4ToTransitGateway (342.72s)
=== CONT  TestAccVPCRoute_ipv4ToVPCPeeringConnection
--- PASS: TestAccVPCRoute_ipv6ToTransitGateway (363.69s)
=== CONT  TestAccVPCRoute_IPv4ToNetworkInterface_twoAttachments
--- PASS: TestAccVPCRoute_ipv4ToVPCPeeringConnection (25.20s)
=== CONT  TestAccVPCRoute_IPv4ToNetworkInterface_attached
--- PASS: TestAccVPCRoute_ipv6ToNatGateway (211.30s)
=== CONT  TestAccVPCRoute_IPv4ToNetworkInterface_unattached
--- PASS: TestAccVPCRoute_IPv4ToNetworkInterface_unattached (27.04s)
=== CONT  TestAccVPCRoute_ipv4ToInstance
--- PASS: TestAccVPCRoute_ipv4ToNatGateway (201.87s)
=== CONT  TestAccVPCRoute_ipv4ToVPNGateway
--- PASS: TestAccVPCRoute_IPv4ToNetworkInterface_attached (128.72s)
=== CONT  TestAccVPCRoute_ipv6ToVPNGateway
--- PASS: TestAccVPCRoute_IPv4ToNetworkInterface_twoAttachments (168.57s)
=== CONT  TestAccVPCRoute_ipv6ToVPCPeeringConnection
--- PASS: TestAccVPCRoute_ipv4ToInstance (121.75s)
=== CONT  TestAccVPCRoute_IPv6ToNetworkInterface_unattached
--- PASS: TestAccVPCRoute_ipv6ToVPCPeeringConnection (33.72s)
=== CONT  TestAccVPCRoute_ipv6ToInstance
--- PASS: TestAccVPCRoute_ipv4ToVPNGateway (112.97s)
=== CONT  TestAccVPCRoute_ipv6ToInternetGateway
--- PASS: TestAccVPCRoute_IPv6ToNetworkInterface_unattached (37.83s)
=== CONT  TestAccVPCRoute_ipv6ToEgressOnlyInternetGateway
--- PASS: TestAccVPCRoute_ipv6ToInternetGateway (33.64s)
=== CONT  TestAccVPCRoute_Disappears_routeTable
--- PASS: TestAccVPCRoute_ipv6ToEgressOnlyInternetGateway (43.42s)
=== CONT  TestAccVPCRoute_disappears
--- PASS: TestAccVPCRoute_Disappears_routeTable (20.13s)
--- PASS: TestAccVPCRoute_ipv6ToVPNGateway (143.46s)
--- PASS: TestAccVPCRoute_disappears (19.76s)
--- PASS: TestAccVPCRoute_ipv6ToInstance (131.28s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	1182.749s
FAIL
make: *** [testacc] Error 1

Failures are unrelated to this change.

@ewbankkit
Copy link
Contributor

@ddericco Thanks for the contribution 🎉 👏.

@ewbankkit ewbankkit merged commit 5226913 into hashicorp:main Apr 17, 2023
@github-actions github-actions bot added this to the v4.64.0 milestone Apr 17, 2023
@ddericco ddericco deleted the f-aws_route-add_local_route_target branch April 18, 2023 23:45
@github-actions
Copy link

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2023
@justinretzolk justinretzolk added the partner Contribution from a partner. label May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. partner Contribution from a partner. service/vpc Issues and PRs that pertain to the vpc service. size/L Managed by automation to categorize the size of a PR. sweeper Pertains to changes to or issues with the sweeper. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants