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.

Sign up for GitHub

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

Jump to bottom

refactor: one trivy report per container image #4596

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

refactor: one trivy report per container image #4596

wants to merge 3 commits into from

Conversation

jason1028kr
Copy link
Collaborator

@jason1028kr jason1028kr commented Jul 8, 2024 •
edited

What type of PR is this?
/kind refactor

What this PR does / why we need it:

  • For container images, instead of producing single table that includes all trivy report, produce single report in json format for each container image
    Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:

Screen Shot 2024-07-25 at 12 52 59 PM

Release note:

none

@coveralls
Copy link

coveralls commented Jul 8, 2024 •
edited

Pull Request Test Coverage Report for Build 10114792544

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 71.005%
Totals Coverage Status
Change from base Build 10113772658: 0.0%
Covered Lines: 2564
Relevant Lines: 3611
💛 - Coveralls

djsly
djsly reviewed Jul 9, 2024
View reviewed changes
vhdbuilder/packer/trivy-scan.sh
done

rm ./trivy
rm ./trivy
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need to delete trivy is the 1es agent will be destroyed ?

Are we reusing the same ADO agent between VHD build ? could we avoid redownloading it for every build in case we are reusing the same agent ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wouldn't this be better for version update flow?

Copy link
Collaborator Author

@jason1028kr jason1028kr Jul 25, 2024 •
edited

Choose a reason for hiding this comment

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

Unless ado agent pickup's deterministic, I think cleaning up the binary may be a good idea.

djsly
djsly requested changes Jul 9, 2024
View reviewed changes
vhdbuilder/packer/vhd-scanning-exe-on-vm.sh Outdated
--container-name ${SIG_CONTAINER_NAME} \
--name ${TRIVY_REPORT_NAME} \
--name ${TRIVY_REPORT_ROOTFS_NAME} \
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid the extra var and align the setting to be in place like you did for the blob upload below ?

--name ${IMAGE_REPORT}-${BUILD_ID}-${TIMESTAMP}.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do!

vhdbuilder/packer/vhd-scanning.sh Outdated
@@ -93,6 +93,8 @@ az vm run-command invoke \
TIMESTAMP=$(date +%s%3N)
TRIVY_REPORT_NAME="trivy-report-${BUILD_ID}-${TIMESTAMP}.json"
TRIVY_TABLE_NAME="trivy-table-${BUILD_ID}-${TIMESTAMP}.txt"
#"TRIVY_REPORT_NAME=${TRIVY_REPORT_NAME}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

remove if not needed

jason1028kr
jason1028kr commented Jul 22, 2024
View reviewed changes
vhdbuilder/packer/vhd-scanning.sh
"SIG_CONTAINER_NAME"=${SIG_CONTAINER_NAME} \
"STORAGE_ACCOUNT_NAME"=${STORAGE_ACCOUNT_NAME} \
"ENABLE_TRUSTED_LAUNCH"=${ENABLE_TRUSTED_LAUNCH}


az storage blob download-batch -d . --pattern "trivy-report-image-*-${BUILD_ID}-${TIMESTAMP}.json" -s ${SIG_CONTAINER_NAME} --account-name ${STORAGE_ACCOUNT_NAME} --auth-mode login
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to check batch works as expected

@jason1028kr jason1028kr marked this pull request as ready for review July 22, 2024 16:55
AlisonB319
AlisonB319 reviewed Jul 25, 2024
View reviewed changes
.pipelines/templates/.builder-release-template.yaml
- task: CopyFiles@2
condition: eq(variables['IS_NOT_1804'], 'true')
inputs:
SourceFolder: '$(System.DefaultWorkingDirectory)'
Contents: 'trivy-images-table.txt'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the table.txt all together? Wondering if it would be useful in the future if someone is manually looking at a run. Or would they just view their test build scans in kusto? This is assuming that the table.txt would have all the information that is in all of the per-container jsons (correct me if that would not be the case)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, we would still have an artifact for each container image scan. It's just that each appended table entry would be separated into individual json format. If you think having one table.txt file would be necessary, I can keep that logic on top of jsons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlisonB319 AlisonB319 AlisonB319 left review comments

@djsly djsly djsly requested changes

@juan-lee juan-lee Awaiting requested review from juan-lee juan-lee is a code owner

@cameronmeissner cameronmeissner Awaiting requested review from cameronmeissner cameronmeissner is a code owner

@UtheMan UtheMan Awaiting requested review from UtheMan UtheMan is a code owner

@ganeshkumarashok ganeshkumarashok Awaiting requested review from ganeshkumarashok ganeshkumarashok is a code owner

@anujmaheshwari1 anujmaheshwari1 Awaiting requested review from anujmaheshwari1 anujmaheshwari1 is a code owner

@Devinwong Devinwong Awaiting requested review from Devinwong Devinwong is a code owner

@lilypan26 lilypan26 Awaiting requested review from lilypan26 lilypan26 is a code owner

@ShiqianTao ShiqianTao Awaiting requested review from ShiqianTao ShiqianTao is a code owner

@AbelHu AbelHu Awaiting requested review from AbelHu AbelHu is a code owner

@junjiezhang1997 junjiezhang1997 Awaiting requested review from junjiezhang1997 junjiezhang1997 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jason1028kr @coveralls @djsly @AlisonB319