fix: add config files to FS for post-analyzers #5218
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simar7
reviewed
Sep 22, 2023
simar7
approved these changes
Sep 23, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@knqyf263 are you able to reproduce the linter issue locally? For some reason it keeps failing in the CI, without a descriptive message that we can use to fix the issue. But locally for me it seems fine: golangci-lint --version
golangci-lint has version 1.54.2 built with go1.21.0 from 411e0bb on 2023-08-21T11:04:00Z
golangci-lint run --verbose --timeout 10m
INFO [config_reader] Config search paths: [./ /Users/simarpreetsingh/repos/trivy /Users/simarpreetsingh/repos /Users/simarpreetsingh /Users /]
INFO [config_reader] Used config file .golangci.yaml
INFO [lintersdb] Active 11 linters: [goconst gocyclo gofmt goimports gosec govet ineffassign misspell revive unconvert unused]
INFO [loader] Go packages loading at mode 575 (exports_file|types_sizes|imports|name|compiled_files|deps|files) took 2.172765416s
INFO [runner/filename_unadjuster] Pre-built 1 adjustments in 17.912125ms
INFO [linters_context/goanalysis] analyzers took 0s with no stages
INFO [runner] Issues before processing: 369, after processing: 0
INFO [runner] Processors filtering stat (out/in): nolint: 0/7, filename_unadjuster: 369/369, skip_dirs: 239/239, exclude-rules: 7/32, skip_files: 239/369, identifier_marker: 32/32, cgo: 369/369, path_prettifier: 369/369, autogenerated_exclude: 32/239, exclude: 32/32
INFO [runner] processing took 4.914291ms with stages: path_prettifier: 1.717251ms, nolint: 1.557541ms, skip_files: 662.001µs, autogenerated_exclude: 482.333µs, identifier_marker: 237.25µs, skip_dirs: 199.542µs, exclude-rules: 33.958µs, cgo: 13.791µs, filename_unadjuster: 6.666µs, exclude: 1.291µs, max_same_issues: 541ns, fixer: 500ns, uniq_by_line: 334ns, diff: 292ns, severity-rules: 209ns, sort_results: 208ns, source_code: 167ns, max_from_linter: 166ns, max_per_file_from_linter: 84ns, path_shortener: 83ns, path_prefixer: 83ns
INFO [runner] linters took 558.617875ms with stages: goanalysis_metalinter: 553.658709ms
INFO File cache stats: 0 entries of total size 0B
INFO Memory: 29 samples, avg is 46.3MB, max is 118.5MB
INFO Execution took 2.783455625s I even tried with the version of the linter that we run within GitHub actions: ~/Downloads/golangci-lint-1.52.2-darwin-arm64/golangci-lint run --verbose --timeout 10m
INFO [config_reader] Config search paths: [./ /Users/simarpreetsingh/repos/trivy /Users/simarpreetsingh/repos /Users/simarpreetsingh /Users /]
INFO [config_reader] Used config file .golangci.yaml
INFO [lintersdb] Active 12 linters: [goconst gocyclo gofmt goimports gosec govet ineffassign misspell revive typecheck unconvert unused]
INFO [loader] Go packages loading at mode 575 (files|imports|compiled_files|deps|exports_file|name|types_sizes) took 1.919123958s
INFO [runner/filename_unadjuster] Pre-built 1 adjustments in 17.8755ms
INFO [linters_context/goanalysis] analyzers took 0s with no stages
INFO [runner] Issues before processing: 445, after processing: 0
INFO [runner] Processors filtering stat (out/in): cgo: 445/445, filename_unadjuster: 445/445, path_prettifier: 445/445, skip_dirs: 271/271, autogenerated_exclude: 64/271, exclude: 64/64, exclude-rules: 7/64, skip_files: 271/445, identifier_marker: 64/64, nolint: 0/7
INFO [runner] processing took 6.53254ms with stages: path_prettifier: 2.625625ms, nolint: 1.459792ms, skip_files: 826.583µs, autogenerated_exclude: 715.541µs, identifier_marker: 511.416µs, skip_dirs: 296.083µs, exclude-rules: 67.667µs, cgo: 18.209µs, filename_unadjuster: 7.833µs, exclude: 1.334µs, max_same_issues: 541ns, fixer: 374ns, diff: 250ns, severity-rules: 250ns, source_code: 209ns, uniq_by_line: 209ns, path_shortener: 209ns, max_from_linter: 126ns, sort_results: 124ns, max_per_file_from_linter: 83ns, path_prefixer: 82ns
INFO [runner] linters took 560.036917ms with stages: goanalysis_metalinter: 553.453417ms
INFO File cache stats: 0 entries of total size 0B
INFO Memory: 27 samples, avg is 49.8MB, max is 122.6MB
INFO Execution took 2.526622916s Any ideas? |
|
@DmitriyLewen Can you help them? |
|
Hm... it is strange case. I tried to reproduce this error on local PC with Ubuntu amd64: But I have 1 idea - But looks like GitHub Action takes settings from Perhaps there is some kind of conflict taking place. UPD: |
6 tasks
simar7
force-pushed
the
fix/fs-config-files
branch
2 times, most recently
from
dcd3fce to
307fdc9
Compare
|
I'm sorry to interrupt it, but please let me leave some comments. |
knqyf263
reviewed
Sep 26, 2023
pkg/fanal/artifact/image/image.go
Outdated
| @@ -277,9 +277,13 @@ func (a Artifact) inspectLayer(ctx context.Context, layerInfo LayerInfo, disable | |||
| } | |||
| defer composite.Cleanup() | |||
|
|
|||
| if err := artifact.AddConfigFilesToFS(composite, a.artifactOption); err != nil { | |||
There was a problem hiding this comment.
IMO, we should not call a misconfiguration-specific method here because we must call it in all artifacts. For example, this PR is not calling it in VM artifacts. It can lead to a bug.
What if putting files in misconf/scanner.go? Then, each artifact doesn't have to care about IaC config files.
diff --git a/pkg/misconf/scanner.go b/pkg/misconf/scanner.go
index 72f97f968..8f76b739c 100644
--- a/pkg/misconf/scanner.go
+++ b/pkg/misconf/scanner.go
@@ -71,6 +71,7 @@ type Scanner struct {
fileType detection.FileType
scanner scanners.FSScanner
hasFilePattern bool
+ configFiles []string
}
func NewAzureARMScanner(filePatterns []string, opt ScannerOption) (*Scanner, error) {
@@ -108,6 +109,7 @@ func newScanner(t detection.FileType, filePatterns []string, opt ScannerOption)
}
var scanner scanners.FSScanner
+ var configFiles []string
switch t {
case detection.FileTypeAzureARM:
scanner = arm.New(opts...)
@@ -117,10 +119,12 @@ func newScanner(t detection.FileType, filePatterns []string, opt ScannerOption)
scanner = dfscanner.NewScanner(opts...)
case detection.FileTypeHelm:
scanner = helm.New(opts...)
+ configFiles = append(opt.HelmValueFiles, opt.HelmValueFiles...)
case detection.FileTypeKubernetes:
scanner = k8sscanner.NewScanner(opts...)
case detection.FileTypeTerraform:
scanner = tfscanner.New(opts...)
+ configFiles = opt.TerraformTFVars
case detection.FileTypeTerraformPlan:
scanner = tfpscanner.New(opts...)
}
@@ -129,6 +133,7 @@ func newScanner(t detection.FileType, filePatterns []string, opt ScannerOption)
fileType: t,
scanner: scanner,
hasFilePattern: hasFilePattern(t, filePatterns),
+ configFiles: configFiles,
}, nil
}
@@ -141,6 +146,9 @@ func (s *Scanner) Scan(ctx context.Context, fsys fs.FS) ([]types.Misconfiguratio
return nil, nil
}
+ // Add config files to newfs here
+ // addConfigFilesToFS(newfs, s.configFiles)
+
log.Logger.Debugf("Scanning %s files for misconfigurations...", s.scanner.Name())
results, err := s.scanner.ScanFS(ctx, newfs, ".")
if err != nil {
@@ -196,6 +204,7 @@ func (s *Scanner) filterFS(fsys fs.FS) (fs.FS, error) {
if !foundRelevantFile {
return nil, nil
}
+
return newfs, nil
}
There was a problem hiding this comment.
But it adds files to the existing filesystem, then there are two concerns.
- Conflict with the existing files
e.g.
- pwd
- tfvars
- foo.tfvars
- app
- tfvars
- foo.tfvars
- main.tf
- tfvars
- tfvars
And call trivy config --tf-vars ./tfvars/foo.tfvars ./app. --tf-vars overwrites app/tfvars/foo.tfvars as the mapfs is rooted /app. Please correct me if I'm wrong.
- Outside of root dir
e.g.
- tfvars
- foo.tfvars
- pwd
- app
- tfvars
- foo.tfvars
- main.tf
- tfvars
- app
And call trivy config --tf-vars ../tfvars/foo.tfvars ./app. --tf-vars points to outside of the root dir, meaning /app here. It may work well, but mapfs should not have ../ as much as possible.
This PR is okay for a workaround, but ideally, we may want to create a dedicated filesystem for config files and pass it to scanners.
There was a problem hiding this comment.
So what do you propose? Should we merge this and create a new issue to track the improvement for a dedicated filesystem for config files?
There was a problem hiding this comment.
Right. That is in my mind.
There was a problem hiding this comment.
Cool! I created #5280 to track that change. Can we merge this PR for now?
There was a problem hiding this comment.
I'd apply this kind of change regardless of #5280.
#5218 (comment)
simar7
force-pushed
the
fix/fs-config-files
branch
from
38aed2d to
aaaa1e3
Compare
|
@nikpivkin - I rebased on top of latest main branch to see if it helps please the CI tests. Not entirely sure why they are still failing only on GitHub CI (ubuntu). |
|
Closed in favor of #5333. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a bug when Trivy could not use configuration files (such as tfvars or helm-values) that are located outside the scan directory or their absolute path was passed.
Related issues
Checklist