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

DO NOT MERGE: Generate OCI Image Indexes by default #776

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

Conversation

arewm
Copy link
Member

@arewm arewm commented Feb 1, 2024

While this PR is a small change, we need to effectively support multi-arch test and release before it can be merged. Therefore, work needs to happen for https://issues.redhat.com/browse/KONFLUX-387 first.

In order to better support the mix of single-architecture and multi-architecture container images, we should put OCI Image Manifests within OCI Image Indexes by default.

Acknowledging that some use cases might not want an Image Index, it is possible to disable their generation.

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@arewm arewm force-pushed the produce-manifest-lists branch 3 times, most recently from a56a17a to 8942590 Compare February 1, 2024 18:20
Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -341,6 +347,15 @@ spec:
buildah config -a org.opencontainers.image.base.name=${base_image_name} -a org.opencontainers.image.base.digest=${base_image_digest} $container
buildah commit $container $IMAGE

IMAGE_INDEX="$IMAGE-index"
if [ -n "${DISABLE_IMAGE_INDEX}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just make sure the build-image-manifest task is included in the Pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would have been another option to achieve this functionality, but it would result in additional tasks in users pipelines. I chose the alternative of always producing Image Indexes while ensuring that the build-image-manifest task supports the case (I just realized that the naming is inconsistent)...

@arewm
Copy link
Member Author

arewm commented Mar 5, 2024

@lcarva , you mention that Chains should sign and attest the Image Index and the Image Manifest. That isn't going to happen with this PR, but how can we introduce that ability? Would we need to use two tasks always and this PR would change to enabling/disabling that second image generation?

@lcarva
Copy link
Contributor

lcarva commented Mar 5, 2024

@lcarva , you mention that Chains should sign and attest the Image Index and the Image Manifest. That isn't going to happen with this PR, but how can we introduce that ability? Would we need to use two tasks always and this PR would change to enabling/disabling that second image generation?

For signing, make sure the TaskRun emits a result for each ref, e.g. INDEX_IMAGE_DIGEST, INDEX_IMAGE_URL, MANIFEST_IMAGE_DIGEST, MANIFEST_IMAGE_URL. The prefix prior to IMAGE_DIGEST and IMAGE_URL can be anything.

For attestation, we need to either "promote" all those results to Pipeline results, or enable deep inspection in Chains.

@arewm
Copy link
Member Author

arewm commented Mar 5, 2024

For attestation, we need to either "promote" all those results to Pipeline results, or enable deep inspection in Chains.

From an implementation perspective of this PR and its desire to be minimally invasive to users' configured PLRs, the preference for me would be for Chains to be able to deep inspect. Is there an issue for that already?

@lcarva
Copy link
Contributor

lcarva commented Mar 6, 2024

For attestation, we need to either "promote" all those results to Pipeline results, or enable deep inspection in Chains.

From an implementation perspective of this PR and its desire to be minimally invasive to users' configured PLRs, the preference for me would be for Chains to be able to deep inspect. Is there an issue for that already?

Not that I know of.

In order to better support the mix of single-architecture and
multi-architecture container images, we should put OCI Image Manifests
within OCI Image Indexes by default.

Acknowledging that some use cases might not want an Image Index, it is
possible to disable their generation.

Signed-off-by: arewm <arewm@users.noreply.github.com>
Copy link

sonarcloud bot commented Mar 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

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.

None yet

2 participants