Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions stores/blob/http/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -143,6 +154,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
Expand All @@ -159,8 +174,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
}

Expand Down Expand Up @@ -227,6 +243,24 @@ 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the header proves a blob server answered, not the right one. A base URL pointed at another Teranode blob server (e.g. the block store instead of the subtree store) still gets X-Blob-Miss on every key and reads as empty, even with requireMissHeader=true. Worth softening "tells it from a wrong base URL" here and in blob_settings.md.

// answering 404.
if resp.Header.Get(MissHeader) != "" {
return false, nil
}

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

if s.requireMissHeader {
// 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.
return false, nil
Comment thread
icellan marked this conversation as resolved.
Resolved
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
Expand Down Expand Up @@ -309,14 +343,23 @@ func (s *HTTPStore) GetIoReader(ctx context.Context, key []byte, fileType filefo
case http.StatusOK:
return resp.Body, nil
case http.StatusNotFound:
miss := isBlobServerMiss(resp)
// Prefer the server's miss header, set on every miss path; fall back to the
// NotFoundMsg body for a server that predates it.
miss := resp.Header.Get(MissHeader) != ""
Comment thread
icellan marked this conversation as resolved.
Resolved
if miss {
// The header already decided it. Drain the small 404 body (bounded) so the
// transport can reuse the connection, as the body-check path below does.
_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, notFoundBodyLimit))
} else {
miss = isBlobServerMiss(resp)
}
resp.Body.Close()

if miss {
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:
Expand Down
81 changes: 81 additions & 0 deletions stores/blob/http/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,87 @@ 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)
})
}

// 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
Expand Down
7 changes: 7 additions & 0 deletions stores/blob/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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
}
Expand Down
43 changes: 43 additions & 0 deletions stores/blob/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package blob
import (
"bytes"
"context"
"encoding/base64"
"fmt"
"io"
"math"
Expand Down Expand Up @@ -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
Expand Down