Deprecation of flags - mistmatch between application and helm chart

[jgournet]

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]

Thanks for bringing this to our attention!

In your cluster, is the problem that we entirely removed the deprecated flags from the Helm chart (e.g., here in deployment.yaml), rather than supporting both flags during the transition period?

Reading through the code now, I notice that we have proper fallback logic to handle CLI flags, both new and deprecated. But I don't see similar code for the Helm chart.

[jgournet]

Yes: the code does handle both new and old flags, but the helm chart just swapped from "checkASGTagBeforeDraining" to "checkTagBeforeDraining".
Since we had our helm chart values set as: checkASGTagBeforeDraining=false, when we upgraded, that variable being removed, helm used the default value of "checkTagBeforeDraining" which is true, thus breaking NTH completely.

To be honest:

• yes, we could/should just tag our ASG/EC2 and use the default value :) It would probably be a "better" setup.
• I don't know how hard it is to handle gracefully the deprecation of values in helm - might be worth checking
• thanks for looking into it ;)

[snay2]

Thanks for the feedback. Our release notes said both values would be supported, but that's not true for the helm chart, as you pointed out. I'm going to log an issue so we can put the deprecated values back in the helm chart, so that customers can use either one for as long as they're both available. In the future when we finally remove the deprecated value, we'll make note that it will be a breaking change. You've already done the upgrade work necessary, so that won't affect you, but hopefully it will spare other customers who haven't upgraded to v1.17.0 yet.

[snay2]

Issue open at #682.

[snay2]

@jgournet Kindly requesting your review on #686, which should fix this issue for folks who haven't upgraded to v1.17.* yet.