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

feat: Allow tagging on per-subnet basis #901

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raxod502-plaid
Copy link

@raxod502-plaid raxod502-plaid commented Feb 28, 2023

Description

New variables public_subnet_tags_per_subnet, private_subnet_tags_per_subnet, etc. Optional, no effect if not specified. If specified, they are additional tags to apply to each respective public and private subnet.

Extend the ability to allow naming and tagging route tables, EIPs, and NAT Gateways on per-subnet basis as well, add variables for that.

Motivation and Context

Previously, it was only possible to customize arbitrary tags on a per-AZ basis, not a per-subnet basis. You could customize the Name tag on a per-subnet basis but not any other tag.

The VPCs in our environment have several different types of subnets. We would like to tag them differently, e.g. for cost attribution and reference in external Terraform code. Currently this is not possible: subnets can only be disambiguated by VPC, AZ, and visibility.

Breaking Changes

No breaking changes

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

This is currently running in production

@raxod502-plaid
Copy link
Author

Testing in our infrastructure with:

image

Generating expected Terraform plan:

image

@raxod502-plaid
Copy link
Author

raxod502-plaid commented Mar 2, 2023

This is now live in our infrastructure. I also added the ability to customize route table tagging.

@raxod502-plaid
Copy link
Author

I've completed the rollout of the new tagging system throughout our infrastructure, using the code from this pull request in production. No problems uncovered.

@kahirokunn
Copy link

kahirokunn commented Mar 6, 2023

Awesome!
LGTM!

@jseiser
Copy link

jseiser commented Mar 14, 2023

I think this behavior is what we are looking for.

We wanted to be able to have 2 sets of private subnets.

1 for our EKS cluster's worker nodes, and 1 for some other random servers that still survive post EKS migration. So we could just target the server private subnets when deploying those specific EC2 Instances

Copy link

@davepattie davepattie left a comment

Choose a reason for hiding this comment

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

Ahh, I see now that tags per az is no use when you have more than one private subnet per az

@hpschry
Copy link

hpschry commented Apr 26, 2023

I'd love to see this merged, this would address our use case as well: We'll need individual tags per sub-net, e.g. to control the assignment of load balancers to specific sub-nets in an EKS scenario, which is controlled by sub-net tags.

@reoring
Copy link

reoring commented Apr 27, 2023

Once this Pull Request is merged, I will use it right away

Copy link

@KevinBrooke KevinBrooke left a comment

Choose a reason for hiding this comment

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

We have been using this branch in production for 3 weeks without any issues.

@jjdiazgarcia jjdiazgarcia mentioned this pull request May 31, 2023
@davepattie
Copy link

@antonbabenko would you mind taking a look at this PR please, it's a really helpful and thorough addition, thanks

@github-actions
Copy link

github-actions bot commented Jul 2, 2023

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jul 2, 2023
@kahirokunn
Copy link

keep

@github-actions github-actions bot removed the stale label Jul 3, 2023
@jseiser
Copy link

jseiser commented Jul 12, 2023

@raxod502-plaid

Any chance you can fix the conflicts, id really love to get this one merged in.

@raxod502-plaid
Copy link
Author

There's not much point in fixing merge conflicts until we get confirmation from a maintainer that they are interested in merging the PR.

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Aug 12, 2023
@kahirokunn
Copy link

i need this PR.

@github-actions github-actions bot removed the stale label Aug 13, 2023
@rarescosma
Copy link

We're hitting the same needs for our EKS setup at the moment: need to tag secondary subnets differently to tell karpenter to slowly start moving the nodes towards these subnets.

What are the plans for moving this PR forward?

@andrewmiskell
Copy link

I'm running into some of the same issues in our environment as well, needing to tag specific subnets differently due to different uses.

@netomin
Copy link

netomin commented Sep 13, 2023

Hi, hitting the same needs here, it would be great to have this PR to move forward ;)

@kahirokunn
Copy link

I think so!

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Oct 14, 2023
@kahirokunn
Copy link

Keep

@github-actions github-actions bot removed the stale label Oct 15, 2023
@raxod502-plaid
Copy link
Author

Added the ability to customize NAT EIP and NAT Gateway tags in the same way as the other resources

@joshuabaird
Copy link

@antonbabenko @bryantbiggs Any chance you could review this PR? This is a much requested feature. Thanks!

Copy link

github-actions bot commented Dec 7, 2023

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Dec 7, 2023
@kahirokunn
Copy link

keep

@github-actions github-actions bot removed the stale label Dec 8, 2023
Copy link

github-actions bot commented Jan 7, 2024

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jan 7, 2024
@raxod502-plaid
Copy link
Author

Not stale. @bryantbiggs would you be able to disable stale-bot on this PR to save people some time?

@manuelmazzuola
Copy link
Contributor

Hi, hitting the same needs here, please move this PR forward!!!!

@Smana
Copy link

Smana commented Apr 2, 2024

tf-controller plan output:

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration
and found no differences, so no changes are needed.

To apply this plan, please merge this pull request.

@jandersen-plaid
Copy link

@bryantbiggs what needs to be done here to get this PR merged? If the conflicts are resolved will that be enough?

@PLeS207
Copy link

PLeS207 commented May 15, 2024

Hello team, any updates on when this PR will be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet