feat(storage): support for soft delete policies and restore #9520
Conversation
Note: test currently fails pending investiagtion into setting generation on restore
product-auto-label
bot
added
size: l
storage/bucket.go
Outdated
|
|
||
| // SoftDeleted returns a new BucketHandle with the option to include | ||
| // soft-deleted items in list results. | ||
| func (b *BucketHandle) SoftDeleted(include bool) *BucketHandle { |
There was a problem hiding this comment.
Since this only applies (I think) to BucketHandle.Objects did we consider making this functionality a field on query instead? I think that would be preferable.
There was a problem hiding this comment.
I agree that it is preferable. Changed.
storage/storage.go
Outdated
| // Restore will restore a soft-deleted object to full object status. | ||
| // Note that you must specify a generation to use this method. | ||
| // copySourceACL indicates whether the restored object should copy the | ||
| // access controls of the source object. |
There was a problem hiding this comment.
Is this only relevant for buckets with fine-grained access? If UBLA is enabled, is this ignored or does it cause an error?
There was a problem hiding this comment.
If the target bucket has UBLA enforced, the user will get an error warning that object ACLs will not be restored and the object will not be restored.
| @@ -731,7 +764,7 @@ func (c *grpcStorageClient) UpdateBucketACL(ctx context.Context, bucket string, | |||
| func (c *grpcStorageClient) DeleteObjectACL(ctx context.Context, bucket, object string, entity ACLEntity, opts ...storageOption) error { | |||
| // There is no separate API for PATCH in gRPC. | |||
| // Make a GET call first to retrieve ObjectAttrs. | |||
| attrs, err := c.GetObject(ctx, bucket, object, defaultGen, nil, nil, opts...) | |||
| attrs, err := c.GetObject(ctx, &getObjectParams{bucket, object, defaultGen, nil, nil, false}, opts...) | |||
There was a problem hiding this comment.
Can ACLs be updated on a soft deleted object?
There was a problem hiding this comment.
I don't think soft deleted objects can be updated at all.
storage/storage.go
Outdated
| @@ -1686,6 +1695,10 @@ type Query struct { | |||
| // prefixes returned by the query. Only applicable if Delimiter is set to /. | |||
| // IncludeFoldersAsPrefixes is not yet implemented in the gRPC API. | |||
| IncludeFoldersAsPrefixes bool | |||
|
|
|||
| // SoftDeleted indicates whether to list soft-deleted objects. If true, only | |||
| // objects that have been soft-deleted will be listed. | |||
There was a problem hiding this comment.
Note as well that by default, soft deleted objects will not be listed.
| CopySourceACL bool | ||
| } | ||
|
|
||
| // Restore will restore a soft-deleted object to a live object. | ||
| // Note that you must specify a generation to use this method. |
There was a problem hiding this comment.
We really should have samples for this feature; people are going to be so confused otherwise.
| CopySourceACL bool | ||
| } | ||
|
|
||
| // Restore will restore a soft-deleted object to a live object. | ||
| // Note that you must specify a generation to use this method. |
There was a problem hiding this comment.
If generation is not specified, do we get an intelligible error from the server based on whatever the Go client sends by default? Might be good to check this.
There was a problem hiding this comment.
The error should be intelligible, let me check...
GRPC
rpc error: code = InvalidArgument desc = You must specify a generation. error details: name = BadRequest field = desc = You must specify a generation.
Ah, HTTP's is no longer intelligible since it's a required param googleapi: Error 400: Invalid argument., invalid
What do you suggest we do here? Set if o.gen < 0 { gen = 0 } to get the intelligible error? Sending generation=0 results in googleapi: Error 400: Restore request must specify a generation., required
There was a problem hiding this comment.
Sending zero seems fine since the default of -1 seems to be a placeholder for the most recent generation. Or we could just return an error without sending the request since a required param is missing. @JesseLovelace what are other languages doing for this case, assuming that it is relevant?
There was a problem hiding this comment.
I think sending zero to get the intelligible error seems best here
|
Hello @BrennaEpp @tritone, when do we expect this PR to be released? Seems like the linked release issue has been created for a while now. |
Hi @bianpengyuan, this PR is released now in storage v1.41.0 |
Note: test currently fails pending investiagtion into setting generation on restore
Fixes #9397