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

aws_route/aws_route_table: Remove 'instance_id' target #14197

Closed
ewbankkit opened this issue Jul 15, 2020 · 6 comments · Fixed by #30804
Closed

aws_route/aws_route_table: Remove 'instance_id' target #14197

ewbankkit opened this issue Jul 15, 2020 · 6 comments · Fixed by #30804
Assignees
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. service/vpc Issues and PRs that pertain to the vpc service.
Milestone

Comments

@ewbankkit
Copy link
Contributor

ewbankkit commented Jul 15, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Relates: #5745.
Relates: #1426.

After a route is created that targets an EC2 instance (as Amazon recommend for NAT instances) via the instance_id attribute, reading back the route from the EC2 API also returns the ENI ID of the instance's primary network interface (network_interface_id attribute). Similarly, after a route that targets an ENI attached to an instance (as Amazon recommend for middlebox applicances) via the network_interface_id attribute, reading back the route also returns the Instance ID of that ENI attachment (instance_id attribute).

For the aws_route resource this was addressed initially in hashicorp/terraform#5321 by setting the route target attributes to Computed: true although then causes additional problems when updating the route, addressed in hashicorp/terraform#7686 and #14531. Even with these fixes the diff displayed by Terraform when changing a route target shows the updated routing having both old and new target attributes set.

For the aws_route_table resource, where the individual routes are members of a set, the individual target attributes are not set as Computed: true (although the enclosing route set is). This causes the additional attribute (network_interface_id or instance_id not to be written to state and the eternal diff mentioned in #1426 occurs.
If we mark instance_id and network_interface_id as both Computed: true in the route attribute's schema then we end up having to change the associated set hash function to choose one or other of those attributes to include in the hash and ignore the other - There is then no way to avoid continual diffs for one of network_interface_id or instance_id.

The long-term solution is to remove the instance_id attribute as a route target as the instance's primary ENI's ID can always be used in the network_interface_id attribute - This seems to be the preferred target according to the newer (middlebox appliance) documentation.

@ewbankkit ewbankkit added enhancement Requests to existing resources that expand the functionality or scope. breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. proposal Proposes new design or functionality. labels Jul 15, 2020
@ewbankkit ewbankkit added this to the v4.0.0 milestone Jul 15, 2020
@sylr
Copy link
Contributor

sylr commented Jan 7, 2022

@ewbankkit Can we proceed with that breaking change?

Or at least could we make network_interface_id and instance_id not mutually exclusive so that we can set both of them in our declarations and thus avoid the diff?

@ewbankkit
Copy link
Contributor Author

@sylr I have added the deprecation of instance_id in aws_route and aws_route_table to the list of tasks for v4.0.0 which we will be starting on very soon. This means that we can remove the argument in v5.0.0.
Given that both the aws_route and aws_route_table resources are heavily used and fundamental to many practitioners' infrastructures we need to be conservative in making changes and give plenty of notice.

@sylr
Copy link
Contributor

sylr commented Jan 7, 2022

Are you saying that for the foreseeable future, anyone who needs to use AWS routes with network interfaces is bound to break terraform plan without any kind of workaround?

sylr added a commit to sylr/terraform-provider-aws that referenced this issue Jan 7, 2022
Route tables routes can be defined with instance_id XOR network_interface_id
and that is a problem because AWS returns both properties when routes are retrieved
and thus it indefinitely generates a terraform diff.

This change is supposed to happen in the v5.0.0 of the provider
according to hashicorp#14197 (comment)

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@anGie44 anGie44 self-assigned this Jan 19, 2022
@anGie44 anGie44 modified the milestones: v4.0.0, v5.0.0 Jan 19, 2022
sylr added a commit to sylr/terraform-provider-aws that referenced this issue Jan 21, 2022
Route table routes can be defined with instance_id XOR network_interface_id
and that is a problem because AWS returns both properties when routes are retrieved
and thus it indefinitely generates a terraform diff.

This change is supposed to happen in the v5.0.0 of the provider
according to hashicorp#14197 (comment)

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@anGie44 anGie44 removed their assignment Jun 21, 2022
@YakDriver YakDriver self-assigned this Apr 18, 2023
@YakDriver YakDriver changed the title Proposal: Remove 'instance_id' target from route (table) resources aws_route/aws_route_table: Remove 'instance_id' target Apr 19, 2023
@YakDriver YakDriver linked a pull request Apr 19, 2023 that will close this issue
@YakDriver YakDriver added service/ec2 Issues and PRs that pertain to the ec2 service. service/vpc Issues and PRs that pertain to the vpc service. and removed proposal Proposes new design or functionality. labels Apr 20, 2023
@github-actions github-actions bot modified the milestones: v5.0.0, v4.64.0 Apr 20, 2023
@YakDriver YakDriver modified the milestones: v4.64.0, v5.0.0 Apr 20, 2023
@jar-b
Copy link
Member

jar-b commented May 23, 2023

Closed by #30804, merged to main via #31392

@jar-b jar-b closed this as completed May 23, 2023
@github-actions
Copy link

This functionality has been released in v5.0.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 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. service/vpc Issues and PRs that pertain to the vpc service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants