From 6d5a3261c604e325315e09421fe83da9b19f3dbc Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 24 Feb 2023 13:44:03 -0800 Subject: [PATCH 01/10] Update auth server to use GitHub App. Signed-off-by: Spencer Schrock --- cron/k8s/auth.yaml | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/cron/k8s/auth.yaml b/cron/k8s/auth.yaml index a1da9537e88..10faab04114 100644 --- a/cron/k8s/auth.yaml +++ b/cron/k8s/auth.yaml @@ -44,12 +44,27 @@ spec: - name: github-auth-server image: gcr.io/openssf/scorecard-github-server:stable imagePullPolicy: Always + volumeMounts: + - name: github-app-key + mountPath: "/etc/github/" + readOnly: true env: - - name: GITHUB_AUTH_TOKEN + - name: GITHUB_APP_KEY_PATH + value: /etc/github/app_key + - name: GITHUB_APP_ID valueFrom: secretKeyRef: name: github - key: token + key: app_id + - name: GITHUB_APP_INSTALLATION_ID + valueFrom: + secretKeyRef: + name: github + key: installation_id + volumes: + - name: github-app-key + secret: + secretName: github strategy: type: "RollingUpdate" rollingUpdate: From 3949a50f490ffb64573c6c81538abe5cccfd1a11 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 24 Feb 2023 14:06:25 -0800 Subject: [PATCH 02/10] Update release worker to use GitHub App tokens directly, as a workaround for the auth server not supporting it. Signed-off-by: Spencer Schrock --- cron/k8s/worker.release.yaml | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/cron/k8s/worker.release.yaml b/cron/k8s/worker.release.yaml index dc445052a41..bfd6f1341bd 100644 --- a/cron/k8s/worker.release.yaml +++ b/cron/k8s/worker.release.yaml @@ -40,8 +40,18 @@ spec: value: "gcppubsub://projects/openssf/subscriptions/scorecard-batch-worker-releasetest" - name: SCORECARD_METRIC_EXPORTER value: "printer" - - name: GITHUB_AUTH_SERVER - value: "10.4.4.210:80" + - name: GITHUB_APP_KEY_PATH + value: /etc/github/app_key + - name: GITHUB_APP_ID + valueFrom: + secretKeyRef: + name: github + key: app_id + - name: GITHUB_APP_INSTALLATION_ID + valueFrom: + secretKeyRef: + name: github + key: installation_id - name: "SCORECARD_API_RESULTS_BUCKET_URL" value: "gs://ossf-scorecard-cron-releasetest-results" resources: @@ -55,10 +65,16 @@ spec: - name: config-volume mountPath: /etc/scorecard readOnly: true + - name: github-app-key + mountPath: "/etc/github/" + readOnly: true volumes: - name: config-volume configMap: name: scorecard-config + - name: github-app-key + secret: + secretName: github strategy: type: "RollingUpdate" rollingUpdate: From 93c347d16d07c8814709ff8f6cd9511db149312f Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 27 Feb 2023 10:00:03 -0800 Subject: [PATCH 03/10] Add Retry-After logic and stats. Signed-off-by: Spencer Schrock --- clients/githubrepo/roundtripper/rate_limit.go | 24 +++++++++++++++++++ clients/githubrepo/roundtripper/transport.go | 6 +++++ clients/githubrepo/stats/stats.go | 4 +++- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/clients/githubrepo/roundtripper/rate_limit.go b/clients/githubrepo/roundtripper/rate_limit.go index 60878f0c316..d5009ef175f 100644 --- a/clients/githubrepo/roundtripper/rate_limit.go +++ b/clients/githubrepo/roundtripper/rate_limit.go @@ -15,7 +15,9 @@ package roundtripper import ( + "bytes" "fmt" + "io" "net/http" "strconv" "time" @@ -44,6 +46,28 @@ func (gh *rateLimitTransport) RoundTrip(r *http.Request) (*http.Response, error) if err != nil { return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("innerTransport.RoundTrip: %v", err)) } + + if resp.StatusCode == http.StatusForbidden { + data, err := io.ReadAll(resp.Body) + resp.Body.Close() + resp.Body = io.NopCloser(bytes.NewBuffer(data)) + if err != nil || data == nil { + return resp, nil + } + if bytes.Contains(data, []byte("You have exceeded a secondary rate limit")) { + retryValue := resp.Header.Get("Retry-After") + retryAfter, err := strconv.Atoi(retryValue) + if err != nil { + retryAfter = 60 + } + gh.logger.Info(fmt.Sprintf("Secondary rate limit exceeded. Waiting %d to retry...", retryAfter)) + time.Sleep(time.Duration(retryAfter) * time.Second) + gh.logger.Info("Secondary rate limit exceeded. Retrying...") + return gh.RoundTrip(r) + } + gh.logger.Info("Non secondary rate limit 403") + } + rateLimit := resp.Header.Get("X-RateLimit-Remaining") remaining, err := strconv.Atoi(rateLimit) if err != nil { diff --git a/clients/githubrepo/roundtripper/transport.go b/clients/githubrepo/roundtripper/transport.go index a9824edc2fe..74fc15bebb4 100644 --- a/clients/githubrepo/roundtripper/transport.go +++ b/clients/githubrepo/roundtripper/transport.go @@ -64,5 +64,11 @@ func (gt *githubTransport) RoundTrip(r *http.Request) (*http.Response, error) { if err == nil { stats.Record(ctx, githubstats.RemainingTokens.M(int64(remaining))) } + + retryAfter, err := strconv.Atoi(resp.Header.Get("Retry-After")) + if err == nil { + stats.Record(r.Context(), githubstats.RetryAfter.M(int64(retryAfter))) + } + return resp, nil } diff --git a/clients/githubrepo/stats/stats.go b/clients/githubrepo/stats/stats.go index e320c910478..a6d8821a96a 100644 --- a/clients/githubrepo/stats/stats.go +++ b/clients/githubrepo/stats/stats.go @@ -24,7 +24,9 @@ var ( // RemainingTokens measures the remaining number of API tokens. RemainingTokens = stats.Int64("RemainingTokens", "Measures the remaining count of API tokens", stats.UnitDimensionless) - + // RetryAfter measures the retry delay when dealing with secondary rate limits. + RetryAfter = stats.Int64("RetryAfter", + "Measures the retry delay when dealing with secondary rate limits", stats.UnitSeconds) // TokenIndex is the tag key for specifying a unique token. TokenIndex = tag.MustNewKey("tokenIndex") // ResourceType specifies the type of GitHub resource. From 843e6d37ff1dc7fedfb51c6d8b4942b24b773e87 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 1 Mar 2023 13:34:14 -0800 Subject: [PATCH 04/10] Change retry-after logic to support any status code. Disable troublesome checks. Signed-off-by: Spencer Schrock --- clients/githubrepo/roundtripper/rate_limit.go | 26 ++++--------------- cron/k8s/worker.release.yaml | 6 +++-- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/clients/githubrepo/roundtripper/rate_limit.go b/clients/githubrepo/roundtripper/rate_limit.go index d5009ef175f..cb779a20460 100644 --- a/clients/githubrepo/roundtripper/rate_limit.go +++ b/clients/githubrepo/roundtripper/rate_limit.go @@ -15,9 +15,7 @@ package roundtripper import ( - "bytes" "fmt" - "io" "net/http" "strconv" "time" @@ -47,25 +45,11 @@ func (gh *rateLimitTransport) RoundTrip(r *http.Request) (*http.Response, error) return nil, sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("innerTransport.RoundTrip: %v", err)) } - if resp.StatusCode == http.StatusForbidden { - data, err := io.ReadAll(resp.Body) - resp.Body.Close() - resp.Body = io.NopCloser(bytes.NewBuffer(data)) - if err != nil || data == nil { - return resp, nil - } - if bytes.Contains(data, []byte("You have exceeded a secondary rate limit")) { - retryValue := resp.Header.Get("Retry-After") - retryAfter, err := strconv.Atoi(retryValue) - if err != nil { - retryAfter = 60 - } - gh.logger.Info(fmt.Sprintf("Secondary rate limit exceeded. Waiting %d to retry...", retryAfter)) - time.Sleep(time.Duration(retryAfter) * time.Second) - gh.logger.Info("Secondary rate limit exceeded. Retrying...") - return gh.RoundTrip(r) - } - gh.logger.Info("Non secondary rate limit 403") + retryValue := resp.Header.Get("Retry-After") + if retryAfter, err := strconv.Atoi(retryValue); err == nil { // if NO error + gh.logger.Info(fmt.Sprintf("Retry-After header set. Waiting %d to retry...", retryAfter)) + time.Sleep(time.Duration(retryAfter) * time.Second) + gh.logger.Info("Retry-After header set. Retrying...") } rateLimit := resp.Header.Get("X-RateLimit-Remaining") diff --git a/cron/k8s/worker.release.yaml b/cron/k8s/worker.release.yaml index bfd6f1341bd..091503a9a7c 100644 --- a/cron/k8s/worker.release.yaml +++ b/cron/k8s/worker.release.yaml @@ -28,8 +28,8 @@ spec: spec: containers: - name: worker - image: gcr.io/openssf/scorecard-batch-worker:latest - args: ["--ignoreRuntimeErrors=false", "--config=/etc/scorecard/config.yaml"] + image: gcr.io/openssf/scorecard-batch-worker:test + args: ["--ignoreRuntimeErrors=true", "--config=/etc/scorecard/config.yaml"] imagePullPolicy: Always env: - name: SCORECARD_DATA_BUCKET_URL @@ -54,6 +54,8 @@ spec: key: installation_id - name: "SCORECARD_API_RESULTS_BUCKET_URL" value: "gs://ossf-scorecard-cron-releasetest-results" + - name: "SCORECARD_BLACKLISTED_CHECKS" + value: "CI-Tests,Contributors,Dependency-Update-Tool,Fuzzing,SAST" resources: requests: memory: 5Gi From 41202188be11bf054181721c61e45c3cfa8f1efa Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 11:44:39 -0800 Subject: [PATCH 05/10] Use GitHub App Token instead of auth server. Signed-off-by: Spencer Schrock --- cron/k8s/worker.yaml | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/cron/k8s/worker.yaml b/cron/k8s/worker.yaml index 9ffe36809e1..9307d2d8d57 100644 --- a/cron/k8s/worker.yaml +++ b/cron/k8s/worker.yaml @@ -32,8 +32,18 @@ spec: args: ["--ignoreRuntimeErrors=true", "--config=/etc/scorecard/config.yaml"] imagePullPolicy: Always env: - - name: GITHUB_AUTH_SERVER - value: "10.4.4.210:80" + - name: GITHUB_APP_KEY_PATH + value: /etc/github/app_key + - name: GITHUB_APP_ID + valueFrom: + secretKeyRef: + name: github + key: app_id + - name: GITHUB_APP_INSTALLATION_ID + valueFrom: + secretKeyRef: + name: github + key: installation_id resources: requests: memory: 5Gi @@ -45,10 +55,16 @@ spec: - name: config-volume mountPath: /etc/scorecard readOnly: true + - name: github-app-key + mountPath: "/etc/github/" + readOnly: true volumes: - name: config-volume configMap: name: scorecard-config + - name: github-app-key + secret: + secretName: github strategy: type: "RollingUpdate" rollingUpdate: From 742ada2439db625f6f6b4bec83cd6cc8af5a957a Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 11:45:45 -0800 Subject: [PATCH 06/10] Temporarily disable additional chhecks. Signed-off-by: Spencer Schrock --- cron/config/config.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cron/config/config.yaml b/cron/config/config.yaml index 27e7d2643f0..a9d4017f82a 100644 --- a/cron/config/config.yaml +++ b/cron/config/config.yaml @@ -43,7 +43,9 @@ additional-params: api-results-bucket-url: gs://ossf-scorecard-cron-results # TODO: Temporarily remove SAST and CI-Tests which require lot of GitHub API tokens. # TODO(#859): Re-add Contributors after fixing inconsistencies. - blacklisted-checks: CI-Tests,Contributors + # TODO: Dependency-Update-Tool, Fuzzing, and SAST are search heavy + # TODO: Vulnerabilities is resource intensive, wait until the next osv-scanner release after v1.2.0 + blacklisted-checks: CI-Tests,Contributors,Dependency-Update-Tool,Fuzzing,SAST,Vulnerabilities cii-data-bucket-url: gs://ossf-scorecard-cii-data # Raw results. raw-bigquery-table: scorecard-rawdata From 8951f6ae81b583abc6b08737a4d6b5d5e13e2e44 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 11:50:14 -0800 Subject: [PATCH 07/10] Disable github auth server as it doesn't work with the GitHub App Tokens. Signed-off-by: Spencer Schrock --- cron/k8s/auth.yaml | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/cron/k8s/auth.yaml b/cron/k8s/auth.yaml index 10faab04114..dbfa2c0b723 100644 --- a/cron/k8s/auth.yaml +++ b/cron/k8s/auth.yaml @@ -31,7 +31,7 @@ kind: Deployment metadata: name: scorecard-github-server spec: - replicas: 1 + replicas: 0 selector: matchLabels: app.kubernetes.io/name: github-auth-server @@ -44,27 +44,12 @@ spec: - name: github-auth-server image: gcr.io/openssf/scorecard-github-server:stable imagePullPolicy: Always - volumeMounts: - - name: github-app-key - mountPath: "/etc/github/" - readOnly: true env: - - name: GITHUB_APP_KEY_PATH - value: /etc/github/app_key - - name: GITHUB_APP_ID + - name: GITHUB_AUTH_TOKEN valueFrom: secretKeyRef: name: github - key: app_id - - name: GITHUB_APP_INSTALLATION_ID - valueFrom: - secretKeyRef: - name: github - key: installation_id - volumes: - - name: github-app-key - secret: - secretName: github + key: token strategy: type: "RollingUpdate" rollingUpdate: From 08e1737b00b20485bc2785df5fd5ef53c2dd5dfe Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 11:53:39 -0800 Subject: [PATCH 08/10] Re-enable Fuzzing check in the release test. Signed-off-by: Spencer Schrock --- cron/k8s/worker.release.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cron/k8s/worker.release.yaml b/cron/k8s/worker.release.yaml index 091503a9a7c..be78ad1318b 100644 --- a/cron/k8s/worker.release.yaml +++ b/cron/k8s/worker.release.yaml @@ -28,7 +28,7 @@ spec: spec: containers: - name: worker - image: gcr.io/openssf/scorecard-batch-worker:test + image: gcr.io/openssf/scorecard-batch-worker:latest args: ["--ignoreRuntimeErrors=true", "--config=/etc/scorecard/config.yaml"] imagePullPolicy: Always env: @@ -55,7 +55,7 @@ spec: - name: "SCORECARD_API_RESULTS_BUCKET_URL" value: "gs://ossf-scorecard-cron-releasetest-results" - name: "SCORECARD_BLACKLISTED_CHECKS" - value: "CI-Tests,Contributors,Dependency-Update-Tool,Fuzzing,SAST" + value: "CI-Tests,Contributors,Dependency-Update-Tool,SAST" resources: requests: memory: 5Gi From 22b52844a8b275ecca29d03ca325b29230cc8a7f Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 11:55:25 -0800 Subject: [PATCH 09/10] Fix unit test for new check change. Signed-off-by: Spencer Schrock --- cron/config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cron/config/config_test.go b/cron/config/config_test.go index 82e607f3123..d11290cd149 100644 --- a/cron/config/config_test.go +++ b/cron/config/config_test.go @@ -34,7 +34,7 @@ const ( prodCompletionThreshold = 0.99 prodWebhookURL = "" prodCIIDataBucket = "gs://ossf-scorecard-cii-data" - prodBlacklistedChecks = "CI-Tests,Contributors" + prodBlacklistedChecks = "CI-Tests,Contributors,Dependency-Update-Tool,Fuzzing,SAST,Vulnerabilities" prodShardSize int = 10 prodMetricExporter string = "stackdriver" prodMetricStackdriverPrefix string = "scorecard-cron" From 69771cf7cc00c4407e3aafe64618d297b06e0c38 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Mar 2023 16:14:26 -0800 Subject: [PATCH 10/10] Move opencensus stat to the ratelimit roundtripped. Signed-off-by: Spencer Schrock --- clients/githubrepo/roundtripper/rate_limit.go | 10 ++++++++-- clients/githubrepo/roundtripper/transport.go | 5 ----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/clients/githubrepo/roundtripper/rate_limit.go b/clients/githubrepo/roundtripper/rate_limit.go index cb779a20460..9730616d1f1 100644 --- a/clients/githubrepo/roundtripper/rate_limit.go +++ b/clients/githubrepo/roundtripper/rate_limit.go @@ -20,6 +20,9 @@ import ( "strconv" "time" + "go.opencensus.io/stats" + + githubstats "github.com/ossf/scorecard/v4/clients/githubrepo/stats" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/log" ) @@ -47,9 +50,12 @@ func (gh *rateLimitTransport) RoundTrip(r *http.Request) (*http.Response, error) retryValue := resp.Header.Get("Retry-After") if retryAfter, err := strconv.Atoi(retryValue); err == nil { // if NO error - gh.logger.Info(fmt.Sprintf("Retry-After header set. Waiting %d to retry...", retryAfter)) - time.Sleep(time.Duration(retryAfter) * time.Second) + stats.Record(r.Context(), githubstats.RetryAfter.M(int64(retryAfter))) + duration := time.Duration(retryAfter) * time.Second + gh.logger.Info(fmt.Sprintf("Retry-After header set. Waiting %s to retry...", duration)) + time.Sleep(duration) gh.logger.Info("Retry-After header set. Retrying...") + return gh.RoundTrip(r) } rateLimit := resp.Header.Get("X-RateLimit-Remaining") diff --git a/clients/githubrepo/roundtripper/transport.go b/clients/githubrepo/roundtripper/transport.go index 74fc15bebb4..fa606979459 100644 --- a/clients/githubrepo/roundtripper/transport.go +++ b/clients/githubrepo/roundtripper/transport.go @@ -65,10 +65,5 @@ func (gt *githubTransport) RoundTrip(r *http.Request) (*http.Response, error) { stats.Record(ctx, githubstats.RemainingTokens.M(int64(remaining))) } - retryAfter, err := strconv.Atoi(resp.Header.Get("Retry-After")) - if err == nil { - stats.Record(r.Context(), githubstats.RetryAfter.M(int64(retryAfter))) - } - return resp, nil }