-
Notifications
You must be signed in to change notification settings - Fork 89
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
Generate source code SBOM in 'context' mode #1430
base: master
Are you sure you want to change the base?
Conversation
Change-Id: Idf989734e10c4329d703fb9295c18efe39043ae3
😊 Welcome @sergii-ssh! This is either your first contribution to the Istio release-builder repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
|
pkg/build/sbom.go
Outdated
@@ -26,42 +26,40 @@ import ( | |||
"istio.io/release-builder/pkg/util" | |||
) | |||
|
|||
// Sbom generates Software Bill Of Materials for istio repo in an SPDX readable format. | |||
const ( | |||
sourceSpdx string = "istio-source.spdx" |
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 was wondering if we should have version appended to the file name... looks like all other artifacts have this. @ericvn thoughts?
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, is the spdx file created on a per-artifact basis or for the project as a whole? If per-artifact, maybe the name should be included as infix as well.
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.
Good idea, I'll add the version name to the file name.
spdx is create for build/project, i.e combined spdx is generated for all docker tars
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.
We don't version manifest.yaml or a few others fwiw. Its already in a per-version folder (https://gcsweb.istio.io/gcs/istio-release/releases/1.16.0).
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.
but I am fine with it as well
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.
updated, ptal
Change-Id: I2e22fb501ff2e44fdc1fcfb561b509f292613a37
return fmt.Errorf("couldn't generate sbom for istio release artifacts: %v", err) | ||
// For Docker output in 'context' mode we will not produce SBOM. | ||
// `bom` can produce bill only for tar and remote images. | ||
if manifest.DockerOutput == model.DockerOutputTar { |
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 am wondering where we are running SBOM in 'context' mode as the PR title says.
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.
Currently we don't call bom at all if context mode is specified. I am enabling it here https://github.com/istio/release-builder/pull/1430/files#diff-e3115b8ca9fdf611ae3dc61dc235e39aeeddbca61036b803ba3c0826f25cc3c9L91
For this snippet. I am skipping sbom generation for docker artifacts since bom
doesn't support local context/registries but generate one for the source code.
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.
OK, so for source code we need to generate SBOM, as docker output mode does not matter in that case. Previously we were fully skipping. @puerco is this correct, that bom cannot support local registries? I remember you mentioning that it also should 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.
@puerco is it possible to generate bom for local registers without exporting to tar?
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.
Could you please clarify how the PR title "[Run SBOM in 'context' mode]" makes sense now? You run it in context mode, but the SBOM is generated only for source code right?
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.
Updated the title to highlight that explicitly.
// For Docker output in 'context' mode we will not produce SBOM. | ||
// `bom` can produce bill only for tar and remote images. |
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.
@sergii-ssh What happens when you build in "context" mode? Happy to learn about the use case if you think a feature is missing. Or is it for local development?
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.
hi @puerco In context
mode images we build are stored in local registry/context, i.e. gcr.io/istio-release/pilot:1.17.2
. Now if I run bom generate -o istio-release.spdx --image gcr.io/istio-release/pilot:1.17.2
it will fail because image hasn't been pushed to remote registry yet.
FATA generating doc: creating SPDX document: generating SPDX package from image ref gcr.io/istio-release/pilot:1.17.2: while downloading images to archive: fetching remote descriptor: GET https://gcr.io/v2/istio-release/pilot/manifests/1.17.2: MANIFEST_UNKNOWN: Failed to fetch "1.17.2" from request "/v2/istio-release/pilot/manifests/1.17.2".
Workaround is to docker save
and then run with --image-archive
but it would be nice to have something like --image-local
flag to avoid doing so.
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.
Can we not point to the images that were created when in context (that's a bad name) mode? They won't been the final location. Or is the SBOM going to end up with inaccurate data? Maybe it would be accurate if we set sbomOutputURI
to the correct value.
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.
SBOM location is configurable in manifest.BillOfMaterialsURI
and filenames are stamped with version. Accuracy shouldn't be an issue unless spdx report overwriten on purpose.
@@ -26,42 +26,38 @@ import ( | |||
"istio.io/release-builder/pkg/util" | |||
) | |||
|
|||
// Sbom generates Software Bill Of Materials for istio repo in an SPDX readable format. | |||
const ( | |||
sbomOutputURI string = "https://storage.googleapis.com/istio-release/releases" |
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.
We generate images to multiple places: We may also put them in istio-prerelease-testing
or istio-testing
depending on what we are building. That said I see we seem to use a single URI in the replaced code.
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.
It was hardcoded so I am keeping it not to break anything. This is the default unless manifest.BillOfMaterialsURI is specified.
I suspect sbom location needs to be specified here as well? https://github.com/istio/release-builder/blob/master/release/build.sh#L38
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.
Updated build script. Now we'll upload SBOMs to the same bucket as other artifacts.
// For Docker output in 'context' mode we will not produce SBOM. | ||
// `bom` can produce bill only for tar and remote images. |
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.
Can we not point to the images that were created when in context (that's a bad name) mode? They won't been the final location. Or is the SBOM going to end up with inaccurate data? Maybe it would be accurate if we set sbomOutputURI
to the correct value.
Change-Id: I2d3b13b82e23dd80d46898d9a0e48c05e66b8e39
LGTM |
if manifest.DockerOutput == model.DockerOutputContext { | ||
log.Warnf("Docker output in 'context' mode; will not produce SBOM.") | ||
} else if manifest.SkipGenerateBillOfMaterials { | ||
if manifest.SkipGenerateBillOfMaterials { |
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.
We create a new option, SkipGenerateBillOfMaterials, and we output a message if set, but I don't see that we actually act on it. I would have expected some check around the actual BOM generation to not run if this value is set.
"--image-archive", strings.Join(dockerImages, ","), "--output", releaseSbomFile).Run(); err != nil { | ||
return fmt.Errorf("couldn't generate sbom for istio release artifacts: %v", err) | ||
// For Docker output in 'context' mode we will not produce SBOM. | ||
// `bom` can produce bill only for tar and remote images. |
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.
We used to generate a message that BOM generation wouldn't happen if we were in context mode and removed that above. If we are in context mode and have not specified to skip the generation, we will not have generated an SBOM, and there would have been no messages in the log that SBOM generation was skipped.
@@ -36,6 +36,7 @@ fi | |||
|
|||
PRERELEASE_DOCKER_HUB=${PRERELEASE_DOCKER_HUB:-gcr.io/istio-prerelease-testing} | |||
GCS_BUCKET=${GCS_BUCKET:-istio-prerelease/prerelease} | |||
SBOM_OUTPUT_URI="https://storage.googleapis.com/${GCS_BUCKET}/releases" |
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.
Do we want this value to be overwrite able as it isn't as written here? If not, then we could include it in the URI below and save a line.
Create SBOM even if the docker output set to context just skip scanning tars.
Add
BillOfMaterialsURI
property to overwrite default location for SBOM