diff --git a/pkg/uhttp/client.go b/pkg/uhttp/client.go index df9474efe..4cd8d7cf0 100644 --- a/pkg/uhttp/client.go +++ b/pkg/uhttp/client.go @@ -104,17 +104,48 @@ func NewClient(ctx context.Context, options ...Option) (*http.Client, error) { } type icache interface { - Get(req *http.Request) (*http.Response, error) - Set(req *http.Request, value *http.Response) error + Get(req *http.Request, opts ...CacheOption) (*http.Response, error) + Set(req *http.Request, value *http.Response, opts ...CacheOption) error Clear(ctx context.Context) error Stats(ctx context.Context) CacheStats } +type cacheKeyConfig struct { + headers []string +} + +// CacheOption configures how CreateCacheKey computes its key, beyond the +// default set of headers (Accept, Content-Type, Cookie, Range). Kept as an +// interface so future dimensions (TTL, query-param keying, etc.) can be +// added without changing CreateCacheKey's or icache's signatures again. +type CacheOption interface { + applyCache(*cacheKeyConfig) +} + +type cacheKeyHeadersOption []string + +func (o cacheKeyHeadersOption) applyCache(c *cacheKeyConfig) { + c.headers = append(c.headers, o...) +} + +// WithCacheKeyHeaders returns a CacheOption that folds the named headers +// into the HTTP response cache key, on top of the default set (Accept, +// Content-Type, Cookie, Range). The value folded in for each is read from +// req.Header at CreateCacheKey time, same as the default set. +func WithCacheKeyHeaders(headers ...string) CacheOption { + return cacheKeyHeadersOption(headers) +} + // CreateCacheKey generates a cache key based on the request URL, query parameters, and headers. -func CreateCacheKey(req *http.Request) (string, error) { +func CreateCacheKey(req *http.Request, opts ...CacheOption) (string, error) { if req == nil { return "", fmt.Errorf("request is nil") } + var cfg cacheKeyConfig + for _, o := range opts { + o.applyCache(&cfg) + } + var sortedParams []string // Normalize the URL path path := strings.ToLower(req.URL.Path) @@ -137,6 +168,12 @@ func CreateCacheKey(req *http.Request) (string, error) { } } } + for _, h := range cfg.headers { + key := http.CanonicalHeaderKey(h) + for _, value := range req.Header[key] { + headerParts = append(headerParts, fmt.Sprintf("%s=%s", key, value)) + } + } sort.Strings(headerParts) headersString := strings.Join(headerParts, "&") diff --git a/pkg/uhttp/client_test.go b/pkg/uhttp/client_test.go new file mode 100644 index 000000000..bdffa07b0 --- /dev/null +++ b/pkg/uhttp/client_test.go @@ -0,0 +1,120 @@ +package uhttp + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func newCacheKeyRequest(t *testing.T, headerKey, headerValue string) *http.Request { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://example.com/widgets?id=1", nil) + require.NoError(t, err) + if headerKey != "" { + req.Header.Set(headerKey, headerValue) + } + return req +} + +func TestCreateCacheKey_NilRequest(t *testing.T) { + _, err := CreateCacheKey(nil) + require.Error(t, err) +} + +func TestCreateCacheKey_IdenticalRequestsMatch(t *testing.T) { + req1 := newCacheKeyRequest(t, "Accept", "application/json") + req2 := newCacheKeyRequest(t, "Accept", "application/json") + + key1, err := CreateCacheKey(req1) + require.NoError(t, err) + key2, err := CreateCacheKey(req2) + require.NoError(t, err) + require.Equal(t, key1, key2) +} + +// TestCreateCacheKey_HeadersOutsideDefaultSetAreIgnoredByDefault documents +// current, intentional behavior: only the default set affects the key +// unless a caller opts in via extraCacheKeyHeaders. Folding in every header +// unconditionally would key the cache on values that have nothing to do +// with the response (transport-injected headers, tracing IDs, etc.) and +// silently tank the hit rate for every caller who never asked for that. +func TestCreateCacheKey_HeadersOutsideDefaultSetAreIgnoredByDefault(t *testing.T) { + headers := []string{"Authorization", "X-Api-Version", "X-Tenant-Id", "User-Agent"} + for _, header := range headers { + t.Run(header, func(t *testing.T) { + reqA := newCacheKeyRequest(t, header, "value-a") + reqB := newCacheKeyRequest(t, header, "value-b") + + keyA, err := CreateCacheKey(reqA) + require.NoError(t, err) + keyB, err := CreateCacheKey(reqB) + require.NoError(t, err) + require.Equal(t, keyA, keyB, "%s is not in the default set and must not affect the key", header) + }) + } +} + +func TestCreateCacheKey_DefaultHeadersStillChangeKey(t *testing.T) { + headers := []string{"Accept", "Content-Type", "Cookie", "Range"} + for _, header := range headers { + t.Run(header, func(t *testing.T) { + reqA := newCacheKeyRequest(t, header, "value-a") + reqB := newCacheKeyRequest(t, header, "value-b") + + keyA, err := CreateCacheKey(reqA) + require.NoError(t, err) + keyB, err := CreateCacheKey(reqB) + require.NoError(t, err) + require.NotEqual(t, keyA, keyB) + }) + } +} + +// TestCreateCacheKey_WithCacheKeyHeadersOptsInAdditionalHeaders is the +// regression test for CE-1056: a caller that knows a header varies the +// response (e.g. Authorization scoping the result set) can now opt that +// header into the key instead of two requests silently colliding. The value +// folded into the key is read from req.Header, same as the default set. +func TestCreateCacheKey_WithCacheKeyHeadersOptsInAdditionalHeaders(t *testing.T) { + reqA := newCacheKeyRequest(t, "Authorization", "value-a") + reqB := newCacheKeyRequest(t, "Authorization", "value-b") + + keyA, err := CreateCacheKey(reqA, WithCacheKeyHeaders("Authorization")) + require.NoError(t, err) + keyB, err := CreateCacheKey(reqB, WithCacheKeyHeaders("Authorization")) + require.NoError(t, err) + require.NotEqual(t, keyA, keyB) +} + +// TestCreateCacheKey_WithCacheKeyHeadersOnlyAffectsNamedHeaders confirms +// opting a header in doesn't widen the key to every header on the request -- +// a header present on req.Header but absent from the CacheOption still falls +// back to the default-set rule. +func TestCreateCacheKey_WithCacheKeyHeadersOnlyAffectsNamedHeaders(t *testing.T) { + reqA := newCacheKeyRequest(t, "X-Tenant-Id", "tenant-a") + reqA.Header.Set("Authorization", "same-token") + reqB := newCacheKeyRequest(t, "X-Tenant-Id", "tenant-b") + reqB.Header.Set("Authorization", "same-token") + + extra := WithCacheKeyHeaders("Authorization") + keyA, err := CreateCacheKey(reqA, extra) + require.NoError(t, err) + keyB, err := CreateCacheKey(reqB, extra) + require.NoError(t, err) + require.Equal(t, keyA, keyB, "X-Tenant-Id was never opted in, so it must not affect the key") +} + +// TestCreateCacheKey_WithCacheKeyHeadersCanonicalizesNames confirms header +// names passed to WithCacheKeyHeaders are treated the same regardless of +// casing. +func TestCreateCacheKey_WithCacheKeyHeadersCanonicalizesNames(t *testing.T) { + req := newCacheKeyRequest(t, "Authorization", "value-a") + + keyA, err := CreateCacheKey(req, WithCacheKeyHeaders("authorization")) + require.NoError(t, err) + keyB, err := CreateCacheKey(req, WithCacheKeyHeaders("Authorization")) + require.NoError(t, err) + require.Equal(t, keyA, keyB) +} diff --git a/pkg/uhttp/dbcache.go b/pkg/uhttp/dbcache.go index 1eeba0615..860af5cfd 100644 --- a/pkg/uhttp/dbcache.go +++ b/pkg/uhttp/dbcache.go @@ -190,12 +190,12 @@ func (d *DBCache) removeDB(ctx context.Context) error { } // Get returns cached response (if exists). -func (d *DBCache) Get(req *http.Request) (*http.Response, error) { +func (d *DBCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) { var ( isFound = false resp *http.Response ) - key, err := CreateCacheKey(req) + key, err := CreateCacheKey(req, opts...) if err != nil { return nil, err } @@ -250,8 +250,8 @@ func (d *DBCache) pick(ctx context.Context, key string) ([]byte, error) { } // Set stores and save response in the db. -func (d *DBCache) Set(req *http.Request, value *http.Response) error { - key, err := CreateCacheKey(req) +func (d *DBCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error { + key, err := CreateCacheKey(req, opts...) if err != nil { return err } diff --git a/pkg/uhttp/gocache.go b/pkg/uhttp/gocache.go index 81b8a8e0f..718e63c70 100644 --- a/pkg/uhttp/gocache.go +++ b/pkg/uhttp/gocache.go @@ -58,13 +58,13 @@ func NewNoopCache(ctx context.Context) *NoopCache { return &NoopCache{} } -func (g *NoopCache) Get(req *http.Request) (*http.Response, error) { +func (g *NoopCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) { // This isn't threadsafe but who cares? It's the noop cache. g.counter++ return nil, nil } -func (n *NoopCache) Set(req *http.Request, value *http.Response) error { +func (n *NoopCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error { return nil } @@ -219,12 +219,12 @@ func (g *GoCache) Stats(ctx context.Context) CacheStats { } } -func (g *GoCache) Get(req *http.Request) (*http.Response, error) { +func (g *GoCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) { if g.rootLibrary == nil { return nil, nil } - key, err := CreateCacheKey(req) + key, err := CreateCacheKey(req, opts...) if err != nil { return nil, err } @@ -247,12 +247,12 @@ func (g *GoCache) Get(req *http.Request) (*http.Response, error) { return resp, nil } -func (g *GoCache) Set(req *http.Request, value *http.Response) error { +func (g *GoCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error { if g.rootLibrary == nil { return nil } - key, err := CreateCacheKey(req) + key, err := CreateCacheKey(req, opts...) if err != nil { return err } diff --git a/pkg/uhttp/wrapper.go b/pkg/uhttp/wrapper.go index 6a5d13988..16351c121 100644 --- a/pkg/uhttp/wrapper.go +++ b/pkg/uhttp/wrapper.go @@ -436,6 +436,22 @@ func (c *BaseHttpClient) recordCacheMiss(ctx context.Context) { } func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Response, error) { + return c.do(req, nil, options...) +} + +// DoWithCacheKeyHeaders is a sibling to Do that additionally folds the named +// headers into the HTTP response cache key for this call, on top of the +// default set (Accept, Content-Type, Cookie, Range). Use this when a request +// varies by a header the cache wouldn't otherwise key on -- e.g. a per-call +// Authorization token -- so requests that only differ in that header don't +// collide. Each header's value is read from req.Header, same as the default +// set. Do's own signature is untouched -- zero compatibility risk for +// existing callers. +func (c *BaseHttpClient) DoWithCacheKeyHeaders(req *http.Request, cacheKeyHeaders []string, options ...DoOption) (*http.Response, error) { + return c.do(req, []CacheOption{WithCacheKeyHeaders(cacheKeyHeaders...)}, options...) +} + +func (c *BaseHttpClient) do(req *http.Request, cacheOpts []CacheOption, options ...DoOption) (*http.Response, error) { var ( err error resp *http.Response @@ -448,7 +464,7 @@ func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Respo } if req.Method == http.MethodGet && req.Header.Get("Cache-Control") != "no-cache" { - resp, err = c.baseHttpCache.Get(req) + resp, err = c.baseHttpCache.Get(req, cacheOpts...) if err != nil { return nil, err } @@ -520,7 +536,7 @@ func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Respo } if req.Method == http.MethodGet && resp.StatusCode == http.StatusOK { - cacheErr := c.baseHttpCache.Set(req, resp) + cacheErr := c.baseHttpCache.Set(req, resp, cacheOpts...) if cacheErr != nil { l.Warn("error setting cache", zap.String("url", req.URL.String()), zap.Error(cacheErr)) } diff --git a/pkg/uhttp/wrapper_test.go b/pkg/uhttp/wrapper_test.go index 7e570a3b4..cad3fc4f8 100644 --- a/pkg/uhttp/wrapper_test.go +++ b/pkg/uhttp/wrapper_test.go @@ -8,7 +8,9 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "net/url" + "sync/atomic" "testing" "time" @@ -727,3 +729,44 @@ func TestWrapper_RedactSensitiveHeaders(t *testing.T) { "Custom-Api-Key": {"REDACTED"}, }, redactedHeaders) } + +func newCountingServer(hits *int32) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(hits, 1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) +} + +// TestWrapper_DoWithCacheKeyHeaders_DistinguishesRequests is the regression +// test for CE-1056: without opting Authorization into the cache key via +// DoWithCacheKeyHeaders, two GET requests that only differ in that header +// would collide. With it named, they don't -- both reach the server -- while +// two calls sharing the same Authorization value still hit the cache on the +// second call. +func TestWrapper_DoWithCacheKeyHeaders_DistinguishesRequests(t *testing.T) { + var hits int32 + ts := newCountingServer(&hits) + defer ts.Close() + + client, err := NewBaseHttpClientWithContext(ctx, http.DefaultClient) + require.NoError(t, err) + + u, err := url.Parse(ts.URL) + require.NoError(t, err) + + doWithAuth := func(token string) { + req, err := client.NewRequest(ctx, http.MethodGet, u, WithBearerToken(token)) + require.NoError(t, err) + resp, err := client.DoWithCacheKeyHeaders(req, []string{"Authorization"}) + require.NoError(t, err) + resp.Body.Close() + } + + doWithAuth("token-a") + doWithAuth("token-b") + require.EqualValues(t, 2, atomic.LoadInt32(&hits), "different Authorization values must not collide in the cache") + + doWithAuth("token-a") + require.EqualValues(t, 2, atomic.LoadInt32(&hits), "repeating the same Authorization value should be served from cache") +}