Repository navigation
Emulate delete markers on a versioned azureblob store - #1207
Open
gavin-thompson-postman wants to merge 10 commits into
Open
gavin-thompson-postman wants to merge 10 commits into
gavin-thompson-postman wants to merge 10 commits into
Conversation
Map S3 object versioning onto Azure's native blob versioning: version ids on writes, copies and multipart completes; reads, heads, deletes, copies and part copies by version id; and ListObjectVersions. Azure enables versioning per storage account on the management plane (ARM), which the data-plane SDK can neither set nor read, so the support is opt-in through s3proxy.azureblob.versioning: the operator asserts the account has versioning on, and only then does the store report supportsVersioning and honor the requests. Left unset it answers NotImplemented as before. Reading or enabling a bucket's versioning still answers NoSuchBucket for a container that does not exist, since the frontend leaves that check to a store that versions. Deleting the current version by id, which Azure refuses on the root blob, deletes the base blob first and then the demoted version. Azure judges a delete's If-Match against the current blob rather than the version named, so a conditional delete of a version is refused rather than carried out unconditionally. Azure keeps no delete markers and cannot suspend versioning per bucket, so those are left unimplemented rather than faked. A version client's URL already carries a versionid query, so the signed copy-source URL joins its SAS with '&' when the URL has one. Unit-test the config gating, the conditional version delete, the ListObjectVersions page mapping, the copy-source SAS join, and the versionId and headerless-error translations -- the paths Azurite cannot exercise -- with hand-built doubles, leaving Azurite for the rest. See gaul#1137. Co-Authored-By: Claude <noreply@anthropic.com>
Removing the current version by id deletes the base blob to demote it, then removes the demoted version. A write landing between Azure's refusal and that delete had its own fresh version demoted instead, leaving the key with no current version. Condition the base delete on the named version's ETag, which the base blob carries while that version is current; when the condition fails or the base is already gone, something else has demoted the version and it is simply removed. Co-Authored-By: Claude <noreply@anthropic.com>
The doubles assumed Azure behavior that a real, versioned storage account does not show: - A listing returns a key's versions oldest first and leaves isCurrentVersion unset, not false, on all but the current one. - A version delete's If-Match is judged against the version named: a non-current version deletes under its own ETag and is refused 412 ConditionNotMet under the current blob's. - A malformed versionId is answered 400 with no x-ms-error-code, on GET, HEAD and DELETE alike; InvalidQueryParameterValue never comes back. A well-formed but unknown one is answered 404 BlobNotFound, where S3 answers NoSuchVersion. A HEAD's 404 still carries its code in the header. With the doubles corrected three tests fail against the store as it stands, each marking code built on the old assumption: testVersionsPageMapsVersions (versions come out oldest first), testConditionalDeleteOfVersionReachesAzure (the condition is refused rather than sent) and testUnknownVersionIdIsNoSuchVersion (NoSuchKey). The fixes follow separately. Co-Authored-By: Claude <noreply@anthropic.com>
A blob written while the account's versioning was off comes back, once versioning is on again, after the versions written before it with no version id -- as the double had it -- but with isCurrentVersion unset like every other version, not true, though it is the one a plain read serves. With the double corrected testVersionsPageNamesUnversionedBlobNull also asserts that this "null" version is the latest, as S3 reports it, and fails: no version of such a key comes out flagged latest. Co-Authored-By: Claude <noreply@anthropic.com>
Modernizer flags Optional.get. Co-Authored-By: Claude <noreply@anthropic.com>
Azure lists a key's versions oldest first; S3 lists them newest first. Gather each key's versions whole and report them reversed, at most MaxKeys entries. Page on S3's own (key, version-id) markers rather than Azure's continuation token: the listing restarts from the key marker, which Azure's startFrom includes, and drops what the client has seen, so a page that fills inside a key -- even one whose versions straddle Azure's pages -- resumes inside it. The latest version is the one Azure flags current or, failing that, the one with no version id: a blob written while the account was unversioned, which Azure lists last and leaves unflagged though a plain read serves it. Azure answers a version the blob does not have 404 BlobNotFound, as for a blob that is not there; when the request named a version, report NoSuchVersion as S3 does. Reads, heads, copies and part copies by version translate so. The refusal of If-Match on a delete naming a version stays, but for the reason S3 gives: S3 itself answers such a delete 501 NotImplemented, whether or not the ETag matches, and honors If-Match only on a delete of the current object. bfc488d had read the refusal as a bug and flipped its test to expect the request to reach Azure; restore it. Co-Authored-By: Claude <noreply@anthropic.com>
On a versioned store "null" names S3's null version: the base blob while it carries no version id, a blob written while the account was unversioned. It was treated as no version at all, so DELETE ?versionId=null removed the current version of a key that had no null version, which S3 leaves alone; and GET, HEAD and the copy sources handed Azure the literal "null", which it rejects as malformed. Reads and copy sources now resolve "null" to the base blob when it has no version id and answer NoSuchVersion otherwise. A delete of "null" removes such a base blob under its ETag, and also the non-current version Azure keeps of it (deleting a base blob on a versioned account keeps its content under a fresh id, where S3 removes the null version for good); with no null version it succeeds and changes nothing. An unversioned store still takes "null" as the object itself. A malformed versionId, which Azure answers 400 with no error code, is InvalidArgument "Invalid version id specified", as S3 answers it, rather than the BadDigest a bare 400 becomes. Found by a GitHub Copilot review run outside this PR. Co-Authored-By: Claude <noreply@anthropic.com>
Azure keeps no delete markers. As the google-cloud-storage store does, a delete naming no version on a versioned store now writes an empty blob carrying s3proxy_delete_marker=true, which Azure versions over the current blob like any write, and reports it as the marker S3 would lay down. A key whose current version is such a blob holds no object: - GET and HEAD answer NoSuchKey with x-amz-delete-marker and the marker's version id; naming the marker's version answers 405. - An unversioned listing leaves the key out; ListObjectVersions reports the marker as a DeleteMarker entry. - A copy from it answers NoSuchKey, and one naming the marker's version is refused as InvalidRequest. - If-None-Match: * on a put or a multipart complete replaces the marker, under its ETag, rather than failing on it. - DeleteObjects routes each key through removeBlob, since a batch cannot write blobs, and reports each marker written. Deleting the current version by id still leaves the key with no current version, since Azure cannot make an existing version current again. S3 promotes the newest remaining one; reads, copies and listings now take that one as current, as the google-cloud-storage store does when its live generation is deleted. This only holds together with markers: a plain delete no longer demotes the current blob, so the newest version is never the content a plain delete removed. Deleting a marker this way is how S3 undeletes a key. If-None-Match: * likewise fails on a key whose current version was deleted by id while a version remains. An unversioned store is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
…rsions on azureblob Addresses review findings on the delete-marker emulation: - drop s3proxy_delete_marker / s3proxy_target_blob_name from user metadata on PUT, copy and multipart, so a client can't make an object read as a delete marker - with versioning on, ListObjectsV2 lists versions and reports each key by its latest, so a key whose current version was deleted by id (including one undeleted by deleting its marker) is listed as GET serves it; pages resume by name - time versions by their version id, not the whole-second Last-Modified, and break ties latest-first, so a PUT and its delete marker in the same second list in order Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
A Copilot review I ran outside this PR, not on the PR itself, found three issues. I reproduced all three against a real Azure storage account, checked the expected behaviour against AWS S3, and fixed them in b40b069:
After the fix, Azure matches S3 for all three. That includes PUT, DELETE, PUT within one second, and paging over promoted, undeleted and hidden keys with |
A versioned azureblob store reads a key with no current version from its newest remaining one, as S3 promotes it once the current version is deleted by id, and finds that version by listing the key's versions. A credential allowed to read and write blobs can still be refused the listing -- an account SAS with resource type o alone, which Azure answers 403 AuthorizationResourceTypeMismatch -- and that refusal escaped untranslated, so a GET or HEAD of a missing key, a copy from a missing source, and an If-None-Match: * write of a missing key all answered 500 InternalError. They now answer 403 AccessDenied, naming what the credential lacks: containers/blobs/read, or for a SAS permission l and resource type c. S3 itself answers a missing key 403 rather than 404 to a caller that may not list the bucket, and NoSuchKey would deny a version that may be there. Any other listing failure is translated as usual. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1139. This branch contains #1139's commits. Until that merges, review only the three commits on top of it,
b6c12d86(delete markers),b40b0691(review fixes) andd4b1917b(AccessDenied when a version listing is refused): diff against #1139. I'll rebase once #1139 is in.Azure keeps no delete markers. This emulates them the way
GCloudBlobStorealready does, and only whens3proxy.azureblob.versioning=true. An unversioned store is unchanged.Delete markers. A delete naming no version writes an empty blob carrying
s3proxy_delete_marker=true. Azure versions it over the current blob like any other write, and S3Proxy reports it as the marker S3 would lay down (DeleteMarker: trueplus its version id). A key whose current version is a marker:404 NoSuchKeywithx-amz-delete-marker: trueand the marker's version id. Naming the marker's version returns405.DeleteMarkerentry.NoSuchKey. Naming the marker's version,InvalidRequest.If-None-Match: *on a put or multipart complete: replaces the marker, conditioned on its ETag, instead of failing.removeBlob, because a batch can't write blobs. It reports each marker it writes.Promotion. Azure can't make an existing version current again, so deleting the current version by id still leaves the key with no current version. S3 promotes the newest remaining version. Reads, copies and listings now treat that version as current, as
GCloudBlobStore.resolveVersiondoes when its live generation is deleted. Deleting a marker this way is how S3 undeletes a key.This only works together with markers. A plain delete now writes a marker rather than demoting the current blob, so the newest remaining version is never content a plain delete just removed.
If-None-Match: *fails, as on S3, on a key whose current version was deleted by id while a version remains.Costs:
If-None-Match: *write, makes one extra version listing.Not covered: suspending versioning per bucket (Azure versioning is account-wide), so a fresh bucket still reports
Enabled. A plain listing still omits a key whose current version was deleted by id, the same gapGCloudBlobStorehas.Testing:
mvn testpasses.openInputStreamreturns the marker's metadataFix: refused version listings (
d4b1917b). With versioning on, a read that finds no current version lists the key's versions to find one to promote. Copies andIf-None-Match: *writes do the same. A credential that can read and write blobs but can't list the container (for example an account SAS withsrt=o, which Azure refuses with403 AuthorizationResourceTypeMismatch) got500 InternalErrorfrom:If-None-Match: *put of a missing keyThese now answer
403 AccessDenied, and the message names the missing permission: RBACcontainers/blobs/read, or for a SAS, permissionlwith resource typec. S3 does the same: it answers a missing key with403, not404, when the caller lacks list permission. A404would also wrongly say the key is absent when a version might remain to be promoted. Other listing failures are translated as before. The fix addsS3Exceptions.accessDenied, which #1234 reuses.AzureBlobStoreTest): with the listing refused, GET, HEAD, copy andIf-None-Match: *PUT answerAccessDeniedand nothing is written. With the listing allowed, GET, HEAD and copy still serve the newest remaining version. The fullmvn test(661 tests) andmvn verify -DskipTests -Pjdepspass on JDK 25.srt=o,racwdlx: GET of a missing key, HEAD of a missing key, a copy from a missing source, and a GET of a key whose current version was deleted by id each went from500to403 AccessDenied. Reads of existing keys are still200. (Before is the earlier build of this stack.) With the account key, a missing key is still404 NoSuchKey, and the marker, undelete and promotion checks pass on this branch alone.Generated with Claude Code