Feat-Implement httpcache middleware for GitHub API

.gitignore

@@ -23,3 +23,5 @@ results.json
 
 # tools 
 bin
+
+cache

Makefile

@@ -63,6 +63,9 @@ ci-e2e: build check-env
 	mkdir -p bin
 	./scorecard --repo=https://github.com/ossf/scorecard --format json > ./bin/results.json
 	ginkgo -p  -v -cover  ./...
+	mkdir -p cache
+	USE_DISK_CACHE=1 DISK_CACHE_PATH="./cache" ./scorecard --repo=https://github.com/ossf/scorecard --format json > ./bin/results.json
+	ginkgo -p  -v -cover  ./...
 
 
 # Verification targets

README.md

@@ -1,6 +1,5 @@
 # Security Scorecards
 
-
 ![build](https://github.com/ossf/scorecard/workflows/build/badge.svg?branch=main)
 ![CodeQL](https://github.com/ossf/scorecard/workflows/CodeQL/badge.svg?branch=main)
 [![codecov](https://codecov.io/gh/ossf/scorecard/branch/main/graph/badge.svg?token=PMJ6NAN9J3)](https://codecov.io/gh/ossf/scorecard)
@@ -153,6 +152,20 @@ Signed-Releases: Fail 0
 Signed-Tags: Fail 10
 ```
 
+### Caching
+
+Scorecard uses `httpcache` with <https://docs.github.com/en/rest/overview/resources-in-the-rest-api#conditional-requests> for caching httpresponse. The default cache is in-memory.
+
+#### Disk Cache
+
+Scorecard results can be cached into a disk for increasing throughput for subsequent runs.
+
+To use disk cache two env variables have to be set `USE_DISK_CACHE=true` and `DISK_CACHE_PATH=./cache`.
+
+There is no TTL on cache.
+
+Some details on caching <https://github.com/ossf/scorecard/issues/80#issuecomment-782723182>
+
 ### Authentication
 
 Before running Scorecard, you need to

go.mod

@@ -7,10 +7,11 @@ require (
 	github.com/golang/protobuf v1.4.3 // indirect
 	github.com/google/go-github/v32 v32.1.0
 	github.com/kr/text v0.2.0 // indirect
+	github.com/naveensrinivasan/httpcache v1.2.1
 	github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
 	github.com/onsi/ginkgo v1.15.0
 	github.com/onsi/gomega v1.10.5
-	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pkg/errors v0.9.1
 	github.com/shurcooL/githubv4 v0.0.0-20200928013246-d292edc3691b
 	github.com/shurcooL/graphql v0.0.0-20200928012149-18c5c3165e3a // indirect
 	github.com/spf13/cobra v1.1.3

pkg/scorecard.go

@@ -78,7 +78,8 @@ func (r *RepoURL) Set(s string) error {
 	}
 }
 
-func RunScorecards(ctx context.Context, logger *zap.SugaredLogger, repo RepoURL, checksToRun []checker.NamedCheck) <-chan (Result) {
+func RunScorecards(ctx context.Context, logger *zap.SugaredLogger,
+	repo RepoURL, checksToRun []checker.NamedCheck) <-chan Result {
 	// Use our custom roundtripper
 	rt := roundtripper.NewTransport(ctx, logger)
 

roundtripper/roundtripper.go

@@ -15,29 +15,30 @@
 package roundtripper
 
 import (
-	"bytes"
 	"context"
-	"io/ioutil"
 	"log"
 	"net/http"
-	"net/url"
 	"os"
 	"strconv"
 	"strings"
-	"sync"
 	"sync/atomic"
 	"time"
 
 	"github.com/bradleyfalzon/ghinstallation"
+	cache "github.com/naveensrinivasan/httpcache"
+	"github.com/naveensrinivasan/httpcache/diskcache"
+	"github.com/pkg/errors"
 	"go.uber.org/zap"
 	"golang.org/x/oauth2"
 )
 
 const (
-	GITHUB_AUTH_TOKEN          = "GITHUB_AUTH_TOKEN" // #nosec G101
-	GITHUB_APP_KEY_PATH        = "GITHUB_APP_KEY_PATH"
-	GITHUB_APP_ID              = "GITHUB_APP_ID"
-	GITHUB_APP_INSTALLATION_ID = "GITHUB_APP_INSTALLATION_ID"
+	GithubAuthToken         = "GITHUB_AUTH_TOKEN" // #nosec G101
+	GithubAppKeyPath        = "GITHUB_APP_KEY_PATH"
+	GithubAppID             = "GITHUB_APP_ID"
+	GithubAppInstallationID = "GITHUB_APP_INSTALLATION_ID"
+	UseDiskCache            = "USE_DISK_CACHE"
+	DiskCachePath           = "DISK_CACHE_PATH"
 )
 
 // RateLimitRoundTripper is a rate-limit aware http.Transport for Github.
@@ -59,26 +60,25 @@ func (r *RoundRobinTokenSource) Token() (*oauth2.Token, error) {
 	}, nil
 }
 
-// NewTransport returns a configured http.Transport for use with GitHub
+// NewTransport returns a configured http.Transport for use with GitHub.
 func NewTransport(ctx context.Context, logger *zap.SugaredLogger) http.RoundTripper {
-
 	// Start with oauth
 	transport := http.DefaultTransport
-	if token := os.Getenv(GITHUB_AUTH_TOKEN); token != "" {
+	if token := os.Getenv(GithubAuthToken); token != "" {
 		ts := &RoundRobinTokenSource{
 			AccessTokens: strings.Split(token, ","),
 		}
 		transport = oauth2.NewClient(ctx, ts).Transport
-	} else if key_path := os.Getenv(GITHUB_APP_KEY_PATH); key_path != "" { // Also try a GITHUB_APP
-		app_id, err := strconv.Atoi(os.Getenv(GITHUB_APP_ID))
+	} else if keyPath := os.Getenv(GithubAppKeyPath); keyPath != "" { // Also try a GITHUB_APP
+		appID, err := strconv.Atoi(os.Getenv(GithubAppID))
 		if err != nil {
 			log.Panic(err)
 		}
-		installation_id, err := strconv.Atoi(os.Getenv(GITHUB_APP_INSTALLATION_ID))
+		installationID, err := strconv.Atoi(os.Getenv(GithubAppInstallationID))
 		if err != nil {
 			log.Panic(err)
 		}
-		transport, err = ghinstallation.NewKeyFromFile(transport, int64(app_id), int64(installation_id), key_path)
+		transport, err = ghinstallation.NewKeyFromFile(transport, int64(appID), int64(installationID), keyPath)
 		if err != nil {
 			log.Panic(err)
 		}
@@ -90,22 +90,37 @@ func NewTransport(ctx context.Context, logger *zap.SugaredLogger) http.RoundTrip
 		InnerTransport: transport,
 	}
 
-	// Wrap that with the response cacher
-	cache := &CachingRoundTripper{
-		Logger:         logger,
-		innerTransport: rateLimit,
-		respCache:      map[url.URL]*http.Response{},
-		bodyCache:      map[url.URL][]byte{},
+	// uses the disk cache
+	if cachePath, useDisk := shouldUseDiskCache(); useDisk {
+		c := cache.NewTransport(diskcache.New(cachePath))
+		c.Transport = rateLimit
+		return c
 	}
 
-	return cache
+	// uses memory cache
+	c := cache.NewTransport(cache.NewMemoryCache())
+	c.Transport = rateLimit
+	return c
+}
+
+// shouldUseDiskCache checks the env variables USE_DISK_CACHE and DISK_CACHE_PATH to determine if
+// disk should be used for caching.
+func shouldUseDiskCache() (string, bool) {
+	if isDiskCache := os.Getenv(UseDiskCache); isDiskCache != "" {
+		if result, err := strconv.ParseBool(isDiskCache); err == nil && result {
+			if cachePath := os.Getenv(DiskCachePath); cachePath != "" {
+				return cachePath, true
+			}
+		}
+	}
+	return "", false
 }
 
 // Roundtrip handles caching and ratelimiting of responses from GitHub.
 func (gh *RateLimitRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
 	resp, err := gh.InnerTransport.RoundTrip(r)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "error in round trip")
 	}
 
 	rateLimit := resp.Header.Get("X-RateLimit-Remaining")
@@ -128,48 +143,5 @@ func (gh *RateLimitRoundTripper) RoundTrip(r *http.Request) (*http.Response, err
 		gh.Logger.Warnf("Rate limit exceeded. Retrying...")
 		return gh.RoundTrip(r)
 	}
-
-	return resp, err
-}
-
-type CachingRoundTripper struct {
-	innerTransport http.RoundTripper
-	respCache      map[url.URL]*http.Response
-	bodyCache      map[url.URL][]byte
-	mutex          sync.Mutex
-	Logger         *zap.SugaredLogger
-}
-
-func (rt *CachingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
-	// Check the cache
-	rt.mutex.Lock()
-	defer rt.mutex.Unlock()
-	resp, ok := rt.respCache[*r.URL]
-
-	if ok {
-		rt.Logger.Debugf("Cache hit on %s", r.URL.String())
-		resp.Body = ioutil.NopCloser(bytes.NewReader(rt.bodyCache[*r.URL]))
-		return resp, nil
-	}
-
-	// Get the real value
-	resp, err := rt.innerTransport.RoundTrip(r)
-	if err != nil {
-		return nil, err
-	}
-
-	// Add to cache
-	if resp.StatusCode == http.StatusOK {
-		defer resp.Body.Close()
-		body, err := ioutil.ReadAll(resp.Body)
-		if err != nil {
-			return nil, err
-		}
-
-		rt.respCache[*r.URL] = resp
-		rt.bodyCache[*r.URL] = body
-
-		resp.Body = ioutil.NopCloser(bytes.NewReader(body))
-	}
-	return resp, err
+	return resp, nil
 }

roundtripper/roundtripper_test.go

@@ -0,0 +1,69 @@
+// Copyright 2020 Security Scorecard Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package roundtripper
+
+import (
+	"os"
+	"testing"
+)
+
+func thelperHandleError(t *testing.T, e error) {
+	if e != nil {
+		t.Errorf(e.Error())
+	}
+}
+
+func Test_shouldUseDiskCache(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name          string
+		diskCachePath string
+		useDiskCache  bool
+	}{
+		{
+			name:          "Want to use Disk Cache",
+			diskCachePath: "foo",
+			useDiskCache:  true,
+		},
+		{
+			name:          "Want to use Disk Cache",
+			diskCachePath: "",
+			useDiskCache:  false,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			if tt.useDiskCache {
+				if tt.diskCachePath != "" {
+					e := os.Setenv(UseDiskCache, "1")
+					thelperHandleError(t, e)
+					e = os.Setenv(DiskCachePath, tt.diskCachePath)
+					thelperHandleError(t, e)
+				}
+			} else {
+				os.Unsetenv(UseDiskCache)
+			}
+			got, got1 := shouldUseDiskCache()
+			if got != tt.diskCachePath {
+				t.Errorf("shouldUseDiskCache() got = %v, want %v", got, tt.diskCachePath)
+			}
+			if got1 != tt.useDiskCache {
+				t.Errorf("shouldUseDiskCache() got1 = %v, want %v", got1, tt.useDiskCache)
+			}
+		})
+	}
+}