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

Add back deprecated flags to the helm chart: CHECK_ASG_TAG_BEFORE_DRAINING and MANAGED_ASG_TAG #682

Closed
snay2 opened this issue Aug 31, 2022 Discussed in #679 · 2 comments
Assignees
Labels
Priority: High This issue will be seen by most users Type: Bug Something isn't working

Comments

@snay2
Copy link
Contributor

snay2 commented Aug 31, 2022

Problem

Release v1.17.0 (chart version v0.19.0) deprecated two flags: CHECK_ASG_TAG_BEFORE_DRAINING and MANAGED_ASG_TAG, replacing them with CHECK_TAG_BEFORE_DRAINING and MANAGED_TAG. This change reflects the fact that we no longer call any ASG APIs when checking tags on an EC2 instance to determine whether NTH should manage it.

Our release notes stated

Release v1.17.0 supports both configs, but you'll see a warning if you use the deprecated name.

These two values are indeed still supported if invoked via CLI flags. However, they are not supported if invoked via the helm chart.

Solution

We need to add CHECK_ASG_TAG_BEFORE_DRAINING and MANAGED_ASG_TAG back to deployment.yaml and add checkASGTagBeforeDraining and managedAsgTag back to values.yaml, with appropriate comments to designate them as deprecated. If possible, we should also issue warnings, similar to how the CLI flags work, so that consumers are aware of the deprecation and can take steps to migrate before we eventually remove the deprecated flags in a future release.

Background

As discussed in #679:

Originally posted by jgournet August 25, 2022
Hi,

Just wanted to bring a pain point we just experienced:
Release 1.17.0 introduced this call out:

Deprecate two config values. Release v1.17.0 supports both configs, but you'll see a warning if you use the deprecated name. We may remove the deprecated configs altogether in a future release.
    Deprecate CheckASGTagBeforeDraining and replace it with CheckTagBeforeDraining

Great :) that's the right way to deprecate flags

But the helm chart changed to:
https://github.com/aws/aws-node-termination-handler/blob/main/config/helm/aws-node-termination-handler/values.yaml

# If true, check that the instance is tagged with "aws-node-termination-handler/managed" as the key before draining the node
checkTagBeforeDraining: true

=> node termination handler silently suddenly stopped working for us after upgrading. Lucky us for randomly catching it :)

So just mentioning it, in case someone can come up with a solution to slowly deprecate helm values too

@snay2 snay2 added Type: Bug Something isn't working Priority: High This issue will be seen by most users labels Aug 31, 2022
@snay2 snay2 changed the title Add back deprecated tags to the helm chart: CHECK_ASG_TAG_BEFORE_DRAINING and MANAGED_ASG_TAG Add back deprecated flags to the helm chart: CHECK_ASG_TAG_BEFORE_DRAINING and MANAGED_ASG_TAG Aug 31, 2022
@snay2 snay2 self-assigned this Sep 12, 2022
@snay2 snay2 added the Pending-Release Pending an NTH or eks-charts release label Sep 13, 2022
@snay2
Copy link
Contributor Author

snay2 commented Sep 13, 2022

Issue #687 will track future work for improving how we deprecate helm chart values, including some validation.

@snay2
Copy link
Contributor Author

snay2 commented Sep 15, 2022

Released this fix in app version v1.17.3 and Helm chart version v0.19.3.

@snay2 snay2 closed this as completed Sep 15, 2022
@snay2 snay2 removed the Pending-Release Pending an NTH or eks-charts release label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High This issue will be seen by most users Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant