✨ Optimize SAST check (#2191)

* Optimize SAST

* Address PR feedback

* split checkruns into separate graphql query

* Enable SAST check in the releasetest cron worker

Co-authored-by: Azeem Shaikh <azeemshaikh38@gmail.com>

Author: spencerschrock

checks/sast.go

@@ -123,19 +123,17 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 	return checker.CreateRuntimeErrorResult(CheckSAST, sce.WithMessage(sce.ErrScorecardInternal, "contact team"))
 }
 
-//nolint
 func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 	commits, err := c.RepoClient.ListCommits()
 	if err != nil {
-		//nolint
 		return checker.InconclusiveResultScore,
 			sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
 	}
 
 	totalMerged := 0
 	totalTested := 0
-	for _, commit := range commits {
-		pr := commit.AssociatedMergeRequest
+	for i := range commits {
+		pr := commits[i].AssociatedMergeRequest
 		// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
 		// but the PR was created in the original repo.
 		if pr.MergedAt.IsZero() {
@@ -187,7 +185,6 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 	return checker.CreateProportionalScore(totalTested, totalMerged), nil
 }
 
-//nolint
 func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) {
 	searchRequest := clients.SearchRequest{
 		Query: "github/codeql-action/analyze",
@@ -228,7 +225,6 @@ type sonarConfig struct {
 	file checker.File
 }
 
-//nolint
 func sonarEnabled(c *checker.CheckRequest) (int, error) {
 	var config []sonarConfig
 	err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{

clients/githubrepo/client.go

@@ -192,7 +192,15 @@ func (client *Client) ListSuccessfulWorkflowRuns(filename string) ([]clients.Wor
 
 // ListCheckRunsForRef implements RepoClient.ListCheckRunsForRef.
 func (client *Client) ListCheckRunsForRef(ref string) ([]clients.CheckRun, error) {
-	return client.checkruns.listCheckRunsForRef(ref)
+	cachedCrs, err := client.graphClient.listCheckRunsForRef(ref)
+	if errors.Is(err, errNotCached) {
+		crs, err := client.checkruns.listCheckRunsForRef(ref)
+		if err == nil {
+			client.graphClient.cacheCheckRunsForRef(ref, crs)
+		}
+		return crs, err
+	}
+	return cachedCrs, err
 }
 
 // ListStatuses implements RepoClient.ListStatuses.

clients/githubrepo/graphql.go

@@ -16,6 +16,7 @@ package githubrepo
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"sync"
@@ -25,18 +26,22 @@ import (
 
 	"github.com/ossf/scorecard/v4/clients"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/log"
 )
 
 const (
 	pullRequestsToAnalyze  = 1
+	checksToAnalyze        = 30
 	issuesToAnalyze        = 30
 	issueCommentsToAnalyze = 30
 	reviewsToAnalyze       = 30
 	labelsToAnalyze        = 30
 	commitsToAnalyze       = 30
 )
 
-//nolint: govet
+var errNotCached = errors.New("result not cached")
+
+//nolint:govet
 type graphqlData struct {
 	Repository struct {
 		IsArchived githubv4.Boolean
@@ -121,34 +126,77 @@ type graphqlData struct {
 	}
 }
 
+//nolint:govet
+type checkRunsGraphqlData struct {
+	Repository struct {
+		Object struct {
+			Commit struct {
+				History struct {
+					Nodes []struct {
+						AssociatedPullRequests struct {
+							Nodes []struct {
+								HeadRefOid githubv4.String
+								Commits    struct {
+									Nodes []struct {
+										Commit struct {
+											CheckSuites struct {
+												Nodes []struct {
+													App struct {
+														Slug githubv4.String
+													}
+													Conclusion githubv4.CheckConclusionState
+													Status     githubv4.CheckStatusState
+												}
+											} `graphql:"checkSuites(first: $checksToAnalyze)"`
+										}
+									}
+								} `graphql:"commits(last:1)"`
+							}
+						} `graphql:"associatedPullRequests(first: $pullRequestsToAnalyze)"`
+					}
+				} `graphql:"history(first: $commitsToAnalyze)"`
+			} `graphql:"... on Commit"`
+		} `graphql:"object(expression: $commitExpression)"`
+	} `graphql:"repository(owner: $owner, name: $name)"`
+	RateLimit struct {
+		Cost *int
+	}
+}
+
+type checkRunCache = map[string][]clients.CheckRun
+
 type graphqlHandler struct {
-	client   *githubv4.Client
-	data     *graphqlData
-	once     *sync.Once
-	ctx      context.Context
-	errSetup error
-	repourl  *repoURL
-	commits  []clients.Commit
-	issues   []clients.Issue
-	archived bool
+	checkRuns          checkRunCache
+	client             *githubv4.Client
+	data               *graphqlData
+	setupOnce          *sync.Once
+	checkData          *checkRunsGraphqlData
+	setupCheckRunsOnce *sync.Once
+	errSetupCheckRuns  error
+	logger             *log.Logger
+	ctx                context.Context
+	errSetup           error
+	repourl            *repoURL
+	commits            []clients.Commit
+	issues             []clients.Issue
+	archived           bool
 }
 
 func (handler *graphqlHandler) init(ctx context.Context, repourl *repoURL) {
 	handler.ctx = ctx
 	handler.repourl = repourl
 	handler.data = new(graphqlData)
 	handler.errSetup = nil
-	handler.once = new(sync.Once)
+	handler.setupOnce = new(sync.Once)
+	handler.checkData = new(checkRunsGraphqlData)
+	handler.setupCheckRunsOnce = new(sync.Once)
+	handler.checkRuns = checkRunCache{}
+	handler.logger = log.NewLogger(log.DefaultLevel)
 }
 
 func (handler *graphqlHandler) setup() error {
-	handler.once.Do(func() {
-		commitExpression := handler.repourl.commitSHA
-		if strings.EqualFold(handler.repourl.commitSHA, clients.HeadSHA) {
-			// TODO(#575): Confirm that this works as expected.
-			commitExpression = fmt.Sprintf("heads/%s", handler.repourl.defaultBranch)
-		}
-
+	handler.setupOnce.Do(func() {
+		commitExpression := handler.commitExpression()
 		vars := map[string]interface{}{
 			"owner":                  githubv4.String(handler.repourl.owner),
 			"name":                   githubv4.String(handler.repourl.repo),
@@ -174,13 +222,54 @@ func (handler *graphqlHandler) setup() error {
 	return handler.errSetup
 }
 
+func (handler *graphqlHandler) setupCheckRuns() error {
+	handler.setupCheckRunsOnce.Do(func() {
+		commitExpression := handler.commitExpression()
+		vars := map[string]interface{}{
+			"owner":                 githubv4.String(handler.repourl.owner),
+			"name":                  githubv4.String(handler.repourl.repo),
+			"pullRequestsToAnalyze": githubv4.Int(pullRequestsToAnalyze),
+			"commitsToAnalyze":      githubv4.Int(commitsToAnalyze),
+			"commitExpression":      githubv4.String(commitExpression),
+			"checksToAnalyze":       githubv4.Int(checksToAnalyze),
+		}
+		if err := handler.client.Query(handler.ctx, handler.checkData, vars); err != nil {
+			// quit early without setting crsErrSetup for "Resource not accessible by integration" error
+			// for whatever reason, this check doesn't work with a GITHUB_TOKEN, only a PAT
+			if strings.Contains(err.Error(), "Resource not accessible by integration") {
+				return
+			}
+			handler.errSetupCheckRuns = err
+			return
+		}
+		handler.checkRuns = parseCheckRuns(handler.checkData)
+	})
+	return handler.errSetupCheckRuns
+}
+
 func (handler *graphqlHandler) getCommits() ([]clients.Commit, error) {
 	if err := handler.setup(); err != nil {
 		return nil, fmt.Errorf("error during graphqlHandler.setup: %w", err)
 	}
 	return handler.commits, nil
 }
 
+func (handler *graphqlHandler) cacheCheckRunsForRef(ref string, crs []clients.CheckRun) {
+	handler.checkRuns[ref] = crs
+}
+
+func (handler *graphqlHandler) listCheckRunsForRef(ref string) ([]clients.CheckRun, error) {
+	if err := handler.setupCheckRuns(); err != nil {
+		return nil, fmt.Errorf("error during graphqlHandler.setupCheckRuns: %w", err)
+	}
+	if crs, ok := handler.checkRuns[ref]; ok {
+		return crs, nil
+	}
+	msg := fmt.Sprintf("listCheckRunsForRef cache miss: %s/%s:%s", handler.repourl.owner, handler.repourl.repo, ref)
+	handler.logger.Info(msg)
+	return nil, errNotCached
+}
+
 func (handler *graphqlHandler) getIssues() ([]clients.Issue, error) {
 	if !strings.EqualFold(handler.repourl.commitSHA, clients.HeadSHA) {
 		return nil, fmt.Errorf("%w: ListIssues only supported for HEAD queries", clients.ErrUnsupportedFeature)
@@ -201,7 +290,39 @@ func (handler *graphqlHandler) isArchived() (bool, error) {
 	return handler.archived, nil
 }
 
-//nolint
+func (handler *graphqlHandler) commitExpression() string {
+	if strings.EqualFold(handler.repourl.commitSHA, clients.HeadSHA) {
+		// TODO(#575): Confirm that this works as expected.
+		return fmt.Sprintf("heads/%s", handler.repourl.defaultBranch)
+	}
+	return handler.repourl.commitSHA
+}
+
+func parseCheckRuns(data *checkRunsGraphqlData) checkRunCache {
+	checkCache := checkRunCache{}
+	for _, commit := range data.Repository.Object.Commit.History.Nodes {
+		for _, pr := range commit.AssociatedPullRequests.Nodes {
+			var crs []clients.CheckRun
+			for _, c := range pr.Commits.Nodes {
+				for _, checkRun := range c.Commit.CheckSuites.Nodes {
+					crs = append(crs, clients.CheckRun{
+						// the REST API returns lowercase. the graphQL API returns upper
+						Status:     strings.ToLower(string(checkRun.Status)),
+						Conclusion: strings.ToLower(string(checkRun.Conclusion)),
+						App: clients.CheckRunApp{
+							Slug: string(checkRun.App.Slug),
+						},
+					})
+				}
+			}
+			headRef := string(pr.HeadRefOid)
+			checkCache[headRef] = crs
+		}
+	}
+	return checkCache
+}
+
+//nolint:all
 func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commit, error) {
 	ret := make([]clients.Commit, 0)
 	for _, commit := range data.Repository.Object.Commit.History.Nodes {

clients/githubrepo/graphql_e2e_test.go

@@ -57,5 +57,17 @@ var _ = Describe("E2E TEST: githubrepo.graphqlHandler", func() {
 			Expect(graphqlhandler.data.RateLimit.Cost).ShouldNot(BeNil())
 			Expect(*graphqlhandler.data.RateLimit.Cost).Should(BeNumerically("<=", 1))
 		})
+		It("Should not have increased for check run query", func() {
+			repourl := &repoURL{
+				owner:     "ossf",
+				repo:      "scorecard",
+				commitSHA: clients.HeadSHA,
+			}
+			graphqlhandler.init(context.Background(), repourl)
+			Expect(graphqlhandler.setupCheckRuns()).Should(BeNil())
+			Expect(graphqlhandler.checkData).ShouldNot(BeNil())
+			Expect(graphqlhandler.checkData.RateLimit.Cost).ShouldNot(BeNil())
+			Expect(*graphqlhandler.checkData.RateLimit.Cost).Should(BeNumerically("<=", 1))
+		})
 	})
 })

clients/pull_request.go

@@ -19,7 +19,8 @@ import (
 )
 
 // PullRequest struct represents a PR as returned by RepoClient.
-//nolint: govet
+//
+//nolint:govet
 type PullRequest struct {
 	Number   int
 	MergedAt time.Time

cron/k8s/worker.release.yaml

@@ -44,6 +44,8 @@ spec:
           value: "10.4.4.210:80"
         - name: "SCORECARD_API_RESULTS_BUCKET_URL"
           value: "gs://ossf-scorecard-cron-releasetest-results"
+        - name: "SCORECARD_BLACKLISTED_CHECKS"
+          value: "CI-Tests,Contributors"
         resources:
           requests:
             memory: 5Gi