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

install/kubernetes: make image digests for all components optional & configurable #22732

Merged
merged 1 commit into from Jan 2, 2023
Merged

install/kubernetes: make image digests for all components optional & configurable #22732

merged 1 commit into from Jan 2, 2023

Conversation

rastislavs
Copy link
Contributor

Signed-off-by: Rastislav Szabo rastislav@kubermatic.com

Most of the core components of Cilium already provide the options to overwrite or disable the use of image digests (*.image.digest & *.image.useDigest) via Helm values. However, overwriting or disabling the digests is not easily possible for some Hubble and etcd component images (without rewriting the whole image tag, including the version). This is cumbersome for deployments with private image registries, as the digest can change when the same image is pushed to a different registry.

This PR adds the digest and useDigest options for all remaining images. Since all the affected templates already use the "cilium.image" helper, all that is needed is to move the actual digest from the *.image.tag to the *.image.digest. The affected digests remain hardcoded in the same file (Makefile.values), just now in dedicated env variables.

This also consolidates whether digests are used or not across the images of all components, based on the USE_DIGESTS variable in values.yaml.tmpl.

install/kubernetes: make image digests for all components optional & configurable

@rastislavs rastislavs requested review from a team as code owners December 14, 2022 10:46
@maintainer-s-little-helper 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 Dec 14, 2022
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 14, 2022
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also set USE_DIGEST to true in Makefile.values? That way existing behavior is preserved?

@rastislavs
Copy link
Contributor Author

rastislavs commented Dec 14, 2022

Can you also set USE_DIGEST to true in Makefile.values? That way existing behavior is preserved?

That would not preserve the existing behaviour, as USE_DIGESTS is computed like this:

export USE_DIGESTS ?= $(shell if grep -q '""' $(DIGESTS_PATH); then echo "false"; else echo "true"; fi)

The existing behaviour in fact is, that the "core" cilium components are using/not using digests based on the above check of DIGESTS_PATH, whereas those components that I am fixing in this PR (hubble, etcd) are always using the digests.

So the existing behaviour is kind of mixed:

$ helm template --set hubble.relay.enabled=true --set hubble.ui.enabled=true  ./cilium  | grep 'image:'
        image: "quay.io/cilium/cilium-ci:latest"
        image: "quay.io/cilium/cilium-ci:latest"
        image: "quay.io/cilium/cilium-ci:latest"
        image: "quay.io/cilium/cilium-ci:latest"
        image: "quay.io/cilium/cilium-ci:latest"
        image: "quay.io/cilium/operator-generic-ci:latest"
          image: "quay.io/cilium/hubble-relay-ci:latest"
        image: "quay.io/cilium/hubble-ui:v0.9.2@sha256:d3596efc94a41c6b772b9afe6fe47c17417658956e04c3e2a28d293f2670663e"
        image: "quay.io/cilium/hubble-ui-backend:v0.9.2@sha256:a3ac4d5b87889c9f7cc6323e86d3126b0d382933bd64f44382a92778b0cde5d7"

Therefore I thought that consolidating all images based on USE_DIGESTS would make sense. But if we want to keep this mixed behaviour, we would need to either introduce a separate variable for these components, or always set useDigest to true for them.

@squeed
Copy link
Contributor

squeed commented Dec 14, 2022

We discussed this briefly in the community meeting and decided that we should be passing digests everywhere. There are tools like skopeo that can mirror to private registries without changing the digest.

So, can you enable digests universally by default?

@rastislavs
Copy link
Contributor Author

rastislavs commented Dec 15, 2022

So, can you enable digests universally by default?

@squeed - sure, I just change them to useDigest: true by default. This way, the PR preserves the existing behavior.

The images that are not affected by this PR have digests enabled if they are present in Makefile.digests, which seems to be always the case in release branches.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this does look good to me now. But let's also wait for Casey to re-review, since he seems to have looked into this more deeply.

@gandro
Copy link
Member

gandro commented Dec 20, 2022

Seems like a recent Helm merge caused conflicts. This unfortunately means that this branch needs to be rebased.

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. area/helm Impacts helm charts and user deployment experience and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 20, 2022
…configurable

Signed-off-by: Rastislav Szabo <rastislav@kubermatic.com>
@rastislavs
Copy link
Contributor Author

@gandro rebased, sorry for the delay

@squeed
Copy link
Contributor

squeed commented Jan 2, 2023

Test failure is a flake (filed issue #22910), this seems good to go.

@squeed squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 2, 2023
@pchaigno
Copy link
Member

pchaigno commented Jan 2, 2023

Only Helm and the documentation are affected so we probably don't need to run the end-to-end tests. Merging.

@pchaigno pchaigno merged commit 8bd9b66 into cilium:master Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants