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

Add daily check for vulnerability issues using Trivy #600

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

caponetto
Copy link
Contributor

https://issues.redhat.com/browse/RHOAIENG-8779

Description

This PR introduces the usage of Trivy to scan the container images and consolidate vulnerability reports. I've configured the Build Notebooks workflow to be triggered daily at 2 am UTC to generate the report (the scan runs after each image is built). I didn't see the need to run it on each push but we can enable it if it makes sense. Each report is uploaded to the workflow summary as soon as each individual job is completed. Finally, reports follow a markdown template.

Demo

Screen.Recording.2024-06-28.at.11.27.45.mp4

Here is an example of workflow run where you can see real reports.

Note: I haven't added the checks for PRs because the scan operation adds an extra ~10 minutes, which would slow our PR jobs down.

How Has This Been Tested?

I've executed the workflow on my fork.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from atheo89 and dibryant June 28, 2024 14:50
Copy link
Member

@jiridanek jiridanek left a comment

Choose a reason for hiding this comment

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

Just a few small suggestions. Over all, lgtm.

We can't fail the build when vulnerabilities are detected, that way we'd never have a pass. But these inforeports are not very practical to work with either, there is too much info to scan. We'd need to either reduce the number of findings the tool produces, so it is possible to read through it easily.

Currently, this is mainly useful to check that some vulnerability we intended to fix was indeed fixed. But, then I'd have to first merge the PR to get scan on it. (Or run scan on my machine.)

It does not seem very actionable to me right away, but it is a great capability to have around. And, I did not know the github feature https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#adding-a-job-summary, so that's interesting for me too.

@caponetto
Copy link
Contributor Author

@jiridanek thanks for the review!

We can't fail the build when vulnerabilities are detected, that way we'd never have a pass. But these inforeports are not very practical to work with either, there is too much info to scan. We'd need to either reduce the number of findings the tool produces, so it is possible to read through it easily.

Yeah, I agree. I've reduced the scope to only HIGH and CRITICAL issues because the list was getting bigger if we allowed all of them. But let's see if it helps otherwise we can revisit it and try to personalize more.

Currently, this is mainly useful to check that some vulnerability we intended to fix was indeed fixed. But, then I'd have to first merge the PR to get scan on it. (Or run scan on my machine.)

We can't enable it to run for all PRs due to the amount of time it adds but now I'm thinking if we can come up with some strategy like running it if the PR has a certain label or something like that.

@caponetto
Copy link
Contributor Author

Ok, this PR is ready.

@openshift-ci openshift-ci bot added the lgtm label Jul 1, 2024
Copy link
Member

@atheo89 atheo89 left a comment

Choose a reason for hiding this comment

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

Great CVE report tool! I have a question: Do we push the images to ghcr after completing the report? If so, I would suggest not doing it because we will fill up the registry on a daily basis.

@jiridanek
Copy link
Member

Do we push the images to ghcr after completing the report? If so, I would suggest not doing it because we will fill up the registry on a daily basis.

we have to push, the makefile is written in such a way that it builds and pushes, without the possibility to skip a push

notebooks/Makefile

Lines 61 to 65 in 3f93529

define image
$(info #*# Image build directory: <$(2)> #(MACHINE-PARSED LINE)#*#...)
$(call build_image,$(1),$(2),$(3))
$(call push_image,$(1))
endef

regarding filling up space, first, there does not seem to be a limit that applies to us, and second, please review this PR that sets up a cleaning job to free up the space daily

@caponetto
Copy link
Contributor Author

@atheo89 Thanks for raising this concern! Another option is to use the approach we have for pull_request, which publishes the image to localhost (ref) but I think it won't be an issue anymore after @jiridanek's PR that cleans up the registry periodically. Plus, it could be helpful to have them published somewhere if someone needs to debug the associated image with the report.

@atheo89
Copy link
Member

atheo89 commented Jul 2, 2024

Great! Let's make sure enough time passes before Jiri's prune workflow starts! 🙂

https://github.com/opendatahub-io/notebooks/pull/601/files#diff-6a2b18c5159aa29ecd02344b37be379eac595c441858bd84f754a0353e0bb5caR12

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Jul 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atheo89, jiridanek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 2, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d7b7438 into opendatahub-io:main Jul 2, 2024
6 checks passed
@atheo89
Copy link
Member

atheo89 commented Jul 2, 2024

we have to push, the makefile is written in such a way that it builds and pushes, without the possibility to skip a push

That's true; but it's not a big deal to make recipes more independent if that makes our lives easier. Added a tracker here

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

Successfully merging this pull request may close these issues.

3 participants