fix(k8s)!: support k8s multi container

[smtan-gl]

Description

When using Trivy k8s, the current report metadata field for workloads with multiple containers only includes metadata from one image. This update changes the metadata field to an array, allowing it to capture metadata from all the scanned images within the workload.

This is a draft PR to gather feedback on whether this approach is acceptable, and I will update the tests accordingly thereafter.

Report before change

{
  "ClusterName": "",
  "Findings": [
    {
      "Namespace": "default",
      "Kind": "Pod",
      "Name": "nginx-fluentd-pod",
      "Metadata": {
        "OS": {
          "Family": "debian",
          "Name": "12.6"
        },
          "RepoTags": [
            "nginx:latest"
          ],
          "DiffIDs": []
      },
      "Results": [
        {
          "Target": "nginx:latest (debian 12.6)",
          "Class": "os-pkgs",
          "Type": "debian",
          "Packages": [],
          "Vulnerabilities": []
        },
        {
          "Target": "fluent/fluentd:v1.17-armhf-debian (debian 12.6)",
          "Class": "os-pkgs",
          "Type": "debian",
          "Packages": [],
          "Vulnerabilities": []
        },
        {
          "Target": "Ruby",
          "Class": "lang-pkgs",
          "Type": "gemspec",
          "Packages": [],
          "Vulnerabilities": []
        }
      ]
    }
  ]
}

Report after change

{
  "ClusterName": "",
  "Findings": [
    {
      "Namespace": "default",
      "Kind": "Pod",
      "Name": "nginx-fluentd-pod",
      "Metadata": [
        {
          "OS": {
            "Family": "debian",
            "Name": "12.6"
          },
          "RepoTags": [
            "nginx:latest"
          ],
          "DiffIDs": []
        },
        {
          "OS": {
            "Family": "debian",
            "Name": "12.6"
          },
          "RepoTags": [
            "fluent/fluentd:v1.17-armhf-debian"
          ],
          "DiffIDs": []
        }
      ],
      "Results": [
        {
          "Target": "nginx:latest (debian 12.6)",
          "Class": "os-pkgs",
          "Type": "debian",
          "Packages": [],
          "Vulnerabilities": []
        },
        {
          "Target": "fluent/fluentd:v1.17-armhf-debian (debian 12.6)",
          "Class": "os-pkgs",
          "Type": "debian",
          "Packages": [],
          "Vulnerabilities": []
        },
        {
          "Target": "Ruby",
          "Class": "lang-pkgs",
          "Type": "gemspec",
          "Packages": [],
          "Vulnerabilities": []
        }
      ]
    }
  ]
}

Related issues

• Close Unexpected JSON output when --report summary is set in trivy k8s for multi-container workloads

Checklist

• [x] I've read the guidelines for contributing to this repository.
• [x] I've followed the conventions in the PR title.
• [ ] I've added tests that prove my fix is effective or that my feature works.
• [ ] I've updated the documentation with the relevant information (if needed).
• [ ] I've added usage information (if the PR introduces new options)
• [x] I've included a "before" and "after" example to the description (if the PR is a user interface change).

[CLAassistant]

All committers have signed the CLA.

[afdesk]

Hi @smtan-gl thanks for the contribution! it's really nice and important.

I left some comments about this PR.

also I took a look at #5889 it seems we have to add more tests for these cases...

[smtan-gl]

Hi @smtan-gl thanks for the contribution! it's really nice and important.

I left some comments about this PR.

also I took a look at #5889 it seems we have to add more tests for these cases...

@afdesk any chance you might have context on this question?

[afdesk]

@afdesk any chance you might have context on this question?

If I understand correctly there is an option to create an image with several tags, so it should be a slice. https://stackoverflow.com/questions/31963525/is-it-possible-for-image-to-have-multiple-tags

[smtan-gl]

@afdesk I've updated the PR, could you have another look please 😄

[afdesk]

@afdesk I've updated the PR, could you have another look please 😄

sure, thanks! I'll take a look

[afdesk]

@smtan-gl could you sign CLA for a while? thanks!

[smtan-gl]

@smtan-gl could you sign CLA for a while? thanks!

@afdesk, I've just signed it. Thank you.

[afdesk]

it's a BREAKING change, so we have to create a new discussion about it. like here: https://github.com/aquasecurity/trivy/discussions/7010 we'll do it after the PR is merged. just to remember about it

[afdesk]

@smtan-gl also, I'm not sure that this PR fixes #5889...

[afdesk]

@smtan-gl please, feel free to correct me if I miss something. I've run an empty minikube. and then I create a pod with 2 images in my test namespace:

apiVersion: v1
kind: Pod
metadata:
  name: my-multiimage-pod
spec:
  containers:
  - name: my-image1-nginx
    image: nginx
  - name: my-image2-alpine-sleep
    image: alpine:3.17.1
    command: [ "/bin/sh", "-c", "--" ]
    args: [ "while true; do sleep 30; done;"]

$ kubectl create namespace k8s-test
$ kubectl create -f multiimagepod.yaml -n k8s

and try to scan this cluster with the update:

$ ./tr k8s --report all -f json -o res2.json --include-namespaces k8s-test --timeout 30m --debug --include-kinds Pod --scanners vuln

Metadata still contains only one record:

[smtan-gl]

@afdesk, the bug https://github.com/aquasecurity/trivy/issues/5889 occurs only with the --report summary configuration.

In your example, you used --report all which does not consolidate the report. As such, there are 2 reports in the Resources field, 1 for each of the scanned images containing only 1 metadata.

When I run the command using --report summary:

$ ./tr k8s --report summary -f json -o res2.json --include-namespaces k8s-test --timeout 30m --debug --include-kinds Pod --scanners vuln

It consolidates the reports into 1 consolidated report in the Findings field, and you can see that there are 2 metadata for the 2 images.

[afdesk]

@afdesk, the bug #5889 occurs only with the --report summary configuration.

exactly! thanks for clarify!

[smtan-gl]

Thanks for reviewing the PR @afdesk 😄 May I know what's the next steps needed to have this PR merged?

[afdesk]

Thanks for reviewing the PR @afdesk 😄 May I know what's the next steps needed to have this PR merged?

@smtan-gl thanks for your contribution again. It's a breaking change so let's wait feedback from another maintainers and community

[smtan-gl]

Thanks for reviewing the PR @afdesk 😄 May I know what's the next steps needed to have this PR merged?

@smtan-gl thanks for your contribution again. It's a breaking change so let's wait feedback from another maintainers and community

Hi @afdesk would you have an estimate on how long we need to wait for feedback?

[afdesk]

Hi @smtan-gl

there is a concern about this solution: how people associate the result with the metadata if it's N x N?

wdyt about it? thanks!

[smtan-gl]

Hi @smtan-gl

there is a concern about this solution: how people associate the result with the metadata if it's N x N?

wdyt about it? thanks!

Thanks for the response @afdesk. I thought we could match the vulnerability's Layer.DiffID with the metadata DiffIDs, but I've realized that the DiffIDs across multiple metadata entries might not be unique. Would it make sense to introduce an ImageID field to vulnerability?

For your convenience, I've attached below a JSON report from a scan with changes from this MR.

multi-image-pod.json

[afdesk]

@smtan-gl there are a few ideas.

1. @knqyf263 suggests to have Metadata and Results per image.

a sample json for a new field

  "Images": [
    {
      "Metadata": {
        "OS": {
          "Family": "debian",
          "Name": "12.6"
        },
        "RepoTags": [
          "image-a:latest"
        ]
      },
      "Results": [
        {
          "Target": "image-a:latest(debian 12.6)",
          "Class": "os-pkgs",
          "Type": "debian",
          "Vulnerabilities": []
        }
      ]
    },
    {
      "Metadata": {
        "OS": {
          "Family": "ubuntu",
          "Name": "22.04"
        },
        "RepoTags": [
          "image-b:latest"
        ]
      },
      "Results": [
        {
          "Target": "image-b:latest(ubuntu 22.04)",
          "Class": "os-pkgs",
          "Type": "ubuntu",
          "Vulnerabilities": []
        }
      ]
    }
  ]

Next ideas came from the point summary report basically is for reference and for table output.

2. disable JSON format for summary reports. People can summarize results by themselves. and maybe someone will create a plugin for it.

3. Pods are the smallest deployable units of computing that you can create and manage in Kubernetes. so If we say Pod is a single object, and we don't distinguish image. and then the current approach is fine.

Wdyt @smtan-gl ?

[smtan-gl]

Next ideas came from the point summary report basically is for reference and for table output.

Thanks for the response @afdesk. I agree with your points. However, I assume that there are other users who already rely on summary report in JSON format, so disabling it might affect them. For that reason, I support moving forward with approach 3.

[afdesk]

@smtan-gl just small update that I didn't forget about your job. I hope we'll resolve it before next release. thanks again