feat(repo)gcs: add support for versioning and retention #3564

guestisp · 2024-01-11T07:43:27Z

still a WIP but this PR aim to add support for versioning and retention for malware protection to the native Google Cloud Storage backend

backup and retention extension are working as expected
point in time still doesn't work, probably i need some help in this regard.

codecov · 2024-02-02T05:44:14Z

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 75.83%. Comparing base (e389d92) to head (22ab4e7).
Report is 124 commits behind head on master.

❗ Current head 22ab4e7 differs from pull request most recent head 2cb602f. Consider uploading reports for the commit 2cb602f to get more accurate results

Files	Patch %	Lines
cli/storage_gcs.go	33.33%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
+ Coverage   75.81%   75.83%   +0.01%     
==========================================
  Files         465      465              
  Lines       37171    37183      +12     
==========================================
+ Hits        28183    28199      +16     
+ Misses       7059     7056       -3     
+ Partials     1929     1928       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

guestisp · 2024-03-12T11:50:13Z

Can someone help a bit ?

KastenMike · 2024-08-29T07:11:48Z

repo/blob/gcs/gcs_storage.go

+	if opts.RetentionPeriod != 0 {
+		retainUntilDate := clock.Now().Add(opts.RetentionPeriod).UTC()
+		writer.ObjectAttrs.Retention = &storage.ObjectRetention{
+			Mode:        "Unlocked",


Suggested change

Mode: "Unlocked",

Mode: blob.Locked,

KastenMike · 2024-08-29T07:12:47Z

repo/blob/gcs/gcs_storage.go

 }

+func (gcs *gcsStorage) ExtendBlobRetention(ctx context.Context, b blob.ID, opts blob.ExtendOptions) error {
+	retentionMode := "Unlocked" // TODO: properly handle lock mode


Suggested change

retentionMode := "Unlocked" // TODO: properly handle lock mode

retentionMode := blob.Locked

KastenMike · 2024-08-29T07:14:34Z

repo/blob/gcs/gcs_storage.go

 		bucket:        cli.Bucket(opt.BucketName),
 	}

+	fmt.Println("Provo il PIT")


in general the Println's can go

i know, it's a WIP . i've opened the PR asking for help because im stuck in object sorting

I'll look into it Today, see if I notice anything

main issue i had is that GCS doesn't allow sorting object versions by creation time so i had to list all versions, sort manually by creation time, cycle the result and keep the newestthan (or older then). but im not a Go programmer, i had huge issues trying to sort and debug this thing. i have an apparently working version on my laptop but i'm not confident on why It works 🤣

(probably on Discord i can explain better)

So a couple notes from looking at it.

blob versions are automatically sorted from oldest to most recently created so no need to add sorting anywhere

IsDeleteMarker needs to consider not only if Deleted is set but also if it was deleted after the PIT e.g. !oi.Deleted.IsZero() && (gcs.PointInTime == nil || oi.Deleted.Before(*gcs.PointInTime)). This is because GCS decided to mark every previous version as being deleted. For convenience you can change infoToVersionMetadata to be a method instead of func

newestAtUnlessDeleted needs to return !v.IsDeleteMarker as mentioned in another comment

getOlderThan needs return vs[:i] like Azure since they're ascending

If you make these changes then PIT will work as expected

Not really to PIT in particular:
I guess better if Timestamp is .Created rather than .Updated

They might not have custom sorting, but we don't need it. Or manual sorting. You can delete your sort calls and create 10 versions of a blob and every list command will have them in ascending order at the PRE SORT log you added

You can see the changes I mentioned here

return v, true is not correct since it would return deleted blobs e.g. PIT=15:00, blob deleted 14:00 even when it is the latest/only blob version

I've seen your branch. If it works, i can safely drop this PR and fork yours as mine was just a wip and not a working one.

Probably they just added sorting to blobs in the time since you created this PR

They might not have custom sorting, but we don't need it. Or manual sorting. You can delete your sort calls and create 10 versions of a blob and every list command will have them in ascending order at the PRE SORT log you added

When i did this PR it wasn't stable, that's why i've added a sort function, trying to make that stable. Probably they have changed something .....

Probably they just added sorting to blobs in the time since you created this PR

Honestly, looking carefully at the versioning list, probably they don't have a sorting option for object NAMES, but the versions are named with the timestamp and even if you sort them lexicographically (https://pkg.go.dev/cloud.google.com/go/storage#BucketHandle.Objects) a timestamp is always sorted properly (as long as you don't add digits). So, probably, it always worked in this way, i've added a buggy code for a non-existant issue. Shame on me.

KastenMike · 2024-08-29T07:27:53Z

repo/blob/gcs/gcs_pit.go

