Configure trivy to scan local images instead of downloading them again #8140

jandubois · 2025-01-24T06:25:33Z

Run as root so trivy has access to the containerd socket.

Don't output the trivy JSON to the browser; this is extremely slow when the output is several megabytes.

Also don't output the trivy JSON to images.log, as it makes the file unwieldy.

On the first scan trivy needs to download trivy.db, which is almost 600MB. Subsequent scans are then almost instantaneous. Even results for python:latest show up in a second, which used to take almost a minute when the JSON was output to the browser.

Fixes #539

mook-as

In general most of my complaints stem from setting the environment inline instead of passing it in as a mapping.

mook-as · 2025-01-24T18:16:06Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

   * @param taggedImageName
+   * @param namespace


Please actually document the arguments, or just drop the JSDoc bits if you don't want to do that. (It's generally better to add docs, of course…)

mook-as · 2025-01-24T18:23:43Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

-   * Run trivy with the given arguments; the first argument is generally a
-   * subcommand to execute.
+   * Run trivy inside the VM with the given arguments; the 'trivy' command must be included
+   * in the args, so it can be called like `CONTAINERD_NAMESPACE=k8s.io trivy image ...`.


So the only reason this exists (instead of having the concrete class calling this.executor.spawn directly) is to call processChildOutput?

It's probably better to take env as a second argument (or first), instead of setting it as part of the arg… or even just take SpawnOptions.

I don't think this.executor.spawn supports env options. And even if it did, they would not survive the sudo call without extra effort.

This will require a ton of changes and not solve the underlying issue that processChildOutput is not designed to be called with anything but docker or nerdctl.

It doesn't support changing the environment, but that's why we have PRs… How about we at least call this something other than runTrivyCommand (say, runTrivyImageCommand) so it's not pretending to be more generic than it is? If we specialize it just for that it's less confusing.

Or just drop this wrapper and have the caller do things manually. But I think just making this function specific to trivy image is fine.

mook-as · 2025-01-24T18:24:39Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

+        // don't dump megabytes of trivy JSON output into the browser
+        if (sendNotifications && commandName !== 'trivy') {


Shouldn't the caller set sendNotifications correctly instead of this weird hack?

I don't understand the logic behind sendNotifications, but it is later used to process the output. If you set it to false, then the output is discarded.

mook-as · 2025-01-27T19:11:59Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

@@ -255,20 +249,23 @@ export abstract class ImageProcessor extends EventEmitter {
   */
  async processChildOutput(child: ChildProcess, subcommandName: string, sendNotifications: boolean, args?: string[]): Promise<childResultType> {
    const result = { stdout: '', stderr: '' };
+    const commandName = args ? 'trivy' : this.processorName;


This is going to be so confusing when we start trying to use args for one of the non-trivy commands…

Yeah, but you won't. This is already a hack. args for other commands are not passed in at all, except for subcommandName, which is needed for another hack to treat nerdctl images differently.

No, subcommandName (per the method docs) is for displaying to the user; the fact that it's used for nerdctl images is the hack, but we still need to preserve it for its primary purpose.

A better way to do this would be to just pass it in:

type ProcessChildOutputOptions = { commandName?: string; subcommandName: string; sendNotifications: boolean; } async processChildOutput(child: ChildProcess, options: ProcessChildOutputOptions, args?: string[]): Promise<childResultType> { const commandName = options.commandName ?? this.processorName;

jandubois

I can update the comments, but the rest is not reasonable without a complete refactoring of this package.

jandubois · 2025-01-27T23:15:13Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

-   * Run trivy with the given arguments; the first argument is generally a
-   * subcommand to execute.
+   * Run trivy inside the VM with the given arguments; the 'trivy' command must be included
+   * in the args, so it can be called like `CONTAINERD_NAMESPACE=k8s.io trivy image ...`.


I don't think this.executor.spawn supports env options. And even if it did, they would not survive the sudo call without extra effort.

This will require a ton of changes and not solve the underlying issue that processChildOutput is not designed to be called with anything but docker or nerdctl.

jandubois · 2025-01-27T23:16:46Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

@@ -255,20 +249,23 @@ export abstract class ImageProcessor extends EventEmitter {
   */
  async processChildOutput(child: ChildProcess, subcommandName: string, sendNotifications: boolean, args?: string[]): Promise<childResultType> {
    const result = { stdout: '', stderr: '' };
+    const commandName = args ? 'trivy' : this.processorName;


Yeah, but you won't. This is already a hack. args for other commands are not passed in at all, except for subcommandName, which is needed for another hack to treat nerdctl images differently.

jandubois · 2025-01-27T23:17:57Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

+        // don't dump megabytes of trivy JSON output into the browser
+        if (sendNotifications && commandName !== 'trivy') {


I don't understand the logic behind sendNotifications, but it is later used to process the output. If you set it to false, then the output is discarded.

mook-as

This is piling on too much short-term hacks, which would make things unmaintainable.

If the decision is that we will not maintain this, then we should just not take this PR…

mook-as · 2025-01-28T01:39:53Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

-   * Run trivy with the given arguments; the first argument is generally a
-   * subcommand to execute.
+   * Run trivy inside the VM with the given arguments; the 'trivy' command must be included
+   * in the args, so it can be called like `CONTAINERD_NAMESPACE=k8s.io trivy image ...`.


It doesn't support changing the environment, but that's why we have PRs… How about we at least call this something other than runTrivyCommand (say, runTrivyImageCommand) so it's not pretending to be more generic than it is? If we specialize it just for that it's less confusing.

Or just drop this wrapper and have the caller do things manually. But I think just making this function specific to trivy image is fine.

mook-as · 2025-01-28T01:47:51Z

pkg/rancher-desktop/backend/images/imageProcessor.ts

@@ -255,20 +249,23 @@ export abstract class ImageProcessor extends EventEmitter {
   */
  async processChildOutput(child: ChildProcess, subcommandName: string, sendNotifications: boolean, args?: string[]): Promise<childResultType> {
    const result = { stdout: '', stderr: '' };
+    const commandName = args ? 'trivy' : this.processorName;


No, subcommandName (per the method docs) is for displaying to the user; the fact that it's used for nerdctl images is the hack, but we still need to preserve it for its primary purpose.

A better way to do this would be to just pass it in:

type ProcessChildOutputOptions = { commandName?: string; subcommandName: string; sendNotifications: boolean; } async processChildOutput(child: ChildProcess, options: ProcessChildOutputOptions, args?: string[]): Promise<childResultType> { const commandName = options.commandName ?? this.processorName;

mook-as · 2025-01-28T01:48:44Z

pkg/rancher-desktop/backend/images/imageProcessor.ts


-    return await this.processChildOutput(child, subcommandName, sendNotifications, args);
+    return await this.processChildOutput(child, subcommandName, true, args);


Suggested change

return await this.processChildOutput(child, subcommandName, true, args);

return await this.processChildOutput(child, subcommandName, false, args);

Why the heck are we passing true here for sendNotifications only to add a hack to disable its effect when processing the argument‽

If you're doing this so we get stderr but not stdout, then we need separate flags for those.

mook-as · 2025-01-28T01:51:37Z

pkg/rancher-desktop/backend/images/nerdctlImageProcessor.ts

    return await this.runTrivyCommand(
      [
+        'CONTAINERD_ADDRESS=/run/k3s/containerd/containerd.sock',


Has this been tested on Windows? You're not going through sudo (nor env) there.

mook-as

I've force-pushed the code into what I'd like to see.
@jandubois please check if you're okay with that, and if yes merge it (/set auto-merge)?

Run as root so trivy has access to the containerd socket. Don't output the trivy JSON to the browser; this is extremely slow when the output is several megabytes. Also don't output the trivy JSON to images.log, as it makes the file unwieldy. Co-authored-by: Mark Yen <[email protected]> Signed-off-by: Jan Dubois <[email protected]> Signed-off-by: Mark Yen <[email protected]>

jandubois marked this pull request as draft January 24, 2025 17:04

jandubois force-pushed the trivy branch from b4f50b1 to e9bb756 Compare January 24, 2025 17:30

jandubois marked this pull request as ready for review January 24, 2025 17:30

jandubois force-pushed the trivy branch 2 times, most recently from 4f695e2 to 9db372d Compare January 24, 2025 17:42

mook-as self-requested a review January 24, 2025 17:58

jandubois force-pushed the trivy branch from 9db372d to 175259a Compare January 24, 2025 23:10

mook-as requested changes Jan 27, 2025

View reviewed changes

jandubois commented Jan 27, 2025

View reviewed changes

jandubois force-pushed the trivy branch from 5da866e to db9d915 Compare January 27, 2025 23:39

mook-as requested changes Jan 28, 2025

View reviewed changes

mook-as force-pushed the trivy branch from db9d915 to 7e7370f Compare January 28, 2025 20:15

mook-as previously approved these changes Jan 28, 2025

View reviewed changes

jandubois dismissed mook-as’s stale review via 4ff11a7 January 31, 2025 22:54

jandubois force-pushed the trivy branch from 7e7370f to 4ff11a7 Compare January 31, 2025 22:54

mook-as approved these changes Jan 31, 2025

View reviewed changes

jandubois merged commit d5260b6 into rancher-sandbox:main Jan 31, 2025
20 checks passed

jandubois deleted the trivy branch February 3, 2025 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure trivy to scan local images instead of downloading them again #8140

Configure trivy to scan local images instead of downloading them again #8140

jandubois commented Jan 24, 2025 •

edited

Loading

mook-as left a comment

mook-as Jan 24, 2025

mook-as Jan 24, 2025

jandubois Jan 27, 2025

mook-as Jan 28, 2025

mook-as Jan 24, 2025

jandubois Jan 27, 2025

mook-as Jan 27, 2025

jandubois Jan 27, 2025

mook-as Jan 28, 2025

jandubois left a comment

jandubois Jan 27, 2025

jandubois Jan 27, 2025

jandubois Jan 27, 2025

mook-as left a comment

mook-as Jan 28, 2025

mook-as Jan 28, 2025

mook-as Jan 28, 2025

mook-as Jan 28, 2025

mook-as left a comment

		// don't dump megabytes of trivy JSON output into the browser
		if (sendNotifications && commandName !== 'trivy') {


		return await this.processChildOutput(child, subcommandName, sendNotifications, args);
		return await this.processChildOutput(child, subcommandName, true, args);

Configure trivy to scan local images instead of downloading them again #8140

Configure trivy to scan local images instead of downloading them again #8140

Conversation

jandubois commented Jan 24, 2025 • edited Loading

mook-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mook-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mook-as left a comment

Choose a reason for hiding this comment

jandubois commented Jan 24, 2025 •

edited

Loading