Add build image support to add_image_reference

[Zokormazo]

The intent of this PR is to extend the funcionality of add_image_reference.py script to be able to inject build image references when the script is called with --build-image flag.

This is useful for konflux-ci/build-definitions#2044 to be able to inject the image that was used to run the script as part of the build images on the final sbom.

The script's default functionality continues being the same when is executed without --build-image .

Calling it with the new flag will change the functionality to:

1. Add a properties block to the package injected on the sbom. The resultant package will be something like this:

            "components": [
                {
                    "type": "container",
                    "name": "quay.io/mkosiarc_rhtap/single-container-app",
                    "purl": "pkg:oci/single-container-app@sha256:8f99627e843e931846855c5d899901bf093f5093e613a92745696a26b5420941?repository_url=quay.io/mkosiarc_rhtap/single-container-app",
                    "properties": [
                        {
                            "name": "konflux:container:is_builder_image:for_stage",
                            "value": "0",
                        }
                    ]
                }

2. For cyclone format:

   • the package will not overwride the metadata[component] block
   • the package will be appended to the formulation section instead of components section

3. For SPDX format:

   • the package will not be colocated on the root of the document
   • the package will be injected with a BUILD_TOOL_OF relationship with the root package.

These changes are following the same format base-images-sbom-script uses to inject the base images coming from the Containerfile.

The main reason to colocate the functionality here and not on base-images-sbom-script is that the base-images-sbom-script logic is dependant on the content of the Dockerfile to inject the images. The changes the script would need to add arbitrary new build images that are not present on the Dockerfile would be big. Meanwhile the changes to repurpose this script to handle the original case and the new case are much smaller. An independent new script would be an option too, but the overlap of the code between add_image_reference and the new script would be huge.

TODO

• Finish SPDX tests
• Update README.md
• Update docstrings of the modified functions

[Zokormazo]

@Allda @mkosiarc @arewm good morning o/

Could I get some feedback on the code here before I finish the tests for spdx and update the doc strings?

Thanks in advance

[mmorhun]

Could you please make PR description explaining the changes, what how and why was done?

[brunoapimentel]

I'm not a direct maintainer of this script, but from my understanding, the add_image_reference script is currently meant for adding the output image to the SBOM, so the proposed change is stretching its goal and recontextualizing it a bit.

I don't think this is a bad choice, though, since the alternative would be to add it to the base_images_sbom_script, which only works by parsing the Containerfile and getting the base images from it. Trying to incorporate these changes to base_images_sbom_script would result in much bigger changes, and I don't see how to do it smoothly.

So I think that your approach is probably the best one, but we'll need to change the README to recontextualize the add_image_reference script's goal and document the new behavior.

[brunoapimentel]

I think hardcoding this as 1 wouldn't be correct, since this can be added to an image that already has several konflux:container:is_builder_image:for_stage, and potentially have one that is already stage 1. Maybe we'll need to increment this accordingly?

[Zokormazo]

Yeah. A hardcoded 1 is wrong. the discussion about the property will condition what we do here :)
#254 (comment)

[brunoapimentel]

I was also wondering if it could be confusing to a user when he inspects the SBOM and finds an additional builder image that is not part of the Containerfile. I imagine that the fact that it is coming from an image used in a specific pipeline task would not be immediately obvious.

That being said, I don't think that SPDX or CycloneDX have any standard means of dealing with "additional images that were used as part of the build pipeline", so probably adding it as a builder image makes the most sense. It'd be nice if we could figure out any additional info that can be added to the SBOM to make this property meaning's clearer.

The only thing that comes to mind is to add the name of the task instead of a integer as value for konflux:container:is_builder_image:for_stage, but I still don't think it solves the issue perfectly. In any case, feel free to dismiss this comment, I am mostly thinking about the user experience here.

[Zokormazo]

I was also wondering if it could be confusing to a user when he inspects the SBOM and finds an additional builder image that is not part of the Containerfile. I imagine that the fact that it is coming from an image used in a specific pipeline task would not be immediately obvious.

