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

fix(KFLUXBUGS-1152): add support for multi-arch #871

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

scoheb
Copy link
Contributor

@scoheb scoheb commented Mar 12, 2024

  • if IMAGE_URL is a multi-arch one, print out sbom
    for each arch present

Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@arewm
Copy link
Member

arewm commented Mar 12, 2024

/retest-required

@chmeliik
Copy link
Contributor

chmeliik commented Mar 12, 2024

Similar: #873

^ is possibly a more generic change?

@ralphbean
Copy link
Member

I started something similar in 95c15a7. A difference is that it would print all of the sboms, one for each arch, if an image index was detected. I didn't get a chance to test it yet though. I got hung up when I realized that the existing image for the task contains only cosign, and doesn't contain skopeo. At that point I backed out and tried #873

If you see a way to easily get skopeo and cosign both in the image here, I think the dynamic approach here is better than #873. If we do go ahead with this, consider pulling in the looping idea from 95c15a7 instead of assuming that amd64 is the only arch the user cares about.

@ralphbean
Copy link
Member

ralphbean commented Mar 12, 2024

Note, the other architectures are actually important. For the bootc images, for instance, we expect the sbom content to differ between the different arches - and those differences will be important for some users to know.

For example, try one of the centos-bootc images: quay.io/redhat-user-workloads/centos-bootc-tenant/centos-bootc/centos-bootc. Download the sbom twice, once with --platform linux/amd64 and once with --platform linux/arm64. Note the grub2-pc package in the diff between those SBOMs.

@scoheb scoheb force-pushed the KBUGS-1152 branch 2 times, most recently from 5a4f9b9 to 7077a82 Compare March 14, 2024 23:06
@scoheb
Copy link
Contributor Author

scoheb commented Mar 14, 2024

updated PR to use different task image that includes skopeo and included looping over all the arches present.

@scoheb scoheb force-pushed the KBUGS-1152 branch 3 times, most recently from 300c4fd to 188f004 Compare March 15, 2024 00:40
Comment on lines 68 to 72
for arch in $ARCHES; do
echo ""
echo "Arch: $arch"
echo ""
download_sbom_with_retry " --platform=linux/$arch "
done
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just a task to show the summary in the console, I don't know if users would get any value iterating through all of the arches and showing all SBOMs. I wouldn't expect this to be a primary means to see the information. Instead, this might decrease the utility as it just increases the size of the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See @ralphbean 's comment:

#871 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the SBOM content is important, but I don't know if it is important in this context. This Tekton task is just an "quick method" to show users what is in the builds. I posit that if users want to see detailed information then they will likely try to either access it locally where they can target specific architectures or they will upload the SBOMs to some tool like guac.sh.

Copy link
Member

Choose a reason for hiding this comment

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

I started this conversation with @ralphbean in #873 (review) but it wasn't resolved before that PR was closed.

@scoheb
Copy link
Contributor Author

scoheb commented Mar 15, 2024

so we remove the task from the pipeline? I am not familiar with the history of why this task was needed in the first place.

@arewm
Copy link
Member

arewm commented Mar 15, 2024

@ralphbean and I chatted again. We agree that it would make sense to use a solution like #873 where we default to amd64 by default. This would prevent e2e tests from breaking as you observed in this PR and it would allow users to show specific architecture's SBOMs on their own if they are interested in it.

@scoheb
Copy link
Contributor Author

scoheb commented Mar 15, 2024

That's what I was thinking too. Although we still have to detect if it is a multi-arch image because you cannot specify a platform for a single Arch image with cosign.

@scoheb scoheb force-pushed the KBUGS-1152 branch 2 times, most recently from 5da9e4b to 43eb761 Compare March 15, 2024 22:25
@scoheb
Copy link
Contributor Author

scoheb commented Mar 15, 2024

@arewm @ralphbean please review

Comment on lines 56 to 57
echo "Inspecting image ${IMAGE_URL}"

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be dropped so as not to break the e2e-tests and other sensitive parsers who are expecting only JSON here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true! dropped.

if [ -z "${PLATFORM}" ]; then
PLATFORM="amd64"
fi
download_sbom_with_retry " --platform=linux/$PLATFORM "
Copy link
Member

Choose a reason for hiding this comment

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

Let's not hardcode the "linux/" prefix here to make the task more generic. Let PLATFORM be exactly whatever the user specifies and modify the documentation to tell users to set it to "linux/amd64" if they want amd64.

Note, there are actually non-linux containers like mcr.microsoft.com/windows/servercore:ltsc2022.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to be more flexible.

| name | description | default value | required |
|-----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|-----------|
| IMAGE_URL | Fully qualified image name to show SBOM for. | | true |
| PLATFORM | Specific architecture to display the SBOM for. An example arch would be "amd64". If IMAGE_URL refers to a multi-arch image and this parameter is empty, the task will default to use "amd64". | | false |
Copy link
Member

Choose a reason for hiding this comment

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

See comment below - I think this should be updated to expect a string like "linux/amd64" so that we don't box ourselves in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

@ralphbean ralphbean left a comment

Choose a reason for hiding this comment

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

See inline comments above. Consider changing both the PLATFORM string and the echo statement.

@scoheb
Copy link
Contributor Author

scoheb commented Mar 16, 2024

/retest

| name | description | default value | required |
|-----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|-----------|
| IMAGE_URL | Fully qualified image name to show SBOM for. | | true |
| PLATFORM | Specific architecture to display the SBOM for. An example arch would be "linux/amd64". If IMAGE_URL refers to a multi-arch image and this parameter is empty, the task will default to use "linux/amd64". | | false |
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The default value is not included in the table column

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@tkdchen tkdchen left a comment

Choose a reason for hiding this comment

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

LGTM.

A new parameter is added, we need to make a new version with MIGRATION.md for this task.

@ralphbean
Copy link
Member

/retest

1 similar comment
@ralphbean
Copy link
Member

/retest

- add new parameter called PLATFORM. The user can supply this parameter value if they want to display
  the sbom for a particular arch in the case of a multi-arch image.
- In the case of a single arch image, the parameter is ignored.
- if PLATFORM is empty and the image is multi-arch, the task defaults to 'amd64'

Signed-off-by: Scott Hebert <[email protected]>
Copy link

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

@scoheb
Copy link
Contributor Author

scoheb commented Apr 15, 2024

A new parameter is added, we need to make a new version with MIGRATION.md for this task.

README mentions this: Adding a new parameter with a default value does not require the task version increase

so we should be ok as-is

@ralphbean ralphbean added this pull request to the merge queue Apr 15, 2024
Merged via the queue into konflux-ci:main with commit 6172b64 Apr 15, 2024
6 checks 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.

5 participants