From 73627dd663b7dc869830c1b1856b8cc6852bf00d Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 24 Jun 2025 09:34:47 +0200 Subject: [PATCH 1/2] verify: improve output of verify-e2e-images.sh Before: Diffing e2e image list ... --- /dev/fd/63 2025-06-24 09:32:43.736397729 +0200 +++ /dev/fd/62 2025-06-24 09:32:43.736397729 +0200 @@ -5,7 +5,7 @@ invalid.registry.k8s.io/invalid/alpine registry.k8s.io/build-image/distroless-iptables registry.k8s.io/cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver registry.k8s.io/e2e-test-images/agnhost -registry.k8s.io/e2e-test-images/apparmor-loader +registry.k8s.io/e2e-test-images/apparmor-no-such-image registry.k8s.io/e2e-test-images/busybox registry.k8s.io/e2e-test-images/httpd registry.k8s.io/e2e-test-images/ipc-utils FAIL: e2e images do not match the approved list! For this test, apparmor-loader was commented out in .permitted-images (making it a forbidden unknown image) and apparmor-no-such-image was added (making it an obsolete image). Problems with the output: - The position of old and new image lists was reversed. - It's not clear what is being diffed. Not referencing .permitted-images directly probably was meant to discourage using some image other than agnhost, but developers can easily find the file anyway and are shown some other images in the diff context. After: Diffing e2e image list ... obsolete image: registry.k8s.io/e2e-test-images/apparmor-no-such-image forbidden image: registry.k8s.io/e2e-test-images/apparmor-loader FAIL: current e2e images do not match the approved list in test/images/.permitted-images! This mentions test/images/.permitted-images because developers might have to edit it if some image really becomes obsolete. --- hack/verify-e2e-images.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/hack/verify-e2e-images.sh b/hack/verify-e2e-images.sh index faa28880e9b..bc0384b0151 100755 --- a/hack/verify-e2e-images.sh +++ b/hack/verify-e2e-images.sh @@ -39,11 +39,14 @@ kube::util::read-array IMAGES < <("${e2e_test}" --list-images | sed -E 's/^(.+): # diff versus known permitted images ret=0 >&2 echo "Diffing e2e image list ..." -diff -Naupr <(printf '%s\n' "${IMAGES[@]}") <(printf '%s\n' "${PERMITTED_IMAGES[@]}") || ret=$? +# diff context is irrelevant here because of sorting. +# Instead we want to know about old images (no longer in use, need to be removed) +# and new images (should not get added). +diff <(printf '%s\n' "${PERMITTED_IMAGES[@]}") <(printf '%s\n' "${IMAGES[@]}") | sed -E -e '/^---$/d' -e '/^[[:digit:]]+[acd][[:digit:]]+$/d' -e 's/^/forbidden image:/' >&2 || ret=$? if [[ $ret -eq 0 ]]; then >&2 echo "PASS: e2e images used are OK." else - >&2 echo "FAIL: e2e images do not match the approved list!" + >&2 echo "FAIL: current e2e images do not match the approved list in test/images/.permitted-images!" >&2 echo "" >&2 echo "Please use registry.k8s.io/e2e-test-images/agnhost wherever possible, we are consolidating test images." >&2 echo "See: test/images/agnhost/README.md" From 49ebabb54e6e9f9d682ab580f55da06c54860fa0 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 24 Jun 2025 10:15:20 +0200 Subject: [PATCH 2/2] verify: additional validation of e2e.test --list-images output If the command failed in the <( ... ) expression, the return code was ignored and the script continued with potentially no output. Not likely, but it's still better to invoke the command where pipefail will catch a non-zero exit code. For example, broken test registration could cause this. There should be no log output, but if there is, failing explicitly is better than ignoring it (on stderr) or treating it like an image (on stdout). Found when experimenting with the logging configuration of e2e.test, currently there is no such unwanted log output. --- hack/verify-e2e-images.sh | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/hack/verify-e2e-images.sh b/hack/verify-e2e-images.sh index bc0384b0151..7c99a73d727 100755 --- a/hack/verify-e2e-images.sh +++ b/hack/verify-e2e-images.sh @@ -22,6 +22,7 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. cd "${KUBE_ROOT}" source hack/lib/init.sh +ret=0 # NOTE: Please do NOT add any to this list!! # @@ -34,10 +35,28 @@ kube::util::read-array PERMITTED_IMAGES < <(sed '/^#/d' ./test/images/.permitted echo "Getting e2e image list ..." make WHAT=test/e2e/e2e.test e2e_test="$(kube::util::find-binary e2e.test)" -kube::util::read-array IMAGES < <("${e2e_test}" --list-images | sed -E 's/^(.+):[^:]+$/\1/' | LC_ALL=C sort -u) + +# validate "e2e.test --list-images": +# - no unexpected output (whether it's on stderr or stdout) +# - zero exit code (indirectly ensures that tests are set up properly) +output=$("${e2e_test}" --list-images 2>&1) || ret=$? +if [[ $ret -ne 0 ]]; then + >&2 echo "FAIL: '${e2e_test} --list-images' failed:" + >&2 echo "${output}" + exit 1 +fi + +unexpected_output=$(echo "${output}" | grep -v -E '^([[:alnum:]/.-]+):[^:]+$' || true) +if [[ -n "${unexpected_output}" ]]; then + >&2 echo "FAIL: '${e2e_test} --list-images' printed unexpected output:" + >&2 echo "${unexpected_output}" + exit 1 +fi + +# extract image names without the version +kube::util::read-array IMAGES < <(echo "${output}" | sed -E 's/^([[:alnum:]/.-]+):[^:]+$/\1/' | LC_ALL=C sort -u) # diff versus known permitted images -ret=0 >&2 echo "Diffing e2e image list ..." # diff context is irrelevant here because of sorting. # Instead we want to know about old images (no longer in use, need to be removed)