From 9bcb12afbd3c27e988122f7385fc7e6efa6e7d05 Mon Sep 17 00:00:00 2001 From: "H.E. Pennypacker" <115990865+pennypacker-he@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:05:16 +0000 Subject: [PATCH] [patch] Harden PPB retry and image delivery --- .github/workflows/goreleaser.yml | 16 +++ .github/workflows/lint-test-build.yml | 37 ++++--- Dockerfile | 4 +- ci/workflow_contract_test.go | 84 ++++++++++++++ main.go | 5 +- main_test.go | 43 +++++++- pkg/proxy/proxy_test.go | 153 ++++++++++++++++++++++++++ pkg/proxy/tls.go | 40 ++++++- 8 files changed, 357 insertions(+), 25 deletions(-) create mode 100644 ci/workflow_contract_test.go diff --git a/.github/workflows/goreleaser.yml b/.github/workflows/goreleaser.yml index c90114e..f1f8a51 100644 --- a/.github/workflows/goreleaser.yml +++ b/.github/workflows/goreleaser.yml @@ -53,3 +53,19 @@ jobs: args: release --clean env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Keep the release run open until both registries have published the tag. + # This covers the normal release dispatch and a manually pushed tag without + # racing a second tag-triggered image workflow. + publish-images: + if: github.ref_type == 'tag' + needs: [test, goreleaser] + permissions: + contents: read + id-token: write + packages: write + uses: ./.github/workflows/lint-test-build.yml + secrets: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + GCLOUD_OIDC_POOL: ${{ secrets.GCLOUD_OIDC_POOL }} + GSA: ${{ secrets.GSA }} diff --git a/.github/workflows/lint-test-build.yml b/.github/workflows/lint-test-build.yml index c6747dd..c239647 100644 --- a/.github/workflows/lint-test-build.yml +++ b/.github/workflows/lint-test-build.yml @@ -1,11 +1,17 @@ name: build-push on: + workflow_call: + secrets: + CODECOV_TOKEN: + required: false + GCLOUD_OIDC_POOL: + required: false + GSA: + required: false pull_request: push: branches: - main - tags: - - "*" workflow_dispatch: permissions: @@ -48,21 +54,24 @@ jobs: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - publish-ghcr: + # PPB is public in GHCR and also runs on Cloud Run. The shared workflow + # builds and scans each native image once, then publishes the same digest to + # both registries. + publish: if: github.event_name != 'pull_request' needs: [lint-test] - uses: libops/.github/.github/workflows/build-push.yaml@3bdb895c3fd05490b0c6a1341e630ac554842468 # main - permissions: - contents: read - packages: write - - publish-gar: - if: github.event_name != 'pull_request' - needs: [lint-test] - uses: libops/.github/.github/workflows/build-push.yaml@3bdb895c3fd05490b0c6a1341e630ac554842468 # main + uses: libops/.github/.github/workflows/build-push.yaml@a86300fb8020d0f7141bb9f833d89b5dbd7aa4d7 # guarded-signed-image-publisher with: - docker-registry: us-docker.pkg.dev/libops-images/public + ref: ${{ github.sha }} + expected-main-sha: ${{ github.ref == 'refs/heads/main' && github.sha || '' }} + additional-gar-registry: us-docker.pkg.dev/libops-images/public + scan: true + sign: true + certificate-identity: https://github.com/libops/.github/.github/workflows/build-push.yaml@a86300fb8020d0f7141bb9f833d89b5dbd7aa4d7 permissions: contents: read + packages: write id-token: write - secrets: inherit + secrets: + GCLOUD_OIDC_POOL: ${{ secrets.GCLOUD_OIDC_POOL }} + GSA: ${{ secrets.GSA }} diff --git a/Dockerfile b/Dockerfile index 5ce92f8..47ca0c8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM ghcr.io/libops/base:main@sha256:389706a359c6ba0a4ffb9c3d21c0d909a1c2e5e0c4ab79cb5779129267207049 AS builder +FROM ghcr.io/libops/go:1.26.5@sha256:ea764e85e42a243217c621891123b3fda9374674c29d59785414fc6b15815b3d AS builder SHELL ["/bin/ash", "-o", "pipefail", "-ex", "-c"] @@ -14,7 +14,7 @@ COPY pkg ./pkg RUN --mount=type=cache,target=/root/.cache/go-build \ CGO_ENABLED=0 go build -ldflags="-s -w" -o /app/binary . -FROM ghcr.io/libops/base:main@sha256:389706a359c6ba0a4ffb9c3d21c0d909a1c2e5e0c4ab79cb5779129267207049 +FROM ghcr.io/libops/base:3.2.2.0@sha256:0c6bab5444d4758bb977a42c1e5199cba733c7776e1aa006f8021108a785c547 COPY --from=builder /app/binary /app/binary diff --git a/ci/workflow_contract_test.go b/ci/workflow_contract_test.go new file mode 100644 index 0000000..98a0a88 --- /dev/null +++ b/ci/workflow_contract_test.go @@ -0,0 +1,84 @@ +package ci + +import ( + "os" + "path/filepath" + "regexp" + "runtime" + "strings" + "testing" +) + +const sharedPublisherSHA = "a86300fb8020d0f7141bb9f833d89b5dbd7aa4d7" + +func repositoryRoot(t *testing.T) string { + t.Helper() + _, current, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("resolve test source path") + } + return filepath.Dir(filepath.Dir(current)) +} + +func readRepositoryFile(t *testing.T, path ...string) string { + t.Helper() + content, err := os.ReadFile(filepath.Join(append([]string{repositoryRoot(t)}, path...)...)) + if err != nil { + t.Fatalf("read %s: %v", filepath.Join(path...), err) + } + return string(content) +} + +func TestImagePublicationUsesGuardedSharedContract(t *testing.T) { + workflow := readRepositoryFile(t, ".github", "workflows", "lint-test-build.yml") + for _, required := range []string{ + "libops/.github/.github/workflows/build-push.yaml@" + sharedPublisherSHA, + "additional-gar-registry: us-docker.pkg.dev/libops-images/public", + "expected-main-sha:", + "scan: true", + "sign: true", + "certificate-identity: https://github.com/libops/.github/.github/workflows/build-push.yaml@" + sharedPublisherSHA, + "GCLOUD_OIDC_POOL: ${{ secrets.GCLOUD_OIDC_POOL }}", + "GSA: ${{ secrets.GSA }}", + } { + if !strings.Contains(workflow, required) { + t.Errorf("image workflow must contain %q", required) + } + } + if strings.Contains(workflow, "secrets: inherit") { + t.Fatal("image workflow must map only its required registry secrets") + } +} + +func TestReleaseImageCallMapsOnlyRegistrySecrets(t *testing.T) { + workflow := readRepositoryFile(t, ".github", "workflows", "goreleaser.yml") + if strings.Contains(workflow, "secrets: inherit") { + t.Fatal("release workflow must not pass every repository secret") + } + for _, required := range []string{ + "if: github.ref_type == 'tag'", + "uses: ./.github/workflows/lint-test-build.yml", + "CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}", + "GCLOUD_OIDC_POOL: ${{ secrets.GCLOUD_OIDC_POOL }}", + "GSA: ${{ secrets.GSA }}", + } { + if !strings.Contains(workflow, required) { + t.Errorf("release workflow must contain %q", required) + } + } +} + +func TestDockerfilePinsPublishedBuildkitImages(t *testing.T) { + dockerfile := readRepositoryFile(t, "Dockerfile") + for _, pattern := range []string{ + `FROM ghcr\.io/libops/go:[^\s]+@sha256:[0-9a-f]{64} AS builder`, + `FROM ghcr\.io/libops/base:[0-9][^\s]*@sha256:[0-9a-f]{64}`, + } { + if !regexp.MustCompile(pattern).MatchString(dockerfile) { + t.Errorf("Dockerfile must match %q", pattern) + } + } + if strings.Contains(dockerfile, "ghcr.io/libops/base:main") { + t.Fatal("runtime base must use a released version tag") + } +} diff --git a/main.go b/main.go index 342677d..0cbae3f 100644 --- a/main.go +++ b/main.go @@ -148,10 +148,13 @@ func newHandler(c *config.Config, backend http.Handler) http.Handler { // requests can then be cancelled cleanly during disconnect or shutdown. powerCtx, powerCancel := context.WithTimeout(r.Context(), time.Duration(c.PowerOnTimeout)*time.Second) err = c.Machine.PowerOnWithCooldown(powerCtx, c.PowerOnCooldown) + powerTimedOut := powerCtx.Err() == context.DeadlineExceeded && r.Context().Err() == nil powerCancel() if err != nil { slog.Error("Power-on attempt failed", "err", err) - w.Header().Set("Retry-After", "5") + if powerTimedOut { + w.Header().Set("Retry-After", "5") + } http.Error(w, "Backend not available", http.StatusServiceUnavailable) return } diff --git a/main_test.go b/main_test.go index 48dffd5..308caff 100644 --- a/main_test.go +++ b/main_test.go @@ -16,7 +16,7 @@ import ( "github.com/libops/ppb/pkg/machine" ) -func TestHandlerPowerFailureReturnsRetryableUnavailable(t *testing.T) { +func TestHandlerPermanentPowerFailureOmitsRetryAfter(t *testing.T) { t.Parallel() _, allowed, err := net.ParseCIDR("127.0.0.1/32") @@ -38,6 +38,45 @@ func TestHandlerPowerFailureReturnsRetryableUnavailable(t *testing.T) { handler.ServeHTTP(recorder, request) + if recorder.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) + } + if got := recorder.Header().Get("Retry-After"); got != "" { + t.Fatalf("Retry-After = %q, want omitted for a permanent power failure", got) + } + if backendCalled { + t.Fatal("backend handler ran after power-on failure") + } +} + +func TestHandlerPowerTimeoutReturnsRetryableUnavailable(t *testing.T) { + t.Parallel() + + _, allowed, err := net.ParseCIDR("127.0.0.1/32") + if err != nil { + t.Fatal(err) + } + machine := machine.NewGceMachine() + if err := machine.Lock.Acquire(context.Background(), 1); err != nil { + t.Fatal(err) + } + defer machine.Lock.Release(1) + + backendCalled := false + handler := newHandler(&config.Config{ + AllowedIps: []config.IPNet{{IPNet: allowed}}, + PowerOnCooldown: 30, + PowerOnTimeout: 1, + Machine: machine, + }, http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + backendCalled = true + })) + request := httptest.NewRequest(http.MethodPost, "http://example.test/", strings.NewReader("must-not-be-dispatched")) + request.RemoteAddr = "127.0.0.1:12345" + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + if recorder.Code != http.StatusServiceUnavailable { t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) } @@ -45,7 +84,7 @@ func TestHandlerPowerFailureReturnsRetryableUnavailable(t *testing.T) { t.Fatalf("Retry-After = %q, want 5", got) } if backendCalled { - t.Fatal("backend handler ran after power-on failure") + t.Fatal("backend handler ran after power-on timeout") } } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 47c1813..0af2a9c 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -294,6 +294,10 @@ func TestRetryingDialerBoundsConnectionWindow(t *testing.T) { if err == nil { t.Fatal("DialContext() unexpectedly succeeded") } + var exhausted *dialExhaustedError + if !errors.As(err, &exhausted) { + t.Fatalf("DialContext() error = %T %v, want dialExhaustedError", err, err) + } if elapsed := time.Since(started); elapsed > 250*time.Millisecond { t.Fatalf("DialContext() elapsed = %s, want a bounded retry window", elapsed) } @@ -302,6 +306,73 @@ func TestRetryingDialerBoundsConnectionWindow(t *testing.T) { } } +func TestReverseProxyMissingTargetReturnsRetryAfter(t *testing.T) { + proxyHandler := New(&config.Config{ + Scheme: "http", + Port: 8080, + Machine: machine.NewGceMachine(), + }) + request := httptest.NewRequest(http.MethodPost, "http://site.example.test/", strings.NewReader("not-dispatched")) + recorder := httptest.NewRecorder() + + proxyHandler.ServeHTTP(recorder, request) + + if recorder.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) + } + if got := recorder.Header().Get("Retry-After"); got != "5" { + t.Fatalf("Retry-After = %q, want 5", got) + } +} + +func TestReverseProxyDialExhaustionReturnsRetryAfter(t *testing.T) { + proxyHandler := New(&config.Config{ + Scheme: "http", + Port: 8080, + ProxyTarget: &config.ProxyTarget{ + Scheme: "http", + Host: "backend.example.test", + Port: 8080, + }, + ProxyTimeouts: config.ProxyTimeouts{ + DialTimeout: 1, + DialAttemptTimeout: 1, + DialRetryInterval: 1, + KeepAlive: 1, + IdleConnTimeout: 1, + TLSHandshakeTimeout: 1, + ExpectContinueTimeout: 1, + MaxIdleConns: 10, + }, + Machine: machine.NewGceMachine(), + }) + attempts := 0 + proxyHandler.Transport.DialContext = (&retryingDialer{ + totalTimeout: 30 * time.Millisecond, + attemptTimeout: 5 * time.Millisecond, + retryInterval: time.Millisecond, + jitter: func(delay time.Duration) time.Duration { return delay }, + dial: func(context.Context, string, string) (net.Conn, error) { + attempts++ + return nil, &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED} + }, + }).DialContext + request := httptest.NewRequest(http.MethodPost, "http://site.example.test/", strings.NewReader("not-dispatched")) + recorder := httptest.NewRecorder() + + proxyHandler.ServeHTTP(recorder, request) + + if recorder.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", recorder.Code, http.StatusServiceUnavailable) + } + if got := recorder.Header().Get("Retry-After"); got != "5" { + t.Fatalf("Retry-After = %q, want 5", got) + } + if attempts < 2 { + t.Fatalf("dial attempts = %d, want a bounded retry sequence", attempts) + } +} + func TestRetryingDialerHonorsRequestCancellation(t *testing.T) { t.Parallel() @@ -495,6 +566,88 @@ func TestReverseProxyDialRetrySendsPostBodyExactlyOnce(t *testing.T) { } } +func TestReverseProxyPostConnectResetOmitsRetryAfterAndDoesNotReplayPost(t *testing.T) { + var mu sync.Mutex + requestCount := 0 + requestBody := "" + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("read backend request body: %v", err) + return + } + mu.Lock() + requestCount++ + requestBody += string(body) + mu.Unlock() + + hijacker, ok := w.(http.Hijacker) + if !ok { + t.Error("backend response writer does not support hijacking") + return + } + connection, _, err := hijacker.Hijack() + if err != nil { + t.Errorf("hijack backend connection: %v", err) + return + } + _ = connection.Close() + })) + t.Cleanup(backend.Close) + + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + backendHost, backendPortText, err := net.SplitHostPort(backendURL.Host) + if err != nil { + t.Fatal(err) + } + backendPort, err := strconv.Atoi(backendPortText) + if err != nil { + t.Fatal(err) + } + proxyHandler := New(&config.Config{ + Scheme: "http", + Port: backendPort, + ProxyTarget: &config.ProxyTarget{ + Scheme: "http", + Host: backendHost, + Port: backendPort, + }, + ProxyTimeouts: config.ProxyTimeouts{ + DialTimeout: 2, + DialAttemptTimeout: 1, + DialRetryInterval: 1, + KeepAlive: 1, + IdleConnTimeout: 1, + TLSHandshakeTimeout: 1, + ExpectContinueTimeout: 1, + MaxIdleConns: 10, + }, + Machine: machine.NewGceMachine(), + }) + frontend := httptest.NewServer(proxyHandler) + t.Cleanup(frontend.Close) + + response, err := http.Post(frontend.URL, "text/plain", strings.NewReader("one-copy")) + if err != nil { + t.Fatalf("Post() error = %v", err) + } + response.Body.Close() + if response.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("response status = %d, want %d", response.StatusCode, http.StatusServiceUnavailable) + } + if got := response.Header.Get("Retry-After"); got != "" { + t.Fatalf("Retry-After = %q, want omitted after an ambiguous post-connect failure", got) + } + mu.Lock() + defer mu.Unlock() + if requestCount != 1 || requestBody != "one-copy" { + t.Fatalf("backend received count=%d body=%q, want one exact request", requestCount, requestBody) + } +} + func TestReverseProxyDoesNotRetryHTTPStatus(t *testing.T) { var mu sync.Mutex requestCount := 0 diff --git a/pkg/proxy/tls.go b/pkg/proxy/tls.go index 72c6316..e606a10 100644 --- a/pkg/proxy/tls.go +++ b/pkg/proxy/tls.go @@ -22,6 +22,28 @@ type ReverseProxy struct { Config *config.Config } +var errProxyTargetUnavailable = errors.New("machine does not have a proxy target IP") + +// dialExhaustedError means every failure happened before an origin connection +// was established, so it is safe to tell the caller that the request can be +// retried. Other RoundTrip errors can occur after a request was delivered. +type dialExhaustedError struct { + address string + timeout time.Duration + cause error +} + +func (e *dialExhaustedError) Error() string { + if e.cause == nil { + return fmt.Sprintf("backend %s did not accept a connection within %s", e.address, e.timeout) + } + return fmt.Sprintf("backend %s did not accept a connection within %s: %v", e.address, e.timeout, e.cause) +} + +func (e *dialExhaustedError) Unwrap() error { + return e.cause +} + type retryingDialer struct { totalTimeout time.Duration attemptTimeout time.Duration @@ -83,7 +105,7 @@ func (p *ReverseProxy) targetURL() (*url.URL, error) { host := p.Config.Machine.Host() if host == "" { - return nil, errors.New("machine does not have a proxy target IP") + return nil, errProxyTargetUnavailable } return &url.URL{ Scheme: scheme, @@ -95,7 +117,9 @@ func (p *ReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { target, err := p.targetURL() if err != nil { slog.Warn("Proxy target is unavailable", "error", err) - w.Header().Set("Retry-After", "5") + if errors.Is(err, errProxyTargetUnavailable) { + w.Header().Set("Retry-After", "5") + } http.Error(w, "Backend not available", http.StatusServiceUnavailable) return } @@ -115,7 +139,10 @@ func (p *ReverseProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { }, ErrorHandler: func(w http.ResponseWriter, _ *http.Request, err error) { slog.Warn("Backend proxy request failed", "target", target.Redacted(), "error", err) - w.Header().Set("Retry-After", "5") + var exhausted *dialExhaustedError + if errors.As(err, &exhausted) { + w.Header().Set("Retry-After", "5") + } http.Error(w, "Backend not available", http.StatusServiceUnavailable) }, } @@ -196,10 +223,11 @@ func (d *retryingDialer) exhaustedError(parent context.Context, address string, if err := parent.Err(); err != nil { return err } - if lastErr == nil { - return fmt.Errorf("backend %s did not accept a connection within %s", address, d.totalTimeout) + return &dialExhaustedError{ + address: address, + timeout: d.totalTimeout, + cause: lastErr, } - return fmt.Errorf("backend %s did not accept a connection within %s: %w", address, d.totalTimeout, lastErr) } func isRetryableDialError(err error) bool {