From 55bddb6ce129b1109b166727b21547f95ad23331 Mon Sep 17 00:00:00 2001 From: Egon Elbre Date: Fri, 27 Oct 2023 12:05:09 +0300 Subject: [PATCH] satellite/metabase: don't use empty stream id for delete markers Using a an empty stream id makes it more difficult to target a specific delete marker. Similarly, we don't want to confuse actual stream id-s with normal ones. So, we'll create stream id-s where the first few bytes are 0xFF, but the rest is random. Change-Id: Ia7fffb0da9a071be2935df99c0846027ee2e03c3 --- satellite/metabase/delete.go | 32 +++++++++++++++++++++---- satellite/metabase/delete_test.go | 8 +++++-- satellite/metabase/get_test.go | 4 ++++ satellite/metabase/metabasetest/test.go | 20 ++++++++++++++-- satellite/metabase/metadata_test.go | 14 +++++------ 5 files changed, 63 insertions(+), 15 deletions(-) diff --git a/satellite/metabase/delete.go b/satellite/metabase/delete.go index 9998b57a7..05a00e65c 100644 --- a/satellite/metabase/delete.go +++ b/satellite/metabase/delete.go @@ -586,6 +586,11 @@ func (db *DB) DeleteObjectLastCommitted( } if opts.Suspended { + deleterMarkerStreamID, err := generateDeleteMarkerStreamID() + if err != nil { + return DeleteObjectResult{}, Error.Wrap(err) + } + err = txutil.WithTx(ctx, db.db, nil, func(ctx context.Context, tx tagsql.Tx) (err error) { // TODO(ver) fold deleteObjectUnversionedCommitted into query below using ON CONFLICT deleted, err := db.deleteObjectUnversionedCommitted(ctx, opts.ObjectLocation, tx) @@ -609,13 +614,14 @@ func (db *DB) DeleteObjectLastCommitted( RETURNING version, created_at - `, opts.ProjectID, []byte(opts.BucketName), opts.ObjectKey, deleted.MaxVersion+1, uuid.UUID{}) + `, opts.ProjectID, []byte(opts.BucketName), opts.ObjectKey, deleted.MaxVersion+1, deleterMarkerStreamID) var marker Object marker.ProjectID = opts.ProjectID marker.BucketName = opts.BucketName marker.ObjectKey = opts.ObjectKey marker.Status = DeleteMarkerUnversioned + marker.StreamID = deleterMarkerStreamID err = row.Scan(&marker.Version, &marker.CreatedAt) if err != nil { @@ -630,7 +636,11 @@ func (db *DB) DeleteObjectLastCommitted( } if opts.Versioned { // Instead of deleting we insert a deletion marker. - streamID := uuid.UUID{} + deleterMarkerStreamID, err := generateDeleteMarkerStreamID() + if err != nil { + return DeleteObjectResult{}, Error.Wrap(err) + } + row := db.db.QueryRowContext(ctx, ` WITH check_existing_object AS ( SELECT status @@ -663,13 +673,13 @@ func (db *DB) DeleteObjectLastCommitted( RETURNING * ) SELECT version, created_at FROM added_object - `, opts.ProjectID, []byte(opts.BucketName), opts.ObjectKey, streamID) + `, opts.ProjectID, []byte(opts.BucketName), opts.ObjectKey, deleterMarkerStreamID) var deleted Object deleted.ProjectID = opts.ProjectID deleted.BucketName = opts.BucketName deleted.ObjectKey = opts.ObjectKey - deleted.StreamID = streamID + deleted.StreamID = deleterMarkerStreamID deleted.Status = DeleteMarkerVersioned err = row.Scan(&deleted.Version, &deleted.CreatedAt) @@ -725,3 +735,17 @@ func (db *DB) DeleteObjectLastCommitted( return result, nil } + +// generateDeleteMarkerStreamID returns a uuid that has the first 6 bytes as 0xff. +// Creating a stream id, where the first bytes are 0xff makes it easily recognizable as a delete marker. +func generateDeleteMarkerStreamID() (uuid.UUID, error) { + v, err := uuid.New() + if err != nil { + return v, Error.Wrap(err) + } + + for i := range v[:6] { + v[i] = 0xFF + } + return v, nil +} diff --git a/satellite/metabase/delete_test.go b/satellite/metabase/delete_test.go index 4efa46bc5..b114181a7 100644 --- a/satellite/metabase/delete_test.go +++ b/satellite/metabase/delete_test.go @@ -782,7 +782,6 @@ func TestDeleteObjectVersioning(t *testing.T) { }.Run(ctx, t, db, obj, 0) marker := committed.ObjectStream - marker.StreamID = uuid.UUID{} marker.Version = committed.Version + 1 now := time.Now() @@ -800,6 +799,7 @@ func TestDeleteObjectVersioning(t *testing.T) { }, }, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) metabasetest.Verify{ @@ -829,7 +829,6 @@ func TestDeleteObjectVersioning(t *testing.T) { }.Run(ctx, t, db, obj, 0) marker := committed.ObjectStream - marker.StreamID = uuid.UUID{} marker.Version = committed.Version + 1 now := time.Now() @@ -847,6 +846,7 @@ func TestDeleteObjectVersioning(t *testing.T) { }, }, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) marker2 := marker @@ -865,6 +865,7 @@ func TestDeleteObjectVersioning(t *testing.T) { }, }, }, + OutputMarkerStreamID: &marker2.StreamID, }.Check(ctx, t, db) metabasetest.Verify{ @@ -1165,6 +1166,7 @@ func TestDeleteObjectsAllVersions(t *testing.T) { metabasetest.Verify{}.Check(ctx, t, db) }) + // TODO(ver): these tests look like they are in the wrong location t.Run("delete last committed unversioned with suspended", func(t *testing.T) { defer metabasetest.DeleteAll{}.Check(ctx, t, db) @@ -1200,6 +1202,7 @@ func TestDeleteObjectsAllVersions(t *testing.T) { object, }, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) metabasetest.Verify{ @@ -1241,6 +1244,7 @@ func TestDeleteObjectsAllVersions(t *testing.T) { Result: metabase.DeleteObjectResult{ Markers: []metabase.Object{marker}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) metabasetest.Verify{ diff --git a/satellite/metabase/get_test.go b/satellite/metabase/get_test.go index 3f6e44b78..0400e926c 100644 --- a/satellite/metabase/get_test.go +++ b/satellite/metabase/get_test.go @@ -200,6 +200,7 @@ func TestGetObjectExactVersion(t *testing.T) { Result: metabase.DeleteObjectResult{ Markers: []metabase.Object{versionedMarker}, }, + OutputMarkerStreamID: &versionedMarker.StreamID, }.Check(ctx, t, db) metabasetest.GetObjectExactVersion{ @@ -229,6 +230,7 @@ func TestGetObjectExactVersion(t *testing.T) { Removed: []metabase.Object{unversioned}, Markers: []metabase.Object{unversionedMarker}, }, + OutputMarkerStreamID: &unversionedMarker.StreamID, }.Check(ctx, t, db) metabasetest.GetObjectExactVersion{ @@ -1678,6 +1680,7 @@ func TestGetLatestObjectLastSegment(t *testing.T) { Result: metabase.DeleteObjectResult{ Markers: []metabase.Object{marker}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) metabasetest.GetLatestObjectLastSegment{ @@ -1737,6 +1740,7 @@ func TestGetLatestObjectLastSegment(t *testing.T) { Markers: []metabase.Object{marker}, Removed: []metabase.Object{unversioned}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) metabasetest.GetLatestObjectLastSegment{ diff --git a/satellite/metabase/metabasetest/test.go b/satellite/metabase/metabasetest/test.go index e98cfc44f..c6edae322 100644 --- a/satellite/metabase/metabasetest/test.go +++ b/satellite/metabase/metabasetest/test.go @@ -17,6 +17,7 @@ import ( "storj.io/common/storj" "storj.io/common/testcontext" + "storj.io/common/uuid" "storj.io/storj/satellite/metabase" ) @@ -430,6 +431,13 @@ func compareDeleteObjectResult(t testing.TB, got, exp metabase.DeleteObjectResul sortObjects(got.Markers) sortObjects(exp.Markers) + if len(got.Markers) == len(exp.Markers) { + // marker stream ID-s are internally generated, so we cannot upfront figure out what + // the values are. + for i := range got.Markers { + exp.Markers[i].StreamID = got.Markers[i].StreamID + } + } sortObjects(got.Removed) sortObjects(exp.Removed) @@ -788,10 +796,13 @@ func (step FinishCopyObject) Check(ctx *testcontext.Context, t testing.TB, db *m // DeleteObjectLastCommitted is for testing metabase.DeleteObjectLastCommitted. type DeleteObjectLastCommitted struct { - Opts metabase.DeleteObjectLastCommitted - Result metabase.DeleteObjectResult + Opts metabase.DeleteObjectLastCommitted + Result metabase.DeleteObjectResult + ErrClass *errs.Class ErrText string + + OutputMarkerStreamID *uuid.UUID } // Check runs the test. @@ -799,6 +810,11 @@ func (step DeleteObjectLastCommitted) Check(ctx *testcontext.Context, t testing. result, err := db.DeleteObjectLastCommitted(ctx, step.Opts) checkError(t, err, step.ErrClass, step.ErrText) compareDeleteObjectResult(t, result, step.Result) + + if step.OutputMarkerStreamID != nil && len(result.Markers) > 0 { + *step.OutputMarkerStreamID = result.Markers[0].StreamID + } + return result } diff --git a/satellite/metabase/metadata_test.go b/satellite/metabase/metadata_test.go index c6d5bb09a..447a3a945 100644 --- a/satellite/metabase/metadata_test.go +++ b/satellite/metabase/metadata_test.go @@ -187,7 +187,6 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { Status: metabase.DeleteMarkerVersioned, CreatedAt: time.Now(), } - marker.StreamID = uuid.UUID{} marker.Version++ metabasetest.DeleteObjectLastCommitted{ @@ -198,6 +197,7 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { Result: metabase.DeleteObjectResult{ Markers: []metabase.Object{marker}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) // verify we cannot update the metadata of a deleted object @@ -226,8 +226,8 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { EncryptedMetadataNonce: encryptedMetadataNonce[:], EncryptedMetadataEncryptedKey: encryptedMetadataKey, }, - ErrClass: &metabase.ErrInvalidRequest, - ErrText: "StreamID missing", + ErrClass: &metabase.ErrObjectNotFound, + ErrText: "object with specified version and committed status is missing", }.Check(ctx, t, db) metabasetest.Verify{ @@ -255,7 +255,6 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { CreatedAt: time.Now(), } marker.Version++ - marker.StreamID = uuid.UUID{} metabasetest.DeleteObjectLastCommitted{ Opts: metabase.DeleteObjectLastCommitted{ @@ -267,6 +266,7 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { Markers: []metabase.Object{marker}, Removed: []metabase.Object{object2}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) encryptedMetadata := testrand.Bytes(1024) @@ -299,8 +299,8 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { EncryptedMetadataNonce: encryptedMetadataNonce[:], EncryptedMetadataEncryptedKey: encryptedMetadataKey, }, - ErrClass: &metabase.ErrInvalidRequest, - ErrText: "StreamID missing", + ErrClass: &metabase.ErrObjectNotFound, + ErrText: "object with specified version and committed status is missing", }.Check(ctx, t, db) metabasetest.Verify{ @@ -322,7 +322,6 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { Status: metabase.DeleteMarkerVersioned, CreatedAt: time.Now(), } - marker.StreamID = uuid.UUID{} marker.Version++ metabasetest.DeleteObjectLastCommitted{ @@ -333,6 +332,7 @@ func TestUpdateObjectLastCommittedMetadata(t *testing.T) { Result: metabase.DeleteObjectResult{ Markers: []metabase.Object{marker}, }, + OutputMarkerStreamID: &marker.StreamID, }.Check(ctx, t, db) obj2 := obj