Migrate trusty eval engine to Trusty v2 API. #5013

blkt · 2024-11-20T17:41:29Z

Summary

This change migrates trusty evaluation engine to use Trusty v2 API. Most of the changes apply to the intermediate representation we recently added to decouple Trusty from Minder, but some additional changes were required due to some fields becoming optional and nullable.

Note: this change is again meant to be bug-compatible with the current evaluation engine, so we treat the lack of "score" as a score of 0, thus triggering false positives as done by the current engine. The idea is to build on top of this to fix known issues of the engine before migrating it to Data Sources.

Fixes stacklok/minder-stories#77

Change Type

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Updated unit tests, ran smoke tests locally.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

blkt · 2024-11-20T17:43:00Z

go.mod

@@ -70,7 +70,7 @@ require (
 	github.com/spf13/viper v1.19.0
 	github.com/sqlc-dev/pqtype v0.3.0
 	github.com/stacklok/frizbee v0.1.4
-	github.com/stacklok/trusty-sdk-go v0.2.2
+	github.com/stacklok/trusty-sdk-go v0.2.3-0.20241120172703-3643fb488d5e


We need this PR to be merged to remove this.

The PR is approved though

It was not 100% ready, some optional changes were requested that I wanted to address first.
I merged it now, we're ok.

blkt · 2024-11-20T22:57:27Z

internal/engine/eval/trusty/actions.go

@@ -324,7 +324,7 @@ func (sph *summaryPrHandler) generateSummary() (string, error) {
 			// (2) we don't suggest malicious packages, I
 			// suggest getting rid of this check
 			// altogether.
-			if altData.Score != nil && *altData.Score <= lowScorePackages[alternative.Dependency.Name].Score {
+			if altData.Score != nil && *altData.Score != 0 && *altData.Score <= lowScorePackages[alternative.Dependency.Name].Score {


The diff here is a bit confusing: this only adds *altData.Score != 0 which fixes bug #4945

should this be moved to a small and dedicated function with an appropriate explanation? you're right that it's a bit confusing and having some docs in the code would probably help.

coveralls · 2024-11-21T08:37:15Z

coverage: 54.582% (-0.05%) from 54.633%
when pulling 026b68d on feat/trusty-engine-v2-api
into 0c315ea on main.

This change migrates `trusty` evaluation engine to use Trusty v2 API. Most of the changes apply to the intermediate representation we recently added to decouple Trusty from Minder, but some additional changes were required due to some fields becoming optional and nullable. Note: this change is again meant to be bug-compatible with the current evaluation engine, so we treat the lack of `"score"` as a score of `0`, thus triggering false positives as done by the current engine. The idea is to build on top of this to fix known issues of the engine before migrating it to Data Sources. Fixes stacklok/minder-stories#77

evankanderson

A few small comments, but nothing to block you if you want to go ahead as-is.

evankanderson · 2024-11-22T18:56:20Z

internal/engine/eval/trusty/trusty.go

+	respSummary, err := trustyClient.Summary(ctx, input)
+	if err != nil {
+		return nil, fmt.Errorf("failed getting summary: %w", err)
+	}
+
+	respPkg, err := trustyClient.PackageMetadata(ctx, input)
+	if err != nil {
+		return nil, fmt.Errorf("failed getting package metadata: %w", err)
+	}
+
+	respAlternatives, err := trustyClient.Alternatives(ctx, input)


Not required, but you might want to issue these calls in parallel, since they may be around a second each. It's a bunch more code, though, so it's also fine to just leave a comment for now:

Suggested change

respSummary, err := trustyClient.Summary(ctx, input)

if err != nil {

return nil, fmt.Errorf("failed getting summary: %w", err)

}

respPkg, err := trustyClient.PackageMetadata(ctx, input)

if err != nil {

return nil, fmt.Errorf("failed getting package metadata: %w", err)

}

respAlternatives, err := trustyClient.Alternatives(ctx, input)

summary := make(chan *v2types.PackageSummaryAnnotation, 1)

metadata := make(chan *v2types.TrustyPackageMetadata, 1)

alternatives := make(chan *v2types.PackageAlternatives, 1)

errors := make(chan error)

defer go func() {

close(summary)

close(metadata)

close(alternatives)

close(errors)

}

go func() {

resp, err := trustyClient.Summary(ctx, input)

errors <- err

summary <- resp

}()

go func() {

resp, err := trustyClient.PackageMetadata(ctx, input)

errors <- err

metadata <- resp

}()

go func() {

resp, err := trustyClient.Alternatives(ctx, input)

errors <- err

alternatives <- resp

}

for (int i = 0; i < 3; i++) {

err := <- errors

if err != nil {

return nil, fmt.Errorf("Trusty call failed: %w", err)

}

}

respSummary := <- summary

respPkg := <- metadata

respAlternatives := <-alternatives

evankanderson · 2024-11-22T18:57:41Z

internal/engine/eval/trusty/trusty.go

 	}

-	res := makeTrustyReport(dep, resp)
+	respProvenance, err := trustyClient.Provenance(ctx, input)


(This would follow the same pattern above)

evankanderson · 2024-11-22T18:57:57Z

internal/engine/eval/trusty/trusty.go

 	}

-	res := makeTrustyReport(dep, resp)
+	respProvenance, err := trustyClient.Provenance(ctx, input)


Do you know whether the Trusty client is emitting metrics on these calls that we could use to track performance over time?

evankanderson · 2024-11-22T19:03:28Z

internal/engine/eval/trusty/actions.go

+const (
+	better packageComparison = iota
+	worse
+)


Can we use the same contract as e.g. slices.SortFunc and use positive numbers to indicate a > b, and negative numbers for a < b?

That would also allow us to use slices.SortFunc elsewhere for lists of packages with the comparison function.

evankanderson · 2024-11-22T19:18:22Z

internal/engine/eval/trusty/actions.go

@@ -428,8 +448,12 @@ func newSummaryPrHandler(
 	}, nil
 }

-func preprocessDetails(s string) string {
-	scanner := bufio.NewScanner(strings.NewReader(s))
+func preprocessDetails(s *string) string {


Why change this to take a *string? (If so, this looks like the correct behavior)

evankanderson · 2024-11-22T19:24:02Z

internal/engine/eval/trusty/trusty.go


 	return res
 }

-func makeScoreComponents(descr map[string]any) []scoreComponent {
+func addSummaryDetails(res *trustyReport, resp *trustytypes.PackageSummaryAnnotation) {


Can we get a nil annotation response without an error? If not, maybe pass through the concrete type or document that it can't be nil.

evankanderson · 2024-11-22T19:27:04Z

internal/engine/eval/trusty/trusty.go

-			Score:       &alt.Score,
+			Score:       alt.Score,


👍 to removing pointer types where possible; it reduces the number of times we need to remember nil checks.

blkt self-assigned this Nov 20, 2024

blkt commented Nov 20, 2024

View reviewed changes

blkt force-pushed the feat/trusty-engine-v2-api branch from 5432bcd to c4601e0 Compare November 21, 2024 08:25

blkt force-pushed the feat/trusty-engine-v2-api branch from c4601e0 to 5af08e7 Compare November 21, 2024 16:17

blkt added 2 commits November 22, 2024 13:09

Refactor package comparison in a separate routine.

026b68d

blkt force-pushed the feat/trusty-engine-v2-api branch from 6b37f59 to 026b68d Compare November 22, 2024 12:09

blkt marked this pull request as ready for review November 22, 2024 12:23

blkt requested a review from a team as a code owner November 22, 2024 12:23

evankanderson approved these changes Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate trusty eval engine to Trusty v2 API. #5013

Migrate trusty eval engine to Trusty v2 API. #5013

blkt commented Nov 20, 2024

blkt Nov 20, 2024

JAORMX Nov 21, 2024

blkt Nov 21, 2024

blkt Nov 20, 2024

JAORMX Nov 21, 2024

coveralls commented Nov 21, 2024 •

edited

Loading

evankanderson left a comment

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

evankanderson Nov 22, 2024

-	respSummary, err := trustyClient.Summary(ctx, input)
-	if err != nil {
-		return nil, fmt.Errorf("failed getting summary: %w", err)
-	}
-	respPkg, err := trustyClient.PackageMetadata(ctx, input)
-	if err != nil {
-		return nil, fmt.Errorf("failed getting package metadata: %w", err)
-	}
-	respAlternatives, err := trustyClient.Alternatives(ctx, input)
+	summary := make(chan *v2types.PackageSummaryAnnotation, 1)
+	metadata := make(chan *v2types.TrustyPackageMetadata, 1)
+	alternatives := make(chan *v2types.PackageAlternatives, 1)
+	errors := make(chan error)
+	defer go func() {
+		close(summary)
+		close(metadata)
+		close(alternatives)
+		close(errors)
+	}
+	go func() {
+		resp, err := trustyClient.Summary(ctx, input)
+		errors <- err
+		summary <- resp
+	}()
+	go func() {
+		resp, err := trustyClient.PackageMetadata(ctx, input)
+		errors <- err
+		metadata <- resp
+	}()
+	go func() {
+		resp, err := trustyClient.Alternatives(ctx, input)
+		errors <- err
+		alternatives <- resp
+	}
+	for (int i = 0; i < 3; i++) {
+		err := <- errors
+		if err != nil {
+			return nil, fmt.Errorf("Trusty call failed: %w", err)
+		}
+	}
+	respSummary := <- summary
+	respPkg := <- metadata
+	respAlternatives := <-alternatives

Migrate trusty eval engine to Trusty v2 API. #5013

Are you sure you want to change the base?

Migrate trusty eval engine to Trusty v2 API. #5013

Conversation

blkt commented Nov 20, 2024

Summary

Change Type

Testing

Review 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

coveralls commented Nov 21, 2024 • edited Loading

evankanderson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 21, 2024 •

edited

Loading