Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Switch Dockerfile image to wolfi and add pipeline for vulnerability scanning #3063
Switch Dockerfile image to wolfi and add pipeline for vulnerability scanning #3063
Changes from 1 commit
a1b50bd
30f59fd
53ae3e1
2334402
157b8e7
fa7c0b0
c9a5e46
bfd0ef8
c243aa4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
wondering if we should consider running this less frequently? I think it would depend on what @elastic/search-extract-and-transform prefer, but the scenario I picture is something like:
let's see what folks think 👍
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 these notifications could potentially be a duplicate of existing Snyk issues detected on the official container image. What's the value added by these new notifications?
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.
is Snyk going to scan the images produced by these CI jobs?
I think this PR is doing 3 things, but maybe doesn't need to.
Dockerfile
to use chainguard (has customer value)Dockerfile
works (has customer value)I think (3) could probably be done separately, and reuse whatever machinery we use to scan other artifacts, especially since (1) and (2) will significantly cut down on false-positives that we'd have if we tried to do (3) on its own today.
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.
The CI jobs in this PR do not push images to the docker registry, instead it relies on Trivy for vulnerability scanning, which runs within the context of the pipeline.
The alternative approach is to push the images to an internal namespace on the docker registry and request for these to be added to Snyk. This would result in duplicate reports though as we're already scanning the
docker.elastic.co/integrations/elastic-connectors
with Snyk built from the Dockerfile.wolfi image which is technically the exact same base OS build.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.
...
These are really good points that I hadn't considered. Given that with this PR we are:
Dockerfile
to become distroless, and making the resultant image considerably more secureDockerfile
essentially the same as theDockerfile.wofli
, the latter of which produces the images that are pushed and scanned by Snykgit
andmake
👍)... then I actually don't see much value in adding the Trivy step (let alone adding notifications based on its results). I wish I'd had these realizations sooner, apologies @kostasb.
What do you all think?
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.
@kostasb thanks, I like the idea of leaving the Trivy step there, but not having it send any notifications. But I'll defer to E&T folks (@seanstory @artem-shelkovnikov) to say what they prefer 👍
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 objection to leaving it (without notifications) but also I don't think a human will be manually looking at the outputs often. So if we're ok as treating it essentially as a source of debug logs that may or may not be examined, great.
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 understand the following might be a big direction change for this PR, but considering the feedback above, I think it might be worthy to explore: what if, instead of adding a new pipeline (with informative value only), we add the Trivy scan step to the existing pipeline? I think the informative value would be higher for developers working in PRs: the Trivy scan would be part of their feedback loop, and not something relegated to a different pipeline.
Similarly to what we did for ent-search, I would run the Trivy scan step in PRs to
main
, and on merge onmain
and other current release branches. What do you all think?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.
Merging the extensible Dockerfile build/test/scan steps into the main pipeline makes sense to me, we can proceed this way if the team agrees.
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 pushed a commit that merges the extensible Dockerfiles pipeline into the main pipeline (easy to revert if the team @seanstory @artem-shelkovnikov has a different opinion after all).
As a bonus point, this also enables testing as part of this PR (the new dedicated pipeline wouldn't be picked up until after merged).