Add tags to Auto Scaling Group and Fleet request #193
Conversation
|
Hi @haugenj Thanks for your PR! Code looks good to me. Happy with escalator adding tags, makes sense. However, would we be able to have this behind a feature gate (scoped under AWS), in order to preserve backwards compatibility? Particularly as this adds a new API call with a new policy rule, preferably defaulted to |
There was a problem hiding this comment.
Thanks for the PR.
Some minor tweaks of the implementation are required.
I'd also like to echo @Jacobious52's comment - this should really be behind a configuration option that defaults to false for the following reasons:
- Will keep backwards compatibility - the change as it is will break a deployment of Escalator if the user does not update the IAM policy at the same time as Escalator.
- Will prevent some cases where a tool such as Terraform might "fight" with escalator in removing the tag when the tool is run independently.
pkg/cloudprovider/aws/aws.go
Outdated
| @@ -92,6 +96,34 @@ func (c *CloudProvider) RegisterNodeGroups(groups ...cloudprovider.NodeGroupConf | |||
| continue | |||
| } | |||
|
|
|||
| // Search the asg for tagKey, then add it if it's not present | |||
There was a problem hiding this comment.
This tag addition functionality should be in a separate, discrete function
pkg/cloudprovider/aws/aws.go
Outdated
| break | ||
| } | ||
| } | ||
| if !hasTag { |
There was a problem hiding this comment.
I'd like to see this return early rather than nesting the addition of the tag
|
Thanks for taking a look at this so quickly! I don't believe the tags will break deployments if the IAM policy isn't updated; the error from the request is caught and logged but it doesn't force an early return or stop the nodegroup from being added. I've tested this on an EKS cluster with both setDesiredCapacity and createFleet scaling strategies and can confirm in both cases Escalator works as before. Perhaps logging with a 'warning' level is more appropriate than an 'error' level? @awprice can you elaborate on the potential Terraform/tag removing issue you mentioned? I only have a passing knowledge of how Terraform works so I've scanned through their docs a little to try and get some context. This page makes me think that the Escalator tags could be removed if Terraform updates the ASG, so there would be some potential back and forth of adding/removing the tags if there's a lot of Escalator deployments mixed with Terraform ASG updates. That doesn't seem like it would cause any real problems, but let me know if I'm misunderstanding this or missing something. |
You are right - this shouldn't break deployments if the IAM policy isn't updated as the error will be logged but not returned. The Terraform issue is more of a concern, as I can see the addition of the tag causing issues with ASGs managed by Terraform. We (Atlassian) do exactly this and will result in a tags being added/removed quite frequently from our own ASGs. One solution to this is to place the following Terraform config on the ASG resource, but to me fixing it in Terraform is not the right way to go about it. lifecycle {
ignore_tags = [tags]
}Is there another reason why this must be enabled by default and not easily disabled? |
|
Gotcha. They definitely don't need to be enabled by default if there are real problems, I just personally lean towards less user configuration whenever possible and was hoping it would work in this case. I'll work on a revision with the requested changes, thanks! |
There was a problem hiding this comment.
Sorry for the late review. Those changes and the default tagging being off lgtm. Thanks @haugenj
Tags are pieces of metadata customers can use to organize resources. They're particularly helpful to customers for breaking down costs, so they can see how much they're spending per tag. Functionally, they don't affect the performance of a resource in any way.
This change checks the ASGs when the nodegroups are created to see if the escalator tag is present on the ASG. If the tag isn't present, a request is made to add the tag. The tag will propagate to any instances launched within the ASG, so instances created with either setDesiredCapacity or CreateFleet are tagged.
Additionally, when CreateFleet is used the request itself is tagged.
The tag names are arbitrary, but I had them follow the same style as the ones cluster autoscaler uses