diff --git a/docs/references/settings/stores/blob_settings.md b/docs/references/settings/stores/blob_settings.md index 4f4776141e..8426ed0b53 100644 --- a/docs/references/settings/stores/blob_settings.md +++ b/docs/references/settings/stores/blob_settings.md @@ -17,6 +17,7 @@ | checksum | bool | true | File backend parameter | Writes a `.sha256` sidecar alongside every blob. No read path verifies it, so this is a write-cost parameter, not an integrity one. Accepted values are `true`/`1`/`yes`/`on`/`enabled` and `false`/`0`/`no`/`off`/`disabled`; anything else fails at startup | | fsyncMode | string | full | File backend parameter | `full` fsyncs the temp file and then the parent directory after the rename. `data` skips the parent-directory fsync, so a freshly published filename can be lost across a crash while already-published content survives. `none` skips both | | header | string | "" | File backend parameter | Custom header prepended to blobs | +| requireMissHeader | bool | false | `http://` store parameter (`storeURL.Query().Get("requireMissHeader") == "true"`) | Makes `Exists` read a 404 as "not held" only when it carries the blob server's `X-Blob-Miss` header. A 404 without it (a wrong base URL, a proxy) is returned as a storage error instead of a miss. Default `false` keeps a bare 404 a miss, for blob servers that predate the header: enable it once every blob server in the deployment has been upgraded. `Get`, `GetIoReader` and `Del` already accept the header and fall back to the server's not-found body | ## Settings diff --git a/stores/blob/http/http.go b/stores/blob/http/http.go index 555f71d3d2..34b7414131 100644 --- a/stores/blob/http/http.go +++ b/stores/blob/http/http.go @@ -50,6 +50,12 @@ const ( // notFoundBodyLimit bounds how much of a 404 body is read to compare it with NotFoundMsg. // It is longer than the body it looks for, so a longer body cannot be cut down to a match. notFoundBodyLimit = 64 + + // MissHeader is the response header the blob server sets on every path that answers a + // miss (404). A HEAD response carries no body, so Exists cannot check NotFoundMsg; this + // header lets it tell the server's own miss from a wrong base URL or a proxy 404, which + // a bare 404 cannot distinguish. GET prefers it too, falling back to the body. + MissHeader = "X-Blob-Miss" ) // HTTPStore implements the blob.Store interface by making HTTP requests to a remote @@ -70,6 +76,11 @@ type HTTPStore struct { logger ulogger.Logger // options contains configuration options for the HTTP blob store options *options.Options + // requireMissHeader, when set, makes Exists treat a 404 that lacks MissHeader as a + // storage error rather than a miss. Off by default so a mixed fleet still works with + // servers that predate the header; enable (?requireMissHeader=true on the store URL) + // once every blob server sends it. + requireMissHeader bool } // refuseBlobRedirect refuses every redirect. A blob server does not redirect, so a redirect @@ -80,10 +91,18 @@ func refuseBlobRedirect(req *http.Request, _ []*http.Request) error { } // isBlobServerMiss reports whether a 404 came from the blob server answering a miss, which it -// does with NotFoundMsg. Anything else answering 404 on that address - a wrong base URL, a proxy - -// is not a verdict on the blob. It reads from the body, so the caller closes it afterwards. +// marks with MissHeader and, for a server that predates the header, answers with NotFoundMsg. +// Anything else answering 404 on that address - a wrong base URL, a proxy - is not a verdict +// on the blob. It always drains the small body, so the transport can reuse the connection, and +// the caller closes it afterwards. func isBlobServerMiss(resp *http.Response) bool { body, err := io.ReadAll(io.LimitReader(resp.Body, notFoundBodyLimit)) + + // The header decides on its own: it is set on every miss path, whatever the body says. + if resp.Header.Get(MissHeader) != "" { + return true + } + if err != nil { return false } @@ -143,6 +162,10 @@ func New(logger ulogger.Logger, storeURL *url.URL, opts ...options.StoreOption) return nil, errors.NewConfigurationError("blob http store URL must not carry an authToken query parameter - set blob_httpAuthToken or pass options.WithHTTPAuthToken") } + // Read before the query is stripped below. Opt-in and default off: until every blob + // server sends MissHeader, a bare HEAD 404 must still read as a miss for compatibility. + requireMissHeader := storeURL.Query().Get("requireMissHeader") == "true" + // baseURL is formatted into "%s/blob/%s?%s", so it must carry no query of its own - // otherwise every request URL comes out malformed. Strip query and fragment. base := *storeURL @@ -159,8 +182,9 @@ func New(logger ulogger.Logger, storeURL *url.URL, opts ...options.StoreOption) Timeout: 30 * time.Second, CheckRedirect: refuseBlobRedirect, }, - logger: logger, - options: storeOpts, + logger: logger, + options: storeOpts, + requireMissHeader: requireMissHeader, }, nil } @@ -227,6 +251,27 @@ func (s *HTTPStore) Exists(ctx context.Context, key []byte, fileType fileformat. case http.StatusOK: return true, nil case http.StatusNotFound: + // The blob server marks its own miss with MissHeader on every miss path. A HEAD + // has no body, so unlike GET this cannot fall back to NotFoundMsg - the header is + // the only signal that tells the server's miss from a wrong base URL or a proxy + // answering 404. + if resp.Header.Get(MissHeader) != "" { + return false, nil + } + + if s.requireMissHeader { + s.logger.Warnf("[HTTPStore] Exists %s %s: %s answered 404 without %s", fileType, util.ReverseAndHexEncodeSlice(key), s.redactedBaseURL, MissHeader) // urlsafe: redactedBaseURL is base.Redacted() output, set in New + + // Error text never names the server (the asset service surfaces it); the address + // is in the Warn above. A bare 404 is a fault, as a wrong status is below. + return false, errors.NewStorageError("[HTTPStore] Exists failed: 404 without %s (wrong base URL or a proxy?)", MissHeader) + } + + // Mixed-fleet compatibility: an old server sends no header, so keep reading a bare + // 404 as a miss until requireMissHeader is set. Debug, not Warn: every legitimate + // miss from such a server lands here. + s.logger.Debugf("[HTTPStore] Exists %s %s: %s answered 404 without %s", fileType, util.ReverseAndHexEncodeSlice(key), s.redactedBaseURL, MissHeader) // urlsafe: redactedBaseURL is base.Redacted() output, set in New + return false, nil default: s.logger.Warnf("[HTTPStore] Exists %s %s: %s answered status code %d", fileType, util.ReverseAndHexEncodeSlice(key), s.redactedBaseURL, resp.StatusCode) // urlsafe: redactedBaseURL is base.Redacted() output, set in New @@ -309,6 +354,8 @@ func (s *HTTPStore) GetIoReader(ctx context.Context, key []byte, fileType filefo case http.StatusOK: return resp.Body, nil case http.StatusNotFound: + // The server's miss header, set on every miss path, decides it; the NotFoundMsg body + // is the fallback for a server that predates it. miss := isBlobServerMiss(resp) resp.Body.Close() @@ -316,7 +363,7 @@ func (s *HTTPStore) GetIoReader(ctx context.Context, key []byte, fileType filefo return nil, errors.NewNotFoundError("[HTTPStore] %s %s not found", fileType, util.ReverseAndHexEncodeSlice(key)) } - s.logger.Warnf("[HTTPStore] GetIoReader %s %s: %s answered 404 without the blob server's not-found body", fileType, util.ReverseAndHexEncodeSlice(key), s.redactedBaseURL) // urlsafe: redactedBaseURL is base.Redacted() output, set in New + s.logger.Warnf("[HTTPStore] GetIoReader %s %s: %s answered 404 without %s or the blob server's not-found body", fileType, util.ReverseAndHexEncodeSlice(key), s.redactedBaseURL, MissHeader) // urlsafe: redactedBaseURL is base.Redacted() output, set in New return nil, errors.NewStorageError("[HTTPStore] GetIoReader failed with status code 404 without the blob server's not-found body") default: diff --git a/stores/blob/http/http_test.go b/stores/blob/http/http_test.go index 8a94fd71db..c0dccf0317 100644 --- a/stores/blob/http/http_test.go +++ b/stores/blob/http/http_test.go @@ -591,6 +591,147 @@ func TestHTTPStore_GetMissIsNotFound(t *testing.T) { } } +// TestHTTPStore_ExistsUsesMissHeader pins #1965: a HEAD carries no body, so Exists tells the +// blob server's own miss from a wrong base URL or a proxy 404 only by MissHeader. A 404 with +// the header is a miss; a bare 404 stays a miss by default (mixed-fleet compatibility with +// servers that predate the header) but becomes a store fault when the store requires it. +func TestHTTPStore_ExistsUsesMissHeader(t *testing.T) { + key := []byte{0x01, 0x02, 0xab} + + withMissHeader := func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(MissHeader, "1") + w.WriteHeader(http.StatusNotFound) + } + bare404 := func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) } + held := func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) } + + t.Run("404 with the miss header is not held, no error", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(withMissHeader)) + t.Cleanup(server.Close) + + exists, err := newStoreFor(t, server).Exists(context.Background(), key, fileformat.FileTypeTesting) + require.NoError(t, err) + require.False(t, exists) + }) + + t.Run("200 is held", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(held)) + t.Cleanup(server.Close) + + exists, err := newStoreFor(t, server).Exists(context.Background(), key, fileformat.FileTypeTesting) + require.NoError(t, err) + require.True(t, exists) + }) + + t.Run("bare 404 is a miss by default (mixed-fleet compat)", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(bare404)) + t.Cleanup(server.Close) + + exists, err := newStoreFor(t, server).Exists(context.Background(), key, fileformat.FileTypeTesting) + require.NoError(t, err) + require.False(t, exists) + }) + + t.Run("bare 404 is a store fault when requireMissHeader is set", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(bare404)) + t.Cleanup(server.Close) + + exists, err := newStoreRequiringMissHeader(t, server).Exists(context.Background(), key, fileformat.FileTypeTesting) + require.Error(t, err) + require.ErrorIs(t, err, errors.ErrStorageError) + require.False(t, exists) + require.NotContains(t, err.Error(), server.Listener.Addr().String(), "a fault does not name the blob server in its text") + }) + + t.Run("404 with the header is still a miss under requireMissHeader", func(t *testing.T) { + // The header, not the strict mode, is what makes a 404 a miss: under + // requireMissHeader a 404 that carries it is still not-held with no error. + server := httptest.NewServer(http.HandlerFunc(withMissHeader)) + t.Cleanup(server.Close) + + exists, err := newStoreRequiringMissHeader(t, server).Exists(context.Background(), key, fileformat.FileTypeTesting) + require.NoError(t, err) + require.False(t, exists) + }) +} + +// TestHTTPStore_MissHeaderAloneDecidesAMiss pins that MissHeader on its own marks a 404 as the +// blob server's miss on the GET and DELETE paths, whatever the body says. The server also sends +// NotFoundMsg, and that fallback would hide a header branch that never fires, so these answers +// carry the header with a body that is not NotFoundMsg. +func TestHTTPStore_MissHeaderAloneDecidesAMiss(t *testing.T) { + key := []byte{0x01, 0x02, 0xab} + + headerForeignBody := func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(MissHeader, "1") + http.Error(w, "an unrelated body", http.StatusNotFound) + } + foreignBodyNoHeader := func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "an unrelated body", http.StatusNotFound) + } + + t.Run("GetIoReader: header with a foreign body is a miss", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(headerForeignBody)) + t.Cleanup(server.Close) + + rc, err := newStoreFor(t, server).GetIoReader(context.Background(), key, fileformat.FileTypeTx) + require.Nil(t, rc) + require.ErrorIs(t, err, errors.ErrNotFound) + require.NotErrorIs(t, err, errors.ErrStorageError) + }) + + t.Run("Get: header with a foreign body is a miss", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(headerForeignBody)) + t.Cleanup(server.Close) + + _, err := newStoreFor(t, server).Get(context.Background(), key, fileformat.FileTypeTx) + require.ErrorIs(t, err, errors.ErrNotFound) + require.NotErrorIs(t, err, errors.ErrStorageError) + }) + + t.Run("GetIoReader: a foreign body without the header is a store fault", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(foreignBodyNoHeader)) + t.Cleanup(server.Close) + + rc, err := newStoreFor(t, server).GetIoReader(context.Background(), key, fileformat.FileTypeTx) + require.Nil(t, rc) + require.ErrorIs(t, err, errors.ErrStorageError) + require.NotErrorIs(t, err, errors.ErrNotFound) + }) + + t.Run("Del: header with a foreign body is a miss, nothing to delete", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(headerForeignBody)) + t.Cleanup(server.Close) + + require.NoError(t, newStoreFor(t, server).Del(context.Background(), key, fileformat.FileTypeTx)) + }) + + t.Run("Del: a foreign body without the header is a store fault", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(foreignBodyNoHeader)) + t.Cleanup(server.Close) + + err := newStoreFor(t, server).Del(context.Background(), key, fileformat.FileTypeTx) + require.ErrorIs(t, err, errors.ErrStorageError) + }) +} + +// newStoreRequiringMissHeader builds a store with ?requireMissHeader=true on its URL. +func newStoreRequiringMissHeader(t *testing.T, server *httptest.Server) *HTTPStore { + t.Helper() + + u, err := url.Parse(server.URL) + require.NoError(t, err) + + q := u.Query() + q.Set("requireMissHeader", "true") + u.RawQuery = q.Encode() + + store, err := New(ulogger.TestLogger{}, u, options.WithHTTPAuthToken("")) + require.NoError(t, err) + + return store +} + const storePassword = "s3cret-password" // logRecorder is the testing.T a ulogger.UnifiedTestLogger writes to. It keeps every line, so a diff --git a/stores/blob/server.go b/stores/blob/server.go index 4b67d01ca6..3641d733e7 100644 --- a/stores/blob/server.go +++ b/stores/blob/server.go @@ -254,6 +254,9 @@ func (s *HTTPBlobServer) handleExists(w http.ResponseWriter, r *http.Request, op if exists { w.WriteHeader(http.StatusOK) } else { + // Mark our own miss so the client's Exists can tell it from a wrong base URL or a + // proxy 404 - a HEAD has no body to carry NotFoundMsg. + w.Header().Set(blobhttp.MissHeader, "1") w.WriteHeader(http.StatusNotFound) } } @@ -687,6 +690,10 @@ func (s *HTTPBlobServer) handleDelete(w http.ResponseWriter, r *http.Request, op // its caller as a miss, so a fault answered 404 would reach that caller as an absent blob. func writeStoreError(w http.ResponseWriter, err error) { if errors.Is(err, errors.ErrNotFound) && !errors.Is(err, errors.ErrStorageError) { + // Mark our own miss (GET, ranged GET, PATCH) so the client reads a 404 as a miss + // only when it came from here, not from a proxy. http.Error sets the status after + // this, so the header survives. + w.Header().Set(blobhttp.MissHeader, "1") http.Error(w, NotFoundMsg, http.StatusNotFound) return } diff --git a/stores/blob/server_test.go b/stores/blob/server_test.go index 6c5dd36355..8a1de56da2 100644 --- a/stores/blob/server_test.go +++ b/stores/blob/server_test.go @@ -4,6 +4,7 @@ package blob import ( "bytes" "context" + "encoding/base64" "fmt" "io" "math" @@ -361,6 +362,48 @@ func (s *fileBackedFakeStore) underlying(t *testing.T) Store { return store } +// TestHTTPBlobServer_MarksMissWithHeader pins #1965: the server marks its own miss (404) with +// MissHeader on every miss path - the Exists/HEAD path and the GET path through writeStoreError - +// so the client can tell it from a wrong base URL or a proxy 404. A HEAD carries no body, so the +// header is the only signal there. +func TestHTTPBlobServer_MarksMissWithHeader(t *testing.T) { + srv := &HTTPBlobServer{store: missOnlyStore{}, logger: ulogger.New("miss-header-test")} + + key := []byte("absent-key") + path := "/blob/" + base64.URLEncoding.EncodeToString(key) + "." + fileformat.FileTypeTesting.String() + + t.Run("Exists (HEAD) miss carries the header", func(t *testing.T) { + rr := httptest.NewRecorder() + srv.handleExists(rr, httptest.NewRequest(http.MethodHead, path, nil)) + + require.Equal(t, http.StatusNotFound, rr.Code) + require.Equal(t, "1", rr.Header().Get(blobhttp.MissHeader), + "a HEAD miss must carry %s - it has no body to carry NotFoundMsg", blobhttp.MissHeader) + }) + + t.Run("GET miss carries the header", func(t *testing.T) { + rr := httptest.NewRecorder() + srv.handleGet(rr, httptest.NewRequest(http.MethodGet, path, nil)) + + require.Equal(t, http.StatusNotFound, rr.Code) + require.Equal(t, "1", rr.Header().Get(blobhttp.MissHeader), + "a GET miss must carry %s so a proxy 404 is not read as a miss", blobhttp.MissHeader) + }) +} + +// missOnlyStore is a Store whose every lookup misses. The embedded nil Store panics if any other +// method is called, which is deliberate: this fixture is valid only for the read/exists paths the +// miss-header test drives. +type missOnlyStore struct{ Store } + +func (missOnlyStore) Exists(context.Context, []byte, fileformat.FileType, ...options.FileOption) (bool, error) { + return false, nil +} + +func (missOnlyStore) GetIoReader(context.Context, []byte, fileformat.FileType, ...options.FileOption) (io.ReadCloser, error) { + return nil, errors.ErrNotFound +} + // TestHandleRangeRequest_FileStoreReaderIsSeekable is a guard against the // regression where semaphoreReadCloser embeds io.ReadCloser (no Seek method), // so dataReader.(io.Seeker) fails the type assertion in handleRangeRequest