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

Pull preloaded images based on digest, not tag #227

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

inteon
Copy link
Member

@inteon inteon commented Dec 16, 2024

Fixes a long-standing issue where images were preloaded by tag instead of digest causing the digest check to fail when the upstream image changed.
Now, the images are pulled by digest, we modify the pulled tarball to have the desired image tag and we use the tarball that way.

Is a long-term fix for the issue we tried to fix here: #224

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2024
@inteon inteon force-pushed the longterm_image_preload_fix branch 2 times, most recently from efa32c3 to 2d38c24 Compare December 16, 2024 15:36
Copy link

@tenstad tenstad left a comment

Choose a reason for hiding this comment

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

LGTM!

modules/kind/kind-image-preload.mk Show resolved Hide resolved
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tenstad
Once this PR has been reviewed and has the lgtm label, please ask for approval from inteon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SgtCoDFish
Copy link
Member

Capturing discussion I remember from standup this morning:

  • We might actually want to break CI if a digest changes, to alert us to potential supply-chain attacks. It's not certain we want to avoid that.
  • This PR essentially introduces a "cert-manager tag" which is pinned to the digest we first observed that tag pointing to. That means that someone running with a locally-pulled image might get different behavior to what we have, since we'll have a different digest and it'll be a bit hidden. It's not certain that we want that behavior.

I don't think either of the above are blockers and I don't have time to actually review this PR - just trying to capture the discussion here.

@SgtCoDFish
Copy link
Member

There's been a more recent kind release: https://github.com/kubernetes-sigs/kind/releases/tag/v0.26.0

I wonder if bumping to that is a short-term fix for the kind issues affecting CI?

@inteon
Copy link
Member Author

inteon commented Dec 17, 2024

There's been a more recent kind release: https://github.com/kubernetes-sigs/kind/releases/tag/v0.26.0

I wonder if bumping to that is a short-term fix for the kind issues affecting CI?

See #228

@inteon
Copy link
Member Author

inteon commented Dec 18, 2024

Capturing discussion I remember from standup this morning:

  • We might actually want to break CI if a digest changes, to alert us to potential supply-chain attacks. It's not certain we want to avoid that.
  • This PR essentially introduces a "cert-manager tag" which is pinned to the digest we first observed that tag pointing to. That means that someone running with a locally-pulled image might get different behavior to what we have, since we'll have a different digest and it'll be a bit hidden. It's not certain that we want that behavior.

I don't think either of the above are blockers and I don't have time to actually review this PR - just trying to capture the discussion here.

  • I don't think it is very useful to break CI when the remote registry changes what digest a tag points to. We might want some notification/ alerts maybe? Also, if there was a supply-chain attack that got fixed, we would probably hear about it in another way. By pinning to the digest disregarding the upstream tag, we prevent upstream changes from breaking our flow and/or introducing any new supply-chain attacks.
  • I don't think this will be a huge problem, the main offenders are kind's node image and busybox's image. For both, I think we are better off pinning the image digests and not breaking CI. This also ensures our tests are more reproducible (eg. when we want to patch an old version of one of our components and the upstream tag changed in the meantime). Additional to those two images, this mechanism is also used to preload the cert-manager images. These will typically not get their tag overwritten.

@inteon inteon force-pushed the longterm_image_preload_fix branch from db8c47c to e75e0dd Compare December 18, 2024 09:29
@hawksight
Copy link
Member

Just adding here that I spent some time looking for other solutions by pulling various image in tarball and oci format. Basically what @inteon has done seems to be sensible.

I'd suggest that we trust kind images when they retag them, moving their version tags to new digests.

It seems kind does not support importing the OCI format images, only tarball. And when pulling a digest bases tarball... you don't get the actual tag in the manifest.json... even id you do @.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants