perf(misconf): retrieve check metadata from annotations once #8478

nikpivkin · 2025-03-03T17:04:05Z

Description

Right now, Trivy retrieves the static metadata of a check each time it is run. This PR retrieves static metadata from annotations after checks are loaded. For dynamic metadata that is specified in the legacy way via the ___rego_metadata__ rule, the behavior has not changed, as they can use input data.
Also for checks that use the __rego_metadata__ and __rego_input__ rules, a warning is issued that this is a deprecated way of specifying metadata and input options and it is recommended to use annotations.

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: nikpivkin <[email protected]>

simar7 · 2025-03-08T06:25:06Z

pkg/iac/rego/embed.go

+			log.Warn("Failed to retrieve metadata",
+				log.String("package", module.Package.String()), log.Err(err))
+			continue
+		} else if metadata == nil {


can we add a sad test case for these two code paths?

I have removed these changes as they are unnecessary.

simar7 · 2025-03-08T06:27:33Z

pkg/iac/rego/embed.go

-		if !metadata.Deprecated {
-			regoCheckIDs.Append(metadata.AVDID)
-		}


this is interesting, should we add a linter check to ensure a variable gets flagged if it's only set and never used?

simar7 · 2025-03-08T06:38:41Z

pkg/iac/rego/embed.go

+		Filtered(paths, func(abspath string, info fs.FileInfo, _ int) bool {
+			return isNotRegoFile(info) || isOptionalChecks(abspath)
+		})


can we add some test cases where .Filtered() can return true, IOW files that are not valid rego?

Added tests de66947

I put the old way of loading back in, since we use the slash as a path separator regardless of OS, and OPA uses the filepath package which is OS dependent.

simar7 · 2025-03-08T06:39:00Z

pkg/iac/rego/embed.go

+func isOptionalChecks(path string) bool {
+	return strings.HasSuffix(filepath.Dir(filepath.ToSlash(path)), filepath.Join("advanced", "optional"))


Is this still needed after the recent k8s checks re-org?

Let's look at this in another PR when updating trivy-checks? For now, trivy works with the old trivy-checks structure.

simar7 · 2025-03-08T07:06:18Z

pkg/iac/rego/load.go

+	}
+}
+
+// moduleHasLegacyMetadataFormat checks if the module has an legacy metadata format.


Suggested change

// moduleHasLegacyMetadataFormat checks if the module has an legacy metadata format.

// moduleHasLegacyMetadataFormat checks if the module has a legacy metadata format.

Fixed 3ee7477

simar7 · 2025-03-08T07:06:37Z

pkg/iac/rego/load.go

+	})
+}
+
+// moduleHasLegacyInputFormat checks if the module has an legacy input format.


Suggested change

// moduleHasLegacyInputFormat checks if the module has an legacy input format.

// moduleHasLegacyInputFormat checks if the module has a legacy input format.

Fixed 3ee7477

simar7 · 2025-03-08T07:14:50Z

pkg/iac/rego/load.go

+
+	if len(metadata.InputOptions.Selectors) == 0 && !metadata.Library {
+		s.logger.Warn(
+			"Module has no input selectors - it will be loaded for all inputs!",


I recall reading somewhere on the Golang website about Go best practices for logging avoiding exclamation marks but I can't find it now.

Suggested change

"Module has no input selectors - it will be loaded for all inputs!",

"Module has no input selectors - it will be loaded for all inputs",

It also talked about case but my memory fails me as to what the recommendation was.

Updated 3ee7477

simar7 · 2025-03-08T07:18:32Z

pkg/iac/rego/metadata.go

@@ -276,6 +285,38 @@ func (m StaticMetadata) ToRule() scan.Rule {
 	}
 }

+func MetadataFromAnnotations(module *ast.Module) (*StaticMetadata, error) {


I see that it is tested with higher level tests but I feel we can write some tests for these function in isolation as it seems to critical and potentially re-usable as it's exported.

Added tests eb72e1d

Signed-off-by: nikpivkin <[email protected]>

nikpivkin force-pushed the perf/metadata branch from 05395f5 to 0ae925c Compare March 3, 2025 19:04

perf(misconf): retrieve check metadata from annotations once

582683e

Signed-off-by: nikpivkin <[email protected]>

nikpivkin force-pushed the perf/metadata branch from 0ae925c to 582683e Compare March 3, 2025 19:34

nikpivkin marked this pull request as ready for review March 6, 2025 08:58

nikpivkin requested a review from simar7 as a code owner March 6, 2025 08:58

simar7 reviewed Mar 8, 2025

View reviewed changes

nikpivkin added 5 commits March 10, 2025 14:10

Merge remote-tracking branch 'origin' into perf/metadata

ce97942

refactor: update comments and logging message

3ee7477

Signed-off-by: nikpivkin <[email protected]>

test: add tests for LoadPoliciesFromDirs

de66947

Signed-off-by: nikpivkin <[email protected]>

test: add tests for MetadataFromAnnotations

eb72e1d

Signed-off-by: nikpivkin <[email protected]>

fix: do not use OPA to load modules

197951e

Signed-off-by: nikpivkin <[email protected]>

simar7 approved these changes Mar 11, 2025

View reviewed changes

simar7 added this pull request to the merge queue Mar 11, 2025

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 11, 2025

Merge remote-tracking branch 'origin' into perf/metadata

d389029

nikpivkin force-pushed the perf/metadata branch from 9052fe0 to d389029 Compare March 11, 2025 04:21

nikpivkin added this pull request to the merge queue Mar 11, 2025

Merged via the queue into aquasecurity:main with commit 7b96351 Mar 11, 2025
12 checks passed

nikpivkin deleted the perf/metadata branch March 11, 2025 05:09

aqua-bot mentioned this pull request Mar 11, 2025

release: v0.61.0 [main] #8507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(misconf): retrieve check metadata from annotations once #8478

perf(misconf): retrieve check metadata from annotations once #8478

nikpivkin commented Mar 3, 2025 •

edited

Loading

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025

nikpivkin Mar 10, 2025

simar7 Mar 8, 2025 •

edited

Loading

nikpivkin Mar 10, 2025

		func isOptionalChecks(path string) bool {
		return strings.HasSuffix(filepath.Dir(filepath.ToSlash(path)), filepath.Join("advanced", "optional"))

	// moduleHasLegacyMetadataFormat checks if the module has an legacy metadata format.
	// moduleHasLegacyMetadataFormat checks if the module has a legacy metadata format.

	// moduleHasLegacyInputFormat checks if the module has an legacy input format.
	// moduleHasLegacyInputFormat checks if the module has a legacy input format.

	"Module has no input selectors - it will be loaded for all inputs!",
	"Module has no input selectors - it will be loaded for all inputs",

perf(misconf): retrieve check metadata from annotations once #8478

perf(misconf): retrieve check metadata from annotations once #8478

Conversation

nikpivkin commented Mar 3, 2025 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simar7 Mar 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikpivkin commented Mar 3, 2025 •

edited

Loading

simar7 Mar 8, 2025 •

edited

Loading