+	fmt.Printf("newestAtUnlessDeleted del pit %s deleted=%t\n", v.Timestamp, v.IsDeleteMarker)
+
+	//return v, !v.IsDeleteMarker
+	return v, true


here you assume a blob is never deleted

KastenMike · 2024-08-29T07:29:45Z

repo/blob/gcs/gcs_pit.go

+func getOlderThan(vs []versionMetadata, t time.Time) []versionMetadata {
+	fmt.Printf("getOlderThan %s \n", t)
+
+	sort.Slice(vs, func(i, j int) bool { return vs[i].Timestamp.Before(vs[j].Timestamp) })


You're sorting both here and in newestAtUnlessDeleted

KastenMike · 2024-08-29T13:28:45Z

repo/blob/gcs/gcs_pit.go

+		return nil, errors.Wrapf(err, "could not get determine if bucket '%s' supports versioning", gcs.BucketName)
+	}
+
+	if !attrs.VersioningEnabled {


We also should error if attrs.ObjectRetentionMode != "Enabled"
Otherwise connecting/creating to a repo fails: ERROR cannot initialize repository: unable to write blobcfg blob: PutBlob() failed for "kopia.blobcfg": unable to complete PutBlob(kopia.blobcfg) despite 10 retries: unexpected GCS error: googleapi: Error 400: Bucket does not have object retention enabled., invalid

KastenMike · 2024-08-29T15:35:41Z

repo/blob/gcs/gcs_pit.go


-	if !attrs.VersioningEnabled {
+	if !attrs.VersioningEnabled || attrs.ObjectRetentionMode != "Enabled" {
 		return nil, errors.Errorf("cannot create point-in-time view for non-versioned bucket '%s'", gcs.BucketName)


A separate check would be better. That way you can highlight that ObjectRetention is the problem in the err

KastenMike · 2024-08-29T15:36:44Z

repo/blob/gcs/gcs_versioned.go

 	return versionMetadata{
 		Metadata:       bm,
-		IsDeleteMarker: !oi.Deleted.IsZero(),
+		IsDeleteMarker: !oi.Deleted.IsZero() && (gcs.PointInTime == nil || oi.Deleted.Before(*gcs.PointInTime)),


it should be func (gcs *gcsStorage) infoToVersionMetadata for this to work

KastenMike · 2024-09-02T12:30:27Z

repo/blob/gcs/gcs_storage.go

+	if opts.RetentionPeriod != 0 {
+		retainUntilDate := clock.Now().Add(opts.RetentionPeriod).UTC()
+		writer.ObjectAttrs.Retention = &storage.ObjectRetention{
+			Mode:        blob.Locked,


Suggested change

Mode: blob.Locked,

Mode: string(blob.Locked),

KastenMike · 2024-09-02T12:30:40Z

repo/blob/gcs/gcs_storage.go

+	retainUntilDate := clock.Now().Add(opts.RetentionPeriod).UTC().Truncate(time.Second)
+
+	ObjectRetention := &storage.ObjectRetention{
+		Mode:        retentionMode,


Suggested change

Mode: retentionMode,

Mode: string(blob.Locked),

KastenMike · 2024-09-02T12:33:39Z

repo/blob/gcs/gcs_versioned.go

+// It returns true even if versioning is enabled but currently suspended for the
+// bucket. Notice that when object locking is enabled in a bucket, object
+// versioning is enabled and cannot be suspended.
+func (gcs *gcsStorage) IsVersioned(ctx context.Context) (bool, error) {


It seems this isn't used and can be removed

KastenMike · 2024-09-02T12:34:32Z

repo/blob/gcs/gcs_versioned.go

+}
+
+// listBlobVersions lists all versions for all the blobs with the given blob ID prefix.
+func (gcs *gcsStorage) listBlobVersions(ctx context.Context, prefix blob.ID, callback versionMetadataCallback) error {


Suggested change

func (gcs *gcsStorage) listBlobVersions(ctx context.Context, prefix blob.ID, callback versionMetadataCallback) error {

func (gcs *gcsPointInTimeStorage) listBlobVersions(ctx context.Context, prefix blob.ID, callback versionMetadataCallback) error {

I guess everything in here should be gcsPointInTimeStorage rather than gcsStorage

KastenMike · 2024-09-02T12:39:11Z

repo/blob/gcs/gcs_versioned.go

+}
+
+func toBlobID(blobName, prefix string) blob.ID {
+	return blob.ID(prefix + strings.TrimPrefix(blobName, prefix))


Suggested change

return blob.ID(prefix + strings.TrimPrefix(blobName, prefix))

return blob.ID(strings.TrimPrefix(blobName, prefix))

KastenMike · 2024-09-02T12:49:23Z

repo/blob/gcs/gcs_versioned.go

+	return blob.ID(prefix + strings.TrimPrefix(blobName, prefix))
+}
+
+func infoToVersionMetadata(prefix string, oi *storage.ObjectAttrs) versionMetadata {


imo it would be better to do the Azure style where getVersionedBlobMeta calls az.getBlobMeta(it). This way there is only 1 place in the code where versionMetadata is created & (there should be, but I missed a bit) 1 place where blob.Metadata is created.

Also it would mean the non-versioned code doesn't touch the versioned side which imo is better. e.g. this calls GetMetadata rather than GetMetadata calling this & discarding things

getVersionedBlobMeta also sounds better than infoToVersionMetadata imo

KastenMike · 2024-09-02T12:56:10Z

Some follow up comments based on the current state of the PR, I also left a few more on this PR:

Can you create an isCreate check on Connect like Azure/AWS has
separate out !attrs.VersioningEnabled || attrs.ObjectRetentionMode != "Enabled" checks
string(blob.Locked) as mentioned in a comment to get around compilation issues-
remove the sorts
remove the print statements

Otherwise it looks ok 👍

Finally could you add some test coverage and a documentation section for this PR as 2 separate PRs? The test coverage can be a new branch based off this one (PR train~) to avoid the PR getting to big
Docs can just be a small section added like this for Azure
For tests you could mimic azure_versioned_test.go and azure_immu_test.go

kreare · 2024-09-02T13:10:18Z

Some follow up comments based on the current state of the PR, I also left a few more on this PR:

Can you create an isCreate check on Connect like Azure/AWS has

separate out !attrs.VersioningEnabled || attrs.ObjectRetentionMode != "Enabled" checks

string(blob.Locked) as mentioned in a comment to get around compilation issues-

remove the sorts

remove the print statements

Otherwise it looks ok 👍

Finally could you add some test coverage and a documentation section for this PR as 2 separate PRs? The test coverage can be a new branch based off this one (PR train~) to avoid the PR getting to big Docs can just be a small section added like this for Azure For tests you could mimic azure_versioned_test.go and azure_immu_test.go

currently i'm a little bit busy at work and my current version rebuilt locally (a couple of days ago) with the new laptop and newer Go version doesn't work anymore, it crash and i don't have time to troubleshoot due to lack of time

i can do the above suggested fixes but i don't have time to test or write docs, in a short time

KastenMike · 2024-09-02T15:43:21Z

repo/blob/gcs/gcs_storage.go

+
+	retainUntilDate := clock.Now().Add(opts.RetentionPeriod).UTC().Truncate(time.Second)
+
+	ObjectRetention := &storage.ObjectRetention{


Suggested change

ObjectRetention := &storage.ObjectRetention{

objectRetention := &storage.ObjectRetention{

julio-lopez · 2024-09-27T04:47:50Z

Implemented.

gandalfthegreydev added 3 commits January 9, 2024 08:50

initial support for versioni in native gcs repo

dd72f2f

Snapshot creation with versioning and retention

f8563f7

gcs: development for PIT

22ab4e7

Fix

2cb602f

KastenMike reviewed Aug 29, 2024

View reviewed changes

Some fixes

926b042

KastenMike reviewed Aug 29, 2024

View reviewed changes

KastenMike reviewed Sep 2, 2024

View reviewed changes

KastenMike mentioned this pull request Sep 26, 2024

feat(providers): GCS immutability #4134

Merged

julio-lopez closed this Sep 27, 2024

julio-lopez mentioned this pull request Sep 27, 2024

refactor(providers): simplify GCS client construction #4120

Merged

	retentionMode := "Unlocked" // TODO: properly handle lock mode
	retentionMode := blob.Locked

	func (gcs *gcsStorage) listBlobVersions(ctx context.Context, prefix blob.ID, callback versionMetadataCallback) error {
	func (gcs *gcsPointInTimeStorage) listBlobVersions(ctx context.Context, prefix blob.ID, callback versionMetadataCallback) error {

	return blob.ID(prefix + strings.TrimPrefix(blobName, prefix))
	return blob.ID(strings.TrimPrefix(blobName, prefix))


		retainUntilDate := clock.Now().Add(opts.RetentionPeriod).UTC().Truncate(time.Second)

		ObjectRetention := &storage.ObjectRetention{

feat(repo)gcs: add support for versioning and retention #3564

feat(repo)gcs: add support for versioning and retention #3564

Uh oh!

Conversation

guestisp commented Jan 11, 2024

Uh oh!

codecov bot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guestisp commented Mar 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KastenMike Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KastenMike Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KastenMike commented Sep 2, 2024

Uh oh!

kreare commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julio-lopez commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Feb 2, 2024 •

edited

Loading

KastenMike Aug 29, 2024 •

edited

Loading

KastenMike Aug 29, 2024 •

edited

Loading

kreare commented Sep 2, 2024 •

edited

Loading