Templatize helm template image references #20066

joestringer · 2022-06-02T21:34:06Z

From the main commit:

Move all of the declarations of the image repositories and tags into
install/kubernetes/Makefile.values, then templatize the values file so
that we can pick up these env variables using envsubst. Then we can get
rid of the big ugly 'sed' hack in the Makefile which is hard to maintain.

Broadly this PR moves the install/kubernetes/cilium/values.yaml to install/kubernetes/cilium/values.yaml.tmpl as the source of truth. This new values file template has variables wherever the docker repositories and tags are.

Inside the Makefiles, we declare the repos/tags as variables, then when we run envsubst against the install/kubernetes/cilium/values.yaml.tmpl, it substitutes the variables into the template and generates the final output install/kubernetes/cilium/values.yaml file.

This looks like a big change (~2KLoC), but most of it is just making a copy of install/kubernetes/values.yaml in order to templatize it. Review commit-by-commit to see incremental changes.

This PR also fixes the default helm in the master branch to point towards quay.io/cilium/cilium-ci:latest so that you can install directly from the repo. When we branch for the next release, we will update the repo and tag to point to the branch instead and then the helm charts should work for that target development branch.

You can also override these default settings by making your own copy of install/kubernetes/Makefile.values and then running this with MAKEFILE_VALUES=/path/to/my/values.mk make -C install/kubernetes.

joestringer · 2022-06-02T22:05:56Z

install/kubernetes/Makefile.digests

-OPERATOR_ALIBABACLOUD_DIGEST := ""
-OPERATOR_GENERIC_DIGEST := ""
-OPERATOR_DIGEST := ""
+export CILIUM_DIGEST := ""


The alternative to all this exporting is to just blanket export all variables to subshells via .EXPORT_ALL_VARIABLES as a Special Target, but I wasn't sure if that might cause other issues since I don't know if that's scoped only to this Makefile or whether it could end up exporting other vars.

michi-covalent

lgtm just one question about phony target. this is much cleaner 🚢

install/kubernetes/Makefile

aditighag

Thanks for tidying up the Makefiles, and fixing the defaults with "-ci" image tags.

install/kubernetes/Makefile.values

aditighag · 2022-06-02T23:00:00Z

install/kubernetes/cilium/values.yaml

+# File generated by install/kubernetes/Makefile; DO NOT EDIT.
+# This file is based on install/kubernetes/cilium/values.yaml.tmpl.


How was this file generated? Helm?

It's generated using envsubst. Maybe I could have described this better in the PR description. Broadly what this PR does is it adds a bunch of exported variables in install/kubernetes/Makefile.values, then runs envsubst which takes in the values.yaml.tmpl on stdin plus the environment variables, then substitutes all of the variables in the template and writes out to this file.

^ I expanded the PR description to help explain this, hopefully that makes sense now.

joestringer · 2022-06-02T23:18:37Z

install/kubernetes/Makefile

+	$(ECHO_GEN)$@
+	echo -e "# File generated by $(RELATIVE_DIR)/Makefile; DO NOT EDIT." > $@
+	echo -e "# This file is based on $(RELATIVE_DIR)/cilium/values.yaml.tmpl.\n" >> $@
+	envsubst < $< >> $@


This is the magic line, basically take $< (the template file, first dependency in the list) as stdin to envsubst, which also takes the exported environment variables as inputs, then redirect the output to $@ (the target values.yaml file).

kaworu

Thanks @joestringer! Couple of comments but overall LGTM.

On the CI side, test-smoke.yaml should detect changes to values.yaml.tmpl without a corresponding values.yaml update:

cilium/.github/workflows/tests-smoke.yaml

Lines 96 to 99 in 9580ea3

    
                 - name: Run helm-charts 
        
                   run: | 
        
                     make -C install/kubernetes 
        
                     test -z "$(git status --porcelain)" || (echo "please run 'make -C install/kubernetes' and submit your changes"; exit 1)

Looking at this workflow, "Run helm-docs" seems extraneous because "Run helm-charts" execute make -C install/kubernetes and the default all target has the docs target as dependency. As this is kind of related to this PR, would you mind adding a commit to fix this (either remove "Run helm-docs" or make "Run helm-charts" execute the update-versions target)? Happy to open another PR if you prefer.

install/kubernetes/Makefile

joestringer · 2022-06-03T18:10:11Z

Addressed @kaworu's feedback, mainly with two new commits on top. Should reduce the amount of duplicate work on the tests-smoke.yaml GHA linting side, and properly track dependencies as well.

pchaigno

I have only reviewed the changes falling under my codeowners, but LGTM.

Default these helm templates to install the latest version from the tip of master, for easy development. Signed-off-by: Joe Stringer <joe@cilium.io>

Signed-off-by: Joe Stringer <joe@cilium.io>

Split up the variables used here for better readability, and add the V=0 quiet line to indicate progress in the way the other targets do. Signed-off-by: Joe Stringer <joe@cilium.io>

The 'docs' target under install/kubernetes is already run by the 'helm-charts' step via the 'all' make target. Given some of the recent re-organizing, we should probably just add linting to the overall steps in the makefile and run all of the targets. This should avoid running these targets twice: * 2x Run cilium/values.yaml generation (via lint, all targets) * 2x Run docs (via docs, all targets) Suggested-by: Alexandre Perrin <alex@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io>

