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
tritone
left a comment
tritone
left a comment
There was a problem hiding this comment.
Overall looks good. One API change suggestion and a few questions/docs notes.
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.
| // 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.
tritone
left a comment
tritone
left a comment
There was a problem hiding this comment.
Overall LGTM, a few more minor questions. Let's try to remove the idempotency check for restore before merging.
storage/storage.go
Outdated
There was a problem hiding this comment.
Note as well that by default, soft deleted objects will not be listed.
storage/storage.go
Outdated
There was a problem hiding this comment.
We really should have samples for this feature; people are going to be so confused otherwise.
storage/storage.go
Outdated
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 |
4 participants
Note: test currently fails pending investiagtion into setting generation on restore
Fixes #9397