Kap 09 #701

Jean-Daniel · 2021-03-03T08:23:08Z

Implementation of kap-9: Bring your own Helm.

Proposed Changes

Replace helm cffi by Helm CLI invocation. This greatly simplify the helm logic and make adoption of new capabilities easier (just add a flag to the command line invocation)

This PR takes care of both helm_fetch and helm input. It completely removes the need for golang and cffi to build and install Kapitan.

It is based on and tested Helm 3 only as Helm 2 support ended the November 13, 2020.

ademariag · 2021-03-04T15:47:05Z

Thank you @Jean-Daniel, this is a great idea.

I know that @simu and @srueg are also interested in this and would also like to get their opinion.

Personally I like the idea of moving away from helm cffi, although we need to consider other angles.

srueg · 2021-03-05T08:06:46Z

That's great, thanks!
I don't think there's any downside to this (at least from our side) and it would solve a lot of problems indeed 👍

I'll do some tests and review this PR 🎉

srueg

LGTM, I ran tests with some of our setups and apart from the app.kubernetes.io/managed-by=Helm label change and some reordered RBAC, all compiled output stays the same 👍

srueg · 2021-03-08T14:16:48Z

kapitan/inputs/helm.py

+        # uses absolute path to make sure helm interpret it as a
+        # local dir and not a chart_name that it should download.
+        args.append(os.path.abspath(chart_dir))
+


Should we add --include-crds or make it configurable?

And probably --skip-tests?

Yes, this is both 2 reasonable flags to add by defaults.
Should we add skip_crds and include_tests configuration flags then.

The only drawback I see with this names, is that helm already has an unrelated --skip-crds flags.

I'd suggest to add include_crds and skip_tests just with a default of true to name them after the flags.

srueg · 2021-03-08T14:23:05Z

I tried to get rid of some of the debug output in this PR: helm/helm#8992
Maybe if you add your vote there?

Or should we try to filter the stdout of the helm cli to remove the wrote [...] debug logs?

Jean-Daniel · 2021-03-08T16:20:55Z

I tried to get rid of some of the debug output in this PR: helm/helm#8992
Maybe if you add your vote there?

Or should we try to filter the stdout of the helm cli to remove the wrote [...] debug logs?

As the Kapitan debug output already print the generated files names, we can probably disable helm stdout completely (stdout=subprocess.DEVNULL).

Or enable it only when debug log is enabled.

ramaro · 2021-03-12T10:08:22Z

Btw @sebradloff you might want to have a look at this 😄

sebradloff · 2021-03-14T01:36:37Z

kapitan/inputs/helm.py

+        args.append("--output-dir")
+        args.append(output_path)
+
+        if self.kube_version:


This is an amazing PR! I would recommend however to take helm_params as an input for all the additional flags like api-versions, validate, etc. I think it makes it a little more flexible.

I pushed a new version that supports most helm flags. There is just a bunch of flags that are disabled because they either affect the output, or are useless for templating and only relevant on install.

Note that helm template has a --release-name flag that conflicts with the release_name helm_params. This version now recommend to use name instead of release_name. To supports inventory that keep using release_name, the helm input check if the release_name value is a bool. If not, it prints a deprecation warning and keep using the release_name value as name.

In regards to the conflict of name and release_name, I think your solution make sense of printing the warning makes sense.

docs/compile.md

srueg · 2021-03-16T10:00:51Z

kapitan/inputs/helm.py

+
+            if param in HELM_DENIED_FLAGS:
+                logger.warning("helm flag '%s' is not supported and will be ignored.", param)
+                continue


Should we maybe abort here? Warning output might be missed and lead to unexpected behavior if the flags are just dropped.

sebradloff · 2021-03-16T12:21:35Z

Dockerfile

    && pip install .

+# Install Helm
+RUN apt-get install --no-install-recommends -y curl \
+    && curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 \


should we be pinning the version of helm3 that gets downloaded? I believe if I'm reading the get_helm.sh script correctly, we could technically set a DESIRED_VERSION to control the helm3 version being downloaded instead of defaulting to latest.

get_helm.sh supports --version flag. What would be the benefit of pinning the version ?

I feel like in general there are bug fixes here and there that happen with any software. I feel like since we directly depend on the helm binary and we have an "open" interface with helm_params it might be worth being methodical on upgrading versions. Just a thought, don't think it's required.

The get_helm.sh script is design to fetch only v3 version. So, all updates should be backward compatible.

Just like this image is using latest python3.7, git or gnupg binaries, I don't think it should cause issues using the latest helm 3 release.

ramaro · 2021-04-27T08:49:39Z

@Jean-Daniel @srueg @sebradloff are we happy with the state of this?

Jean-Daniel · 2021-04-27T08:59:53Z

I'm using it for some times now and haven't encounter any issue yet. So I'm fine with it.

ccremer · 2021-04-27T10:04:23Z

kapitan/helm_cli.py

+    try:
+        logger.debug("launching helm with arguments: %s", args)
+        res = subprocess.run(
+            args=["helm"] + args, stderr=PIPE, stdout=stdout or (PIPE if verbose else DEVNULL)


I think it would make sense if we can somehow configure which helm binary to use instead of relying on a hardcoded value. Some users might have a dual-helm setup with helm v2 and v3 and their helm binary might be called helm3 or so. Maybe try to use the binary in an env var like "HELM_BIN" (or whatever appropriate) and default to helm if not defined?

e.g. https://github.com/roboll/helmfile allows to set the "helmBinary" config.

For that, I think it would be best to support a helm_path option along the other ones

Both use cases are valid IMHO. Some people may need to set the path globally (if they want to use a specific helm for everything), and some will need to set it per compile action (to compile a helm2 chart for instance).

yeah, sure. being able to specify it globally or per compilation should give enough flexibility

sebradloff · 2021-04-27T11:48:53Z

@Jean-Daniel @srueg @sebradloff are we happy with the state of this?

I think this PR is great. Helm2 binary as an option sounds like a good idea, since some people might be stuck with old open source charts.

zstoth · 2021-06-09T08:07:04Z

@Jean-Daniel is anything blocking this?

ramaro · 2021-06-18T09:22:54Z

Hey @Jean-Daniel do you wanna master rebase this? It should fix the failed binary tests as we discontinued the binary in #740

An actually difference with the cffi implementation is that if something goes wrong, it raises a HelmFetchingError instead of silently failing. It is consistent with Git and Http behaviour which raise errors too if the fetch operation fails.

It mostly matches the cffi implementation with one difference: - release_name is required when using Helm CLI. This implementation uses —generate-name flag if neither release_name nor template_name are specified, which may cause different output. This commit also add `validate` flag support, which is required to compile some templates that rely on cluster Capabilities (cilium with ServiceMonitor for instance).

- reduce denied flags list to the bare minimum. No need to raise error when using an irrelevant flag like atomic. - raise an error if flags contains a '-' instead of '_'. Allowing both may cause subtle bugs if both syntaxes are used in the same target for the same flag.

Co-authored-by: Simon Rüegg <[email protected]>

When specified, run helm template without specifying --output-dir and pipe the result into a single file.

All helm invocation will try to use the value set in the env var KAPITAN_HELM_PATH. Fetch and compile action takes a helm_path parameter to specify per target helm binary.

Jean-Daniel · 2021-06-18T11:28:07Z

Hey @Jean-Daniel do you wanna master rebase this? It should fix the failed binary tests as we discontinued the binary in #740

Done. Note that #741 erroneously remove the build_helm_binding target in the Makefile, which breaks helm support (until this PR is merged).

ademariag · 2021-07-06T12:54:07Z

I did a quick test with my repo, and this pretty much doubles the compile speed (for prometeus chart) in my case

before

time kapitan compile -t intranet-cluster-infra
Compiled intranet-cluster-infra (8.97s)
real    0m12.099s
user    0m11.563s
sys     0m1.508s

after

time kapitan compile -t intranet-cluster-infra
Compiled intranet-cluster-infra (3.41s)
real    0m6.199s
user    0m8.038s
sys     0m1.285s

ademariag · 2021-07-06T12:59:01Z

A tiny issue I have found, is that the produced files have a different formatting (which we already experienced in #360) so that replacing the old helm support for the new one will make the review process tedious (because we will have lots of formatting diffs)

Do you know whether it is possible to still prettify the helm output even with this new input type?

ramaro · 2021-07-12T20:44:07Z

We can do the same we've done before here https://github.com/kapicorp/kapitan/pull/385/files @ademariag

ademariag mentioned this pull request Mar 4, 2021

Allow helm binding to be compiled with go 1.16 #702

Merged

ademariag requested a review from ramaro March 4, 2021 15:42

srueg mentioned this pull request Mar 8, 2021

Explicitly set multiprocessing method projectsyn/commodore#288

Closed

1 task

srueg approved these changes Mar 8, 2021

View reviewed changes

srueg approved these changes Mar 9, 2021

View reviewed changes

srueg mentioned this pull request Mar 12, 2021

Update to Helm 3 #648

Closed

sebradloff reviewed Mar 14, 2021

View reviewed changes

srueg suggested changes Mar 16, 2021

View reviewed changes

sebradloff reviewed Mar 16, 2021

View reviewed changes

ccremer reviewed Apr 27, 2021

View reviewed changes

simu mentioned this pull request May 7, 2021

Can't compile the helm chart version 11+ projectsyn/component-keycloak#8

Closed

Jean-Daniel Dupas and others added 9 commits June 18, 2021 13:22

[kap-9] Rewrite helm_fetch to use helm_cli.

b3ee085

An actually difference with the cffi implementation is that if something goes wrong, it raises a HelmFetchingError instead of silently failing. It is consistent with Git and Http behaviour which raise errors too if the fetch operation fails.

Ignore helm stdout.

be31e1c

[helm] add include_crds and skip_tests options.

f77b11e

[helm] log helm commands arguments.

f1f1344

[helm] Fix usage of undefined var.

af54f7f

Support arbitrary flags for helm input.

1f825a6

Typo in doc

f410612

Co-authored-by: Simon Rüegg <[email protected]>

Jean-Daniel added 2 commits June 18, 2021 13:22

Add output_file helm params.

37597b4

When specified, run helm template without specifying --output-dir and pipe the result into a single file.

Add 2 ways to specify custom helm binary.

a675509

All helm invocation will try to use the value set in the env var KAPITAN_HELM_PATH. Fetch and compile action takes a helm_path parameter to specify per target helm binary.

Merge branch 'master' into kap-09

87eb9c6

ramaro approved these changes Aug 1, 2021

View reviewed changes

ramaro merged commit 103ccf3 into kapicorp:master Aug 1, 2021

ramaro mentioned this pull request Aug 1, 2021

[RFC] Python packages for helm binaries #631

Closed

ofen mentioned this pull request Aug 12, 2021

null at the begining of each yaml file rendered via helm binding integration #735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kap 09 #701

Kap 09 #701

Jean-Daniel commented Mar 3, 2021

ademariag commented Mar 4, 2021

srueg commented Mar 5, 2021

srueg left a comment

srueg Mar 8, 2021

srueg Mar 8, 2021

Jean-Daniel Mar 8, 2021

srueg Mar 9, 2021

srueg commented Mar 8, 2021 •

edited

Loading

Jean-Daniel commented Mar 8, 2021

ramaro commented Mar 12, 2021

sebradloff Mar 14, 2021

Jean-Daniel Mar 16, 2021

sebradloff Mar 16, 2021

srueg Mar 16, 2021

sebradloff Mar 16, 2021 •

edited

Loading

Jean-Daniel Mar 16, 2021

sebradloff Mar 18, 2021

Jean-Daniel Mar 18, 2021

ramaro commented Apr 27, 2021

Jean-Daniel commented Apr 27, 2021

ccremer Apr 27, 2021

ramaro Apr 27, 2021

Jean-Daniel Apr 27, 2021

ccremer May 14, 2021

sebradloff commented Apr 27, 2021

zstoth commented Jun 9, 2021

ramaro commented Jun 18, 2021

Jean-Daniel commented Jun 18, 2021 •

edited

Loading

ademariag commented Jul 6, 2021

ademariag commented Jul 6, 2021

ramaro commented Jul 12, 2021

Kap 09 #701

Kap 09 #701

Conversation

Jean-Daniel commented Mar 3, 2021

Proposed Changes

ademariag commented Mar 4, 2021

srueg commented Mar 5, 2021

srueg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srueg commented Mar 8, 2021 • edited Loading

Jean-Daniel commented Mar 8, 2021

ramaro commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebradloff Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaro commented Apr 27, 2021

Jean-Daniel commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebradloff commented Apr 27, 2021

zstoth commented Jun 9, 2021

ramaro commented Jun 18, 2021

Jean-Daniel commented Jun 18, 2021 • edited Loading

ademariag commented Jul 6, 2021

before

after

ademariag commented Jul 6, 2021

ramaro commented Jul 12, 2021

srueg commented Mar 8, 2021 •

edited

Loading

sebradloff Mar 16, 2021 •

edited

Loading

Jean-Daniel commented Jun 18, 2021 •

edited

Loading