Skip to content

Commit 886304e

Browse files
top-oaitop-oai
authored
Leave volatile qualifiers out of asset store digest (#61)
* Leave volatile qualifiers out of asset store digest * fix style
1 parent 4a9f8ac commit 886304e

File tree

2 files changed

+228
-4
lines changed

2 files changed

+228
-4
lines changed

‎pkg/fetch/caching_fetcher.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"strings"
78
"time"
89

910
"github.com/buildbarn/bb-remote-asset/pkg/proto/asset"
@@ -50,7 +51,7 @@ func (cf *cachingFetcher) FetchBlob(ctx context.Context, req *remoteasset.FetchB
5051

5152
// Check assetStore
5253
for _, uri := range req.Uris {
53-
assetData, err := getAndCheckAsset(ctx, cf.assetStore, uri, req.Qualifiers, digestFunction, oldestContentAccepted)
54+
assetData, err := getAndCheckAsset(ctx, cf.assetStore, uri, removeVolatileQualifiers(req.Qualifiers), digestFunction, oldestContentAccepted)
5455
if err != nil {
5556
allCachingErrors = append(allCachingErrors, err)
5657
continue
@@ -82,7 +83,7 @@ func (cf *cachingFetcher) FetchBlob(ctx context.Context, req *remoteasset.FetchB
8283
}
8384

8485
// Cache fetched blob with single URI
85-
assetRef := storage.NewAssetReference([]string{response.Uri}, response.Qualifiers)
86+
assetRef := storage.NewAssetReference([]string{response.Uri}, removeVolatileQualifiers(response.Qualifiers))
8687
assetData := storage.NewBlobAsset(response.BlobDigest, getDefaultTimestamp())
8788
err = cf.assetStore.Put(ctx, assetRef, assetData, digestFunction)
8889
if err != nil {
@@ -133,6 +134,21 @@ func getAndCheckAsset(
133134
return assetData, nil
134135
}
135136

137+
func removeVolatileQualifiers(qualifiers []*remoteasset.Qualifier) []*remoteasset.Qualifier {
138+
// Remove qualifiers that are volatile, like auth headers which may change frequently.
139+
var stableQualifiers []*remoteasset.Qualifier
140+
for _, qualifier := range qualifiers {
141+
if strings.HasPrefix(qualifier.Name, "http_header_url:") {
142+
continue
143+
}
144+
if qualifier.Name == "bazel.auth_headers" {
145+
continue
146+
}
147+
stableQualifiers = append(stableQualifiers, qualifier)
148+
}
149+
return stableQualifiers
150+
}
151+
136152
func (cf *cachingFetcher) FetchDirectory(ctx context.Context, req *remoteasset.FetchDirectoryRequest) (*remoteasset.FetchDirectoryResponse, error) {
137153
digestFunction, err := getDigestFunction(req.DigestFunction, req.InstanceName)
138154
if err != nil {
@@ -148,7 +164,7 @@ func (cf *cachingFetcher) FetchDirectory(ctx context.Context, req *remoteasset.F
148164

149165
// Check refStore
150166
for _, uri := range req.Uris {
151-
assetData, err := getAndCheckAsset(ctx, cf.assetStore, uri, req.Qualifiers, digestFunction, oldestContentAccepted)
167+
assetData, err := getAndCheckAsset(ctx, cf.assetStore, uri, removeVolatileQualifiers(req.Qualifiers), digestFunction, oldestContentAccepted)
152168
if err != nil {
153169
allCachingErrors = append(allCachingErrors, err)
154170
continue
@@ -177,7 +193,7 @@ func (cf *cachingFetcher) FetchDirectory(ctx context.Context, req *remoteasset.F
177193
}
178194

179195
// Cache fetched blob with single URI
180-
assetRef := storage.NewAssetReference([]string{response.Uri}, response.Qualifiers)
196+
assetRef := storage.NewAssetReference([]string{response.Uri}, removeVolatileQualifiers(response.Qualifiers))
181197
assetData := storage.NewDirectoryAsset(response.RootDirectoryDigest, getDefaultTimestamp())
182198
err = cf.assetStore.Put(ctx, assetRef, assetData, digestFunction)
183199
if err != nil {

‎pkg/fetch/caching_fetcher_test.go‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,3 +223,211 @@ func TestCachingFetcherOldestContentAccepted(t *testing.T) {
223223
require.Contains(t, errAsStatus.Message(), "Asset older than")
224224
require.Equal(t, errAsStatus.Code(), codes.NotFound)
225225
}
226+
227+
func TestFetchBlobVolatileQualifiersIgnored(t *testing.T) {
228+
ctrl, ctx := gomock.WithContext(context.Background(), t)
229+
230+
uri := "https://example.com/blob.tar.gz"
231+
232+
req1 := &remoteasset.FetchBlobRequest{
233+
InstanceName: "",
234+
Uris: []string{uri},
235+
Qualifiers: []*remoteasset.Qualifier{
236+
{Name: "checksum.sri", Value: "sha256-aaa"},
237+
{Name: "http_header_url:0:Authorization", Value: "Bearer first"},
238+
{Name: "bazel.auth_headers", Value: "token‑one"},
239+
},
240+
}
241+
242+
// 2nd request differs only in auth headers.
243+
req2 := proto.Clone(req1).(*remoteasset.FetchBlobRequest)
244+
req2.Qualifiers[1].Value = "Bearer second"
245+
req2.Qualifiers[2].Value = "token‑two"
246+
247+
// 3rd request differs in non-auth headers and should be a cache miss from the first.
248+
req3 := proto.Clone(req1).(*remoteasset.FetchBlobRequest)
249+
req3.Qualifiers[0].Value = "Windows"
250+
251+
blobDigest := &remoteexecution.Digest{
252+
Hash: "1111111111111111111111111111111111111111111111111111111111111111",
253+
SizeBytes: 42,
254+
}
255+
256+
backend := mock.NewMockBlobAccess(ctrl)
257+
assetStore := storage.NewBlobAccessAssetStore(backend, 16*1024*1024)
258+
mockFetcher := mock.NewMockFetcher(ctrl)
259+
cachingFetcher := fetch.NewCachingFetcher(mockFetcher, assetStore)
260+
261+
// 1st fetch is a cache miss, and we'll record the digest used.
262+
var firstDigest bb_digest.Digest
263+
getMiss := backend.
264+
EXPECT().
265+
Get(ctx, gomock.Any()).
266+
Do(func(_ context.Context, d bb_digest.Digest) {
267+
firstDigest = d
268+
}).
269+
Return(buffer.NewBufferFromError(status.Error(codes.NotFound, "miss")))
270+
mockFetcher.
271+
EXPECT().
272+
FetchBlob(ctx, req1).
273+
After(getMiss).
274+
Return(&remoteasset.FetchBlobResponse{
275+
Status: status.New(codes.OK, "fetched").Proto(),
276+
Uri: uri,
277+
BlobDigest: blobDigest,
278+
Qualifiers: req1.Qualifiers,
279+
}, nil)
280+
backend.
281+
EXPECT().
282+
Put(ctx, gomock.Any(), gomock.Any()).
283+
DoAndReturn(func(_ context.Context, d bb_digest.Digest, b buffer.Buffer) error {
284+
require.Equal(t, firstDigest, d)
285+
return nil
286+
})
287+
288+
_, err := cachingFetcher.FetchBlob(ctx, req1)
289+
require.NoError(t, err)
290+
291+
// 2nd fetch should be a cache hit, despite the auth qualifiers being different.
292+
backend.
293+
EXPECT().
294+
Get(ctx, firstDigest).
295+
Return(buffer.NewProtoBufferFromProto(storage.NewBlobAsset(blobDigest, nil), buffer.UserProvided))
296+
297+
_, err = cachingFetcher.FetchBlob(ctx, req2)
298+
require.NoError(t, err)
299+
300+
// 3rd fetch should be a cache miss since non-auth qualifiers differ.
301+
var thirdDigest bb_digest.Digest
302+
getMiss = backend.
303+
EXPECT().
304+
Get(ctx, gomock.Any()).
305+
Do(func(_ context.Context, d bb_digest.Digest) {
306+
require.NotEqual(t, firstDigest, d)
307+
thirdDigest = d
308+
}).
309+
Return(buffer.NewBufferFromError(status.Error(codes.NotFound, "miss")))
310+
mockFetcher.
311+
EXPECT().
312+
FetchBlob(ctx, req3).
313+
After(getMiss).
314+
Return(&remoteasset.FetchBlobResponse{
315+
Status: status.New(codes.OK, "fetched").Proto(),
316+
Uri: uri,
317+
BlobDigest: blobDigest,
318+
Qualifiers: req3.Qualifiers,
319+
}, nil)
320+
backend.
321+
EXPECT().
322+
Put(ctx, gomock.Any(), gomock.Any()).
323+
DoAndReturn(func(_ context.Context, d bb_digest.Digest, b buffer.Buffer) error {
324+
require.Equal(t, thirdDigest, d)
325+
return nil
326+
})
327+
_, err = cachingFetcher.FetchBlob(ctx, req3)
328+
require.NoError(t, err)
329+
}
330+
331+
func TestFetchDirectoryVolatileQualifiersIgnored(t *testing.T) {
332+
ctrl, ctx := gomock.WithContext(context.Background(), t)
333+
334+
uri := "https://example.com/dir.zip"
335+
336+
req1 := &remoteasset.FetchDirectoryRequest{
337+
InstanceName: "",
338+
Uris: []string{uri},
339+
Qualifiers: []*remoteasset.Qualifier{
340+
{Name: "checksum.sri", Value: "sha256-aaa"},
341+
{Name: "http_header_url:0:Authorization", Value: "application/zip"},
342+
{Name: "bazel.auth_headers", Value: "token‑A"},
343+
},
344+
}
345+
346+
// 2nd request differs only in auth headers.
347+
req2 := proto.Clone(req1).(*remoteasset.FetchDirectoryRequest)
348+
req2.Qualifiers[1].Value = "Bearer second"
349+
req2.Qualifiers[2].Value = "token‑two"
350+
351+
// 3rd request differs in non-auth headers and should be a cache miss from the first.
352+
req3 := proto.Clone(req1).(*remoteasset.FetchDirectoryRequest)
353+
req3.Qualifiers[0].Value = "Windows"
354+
355+
dirDigest := &remoteexecution.Digest{
356+
Hash: "2222222222222222222222222222222222222222222222222222222222222222",
357+
SizeBytes: 99,
358+
}
359+
360+
backend := mock.NewMockBlobAccess(ctrl)
361+
assetStore := storage.NewBlobAccessAssetStore(backend, 16*1024*1024)
362+
mockFetcher := mock.NewMockFetcher(ctrl)
363+
cachingFetcher := fetch.NewCachingFetcher(mockFetcher, assetStore)
364+
365+
// 1st fetch is a cache miss, and we'll record the digest used.
366+
var firstDigest bb_digest.Digest
367+
getMiss := backend.
368+
EXPECT().
369+
Get(ctx, gomock.Any()).
370+
Do(func(_ context.Context, d bb_digest.Digest) {
371+
firstDigest = d
372+
}).
373+
Return(buffer.NewBufferFromError(status.Error(codes.NotFound, "miss")))
374+
mockFetcher.
375+
EXPECT().
376+
FetchDirectory(ctx, req1).
377+
After(getMiss).
378+
Return(&remoteasset.FetchDirectoryResponse{
379+
Status: status.New(codes.OK, "fetched").Proto(),
380+
Uri: uri,
381+
RootDirectoryDigest: dirDigest,
382+
Qualifiers: req1.Qualifiers,
383+
}, nil)
384+
backend.
385+
EXPECT().
386+
Put(ctx, gomock.Any(), gomock.Any()).
387+
DoAndReturn(func(_ context.Context, d bb_digest.Digest, b buffer.Buffer) error {
388+
require.Equal(t, firstDigest, d)
389+
return nil
390+
})
391+
392+
_, err := cachingFetcher.FetchDirectory(ctx, req1)
393+
require.NoError(t, err)
394+
395+
// 2nd fetch should be a cache hit, despite the auth qualifiers being different.
396+
backend.
397+
EXPECT().
398+
Get(ctx, firstDigest).
399+
Return(buffer.NewProtoBufferFromProto(storage.NewBlobAsset(dirDigest, nil), buffer.UserProvided))
400+
401+
_, err = cachingFetcher.FetchDirectory(ctx, req2)
402+
require.NoError(t, err)
403+
404+
// 3rd fetch should be a cache miss since non-auth qualifiers differ.
405+
var thirdDigest bb_digest.Digest
406+
getMiss = backend.
407+
EXPECT().
408+
Get(ctx, gomock.Any()).
409+
Do(func(_ context.Context, d bb_digest.Digest) {
410+
require.NotEqual(t, firstDigest, d)
411+
thirdDigest = d
412+
}).
413+
Return(buffer.NewBufferFromError(status.Error(codes.NotFound, "miss")))
414+
mockFetcher.
415+
EXPECT().
416+
FetchDirectory(ctx, req3).
417+
After(getMiss).
418+
Return(&remoteasset.FetchDirectoryResponse{
419+
Status: status.New(codes.OK, "fetched").Proto(),
420+
Uri: uri,
421+
RootDirectoryDigest: dirDigest,
422+
Qualifiers: req3.Qualifiers,
423+
}, nil)
424+
backend.
425+
EXPECT().
426+
Put(ctx, gomock.Any(), gomock.Any()).
427+
DoAndReturn(func(_ context.Context, d bb_digest.Digest, b buffer.Buffer) error {
428+
require.Equal(t, thirdDigest, d)
429+
return nil
430+
})
431+
_, err = cachingFetcher.FetchDirectory(ctx, req3)
432+
require.NoError(t, err)
433+
}

0 commit comments

Comments
 (0)