Skip to content

Fix image checksum validation and upgrade ingress NGINX to demonstrate the problem #6440

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 24, 2023
Merged

Fix image checksum validation and upgrade ingress NGINX to demonstrate the problem #6440

merged 4 commits into from
Oct 24, 2023

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Oct 24, 2023 •
edited
Loading

The checksum validation for Docker images is broken because we use crane manifest repo:tag@sha256:digest which gets the file by checksum (the checksum from the makefile) rather than by tag....so the checksum always matches.
But the image is then pulled by reference to its tag, so although the local image on the filesystem will be the right version,
we won't detect if someone has pushed a different image to the registry using the same tag.

The problem is demonstrated in this PR follows:

  1. Updated only the semver tag of the ingress-nginx image and Helm chart. Did not update the sha256 sum of the images.
  2. Expected the build to fail because of a checksum validation. But it passed.
  3. Updated the checksum validation. The checksum test still passes, because the script this time loads the cached image which the previous CI run downloaded and cached with the wrong checksum in the filename.
  4. Updated the checksums. Expect the checksum validation to pass and the image to be downloaded again but with new filename containing new checksum.

I also removed the multi-arch variant of the image-download rule because it was also broken which had been obscuring the fact that we were supplying it with digests of single-arch vault and kind images.

Testing

There were a couple of test flakes, which I assume was caused by Prow jobs getting killed (or something):

I deleted _bin/ and tested locally and everything worked.

 rm -rf _bin/
make -j e2e-setup
make e2e GINKGO_FOCUS="Vault\ Issuer"
...
Ran 25 of 778 Specs in 51.981 seconds
SUCCESS! -- 25 Passed | 0 Failed | 0 Pending | 753 Skipped
--- PASS: TestE2E (52.02s)
PASS

Ginkgo ran 1 suite in 1m27.4800163s
Test Suite Passed
NONE

wallrj added 4 commits October 24, 2023 13:16
@wallrj
Upgrade ingress NGINX
ecada9c 
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Fix the digest check for single-arch images
5db745b 
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Update ingress-nginx image checksums
c34bdda 
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Remove the multi-arch variant
4d2a227 
Because it was also broken and was being supplied with digests of
single-architecture images rather than multi-arch manifests

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 24, 2023
@wallrj wallrj changed the title WIP: Upgrade ingress NGINX WIP: Fix image checksum validation and upgrade ingress NGINX to demonstrate the problem Oct 24, 2023
@jetstack-bot jetstack-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 24, 2023
@wallrj wallrj changed the title WIP: Fix image checksum validation and upgrade ingress NGINX to demonstrate the problem Fix image checksum validation and upgrade ingress NGINX to demonstrate the problem Oct 24, 2023
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2023
@wallrj wallrj requested a review from SgtCoDFish October 24, 2023 16:17
@wallrj
Copy link
Member Author

wallrj commented Oct 24, 2023

/kind cleanup

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 24, 2023
SgtCoDFish
SgtCoDFish approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

I really like how you told the story of this using different commits 🤩

Nice stuff, thanks!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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

The pull request process is described 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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023
@jetstack-bot jetstack-bot merged commit 446f133 into cert-manager:master Oct 24, 2023
@wallrj wallrj deleted the fix-image-digest-check branch October 24, 2023 16:40
@wallrj wallrj mentioned this pull request Oct 26, 2023
Merged
@wallrj
Copy link
Member Author

wallrj commented Oct 26, 2023

This has caused some of the recent test grid failures

It turns out that the kind images were meant to be referenced by multi-arch digest,
but that the digest for 1.28 had been accidentally set to the digest of the linux/amd64 image.

@wallrj wallrj mentioned this pull request Oct 26, 2023
Merged
@inteon inteon mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SgtCoDFish SgtCoDFish SgtCoDFish approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wallrj @jetstack-bot @SgtCoDFish