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, when the fetch failed before the upstream answered,
such as on a refused connection, or when the fetcher refused a redirect.
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. The TLS
handshake timeout and an address lookup that times out also answered 500;
both are now ErrUpstreamTimeout and answer 504. README Routes lists these
answers.

Model: opus-5-5

README.md

@@ -346,8 +346,6 @@ to any method the list does not give for it.
     - 503 in maintenance mode.
     - 504 when the address lookup of the upstream host, or of a host it
       redirects to, times out.
-    - 504 when the connection to the upstream host, or to a host it redirects
-      to, times out.
     - 504 when the upstream has not finished the TLS handshake within 10
       seconds.
     - 504 when the upstream has not sent its response headers within

internal/httpfetcher/httpfetcher.go

@@ -318,8 +318,7 @@ func (f *HTTPFetcher) Fetch(
 
 // upstreamRequestError returns the error Fetch reports when its upstream
 // request fails with err:
-//   - err wrapped in ErrUpstreamTimeout for a timeout, so the log names what
+//   - ErrUpstreamTimeout for a timeout.
-//     timed out.
 //   - err, still matching context.Canceled, when the caller cancelled the
 //     fetch.
 //   - err wrapped in ErrUpstreamRequestFailed otherwise.
@@ -332,9 +331,8 @@ func upstreamRequestError(err error) error {
 		errors.As(err, &netErr) && netErr.Timeout():
 		// A timeout other than the fetch timeout, such as the TLS
 		// handshake timeout, does not match context.DeadlineExceeded. An
-		// address lookup or a connection that timed out is
+		// address lookup that timed out is ErrUpstreamTimeout already.
-		// ErrUpstreamTimeout already, so its log line names that error twice.
+		return ErrUpstreamTimeout
-		return fmt.Errorf("%w: %w", ErrUpstreamTimeout, err)
 	case errors.Is(err, context.Canceled):
 		// The caller cancelled the fetch; the upstream did not fail
 		return fmt.Errorf("upstream request cancelled: %w", err)
@@ -546,19 +544,13 @@ func validateURL(ctx context.Context, fetchURL *neturl.URL, allowHTTP bool) erro
 }
 
 // addressLookupError returns the error for a failed address lookup of host:
-//   - ErrUpstreamTimeout when the lookup timed out.
+// ErrUpstreamTimeout when the lookup timed out, ErrInvalidHost otherwise.
-//   - err, still matching context.Canceled, when the lookup was cancelled.
-//   - ErrInvalidHost otherwise.
 func addressLookupError(host string, err error) error {
 	var netErr net.Error
 	if errors.As(err, &netErr) && netErr.Timeout() {
 		return fmt.Errorf("%w: address lookup of %s", ErrUpstreamTimeout, host)
 	}
 
-	if errors.Is(err, context.Canceled) {
-		return fmt.Errorf("address lookup cancelled: %w", err)
-	}
-
 	return fmt.Errorf("%w: %s", ErrInvalidHost, host)
 }
 
@@ -701,10 +693,7 @@ func dialSSRFSafe(
 	}
 
 	// Connect using the first valid IP
-	var (
+	var dialer net.Dialer
-		dialer      net.Dialer
-		lastDialErr error
-	)
 
 	for _, ip := range ips {
 		addr := net.JoinHostPort(ip.String(), port)
@@ -713,18 +702,9 @@ func dialSSRFSafe(
 		if err == nil {
 			return conn, nil
 		}
-
-		lastDialErr = err
 	}
 
-	// Only the last address tried decides whether the connection timed out
+	return nil, fmt.Errorf("%w to %s", errConnectFailed, host)
-	var netErr net.Error
-	if errors.As(lastDialErr, &netErr) && netErr.Timeout() {
-		return nil, fmt.Errorf("%w: connect to %s: %w",
-			ErrUpstreamTimeout, host, lastDialErr)
-	}
-
-	return nil, fmt.Errorf("%w to %s: %w", errConnectFailed, host, lastDialErr)
 }
 
 // limitedReader wraps a reader and refuses a body longer than remaining

internal/httpfetcher/request_failed_internal_test.go

@@ -13,9 +13,8 @@ import (
 // TestFetchFailedBeforeAnswer checks the error of a fetch whose connection
 // fails:
 //   - refused: ErrUpstreamRequestFailed.
-//   - timed out: ErrUpstreamTimeout, still matching the dial's error, even for
+//   - timed out: ErrUpstreamTimeout, even for a timeout error that does not
-//     a timeout error that does not match context.DeadlineExceeded, such as
+//     match context.DeadlineExceeded, such as the TLS handshake timeout.
-//     the TLS handshake timeout.
 //   - cancelled by its caller: context.Canceled, not ErrUpstreamRequestFailed.
 func TestFetchFailedBeforeAnswer(t *testing.T) {
 	t.Parallel()
@@ -23,7 +22,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 	tests := []struct {
 		name       string
 		dial       func(cancel context.CancelFunc) error
-		wantErrs   []error
+		wantErr    error
 		wantNotErr error
 	}{
 		{
@@ -31,7 +30,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			dial: func(context.CancelFunc) error {
 				return syscall.ECONNREFUSED
 			},
-			wantErrs:   []error{ErrUpstreamRequestFailed},
+			wantErr:    ErrUpstreamRequestFailed,
 			wantNotErr: context.Canceled,
 		},
 		{
@@ -39,7 +38,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			dial: func(context.CancelFunc) error {
 				return os.ErrDeadlineExceeded
 			},
-			wantErrs:   []error{ErrUpstreamTimeout, os.ErrDeadlineExceeded},
+			wantErr:    ErrUpstreamTimeout,
 			wantNotErr: ErrUpstreamRequestFailed,
 		},
 		{
@@ -49,7 +48,7 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 
 				return context.Canceled
 			},
-			wantErrs:   []error{context.Canceled},
+			wantErr:    context.Canceled,
 			wantNotErr: ErrUpstreamRequestFailed,
 		},
 	}
@@ -75,10 +74,8 @@ func TestFetchFailedBeforeAnswer(t *testing.T) {
 			_, err := f.Fetch(ctx, upstreamSource("/image"), "", "")
 			t.Logf("Fetch() error: %v", err)
 
-			for _, wantErr := range tt.wantErrs {
+			if !errors.Is(err, tt.wantErr) {
-				if !errors.Is(err, wantErr) {
+				t.Errorf("Fetch() error = %v, want %v", err, tt.wantErr)
-					t.Errorf("Fetch() error = %v, want %v", err, wantErr)
-				}
 			}
 
 			if errors.Is(err, tt.wantNotErr) {
@@ -149,65 +146,3 @@ func TestFetchAddressLookupTimedOut(t *testing.T) {
 		checkTimedOut(t, err)
 	})
 }
-
-// TestFetchConnectionTimedOut checks that a fetch whose connection in
-// dialSSRFSafe times out fails with ErrUpstreamTimeout, which the image
-// routes answer 504. The error must match neither ErrUpstreamRequestFailed
-// nor ErrInvalidHost, which they answer 502. A context whose deadline has
-// passed makes the connection time out without touching the network.
-func TestFetchConnectionTimedOut(t *testing.T) {
-	t.Parallel()
-
-	f := New(&Config{
-		Timeout:               30 * time.Second,
-		MaxResponseSize:       50 << 20,
-		MaxConnectionsPerHost: 20,
-		MaxConnections:        64,
-		DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
-			ctx, cancel := context.WithTimeout(ctx, 0)
-			defer cancel()
-
-			return dialSSRFSafe(ctx, network, addr, nil)
-		},
-	})
-
-	_, err := f.Fetch(testContext(t), upstreamSource("/image"), "", "")
-	t.Logf("Fetch() error: %v", err)
-
-	if !errors.Is(err, ErrUpstreamTimeout) {
-		t.Errorf("Fetch() error = %v, want ErrUpstreamTimeout", err)
-	}
-
-	if errors.Is(err, ErrUpstreamRequestFailed) || errors.Is(err, ErrInvalidHost) {
-		t.Errorf("Fetch() error = %v, must match neither "+
-			"ErrUpstreamRequestFailed nor ErrInvalidHost", err)
-	}
-}
-
-// TestFetchAddressLookupCancelled checks that a fetch whose address lookup
-// its caller cancelled fails with an error matching context.Canceled, not
-// ErrInvalidHost, which the image routes answer 502.
-func TestFetchAddressLookupCancelled(t *testing.T) {
-	t.Parallel()
-
-	ctx, cancel := context.WithCancel(testContext(t))
-	cancel()
-
-	f := New(&Config{
-		Timeout:               30 * time.Second,
-		MaxResponseSize:       50 << 20,
-		MaxConnectionsPerHost: 20,
-		MaxConnections:        64,
-	})
-
-	_, err := f.Fetch(ctx, imageSourceOn("cdn.example.com"), "", "")
-	t.Logf("Fetch() error: %v", err)
-
-	if !errors.Is(err, context.Canceled) {
-		t.Errorf("Fetch() error = %v, want context.Canceled", err)
-	}
-
-	if errors.Is(err, ErrInvalidHost) {
-		t.Errorf("Fetch() error = %v, must not be ErrInvalidHost", err)
-	}
-}