-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(image): custom docker host option #3599
Conversation
e137ce4
to
34e7859
Compare
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.
as for testing: there is probably nothing that can be tested in docker_test
. However in image_test
you can set custom DockerHost
for test docker engine and then check if you can successfully reach it
Hey @AndreyLevchenko , Thank you so much for suggesting changes. Have tried my bit in improving the code as per the mentioned suggestions. Please take a look at it and let me know ! Thanks again. |
Not sure why the windows section in pipeline failed. Any ideas on this ? |
I think i got it, for windows, the domain socket url starts with |
There is lot of examples in trivy code. For instance https://github.com/aquasecurity/trivy/blob/main/pkg/misconf/scanner_test.go#L128 |
…socket path works on windows (aquasecurity#2997)
@AndreyLevchenko, can you trigger the pipeline to see if the current change works ? |
There is already a way to set the socket address for containerd, and it's done by environment variable. This PR splits the configuration of the socket: docker uses a cli arg and containerd uses an env var. What's more, the socket for the podman daemon can only be set indirectly by the It's a little awkward to have three different ways to configure the socket address for the three different supported runtimes. Would it be possible to expand the scope of this PR to include support for the other runtimes as well? You would probably also want to rename the flag from |
sure @pmengelbert , that makes total sense. I can maybe get into implementing your suggestion right after we figure out how to resolve the test for windows environment. |
I feel like |
@knqyf263 @pmengelbert I think we can limit the scope of this issue / PR to support @AndreyLevchenko Also the pipeline is green RN. Please review and accept the PR or suggest changes if any. Thanks ! |
@knqyf263 Since the code looks for the image by first attempting the docker socket, then the containerd socket, then podman, then remote registry, I think you are right that there should be either a) separate flags or, b) The other issue is, what happens if users supply both the flag and the env var? I assume the CLI arg takes precedence.
I'll defer to @knqyf263 but IMO this PR should at least allow setting the socket address by environment variable, since users can do so already with containerd and podman. Thoughts? |
I guess all 3 container runtimes can be configured by ENV vars already. The scope of this issue was to add a flag to control the host for local docker engine scan. |
or
The first one may be more intuitive.
Yes, CLI flags should take precedence. I believe trivy/pkg/fanal/image/daemon/docker.go Line 20 in c447d1c
Right. |
So shall I start working on these changes ? I have one doubt though. Let's assume this scenario. If the user provides |
Related #3049 |
Any updates on this comment ? I can work accordingly if we can make a decision ! |
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 recently refactored options, and it causes conflicts. It is just bothering you, so I've resolved them.
No, I don't think so. |
Thanks for your patience! |
Description
This is a draft. Added custom docker socket option
--docker-host
. Please review and suggest changes.Related issues
Checklist