diff --git a/modules/packages/content_store.go b/modules/packages/content_store.go index 7554c4900e3..d28c81e4c44 100644 --- a/modules/packages/content_store.go +++ b/modules/packages/content_store.go @@ -4,11 +4,14 @@ package packages import ( + "errors" "io" + "io/fs" "net/url" "path" "strings" + "gitea.dev/modules/optional" "gitea.dev/modules/setting" "gitea.dev/modules/storage" "gitea.dev/modules/util" @@ -40,11 +43,15 @@ func (s *ContentStore) GetServeDirectURL(key BlobHash256Key, filename, method st return s.store.ServeDirectURL(KeyToRelativePath(key), filename, method, reqParams) } -// FIXME: Workaround to be removed in v1.20 -// https://github.com/go-gitea/gitea/issues/19586 -func (s *ContentStore) Has(key BlobHash256Key) error { - _, err := s.store.Stat(KeyToRelativePath(key)) - return err +func (s *ContentStore) OptionalSize(key BlobHash256Key) (sz optional.Option[int64], _ error) { + st, err := s.store.Stat(KeyToRelativePath(key)) + if errors.Is(err, fs.ErrNotExist) { + return sz, nil + } + if err != nil { + return sz, err + } + return optional.Some(st.Size()), nil } // Save stores a package blob diff --git a/modules/storage/azureblob.go b/modules/storage/azureblob.go index 2981b48177c..9ff6d2ab2ed 100644 --- a/modules/storage/azureblob.go +++ b/modules/storage/azureblob.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "net/url" "os" @@ -108,7 +109,7 @@ func convertAzureBlobErr(err error) error { } if bloberror.HasCode(err, bloberror.BlobNotFound) { - return os.ErrNotExist + return fs.ErrNotExist } var respErr *azcore.ResponseError if !errors.As(err, &respErr) { diff --git a/modules/storage/local.go b/modules/storage/local.go index 1e4637a6468..cc8b572dbb3 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/url" "os" "path/filepath" @@ -138,7 +139,7 @@ func (l *LocalStorage) ServeDirectURL(path, name, _ string, reqParams *ServeDire } func (l *LocalStorage) normalizeWalkError(err error) error { - if errors.Is(err, os.ErrNotExist) { + if errors.Is(err, fs.ErrNotExist) { // ignore it because the file may be deleted during the walk, and we don't care about it return nil } diff --git a/modules/storage/local_test.go b/modules/storage/local_test.go index a12b1cda03b..42d183c4480 100644 --- a/modules/storage/local_test.go +++ b/modules/storage/local_test.go @@ -4,6 +4,7 @@ package storage import ( + "io/fs" "os" "strings" "testing" @@ -72,7 +73,7 @@ func TestLocalStorageDelete(t *testing.T) { if exists { require.NoError(t, err) } else { - require.ErrorIs(t, err, os.ErrNotExist) + require.ErrorIs(t, err, fs.ErrNotExist) } } diff --git a/modules/storage/minio.go b/modules/storage/minio.go index db43ba2484e..6bf08f4158e 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "net/url" "os" @@ -87,9 +88,9 @@ func convertMinioErr(err error, optMsg ...string) error { // Convert two responses to standard analogues switch errResp.Code { case "NoSuchKey": - return wrapErr(os.ErrNotExist) + return wrapErr(fs.ErrNotExist) case "AccessDenied": - return wrapErr(os.ErrPermission) + return wrapErr(fs.ErrPermission) } return wrapErr(err) diff --git a/routers/api/packages/container/blob.go b/routers/api/packages/container/blob.go index b2ba5eac412..cb93e5c2f18 100644 --- a/routers/api/packages/container/blob.go +++ b/routers/api/packages/container/blob.go @@ -8,7 +8,6 @@ import ( "encoding/hex" "errors" "fmt" - "os" "strings" "gitea.dev/models/db" @@ -18,7 +17,6 @@ import ( "gitea.dev/modules/log" packages_module "gitea.dev/modules/packages" container_module "gitea.dev/modules/packages/container" - "gitea.dev/modules/util" packages_service "gitea.dev/services/packages" "github.com/opencontainers/go-digest" @@ -51,28 +49,10 @@ func saveAsPackageBlobInternal(ctx context.Context, hsr packages_module.HashedSi if err := packages_service.CheckSizeQuotaExceeded(ctx, pci.Creator, pci.Owner, packages_model.TypeContainer, hsr.Size()); err != nil { return err } - - pb, exists, err = packages_model.GetOrInsertBlob(ctx, pb) + pb, exists, err = packages_service.GetOrSavePackageBlob(ctx, contentStore, pb, hsr) if err != nil { - log.Error("Error inserting package blob: %v", err) return err } - // FIXME: Workaround to be removed in v1.20 - // https://github.com/go-gitea/gitea/issues/19586 - if exists { - err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256)) - if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) { - log.Debug("Package registry inconsistent: blob %s does not exist on file system", pb.HashSHA256) - exists = false - } - } - if !exists { - if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), hsr, hsr.Size()); err != nil { - log.Error("Error saving package blob in content store: %v", err) - return err - } - } - return createFileForBlob(ctx, uploadVersion, pb) }) if err != nil { diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index a9124b70cb0..3201250c93f 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -9,7 +9,6 @@ import ( "io" "net/http" "net/url" - "os" "regexp" "strconv" "strings" @@ -28,7 +27,6 @@ import ( "gitea.dev/modules/setting" "gitea.dev/modules/storage" "gitea.dev/modules/structs" - "gitea.dev/modules/util" "gitea.dev/routers/api/packages/helper" auth_service "gitea.dev/services/auth" "gitea.dev/services/context" @@ -250,7 +248,7 @@ func PostBlobsUploads(ctx *context.Context) { mount := ctx.FormTrim("mount") from := ctx.FormTrim("from") if mount != "" { - blob, _ := workaroundGetContainerBlob(ctx, &container_model.BlobSearchOptions{ + blob, _ := getExistingContainerBlob(ctx, &container_model.BlobSearchOptions{ Repository: from, Digest: mount, }) @@ -506,7 +504,7 @@ func getBlobFromContext(ctx *context.Context) (*packages_model.PackageFileDescri return nil, container_model.ErrContainerBlobNotExist } - return workaroundGetContainerBlob(ctx, &container_model.BlobSearchOptions{ + return getExistingContainerBlob(ctx, &container_model.BlobSearchOptions{ OwnerID: ctx.Package.Owner.ID, Image: ctx.PathParam("image"), Digest: string(d), @@ -643,7 +641,7 @@ func getManifestFromContext(ctx *context.Context) (*packages_model.PackageFileDe return nil, err } - return workaroundGetContainerBlob(ctx, opts) + return getExistingContainerBlob(ctx, opts) } // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#checking-if-content-exists-in-the-registry @@ -789,23 +787,20 @@ func GetTagsList(ctx *context.Context) { }) } -// FIXME: Workaround to be removed in v1.20. -// Update maybe we should never really remote it, as long as there is legacy data? -// https://github.com/go-gitea/gitea/issues/19586 -func workaroundGetContainerBlob(ctx *context.Context, opts *container_model.BlobSearchOptions) (*packages_model.PackageFileDescriptor, error) { +// getExistingContainerBlob checks if the blob exists in the database and content store, and returns the package file descriptor if it does. +func getExistingContainerBlob(ctx *context.Context, opts *container_model.BlobSearchOptions) (*packages_model.PackageFileDescriptor, error) { blob, err := container_model.GetContainerBlob(ctx, opts) if err != nil { return nil, err } - err = packages_module.NewContentStore().Has(packages_module.BlobHash256Key(blob.Blob.HashSHA256)) + storeKey := packages_module.BlobHash256Key(blob.Blob.HashSHA256) + sz, err := packages_module.NewContentStore().OptionalSize(storeKey) if err != nil { - if errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist) { - log.Debug("Package registry inconsistent: blob %s does not exist on file system", blob.Blob.HashSHA256) - return nil, container_model.ErrContainerBlobNotExist - } - return nil, err + return nil, fmt.Errorf("unable to check object size in content store: %w", err) + } + if sz.ValueOrDefault(-1) != blob.Blob.Size { + return nil, container_model.ErrContainerBlobNotExist } - return blob, nil } diff --git a/routers/api/packages/container/manifest.go b/routers/api/packages/container/manifest.go index 79b8375a4a8..54a93ed7ca1 100644 --- a/routers/api/packages/container/manifest.go +++ b/routers/api/packages/container/manifest.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "io" - "os" "strings" "gitea.dev/models/db" @@ -20,7 +19,6 @@ import ( "gitea.dev/modules/log" packages_module "gitea.dev/modules/packages" container_module "gitea.dev/modules/packages/container" - "gitea.dev/modules/util" notify_service "gitea.dev/services/notify" packages_service "gitea.dev/services/packages" container_service "gitea.dev/services/packages/container" @@ -379,26 +377,10 @@ func createFileFromBlobReference(ctx context.Context, pv, uploadVersion *package } func createManifestBlob(ctx context.Context, contentStore *packages_module.ContentStore, mci *manifestCreationInfo, pv *packages_model.PackageVersion, buf *packages_module.HashedBuffer) (_ *packages_model.PackageBlob, created bool, manifestDigest string, _ error) { - pb, exists, err := packages_model.GetOrInsertBlob(ctx, packages_service.NewPackageBlob(buf)) + pb, exists, err := packages_service.GetOrSavePackageBlob(ctx, contentStore, packages_service.NewPackageBlob(buf), buf) if err != nil { - log.Error("Error inserting package blob: %v", err) return nil, false, "", err } - // FIXME: Workaround to be removed in v1.20 - // https://github.com/go-gitea/gitea/issues/19586 - if exists { - err = contentStore.Has(packages_module.BlobHash256Key(pb.HashSHA256)) - if err != nil && (errors.Is(err, util.ErrNotExist) || errors.Is(err, os.ErrNotExist)) { - log.Debug("Package registry inconsistent: blob %s does not exist on file system", pb.HashSHA256) - exists = false - } - } - if !exists { - if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), buf, buf.Size()); err != nil { - log.Error("Error saving package blob in content store: %v", err) - return nil, false, "", err - } - } manifestDigest = digestFromHashSummer(buf) pf, err := createFileFromBlobReference(ctx, pv, nil, &blobReference{ diff --git a/services/packages/packages.go b/services/packages/packages.go index 68e43cfab5d..e102b52ea6d 100644 --- a/services/packages/packages.go +++ b/services/packages/packages.go @@ -264,6 +264,42 @@ func NewPackageBlob(hsr packages_module.HashedSizeReader) *packages_model.Packag } } +func GetOrSavePackageBlob(ctx context.Context, contentStore *packages_module.ContentStore, blob *packages_model.PackageBlob, data packages_module.HashedSizeReader) (_ *packages_model.PackageBlob, _ bool, retErr error) { + if blob.Size != data.Size() { + return nil, false, fmt.Errorf("size mismatch: blob size %d, data size %d", blob.Size, data.Size()) + } + pb, existsInDatabase, err := packages_model.GetOrInsertBlob(ctx, blob) + if err != nil { + return nil, false, fmt.Errorf("unable to get or insert blob: %w", err) + } + + defer func() { + if retErr != nil && !existsInDatabase { + if errDelete := packages_model.DeleteBlobByID(ctx, pb.ID); errDelete != nil { + log.Error("unable to delete blob from database after failed save in content store: %v", errDelete) + } + } + }() + + var objSize optional.Option[int64] + storeKey := packages_module.BlobHash256Key(pb.HashSHA256) + if existsInDatabase { + // check if the blob file actually is valid in the content store + objSize, err = contentStore.OptionalSize(storeKey) + if err != nil { + return nil, false, fmt.Errorf("unable to check object size in content store: %w", err) + } + } + if objSize.ValueOrDefault(-1) != blob.Size { + if err := contentStore.Save(storeKey, data, data.Size()); err != nil { + return nil, false, fmt.Errorf("unable to save object in content store: %w", err) + } + } + // existsInDatabase controls the "roll back", if other errors happen later, + // the "non-existing (newly created)" blob will be deleted from the content store, but not if it already existed in the database. + return pb, existsInDatabase, nil +} + func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVersion, pvi *PackageInfo, pfci *PackageFileCreationInfo) (*packages_model.PackageFile, *packages_model.PackageBlob, bool, error) { if err := CheckSizeQuotaExceeded(ctx, pfci.Creator, pvi.Owner, pvi.PackageType, pfci.Data.Size()); err != nil { return nil, nil, false, err @@ -272,21 +308,13 @@ func addFileToPackageVersion(ctx context.Context, pv *packages_model.PackageVers return addFileToPackageVersionUnchecked(ctx, pv, pfci) } -func addFileToPackageVersionUnchecked(ctx context.Context, pv *packages_model.PackageVersion, pfci *PackageFileCreationInfo) (*packages_model.PackageFile, *packages_model.PackageBlob, bool, error) { +func addFileToPackageVersionUnchecked(ctx context.Context, pv *packages_model.PackageVersion, pfci *PackageFileCreationInfo) (_ *packages_model.PackageFile, _ *packages_model.PackageBlob, created bool, _ error) { log.Trace("Adding package file: %v, %s", pv.ID, pfci.Filename) - pb, exists, err := packages_model.GetOrInsertBlob(ctx, NewPackageBlob(pfci.Data)) + pb, exists, err := GetOrSavePackageBlob(ctx, packages_module.NewContentStore(), NewPackageBlob(pfci.Data), pfci.Data) if err != nil { - log.Error("Error inserting package blob: %v", err) return nil, nil, false, err } - if !exists { - contentStore := packages_module.NewContentStore() - if err := contentStore.Save(packages_module.BlobHash256Key(pb.HashSHA256), pfci.Data, pfci.Data.Size()); err != nil { - log.Error("Error saving package blob in content store: %v", err) - return nil, nil, false, err - } - } if pfci.OverwriteExisting { pf, err := packages_model.GetFileForVersionByName(ctx, pv.ID, pfci.Filename, pfci.CompositeKey) diff --git a/services/packages/packages_test.go b/services/packages/packages_test.go index b4563cf74ff..ea27d62d4ba 100644 --- a/services/packages/packages_test.go +++ b/services/packages/packages_test.go @@ -4,6 +4,11 @@ package packages import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "io" + "net/http" "testing" "gitea.dev/models/db" @@ -13,6 +18,8 @@ import ( unit_model "gitea.dev/models/unit" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" + packages_module "gitea.dev/modules/packages" + "gitea.dev/modules/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -22,6 +29,72 @@ func TestMain(m *testing.M) { unittest.MainTest(m) } +func TestCreatePackageAndAddFileRestoresMissingBlobFile(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + uploadPackage := func(t *testing.T, user *user_model.User, name, filename string, data []byte) (*packages_model.PackageFile, error) { + buf, err := packages_module.CreateHashedBufferFromReader(bytes.NewReader(data)) + require.NoError(t, err) + _, pf, err := CreatePackageAndAddFile(t.Context(), + &PackageCreationInfo{ + Owner: user, + PackageType: packages_model.TypeNuGet, + Name: name, + Version: "1.0.0", + SemverCompatible: true, + Creator: user, + }, + &PackageFileCreationInfo{ + Filename: filename, + Creator: user, + Data: buf, + IsLead: true, + }) + return pf, err + } + + // This test data is from https://github.com/go-gitea/gitea/issues/39215, it doesn't really matter, actually. + // The key point is that if the blob object is missing in the content storage, it must be restored when uploaded again. + pkgData := test.WriteZipArchive(map[string]string{ + "package.nuspec": "nuget.repro1.0.0", + "lib/netstandard2.0/_._": "", + }).Bytes() + pkgDataSum := sha256.Sum256(pkgData) + key := packages_module.BlobHash256Key(hex.EncodeToString(pkgDataSum[:])) + contentStore := packages_module.NewContentStore() + + // The initial upload writes the blob row and its file + pf1, err := uploadPackage(t, user, "nuget.repro", "nuget.repro.1.0.0.nupkg", pkgData) + require.NoError(t, err) + sz, err := contentStore.OptionalSize(key) + assert.NoError(t, err) + assert.EqualValues(t, len(pkgData), sz.ValueOrDefault(-1)) + + // Simulate the storage inconsistency: the blob row survives but its file is missing + require.NoError(t, contentStore.Delete(key)) + sz, err = contentStore.OptionalSize(key) + assert.NoError(t, err) + assert.EqualValues(t, -1, sz.ValueOrDefault(-1)) + + // Publishing a package with identical content must restore the blob file + pf2, err := uploadPackage(t, user, "nuget.repro-copy", "nuget.repro-copy.1.0.0.nupkg", pkgData) + require.NoError(t, err) + sz, err = contentStore.OptionalSize(key) + assert.NoError(t, err) + assert.EqualValues(t, len(pkgData), sz.ValueOrDefault(-1)) + + // The blob file must be present and both packages must be downloadable + for _, pf := range []*packages_model.PackageFile{pf1, pf2} { + s, _, _, err := OpenFileForDownload(t.Context(), pf, http.MethodGet) + require.NoError(t, err) + respData, err := io.ReadAll(s) + require.NoError(t, err) + assert.NoError(t, s.Close()) + assert.Equal(t, pkgData, respData) + } +} + func TestUnlinkFromRepositoryRequiresTargetRepoAdmin(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) repo := &repo_model.Repository{OwnerID: 3, OwnerName: "org3", Name: "package-repo", LowerName: "package-repo", IsPrivate: true}