feat(repository): Add support for Azure DeleteBlob operations where immutability protection is on
#3394
Conversation
… immutability protection is on
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3394 +/- ##
==========================================
- Coverage 75.82% 75.81% -0.01%
==========================================
Files 465 465
Lines 37165 37165
==========================================
- Hits 28179 28178 -1
Misses 7055 7055
- Partials 1931 1932 +1 ☔ View full report in Codecov by Sentry. |
julio-lopez
left a comment
There was a problem hiding this comment.
@KastenMike See inline comments. Thanks.
| bc, err := az.service.ServiceClient(). | ||
| NewContainerClient(az.container). | ||
| NewBlobClient(az.getObjectNameString(b)). | ||
| WithVersionID(*resp.VersionID) |
There was a problem hiding this comment.
A response from the server is an "external input" and should be validated. Check that VersionID is set before de-referencing it.
There was a problem hiding this comment.
Since this path is only reached for those with versioning + immutability support enabled the VersionID can assumed to be populated like how things like *it.Properties.ContentLength are also not checked for...but will add some checks
There was a problem hiding this comment.
Understood.
Nevertheless,
- a client should always validate the response and never trust that the server is sending a valid response;
- similarly, a server should never trust that a client is sending a correct request.
repo/blob/azure/azure_storage.go
Outdated
| NewBlobClient(az.getObjectNameString(b)). | ||
| WithVersionID(*resp.VersionID) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get versioned blob client") |
There was a problem hiding this comment.
It may make sense to simply log the error and return success in this case. WDYT?
Something like:
| return errors.Wrap(err, "failed to get versioned blob client") | |
| log(ctx).Info("failed to get versioned client for deleting blob version for delete marker", err) | |
| return nil |
There was a problem hiding this comment.
There's no logging in any of the client funcs but I can silently return null if preferred, but I guess the odds of an error being hit are very low
There was a problem hiding this comment.
The odds of the error occurring seem low, however at a large enough scale, the error will certainly happen. Silently ignoring the error makes it really hard to debug when the error does occur. We can add the log message. This is unlikely to become noisy, but if it does, we can change the level to Debug, or we can consider other alternatives such as adding a counter for "handled" errors, or some other mechanism.
repo/blob/azure/azure_storage.go
Outdated
|
|
||
| _, err = bc.Delete(ctx, nil) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to delete the delete marker blob version") |
There was a problem hiding this comment.
Same here, if the 0-byte blob version could not be deleted, it seems OK to not fail the blob delete operation. Removing the specific blob version could happen in the background via a 3rd party process such as lifecycle policy in the blob store.
| return errors.Wrap(err, "failed to delete the delete marker blob version") | |
| log(ctx).Info("failed to delete the delete marker blob version", err) |
|
Running provider tests as well, and the Azure tests passed. https://github.com/kopia/kopia/actions/runs/6636153104/job/18028167562?pr=3394 |
…/kopia into DeleteBlob-immu-support
| return errors.Wrap(err, "failed to soft delete blob") | ||
| } | ||
|
|
||
| log := logging.Module("azure-immutability") |
There was a problem hiding this comment.
This works, however the pattern elsewhere is to have a package-level variable.
| NewBlobClient(blobName). | ||
| WithVersionID(*resp.VersionID) | ||
| if err != nil { | ||
| log(ctx).Infof("Issue preparing versioned blob client: %v", err) |
There was a problem hiding this comment.
isn't this going to require a blank line before the return statement?
Same below.
|
Core provider tests => https://github.com/kopia/kopia/actions/runs/6646559684/job/18060262479?pr=3394 |
|
Thanks @KastenMike |
Currently kopia does not support Azure where blobs are protected by something like a policy. A session file might be prevented by a 5 day protection window but it wants to be deleted immediately. This gets around that issue e.g.
Other related Azure changes in a PR train: