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

Support specifying checksum(s) #1

Closed
daurnimator opened this issue Jun 29, 2020 · 11 comments
Closed

Support specifying checksum(s) #1

daurnimator opened this issue Jun 29, 2020 · 11 comments
Labels
enhancement New feature or request

Comments

@daurnimator
Copy link

To ensure that a chart doesn't change out from under me, I'd like to include checksums in my generator (the same checksum you'd see in requirements.lock)

@mgoltzsche
Copy link
Owner

Makes sense!
Would you like to create a PR?

@scjudd
Copy link
Contributor

scjudd commented Jul 13, 2020

Could most of helm.LoadChart be replaced with logic to copy a requirements.lock into the unpacked chart if specified, and then a call to man.Build()? From looking over this a bit today, it seems that LoadChart is doing a subset of the work of Helm's download manager's Build method.

Importantly, the Build method verifies requirements.lock.

I'd imagine adding a lockFile to the ChartRenderer spec that specifies a relative path to a requirements.lock, much like the valuesFiles does for values?

@mgoltzsche
Copy link
Owner

mgoltzsche commented Jul 13, 2020

helm.LoadChart is also downloading dependencies while man.Build() isn't if I am not mistaken but you're right: this can probably be solved more elegantly or rather without copying code that helm provides anyway. I think I simply copied it from ContainerSolutions/helm-convert.

Also this repo has a git_chart branch which provides additional but hacky features: Referencing helm charts using go-getters and excluding particular resources like in this example where I fetched linkerd's helm chart from its git repo because they don't publish it but ship their own CLI that uses it. However I did not merge the feature into master because this is something you usually don't want to do or rather using charts in a way they are not meant to be used is likely to cause problems during upgrades. Though this branch contains a few other small improvements as well.

@scjudd
Copy link
Contributor

scjudd commented Jul 13, 2020

Right, I didn't consider the initial download/unpack logic.

Well, I think I'll take a stab at this. :)

@mgoltzsche
Copy link
Owner

mgoltzsche commented Jul 13, 2020

@scjudd great!
To avoid potential conflicts and since I was using the go getter and resource filter features already I merged my very long-lived feature branch into master now - with updated dependencies and conditionally using the go getter feature when its feature flag is enabled. Please pull before proceeding.

This was referenced Jul 14, 2020
@mgoltzsche
Copy link
Owner

Solved by PR #5

@mgoltzsche mgoltzsche reopened this Oct 1, 2020
@mgoltzsche
Copy link
Owner

mgoltzsche commented Oct 1, 2020

I have to reopen this to clarify some requirements because I have to change some of the logic when upgrading to helm 3. The features for helm 2 done in PR #5 can be tested in release v0.9.2.

People (including me) were confused about the actual feature that should be implemented:

  • Supporting requirements.lock: As I understand this is needed to lock chart versions (particularly in case the requirements.yaml specifies version ranges). PR Add lockFile support #5 also introduced the field lockFile in the generator config so that you could point it to an existing requirements.lock
  • Verifying a chart: this can be done using the generator config fields verify (bool) and keyring.

The changes introduced in PR #5 unfortunately don't work this way with the helm 3 code anymore (since e.g. HashReq() is inside a helm-internal package; see PR #8, issue #6). Also the lockFile field doesn't provide good usability as long as the requirements.lock is not created by the plugin if it doesn't exist. Therefore, before I remove or reimplement the feature, I need to ask if you really need to specify a lock file for a remote chart without specifying a local umbrella chart or rather if you need the lockFile field within the generator config? (if it is just about version ranges you could as well specify that single version without range and don't need another requirements.lock for it, couldn't you?)
cc @daurnimator @scjudd @james-callahan

If you really need the lockFile field I'd generate a temporary chart and place the chart reference from the generator config file into the requirements.yaml (or Chart.yaml/dependencies in helm 3) file of that temporary chart and nest values correspondingly to make helm deal with the requirements.lock and copy it back and forth from/to lockFile so that the user doesn't need other tools and knowledge in order to create that lock file initially and the code remains maintainable and consistent with helm. Would that be helpful? Do you see problems with that? Am I missing anything? Opinions are welcome.

I am looking forward to receiving your feedback!

@daurnimator
Copy link
Author

@mgoltzsche what I want to do is lock to a particular version of a chart, so that what I audit and test locally (e.g. cert-manager 1.0.1) is the exact same thing that gets deployed onto my cluster.

verify and keyring only check that it was signed by a given author; but I don't trust the author to not push out a malicious update in future overwriting their previous chart.

I was under the impression that the checksums in requirements.lock would protect against this issue, but perhaps not?

@mgoltzsche
Copy link
Owner

mgoltzsche commented Nov 11, 2020

Using requirements.lock you can lock the version of a chart in case you're referring to it by version range but you cannot guarantee it doesn't change on the server meanwhile. As I understand the digest is to make the user rebuild/fetch a local chart's dependencies after the requirements.yaml was changed. You can see here that the digest is basically the hash of your requirements.yaml.
I don't think helm supports the feature you're looking for.
Actually you wouldn't want that because preventing redeployment would be bad for desaster recovery.

Instead I recommend to use kpt to render a helm chart into a git repo using a kpt function and commit and push the result (see here). Your CD pipeline (kpt live apply .) can always deploy the plain rendered manifests from the repo and you can reliably audit what gets deployed. Also your CD pipeline doesn't depend on rendering technology like helm or kustomize and availability of their repositories (kpt can also manage other (helm/kustomize/...) repositories within your repository as dependencies). There is also an official helm-template kpt function.
Also you can chain kpt functions so that you could run a kustomize function after a helm function to achieve the same as with kustomize and this plugin while managing functions as containers opposed to relying on particular kustomize plugin binary versions to be present within the CD pipeline image.

Soon I ll ship this project as kpt function container image as well since it supports a few more features than the official one. As I understand, since their code is maintained within the kustomize repo, kpt functions will soon also be available within kustomize directly and eventually replace kustomize' current plugin interface (which is still alpha) - the latter is just me guessing.

@mgoltzsche
Copy link
Owner

I am closing this since requirements.lock is supported for local charts but does not guarantee that a chart does not change on the server.

@daurnimator you can guarantee that manifests don't change after you have audited them using the kpt functionality for which I've also prepared an example - though in practice you would also commit example/kpt/cert-manager/static/generated-manifest.yaml with the repository to be sure it doesn't change after you audited it and avoid dependencies within the CD pipeline (I just didn't do that to avoid polluting the repo for the sake of an example).

@mgoltzsche
Copy link
Owner

mgoltzsche commented Mar 7, 2021

fyi: there is now an example that shows how to pull in charts from other git repositories as kpt dependencies before rendering them with khelm - though currently the resulting manifest is not functional yet.

@mgoltzsche mgoltzsche added the enhancement New feature or request label Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants