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

Handle building unsigned containers #240

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

Conversation

tnevrlka
Copy link
Contributor

@tnevrlka tnevrlka commented Feb 3, 2025

  • source-container-build fails fatally if the containers are unsigned
  • Users should still be able to build even if they are unsigned (EC checks will still fail though)

@tnevrlka tnevrlka requested a review from a team as a code owner February 3, 2025 09:25
@chmeliik
Copy link
Contributor

chmeliik commented Feb 4, 2025

source-container-build fails fatally if the containers are unsigned
Users should still be able to build even if they are unsigned

Was the intention of the story to skip unsigned base images, or to use them even if they're unsigned?

(EC checks will still fail though)

Will they? I'm not sure if there's an EC check for base image sources being included

@tnevrlka tnevrlka force-pushed the handle-missing-signature branch 2 times, most recently from 7daa363 to b0834e2 Compare February 5, 2025 16:10
- source-container-build fails fatally if the containers are unsigned
- Users should still be able to build even if they are unsigned
(EC checks will still fail though)

Signed-off-by: Tomáš Nevrlka <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@tnevrlka tnevrlka force-pushed the handle-missing-signature branch from b0834e2 to 18aa012 Compare February 5, 2025 16:12
@tnevrlka tnevrlka requested a review from chmeliik February 7, 2025 11:36
@chmeliik
Copy link
Contributor

chmeliik commented Feb 7, 2025

source-container-build fails fatally if the containers are unsigned
Users should still be able to build even if they are unsigned

Was the intention of the story to skip unsigned base images, or to use them even if they're unsigned?

(EC checks will still fail though)

Will they? I'm not sure if there's an EC check for base image sources being included

I checked how we currently set the base_image_source_included property. If the script doesn't find a source container for the base image at all, it will be false, otherwise true. With this, we would be adding a case of "the source container was there, but we failed to pull it (possibly due to temporary problems)" -> false.

I also checked https://github.com/enterprise-contract/ec-policies/ and https://github.com/release-engineering/rhtap-ec-policy for occurrences of base_image_source_included and didn't find any rules.

This introduces two problems

  • Until an EC check exists, this would make it possible to release builds with missing base image sources even when the source container for the base image does exist
  • There wouldn't really be a sane way to implement said EC check, because the base_image_source_included=false could mean both "base image sources don't exist" and "the build just failed to pull them"

Thinking about this more, maybe we could do something different than ignore the skopeo copy failure. Maybe we want to handle this in the Tekton task. If the python script fails, don't fail the whole task but do something similar to the scan tasks - succeed and return a TEST_OUTPUT Tekton result to indicate the failure. Then, EC would block releases because
a) TEST_OUTPUT indicates failure
b) the task probably failed to build the source container, and I believe EC does verify the existence of it

@chmeliik
Copy link
Contributor

chmeliik commented Feb 7, 2025

But TBH, I also wouldn't mind leaving things as they are and letting the source-build task fail on failure 😄 (using TEST_OUTPUT here feels like abusing it)

IMO we should ask Ralph what his expectations were about the "graceful failure"

@ralphbean
Copy link
Member

The original idea in Konflux ADR 14. Let Pipelines Proceed was that we want users to get functionally testable builds out of their pipeline as frequently as possible, even in the face of other failures so that scanning or linting issues don't prevent them from at least making progress in validating the functional state of their product. They might be blocked from releasing, but at least they won't be blocked from testing or messing around with their artifact in a dev environment.

How do you see that idea apply or not apply here?

@chmeliik
Copy link
Contributor

The original idea in Konflux ADR 14. Let Pipelines Proceed was that we want users to get functionally testable builds out of their pipeline as frequently as possible, even in the face of other failures so that scanning or linting issues don't prevent them from at least making progress in validating the functional state of their product. They might be blocked from releasing, but at least they won't be blocked from testing or messing around with their artifact in a dev environment.

How do you see that idea apply or not apply here?

Unlike scanning and linting, source-container build failures will usually be intermittent. It doesn't seem right in general to let those proceed, since the solution will be a retry. And if we let the pipeline proceed, then the feedback loop won't be just to keep retrying the build but will also involve waiting for the release pipeline to succeed/fail.

@chmeliik
Copy link
Contributor

chmeliik commented Feb 11, 2025

Unlike scanning and linting, source-container build failures will usually be intermittent. It doesn't seem right in general to let those proceed, since the solution will be a retry. And if we let the pipeline proceed, then the feedback loop won't be just to keep retrying the build but will also involve waiting for the release pipeline to succeed/fail.

But maybe we can re-consider if we want to fail the source builds because of missing signatures. During the build step, buildah pulled the base images without complaining, so perhaps the source-build step shouldn't care about the missing signatures either.

@mmorhun
Copy link
Collaborator

mmorhun commented Feb 11, 2025

I think we can have a task parameter that would control if the task should fail or not on missing signature. I'd keep fail as the default behaviour, but if a user wants to proceed with the build and skip the source image build problem for now, they still can.

@tnevrlka
Copy link
Contributor Author

Hi @ralphbean, what do you think about the proposed solutions?

@tnevrlka
Copy link
Contributor Author

@chmeliik Would you be okay with the task parameter?

@chmeliik
Copy link
Contributor

chmeliik commented Feb 19, 2025

@chmeliik Would you be okay with the task parameter?

Sounds okay. Just to clarify what it would do: IMO when set to ignore missing signatures, the source build task should download the parent image even if it's unsigned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants