-
Notifications
You must be signed in to change notification settings - Fork 86
CLOUDP-315271: Onboard Kundukto to CI #3862
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
base: CLOUDP-227276_ssdlc_phase_2
Are you sure you want to change the base?
CLOUDP-315271: Onboard Kundukto to CI #3862
Conversation
build/package/.goreleaser.yml
Outdated
@@ -55,6 +55,7 @@ archives: | |||
- LICENSE | |||
- LICENSING-NOTES.txt | |||
- third_party_notices/**/* | |||
- compliance/**/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directory currently holds the generated SBOM.
It will also hold the generated SSDLC report after future work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no expectations to produce this report with the release, any reason we are including it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the scope we have as a goal to include SSDLC report as a part of release.
The rationale is that releases provide a convenient place to store the report and enable customers to access the report on release. And then up-to-date reports with augmented SBOMs can be provided to customers on request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions about the utility of packaging it with the bin, I don't see any other binary doing it, AKO for example is commited to the repo and not shipped
Then if we really want to ship it then I wonder why not use https://goreleaser.com/customization/sbom/#usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmenezes any opinion on shifting approach of where we store the report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just got off a call with @cveticm basically there are 3 approaches:
- Store on main/master (used by https://github.com/mongodb/mongodb-atlas-kubernetes)
- Store in the release package (used by ops manager)
- Store in github releases page (which it would be used by CLI/LocalDev)
The reason why we wanted to do on releases page was to make it same for CLI and Local Dev, also bear in mind our policy does not specify where to store these and most projects simply upload to S3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also bear in mind our policy does not specify where to store these
Yes and my initial point, there's no expectation to be proactive about these and only to make it available upon request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store in the release package (used by ops manager)
Thanks for reminding me of this I downloaded OM to see where they are but since it's 2GB package I forgot to check
in ops manager they are included in the notices folder, which in the CLI is the thirdparty notice folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting the compliance report on the GitHub release page makes the most sense. It allows customers to grab the report easily before downloading any packages.
I also think that being proactive about making these reports available could reflect well on our SSDLC policy to customers.
Let me know if either of you have strong objections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's proceed with the releases page we might amend README/docs about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general design comment, can we commit the purl file and make it a CI check (GH or EVG no preference) that is kept up to date, this is similar to how other repos manage this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check the library owners check which is kind of similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would't go for this approach we would jump from 2 files storing dependency information (library_owners.json and go.mod) to 3 files (library_owners.json, go.mod and purls.txt).
I would prefer for the tooling to translate dependencies into purls and later json on the fly as needed.
I'm inclined to even remove library_owners.json, given only our team has code in atlasCLI since the kubernetes plugin extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to remove lib owners (same reason) but I see value on purls being committed given shipped dependencies are not the same as the ones in go.mod and this raises awareness when adding new libs and the implications, this also comes with the comment that I'd like if purls are auto generated by the precommit hook, similar to mms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong opinion here. Having the purl file committed could definitely help with making dependencies more visible, but I see @fmenezes's point about not wanting to manage yet another file.
I can create a make command to generate purls from the binary and set up a GH action to check that it's up-to-date similar to how we do the docs check.
@fmenezes, any strong feelings about this idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure that is fine, we can commit it but bear in mind the extra checks
build/package/generate-sbom.sh
Outdated
echo "Generating PURLs..." | ||
cd "$WORKDIR/src/github.com/mongodb/mongodb-atlas-cli" | ||
|
||
go build -C cmd/atlas -o tmp_binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use make build
git is already set up to ignore the files generated by that command, and that build process is hook to generate a similar binary to what we ship with go releaser
build/ci/release.yml
Outdated
- workdir | ||
binary: make | ||
args: | ||
- gen-purls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't do it anymore, we should make sure the file is fully updated
build/package/generate-sbom.sh
Outdated
# Authenticate Docker to AWS ECR | ||
aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 901841024863.dkr.ecr.us-east-1.amazonaws.com | ||
|
||
cd "$WORKDIR/src/github.com/mongodb/mongodb-atlas-cli" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this we can set working dir at evergreen function
build/package/generate-sbom.sh
Outdated
aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 901841024863.dkr.ecr.us-east-1.amazonaws.com | ||
|
||
cd "$WORKDIR/src/github.com/mongodb/mongodb-atlas-cli" | ||
mkdir ./compliance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for empty folders you can commit a complicance/.gitkeep
empty file to keep folder on source code
build/ci/release.yml
Outdated
- AWS_SESSION_TOKEN | ||
- workdir | ||
binary: build/package/generate-sbom.sh | ||
"write kondukto credentials": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge "write kondukto credentials"
and "run silkbomb"
and make sure to add a rm kondukto_credentials.env
in the end
@@ -533,6 +533,21 @@ functions: | |||
binary: make | |||
args: | |||
- otel | |||
"check purls": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of check failing if purls.txt is not up-to-date:
https://spruce.mongodb.com/task/mongodb_atlas_cli_master_code_health_check_purls_patch_36f21cd8defe25ad9c89cf4d1d09f424e62cff55_681485dc659ffc00070b41ef_25_05_02_08_44_17/logs?execution=0&logtype=all
.PHONY: gen-purls | ||
gen-purls: # Generate purls on linux os | ||
@echo "==> Generating purls" | ||
GOOS=linux GOARCH=amd64 go build -trimpath -mod=readonly -o bin/atlas-linux ./cmd/atlas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why hardcode to linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some dependencies which are used only by some builds. For the moment, we will only generate purls for linux (in the interest of reducing this PRs scope) and will include generation for all builds, compiling them into one list which will be used for sbom generation in CLOUDP-316920
build/ci/check-purls.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to /scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is only run as part of CI and other scripts live in this dir such as check-licenses.sh so IMO it makes more sense to keep this script within ./build/ci
@@ -149,6 +149,15 @@ gen-docs: gen-docs-metadata ## Generate docs for atlascli commands | |||
@echo "==> Generating docs" | |||
go run -ldflags "$(LINKER_FLAGS)" ./tools/cmd/docs | |||
|
|||
.PHONY: gen-purls | |||
gen-purls: # Generate purls on linux os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will hooking this to the pre commit be a separate task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't intended to since, as a part of CI, we do a check to see if purls.txt is up-to-date. Devs would have to manually run this command. My intent here was to give devs visual on changes to sbom because of their work.
I'm happy to include this command in pre commit. To verify @gssbzn, this would make the CI check is redundant, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would make the CI check is redundant, correct?
No cause having the pre commit installed is opt-in, and you can also do git commit --no-verify
so you still need to check the files is up to date on the CI
Also just inc ase, ideally this should only run if the go.mod is modified
Proposed changes
Adds new evergreen task "generate_and_upload_sbom" to generate an SBOM and package it in the release.
Adds new file build/ci/purls.txt which is a list of purls built from the binary.
This step of work does the following:
compliance/.
This PR does not:
Jira ticket: CLOUDP-315271
Checklist
make fmt
and formatted my code