-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(k8s): scan config files as a folder #7690
Conversation
Thanks!
I'm unsure, because I don't understand a reason of #7684. |
@afdesk Left a couple small comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return report.CreateResource(artifact, configReport, err), err | ||
return nil, xerrors.Errorf("failed to scan filesystem: %w", err) | ||
} | ||
resources := make([]report.Resource, 0, len(k8sArtifacts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any benefit of defining this of fixed length? Why not simplify like so
resources := make([]report.Resource, 0, len(k8sArtifacts)) | |
var resources []report.Resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this file is a little short on test coverage, maybe we can improve that a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you're right. we should improve test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but I'll wait for @knqyf263 to take a look as well.
pkg/k8s/scanner/io.go
Outdated
|
||
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think directory
is more common than folder
in UNIX. This function actually calls MkdirTemp
, not MkfolderTemp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename 84cacc5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@knqyf263 thanks for your review. 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". |
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
After
Related issues
Checklist