feat: add direct OPA evaluator with shared base infrastructure#3276
Draft
joejstuart wants to merge 8 commits intoconforma:mainfrom
Draft
feat: add direct OPA evaluator with shared base infrastructure#3276joejstuart wants to merge 8 commits intoconforma:mainfrom
joejstuart wants to merge 8 commits intoconforma:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request implements a functional OPA evaluator by introducing a shared ChangesOPA Evaluator Implementation
Sequence DiagramsequenceDiagram
participant Client as Validate Command
participant Input as ApplicationSnapshotImage
participant Build as BuildInput
participant Eval as OPA Evaluator
participant Base as Base Evaluator
participant Policy as Policy Sources
participant Engine as OPA Engine
participant Rules as Rule Registry
Client->>Input: ValidateImage(ctx)
Input->>Build: BuildInput(ctx)
Build-->>Input: (parsedInput map, JSON bytes)
Client->>Eval: NewOPAEvaluator(ctx, policySources, ...)
Eval->>Base: Initialize basePolicyEvaluator
Base->>Base: initWorkDir, createDataDirectory
Client->>Eval: Evaluate(ctx, target{ParsedInput, Inputs})
Eval->>Eval: ensureInitialized(ctx)
Eval->>Base: downloadAndInspectPolicies(ctx)
Base->>Policy: Download policy sources
Base->>Base: Extract rule annotations, discover data dirs
Base->>Rules: Register rules with metadata
Eval->>Engine: compileEngine()
Engine->>Engine: Load capabilities, build OPA engine
Eval->>Eval: evaluateWithEngine(target, namespaces)
Eval->>Engine: Query deny/violation/warn rules
Engine-->>Eval: Rule results + exceptions
Eval->>Base: postProcessResults(outcomes)
Base->>Base: Enrich with metadata, compute successes
Base->>Base: Apply filtering, validate rule coverage
Base-->>Eval: Final Outcome[]
Eval-->>Client: Outcome[] with aggregated results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/image/validate.go (1)
109-119: ⚡ Quick winAvoid building the same policy input twice per image.
BuildInputalready marshals/unmarshals the full payload, andWriteInputFilethen marshals it again immediately afterward. On the conftest path this is pure overhead in a hot loop, and it also creates two places that must stay semantically identical. Prefer a single shared builder, or only populateParsedInputwhen an OPA evaluator is actually present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/image/validate.go` around lines 109 - 119, The code currently calls BuildInput and then WriteInputFile, causing duplicate marshaling; instead call BuildInput once and reuse its result when writing files or populating ParsedInput. Modify WriteInputFile (or add an overload) to accept the already-built inputJSON/inputMap from BuildInput so it does not re-marshal, and only populate a.ParsedInput (or run the OPA/conftest path) when an OPA evaluator/conftest is actually present; update callers to pass the BuildInput output into WriteInputFile or skip WriteInputFile when no evaluator is used (referencing BuildInput, WriteInputFile, and ParsedInput in your changes).internal/evaluator/opa_evaluator.go (1)
243-267: ⚡ Quick winCache rule discovery per namespace after compilation.
This rescans every compiled module for every
(namespace, input)pair, then does an O(n²) dedupe on top. With many inputs, rule discovery becomes part of the hot path even thougho.engine.Modules()is immutable aftercompileEngine(). Precomputing amap[string][]stringonce during initialization would remove that repeated AST walk and simplifyqueryNamespace.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/evaluator/opa_evaluator.go` around lines 243 - 267, Precompute and cache discovery of OPA failure/warning rule names per namespace during engine initialization (e.g., in compileEngine or immediately after it) instead of recomputing inside queryNamespace by walking o.engine.Modules() each time; build a map[string][]string (namespace -> ruleNames) using the same filtering logic (isOPAFailure/isOPAWarning and dedupe) and store it on the evaluator struct, update usages of ruleNames/ruleCount in queryNamespace to read from that cache, and ensure cache is populated once after compileEngine() so o.engine.Modules() is not rescanned for every (namespace, input) pair.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/evaluator/base_evaluator.go`:
- Around line 332-333: computeSuccesses is calling FilterResults with time.Now()
and discarding its returned missingIncludes; change those calls to pass the
evaluator's effective time (use b.effectiveTime or the evaluator's effectiveTime
field) instead of time.Now(), and capture/propagate the missingIncludes return
value so result.Successes is computed with the same effective time and the
missingIncludes set is not lost; apply the same fix to the other FilterResults
call in the same file (the block around the alternate call at ~406-412) so both
successes and the other filter use the evaluator effective time and preserve
missingIncludes.
- Around line 307-325: The loop always constructs
NewUnifiedPostEvaluationFilter, which overrides any custom filter set by
NewConftestEvaluatorWithPostEvaluationFilter; change the code to use the
evaluator's configured post-evaluation filter (e.g., b.postEvaluationFilter)
when present, falling back to NewUnifiedPostEvaluationFilter(b.policyResolver)
only if b.postEvaluationFilter is nil, and then call FilterResults and
CategorizeResults on that chosen filter instance so custom filters are preserved
(apply this to the variables used around FilterResults/CategorizeResults).
---
Nitpick comments:
In `@internal/evaluator/opa_evaluator.go`:
- Around line 243-267: Precompute and cache discovery of OPA failure/warning
rule names per namespace during engine initialization (e.g., in compileEngine or
immediately after it) instead of recomputing inside queryNamespace by walking
o.engine.Modules() each time; build a map[string][]string (namespace ->
ruleNames) using the same filtering logic (isOPAFailure/isOPAWarning and dedupe)
and store it on the evaluator struct, update usages of ruleNames/ruleCount in
queryNamespace to read from that cache, and ensure cache is populated once after
compileEngine() so o.engine.Modules() is not rescanned for every (namespace,
input) pair.
In `@internal/image/validate.go`:
- Around line 109-119: The code currently calls BuildInput and then
WriteInputFile, causing duplicate marshaling; instead call BuildInput once and
reuse its result when writing files or populating ParsedInput. Modify
WriteInputFile (or add an overload) to accept the already-built
inputJSON/inputMap from BuildInput so it does not re-marshal, and only populate
a.ParsedInput (or run the OPA/conftest path) when an OPA evaluator/conftest is
actually present; update callers to pass the BuildInput output into
WriteInputFile or skip WriteInputFile when no evaluator is used (referencing
BuildInput, WriteInputFile, and ParsedInput in your changes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 46c4d173-fd5a-48f1-94ce-2a52bf4b8246
📒 Files selected for processing (9)
cmd/validate/image.gointernal/evaluation_target/application_snapshot_image/application_snapshot_image.gointernal/evaluator/base_evaluator.gointernal/evaluator/conftest_evaluator.gointernal/evaluator/conftest_evaluator_unit_data_test.gointernal/evaluator/evaluator.gointernal/evaluator/opa_evaluator.gointernal/evaluator/opa_evaluator_test.gointernal/image/validate.go
Comment on lines
+307
to
+325
| for _, result := range runResults { | ||
| unifiedFilter := NewUnifiedPostEvaluationFilter(b.policyResolver) | ||
|
|
||
| allResults := []Result{} | ||
| allResults = append(allResults, result.Warnings...) | ||
| allResults = append(allResults, result.Failures...) | ||
| allResults = append(allResults, result.Exceptions...) | ||
| allResults = append(allResults, result.Skipped...) | ||
|
|
||
| for j := range allResults { | ||
| addRuleMetadata(ctx, &allResults[j], b.rules) | ||
| } | ||
|
|
||
| filteredResults, updatedMissingIncludes := unifiedFilter.FilterResults( | ||
| allResults, b.allRules, target.Target, missingIncludes, effectiveTime) | ||
| missingIncludes = updatedMissingIncludes | ||
|
|
||
| warnings, failures, exceptions, skipped := unifiedFilter.CategorizeResults( | ||
| filteredResults, result, effectiveTime) |
There was a problem hiding this comment.
Preserve caller-supplied post-evaluation filters.
postProcessResults now always creates NewUnifiedPostEvaluationFilter(...), so NewConftestEvaluatorWithPostEvaluationFilter no longer changes behavior after this refactor. That is a functional regression for any path relying on a custom filter.
Suggested fix
-func (b *basePolicyEvaluator) postProcessResults(ctx context.Context, runResults []Outcome, target EvaluationTarget) ([]Outcome, error) {
+func (b *basePolicyEvaluator) postProcessResults(ctx context.Context, runResults []Outcome, target EvaluationTarget, filter PostEvaluationFilter) ([]Outcome, error) {
...
for _, result := range runResults {
- unifiedFilter := NewUnifiedPostEvaluationFilter(b.policyResolver)
+ unifiedFilter := filter
+ if unifiedFilter == nil {
+ unifiedFilter = NewUnifiedPostEvaluationFilter(b.policyResolver)
+ }
...
}-return c.postProcessResults(ctx, runResults, target)
+return c.postProcessResults(ctx, runResults, target, c.postEvaluationFilter)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/evaluator/base_evaluator.go` around lines 307 - 325, The loop always
constructs NewUnifiedPostEvaluationFilter, which overrides any custom filter set
by NewConftestEvaluatorWithPostEvaluationFilter; change the code to use the
evaluator's configured post-evaluation filter (e.g., b.postEvaluationFilter)
when present, falling back to NewUnifiedPostEvaluationFilter(b.policyResolver)
only if b.postEvaluationFilter is nil, and then call FilterResults and
CategorizeResults on that chosen filter instance so custom filters are preserved
(apply this to the variables used around FilterResults/CategorizeResults).
Comment on lines
+332
to
+333
| result.Successes = b.computeSuccesses(result, b.rules, target.Target, missingIncludes, unifiedFilter) | ||
|
|
There was a problem hiding this comment.
Success filtering is using the wrong clock and dropping include matches.
The FilterResults call inside computeSuccesses uses time.Now() instead of the evaluator's effective time, and it ignores the returned missingIncludes. That means a rule matched only by a success can still trigger the later "Include criterion ... doesn't match any policy rule" warning, and backdated/effective-time runs can report successes differently from failures and warnings.
Also applies to: 406-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/evaluator/base_evaluator.go` around lines 332 - 333,
computeSuccesses is calling FilterResults with time.Now() and discarding its
returned missingIncludes; change those calls to pass the evaluator's effective
time (use b.effectiveTime or the evaluator's effectiveTime field) instead of
time.Now(), and capture/propagate the missingIncludes return value so
result.Successes is computed with the same effective time and the
missingIncludes set is not lost; apply the same fix to the other FilterResults
call in the same file (the block around the alternate call at ~406-412) so both
successes and the other filter use the evaluator effective time and preserve
missingIncludes.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
joejstuart
force-pushed
the
opa-evaluator
branch
2 times, most recently
from
8dbf508 to
adbdb0a
Compare
Add a new opaEvaluator that evaluates policies directly via OPA's rego API instead of going through conftest's runner. This eliminates the conftest runner overhead while reusing conftest's Engine for policy compilation and data loading. Extract shared infrastructure into basePolicyEvaluator to eliminate ~400 lines of duplication between the two evaluators. Both now share policy download/inspection, data directory preparation, capabilities management, post-processing, and success computation. Key changes: - New opaEvaluator with direct rego.Eval() queries matching conftest's engine.Check() semantics (same regexes, exception handling, success counting) - basePolicyEvaluator embedded struct with 12 shared methods - sync.Once lazy initialization on both evaluators to prevent data races from concurrent worker goroutines - BuildInput() on ApplicationSnapshotImage for in-memory OPA input delivery without disk I/O - ParsedInput field on EvaluationTarget for passing pre-parsed input - EC_USE_OPA=1 environment variable gate for selecting the OPA evaluator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BuildInput was missing ComponentName and PolicySpec fields that WriteInputFile includes, causing acceptance test snapshot failures. Also fix gci import ordering in fallback.go and tidy acceptance/go.mod. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add unit tests covering evalOPAQuery, queryNamespace, evaluateWithEngine, helper functions (isOPAFailure, isOPAWarning, stripRulePrefix), input parsing, and base evaluator methods (prepareDataDirs, computeSuccesses, postProcessResults, createDataDirectory, createCapabilitiesFile, initWorkDir, initPolicyResolver, resolveFilteredNamespaces, isResultIncluded). Also fix gci import ordering in base_evaluator.go and opa_evaluator.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Full end-to-end integration tests for the OPA evaluator covering: - Basic creation and capabilities path - Evaluation with file-based and parsed input - Deny/warn semantics (both triggered, warn only, all pass) - Component-name-based VolatileConfig exclude filtering Mirrors the existing conftest evaluator integration tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Run both evaluators against the same policy and input, assert identical outcomes (failure/warning/success codes and messages). Covers deny, warn, conditional rules, multiple rules, parsed vs file input, and component-name-based VolatileConfig filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Inject EC_USE_OPA from the test runner's process environment into every acceptance scenario's ec binary invocation, enabling `EC_USE_OPA=1 make acceptance` to run the full suite with the OPA evaluator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add validate_image_opa.feature (7 scenarios) and validate_input_opa.feature (2 scenarios) that exercise the OPA evaluator end-to-end via EC_USE_OPA=1. Covers happy day, rejection, multiple sources, rule filtering, future deny conversion, volatile config, and input validation paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Generated by running the OPA acceptance test scenarios locally. These snapshots enable CI to validate OPA evaluator output matches expected results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add a new opaEvaluator that evaluates policies directly via OPA's rego API instead of going through conftest's runner. This eliminates the conftest runner overhead while reusing conftest's Engine for policy compilation and data loading.
Extract shared infrastructure into basePolicyEvaluator to eliminate ~400 lines of duplication between the two evaluators. Both now share policy download/inspection, data directory preparation, capabilities management, post-processing, and success computation.
Key changes: