fix(k8s)!: support k8s multi container #7444

smtan-gl · 2024-09-05T03:10:29Z

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

I've read the guidelines for contributing to this repository.
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)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

CLAassistant · 2024-09-05T03:10:35Z

All committers have signed the CLA.

afdesk · 2024-09-05T11:40:30Z

pkg/k8s/report/report.go

@@ -65,10 +65,26 @@ type Resource struct {
 	Report types.Report `json:"-"`
 }

+type ConsolidatedResource struct {


can we update Resource? I think it's a bit hard to keep 2 similar structs.

also there are no tests for this case.

Thanks for the feedback, I'm not sure why I assumed the Resource struct was used by other packages in Trivy, but it doesn't seem to be the case. I'll work on updating the Resource struct accordingly.

also there are no tests for this case.

Regarding the tests, I wanted to get feedback on the approach before proceeding. I'll focus on adding the tests next.

afdesk · 2024-09-05T11:48:16Z

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 · 2024-09-06T09:39:23Z

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 · 2024-09-10T04:52:24Z

@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 · 2024-09-12T09:35:11Z

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

afdesk · 2024-09-16T05:13:41Z

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

sure, thanks! I'll take a look

afdesk · 2024-09-17T08:05:34Z

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

smtan-gl · 2024-09-17T10:07:50Z

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

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

afdesk

Thanks for your contribute! I left a few comments

pkg/k8s/report/report_test.go

afdesk · 2024-09-18T11:25:03Z

it's a BREAKING change, so we have to create a new discussion about it.
like here: #7010
we'll do it after the PR is merged. just to remember about it

afdesk · 2024-09-18T11:43:39Z

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

afdesk · 2024-09-18T11:49:22Z

@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 · 2024-09-19T06:22:14Z

@afdesk, the bug #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

LGTM!

afdesk · 2024-09-19T07:58:25Z

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

exactly! thanks for clarify!

smtan-gl · 2024-09-20T02:22:17Z

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

afdesk · 2024-09-20T03:03:25Z

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 requested a review from afdesk as a code owner September 5, 2024 03:10

smtan-gl mentioned this pull request Sep 5, 2024

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

Open

2 tasks

afdesk requested changes Sep 5, 2024

View reviewed changes

smtan-gl added 2 commits September 12, 2024 16:46

Convert k8s metadata to slice

7cf7654

Add test for multi image pod scenario

c99c624

smtan-gl force-pushed the fix_k8s_multi_container branch from 1571d0d to c99c624 Compare September 12, 2024 09:33

smtan-gl changed the title ~~fix(k8s): support k8s multi container(DRAFT)~~ fix(k8s): support k8s multi container Sep 12, 2024

afdesk requested changes Sep 18, 2024

View reviewed changes

pkg/k8s/report/report_test.go Outdated Show resolved Hide resolved

pkg/k8s/report/report_test.go Outdated Show resolved Hide resolved

afdesk changed the title ~~fix(k8s): support k8s multi container~~ fix(k8s)!: support k8s multi container Sep 18, 2024

Simplify test

ed599f1

smtan-gl requested a review from afdesk September 19, 2024 06:35

afdesk approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into fix_k8s_multi_container

393994f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(k8s)!: support k8s multi container #7444

fix(k8s)!: support k8s multi container #7444

smtan-gl commented Sep 5, 2024

CLAassistant commented Sep 5, 2024 •

edited

Loading

afdesk Sep 5, 2024

afdesk Sep 5, 2024

smtan-gl Sep 6, 2024 •

edited

Loading

afdesk commented Sep 5, 2024

smtan-gl commented Sep 6, 2024

afdesk commented Sep 10, 2024

smtan-gl commented Sep 12, 2024

afdesk commented Sep 16, 2024

afdesk commented Sep 17, 2024

smtan-gl commented Sep 17, 2024

afdesk left a comment

afdesk commented Sep 18, 2024

afdesk commented Sep 18, 2024

afdesk commented Sep 18, 2024

smtan-gl commented Sep 19, 2024

afdesk left a comment

afdesk commented Sep 19, 2024

smtan-gl commented Sep 20, 2024

afdesk commented Sep 20, 2024

fix(k8s)!: support k8s multi container #7444

Are you sure you want to change the base?

fix(k8s)!: support k8s multi container #7444

Conversation

smtan-gl commented Sep 5, 2024

Description

Related issues

Checklist

CLAassistant commented Sep 5, 2024 • edited Loading

afdesk Sep 5, 2024

Choose a reason for hiding this comment

afdesk Sep 5, 2024

Choose a reason for hiding this comment

smtan-gl Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

afdesk commented Sep 5, 2024

smtan-gl commented Sep 6, 2024

afdesk commented Sep 10, 2024

smtan-gl commented Sep 12, 2024

afdesk commented Sep 16, 2024

afdesk commented Sep 17, 2024

smtan-gl commented Sep 17, 2024

afdesk left a comment

Choose a reason for hiding this comment

afdesk commented Sep 18, 2024

afdesk commented Sep 18, 2024

afdesk commented Sep 18, 2024

smtan-gl commented Sep 19, 2024

afdesk left a comment

Choose a reason for hiding this comment

afdesk commented Sep 19, 2024

smtan-gl commented Sep 20, 2024

afdesk commented Sep 20, 2024

CLAassistant commented Sep 5, 2024 •

edited

Loading

smtan-gl Sep 6, 2024 •

edited

Loading