Previously this script wouldn't work when invoked on the master branch via `make -C install/kubernetes check-docker-images`, because the script expected some images to be present on dockerhub when we don't keep them up to date there (startup-script, cilium-etcd-operator), and also because the tree specifies the image tag as "latest" which are only published on the quay.io/cilium/*-ci repositories. Teach the script to pick up the correct set of image dependencies to handle these cases. Signed-off-by: Joe Stringer <joe@cilium.io>

Following up on #20066, templatize preflight and clustermesh-apiserver repos. Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

joestringer requested a review from a team June 2, 2022 21:34

joestringer requested review from a team as code owners June 2, 2022 21:34

joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Jun 2, 2022

joestringer requested review from sayboras and aditighag June 2, 2022 21:34

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 2, 2022

joestringer requested a review from pchaigno June 2, 2022 21:34

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 2, 2022

joestringer requested a review from michi-covalent June 2, 2022 21:36

joestringer force-pushed the submit/improve-version-tracking branch 2 times, most recently from 90ce1c7 to ba5a725 Compare June 2, 2022 21:56

joestringer commented Jun 2, 2022

View reviewed changes

michi-covalent approved these changes Jun 2, 2022

View reviewed changes

install/kubernetes/Makefile Show resolved Hide resolved

joestringer force-pushed the submit/improve-version-tracking branch from ba5a725 to 8976587 Compare June 2, 2022 22:55

aditighag reviewed Jun 2, 2022

View reviewed changes

joestringer commented Jun 2, 2022

View reviewed changes

joestringer requested a review from a team as a code owner June 2, 2022 23:23

joestringer requested a review from qmonnet June 2, 2022 23:23

joestringer force-pushed the submit/improve-version-tracking branch from 0c0bba3 to bed5b6b Compare June 2, 2022 23:27

kaworu requested changes Jun 3, 2022

View reviewed changes

install/kubernetes/Makefile Outdated Show resolved Hide resolved

install/kubernetes/Makefile Show resolved Hide resolved

joestringer force-pushed the submit/improve-version-tracking branch from bed5b6b to d6203ec Compare June 3, 2022 18:09

joestringer requested a review from kaworu June 3, 2022 18:09

pchaigno approved these changes Jun 6, 2022

View reviewed changes

aditighag approved these changes Jun 6, 2022

View reviewed changes

qmonnet approved these changes Jun 6, 2022

View reviewed changes

joestringer force-pushed the submit/improve-version-tracking branch from d6203ec to 7fc4299 Compare June 6, 2022 16:57

joestringer added 5 commits June 13, 2022 10:09

install/kubernetes: Default to installing latest

8c654c3

Default these helm templates to install the latest version from the tip of master, for easy development. Signed-off-by: Joe Stringer <joe@cilium.io>

docs: Document new values step when branching

3268448

Signed-off-by: Joe Stringer <joe@cilium.io>

install/kubernetes: Tidy up lint target

4e5e1b4

Split up the variables used here for better readability, and add the V=0 quiet line to indicate progress in the way the other targets do. Signed-off-by: Joe Stringer <joe@cilium.io>

joestringer force-pushed the submit/improve-version-tracking branch from 3bd8dc9 to bab8e48 Compare June 13, 2022 17:09

joestringer merged commit 3ec10e7 into cilium:master Jun 13, 2022

joestringer deleted the submit/improve-version-tracking branch June 13, 2022 20:27

joestringer added the needs-backport/1.11 label Jun 13, 2022

joestringer mentioned this pull request Jun 13, 2022

[v1.11] Templatize helm template image references #20189

Merged

joestringer added backport-pending/1.11 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.11 labels Jun 13, 2022

joestringer mentioned this pull request Jun 13, 2022

Prepare for release v1.11.6 #20190

Merged

michi-covalent mentioned this pull request Jun 14, 2022

helm: Templatize preflight and clustermesh-apiserver repos #20206

Merged

joestringer mentioned this pull request Jun 14, 2022

feat(helm): Add ability to specify topologySpreadConstraints #20046

Merged

6 tasks

michi-covalent added a commit that referenced this pull request Jun 14, 2022

helm: Templatize preflight and clustermesh-apiserver repos

bb9e8ac

Following up on #20066, templatize preflight and clustermesh-apiserver repos. Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

joestringer pushed a commit that referenced this pull request Jun 15, 2022

helm: Templatize preflight and clustermesh-apiserver repos

cb47011

Following up on #20066, templatize preflight and clustermesh-apiserver repos. Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

aanm mentioned this pull request Jun 22, 2022

Prepare for release v1.12.0-rc3 #20279

Merged

gandro mentioned this pull request Jun 22, 2022

helm: Use hubble-relay-ci image for master branch #20283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Templatize helm template image references #20066

Templatize helm template image references #20066

Uh oh!

joestringer commented Jun 2, 2022 •

edited

Loading

Uh oh!

joestringer Jun 2, 2022

Uh oh!

michi-covalent left a comment

Uh oh!

Uh oh!

aditighag left a comment

Uh oh!

Uh oh!

aditighag Jun 2, 2022

Uh oh!

joestringer Jun 2, 2022

Uh oh!

joestringer Jun 2, 2022

Uh oh!

joestringer Jun 2, 2022 •

edited

Loading

Uh oh!

kaworu left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

joestringer commented Jun 3, 2022

Uh oh!

pchaigno left a comment

Uh oh!

Uh oh!

		# File generated by install/kubernetes/Makefile; DO NOT EDIT.
		# This file is based on install/kubernetes/cilium/values.yaml.tmpl.

	- name: Run helm-charts
	run: \|
	make -C install/kubernetes
	test -z "$(git status --porcelain)" \|\| (echo "please run 'make -C install/kubernetes' and submit your changes"; exit 1)

Templatize helm template image references #20066

Templatize helm template image references #20066

Uh oh!

Conversation

joestringer commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

michi-covalent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditighag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditighag Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

joestringer Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

joestringer Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

joestringer Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaworu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joestringer commented Jun 3, 2022

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joestringer commented Jun 2, 2022 •

edited

Loading

joestringer Jun 2, 2022 •

edited

Loading

kaworu left a comment •

edited

Loading