feat(CLOUDDST-25034): run rh-sign-image-cosign signing in paralles (FIXED) #771

midnightercz · 2025-01-16T15:43:51Z

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found. Also whole procedure now runs in parallel.

This includes fixed reported by user in the second commit

openshift-ci · 2025-01-16T15:44:03Z

Hi @midnightercz. Thanks for your PR.

I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jinqi7 · 2025-01-17T00:56:00Z

/ok-to-test

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml

mmalina · 2025-01-17T08:16:54Z

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml

@@ -147,7 +148,8 @@ spec:
          fi

          if [ "$found_signatures" -eq 0 ]; then
-            run_cosign -t 3m0s sign "$COSIGN_REKOR_ARGS" \
+            # shellcheck disable=SC2086


I would suggest to use an array for the args and then this won't be needed. See my other comment.

It's not correct right now. An array needs to be used. Right now it's just one big string.

emilyzheng · 2025-01-21T12:03:41Z

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml

+              COSIGN_REKOR_ARGS="--insecure-ignore-tlog=true"
+          fi
+          # shellcheck disable=SC2086
+          verify_output=$(run_cosign verify $COSIGN_REKOR_ARGS --key "$PUBLIC_KEY_FILE" "$reference")


When running cosign verify locally, I had to download public key of rekor and set SIGSTORE_REKOR_PUBLIC_KEY with it. Otherwise, it fails with "Error: no matching signatures: error verifying bundle: verifying bundle: rekor log public key not found for payload". Does it need to be set here?

emilyzheng · 2025-01-22T03:51:37Z

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml

+          local reference=$2
+          local tag=$3
+          local digest=$4
+          found_signatures=$(check_existing_signatures "$identity" "$reference:$tag" "$digest")


Verifying tag reference can only get signature of the manifest list. Manifest digests would always have 0 found signatures. Verify reference@digest instead?

emilyzheng · 2025-01-22T08:31:52Z

Parallel runs of the same image may result in some signatures not pushed. A reproducer:

$ cat cosign.sh 
for tag in t1 t2 t3; do
    cosign sign --tlog-upload=false --key awskms:///thepreprodkey --sign-container-identity registry.test/test/repo2:$tag quay.io/redhat-dev/test----repo2@sha256:1a452c013d37a60014c5506c4230d5b85686b106f1b7dbd9b93fc44f87a12643 &
done

$ sh cosign.sh 
Pushing signature to: quay.io/redhat-dev/test----repo2
Pushing signature to: quay.io/redhat-dev/test----repo2
Pushing signature to: quay.io/redhat-dev/test----repo2

$ cosign verify --insecure-ignore-tlog=true --key /tmp/pubkey quay.io/redhat-dev/test----repo2@sha256:1a452c013d37a60014c5506c4230d5b85686b106f1b7dbd9b93fc44f87a12643 | jq .[].critical.identity.'"docker-reference"'
"registry.test/test/repo2:t1"
"registry.test/test/repo2:t3"

(registry.test/test/repo2:t2 is lost)

We've met this in pubtools-sign release-engineering/pubtools-sign#37 (comment).

emilyzheng · 2025-01-23T04:24:56Z

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml

+          fi
+
+          if [ "$found_signatures" -eq 0 ]; then
+            run_cosign -t 3m0s sign "${COSIGN_REKOR_ARGS[@]}" \


$ COSIGN_REKOR_ARGS="-y --rekor-url=abc"; cosign sign "${COSIGN_REKOR_ARGS[@]}" WARNING: the -y --rekor-url=abc flag is deprecated and will be removed in a future release. Please use the --y --rekor-url=abc flag instead. Error: unknown flag: --y --rekor-url main.go:74: error during command execution: unknown flag: --y --rekor-url

Remove the quotes?

The quotes were used because otherwise shellcheck complains.

But as I suggested in #771 (comment) , an array should be used. Currently COSIGN_REKOR_ARGS is defined as a plain string. See here for an example: https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/managed/create-pyxis-image/create-pyxis-image.yaml#L165

johnbieren · 2025-01-23T12:49:57Z

/ok-to-test

konflux-ci-qe-bot · 2025-01-23T13:52:37Z

@midnightercz: The following test has Failed, say /retest to rerun failed tests.

PipelineRun Name	Status	Rerun command	Build Log	Test Log
`konflux-e2e-tests-catalog-b6wf8`	Failed	`/retest`	View Pipeline Log	View Test Logs

Inspecting Test Artifacts

To inspect your test artifacts, follow these steps:

Install ORAS (see the ORAS installation guide).
Download artifacts with the following commands:

mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/release-service-catalog:konflux-e2e-tests-catalog-b6wf8

Test results analysis

🚨 Error occurred while running the E2E tests, list of failed Spec(s):

➡️ [failed] [It] [release-pipelines-suite e2e tests for rh-push-to-redhat-io pipeline] Rh-push-to-redhat-io happy path Post-release verification verifies the rhio release pipelinerun is running and succeeds [release-pipelines, rh-push-to-redhat-io, PushToRedhatIO]

Click to view logs

PipelineRun managed-nk8pb failed
Expected
    : Pipelinerun 'managed-nk8pb' didn't succeed\n
to equal
    :

➡️ [failed] [It] [release-pipelines-suite e2e tests for rh-advisories pipeline] Rh-advisories happy path Post-release verification verifies the advs release pipelinerun is running and succeeds [release-pipelines, rh-advisories, rhAdvisories]

Click to view logs

PipelineRun managed-rs8dx failed
Expected
    : Pipelinerun 'managed-rs8dx' didn't succeed\n
to equal
    :

➡️ [failed] [It] [release-pipelines-suite e2e tests for multi arch with rh-advisories pipeline] Multi arch test happy path Post-release verification verifies the multiarch release pipelinerun is running and succeeds [release-pipelines, multiarch-advisories, multiArchAdvisories]

Click to view logs

PipelineRun managed-7b5vz failed
Expected
    : Pipelinerun 'managed-7b5vz' didn't succeed\n
to equal
    :

➡️ [failed] [It] [release-pipelines-suite e2e tests for rhtap-service-push pipeline] Rhtap-service-push happy path Post-release verification verifies if the PR in infra-deployments repo is created/updated [release-pipelines, rhtap-service-push, RhtapServicePush]

Click to view logs

Timed out after 600.088s.
Expected success, but got an error:
    <*errors.errorString | 0xc0005e9c10>: 
    The reference is not updated and the source PR#1808 is not added to the PR of infra-deployments
    {
        s: "The reference is not updated and the source PR#1808 is not added to the PR of infra-deployments",
    }

johnbieren

No comments from me beyond the open threads

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found. Also whole procedure now runs in parallel. Signed-off-by: Jindrich Luza <[email protected]>

- Added missing --key "$PUBLIC_KEY_FILE" to cosign verify calls - Removed string wrap from COSIGN_REKOR_ARGS Signed-off-by: Jindrich Luza <[email protected]>

- export SIGSTORE_REKOR_PUBLIC_KEY for verification - fixed bash array argument Signed-off-by: Jindrich Luza <[email protected]>

Verify images by digest not tag Signed-off-by: Jindrich Luza <[email protected]>

midnightercz requested a review from a team as a code owner January 16, 2025 15:43

openshift-ci bot added the needs-ok-to-test label Jan 16, 2025

midnightercz force-pushed the parallel-cosign-signing branch 4 times, most recently from 3edf95a to a54fd80 Compare January 16, 2025 16:41

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Jan 17, 2025

mmalina reviewed Jan 17, 2025

View reviewed changes

tasks/managed/rh-sign-image-cosign/rh-sign-image-cosign.yaml Outdated Show resolved Hide resolved

mmalina reviewed Jan 17, 2025

View reviewed changes

emilyzheng reviewed Jan 22, 2025

View reviewed changes

midnightercz force-pushed the parallel-cosign-signing branch from 5aa7d15 to 40856f8 Compare January 22, 2025 15:49

midnightercz requested review from mmalina and emilyzheng January 22, 2025 16:48

emilyzheng reviewed Jan 23, 2025

View reviewed changes

johnbieren reviewed Jan 23, 2025

View reviewed changes

midnightercz added 3 commits January 30, 2025 16:07

fix(CLOUDDST-25034): run rh-sign-image-cosign signing in parallel

d6959cf

- Added missing --key "$PUBLIC_KEY_FILE" to cosign verify calls - Removed string wrap from COSIGN_REKOR_ARGS Signed-off-by: Jindrich Luza <[email protected]>

fix(CLOUDDST-25034): run rh-sign-image-cosign signing in parallel

028af7e

- export SIGSTORE_REKOR_PUBLIC_KEY for verification - fixed bash array argument Signed-off-by: Jindrich Luza <[email protected]>

midnightercz force-pushed the parallel-cosign-signing branch 3 times, most recently from 18d4b00 to 332dde2 Compare January 30, 2025 15:22

fix(CLOUDDST-25034): run rh-sign-image-cosign signing in parallel

71a8311

Verify images by digest not tag Signed-off-by: Jindrich Luza <[email protected]>

midnightercz force-pushed the parallel-cosign-signing branch from 332dde2 to 71a8311 Compare January 30, 2025 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CLOUDDST-25034): run rh-sign-image-cosign signing in paralles (FIXED) #771

feat(CLOUDDST-25034): run rh-sign-image-cosign signing in paralles (FIXED) #771

midnightercz commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

jinqi7 commented Jan 17, 2025

mmalina Jan 17, 2025 •

edited

Loading

midnightercz Jan 22, 2025

mmalina Jan 24, 2025

emilyzheng Jan 21, 2025

emilyzheng Jan 22, 2025

emilyzheng commented Jan 22, 2025

emilyzheng Jan 23, 2025

mmalina Jan 24, 2025

johnbieren commented Jan 23, 2025

konflux-ci-qe-bot commented Jan 23, 2025

johnbieren left a comment

feat(CLOUDDST-25034): run rh-sign-image-cosign signing in paralles (FIXED) #771

Are you sure you want to change the base?

feat(CLOUDDST-25034): run rh-sign-image-cosign signing in paralles (FIXED) #771

Conversation

midnightercz commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

jinqi7 commented Jan 17, 2025

mmalina Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

midnightercz Jan 22, 2025

Choose a reason for hiding this comment

mmalina Jan 24, 2025

Choose a reason for hiding this comment

emilyzheng Jan 21, 2025

Choose a reason for hiding this comment

emilyzheng Jan 22, 2025

Choose a reason for hiding this comment

emilyzheng commented Jan 22, 2025

emilyzheng Jan 23, 2025

Choose a reason for hiding this comment

mmalina Jan 24, 2025

Choose a reason for hiding this comment

johnbieren commented Jan 23, 2025

konflux-ci-qe-bot commented Jan 23, 2025

Inspecting Test Artifacts

Test results analysis

johnbieren left a comment

Choose a reason for hiding this comment

mmalina Jan 17, 2025 •

edited

Loading