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

Feat: Add script for image index SBOM creation. #165

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

BorekZnovustvoritel
Copy link
Contributor

@BorekZnovustvoritel BorekZnovustvoritel commented Sep 25, 2024

Ticket for tracking: ISV-5220.

This task is a part of an effort to create SBOMs for index images (manifest lists). The work breakdown can be found in the links of this ticket, which is a part of the epic Manifest list SBOM.

After this PR is merged, another PR should follow to Konflux task build-image-index, which should call this script. The current consent is that the Tekton task should call buildah manifest inspect on the newly created index, then provide the output to this script along with the index url and digest.

This script should output a valid SPDX 2.3 SBOM.

@BorekZnovustvoritel
Copy link
Contributor Author

The script should be ready for a review. If you have any points of improvement, I'll be happy to include them. Not sure who to tag as a repository owner...
@chmeliik @mkosiarc

@chmeliik
Copy link
Contributor

@BorekZnovustvoritel thanks for contributing!

Some more context in the PR description/commit messages would go a long way to make this easier for us to review. What is this script for, how will it be used, etc. If there's a design for it somewhere a link would be appreciated

@BorekZnovustvoritel
Copy link
Contributor Author

@BorekZnovustvoritel thanks for contributing!

Some more context in the PR description/commit messages would go a long way to make this easier for us to review. What is this script for, how will it be used, etc. If there's a design for it somewhere a link would be appreciated

Sorry for the lack of clarity, I have updated the PR description. If something is still missing, let me know and I will add additional info.

@BorekZnovustvoritel
Copy link
Contributor Author

Hello, any updates on the review? I have synced with midnightercz and implemented his SPDXID creation mechanism as well.

@chmeliik
Copy link
Contributor

Sorry, I'm on konflux-user support rotation. I've had a total of about 1 hour of time between support threads this week (╯°□°)╯︵ ┻━┻

Will get to this eventually. I still have to read through the design to understand how this will be used.

@mkosiarc
Copy link
Contributor

The code looks ok to me, the tests are passing, so I will provide an ack. I don't have the time to go through the design, so I will just assume that this phase was reviewed and everything worked out.

But please update one more thing - https://github.com/konflux-ci/build-tasks-dockerfiles/blob/main/.github/workflows/build-sbom-utility-scripts-image.yml#L34

Update the file so it will run a tox check on your added script as well. This will make sure that the tests are passing.

@mkosiarc
Copy link
Contributor

/ok-to-test

@BorekZnovustvoritel
Copy link
Contributor Author

Thank you for the feedback, workflow step added.

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Nice code! 🌮

Just a few functional concerns

@chmeliik
Copy link
Contributor

My understanding of the overall feature:

This script will be used in the build-image-index task.

The content of the SBOM as generated by this script will be the entire SBOM - we will not be merging the arch-specific SBOMs into it in any way.

We will push the SBOM to the registry and associate it with the index image the same way that the arch-specific SBOMs are associated with the arch-specific manifests.

Some system will then be able to associate the index-image SBOM with the arch-specific SBOMs based on... something. This is the part that I'm missing - what is the "something" that will allow a consumer to associate the SBOMs?

@BorekZnovustvoritel
Copy link
Contributor Author

Thank you for the review. I will rework this PR to resolve all comments. Your understanding of the overall feature is correct (or at least we understand it in the same way 😀). The linking to the outside is supposed to be represented by the externalRefs field, where PURLs are specified, as represented here.

If you prefer some other method of referencing, be sure to comment on it, however that would require bigger changes to the Build Image Index task as there is not enough information for other SBOM linking right now.

@BorekZnovustvoritel
Copy link
Contributor Author

Hello again, just checking on the status. If there is something else that should be added to this PR / changed, be sure to let me know 🫡

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM, last nitpick about the SPDXID

@chmeliik
Copy link
Contributor

The linking to the outside is supposed to be represented by the externalRefs field, where PURLs are specified, as represented here.

That's what I was thinking, makes sense to me. However, I have a feeling that the arch-specific SBOMs do not include the necessary PURL anywhere. This is not something to be solved within this PR, but may need to be solved within the scope of the epic

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM

Would you mind squashing the commits? We generally prefer commits to be atomic

@BorekZnovustvoritel
Copy link
Contributor Author

Understandable, squashed into a single commit. Thank you for your assistance

@chmeliik chmeliik merged commit d4935b3 into konflux-ci:main Oct 16, 2024
1 check passed
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