Skip to content
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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dcdc1a7
fix(k8s): scan config files as a folder
afdesk Oct 9, 2024
a9a9c58
split results
afdesk Oct 9, 2024
33f9633
fix linter error
afdesk Oct 9, 2024
0b8a9a7
restore log disabling
afdesk Oct 10, 2024
c941d62
refactor: create a temp file in the separate func
afdesk Oct 17, 2024
67cadae
update regex
afdesk Oct 18, 2024
84cacc5
refactor: rename a folder to dir
afdesk Oct 21, 2024
dba7527
refactor: rename folders to dirs
afdesk Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 31 additions & 16 deletions pkg/k8s/scanner/io.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package scanner
import (
"fmt"
"os"
"path/filepath"
"regexp"
"runtime"

Expand All @@ -15,29 +16,43 @@ import (

var r = regexp.MustCompile("\\\\|/|:|\\*|\\?|<|>")
simar7 marked this conversation as resolved.
Show resolved Hide resolved

func createTempFile(artifact *artifacts.Artifact) (string, error) {
filename := fmt.Sprintf("%s-%s-%s-*.yaml", artifact.Namespace, artifact.Kind, artifact.Name)

if runtime.GOOS == "windows" {
// removes characters not permitted in file/directory names on Windows
filename = filenameWindowsFriendly(filename)
}
file, err := os.CreateTemp("", filename)
// 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) {
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename 84cacc5

tempFolder, err := os.MkdirTemp("", "trivyk8s*")
if err != nil {
return "", xerrors.Errorf("creating tmp file error: %w", err)
return "", nil, xerrors.Errorf("failed to create temp folder: %w", err)
}
defer func() {

m := make(map[string]*artifacts.Artifact)
for _, artifact := range arts {
filename := fmt.Sprintf("%s-%s-%s-*.yaml", artifact.Namespace, artifact.Kind, artifact.Name)
nikpivkin marked this conversation as resolved.
Show resolved Hide resolved
if runtime.GOOS == "windows" {
// removes characters not permitted in file/directory names on Windows
filename = filenameWindowsFriendly(filename)
}
file, err := os.CreateTemp(tempFolder, filename)
if err != nil {
log.Error("Failed to create temp file", log.String("path", filename), log.Err(err))
continue
}
if err := yaml.NewEncoder(file).Encode(artifact.RawResource); err != nil {
removeFile(filename)
log.Error("Failed marshaling resource to a temp file", log.String("path", filename), log.Err(err))
nikpivkin marked this conversation as resolved.
Show resolved Hide resolved
continue
}
if err := file.Close(); err != nil {
log.Error("Failed to close temp file", log.String("path", file.Name()), log.Err(err))
}
}()

if err := yaml.NewEncoder(file).Encode(artifact.RawResource); err != nil {
removeFile(filename)
return "", xerrors.Errorf("marshaling resource error: %w", err)
m[filepath.Base(file.Name())] = artifact
}
return tempFolder, m, nil
}

return file.Name(), nil
func removeFolder(foldername string) {
if err := os.RemoveAll(foldername); err != nil {
log.Error("Failed to remove temp folder", log.String("path", foldername), log.Err(err))
}
}

func removeFile(filename string) {
Expand Down
82 changes: 48 additions & 34 deletions pkg/k8s/scanner/scanner.go
View file
Open in desktop
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,18 @@ func (s *Scanner) Scan(ctx context.Context, artifactsData []*artifacts.Artifact)

var resources []report.Resource

type scanResult struct {
vulns []report.Resource
misconfig report.Resource
// scans kubernetes artifacts as a scope of yaml files
if local.ShouldScanMisconfigOrRbac(s.opts.Scanners) {
misconfigs, err := s.scanMisconfigs(ctx, resourceArtifacts)
if err != nil {
return report.Report{}, xerrors.Errorf("scanning misconfigurations error: %w", err)
}
resources = append(resources, misconfigs...)
}

onItem := func(ctx context.Context, artifact *artifacts.Artifact) (scanResult, error) {
scanResults := scanResult{}
if s.opts.Scanners.AnyEnabled(types.VulnerabilityScanner, types.SecretScanner) && !s.opts.SkipImages {
// scan images from kubernetes cluster in parallel
if s.opts.Scanners.AnyEnabled(types.VulnerabilityScanner, types.SecretScanner) && !s.opts.SkipImages {
onItem := func(ctx context.Context, artifact *artifacts.Artifact) ([]report.Resource, error) {
opts := s.opts
opts.Credentials = make([]ftypes.Credential, len(s.opts.Credentials))
copy(opts.Credentials, s.opts.Credentials)
Expand All @@ -106,33 +110,22 @@ func (s *Scanner) Scan(ctx context.Context, artifactsData []*artifacts.Artifact)
}
vulns, err := s.scanVulns(ctx, artifact, opts)
if err != nil {
return scanResult{}, xerrors.Errorf("scanning vulnerabilities error: %w", err)
return nil, xerrors.Errorf("scanning vulnerabilities error: %w", err)
}
scanResults.vulns = vulns
return vulns, nil
}
if local.ShouldScanMisconfigOrRbac(s.opts.Scanners) {
misconfig, err := s.scanMisconfigs(ctx, artifact)
if err != nil {
return scanResult{}, xerrors.Errorf("scanning misconfigurations error: %w", err)
}
scanResults.misconfig = misconfig

onResult := func(result []report.Resource) error {
resources = append(resources, result...)
return nil
}
return scanResults, nil
}

onResult := func(result scanResult) error {
resources = append(resources, result.vulns...)
// don't add empty misconfig results to resources slice to avoid an empty resource
if result.misconfig.Results != nil {
resources = append(resources, result.misconfig)
p := parallel.NewPipeline(s.opts.Parallel, !s.opts.Quiet, resourceArtifacts, onItem, onResult)
if err := p.Do(ctx); err != nil {
return report.Report{}, err
}
return nil
}

p := parallel.NewPipeline(s.opts.Parallel, !s.opts.Quiet, resourceArtifacts, onItem, onResult)
if err := p.Do(ctx); err != nil {
return report.Report{}, err
}
if s.opts.Scanners.AnyEnabled(types.VulnerabilityScanner) {
k8sResource, err := s.scanK8sVulns(ctx, k8sCoreArtifacts)
if err != nil {
Expand Down Expand Up @@ -173,22 +166,43 @@ func (s *Scanner) scanVulns(ctx context.Context, artifact *artifacts.Artifact, o
return resources, nil
}

func (s *Scanner) scanMisconfigs(ctx context.Context, artifact *artifacts.Artifact) (report.Resource, error) {
configFile, err := createTempFile(artifact)
func (s *Scanner) scanMisconfigs(ctx context.Context, k8sArtifacts []*artifacts.Artifact) ([]report.Resource, error) {
folder, artifactsByFilename, err := generateTempFolder(k8sArtifacts)
if err != nil {
return report.Resource{}, xerrors.Errorf("scan error: %w", err)
return nil, xerrors.Errorf("failed to generate temp folder: %w", err)
}

s.opts.Target = configFile
s.opts.Target = folder

configReport, err := s.runner.ScanFilesystem(ctx, s.opts)
// remove config file after scanning
removeFile(configFile)
// remove config files after scanning
removeFolder(folder)

if err != nil {
return report.CreateResource(artifact, configReport, err), err
return nil, xerrors.Errorf("failed to scan filesystem: %w", err)
}
resources := make([]report.Resource, 0, len(k8sArtifacts))
Copy link
Member

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

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

Copy link
Contributor Author

@afdesk afdesk Oct 18, 2024 •
edited
Loading

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?

Copy link
Member

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.

Copy link
Collaborator

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.


for _, res := range configReport.Results {
artifact := artifactsByFilename[res.Target]

singleReport := types.Report{
SchemaVersion: configReport.SchemaVersion,
CreatedAt: configReport.CreatedAt,
ArtifactName: res.Target,
ArtifactType: configReport.ArtifactType,
Metadata: configReport.Metadata,
Results: types.Results{res},
}

resource, err := s.filter(ctx, singleReport, artifact)
if err != nil {
resource = report.CreateResource(artifact, singleReport, err)
}
resources = append(resources, resource)
}

return s.filter(ctx, configReport, artifact)
return resources, nil
}
func (s *Scanner) filter(ctx context.Context, r types.Report, artifact *artifacts.Artifact) (report.Resource, error) {
var err error
Expand Down