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.

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
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
46 changes: 37 additions & 9 deletions pkg/k8s/scanner/io.go
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,56 @@ import (

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

func createTempFile(artifact *artifacts.Artifact) (string, error) {
func generateTempFileByArtifact(artifact *artifacts.Artifact, tempFolder string) (string, error) {
simar7 marked this conversation as resolved.
Show resolved Hide resolved
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)
file, err := os.CreateTemp(tempFolder, filename)
if err != nil {
return "", xerrors.Errorf("creating tmp file error: %w", err)
return "", xerrors.Errorf("failed to create temporary file: %w", err)
}
shouldRemove := false
defer func() {
if err := file.Close(); err != nil {
log.Error("Failed to close temp file", log.String("path", file.Name()), log.Err(err))
log.Error("Failed to close temp file", log.FilePath(file.Name()), log.Err(err))
}
if shouldRemove {
removeFile(file.Name())
}
}()

if err := yaml.NewEncoder(file).Encode(artifact.RawResource); err != nil {
removeFile(filename)
return "", xerrors.Errorf("marshaling resource error: %w", err)
shouldRemove = true
return "", xerrors.Errorf("failed to encode artifact: %w", err)
}
return filepath.Base(file.Name()), nil
}

// 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 "", nil, xerrors.Errorf("failed to create temp folder: %w", err)
}

m := make(map[string]*artifacts.Artifact)
for _, artifact := range arts {
filename, err := generateTempFileByArtifact(artifact, tempFolder)
if err != nil {
log.Error("Failed to create temp file", log.FilePath(filename), log.Err(err))
continue
}
m[filename] = 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
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

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