refactor(k8s): scan config files as a folder #7690

afdesk · 2024-10-09T10:42:26Z

Description

Trivy kubernetes scan tries to process the k8s configs in parallel.
but this leads to large overhead, due to initialize the scanner for each file separately (it needs to read rego policies).

This PR stores all k8s config in the same folder and scan this one at once.
it gives a boost about 9 times on my test minikube instance.

Before

$ time trivy k8s --report all --skip-images -f json -o result.json
2024-10-10T10:12:01+06:00	INFO	Node scanning is enabled
2024-10-10T10:12:01+06:00	INFO	If you want to disable Node scanning via an in-cluster Job, please try '--disable-node-collector' to disable the Node-Collector job.
235 / 235 [-----------------------------------------------------------------------------------------------------------------] 100.00% 3 p/s
trivy k8s --report all --skip-images -f json -o result.json  147,50s user 8,63s system 148% cpu 1:45,12 total

After

$ time ./tr k8s --report all --skip-images -f json -o result.json
2024-10-10T17:04:04+06:00	INFO	Node scanning is enabled
2024-10-10T17:04:04+06:00	INFO	If you want to disable Node scanning via an in-cluster Job, please try '--disable-node-collector' to disable the Node-Collector job.
./tr k8s --report all --skip-images -f json -o result.json  16,83s user 1,12s system 85% cpu 21,002 total

Related issues

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).

afdesk · 2024-10-10T04:25:59Z

@itaysk @knqyf263 Could you take a look at this PR when you have time?

I'm unsure about logs. maybe it's a bit confusing for people.

also I can't reproduce #7684 with this update.

wdyt?

thanks a lot for your time.

itaysk · 2024-10-10T10:23:09Z

I'm unsure about logs. maybe it's a bit confusing for people.

cluster scanning can have many resources, and logging each individual resource scan is too much IMO. we can make it a debug level log.

also I can't reproduce #7684 with this update.

Good, that means you think it solved #7684 too?

afdesk · 2024-10-10T10:58:13Z

I'm unsure about logs. maybe it's a bit confusing for people.
cluster scanning can have many resources, and logging each individual resource scan is too much IMO. we can make it a debug level log.

Thanks!
I understood your point and agree with them.
so I remove the log update from this PR, and will create a new one for logs only.

also I can't reproduce #7684 with this update.
Good, that means you think it solved #7684 too?

I'm unsure, because I don't understand a reason of #7684.
I'm investigating it now

afdesk · 2024-10-16T07:00:30Z

@itaysk @knqyf263 could you take a look at this PR again? thanks!

afdesk · 2024-10-17T05:39:50Z

@itaysk I finally clarify the reason of #7684 - the race condition appears between a few misconfig scanners, so this PR should resolve it too.

pkg/k8s/scanner/io.go

nikpivkin · 2024-10-17T09:23:45Z

@afdesk Left a couple small comments

nikpivkin

LGTM

simar7 · 2024-10-18T00:37:27Z

pkg/k8s/scanner/scanner.go

- return report.CreateResource(artifact, configReport, err), err
+ return nil, xerrors.Errorf("failed to scan filesystem: %w", err)
+ }
+ resources := make([]report.Resource, 0, len(k8sArtifacts))


Is there any benefit of defining this of fixed length? Why not simplify like so

Suggested change

resources := make([]report.Resource, 0, len(k8sArtifacts))

var resources []report.Resource

