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

Don't touch addons if they are managed by Helm #2334

Closed
TBBle opened this issue Jun 15, 2020 · 9 comments
Closed

Don't touch addons if they are managed by Helm #2334

TBBle opened this issue Jun 15, 2020 · 9 comments
Labels
kind/feature New feature or request stale

Comments

@TBBle
Copy link
Contributor

TBBle commented Jun 15, 2020

Why do you want this feature?

I've been looking at replacing the existing Addons (CoreDNS, kube-proxy, and aws-vpc-cni) with Helm-managed versions of the same, since everything else on my cluster is Helm-managed now below the eksctl layer. Others are working to do the same thing.

One area of risk is that if I do that, and someone else runs eksctl upgrade addon ..., it will blat my configs, and leave the Helm metadata in an inconsistent state.

What feature/behavior/change do you want?

So as a preventative request, it'd be good if the addon upgrade code rejected doing upgrades if an affected resource (perhaps pick one per addon?) contains the app.kubernetes.io/managed-by well-known label, unless its value is eksctl.

Relatedly, the addons should have an app.kubernetes.io/managed-by=eksctl label added to their manifests, so that once you do an upgrade with eksctl, anyone looking at the resource knows eksctl was used. It might also scare-off other automated tools that would otherwise clobber the resources.

Slightly tricky for kube-proxy, since eksctl doesn't actually have a manifest for it, it's just patching the image tag, as recommended by EKS docs. Patching in the label in that case probably isn't too bad though, but it won't pick up related resources, such as the ConfigMap for kube-proxy.

@TBBle TBBle added the kind/feature New feature or request label Jun 15, 2020
@michaelbeaumont
Copy link
Contributor

I really like the idea of adding the label.
But do other tools actually respect these labels? I'm curious and can't really find a clear answer anywhere.

@TBBle
Copy link
Contributor Author

TBBle commented Jun 25, 2020

As far as I know, I've only seen app.kubernetes.io/managed-by=helm so far, which puts us at 100% of CNCF-graduated k8s Application Managers. ^_^

I haven't really used any other k8s package management tools. Poking at the code, the Kubernetes provider for Terraform doesn't seem to use the label, but will let you set it in your templates. I'm reasonably sure if you're using Helm to generate templates in Terraform, you won't get this label for Helm, since Helm didn't install the resources, just generated the manifest. But I could be wrong about that.

The new Kubernetes backed for Terraform is aware of this label, and uses the value "terraform". That's not for user resources though, but its own internal resources.

If more tools start using them, then more tools will see value in using them to avoid interfering with each other by surprise. Helm for example won't touch a resource that isn't marked as managed by Helm even when doing adoption of existing resources. (The adoption process currently involves marking resources as Helm-managed, so Helm can slurp them up)

If I used a tool and it blindly overwrote resources managed by another tool, then I'd consider reporting a bug (which is what I'm doing here, but before it happens). A tool that refuses to touch existing objects when told to create them is doing the right thing, for example.

I'm suggesting that (since we do adopt EKS-deployed manifests by dint of upgrading addons) we merely don't touch things that are clearly modified since EKS deployment to say "Some other tool is managing this".

If I were emporer of the universe, I'd have the EKS-deployed addons mark their manifests as 'managed by EKS', and then eksctl could replace that when upgrading the addon. However, that might give the false impression that EKS is also going to upgrade them in future, rather than pointing users at either eksctl or kubectl apply.

Anyway... if there's concerns about the value of this, or even if I'm suggesting the right thing, it might be worth reaching out to sig-apps who I believe owns the apps.kubernetes.io label-space.

@martina-if
Copy link
Contributor

There is also interest from the community to have eksctl manage clusters that were not necessarily created by it. The first step to do this is to allow the eksctl utils commands to operate non-eksctl clusters. Perhaps a mid-way solution is to check said label and warn the user, but if the user uses the --approve flag, eksctl will still upgrade the addon.

@TBBle
Copy link
Contributor Author

TBBle commented Jul 3, 2020

That sounds reasonable. My main concern is that if a Helm-deployed aws-vpc-cni is upgraded by a second person using eksctl utils --approve, then a third person who modifies it again with Helm is likely to unintentionally and silently roll back the changes made by the second person. Depending on the changes made, Helm might happily and silently merge the changes, but it does mean that helm get values would not be telling the whole truth of the state of the release.

I'd be surprised if the interest from the community is in using eksctl to manage things already managed by helm. I'd have expected that the interest was in adopting hand-rolled or EKS-deployed clusters, which wouldn't have any such app.kubernetes.io/managed-by label already present.

So as long as the complaint about the label reports the value, e.g., "This utility is marked as managed by helm, consider using that tool to upgrade the utility instead" then that will point users in the correct direction for how they can keep managing their system with the tool that's already in-use, or clean up the metadata used by that tool, e.g. helm unadopt if that existed, and then try the eksctl-based upgrade again.

Of course, this gets somewhat muddied if eksctl starts using Helm to manage the addons (#2245). Well, muddied or awsomed. Maybe both. ^_^

@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jan 18, 2021
@github-actions
Copy link
Contributor

This issue was closed because it has been stalled for 5 days with no activity.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Feb 24, 2021
@michaelbeaumont michaelbeaumont added priority/important-longterm Important over the long term, but may not be currently staffed and/or may require multiple releases and removed stale priority/backlog Not staffed at the moment. Help wanted. labels Feb 24, 2021
@aclevername aclevername removed the priority/important-longterm Important over the long term, but may not be currently staffed and/or may require multiple releases label Sep 21, 2021
@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Oct 22, 2021
@github-actions
Copy link
Contributor

This issue was closed because it has been stalled for 5 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request stale
Projects
None yet
Development

No branches or pull requests

4 participants