Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate trusty eval engine to Trusty v2 API. #5013

Merged
merged 3 commits into from
Nov 25, 2024
+212 −119
Merged

Migrate trusty eval engine to Trusty v2 API. #5013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6e8110
Migrate trusty eval engine to Trusty v2 API.
blkt Nov 20, 2024
1b2a180
Refactor package comparison in a separate routine.
blkt Nov 22, 2024
bae5027
Address PR comments.
blkt Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.20241121160719-089f44e88687
github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.1
github.com/stretchr/testify v1.10.0
github.com/styrainc/regal v0.29.2
Expand Down Expand Up @@ -382,7 +382,7 @@ require (
golang.org/x/mod v0.22.0
golang.org/x/net v0.31.0 // indirect
golang.org/x/sys v0.27.0 // indirect
golang.org/x/text v0.20.0
golang.org/x/text v0.20.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20241118233622-e639e219e697 // indirect
gopkg.in/ini.v1 v1.67.0 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,8 +1019,8 @@ github.com/sqlc-dev/pqtype v0.3.0 h1:b09TewZ3cSnO5+M1Kqq05y0+OjqIptxELaSayg7bmqk
github.com/sqlc-dev/pqtype v0.3.0/go.mod h1:oyUjp5981ctiL9UYvj1bVvCKi8OXkCa0u645hce7CAs=
github.com/stacklok/frizbee v0.1.4 h1:00v6/2HBmwzNdOyVAP4e1isOeUAIWTlb5eggoNUpHmk=
github.com/stacklok/frizbee v0.1.4/go.mod h1:rFA90VkGFYLb7qCiUniAihmkgXfZAj2BnfF6jR8Csro=
github.com/stacklok/trusty-sdk-go v0.2.2 h1:55B2DrneLYZXxBaNEeyRMGac7mj+pFbrKomx/hSxUyI=
github.com/stacklok/trusty-sdk-go v0.2.2/go.mod h1:BbXNVVT32meuxyJuO4pmXRzVPjc/9AXob2sBbOPiKpk=
github.com/stacklok/trusty-sdk-go v0.2.3-0.20241121160719-089f44e88687 h1:TIZiO871n9V6sSN+bKsG5SwQ4ZHwGtxcmfcL3siimcY=
github.com/stacklok/trusty-sdk-go v0.2.3-0.20241121160719-089f44e88687/go.mod h1:QR01jLW/yfwcXY38dwDpgeEjVc2MAR1LycH1fXtoSXs=
github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.1 h1:/m2cTZHpqgofDsrwPqsASI6fSNMNhb+9EmUYtHEV2Uk=
github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.1/go.mod h1:Z5KcoM0YLC7INlNhEezeIZ0TZNYf7WSNO0Lvah4DSeQ=
github.com/stoewer/go-strcase v1.3.0 h1:g0eASXYtp+yvN9fK8sH94oCIk0fau9uV1/ZdJ0AVEzs=
Expand Down
36 changes: 30 additions & 6 deletions internal/engine/eval/trusty/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ func (sph *summaryPrHandler) generateSummary() (string, error) {
malicious = append(malicious, maliciousTemplateData{
templatePackageData: packageData,
Summary: alternative.trustyReply.Malicious.Summary,
Details: preprocessDetails(alternative.trustyReply.Malicious.Details),
Details: alternative.trustyReply.Malicious.Details,
})
continue
}
Expand All @@ -323,8 +323,9 @@ func (sph *summaryPrHandler) generateSummary() (string, error) {
// Since (1) we don't have score anymore, and
// (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 {
// altogether and always report all available
// alternatives.
if comparePackages(altData, lowScorePackages[alternative.Dependency.Name]) == worse {
continue
}

Expand All @@ -333,9 +334,11 @@ func (sph *summaryPrHandler) generateSummary() (string, error) {
Ecosystem: altData.PackageType,
PackageName: altData.PackageName,
TrustyURL: altData.TrustyURL,
Score: *altData.Score,
},
}
if altData.Score != nil {
altPackageData.templatePackageData.Score = *altData.Score
}

dep := lowScorePackages[alternative.Dependency.Name]
dep.Alternatives = append(dep.Alternatives, altPackageData)
Expand All @@ -346,6 +349,23 @@ func (sph *summaryPrHandler) generateSummary() (string, error) {
return sph.compileTemplate(malicious, lowScorePackages)
}

type packageComparison int

const (
better packageComparison = iota
worse
)
Comment on lines +354 to +357
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, comparePackages arguments are of different types, so it would not be possible to use that as argument to slices.SortFunc.

I'm inclined to keep this implementation because

  • it's unclear whether smaller-is-better or greater is
  • the score is deprecated, so this comparison doesn't really make sense anymore
  • we're moving pr_trusty_check rule to Data Sources and Rego evaluation type


// comparePackages compares two packages to determine whether the
// first argument is better or worse than the second one. It does so
// by checking Trusty scores.
func comparePackages(alt alternative, examined templatePackage) packageComparison {
if alt.Score != nil && *alt.Score != 0 && *alt.Score <= examined.Score {
return worse
}
return better
}

// buildProvenanceStruct builds the provenance data structure for the PR template
func buildProvenanceStruct(r *trustyReport) *templateProvenance {
if r == nil || r.Provenance == nil {
Expand Down Expand Up @@ -428,8 +448,12 @@ func newSummaryPrHandler(
}, nil
}

func preprocessDetails(s string) string {
scanner := bufio.NewScanner(strings.NewReader(s))
func preprocessDetails(s *string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because its argument maliciousInfo.Details is now a pointer to string.
I can change this, but I'd have to move the nil check to the caller.

if s == nil {
return ""
}

scanner := bufio.NewScanner(strings.NewReader(*s))
text := ""
for scanner.Scan() {
if strings.HasPrefix(scanner.Text(), "#") {
Expand Down
Loading