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

Adds support for tags in aws provider spec #769

Merged
merged 4 commits into from
Oct 26, 2021
Merged

Conversation

suket22
Copy link
Contributor

@suket22 suket22 commented Oct 23, 2021

1. Issue, if available:
#642

2. Description of changes:

  • Adding tags to the provider which will be applied to all EC2 instances and launch templates being created by Karpenter.

3. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: TODO
  • No

Wasn't sure but I think this should be documented here? - https://github.com/awslabs/karpenter/blob/main/website/content/en/docs/cloud-providers/AWS.md ? I can edit this PR to include the doc changes if everything else looks good.

Testing done

  • Set up tags in the ProvisionerSpec and saw the tags get applied on both the EC2 instance and the launch templates.
  • Passed in an empty tag key and saw that validation work.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Oct 23, 2021

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 57914da

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6178651675f93500079c444c

@bwagner5
Copy link
Contributor

We should note in the docs that these tags will not be applied if a custom launch template is used.

ellistarn
ellistarn previously approved these changes Oct 26, 2021
KarpenterTagKeyFormat = "karpenter.sh/cluster/%s"
)

func ManagedTagsFor(clusterName string) map[string]string {
Copy link
Contributor

@ellistarn ellistarn Oct 26, 2021

Choose a reason for hiding this comment

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

Consider attaching this method to the AWS struct.

AWS.GetTags() // includes both API specified tags as well as our decorated ones.

It might simplify your code a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might also lean away from the term "managed" since it's such an overloaded concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke about this offline but I needed a way to separate out just our decorated tags. I wanted to compare the user's tags with our decorated ones prior to the merge so had to do it this way.

Agreed on the managed bit. I thought about calling it something like karpenterTags or controllerAppliedTags but nothing felt any nicer so just stuck with managed for now.

@suket22 suket22 merged commit 4e0a4b4 into aws:main Oct 26, 2021
@geoffcline
Copy link
Contributor

We should note in the docs that these tags will not be applied if a custom launch template is used.

Thanks for calling this out.

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

Successfully merging this pull request may close these issues.

5 participants