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: Add ability to tag network-interface using Launch Template #1563

Conversation

nikitacr7
Copy link
Contributor

PR o'clock

Description

The PR should resolve: #1361

Please explain the changes you made here and link to any relevant issues.

Checklist

@bryantbiggs
Copy link
Member

FYI - we'll need to bump the min AWS provider version to 3.53.0 for this change and we'll also want to update an example to show/test/validate its use

@antonbabenko
Copy link
Member

I guess there is no better reviewer than @lisfo4ka who added a similar feature a few days ago :)

@nikitacr7
Copy link
Contributor Author

@bryantbiggs @antonbabenko

Should I update AWS provider version in my PR or it will be updated by you? I've added example for Managed Node Group launch template.

Worker group launch template is already have example with tags

@nikitacr7
Copy link
Contributor Author

@antonbabenko What are the next steps?

@antonbabenko
Copy link
Member

Looks good to me, let's hear if @lisfo4ka agrees (hopefully during the next couple of days she replies).

@lisfo4ka
Copy link
Contributor

lisfo4ka commented Sep 3, 2021

Hi @nikitacr7 ,
Thanks for delivering the latest features from the AWS provider, it's a really useful pull request.

Some notes - at the moment, we can define tags on the global level via var.tags map and more specific via var.worker_groups_launch_template list of maps and the local.workers_group_default.

As I can see, your changes will add var.tags to the ENIs of the worker groups created from the launch template, which is great.
But you exclude all the tags which are defined in the var.worker_groups_launch_template and the local.workers_group_default, which looks like copy-pasting from instance tag_specification, but doesn't make sense here (more details #1095). Meanwhile, worker group instances inherit additional tags from the ASG with propagate_at_launch=true attribute. But unfortunately, there is no option to inherit ENI's tags from the ASG in the same way, so we'll prefer to do that by merging all the tags for ENI in the tag_specifications.

You can refer to the following PR #450 if you'd like to tag ENIs with var.tags only.
Or take into account this one #1397 to add more specific tags as well.

For the node_groups everything looks fine except indents, please fix it as well.

Also, I believe it's pretty important to include AWS provider version pinning exactly in the same commit with this change.

Please, fill free to ask any questions to get more details or let me know if I've misunderstood something.

FYI @antonbabenko

BR,
Olesia

workers_launch_template.tf Outdated Show resolved Hide resolved
@nikitacr7
Copy link
Contributor Author

Hi @lisfo4ka @antonbabenko
Thanks for reviewing my PR. I really appreciate it.

I've updated Terraform code to address all comments. Could you please review it once again?

Btw, AWS provider version has been already updated by @antonbabenko in master branch.

Best regards,
Nikita.

Copy link
Contributor

@lisfo4ka lisfo4ka left a comment

Choose a reason for hiding this comment

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

Hi @nikitacr7,

Thanks for your contribution. Now it looks fine to me. I've applied the changes and everything works properly.

@antonbabenko, approved from my side)

@antonbabenko antonbabenko merged commit 7512f17 into terraform-aws-modules:master Sep 6, 2021
@antonbabenko
Copy link
Member

Thanks @nikitacr7 and @lisfo4ka for the review!

v17.12.0 has been just released.

@nikitacr7 nikitacr7 deleted the feat/add-tags-to-network-interfaces branch September 6, 2021 09:30
@nikitacr7
Copy link
Contributor Author

@antonbabenko @lisfo4ka

I just want to let you know that changes implemented in the PR don't work due to internal issue at AWS side. AWS support has created issue in container roadmap: aws/containers-roadmap#1496

@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 Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Network interfaces are missing tags
5 participants