fix(k8s)!: support k8s multi container #7444
Conversation
|
|
pkg/k8s/report/report.go
Outdated
| @@ -65,10 +65,26 @@ type Resource struct { | |||
| Report types.Report `json:"-"` | |||
| } | |||
|
|
|||
| type ConsolidatedResource struct { | |||
There was a problem hiding this comment.
can we update Resource? I think it's a bit hard to keep 2 similar structs.
There was a problem hiding this comment.
also there are no tests for this case.
There was a problem hiding this comment.
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.
|
If I understand correctly there is an option to create an image with several tags, so it should be a slice. |
smtan-gl
force-pushed
the
fix_k8s_multi_container
branch
from
1571d0d to
c99c624
Compare
smtan-gl
changed the title
|
@afdesk I've updated the PR, could you have another look please 😄 |
sure, thanks! I'll take a look |
|
@smtan-gl could you sign CLA for a while? thanks! |
afdesk
changed the title
|
it's a BREAKING change, so we have to create a new discussion about it. |
|
@smtan-gl please, feel free to correct me if I miss something. 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 k8sand 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 vulnMetadata still contains only one record: |
|
@afdesk, the bug #5889 occurs only with the In your example, you used When I run the command using It consolidates the reports into 1 consolidated report in the
|
|
Thanks for reviewing the PR @afdesk 😄 May I know what's the next steps needed to have this PR merged? |
Hi @afdesk would you have an estimate on how long we need to wait for feedback? |
|
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 For your convenience, I've attached below a JSON report from a scan with changes from this MR. |
|
@smtan-gl there are a few ideas.
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
Wdyt @smtan-gl ? |
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. |
|
@smtan-gl just small update that I didn't forget about your job. |
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
Report after change
Related issues
Checklist