try to avoid a few copy operations and memory allocations by grabbing it all up front. (it's a quota )).
I'm not sure it makes sense, so we can change this one.

do you think we should use a simple construction?

I haven't benchmarked this so I can't say for sure but I would assume the Go runtime can grow the slice as needed without much of an overhead.

make is faster than var, so if we know the length beforehand, we should use make for performance reasons.
https://sampath04.hashnode.dev/make-your-go-code-efficient-using-make-when-creating-slices

If the length of the slice is small enough, I personally prefer var because it's easier to read. In this case, k8s resources can be a lot. It makes sense to use make.

simar7 · 2024-10-18T00:43:32Z

pkg/k8s/scanner/scanner.go

It seems that this file is a little short on test coverage, maybe we can improve that a bit?

yes, you're right. we should improve test cases

OK - Since this PR also fixes a critical bug for k8s scanning, we can merge it first.

I opened #7768 to track improving the test coverage.

pkg/k8s/scanner/io.go

simar7

lgtm, but I'll wait for @knqyf263 to take a look as well.

afdesk · 2024-10-19T04:20:03Z

lgtm, but I'll wait for @knqyf263 to take a look as well.

@simar7 thanks for your review!!

knqyf263 · 2024-10-21T09:18:39Z

pkg/k8s/scanner/io.go

+
+// generateTempFolder creates a folder with yaml files generated from kubernetes artifacts
+// returns a folder name, a map for mapping a temp target file to k8s artifact and error
+func generateTempFolder(arts []*artifacts.Artifact) (string, map[string]*artifacts.Artifact, error) {


nit: I think directory is more common than folder in UNIX. This function actually calls MkdirTemp, not MkfolderTemp.

rename 84cacc5

knqyf263

LGTM

afdesk · 2024-10-21T09:55:49Z

LGTM

@knqyf263 thanks for your review.

when tests are completed, I'll add this PR to merge queue, ok?

knqyf263 · 2024-10-21T10:10:52Z

when tests are completed, I'll add this PR to merge queue, ok?

Sure. Just FYI: There are many places where it still says "folder".
84cacc5

afdesk added 3 commits October 9, 2024 16:42

fix(k8s): scan config files as a folder

dcdc1a7

split results

a9a9c58

fix linter error

33f9633

afdesk changed the title ~~fix(k8s): scan config files as a folder~~ refactor(k8s): scan config files as a folder Oct 10, 2024

afdesk marked this pull request as ready for review October 10, 2024 04:21

afdesk requested review from knqyf263 and itaysk October 10, 2024 04:22

restore log disabling

0b8a9a7

nikpivkin reviewed Oct 17, 2024

View reviewed changes

pkg/k8s/scanner/io.go Outdated Show resolved Hide resolved

nikpivkin reviewed Oct 17, 2024

View reviewed changes

pkg/k8s/scanner/io.go Outdated Show resolved Hide resolved

refactor: create a temp file in the separate func

c941d62

afdesk requested a review from nikpivkin October 17, 2024 10:54

nikpivkin approved these changes Oct 17, 2024

View reviewed changes

simar7 reviewed Oct 18, 2024

View reviewed changes

pkg/k8s/scanner/io.go Outdated Show resolved Hide resolved

simar7 reviewed Oct 18, 2024

View reviewed changes

pkg/k8s/scanner/io.go Outdated Show resolved Hide resolved

update regex

67cadae

simar7 mentioned this pull request Oct 19, 2024

chore(k8s): Improve testing for k8s scanner #7768

Open

simar7 self-requested a review October 19, 2024 01:26

simar7 approved these changes Oct 19, 2024

View reviewed changes

knqyf263 reviewed Oct 21, 2024

View reviewed changes

knqyf263 approved these changes Oct 21, 2024

View reviewed changes

refactor: rename a folder to dir

84cacc5

refactor: rename folders to dirs

dba7527

afdesk added this pull request to the merge queue Oct 21, 2024

Merged via the queue into aquasecurity:main with commit 010b213 Oct 21, 2024
12 checks passed

afdesk deleted the fix/k8s-scantime branch October 21, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(k8s): scan config files as a folder #7690

refactor(k8s): scan config files as a folder #7690

afdesk commented Oct 9, 2024 •

edited

Loading

afdesk commented Oct 10, 2024

itaysk commented Oct 10, 2024

afdesk commented Oct 10, 2024

afdesk commented Oct 16, 2024

afdesk commented Oct 17, 2024

nikpivkin commented Oct 17, 2024

nikpivkin left a comment

simar7 Oct 18, 2024

afdesk Oct 18, 2024 •

edited

Loading

simar7 Oct 19, 2024

knqyf263 Oct 21, 2024

simar7 Oct 18, 2024

afdesk Oct 18, 2024

simar7 Oct 19, 2024

simar7 left a comment

afdesk commented Oct 19, 2024

knqyf263 Oct 21, 2024

afdesk Oct 21, 2024

knqyf263 left a comment

afdesk commented Oct 21, 2024

knqyf263 commented Oct 21, 2024

	resources := make([]report.Resource, 0, len(k8sArtifacts))
	var resources []report.Resource

refactor(k8s): scan config files as a folder #7690

refactor(k8s): scan config files as a folder #7690

Conversation

afdesk commented Oct 9, 2024 • edited Loading

Description

Related issues

Checklist

afdesk commented Oct 10, 2024

itaysk commented Oct 10, 2024

afdesk commented Oct 10, 2024

afdesk commented Oct 16, 2024

afdesk commented Oct 17, 2024

nikpivkin commented Oct 17, 2024

nikpivkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afdesk Oct 18, 2024 • edited Loading

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

simar7 left a comment

Choose a reason for hiding this comment

afdesk commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 left a comment

Choose a reason for hiding this comment

afdesk commented Oct 21, 2024

knqyf263 commented Oct 21, 2024

afdesk commented Oct 9, 2024 •

edited

Loading

afdesk Oct 18, 2024 •

edited

Loading