Skip to content

Commit 39af44b

Browse files
joejstuartclaude
andcommitted
fix: address review feedback on OPA evaluator
- Add default case to type switch in evalOPAQuery to surface unexpected result types instead of silently dropping them (fail-open fix) - Use case-sensitive comparison for rule name dedup (OPA rules are case-sensitive) - Fix success padding to use unique rule count instead of total rule occurrences across modules - Wire IsOpaEnabled() into input validation path so EC_USE_OPA=1 actually uses the OPA evaluator for validate input - Propagate error from theEnvironmentVarilableIsSet in acceptance test setup - Fix and rename broken "returns error from init" test - Extract collectRuleNames and evaluateRule helpers from queryNamespace for better separation of concerns and testability - Add TestBuildInput, TestCollectRuleNames, TestEvaluateRule, and unexpected result type test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 09561ba commit 39af44b

5 files changed

Lines changed: 311 additions & 57 deletions

File tree

acceptance/cli/cli.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -861,7 +861,11 @@ func AddStepsTo(sc *godog.ScenarioContext) {
861861
sc.Step(`^a track bundle file named "([^"]*)" containing$`, createTrackBundleFile)
862862
sc.Before(func(ctx context.Context, _ *godog.Scenario) (context.Context, error) {
863863
if v := os.Getenv("EC_USE_OPA"); v != "" {
864-
ctx, _ = theEnvironmentVarilableIsSet(ctx, "EC_USE_OPA="+v)
864+
var err error
865+
ctx, err = theEnvironmentVarilableIsSet(ctx, "EC_USE_OPA="+v)
866+
if err != nil {
867+
return ctx, fmt.Errorf("setting EC_USE_OPA: %w", err)
868+
}
865869
}
866870
return ctx, nil
867871
})

internal/evaluation_target/application_snapshot_image/application_snapshot_image_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,100 @@ func TestWriteInputFileMultipleAttestations(t *testing.T) {
326326
assert.JSONEq(t, string(inputJSON), string(bytes))
327327
}
328328

329+
func TestBuildInput(t *testing.T) {
330+
t.Run("basic round-trip", func(t *testing.T) {
331+
a := ApplicationSnapshotImage{
332+
reference: name.MustParseReference("registry.io/repository/image:tag"),
333+
attestations: []attestation.Attestation{createSimpleAttestation(nil)},
334+
snapshot: app.SnapshotSpec{
335+
Components: []app.SnapshotComponent{
336+
{ContainerImage: "registry.io/repository/image:tag"},
337+
},
338+
},
339+
component: app.SnapshotComponent{
340+
Name: "my-component",
341+
ContainerImage: "registry.io/repository/image:tag",
342+
},
343+
}
344+
345+
parsed, inputJSON, err := a.BuildInput(context.Background())
346+
require.NoError(t, err)
347+
assert.NotNil(t, parsed)
348+
assert.NotEmpty(t, inputJSON)
349+
350+
img, ok := parsed["image"].(map[string]any)
351+
require.True(t, ok)
352+
assert.Equal(t, "registry.io/repository/image:tag", img["ref"])
353+
354+
assert.Equal(t, "my-component", parsed["component_name"])
355+
356+
var fromJSON map[string]any
357+
require.NoError(t, json.Unmarshal(inputJSON, &fromJSON))
358+
assert.Equal(t, parsed["component_name"], fromJSON["component_name"])
359+
})
360+
361+
t.Run("with parent image", func(t *testing.T) {
362+
a := ApplicationSnapshotImage{
363+
reference: name.MustParseReference("registry.io/repository/image:tag"),
364+
parentRef: name.MustParseReference("registry.io/repository/parent:latest"),
365+
parentConfigJSON: json.RawMessage(`{"Labels":{"io.k8s.display-name":"Base"}}`),
366+
}
367+
368+
parsed, _, err := a.BuildInput(context.Background())
369+
require.NoError(t, err)
370+
371+
img, ok := parsed["image"].(map[string]any)
372+
require.True(t, ok)
373+
parent, ok := img["parent"].(map[string]any)
374+
require.True(t, ok)
375+
assert.Equal(t, "registry.io/repository/parent:latest", parent["ref"])
376+
})
377+
378+
t.Run("without parent image", func(t *testing.T) {
379+
a := ApplicationSnapshotImage{
380+
reference: name.MustParseReference("registry.io/repository/image:tag"),
381+
}
382+
383+
parsed, _, err := a.BuildInput(context.Background())
384+
require.NoError(t, err)
385+
386+
img, ok := parsed["image"].(map[string]any)
387+
require.True(t, ok)
388+
parent, exists := img["parent"]
389+
if exists {
390+
parentMap, ok := parent.(map[string]any)
391+
assert.True(t, !ok || parentMap["ref"] == nil || parentMap["ref"] == "")
392+
}
393+
})
394+
395+
t.Run("matches WriteInputFile output", func(t *testing.T) {
396+
a := ApplicationSnapshotImage{
397+
reference: name.MustParseReference("registry.io/repository/image:tag"),
398+
attestations: []attestation.Attestation{createSimpleAttestation(nil)},
399+
snapshot: app.SnapshotSpec{
400+
Components: []app.SnapshotComponent{
401+
{ContainerImage: "registry.io/repository/image:tag"},
402+
},
403+
},
404+
component: app.SnapshotComponent{
405+
Name: "test-component",
406+
ContainerImage: "registry.io/repository/image:tag",
407+
},
408+
}
409+
410+
parsed, buildJSON, err := a.BuildInput(context.Background())
411+
require.NoError(t, err)
412+
413+
fs := afero.NewMemMapFs()
414+
ctx := utils.WithFS(context.Background(), fs)
415+
_, writeJSON, err := a.WriteInputFile(ctx)
416+
require.NoError(t, err)
417+
418+
assert.JSONEq(t, string(writeJSON), string(buildJSON))
419+
assert.NotNil(t, parsed)
420+
})
421+
}
422+
329423
func TestNewApplicationSnapshotImage(t *testing.T) {
330424
ctx := context.Background()
331425

internal/evaluation_target/input/input.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@ import (
2424
"github.com/conforma/cli/internal/evaluator"
2525
"github.com/conforma/cli/internal/policy"
2626
"github.com/conforma/cli/internal/policy/source"
27+
"github.com/conforma/cli/internal/utils"
2728
)
2829

2930
var newConftestEvaluator = evaluator.NewConftestEvaluator
31+
var newOPAEvaluator = evaluator.NewOPAEvaluator
3032

3133
// Input represents the structure needed to evaluate a generic file input
3234
type Input struct {
@@ -48,13 +50,19 @@ func NewInput(ctx context.Context, paths []string, p policy.Policy) (*Input, err
4850
log.Debugf("policySource: %#v", policySource)
4951
}
5052

51-
c, err := newConftestEvaluator(ctx, policySources, p, sourceGroup)
53+
var c evaluator.Evaluator
54+
var err error
55+
if utils.IsOpaEnabled() {
56+
c, err = newOPAEvaluator(ctx, policySources, p, sourceGroup, nil)
57+
} else {
58+
c, err = newConftestEvaluator(ctx, policySources, p, sourceGroup)
59+
}
5260
if err != nil {
53-
log.Debug("Failed to initialize the conftest evaluator!")
61+
log.Debug("Failed to initialize the evaluator!")
5462
return nil, err
5563
}
5664

57-
log.Debug("Conftest evaluator initialized")
65+
log.Debug("Evaluator initialized")
5866
i.Evaluators = append(i.Evaluators, c)
5967

6068
}

internal/evaluator/opa_evaluator.go

Lines changed: 68 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -240,8 +240,31 @@ func (o *opaEvaluator) queryNamespace(ctx context.Context, fileName string, inpu
240240
Namespace: namespace,
241241
}
242242

243+
ruleNames := o.collectRuleNames(namespace)
244+
245+
var successes int
246+
for _, ruleName := range ruleNames {
247+
ruleResult, err := o.evaluateRule(ctx, input, namespace, ruleName)
248+
if err != nil {
249+
return Outcome{}, err
250+
}
251+
successes += ruleResult.successes
252+
outcome.Failures = append(outcome.Failures, ruleResult.failures...)
253+
outcome.Warnings = append(outcome.Warnings, ruleResult.warnings...)
254+
outcome.Exceptions = append(outcome.Exceptions, ruleResult.exceptions...)
255+
}
256+
257+
resultCount := len(outcome.Failures) + len(outcome.Warnings) + len(outcome.Exceptions) + successes
258+
if resultCount < len(ruleNames) {
259+
successes += len(ruleNames) - resultCount
260+
}
261+
outcome.Successes = make([]Result, successes)
262+
263+
return outcome, nil
264+
}
265+
266+
func (o *opaEvaluator) collectRuleNames(namespace string) []string {
243267
var ruleNames []string
244-
var ruleCount int
245268
for _, module := range o.engine.Modules() {
246269
ns := strings.Replace(module.Package.Path.String(), "data.", "", 1)
247270
if ns != namespace {
@@ -252,10 +275,9 @@ func (o *opaEvaluator) queryNamespace(ctx context.Context, fileName string, inpu
252275
if !isOPAFailure(name) && !isOPAWarning(name) {
253276
continue
254277
}
255-
ruleCount++
256278
found := false
257279
for _, existing := range ruleNames {
258-
if strings.EqualFold(existing, name) {
280+
if existing == name {
259281
found = true
260282
break
261283
}
@@ -265,52 +287,53 @@ func (o *opaEvaluator) queryNamespace(ctx context.Context, fileName string, inpu
265287
}
266288
}
267289
}
290+
return ruleNames
291+
}
268292

269-
var successes int
270-
for _, ruleName := range ruleNames {
271-
exceptionQuery := fmt.Sprintf("data.%s.exception[_][_] == %q", namespace, stripRulePrefix(ruleName))
272-
exceptionResults, err := o.evalOPAQuery(ctx, input, exceptionQuery)
273-
if err != nil {
274-
return Outcome{}, fmt.Errorf("query exception: %w", err)
275-
}
293+
type ruleEvalResult struct {
294+
failures []Result
295+
warnings []Result
296+
exceptions []Result
297+
successes int
298+
}
276299

277-
var exceptions []Result
278-
for _, er := range exceptionResults {
279-
if er.Message == "" {
280-
exceptions = append(exceptions, Result{Message: exceptionQuery})
281-
}
282-
}
300+
func (o *opaEvaluator) evaluateRule(ctx context.Context, input any, namespace string, ruleName string) (ruleEvalResult, error) {
301+
var result ruleEvalResult
283302

284-
ruleQuery := fmt.Sprintf("data.%s.%s", namespace, ruleName)
285-
ruleResults, err := o.evalOPAQuery(ctx, input, ruleQuery)
286-
if err != nil {
287-
return Outcome{}, fmt.Errorf("query rule: %w", err)
288-
}
303+
exceptionQuery := fmt.Sprintf("data.%s.exception[_][_] == %q", namespace, stripRulePrefix(ruleName))
304+
exceptionResults, err := o.evalOPAQuery(ctx, input, exceptionQuery)
305+
if err != nil {
306+
return result, fmt.Errorf("query exception: %w", err)
307+
}
289308

290-
for _, rr := range ruleResults {
291-
if len(exceptions) > 0 {
292-
continue
293-
}
294-
if rr.Message == "" {
295-
successes++
296-
continue
297-
}
298-
if isOPAFailure(ruleName) {
299-
outcome.Failures = append(outcome.Failures, rr)
300-
} else {
301-
outcome.Warnings = append(outcome.Warnings, rr)
302-
}
309+
for _, er := range exceptionResults {
310+
if er.Message == "" {
311+
result.exceptions = append(result.exceptions, Result{Message: exceptionQuery})
303312
}
304-
outcome.Exceptions = append(outcome.Exceptions, exceptions...)
305313
}
306314

307-
resultCount := len(outcome.Failures) + len(outcome.Warnings) + len(outcome.Exceptions) + successes
308-
if resultCount < ruleCount {
309-
successes += ruleCount - resultCount
315+
ruleQuery := fmt.Sprintf("data.%s.%s", namespace, ruleName)
316+
ruleResults, err := o.evalOPAQuery(ctx, input, ruleQuery)
317+
if err != nil {
318+
return result, fmt.Errorf("query rule: %w", err)
310319
}
311-
outcome.Successes = make([]Result, successes)
312320

313-
return outcome, nil
321+
for _, rr := range ruleResults {
322+
if len(result.exceptions) > 0 {
323+
continue
324+
}
325+
if rr.Message == "" {
326+
result.successes++
327+
continue
328+
}
329+
if isOPAFailure(ruleName) {
330+
result.failures = append(result.failures, rr)
331+
} else {
332+
result.warnings = append(result.warnings, rr)
333+
}
334+
}
335+
336+
return result, nil
314337
}
315338

316339
func (o *opaEvaluator) evalOPAQuery(ctx context.Context, input any, query string) ([]Result, error) {
@@ -372,6 +395,11 @@ func (o *opaEvaluator) evalOPAQuery(ctx context.Context, input any, query string
372395
Message: msg,
373396
Metadata: metadata,
374397
})
398+
default:
399+
results = append(results, Result{
400+
Message: fmt.Sprintf("unexpected policy result type %T: %v", v, v),
401+
Metadata: map[string]any{},
402+
})
375403
}
376404
}
377405
}

0 commit comments

Comments
 (0)