From 3a205d7d24408496f2c02b907cbdb44ae59cd993 Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach Date: Tue, 25 Aug 2026 16:00:42 +0200 Subject: [PATCH] Add retries for the raw result upload from the lurker sidecar When upload fails it can be painful to retry the entire scan just to rerun the file upload. Signed-off-by: Jannik Hollenbach --- .github/workflows/ci.yaml | 2 +- lurker/.gitignore | 5 ++ lurker/Taskfile.yaml | 38 +++++++++ lurker/main.go | 50 ++++++++++-- lurker/main_test.go | 162 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 248 insertions(+), 9 deletions(-) create mode 100644 lurker/Taskfile.yaml create mode 100644 lurker/main_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a5fde4a33f..71dc1b7c38 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -194,7 +194,7 @@ jobs: go vet ./... - name: Test - working-directory: ./operator + working-directory: ./${{ matrix.component }} run: task test - name: Build Container Image diff --git a/lurker/.gitignore b/lurker/.gitignore index 60f115474b..8e26c7fb31 100644 --- a/lurker/.gitignore +++ b/lurker/.gitignore @@ -3,3 +3,8 @@ # SPDX-License-Identifier: Apache-2.0 main +lurker +bin + +# Output of the go coverage tool +*.out diff --git a/lurker/Taskfile.yaml b/lurker/Taskfile.yaml new file mode 100644 index 0000000000..728e89a172 --- /dev/null +++ b/lurker/Taskfile.yaml @@ -0,0 +1,38 @@ +# SPDX-FileCopyrightText: the secureCodeBox authors +# +# SPDX-License-Identifier: Apache-2.0 + +version: "3.48.0" + +tasks: + fmt: + desc: "Run go fmt against code" + dir: '{{ .TASKFILE_DIR }}' + cmds: + - go fmt ./... + + vet: + desc: "Run go vet against code" + dir: '{{ .TASKFILE_DIR }}' + cmds: + - go vet ./... + + test: + desc: "Run all tests" + deps: [fmt, vet] + dir: '{{ .TASKFILE_DIR }}' + cmds: + - go test ./... -coverprofile cover.out + + view-coverage: + desc: "View test coverage in browser" + dir: '{{ .TASKFILE_DIR }}' + cmds: + - go tool cover -html=cover.out + + build: + desc: "Build the lurker binary" + deps: [fmt, vet] + dir: '{{ .TASKFILE_DIR }}' + cmds: + - go build -o bin/lurker main.go diff --git a/lurker/main.go b/lurker/main.go index 2332777a36..43e50e1328 100644 --- a/lurker/main.go +++ b/lurker/main.go @@ -8,6 +8,7 @@ import ( "context" "flag" "fmt" + "io" "log" "net/http" "net/http/httputil" @@ -55,13 +56,44 @@ func main() { log.Printf("Uploading result files.") log.Printf("Uploading %s", filePath) - err = uploadFile(filePath, uploadURL) + err = uploadFileWithRetries(filePath, uploadURL) if err != nil { log.Fatal(err) } log.Printf("Uploaded file successfully") } +// delays waited before each retry of a failed upload. The number of entries +// defines how often the upload is retried after the initial attempt. +var uploadBackoffs = []time.Duration{1 * time.Second, 3 * time.Second, 5 * time.Second, 10 * time.Second, 30 * time.Second} + +// indirection to allow tests to run without actually waiting +var sleep = time.Sleep + +// uploadFileWithRetries uploads the file and retries every failure (transport +// errors as well as non 2xx responses) using the uploadBackoffs schedule. +func uploadFileWithRetries(path, url string) error { + attempts := len(uploadBackoffs) + 1 + + var err error + for attempt := 1; attempt <= attempts; attempt++ { + err = uploadFile(path, url) + if err == nil { + return nil + } + + if attempt == attempts { + break + } + + backoff := uploadBackoffs[attempt-1] + log.Printf("Upload attempt %d of %d failed: %v. Retrying in %s", attempt, attempts, err, backoff) + sleep(backoff) + } + + return fmt.Errorf("lurker failed to upload scan result file after %d attempts: %w", attempts, err) +} + func uploadFile(path, url string) error { file, err := os.Open(path) if err != nil { @@ -81,7 +113,7 @@ func uploadFile(path, url string) error { // Create a new file upload request req, err := http.NewRequest("PUT", url, file) if err != nil { - log.Fatalf("Failed to create request: %v", err) + log.Printf("Failed to create request: %v", err) return err } @@ -109,14 +141,16 @@ func uploadFile(path, url string) error { log.Printf("File upload returned non 2xx status code (%d)", res.StatusCode) // Dump response for debugging purposes - resultBytes, err := httputil.DumpResponse(res, true) - if err != nil { - log.Fatal(fmt.Errorf("failed to dump out failed requests to upload scan report to the s3 bucket: %w", err)) + resultBytes, dumpErr := httputil.DumpResponse(res, true) + if dumpErr != nil { + log.Printf("failed to dump out failed requests to upload scan report to the s3 bucket: %v", dumpErr) + // drain the body so that the connection can be reused by the next attempt + io.Copy(io.Discard, res.Body) + } else { + log.Println("Response of Failed Request:") + log.Println(string(resultBytes)) } - log.Println("Response of Failed Request:") - log.Println(string(resultBytes)) - return fmt.Errorf("lurker failed to upload scan result file. File upload returned non 2xx status code (%d)", res.StatusCode) } diff --git a/lurker/main_test.go b/lurker/main_test.go new file mode 100644 index 0000000000..a65620efed --- /dev/null +++ b/lurker/main_test.go @@ -0,0 +1,162 @@ +// SPDX-FileCopyrightText: the secureCodeBox authors +// +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" +) + +const testFileContents = `{"findings": []}` + +// writeResultFile creates a temporary scan result file and returns its path. +func writeResultFile(t *testing.T) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "findings.json") + if err := os.WriteFile(path, []byte(testFileContents), 0o600); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + return path +} + +// stubSleep replaces the backoff sleep with a recorder, so that tests don't +// have to actually wait. Returns a pointer to the recorded delays. +func stubSleep(t *testing.T) *[]time.Duration { + t.Helper() + + original := sleep + delays := []time.Duration{} + sleep = func(d time.Duration) { + delays = append(delays, d) + } + t.Cleanup(func() { sleep = original }) + + return &delays +} + +func TestUploadFileWithRetriesSucceedsOnFirstAttempt(t *testing.T) { + delays := stubSleep(t) + + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + if err := uploadFileWithRetries(writeResultFile(t), server.URL); err != nil { + t.Fatalf("expected upload to succeed, got: %v", err) + } + if requests != 1 { + t.Errorf("expected 1 request, got %d", requests) + } + if len(*delays) != 0 { + t.Errorf("expected no backoff, got %v", *delays) + } +} + +func TestUploadFileWithRetriesRetriesUntilSuccess(t *testing.T) { + delays := stubSleep(t) + + requests := 0 + receivedBodies := []string{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("failed to read request body: %v", err) + } + receivedBodies = append(receivedBodies, string(body)) + + if requests < 3 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + if err := uploadFileWithRetries(writeResultFile(t), server.URL); err != nil { + t.Fatalf("expected upload to succeed after retries, got: %v", err) + } + if requests != 3 { + t.Errorf("expected 3 requests, got %d", requests) + } + + // every attempt, especially the last one, has to send the complete file + for attempt, body := range receivedBodies { + if body != testFileContents { + t.Errorf("attempt %d uploaded %q, expected %q", attempt+1, body, testFileContents) + } + } + + expectedDelays := uploadBackoffs[:2] + if len(*delays) != len(expectedDelays) { + t.Fatalf("expected %d backoffs, got %v", len(expectedDelays), *delays) + } + for i, delay := range *delays { + if delay != expectedDelays[i] { + t.Errorf("backoff %d was %s, expected %s", i+1, delay, expectedDelays[i]) + } + } +} + +func TestUploadFileWithRetriesGivesUpAfterAllAttempts(t *testing.T) { + delays := stubSleep(t) + + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + if err := uploadFileWithRetries(writeResultFile(t), server.URL); err == nil { + t.Fatal("expected upload to fail") + } + + expectedRequests := len(uploadBackoffs) + 1 + if requests != expectedRequests { + t.Errorf("expected %d requests, got %d", expectedRequests, requests) + } + if len(*delays) != len(uploadBackoffs) { + t.Errorf("expected %d backoffs, got %v", len(uploadBackoffs), *delays) + } + for i, delay := range *delays { + if delay != uploadBackoffs[i] { + t.Errorf("backoff %d was %s, expected %s", i+1, delay, uploadBackoffs[i]) + } + } +} + +func TestUploadFileWithRetriesRetriesTransportErrors(t *testing.T) { + delays := stubSleep(t) + + // closing the server up front makes every attempt fail on the transport level + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := server.URL + server.Close() + + if err := uploadFileWithRetries(writeResultFile(t), url); err == nil { + t.Fatal("expected upload to fail") + } + if len(*delays) != len(uploadBackoffs) { + t.Errorf("expected %d backoffs, got %v", len(uploadBackoffs), *delays) + } +} + +func TestUploadFileWithRetriesFailsForMissingFile(t *testing.T) { + stubSleep(t) + + if err := uploadFileWithRetries(filepath.Join(t.TempDir(), "does-not-exist.json"), "http://127.0.0.1:1"); err == nil { + t.Fatal("expected upload of a missing file to fail") + } +}