feat: add cli option for custom runtime path #633
Conversation
cryptaliagy
changed the title
There was a problem hiding this comment.
Thank you for the PR! I left a few comments which should help point you in the right direction for completing this.
One hurdle that might block progress is that the default scanner, trivy, only has partial support for setting the runtime socket, and the way in which the socket is set is different for each of the three supported runtimes.
I know this is a feature that you want, so If you have any questions I'm happy to provide more information on how we can make progress!
| enableProfile = flag.Bool("enable-pprof", false, "enable pprof profiling") | ||
| profilePort = flag.Int("pprof-port", 6060, "port for pprof profiling. defaulted to 6060 if unspecified") | ||
| runtimePtr = flag.String("runtime", "containerd", "container runtime") | ||
| isCustomRuntime = flag.Bool("custom-runtime", false, "assume runtime passed is local socket path") |
There was a problem hiding this comment.
I think splitting the behavior of a configuration value (runtimePtr) with a bool might introduce too much cognitive overhead to the end user. They then have to hold two different behaviors in their mind for a single variable.
Overall, I think it's better to allow the user to supply the socket path as a separate variable, for example
flag.String("socket-path", "", "path to the CRI runtime socket")If empty string, use the default location for the provided runtimePtr, else use the provided socket address.
| @@ -61,6 +62,14 @@ func main() { | |||
| } | |||
|
|
|||
| socketPath, found := util.RuntimeSocketPathMap[*runtimePtr] | |||
| if *isCustomRuntime { | |||
There was a problem hiding this comment.
See the above comment on socket-path.
| enableProfile = flag.Bool("enable-pprof", false, "enable pprof profiling") | ||
| profilePort = flag.Int("pprof-port", 6060, "port for pprof profiling. defaulted to 6060 if unspecified") | ||
| runtimePtr = flag.String("runtime", "containerd", "container runtime") | ||
| isCustomRuntime = flag.Bool("custom-runtime", false, "assume runtime passed is local socket path") |
There was a problem hiding this comment.
The custom socket address also needs to make its way to the collector and scanner pods.
| enableProfile = flag.Bool("enable-pprof", false, "enable pprof profiling") | ||
| profilePort = flag.Int("pprof-port", 6060, "port for pprof profiling. defaulted to 6060 if unspecified") | ||
| runtimePtr = flag.String("runtime", "containerd", "container runtime") | ||
| isCustomRuntime = flag.Bool("custom-runtime", false, "assume runtime passed is local socket path") |
There was a problem hiding this comment.
One other gotcha is that the configuration system recently moved from cli args to configmaps. That means that the end-user no longer has a way to set an arg on an eraser container directly, and it would need to be handled via the configmap and propagated down to the container (through this arg, or through an env var).
|
Abandoning this PR since it is hopelessly out-of-date with the release of 1.0.0, will give it another go using the ConfigMap approach |
What this PR does / why we need it: This PR adds in a CLI flag into the manager component so that it can interpret the runtime option as a literal path.
This will allow non-standard paths (e.g. with the bundled containerd in k3s) for container runtimes to be supported
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #594
Special notes for your reviewer: