Provide all components as additional input to policy check #1737

yashvardhannanavati · 2024-07-03T06:40:39Z

ec-cli spawns workers with individual components of the input to perform policy check. This commit adds all input components when performing the policy check as an additional parameter. So each worker has access to all input components during policy check

codecov · 2024-07-03T06:46:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.92%. Comparing base (de191f2) to head (a6c123e).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1737      +/-   ##
==========================================
+ Coverage   80.28%   86.92%   +6.64%     
==========================================
  Files          67       78      +11     
  Lines        4874     5231     +357     
==========================================
+ Hits         3913     4547     +634     
+ Misses        961      684     -277

Flag	Coverage Δ
acceptance	`71.84% <100.00%> (?)`
generative	`80.68% <100.00%> (+0.40%)`	⬆️
integration	`80.68% <100.00%> (+0.40%)`	⬆️
unit	`80.68% <100.00%> (+0.40%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
cmd/validate/image.go	`95.23% <100.00%> (+0.31%)`	⬆️
...ation_snapshot_image/application_snapshot_image.go	`77.96% <100.00%> (+20.82%)`	⬆️
internal/image/validate.go	`73.58% <100.00%> (+0.94%)`	⬆️

... and 38 files with indirect coverage changes

zregvart · 2024-07-03T11:24:54Z

cmd/validate/image.go

@@ -42,7 +42,7 @@ import (
 	validate_utils "github.com/enterprise-contract/ec-cli/internal/validate"
 )

-type imageValidationFunc func(context.Context, app.SnapshotComponent, policy.Policy, []evaluator.Evaluator, bool) (*output.Output, error)
+type imageValidationFunc func(context.Context, app.SnapshotComponent, policy.Policy, []evaluator.Evaluator, bool, []app.SnapshotComponent) (*output.Output, error)


Could pass in the whole snapshot spec:

Suggested change

type imageValidationFunc func(context.Context, app.SnapshotComponent, policy.Policy, []evaluator.Evaluator, bool, []app.SnapshotComponent) (*output.Output, error)

type imageValidationFunc func(context.Context, app.SnapshotComponent, app.SnapshotSpec, policy.Policy, []evaluator.Evaluator, bool) (*output.Output, error)

zregvart · 2024-07-03T11:28:06Z

cmd/validate/image.go

 				log.Debugf("Starting worker %d", id)
 				for comp := range jobs {
 					log.Debugf("Worker %d got a component %q", id, comp.ContainerImage)
 					ctx := cmd.Context()
-					out, err := validate(ctx, comp, data.policy, evaluators, data.info)
+					log.Debugf("Worker %d got appComponents %v", id, appComponents)


This might be a bit too verbose, IIRC the whole app snapshot is logged before

zregvart · 2024-07-03T11:30:56Z

internal/evaluation_target/application_snapshot_image/application_snapshot_image.go

 }

 // WriteInputFile writes the JSON from the attestations to input.json in a random temp dir
-func (a *ApplicationSnapshotImage) WriteInputFile(ctx context.Context) (string, []byte, error) {
+func (a *ApplicationSnapshotImage) WriteInputFile(ctx context.Context, allcomp []app.SnapshotComponent) (string, []byte, error) {


Rather that doing this, hold the *app.SnapshotSpec in the ApplicationSnapshotImage struct

zregvart · 2024-07-03T11:32:44Z

internal/image/validate_test.go

@@ -146,7 +146,7 @@ func TestBuiltinChecks(t *testing.T) {
 			client := ecoci.NewClient(ctx)
 			c.setup(client.(*fake.FakeClient))

-			actual, err := ValidateImage(ctx, c.component, p, evaluators, false)
+			actual, err := ValidateImage(ctx, c.component, p, evaluators, false, []app.SnapshotComponent{})


Could make the test a bit more realistic an include the component that is validated in the array

simonbaird · 2024-07-03T17:20:43Z

internal/evaluation_target/application_snapshot_image/application_snapshot_image.go

-	Image        image             `json:"image"`
+	Attestations  []attestationData       `json:"attestations"`
+	Image         image                   `json:"image"`
+	AllComponents []app.SnapshotComponent `json:"allcomponents"`


I think some camel case would be more conventional here:

Suggested change

AllComponents []app.SnapshotComponent `json:"allcomponents"`

AllComponents []app.SnapshotComponent `json:"allComponents"`

simonbaird · 2024-07-03T17:22:40Z

Looks pretty good so far.

simonbaird · 2024-07-03T17:26:36Z

The acceptance test failures looks like some missing allcomponents field in input.json specified in features/__snapshots__/validate_image.snap

yashvardhannanavati · 2024-07-04T09:37:32Z

@simonbaird @zregvart Thank you for the review! I addressed the suggestions. Can I please request you to take another look?

zregvart · 2024-07-04T09:46:59Z

/ok-to-test

zregvart

Naming suggestions, otherwise looks good

zregvart · 2024-07-04T09:48:36Z

internal/evaluation_target/application_snapshot_image/application_snapshot_image.go

@@ -59,21 +59,23 @@ type ApplicationSnapshotImage struct {
 	Evaluators       []evaluator.Evaluator
 	files            map[string]json.RawMessage
 	component        app.SnapshotComponent
+	snap             app.SnapshotSpec


Nit, I'd use the full name

Suggested change

snap app.SnapshotSpec

snapshot app.SnapshotSpec

zregvart · 2024-07-04T09:50:00Z

internal/evaluation_target/application_snapshot_image/application_snapshot_image.go

@@ -326,6 +328,7 @@ type image struct {
 type Input struct {
 	Attestations []attestationData `json:"attestations"`
 	Image        image             `json:"image"`
+	AppSnapshot  app.SnapshotSpec  `json:"appSnapshot"`


Makes it more alike the other fields

Suggested change

AppSnapshot app.SnapshotSpec `json:"appSnapshot"`

AppSnapshot app.SnapshotSpec `json:"snapshot"`

ec-cli spawns workers with individual components of the input to perform policy check. This commit adds all input components when performing the policy check as an additional parameter. So each worker has access to all input components during policy check resolves: CVP-4191 Signed-off-by: Yashvardhan Nanavati <[email protected]>

zregvart · 2024-07-04T12:21:09Z

Thanks!

This commit documents the modification that was introduced to the policy input in enterprise-contract#1737 to include the provided snapshot. Signed-off-by: robnester-rh <[email protected]>

yashvardhannanavati force-pushed the rpa_rule_data branch from 317c90f to 1270117 Compare July 3, 2024 06:50

zregvart reviewed Jul 3, 2024

View reviewed changes

simonbaird reviewed Jul 3, 2024

View reviewed changes

yashvardhannanavati force-pushed the rpa_rule_data branch 8 times, most recently from 4c37650 to cf01797 Compare July 4, 2024 09:30

yashvardhannanavati requested review from simonbaird and zregvart July 4, 2024 09:36

zregvart reviewed Jul 4, 2024

View reviewed changes

yashvardhannanavati force-pushed the rpa_rule_data branch from cf01797 to a6c123e Compare July 4, 2024 10:17

zregvart approved these changes Jul 4, 2024

View reviewed changes

zregvart merged commit 1554775 into enterprise-contract:main Jul 4, 2024
13 checks passed

lcarva mentioned this pull request Oct 3, 2024

Document input.snapshot #2046

Open

robnester-rh mentioned this pull request Oct 24, 2024

EC-905 - Document input.snapshot in policy.input #2108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide all components as additional input to policy check #1737

Provide all components as additional input to policy check #1737

yashvardhannanavati commented Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

zregvart Jul 3, 2024

zregvart Jul 3, 2024

zregvart Jul 3, 2024

zregvart Jul 3, 2024

simonbaird Jul 3, 2024

simonbaird commented Jul 3, 2024

simonbaird commented Jul 3, 2024

yashvardhannanavati commented Jul 4, 2024

zregvart commented Jul 4, 2024

zregvart left a comment

zregvart Jul 4, 2024

yashvardhannanavati Jul 4, 2024

zregvart Jul 4, 2024

yashvardhannanavati Jul 4, 2024

zregvart commented Jul 4, 2024

	type imageValidationFunc func(context.Context, app.SnapshotComponent, policy.Policy, []evaluator.Evaluator, bool, []app.SnapshotComponent) (*output.Output, error)
	type imageValidationFunc func(context.Context, app.SnapshotComponent, app.SnapshotSpec, policy.Policy, []evaluator.Evaluator, bool) (*output.Output, error)

	AllComponents []app.SnapshotComponent `json:"allcomponents"`
	AllComponents []app.SnapshotComponent `json:"allComponents"`

	AppSnapshot app.SnapshotSpec `json:"appSnapshot"`
	AppSnapshot app.SnapshotSpec `json:"snapshot"`

Provide all components as additional input to policy check #1737

Provide all components as additional input to policy check #1737

Conversation

yashvardhannanavati commented Jul 3, 2024

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

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

simonbaird commented Jul 3, 2024

simonbaird commented Jul 3, 2024

yashvardhannanavati commented Jul 4, 2024

zregvart commented Jul 4, 2024

zregvart left a 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

zregvart commented Jul 4, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading