charts: Add helm charts for Inspektor Gadget #1707

mqasimsarfraz · 2023-06-02T15:08:13Z

This PR introduces helm charts for Inspektor Gadget. The idea is to have a single source of truth for deploy command as helm charts to avoid running into issues in future. The current approach will reuse the CRDs in both installation workflows. In case of manifest, helm will be source of truth and helm generated manifest is used as base (deploy.yaml) for deploy command. In terms of versioning, I want to start with 1:1 versioning between chart version and app version since it is simple and recommend in the start and app version will used for container image tag unless specified.

For publishing we will setup a index like here using helm repo index ... but pointing to charts in our release assets. I want this change to go in and then automate that step.

Related: #414

Testing done

$ make minikube-start
$ make -C charts APP_VERSION=latest install
NAME: gadget
LAST DEPLOYED: Fri Jun  2 17:31:35 2023
NAMESPACE: gadget
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Inspektor Gadget latest installed successfully!

For usage details, visit https://www.inspektor-gadget.io
$ kubectl get pods -n gadget
NAME           READY   STATUS    RESTARTS   AGE
gadget-nsrx2   1/1     Running   0          2m30s

You can verify the charts using:

$ make -C charts [build/package]
$ ls -la charts/bin/gadget
...
-rw-rw-r-- 1 qasim qasim  529 Jun  2 17:04 Chart.yaml
drwxrwxr-x 2 qasim qasim 4096 Jun  2 17:04 crds
-rw-rw-r-- 1 qasim qasim  358 Jun  2 17:04 .helmignore
-rw-r--r-- 1 root  root  2887 Jun  2 17:04 README.md
-rw-rw-r-- 1 qasim qasim 1549 Jun  2 17:04 README.md.gotmpl
drwxrwxr-x 2 qasim qasim 4096 Jun  2 17:04 templates
-rw-rw-r-- 1 qasim qasim 1173 Jun  2 17:04 values.yaml

Ensuring no regression in deploy.yaml

# deploy IG on minikube via 'main'
$ git checkout main
$ make kubectl-gadget
$ ./kubectl-gadget deploy
$ git checkout qasim/helm-charts
$ cat pkg/resources/manifests/*.yaml | kubectl diff -f -
...

Todo

Add a test in CI that generated manifest has no changes.
Add a step in release job to publish helm package.
Add an action to publish charts index to charts repository (possibly inspektor-gadget/charts). [Post Merge]
Verify helm charts are available on https://artifacthub.io with generated documentation [Post Merge]

invidian

I'm not very familiar with charts publishing process, so I did not review this part of the PR, but manifests etc looks good. Left some questions/suggestions.

invidian · 2023-06-23T13:37:10Z

charts/Makefile

+	sed -i "s/%VERSION%/$(CHART_VERSION)/g" $(CHART_DIR)/Chart.yaml
+	sed -i "s/%APP_VERSION%/$(IMAGE_TAG)/g" $(CHART_DIR)/Chart.yaml
+	@echo "Preparing docs"
+	docker run --user $(shell id -u):$(shell id -g) -v $(shell pwd)/$(CHART_DIR):/helm-docs jnorwood/helm-docs:v1.11.0 -s file


jnorwood/helm-docs:v1.11.0 could also be in a variable in case an override is needed.

invidian · 2023-06-23T13:38:33Z

charts/Makefile

+	@echo "Charts available at: $(CHART_DIR)"
+
+install: build
+	$(HELM) install gadget $(CHART_DIR) --namespace gadget --create-namespace


Maybe namespace could be a variable as well? Is it supported to deploy IG in other namespaces?

IG daemonset itself might be fine in different namespace but the client kubectl-gadget expects the pod to be in gadget namespace so perhaps we keep this way. @mauriciovasquezbernal do you have any thoughts on this?

If that's the case, perhaps all resources should have this namespace hardcoded then? As installing in other namespace will result in broken scenario.

all resources should have this namespace hardcoded then?

Good Point, I refactored the templates to use hardcored "gadget" namespace.

invidian · 2023-06-23T13:40:58Z

charts/gadget/templates/namespace.yaml

@@ -0,0 +1,6 @@
+{{ if .Values.createNamespace }}


I think this manifest is not necessary. IIRC Helm will complain if target namespace does not exist and --create-namespace flag is not specified.

I removed it. This was a workaround for helm template not adding namespace in the generated manifest :( but makes sense to move this out of charts.

invidian · 2023-06-23T13:44:02Z

charts/Makefile

+	@echo "Charts available at: $(CHART_DIR)"
+
+install: build
+	$(HELM) install gadget $(CHART_DIR) --namespace gadget --create-namespace


Maybe it would make sense to use helm upgrade --install + kubectl apply for CRDs (as helm upgrade does not update CRDs) to be able to iterate on chart when developing it? install is not idempotent.

Great. Thanks for a suggestion. The flow is much better now!

mqasimsarfraz

@invidian Thank you so much for the review!

mqasimsarfraz · 2023-06-26T13:23:43Z

charts/Makefile

+	sed -i "s/%VERSION%/$(CHART_VERSION)/g" $(CHART_DIR)/Chart.yaml
+	sed -i "s/%APP_VERSION%/$(IMAGE_TAG)/g" $(CHART_DIR)/Chart.yaml
+	@echo "Preparing docs"
+	docker run --user $(shell id -u):$(shell id -g) -v $(shell pwd)/$(CHART_DIR):/helm-docs jnorwood/helm-docs:v1.11.0 -s file


mqasimsarfraz · 2023-06-26T13:24:09Z

charts/Makefile

+	@echo "Charts available at: $(CHART_DIR)"
+
+install: build
+	$(HELM) install gadget $(CHART_DIR) --namespace gadget --create-namespace


Great. Thanks for a suggestion. The flow is much better now!

mqasimsarfraz · 2023-06-26T13:28:23Z

charts/Makefile

+	@echo "Charts available at: $(CHART_DIR)"
+
+install: build
+	$(HELM) install gadget $(CHART_DIR) --namespace gadget --create-namespace


IG daemonset itself might be fine in different namespace but the client kubectl-gadget expects the pod to be in gadget namespace so perhaps we keep this way. @mauriciovasquezbernal do you have any thoughts on this?

mqasimsarfraz · 2023-06-26T13:29:28Z

charts/gadget/templates/namespace.yaml

@@ -0,0 +1,6 @@
+{{ if .Values.createNamespace }}


I removed it. This was a workaround for helm template not adding namespace in the generated manifest :( but makes sense to move this out of charts.

invidian · 2023-06-30T10:34:30Z

charts/gadget/templates/_helpers.tpl

+{{/*
+Namespace used by all resources.
+*/}}
+{{- define "gadget.namespace" -}}


Great idea for still using a template for the namespace in case this changes in the future!

eiffel-fl

I watched your presentation about it and it seems cool.
I nonetheless need to gain more knowledge about helm to test it and then give an approval.

eiffel-fl · 2023-06-30T13:19:51Z

charts/Makefile

+# also helm upgrade does not update CRDs so we need to apply them separately
+install: build
+	$(HELM) upgrade --install gadget $(CHART_DIR) --namespace gadget --create-namespace
+	kubectl apply -f $(CHART_DIR)/crds


Suggested change

kubectl apply -f $(CHART_DIR)/crds

