Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/*] Helm 3 backwards-compatibility for community charts #19008

Closed
18 tasks done
scottrigby opened this issue Nov 19, 2019 · 11 comments
Closed
18 tasks done

[stable/*] Helm 3 backwards-compatibility for community charts #19008

scottrigby opened this issue Nov 19, 2019 · 11 comments

Comments

@scottrigby
Copy link
Member

scottrigby commented Nov 19, 2019

Helm 3.0.0 has been released, with a 6 mo bug / 1 yr security deprecation timeline for helm v2.

This repo is intended for charts apiVersion v1 (not v2 charts), because of the helm/charts deprecation timeilne. This git repo - the source for Helm 2 stable and incubator repos - has a similar deprecation timeline as helm (follow details here: #16720).

However, since Helm v3 is the default version of helm now, the stable/incubator apiVersion v1 charts should run with Helm 3 as well as Helm 2.

CRDs

We'll need to support both crd-install helm hook for helm 2, as well as crds directory for helm 3 (https://helm.sh/docs/topics/charts/#custom-resource-definitions-crds). A quick grep for crd- turned up these charts. Let's look closer and refine this.

Note #18721 uses a neat glob trick I'd like to see the other charts emulate 👍

Namespace creation

Quick grep for kind: Namespace. Please comment to help revise (mitigate/add to) this list and I'll keep it updated redacted several. See comments below:

Horizontal Pod Autoscaler requires condition

Helm 3's 3-way strategic merge strategy is a huge improvement, and is the correct way to go. However it uncovered that one use case for the incorrect behavior in Helm 2 actually had a nice outcome: HPA-scaled replicas were untouched on helm upgrade, whereas Helm 3 now sets the replicas back to the values/template-defined number of replicas.

The problem was found by @naseemkullah and discussed with Helm maintainers in helm/helm#7090. The solution is:

All charts that offer an HPA, need to add a condition in the HPA managed deployment to omit the replica count of the deployment when rendering the deployment template, if the HPA is enabled.

Quick grep for kind: HorizontalPodAutoscaler. Please comment to help revise (mitigate/add to) this list and I'll keep it updated:

@scottrigby scottrigby added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Nov 19, 2019
@taylorsilva
Copy link
Contributor

The chart maintainers for stable/concourse have moved development to their own repo and chart repository: #18781

@daixiang0
Copy link
Collaborator

@scottrigby what is the namespace issue?

@ewbankkit
Copy link
Contributor

@scottrigby RE stable/kiam, the reference to kind: Namespace was in NOTES.txt, not in any actual templates.
Also, that chart has now been deprecated in this repo and moved to https://github.com/uswitch/kiam/tree/master/helm/kiam.
Thanks.

@scottrigby
Copy link
Member Author

scottrigby commented Dec 1, 2019

@taylorsilva thanks, removed stable/concourse from the list 👍

@ewbankkit Thanks, removed stable/kiam from the list 👍

@daixiang0 Thanks for the question. There isn't a specific issue with charts creating namespaces to accomplish application-speciifc goals. I grepped/listed these initially because namespace creation has changed in Helm 3 and I wanted to review charts that created their own. I removed the two above, and checked stable/drone (which seems fine). The only remaining chart in that list is stable/magic-namespace, which appears to be very Helm 2/Tiller-specific, so am unclear how we could update that chart for Helm 3.

We really need to look over these individually and more closely after the holidays 😄

@scottrigby
Copy link
Member Author

Removed stable/ark as it's deprecated (thanks @davidkarlsen for checking - I didn't have time when I initially grepped for the potential problem charts 😅 )

@kimxogus
Copy link
Contributor

kimxogus commented Dec 5, 2019

#19212 resolved this issue for stable/jaeger-operator

@daixiang0
Copy link
Collaborator

daixiang0 commented Dec 6, 2019

@scottrigby is there any action for those deprecated charts? Mark as deprecated?

For stable/magic-namespace, do not think it need to work in helm3 since it slove helm2 tiller issue indeed.

@scottrigby
Copy link
Member Author

@kimxogus re stable/jaeger-operator thanks and yes looks like an issue from that PR was fixed in #19402 👍

@scottrigby
Copy link
Member Author

@daixiang0 there is not an action for this yet. I have reached out to @krancour about stable/magic-namespace, and would like to give them time to reply before deprecating.

@krancour
Copy link
Contributor

@scottrigby I responded to you in slack, but I'm putting my response here for the public record as well:

the main point behind magic namespace was to make the tiller-per-namespace pattern easy to implement, to in turn make namespace-per-team easy to implement securely. since tiller is gone now in v3 (and took its vulnerabilities with it), i see this chart as being helm 2 specific and feel it should be deprecated on the same schedule as helm 2 itself, as it will then have outlived its usefulness.

@scottrigby
Copy link
Member Author

All of the Helm 3 issues with community charts identified so far have been resolved. If more issue categories, or individual chart issues, are identified please ping us on Slack and we'll re-open this. Closing for now. Thanks everyone 💯 🎉

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

No branches or pull requests

6 participants