fix(storage): sanitize storage locks (#1003)

removed all locks from common code
removed locks from GetBlobContent

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>
This commit is contained in:
peusebiu
2022-11-22 20:13:08 +02:00
committed by GitHub
parent 6ee3d802e7
commit 49c3d05706
8 changed files with 220 additions and 101 deletions
+5 -26
View File
@@ -6,7 +6,6 @@ import (
"os"
"path"
"strings"
"time"
"github.com/docker/distribution/registry/storage/driver"
"github.com/notaryproject/notation-go"
@@ -134,28 +133,24 @@ func validateOCIManifest(imgStore ImageStore, repo, reference string, manifest *
// validate image config
config := manifest.Config
blobFile, _, err := imgStore.GetBlob(repo, config.Digest, "")
blobBuf, err := imgStore.GetBlobContent(repo, config.Digest)
if err != nil {
return config.Digest, zerr.ErrBlobNotFound
}
defer blobFile.Close()
dec := json.NewDecoder(blobFile)
var cspec ispec.Image
if err := dec.Decode(&cspec); err != nil {
err = json.Unmarshal(blobBuf, &cspec)
if err != nil {
return "", zerr.ErrBadManifest
}
// validate the layers
for _, l := range manifest.Layers {
blobFile, _, err := imgStore.GetBlob(repo, l.Digest, "")
_, err := imgStore.GetBlobContent(repo, l.Digest)
if err != nil {
return l.Digest, zerr.ErrBlobNotFound
}
defer blobFile.Close()
}
return "", nil
@@ -461,8 +456,6 @@ func ApplyLinter(imgStore ImageStore, linter Lint, repo string, manifestDesc isp
func GetOrasReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest, artifactType string,
log zerolog.Logger,
) ([]oras.Descriptor, error) {
var lockLatency time.Time
if err := gdigest.Validate(); err != nil {
return nil, err
}
@@ -477,9 +470,6 @@ func GetOrasReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest,
return nil, err
}
imgStore.RLock(&lockLatency)
defer imgStore.RUnlock(&lockLatency)
found := false
result := []oras.Descriptor{}
@@ -489,10 +479,7 @@ func GetOrasReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest,
continue
}
imgStore.RUnlock(&lockLatency)
buf, err := imgStore.GetBlobContent(repo, manifest.Digest)
imgStore.RLock(&lockLatency)
if err != nil {
log.Error().Err(err).Str("blob", imgStore.BlobPath(repo, manifest.Digest)).Msg("failed to read manifest")
@@ -540,8 +527,6 @@ func GetOrasReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest,
func GetReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest, artifactType string,
log zerolog.Logger,
) (ispec.Index, error) {
var lockLatency time.Time
nilIndex := ispec.Index{}
if err := gdigest.Validate(); err != nil {
@@ -558,9 +543,6 @@ func GetReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest, art
return nilIndex, err
}
imgStore.RLock(&lockLatency)
defer imgStore.RUnlock(&lockLatency)
found := false
result := []ispec.Descriptor{}
@@ -570,10 +552,7 @@ func GetReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest, art
continue
}
imgStore.RUnlock(&lockLatency)
buf, err := imgStore.GetBlobContent(repo, manifest.Digest)
imgStore.RLock(&lockLatency)
if err != nil {
log.Error().Err(err).Str("blob", imgStore.BlobPath(repo, manifest.Digest)).Msg("failed to read manifest")
+38 -37
View File
@@ -1,7 +1,6 @@
package local
import (
"bytes"
"context"
"encoding/json"
"errors"
@@ -378,11 +377,16 @@ func (is *ImageStoreLocal) GetNextRepository(repo string) (string, error) {
// GetImageTags returns a list of image tags available in the specified repository.
func (is *ImageStoreLocal) GetImageTags(repo string) ([]string, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
if !is.DirExists(dir) {
return nil, zerr.ErrRepoNotFound
}
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return nil, err
@@ -393,11 +397,16 @@ func (is *ImageStoreLocal) GetImageTags(repo string) ([]string, error) {
// GetImageManifest returns the image manifest of an image in the specific repository.
func (is *ImageStoreLocal) GetImageManifest(repo, reference string) ([]byte, godigest.Digest, string, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
if !is.DirExists(dir) {
return nil, "", "", zerr.ErrRepoNotFound
}
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return nil, "", "", err
@@ -439,6 +448,11 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, /
return "", err
}
var lockLatency time.Time
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
digest, err := storage.ValidateManifest(is, repo, reference, mediaType, body, is.log)
if err != nil {
return digest, err
@@ -478,11 +492,6 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, /
return desc.Digest, nil
}
var lockLatency time.Time
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
// write manifest to "blobs"
dir := path.Join(is.rootDir, repo, "blobs", mDigest.Algorithm().String())
_ = ensureDir(dir, is.log)
@@ -495,15 +504,11 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, /
return "", err
}
is.Unlock(&lockLatency)
err = storage.UpdateIndexWithPrunedImageManifests(is, &index, repo, desc, oldDgst, is.log)
if err != nil {
return "", err
}
is.Lock(&lockLatency)
// now update "index.json"
index.Manifests = append(index.Manifests, desc)
dir = path.Join(is.rootDir, repo)
@@ -516,13 +521,8 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, /
return "", err
}
is.Unlock(&lockLatency)
// apply linter only on images, not signatures
pass, err := storage.ApplyLinter(is, is.linter, repo, desc)
is.Lock(&lockLatency)
if !pass {
is.log.Error().Err(err).Str("repo", repo).Str("reference", reference).Msg("linter didn't pass")
@@ -557,6 +557,9 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCol
return zerr.ErrRepoNotFound
}
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return err
@@ -576,9 +579,6 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCol
return err
}
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
// now update "index.json"
dir = path.Join(is.rootDir, repo)
file := path.Join(dir, "index.json")
@@ -1258,37 +1258,30 @@ func (is *ImageStoreLocal) GetBlob(repo string, digest godigest.Digest, mediaTyp
return blobReadCloser, binfo.Size(), nil
}
// GetBlobContent returns blob contents, SHOULD lock from outside.
func (is *ImageStoreLocal) GetBlobContent(repo string, digest godigest.Digest) ([]byte, error) {
if err := digest.Validate(); err != nil {
return []byte{}, err
}
blob, _, err := is.GetBlob(repo, digest, ispec.MediaTypeImageManifest)
if err != nil {
return []byte{}, err
}
defer blob.Close()
blobPath := is.BlobPath(repo, digest)
buf := new(bytes.Buffer)
blob, err := os.ReadFile(blobPath)
if os.IsNotExist(err) {
is.log.Error().Err(err).Str("blob", blobPath).Msg("blob doesn't exist")
_, err = buf.ReadFrom(blob)
if err != nil {
is.log.Error().Err(err).Str("digest", digest.String()).Msg("failed to read blob")
return []byte{}, err
return []byte{}, zerr.ErrBlobNotFound
}
return buf.Bytes(), nil
is.log.Error().Err(err).Str("blob", blobPath).Msg("failed to read blob")
return blob, nil
}
// GetIndexContent returns index.json contents, SHOULD lock from outside.
func (is *ImageStoreLocal) GetIndexContent(repo string) ([]byte, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
buf, err := os.ReadFile(path.Join(dir, "index.json"))
if err != nil {
if os.IsNotExist(err) {
@@ -1345,11 +1338,21 @@ func (is *ImageStoreLocal) DeleteBlob(repo string, digest godigest.Digest) error
func (is *ImageStoreLocal) GetReferrers(repo string, gdigest godigest.Digest, artifactType string,
) (ispec.Index, error) {
var lockLatency time.Time
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
return storage.GetReferrers(is, repo, gdigest, artifactType, is.log)
}
func (is *ImageStoreLocal) GetOrasReferrers(repo string, gdigest godigest.Digest, artifactType string,
) ([]oras.Descriptor, error) {
var lockLatency time.Time
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
return storage.GetOrasReferrers(is, repo, gdigest, artifactType, is.log)
}
@@ -1483,9 +1486,7 @@ func (is *ImageStoreLocal) gcRepo(repo string) error {
var lockLatency time.Time
is.Lock(&lockLatency)
err := is.garbageCollect(dir, repo)
is.Unlock(&lockLatency)
if err != nil {
+80 -37
View File
@@ -284,11 +284,16 @@ func (is *ObjectStorage) GetNextRepository(repo string) (string, error) {
// GetImageTags returns a list of image tags available in the specified repository.
func (is *ObjectStorage) GetImageTags(repo string) ([]string, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
if fi, err := is.store.Stat(context.Background(), dir); err != nil || !fi.IsDir() {
return nil, zerr.ErrRepoNotFound
}
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return nil, err
@@ -299,11 +304,16 @@ func (is *ObjectStorage) GetImageTags(repo string) ([]string, error) {
// GetImageManifest returns the image manifest of an image in the specific repository.
func (is *ObjectStorage) GetImageManifest(repo, reference string) ([]byte, godigest.Digest, string, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
if fi, err := is.store.Stat(context.Background(), dir); err != nil || !fi.IsDir() {
return nil, "", "", zerr.ErrRepoNotFound
}
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return nil, "", "", zerr.ErrRepoNotFound
@@ -345,6 +355,11 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n
return "", err
}
var lockLatency time.Time
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
dig, err := storage.ValidateManifest(is, repo, reference, mediaType, body, is.log)
if err != nil {
return dig, err
@@ -384,11 +399,6 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n
return desc.Digest, nil
}
var lockLatency time.Time
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
// write manifest to "blobs"
dir := path.Join(is.rootDir, repo, "blobs", mDigest.Algorithm().String())
manifestPath := path.Join(dir, mDigest.Encoded())
@@ -399,10 +409,7 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n
return "", err
}
is.Unlock(&lockLatency)
err = storage.UpdateIndexWithPrunedImageManifests(is, &index, repo, desc, oldDgst, is.log)
is.Lock(&lockLatency)
if err != nil {
return "", err
}
@@ -419,10 +426,8 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n
return "", err
}
is.Unlock(&lockLatency)
// apply linter only on images, not signatures
pass, err := storage.ApplyLinter(is, is.linter, repo, desc)
is.Lock(&lockLatency)
if !pass {
is.log.Error().Err(err).Str("repo", repo).Str("reference", reference).Msg("linter didn't pass")
@@ -450,6 +455,9 @@ func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectColli
return zerr.ErrRepoNotFound
}
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
index, err := storage.GetIndex(is, repo, is.log)
if err != nil {
return err
@@ -469,9 +477,6 @@ func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectColli
return err
}
is.Lock(&lockLatency)
defer is.Unlock(&lockLatency)
// now update "index.json"
dir = path.Join(is.rootDir, repo)
file := path.Join(dir, "index.json")
@@ -686,8 +691,6 @@ func (is *ObjectStorage) FinishBlobUpload(repo, uuid string, body io.Reader, dst
return zerr.ErrBadBlobDigest
}
defer fileWriter.Close()
if err := fileWriter.Commit(); err != nil {
is.log.Error().Err(err).Msg("failed to commit file")
@@ -696,6 +699,8 @@ func (is *ObjectStorage) FinishBlobUpload(repo, uuid string, body io.Reader, dst
if err := fileWriter.Close(); err != nil {
is.log.Error().Err(err).Msg("failed to close file")
return err
}
fileReader, err := is.store.Reader(context.Background(), src, 0)
@@ -705,6 +710,8 @@ func (is *ObjectStorage) FinishBlobUpload(repo, uuid string, body io.Reader, dst
return zerr.ErrUploadNotFound
}
defer fileReader.Close()
srcDigest, err := godigest.FromReader(fileReader)
if err != nil {
is.log.Error().Err(err).Str("blob", src).Msg("failed to open blob")
@@ -719,8 +726,6 @@ func (is *ObjectStorage) FinishBlobUpload(repo, uuid string, body io.Reader, dst
return zerr.ErrBadBlobDigest
}
fileReader.Close()
dst := is.BlobPath(repo, dstDigest)
var lockLatency time.Time
@@ -1209,48 +1214,86 @@ func (is *ObjectStorage) GetBlob(repo string, digest godigest.Digest, mediaType
return blobReadCloser, binfo.Size(), nil
}
// GetBlobContent returns blob contents, SHOULD lock from outside.
func (is *ObjectStorage) GetBlobContent(repo string, digest godigest.Digest) ([]byte, error) {
blob, _, err := is.GetBlob(repo, digest, ispec.MediaTypeImageManifest)
if err != nil {
return []byte{}, err
}
defer blob.Close()
buf := new(bytes.Buffer)
_, err = buf.ReadFrom(blob)
if err != nil {
is.log.Error().Err(err).Msg("failed to read blob")
if err := digest.Validate(); err != nil {
return []byte{}, err
}
return buf.Bytes(), nil
blobPath := is.BlobPath(repo, digest)
binfo, err := is.store.Stat(context.Background(), blobPath)
if err != nil {
is.log.Error().Err(err).Str("blob", blobPath).Msg("failed to stat blob")
return []byte{}, zerr.ErrBlobNotFound
}
blobBuf, err := is.store.GetContent(context.Background(), blobPath)
if err != nil {
is.log.Error().Err(err).Str("blob", blobPath).Msg("failed to open blob")
return nil, err
}
// is a 'deduped' blob?
if binfo.Size() == 0 && fmt.Sprintf("%v", is.cache) != fmt.Sprintf("%v", nil) {
// Check blobs in cache
dstRecord, err := is.checkCacheBlob(digest)
if err != nil {
is.log.Error().Err(err).Str("digest", digest.String()).Msg("cache: not found")
return nil, zerr.ErrBlobNotFound
}
blobBuf, err := is.store.GetContent(context.Background(), dstRecord)
if err != nil {
is.log.Error().Err(err).Str("blob", dstRecord).Msg("failed to open blob")
return nil, err
}
return blobBuf, nil
}
return blobBuf, nil
}
func (is *ObjectStorage) GetReferrers(repo string, gdigest godigest.Digest, artifactType string,
) (ispec.Index, error) {
var lockLatency time.Time
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
return storage.GetReferrers(is, repo, gdigest, artifactType, is.log)
}
func (is *ObjectStorage) GetOrasReferrers(repo string, gdigest godigest.Digest, artifactType string,
) ([]artifactspec.Descriptor, error) {
return storage.GetOrasReferrers(is, repo, gdigest, artifactType, is.log)
}
func (is *ObjectStorage) GetIndexContent(repo string) ([]byte, error) {
var lockLatency time.Time
dir := path.Join(is.rootDir, repo)
is.RLock(&lockLatency)
defer is.RUnlock(&lockLatency)
return storage.GetOrasReferrers(is, repo, gdigest, artifactType, is.log)
}
// GetIndexContent returns index.json contents, SHOULD lock from outside.
func (is *ObjectStorage) GetIndexContent(repo string) ([]byte, error) {
dir := path.Join(is.rootDir, repo)
buf, err := is.store.GetContent(context.Background(), path.Join(dir, "index.json"))
if err != nil {
if errors.Is(err, driver.PathNotFoundError{}) {
is.log.Error().Err(err).Str("dir", dir).Msg("index.json doesn't exist")
return []byte{}, zerr.ErrRepoNotFound
}
is.log.Error().Err(err).Str("dir", dir).Msg("failed to read index.json")
return []byte{}, zerr.ErrRepoNotFound
return []byte{}, err
}
return buf, nil
+51
View File
@@ -891,6 +891,16 @@ func TestNegativeCasesObjectsStorage(t *testing.T) {
So(err, ShouldNotBeNil)
})
Convey("Test GetIndexContent", func(c C) {
imgStore = createMockStorage(testDir, tdir, false, &StorageDriverMock{
GetContentFn: func(ctx context.Context, path string) ([]byte, error) {
return []byte{}, driver.PathNotFoundError{}
},
})
_, err := imgStore.GetIndexContent(testImage)
So(err, ShouldNotBeNil)
})
Convey("Test DeleteImageManifest2", func(c C) {
imgStore = createMockStorage(testDir, tdir, false, &StorageDriverMock{})
err := imgStore.DeleteImageManifest(testImage, "1.0", false)
@@ -1092,6 +1102,18 @@ func TestNegativeCasesObjectsStorage(t *testing.T) {
So(err, ShouldNotBeNil)
})
Convey("Test GetBlobContent", func(c C) {
imgStore = createMockStorage(testDir, tdir, false, &StorageDriverMock{
GetContentFn: func(ctx context.Context, path string) ([]byte, error) {
return []byte{}, errS3
},
})
d := godigest.FromBytes([]byte(""))
_, err := imgStore.GetBlobContent(testImage, d)
So(err, ShouldNotBeNil)
})
Convey("Test DeleteBlob", func(c C) {
imgStore = createMockStorage(testDir, tdir, false, &StorageDriverMock{
DeleteFn: func(ctx context.Context, path string) error {
@@ -1233,6 +1255,14 @@ func TestS3Dedupe(t *testing.T) {
err = blobReadCloser.Close()
So(err, ShouldBeNil)
blobContent, err := imgStore.GetBlobContent("dedupe2", digest)
So(err, ShouldBeNil)
So(len(blobContent), ShouldBeGreaterThan, 0)
So(checkBlobSize1, ShouldEqual, len(blobContent))
So(getBlobSize1, ShouldEqual, len(blobContent))
err = blobReadCloser.Close()
So(err, ShouldBeNil)
cblob, cdigest = test.GetRandomImageConfig()
_, clen, err = imgStore.FullBlobUpload("dedupe2", bytes.NewReader(cblob), cdigest)
So(err, ShouldBeNil)
@@ -1359,6 +1389,10 @@ func TestS3Dedupe(t *testing.T) {
So(err, ShouldBeNil)
So(getBlobSize1, ShouldEqual, getBlobSize3)
blobContent, err := imgStore.GetBlobContent("dedupe3", digest)
So(err, ShouldBeNil)
So(getBlobSize1, ShouldEqual, len(blobContent))
_, checkBlobSize3, err := imgStore.CheckBlob("dedupe3", digest)
So(err, ShouldBeNil)
So(checkBlobSize3, ShouldBeGreaterThan, 0)
@@ -2261,6 +2295,9 @@ func TestS3DedupeErr(t *testing.T) {
_, _, err = imgStore.GetBlob("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip")
So(err, ShouldNotBeNil)
_, err = imgStore.GetBlobContent("repo2", digest)
So(err, ShouldNotBeNil)
_, _, _, err = imgStore.GetBlobPartial("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip", 0, 1)
So(err, ShouldNotBeNil)
})
@@ -2303,11 +2340,22 @@ func TestS3DedupeErr(t *testing.T) {
return io.NopCloser(strings.NewReader("")), nil
},
GetContentFn: func(ctx context.Context, path string) ([]byte, error) {
if strings.Contains(path, "repo1/dst1") {
return []byte{}, errS3
}
return []byte{}, nil
},
})
_, _, err = imgStore.GetBlob("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip")
So(err, ShouldNotBeNil)
_, err = imgStore.GetBlobContent("repo2", digest)
So(err, ShouldNotBeNil)
_, _, _, err = imgStore.GetBlobPartial("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip", 0, 1)
So(err, ShouldNotBeNil)
})
@@ -2331,6 +2379,9 @@ func TestS3DedupeErr(t *testing.T) {
_, _, err = imgStore.GetBlob("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip")
So(err, ShouldNotBeNil)
_, err = imgStore.GetBlobContent("repo2", digest)
So(err, ShouldNotBeNil)
_, _, _, err = imgStore.GetBlobPartial("repo2", digest, "application/vnd.oci.image.layer.v1.tar+gzip", 0, 1)
So(err, ShouldNotBeNil)
})
+13
View File
@@ -7,6 +7,7 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"os"
"path"
"strings"
@@ -257,6 +258,18 @@ func TestStorageAPIs(t *testing.T) {
blob, _, err := imgStore.GetBlob("test", digest, "application/vnd.oci.image.layer.v1.tar+gzip")
So(err, ShouldBeNil)
blobBuf := new(strings.Builder)
n, err := io.Copy(blobBuf, blob)
// check errors
So(n, ShouldEqual, buflen)
So(err, ShouldBeNil)
So(blobBuf.String(), ShouldEqual, buf.String())
blobContent, err := imgStore.GetBlobContent("test", digest)
So(err, ShouldBeNil)
So(blobContent, ShouldResemble, content)
err = blob.Close()
So(err, ShouldBeNil)