From a2d738c5757d5695f2b333bff29a3e6538a9f068 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani <45800463+rchincha@users.noreply.github.com> Date: Tue, 26 May 2026 14:58:30 -0700 Subject: [PATCH] fix: miscellaneous fixes for ai-reported suggestions (#4101) * test(cli): replace panic with t.Fatalf in deprecated config tests Signed-off-by: Ramkumar Chinchani * fix(trivy): keep sbom generation failures non-fatal in runTrivy Signed-off-by: Ramkumar Chinchani * docs(meta): fix typos in hooks comments Signed-off-by: Ramkumar Chinchani * refactor(requestcontext): align package name and godoc comments Signed-off-by: Ramkumar Chinchani * test(gc): factor metrics setup helper and fix typo Signed-off-by: Ramkumar Chinchani * test(trivy): cover non-fatal SBOM generation failures - add runTrivy test ensuring scan succeeds when SBOM generation fails - inject artifact runner constructor for deterministic internal testing - fix matchesRepo doc comment to be action-agnostic Signed-off-by: Ramkumar Chinchani --------- Signed-off-by: Ramkumar Chinchani --- pkg/cli/client/config_cmd_deprecated_test.go | 8 +-- pkg/extensions/search/cve/trivy/scanner.go | 13 +++-- .../search/cve/trivy/scanner_internal_test.go | 50 +++++++++++++++++++ pkg/meta/hooks.go | 6 +-- pkg/requestcontext/authn.go | 2 +- pkg/requestcontext/user_access_control.go | 13 ++--- pkg/storage/gc/gc_test.go | 34 +++++++------ 7 files changed, 91 insertions(+), 35 deletions(-) diff --git a/pkg/cli/client/config_cmd_deprecated_test.go b/pkg/cli/client/config_cmd_deprecated_test.go index 931a719d..423d0d8b 100644 --- a/pkg/cli/client/config_cmd_deprecated_test.go +++ b/pkg/cli/client/config_cmd_deprecated_test.go @@ -82,7 +82,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) { actual, err := os.ReadFile(configPath) if err != nil { - panic(err) + t.Fatalf("failed to read config file %s: %v", configPath, err) } actualStr := string(actual) So(actualStr, ShouldContainSubstring, "configtest1") @@ -405,7 +405,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) { actual, err := os.ReadFile(configPath) if err != nil { - panic(err) + t.Fatalf("failed to read config file %s: %v", configPath, err) } actualStr := string(actual) So(actualStr, ShouldContainSubstring, "https://test-url.com") @@ -450,7 +450,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) { actual, err := os.ReadFile(configPath) if err != nil { - panic(err) + t.Fatalf("failed to read config file %s: %v", configPath, err) } actualStr := string(actual) So(actualStr, ShouldContainSubstring, `https://new-url.com`) @@ -477,7 +477,7 @@ func TestConfigCmdDeprecatedMain(t *testing.T) { actual, err := os.ReadFile(configPath) if err != nil { - panic(err) + t.Fatalf("failed to read config file %s: %v", configPath, err) } actualStr := string(actual) So(actualStr, ShouldNotContainSubstring, "showspinner") diff --git a/pkg/extensions/search/cve/trivy/scanner.go b/pkg/extensions/search/cve/trivy/scanner.go index 5518c127..e024ef67 100644 --- a/pkg/extensions/search/cve/trivy/scanner.go +++ b/pkg/extensions/search/cve/trivy/scanner.go @@ -57,6 +57,8 @@ const ( var errImageStoreNotFound = errors.New("image store not found") +var newArtifactRunner = artifact.NewRunner //nolint:gochecknoglobals // test seam for deterministic runner injection + // getNewScanOptions sets trivy configuration values for our scans and returns them as // a trivy Options structure. func getNewScanOptions(dir string, dbRepositoryRef, javaDBRepositoryRef name.Reference, @@ -330,7 +332,7 @@ func (scanner Scanner) runTrivy(ctx context.Context, opts flag.Options) (types.R var sbom *generatedSBOM err = scanner.withTempDir(func() error { - runner, err := artifact.NewRunner(ctx, opts, artifact.TargetContainerImage) + runner, err := newArtifactRunner(ctx, opts, artifact.TargetContainerImage) if err != nil { return err } @@ -347,13 +349,14 @@ func (scanner Scanner) runTrivy(ctx context.Context, opts flag.Options) (types.R } if scanner.sbomOptions.enabled { - sbom, err = scanner.generateSBOM(ctx, runner, opts, report) - if err != nil { - scanner.log.Warn().Err(err).Str("image", opts.ImageOptions.Input).Msg("failed to generate sbom") + var sbomErr error + sbom, sbomErr = scanner.generateSBOM(ctx, runner, opts, report) + if sbomErr != nil { + scanner.log.Warn().Err(sbomErr).Str("image", opts.ImageOptions.Input).Msg("failed to generate sbom") } } - return err + return nil }) return report, sbom, err diff --git a/pkg/extensions/search/cve/trivy/scanner_internal_test.go b/pkg/extensions/search/cve/trivy/scanner_internal_test.go index caaefb2b..43605738 100644 --- a/pkg/extensions/search/cve/trivy/scanner_internal_test.go +++ b/pkg/extensions/search/cve/trivy/scanner_internal_test.go @@ -5,11 +5,13 @@ package trivy import ( "context" "encoding/json" + "errors" "os" "path" "testing" "time" + "github.com/aquasecurity/trivy-db/pkg/metadata" dbTypes "github.com/aquasecurity/trivy-db/pkg/types" "github.com/aquasecurity/trivy/pkg/commands/artifact" "github.com/aquasecurity/trivy/pkg/flag" @@ -132,6 +134,54 @@ func TestGenerateSBOM(t *testing.T) { }) } +func TestRunTrivySBOMGenerationFailureIsNonFatal(t *testing.T) { + Convey("runTrivy should return report and nil error when SBOM generation fails", t, func() { + logger := log.NewTestLogger() + rootDir := t.TempDir() + + dbDir := path.Join(rootDir, "_trivy", "db") + err := os.MkdirAll(dbDir, 0o755) + So(err, ShouldBeNil) + err = os.WriteFile(metadata.Path(dbDir), []byte(`{"Version":2}`), 0o600) + So(err, ShouldBeNil) + + store := local.NewImageStore(rootDir, false, false, logger, monitoring.NewMetricsServer(false, logger), nil, nil, nil, nil) + storeController := storage.StoreController{DefaultStore: store} + + scanner := Scanner{ + log: logger, + storeController: storeController, + sbomOptions: sbomOptions{ + enabled: true, + reportFormat: trivyTypes.FormatSPDXJSON, + }, + } + + sbomErr := errors.New("sbom generation failed") + oldNewArtifactRunner := newArtifactRunner + newArtifactRunner = func(ctx context.Context, opts flag.Options, target artifact.TargetKind, + runnerOpts ...artifact.RunnerOption, + ) (artifact.Runner, error) { + return fakeArtifactRunner{ + reportFn: func(ctx context.Context, opts flag.Options, report trivyTypes.Report) error { + return sbomErr + }, + }, nil + } + defer func() { + newArtifactRunner = oldNewArtifactRunner + }() + + report, generated, err := scanner.runTrivy(context.Background(), flag.Options{ + ImageOptions: flag.ImageOptions{Input: "repo:tag"}, + }) + + So(err, ShouldBeNil) + So(report, ShouldResemble, trivyTypes.Report{}) + So(generated, ShouldBeNil) + }) +} + func TestMultipleStoragePath(t *testing.T) { Convey("Test multiple storage path", t, func() { // Create temporary directory diff --git a/pkg/meta/hooks.go b/pkg/meta/hooks.go index 8ccbd9e5..2a8ff617 100644 --- a/pkg/meta/hooks.go +++ b/pkg/meta/hooks.go @@ -143,7 +143,7 @@ func rollbackDigestManifestTags(ctx context.Context, repo string, tags, appliedM } // OnUpdateManifest is called when a new manifest is added. It updates metadb according to the type -// of image pushed(normal images, signatues, etc.). In care of any errors, it makes sure to keep +// of image pushed(normal images, signatures, etc.). In case of any errors, it makes sure to keep // consistency between metadb and the image store. func OnUpdateManifest(ctx context.Context, repo, reference, mediaType string, digest godigest.Digest, body []byte, storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger, @@ -213,7 +213,7 @@ func OnUpdateManifestDigestTags(ctx context.Context, repo string, tags []string, } // OnDeleteManifest is called when a manifest is deleted. It updates metadb according to the type -// of image pushed(normal images, signatues, etc.). In care of any errors, it makes sure to keep +// of image pushed(normal images, signatures, etc.). In case of any errors, it makes sure to keep // consistency between metadb and the image store. func OnDeleteManifest(repo, reference, mediaType string, digest godigest.Digest, manifestBlob []byte, storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger, @@ -271,7 +271,7 @@ func OnDeleteManifest(repo, reference, mediaType string, digest godigest.Digest, return nil } -// OnGetManifest is called when a manifest is downloaded. It increments the download couter on that manifest. +// OnGetManifest is called when a manifest is downloaded. It increments the download counter on that manifest. func OnGetManifest(name, reference, mediaType string, body []byte, storeController storage.StoreController, metaDB mTypes.MetaDB, log log.Logger, ) error { diff --git a/pkg/requestcontext/authn.go b/pkg/requestcontext/authn.go index 512fd016..38aaa9c8 100644 --- a/pkg/requestcontext/authn.go +++ b/pkg/requestcontext/authn.go @@ -1,4 +1,4 @@ -package uac +package requestcontext import ( "context" diff --git a/pkg/requestcontext/user_access_control.go b/pkg/requestcontext/user_access_control.go index dbac46ca..7999cf31 100644 --- a/pkg/requestcontext/user_access_control.go +++ b/pkg/requestcontext/user_access_control.go @@ -1,4 +1,4 @@ -package uac +package requestcontext import ( "context" @@ -167,9 +167,8 @@ func (uac *UserAccessControl) SetGlobPatterns(action string, patterns map[string uac.authzInfo.globPatterns[action] = patterns } -/* -Can returns whether or not the user/anonymous who made the request has 'action' permission on 'repository'. -*/ +// Can returns whether or not the user/anonymous who made the request has +// action permission on repository. func (uac *UserAccessControl) Can(action, repository string) bool { var defaultRet bool if uac.isBehaviourAction(action) { @@ -205,10 +204,8 @@ func (uac *UserAccessControl) areGlobPatternsSet() bool { return !notSet } -/* -returns whether or not 'repository' can be found in the list of patterns -on which the user who made the request has read permission on. -*/ +// matchesRepo returns whether repository matches the provided action's glob patterns +// and is allowed by the longest matching pattern. func (uac *UserAccessControl) matchesRepo(globPatterns map[string]bool, repository string) bool { var longestMatchedPattern string diff --git a/pkg/storage/gc/gc_test.go b/pkg/storage/gc/gc_test.go index e4406a88..64fb7e2c 100644 --- a/pkg/storage/gc/gc_test.go +++ b/pkg/storage/gc/gc_test.go @@ -58,12 +58,20 @@ var testCases = []struct { }, } +func newTestMetricsServer(t *testing.T, log zlog.Logger) monitoring.MetricServer { + t.Helper() + + metrics := monitoring.NewMetricsServer(false, log) + t.Cleanup(metrics.Stop) + + return metrics +} + func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { log := zlog.NewTestLogger() audit := zlog.NewAuditLogger("debug", "/dev/null") - metrics := monitoring.NewMetricsServer(false, log) - defer metrics.Stop() // Clean up metrics server to prevent resource leaks + metrics := newTestMetricsServer(t, log) trueVal := true @@ -1242,7 +1250,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { So(err, ShouldBeNil) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(uploads, ShouldEqual, []string{}) } else { @@ -1253,7 +1261,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { isPresent, _, _, err := imgStore.StatBlobUpload(repoName, blobUploadID) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(err, ShouldNotBeNil) So(isPresent, ShouldBeFalse) @@ -1271,7 +1279,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { So(err, ShouldBeNil) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(uploads, ShouldEqual, []string{}) } else { @@ -1282,7 +1290,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { isPresent, _, _, err = imgStore.StatBlobUpload(repoName, blobUploadID) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(err, ShouldNotBeNil) So(isPresent, ShouldBeFalse) @@ -1316,8 +1324,7 @@ func TestGarbageCollectDeletion(t *testing.T) { log := zlog.NewTestLogger() audit := zlog.NewAuditLogger("debug", "/dev/null") - metrics := monitoring.NewMetricsServer(false, log) - defer metrics.Stop() // Clean up metrics server to prevent resource leaks + metrics := newTestMetricsServer(t, log) trueVal := true falseVal := false @@ -1755,8 +1762,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { log := zlog.NewTestLogger() audit := zlog.NewAuditLogger("debug", "/dev/null") - metrics := monitoring.NewMetricsServer(false, log) - defer metrics.Stop() // Clean up metrics server to prevent resource leaks + metrics := newTestMetricsServer(t, log) trueVal := true @@ -2566,7 +2572,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { So(err, ShouldBeNil) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(uploads, ShouldEqual, []string{}) } else { @@ -2577,7 +2583,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { isPresent, _, _, err := imgStore.StatBlobUpload(repoName, blobUploadID) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(err, ShouldNotBeNil) So(isPresent, ShouldBeFalse) @@ -2595,7 +2601,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { So(err, ShouldBeNil) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(uploads, ShouldEqual, []string{}) } else { @@ -2606,7 +2612,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { isPresent, _, _, err = imgStore.StatBlobUpload(repoName, blobUploadID) if testcase.testCaseName == s3TestName { - // Remote sorage is written to only after the blob upload is finished, + // Remote storage is written to only after the blob upload is finished, // there should be no space used by blob uploads So(err, ShouldNotBeNil) So(isPresent, ShouldBeFalse)