That being said, I don't think that SPDX or CycloneDX have any standard means of dealing with "additional images that were used as part of the build pipeline", so probably adding it as a builder image makes the most sense. It'd be nice if we could figure out any additional info that can be added to the SBOM to make this property meaning's clearer.

The only thing that comes to mind is to add the name of the task instead of a integer as value for konflux:container:is_builder_image:for_stage, but I still don't think it solves the issue perfectly. In any case, feel free to dismiss this comment, I am mostly thinking about the user experience here.

This is a great appreciation, and I wondered the same thing :)

This image is not really part of the multi stage dockerfile, so for_stage and an integer does not codify properly what this image was used for. I don't have enough knowledge around sbom formats to make a decision here either :)

• Can we use something different to konflux:container:is_builder_image:for_stage ? Is there a field that suits better for this case?
• Can konflux:container:is_builder_image:for_stage hold an string? Could run-script be the value on that case?
• If it can not hold an string, could we use -1 ? Would this represent he use case better?

I need some help to answer those questions and make a decision here :)

[brunoapimentel]

* Can we use something different to `konflux:container:is_builder_image:for_stage` ? Is there a field that suits better for this case?

I believe we can. These konflux: properties were defined by us, since there was no official property that fit our use case.

* Can `konflux:container:is_builder_image:for_stage`  hold an string? Could `run-script` be the value on that case?

Yes, the value field type is string (CycloneDx docs).

As for SPDX, things are a little different. We tend to simply covert the CycloneDX into annotations, but if you look at the add_image_reference script, it is not being done there, and I assume it's because the component is clearly declared by making use of SPDX's relationships. I think BUILD_DEPENDENCY_OF still makes the most sense in our case (as it is used here), but the full list is here, maybe you can find something more fitting (or we can simply dump the CDX property).

[Zokormazo]

Since they are defined by us, it would make more sense to not use for_stage then?

Something like konflux:container:is_builder_image:run_script or konflux:container:is_builder_image:additional_build_image , value: run-script ? for SPDX, OTHER and a description on the field could make more sense?

[Zokormazo]

As for SPDX, things are a little different. We tend to simply covert the CycloneDX into annotations, but if you look at the add_image_reference script, it is not being done there, and I assume it's because the component is clearly declared by making use of SPDX's relationships. I think BUILD_DEPENDENCY_OF still makes the most sense in our case (as it is used here), but the full list is here, maybe you can find something more fitting (or we can simply dump the CDX property).

This is great info. I miss something similar to https://github.com/konflux-ci/build-tasks-dockerfiles/blob/main/sbom-utility-scripts/scripts/base-images-sbom-script/app/base_images_sbom_script.py#L201 then for the spdx, at least the annotation part :)

[brunoapimentel]

/ok-to-test

[brunoapimentel]

Since they are defined by us, it would make more sense to not use for_stage then?

Something like konflux:container:is_builder_image:run_script or konflux:container:is_builder_image:additional_build_image , value: run-script ? for SPDX, OTHER and a description on the field could make more sense?

Naming things is always hard 😓

I still like konflux:container:is_builder_image:additional_build_image , value: run-script a little better, and this choice will allow us to add even additional builder images types (as values) in case any other pops up in the future. I'd just rename it to additional_builder_image, for consistency.

For SPDX, I like BUILD_TOOL_OF, was you've written in the description.

[Zokormazo]

I still like konflux:container:is_builder_image:additional_build_image , value: run-script a little better, and this choice will allow us to add even additional builder images types (as values) in case any other pops up in the future. I'd just rename it to additional_builder_image, for consistency.

Haha, just finished implementation and tests without this. Will update after lunch

[Zokormazo]

@brunoapimentel changed the annotation name/value. wdyt now? Ready to start updating docstrings and the README ?

[brunoapimentel]

@brunoapimentel changed the annotation name/value. wdyt now? Ready to start updating docstrings and the README ?

It's fine by me. Any more considerations, @mmorhun?