Answer 502 for an upstream host with no address or a failed fetch (closes #367)

The image routes answered 500 and logged at error level when the upstream
host had no address, or when the fetch failed before the upstream
answered, such as on a refused connection. The fetcher now marks such a
failure with ErrUpstreamRequestFailed; a cancelled fetch is not marked.
respondImageError answers it, and ErrInvalidHost, 502 "upstream error"
and logs them at warn level. README Routes lists both under 502.

Model: opus-5-5

README.md

@@ -15,10 +15,9 @@ In this document, the upstream host is the server that holds an original image.
 ## Getting Started
 
 ```bash
-# clone, install what compiling needs, and build
+# clone and build
 git clone https://git.eeqj.de/sneak/pixa.git
 cd pixa
-script/bootstrap --cgo
 make build
 
 # make a config file from the example and put a real signing key in it
@@ -27,30 +26,18 @@ openssl rand -base64 32   # prints a new signing key
 $EDITOR config.yml        # replace the signing_key placeholder with it
 ./bin/pixad --config config.yml
 
-# or build and run the Docker image with that signing key
+# or build and run the Docker image
 make docker
-docker run -p 8080:8080 -e PIXA_SIGNING_KEY='the new signing key' pixa:latest
+docker run -p 8080:8080 -e PIXA_SIGNING_KEY="$(openssl rand -base64 32)" pixa:latest
 ```
 
-`script/bootstrap --cgo` installs Go, a C compiler and the image libraries pixa
-needs (see Deployment). `make build` then compiles pixa on your machine into
-`bin/pixad`. `pixad` is the server program.
+`make build` compiles pixa on your machine into `bin/pixad`. `pixad` is the
+server program. Compiling needs the libraries `script/bootstrap --cgo` installs
+(see Deployment).
 
 The signing key is pixa's one required secret. It must be 32 characters or
 longer. The placeholder in the example file is refused at startup.
 
-A container takes its settings from environment variables (see Configuration).
-Only `PIXA_SIGNING_KEY` is required. Without it, the container exits at startup
-and names the variable. Every other setting has a built-in default. A config
-file mounted at `/etc/pixa/config.yml` is read when present. An environment
-variable wins over the same setting in the file.
-
-pixa's login page is at `http://localhost:8080/`. Logging in there works only
-with `debug: true` in `config.yml`, or `-e PIXA_DEBUG=true` for the container.
-Otherwise pixa refuses the login with 403, because the page was opened over
-plain HTTP. Use `debug` only on your own machine. A pixa that others use serves
-its pages over HTTPS through a reverse proxy (see Deployment).
-
 Once pixa runs, it serves images through URLs of this form:
 
 ```
@@ -73,25 +60,11 @@ The example config allowlists no host. Every image on an allowlisted host is
 served to anyone, without a signature, at any size and in any format. So
 allowlist only hosts you control, where nobody else can put images.
 
-To see pixa serve an image, sign a URL for an image on any public HTTPS server.
-Run this in a second terminal while pixa runs, with your own signing key and
-image:
-
-```bash
-key='the new signing key'
-image_host=cdn.example.com    # a public HTTPS server
-image_path=/photos/cat.jpg    # an image on it
-exp=$(($(date +%s) + 3600))   # the URL expires in one hour
-sig=$(printf '%s' "$image_host:$image_path::400:300:webp:$exp:85:cover" |
-    openssl dgst -sha256 -hmac "$key" -binary | base64 | tr '+/' '-_')
-curl --fail-with-body -o cat.webp \
-    "http://localhost:8080/v1/image/$image_host$image_path/400x300.webp?sig=$sig&exp=$exp"
-```
-
-`cat.webp` is then the image at 400x300. If pixa refuses the request, curl
-reports an error and `cat.webp` holds pixa's error message instead. The image
-path must not hold a `%`, a `:` or a query. Signed URLs explains how to sign
-those.
+A container takes its settings from environment variables (see Configuration).
+Only `PIXA_SIGNING_KEY` is required. Without it, the container exits at startup
+and names the variable. Every other setting has a built-in default. A config
+file mounted at `/etc/pixa/config.yml` is read when present. An environment
+variable wins over the same setting in the file.
 
 ## Deployment
 
@@ -337,15 +310,11 @@ to any method the list does not give for it.
       neither the image nor its source in its cache.
     - 502 likewise for 5 minutes after a body whose first bytes are not those of
       its `Content-Type`.
-    - 502 when the upstream host, or a host it redirects to, has no address.
-    - 502 when the upstream redirects to a URL that is not `https`, or redirects
-      10 times.
+    - 502 when the upstream host has no address.
     - 502 when the fetch fails before the upstream answers, such as on a refused
       connection or a failed TLS handshake.
     - 503 when pixa is busy (see Limits).
     - 503 in maintenance mode.
-    - 504 when the upstream has not finished the TLS handshake within 10
-      seconds.
     - 504 when the upstream has not sent its response headers within
       `upstream_fetch_timeout`.
     - 500 when that time runs out while the image itself is still arriving.

go.mod

@@ -17,7 +17,6 @@ require (
 	github.com/prometheus/client_golang v1.23.2
 	github.com/slok/go-http-metrics v0.13.0
 	github.com/spf13/cobra v1.10.2
-	go.uber.org/dig v1.19.0
 	go.uber.org/fx v1.24.0
 	golang.org/x/crypto v0.41.0
 	golang.org/x/sync v0.19.0
@@ -127,6 +126,7 @@ require (
 	go.opentelemetry.io/otel v1.36.0 // indirect
 	go.opentelemetry.io/otel/metric v1.36.0 // indirect
 	go.opentelemetry.io/otel/trace v1.36.0 // indirect
+	go.uber.org/dig v1.19.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.27.0 // indirect
 	go.yaml.in/yaml/v2 v2.4.2 // indirect

internal/app/app.go

@@ -2,16 +2,13 @@
 package app
 
 import (
-	"context"
 	"fmt"
-	"io"
 	"os"
 	"os/signal"
 	"syscall"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/spf13/cobra"
-	"go.uber.org/dig"
 	"go.uber.org/fx"
 	"sneak.berlin/go/pixa/internal/config"
 	"sneak.berlin/go/pixa/internal/database"
@@ -56,10 +53,7 @@ func run(_ *cobra.Command, _ []string) {
 	// A write to a closed stdout or stderr must not end the process.
 	signal.Ignore(syscall.SIGPIPE)
 
-	app := fx.New(
-		// pixad logs with slog only; runApp prints a startup or shutdown
-		// error itself.
-		fx.NopLogger,
+	fx.New(
 		fx.Provide(
 			config.New,
 			database.New,
@@ -77,40 +71,5 @@ func run(_ *cobra.Command, _ []string) {
 			func(log *logger.Logger) { log.Identify() },
 			func(*server.Server) {},
 		),
-	)
-
-	exitCode := runApp(app, os.Stderr)
-	if exitCode != 0 {
-		os.Exit(exitCode)
-	}
-}
-
-// runApp starts app, waits for a signal or a shutdown request, stops app,
-// and returns the status pixad exits with, as fx.App.Run does. It prints
-// a start or stop error to stderr, once, as the error that caused it:
-// dig.RootCause drops the frames fx and dig wrap around it.
-func runApp(app *fx.App, stderr io.Writer) int {
-	startCtx, cancelStart := context.WithTimeout(context.Background(), app.StartTimeout())
-	defer cancelStart()
-
-	err := app.Start(startCtx)
-	if err != nil {
-		_, _ = fmt.Fprintln(stderr, dig.RootCause(err))
-
-		return 1
-	}
-
-	shutdown := <-app.Wait()
-
-	stopCtx, cancelStop := context.WithTimeout(context.Background(), app.StopTimeout())
-	defer cancelStop()
-
-	err = app.Stop(stopCtx)
-	if err != nil {
-		_, _ = fmt.Fprintln(stderr, dig.RootCause(err))
-
-		return 1
-	}
-
-	return shutdown.ExitCode
+	).Run()
 }

internal/app/app_internal_test.go

@@ -1,65 +0,0 @@
-package app
-
-import (
-	"bytes"
-	"context"
-	"errors"
-	"io"
-	"testing"
-
-	"go.uber.org/fx"
-)
-
-var errTestStopFailed = errors.New("test stop hook failed")
-
-// TestRunAppPrintsStopErrorOnce checks that when a stop hook fails, runApp
-// prints the hook's error once and returns exit status 1.
-func TestRunAppPrintsStopErrorOnce(t *testing.T) {
-	t.Parallel()
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Invoke(func(lifecycle fx.Lifecycle, shutdowner fx.Shutdowner) {
-			lifecycle.Append(fx.Hook{
-				// Ask to shut down as soon as the app has started, so
-				// that runApp goes on to stop it.
-				OnStart: func(context.Context) error { return shutdowner.Shutdown() },
-				OnStop:  func(context.Context) error { return errTestStopFailed },
-			})
-		}),
-	)
-
-	var stderr bytes.Buffer
-
-	exitCode := runApp(app, &stderr)
-	if exitCode != 1 {
-		t.Errorf("runApp() = %d, want 1", exitCode)
-	}
-
-	want := errTestStopFailed.Error() + "\n"
-	if stderr.String() != want {
-		t.Errorf("stderr = %q, want %q", stderr.String(), want)
-	}
-}
-
-// TestRunAppReturnsShutdownExitCode checks that runApp returns the exit
-// code a shutdown request names.
-func TestRunAppReturnsShutdownExitCode(t *testing.T) {
-	t.Parallel()
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Invoke(func(lifecycle fx.Lifecycle, shutdowner fx.Shutdowner) {
-			lifecycle.Append(fx.Hook{
-				OnStart: func(context.Context) error {
-					return shutdowner.Shutdown(fx.ExitCode(3))
-				},
-			})
-		}),
-	)
-
-	exitCode := runApp(app, io.Discard)
-	if exitCode != 3 {
-		t.Errorf("runApp() = %d, want 3", exitCode)
-	}
-}

internal/app/app_test.go

@@ -1,177 +0,0 @@
-package app_test
-
-import (
-	"bytes"
-	"errors"
-	"fmt"
-	"net"
-	"net/http"
-	"os"
-	"os/exec"
-	"strconv"
-	"syscall"
-	"testing"
-	"time"
-
-	"sneak.berlin/go/pixa/internal/app"
-)
-
-// These tests run pixad in a child process: the test binary itself,
-// started again with runPixadInChildEnvVar set, which makes TestMain run
-// pixad instead of the tests.
-const runPixadInChildEnvVar = "TEST_RUN_PIXAD_IN_CHILD"
-
-const validTestSigningKey = "test-signing-key-for-app-tests-0123456789"
-
-func TestMain(m *testing.M) {
-	if os.Getenv(runPixadInChildEnvVar) == "1" {
-		os.Args = []string{"pixad"}
-
-		app.Run("test")
-		os.Exit(0)
-	}
-
-	os.Exit(m.Run())
-}
-
-// pixadCommand returns a command that runs pixad with only the given
-// environment, in an empty directory with an empty HOME, so that no
-// config file is found.
-func pixadCommand(t *testing.T, env ...string) (*exec.Cmd, *bytes.Buffer) {
-	t.Helper()
-
-	testBinary := os.Args[0]
-	cmd := exec.CommandContext(t.Context(), testBinary) //nolint:gosec // not tainted
-	cmd.Dir = t.TempDir()
-	// A test binary built with -cover warns on stderr at exit when
-	// GOCOVERDIR is not set.
-	cmd.Env = append([]string{
-		runPixadInChildEnvVar + "=1",
-		"HOME=" + t.TempDir(),
-		"GOCOVERDIR=" + t.TempDir(),
-	}, env...)
-
-	var stderr bytes.Buffer
-
-	cmd.Stderr = &stderr
-
-	return cmd, &stderr
-}
-
-// TestRunPrintsStartupErrorOnce checks that an invalid setting is
-// printed once, as the line that names it, and nothing else goes to
-// stderr.
-func TestRunPrintsStartupErrorOnce(t *testing.T) {
-	t.Parallel()
-
-	cmd, stderr := pixadCommand(t, "PIXA_SIGNING_KEY=short")
-
-	err := cmd.Run()
-
-	var exitErr *exec.ExitError
-	if !errors.As(err, &exitErr) || exitErr.ExitCode() != 1 {
-		t.Fatalf("pixad: got %v, want exit status 1; stderr:\n%s", err, stderr)
-	}
-
-	want := `config key "signing_key" (environment variable PIXA_SIGNING_KEY): ` +
-		"value too short: must be at least 32 characters, got 5\n"
-	if stderr.String() != want {
-		t.Errorf("stderr:\n%s\nwant:\n%s", stderr, want)
-	}
-}
-
-// TestRunWritesNothingToStderrOnStartAndStop starts pixad, waits until
-// it answers its health check, stops it with SIGTERM, and checks that it
-// wrote nothing to stderr.
-func TestRunWritesNothingToStderrOnStartAndStop(t *testing.T) {
-	t.Parallel()
-
-	port := freePort(t)
-	cmd, stderr := pixadCommand(t,
-		"PORT="+strconv.Itoa(port),
-		"PIXA_STATE_DIR="+t.TempDir(),
-		"PIXA_SIGNING_KEY="+validTestSigningKey,
-	)
-
-	err := cmd.Start()
-	if err != nil {
-		t.Fatalf("starting pixad: %v", err)
-	}
-
-	waitForHealthCheck(t, cmd, port, stderr)
-
-	err = cmd.Process.Signal(syscall.SIGTERM)
-	if err != nil {
-		t.Fatalf("signalling pixad: %v", err)
-	}
-
-	err = cmd.Wait()
-	if err != nil {
-		t.Fatalf("pixad: got %v, want exit status 0; stderr:\n%s", err, stderr)
-	}
-
-	if stderr.Len() != 0 {
-		t.Errorf("stderr is not empty:\n%s", stderr)
-	}
-}
-
-// freePort returns a TCP port on the loopback address that nothing
-// listens on.
-func freePort(t *testing.T) int {
-	t.Helper()
-
-	var listenConfig net.ListenConfig
-
-	listener, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0")
-	if err != nil {
-		t.Fatalf("finding a free port: %v", err)
-	}
-
-	addr, ok := listener.Addr().(*net.TCPAddr)
-	if !ok {
-		t.Fatalf("listener address %v is not a TCP address", listener.Addr())
-	}
-
-	err = listener.Close()
-	if err != nil {
-		t.Fatalf("closing listener: %v", err)
-	}
-
-	return addr.Port
-}
-
-// waitForHealthCheck waits up to 30 seconds for pixad, started as cmd, to
-// answer its health check on port. If it does not, it stops pixad and
-// fails the test with what pixad wrote to stderr.
-func waitForHealthCheck(t *testing.T, cmd *exec.Cmd, port int, stderr *bytes.Buffer) {
-	t.Helper()
-
-	url := fmt.Sprintf("http://127.0.0.1:%d/.well-known/healthcheck.json", port)
-	deadline := time.Now().Add(30 * time.Second)
-
-	for time.Now().Before(deadline) {
-		req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, nil)
-		if err != nil {
-			t.Fatalf("health check request: %v", err)
-		}
-
-		resp, err := http.DefaultClient.Do(req)
-		if err == nil {
-			_ = resp.Body.Close()
-
-			if resp.StatusCode == http.StatusOK {
-				return
-			}
-		}
-
-		time.Sleep(100 * time.Millisecond)
-	}
-
-	// Stop pixad first: stderr is complete, and safe to read, only once
-	// cmd.Wait has returned.
-	_ = cmd.Process.Kill()
-	_ = cmd.Wait()
-
-	t.Fatalf("pixad did not answer its health check on port %d; stderr:\n%s",
-		port, stderr)
-}

internal/httpfetcher/httpfetcher.go

@@ -287,7 +287,15 @@ func (f *HTTPFetcher) Fetch(
 	fetchDuration := time.Since(startTime)
 
 	if err != nil {
-		return nil, upstreamRequestError(err)
+		switch {
+		case errors.Is(err, context.DeadlineExceeded):
+			return nil, ErrUpstreamTimeout
+		case errors.Is(err, context.Canceled):
+			// The caller cancelled the fetch; the upstream did not fail
+			return nil, fmt.Errorf("upstream request cancelled: %w", err)
+		default:
+			return nil, fmt.Errorf("%w: %w", ErrUpstreamRequestFailed, err)
+		}
 	}
 
 	// The deferred release gives the connection back, as no body follows
@@ -316,29 +324,6 @@ func (f *HTTPFetcher) Fetch(
 	return result, nil
 }
 
-// upstreamRequestError returns the error Fetch reports when its upstream
-// request fails with err:
-//   - ErrUpstreamTimeout for a timeout.
-//   - err, still matching context.Canceled, when the caller cancelled the
-//     fetch.
-//   - err wrapped in ErrUpstreamRequestFailed otherwise.
-func upstreamRequestError(err error) error {
-	var netErr net.Error
-
-	switch {
-	case errors.Is(err, context.DeadlineExceeded),
-		errors.As(err, &netErr) && netErr.Timeout():
-		// A timeout other than the fetch timeout, such as the TLS
-		// handshake timeout, does not match context.DeadlineExceeded
-		return ErrUpstreamTimeout
-	case errors.Is(err, context.Canceled):
-		// The caller cancelled the fetch; the upstream did not fail
-		return fmt.Errorf("upstream request cancelled: %w", err)
-	default:
-		return fmt.Errorf("%w: %w", ErrUpstreamRequestFailed, err)
-	}
-}
-
 // requestHeaders returns the headers of a fetch: User-Agent, Accept, the
 // ID of the request the fetch serves, so the fetch can be found in the
 // upstream host's logs, and a non-empty ifNoneMatch or ifModifiedSince.

internal/httpfetcher/request_failed_internal_test.go

@@ -4,18 +4,15 @@ import (
 	"context"
 	"errors"
 	"net"
-	"os"
 	"syscall"
 	"testing"
 	"time"
 )
 
-// TestFetchFailedBeforeAnswer checks the error of a fetch whose connection
-// fails:
-//   - refused: ErrUpstreamRequestFailed.
-//   - timed out: ErrUpstreamTimeout, even for a timeout error that does not
-//     match context.DeadlineExceeded, such as the TLS handshake timeout.
-//   - cancelled by its caller: context.Canceled, not ErrUpstreamRequestFailed.
+// TestFetchFailedBeforeAnswer checks that a fetch whose connection is refused
+// fails with ErrUpstreamRequestFailed, and that a fetch its caller cancels
+// while connecting fails with context.Canceled and not with
+// ErrUpstreamRequestFailed.
 func TestFetchFailedBeforeAnswer(t *testing.T) {
 	t.Parallel()
 
@@ -33,14 +30,6 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			wantErr:    ErrUpstreamRequestFailed,
 			wantNotErr: context.Canceled,
 		},
-		{
-			name: "timed out while connecting",
-			dial: func(context.CancelFunc) error {
-				return os.ErrDeadlineExceeded
-			},
-			wantErr:    ErrUpstreamTimeout,
-			wantNotErr: ErrUpstreamRequestFailed,
-		},
 		{
 			name: "cancelled while connecting",
 			dial: func(cancel context.CancelFunc) error {