kubectl apply -f $(CHART_DIR)/crds/*

?
Same for below.

charts/README.md

eiffel-fl · 2023-06-30T13:25:45Z

pkg/resources/manifests/deploy.yaml

-kind: Role
+kind: ClusterRole


Is it OK to change the order of the stuff there?
Wouldn't it be possible for the deploy to fail if B is defined before A while needing A?

In general, It might be problem like if namespace is moved in the end we will run into errors. But here we only swap ClusterRole and Role which aren't related so should be fine. Helm decide to order them like that as mentioned in this section.

mauriciovasquezbernal

Some comments. I'm a bit worried about the handling of the latest version.

I was on this branch, I did:

$ make -C charts/ install 
...
Inspektor Gadget 0.17.0 installed successfully

That was surprising as I was expecting the current version to be installed. Is that the expected version to install the last release?

mauriciovasquezbernal · 2023-06-30T13:18:23Z

charts/Makefile

+OUTPUT_DIR ?= $(BUILD_DIR)
+CHART_DIR := $(BUILD_DIR)/gadget
+
+IMAGE_TAG := $(shell git describe --tags --abbrev=0 --match "v*")


We already have a script to get the tag: tools/image-tag. It has some additional logic to take the name of the branch into consideration. Should we use it here too?

Btw, if I'm on main and I run make template it'll use the latest version instead of latest as tag. Is that the desired behaviour?

Thanks for the feedback. I wasn't too sure about it. I was trying to have 1:1 versioning between chart version and appVersion and that is making it confusing. Chart Version is suppose to be semver and hence tools/image-tag won't work for it so I used on my own way of handling it. But I agree we should use tools/image-tag for appVersion this should make the behavior exactly like deploy command.

How do other folks handle the chart version of the "latest" app? Or they aren't charts available for "latest" apps?

Most of the charts are living outside so they perhaps don't care about latest. Having said that, one can have dev versions (1.0.0-dev) for both chart/app and use --set image.tag=$IMAGE_TAG (e.g cilium) installation and worry about versioning only for releases. What we did now should work fine but in case it gets confusing we can rework it in future?

mauriciovasquezbernal · 2023-06-30T13:47:12Z

charts/Makefile

+	$(HELM) uninstall gadget --namespace gadget
+	kubectl delete -f $(CHART_DIR)/crds


Shouldn't it be done the other way around? This is how we do this in the deploy command

inspektor-gadget/cmd/kubectl-gadget/undeploy.go

Line 136 in a627182

// 2. remove crd

Also, what happens to the traces CR?: they need the operator to be running to be able to remove it, otherwise the finalizer will never let them be removed. In the undeploy command we have some logic to handle this

inspektor-gadget/cmd/kubectl-gadget/undeploy.go

Line 88 in a627182

// 1. remove traces

I agree we can just simply say it's the user's responsibility to delete them before if this is difficult to handle.

charts/gadget/templates/daemonset.yaml

charts/gadget/values.yaml

Makefile

mauriciovasquezbernal · 2023-06-30T14:27:54Z

charts/Makefile

+	@echo "Charts available at: $(CHART_DIR)"
+
+# install uses 'helm upgrade --install' to make chart installation idempotent,
+# also helm upgrade does not update CRDs so we need to apply them separately


:( this is too bad as users will have to handle additional commands manually.

According to helm/helm#8668 (comment):

If you're chart doesn't install custom resources just the CRDs... you can put the CRDs right into the templates directory.

That seems to be our case. Perhaps we can investigate more to see if we can improve this. Anyway, I'll agree this an improvement and not a blocker for this PR.

mauriciovasquezbernal

I think it's ready to go! Thanks for handling this!

mauriciovasquezbernal · 2023-06-30T17:59:26Z

charts/Makefile

+CHART_DIR := $(BUILD_DIR)/gadget
+
+# CHART_VERSION must be a valid SemVer string so strip the leading 'v'
+CHART_VERSION ?= $(shell git describe --tags --always | sed 's/^v//')


Would it make sense to use something like 0.9999.0 to avoid any kind of confusion between app and chart version? (Don't hesitate to say not if it doesn't make sense at all)

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz · 2023-07-03T08:26:25Z

Thanks for all the reviews! :)

mqasimsarfraz force-pushed the qasim/helm-charts branch 5 times, most recently from 4541472 to c4a252a Compare June 23, 2023 10:10

mqasimsarfraz marked this pull request as ready for review June 23, 2023 13:19

invidian reviewed Jun 23, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/helm-charts branch from c4a252a to 5107b97 Compare June 27, 2023 12:09

mqasimsarfraz commented Jun 27, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/helm-charts branch 5 times, most recently from 4dfc944 to 423bd4c Compare June 30, 2023 10:32

invidian reviewed Jun 30, 2023

View reviewed changes

eiffel-fl reviewed Jun 30, 2023

View reviewed changes

mauriciovasquezbernal reviewed Jun 30, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/helm-charts branch 3 times, most recently from 56e3999 to e690d5c Compare June 30, 2023 17:04

mauriciovasquezbernal approved these changes Jun 30, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/helm-charts branch from e690d5c to 5f62055 Compare July 3, 2023 06:55

mqasimsarfraz added 3 commits July 3, 2023 09:00

charts: Add helm charts for gadget

deeee9e

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

pkg/resources: Use generated manifests

c84fb61

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

ci: Publish helm charts

e341492

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz force-pushed the qasim/helm-charts branch from 5f62055 to e341492 Compare July 3, 2023 07:00

mqasimsarfraz merged commit 19c88f6 into main Jul 3, 2023
46 checks passed

mqasimsarfraz deleted the qasim/helm-charts branch July 3, 2023 08:26

mqasimsarfraz mentioned this pull request Jul 4, 2023

[RFE] Improve CRD handling of Helm Charts #1814

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

charts: Add helm charts for Inspektor Gadget #1707

charts: Add helm charts for Inspektor Gadget #1707

mqasimsarfraz commented Jun 2, 2023 •

edited

Loading

invidian left a comment

invidian Jun 23, 2023

mqasimsarfraz Jun 26, 2023

invidian Jun 23, 2023

mqasimsarfraz Jun 26, 2023

invidian Jun 27, 2023

mqasimsarfraz Jun 27, 2023

invidian Jun 23, 2023

mqasimsarfraz Jun 26, 2023

invidian Jun 23, 2023

mqasimsarfraz Jun 26, 2023

mqasimsarfraz left a comment

mqasimsarfraz Jun 26, 2023

mqasimsarfraz Jun 26, 2023

mqasimsarfraz Jun 26, 2023

mqasimsarfraz Jun 26, 2023

invidian Jun 30, 2023

eiffel-fl left a comment

eiffel-fl Jun 30, 2023

eiffel-fl Jun 30, 2023

mqasimsarfraz Jun 30, 2023

mauriciovasquezbernal left a comment

mauriciovasquezbernal Jun 30, 2023

mqasimsarfraz Jun 30, 2023

mauriciovasquezbernal Jun 30, 2023

mqasimsarfraz Jul 3, 2023

mauriciovasquezbernal Jun 30, 2023

mauriciovasquezbernal Jun 30, 2023

mauriciovasquezbernal left a comment

mauriciovasquezbernal Jun 30, 2023

mqasimsarfraz Jul 3, 2023

mqasimsarfraz commented Jul 3, 2023

	kubectl apply -f $(CHART_DIR)/crds
	kubectl apply -f $(CHART_DIR)/crds/*

		$(HELM) uninstall gadget --namespace gadget
		kubectl delete -f $(CHART_DIR)/crds

		kind: Role
		kind: ClusterRole

charts: Add helm charts for Inspektor Gadget #1707

charts: Add helm charts for Inspektor Gadget #1707

Conversation

mqasimsarfraz commented Jun 2, 2023 • edited Loading

Testing done

Ensuring no regression in deploy.yaml

Todo

invidian 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

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

Choose a reason for hiding this comment

mqasimsarfraz 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

Choose a reason for hiding this comment

eiffel-fl 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

mauriciovasquezbernal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqasimsarfraz commented Jul 3, 2023

mqasimsarfraz commented Jun 2, 2023 •

edited

Loading