-
Notifications
You must be signed in to change notification settings - Fork 594
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
feat(detectExecuteScan): Also scan images that are in the CPE #4678
Conversation
f176377
to
9a334ae
Compare
/it |
252f89c
to
c402207
Compare
ab38479
to
fe3f3b4
Compare
d0682d3
to
e805329
Compare
5d76dcf
to
606a068
Compare
606a068
to
3fdefae
Compare
/it-go |
args = append(args, "--detect.docker.passthrough.shared.dir.path.local=/opt/blackduck/blackduck-imageinspector/shared/") | ||
args = append(args, "--detect.docker.passthrough.shared.dir.path.imageinspector=/opt/blackduck/blackduck-imageinspector/shared") | ||
args = append(args, fmt.Sprintf("--detect.docker.passthrough.imageinspector.service.distro.default=%s", config.ContainerDistro)) | ||
args = append(args, "--detect.docker.passthrough.imageinspector.service.start=false") |
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.
Here it is indicating that the inspector itself will not need to be started, in that case how the image inspector is being started and exposed ?
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 should be started as a sidecar container by the Piper step infrastructure. See https://github.com/SAP/jenkins-library/pull/4678/files/3fdefae8c191e82862b24148a873a3a84acbc928#diff-da165ed834ab60bb2fc7bc2fed1e93646b0f36264f16e79b160b4361781207efR596
3fdefae
to
927d34b
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
cmd/detectExecuteScan.go
Outdated
return errors.Wrap(err, "Unable to read cpe") | ||
} | ||
|
||
registryUser := piperenv.GetResourceParameter(cpePath, "container", "repositoryUsername") |
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.
How repositoryUsername
and repositoryPassword
are created under CPE container path.
Looks like KanikoExecute only generates imageNameTags.json
and registryUrl
but not username/password
. We referred this - https://github.com/SAP/jenkins-library/blob/master/resources/metadata/kanikoExecute.yaml#L309
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, that should be given by the staging service step: https://github.wdf.sap.corp/ContinuousDelivery/piper-library/blob/51fe98e7f596950034475b3e4c7f3bd417f4a9dc/resources/metadata/sapCallStagingService.yaml#L274-L276
cc @anilkeshav27 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.
Thanks @loewenstein.
Tried with the combination of kanikoExecute
and sapCallStagingService
For generating repositoryUsername/Password. But we are getting failure message as Wrong action.
Could you please suggest the right configuration for generating the credentials file under CPE container path.
cc @anilkeshav27 @c0d1ngm0nk3y
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 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 do we need to read a CPE value from the file system, instead we could allow new params and have cpe references like ? https://github.com/SAP/jenkins-library/pull/4804/files#diff-da165ed834ab60bb2fc7bc2fed1e93646b0f36264f16e79b160b4361781207ef
with the new params, we also introduce the possibility to scan images that run outside the build, if the image lives in a private registry then users can bring in their registry url, password (maybe we would need a vault reference as well) and username . will be a good selling feature 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.
@loewenstein It sounds good to extend support for external registries as well. With this enhancement, the behaviour of both Mend and DetectScan will align with each other.
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 second to use another PR for any additional features
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 leave the final decision to @t-vijayan , who owns the piper step , since resourceRef
is the standard way to read the CPE values
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 do we need to read a CPE value from the file system, instead we could allow new params and have cpe references like?
Because we need 4 values from the cpe
: user, password, registry AND images. Since images is an array and can't be mapped (right?), it would be pointless and very confusing to expose those parameters. If we can map all 4 to the cpe
, it would be preferable over reading it manually of course.
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.
yes we should be able to map all 4 cpe the imageNameTags
is the tricky bit , since there can be one image or multiple build, if it ties back to the cnbBuild
/ kaniko
and if we fill this cpe
jenkins-library/resources/metadata/cnbBuild.yaml
Lines 369 to 370 in 4e3fa38
- name: container/imageNameTags | |
type: "[]string" |
i know in kaniko
we fill this list even in single image is built and i am assuming its the same in cnbBuild
cmd/detectExecuteScan.go
Outdated
return errors.Wrap(err, "Unable to read cpe") | ||
} | ||
|
||
registryUser := piperenv.GetResourceParameter(cpePath, "container", "repositoryUsername") |
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 do we need to read a CPE value from the file system, instead we could allow new params and have cpe references like ? https://github.com/SAP/jenkins-library/pull/4804/files#diff-da165ed834ab60bb2fc7bc2fed1e93646b0f36264f16e79b160b4361781207ef
with the new params, we also introduce the possibility to scan images that run outside the build, if the image lives in a private registry then users can bring in their registry url, password (maybe we would need a vault reference as well) and username . will be a good selling feature as well ?
927d34b
to
851022c
Compare
We tested it with no |
4df39b8
to
4d83e0c
Compare
/it-go |
f171d11
to
1789f0f
Compare
/it-go |
@anilkeshav27 @t-vijayan @vijayanjay Anything we could or should do? From our perspective, this is good to go. |
I still want to look at this comment - I think this is still suboptimal. |
5043035
to
be744f3
Compare
/it-go |
be744f3
to
fb51f13
Compare
Co-authored-by: Johannes Dillmann <[email protected]> Signed-off-by: Ralf Pannemans <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Pavel Busko <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]> Signed-off-by: Ralf Pannemans <[email protected]> Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Johannes Dillmann <[email protected]> Signed-off-by: Johannes Dillmann <[email protected]> Co-authored-by: Ralf Pannemans <[email protected]>
fb51f13
to
1dd12f6
Compare
/it-go |
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.
lgtm, i am not sure about the actual scan logic and the error handling , but i trust that this has been tested well enough
from the overall cpe auto wiring and side car logic it looks good to me
cc @t-vijayan @vijayanjay would also share your thoughts
Signed-off-by: Pavel Busko <[email protected]> Co-authored-by: Ralf Pannemans <[email protected]> Signed-off-by: Ralf Pannemans <[email protected]> Co-authored-by: Pavel Busko <[email protected]> Signed-off-by: Pavel Busko <[email protected]>
1dd12f6
to
bd957bb
Compare
Quality Gate passedIssues Measures |
/it-go |
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.
@c0d1ngm0nk3y @anilkeshav27 With the latest changes able to trigger detect scan for the docker images built by kaniko and able to verify the scan results.
changes looks good to me.
@anilkeshav27 The failing integration tests seem not to be related at the first glance. WDYT? Also the the mention |
feat(detectExecuteScan): Also scan images that are in the cpe Signed-off-by: Ralf Pannemans <[email protected]> Signed-off-by: Johannes Dillmann <[email protected]> Signed-off-by: Pavel Busko <[email protected]> Co-authored-by: Johannes Dillmann <[email protected]> Co-authored-by: Pavel Busko <[email protected]>
This has following prerequisites to
jenkins-library
(cherry-picked):sidecars
to haveconditions
volumeMount
support tosidecars
The enhances
detectExecuteScan
to not only scan the workspace, but also all images that can be found in thecpe
. IfkanikoExecute
orcnbBuild
produce images, the information will be written to thecpe
. In order to do these scans, a sidecar is needed. The sidecar is different for the distro that is scanned. The user can configure this via the new parametercontainerDistro
(default "ubuntu").The user can can use
scanImages: false
to prevent this.Changes