Repository navigation
feat(s3): S3プロバイダにSigV4署名を追加 - #93
Open
tishin-endou wants to merge 22 commits into
Open
tishin-endou wants to merge 22 commits into
tishin-endou wants to merge 22 commits into
Conversation
Co-Authored-By: An Qiuyu <qiuyu.an@hotmail.com>
This was referenced Jun 5, 2026
tishin-endou
changed the title
Feature/s3 addon migration
- test_region_host: mock get_s3_bucket_object_location to use pre-registered URL and avoid timestamp divergence between two aiobotocore sessions - test_validate_v1_path_file: register root_listing_url (my-subfolder/) instead of bucket root - test_validate_v1_path_file_with_subfolder: fix listing_url to my-subfolder/ and add proper XML body - test_validate_v1_path_folder: fix listing_url to my-subfolder/Photos/ - test_chunked_upload_upload_part: pass params= to register_uri/has_call so ImmutableFurl hash matches when make_request passes params separately - test_metadata_folder: assert 'photos' (xmltodict strips leading whitespace) - test_errors_out/test_creates: GET 200 empty XML + HEAD 404 to let exists() return False; DownloadError from GET 404 is not caught by exists() - test_errors_out_metadata: expect DownloadError (not MetadataError) on GET 403
tishin-endou
marked this pull request as ready for review
July 17, 2026 00:53
yacchin1205
requested changes
Aug 10, 2026
There was a problem hiding this comment.
過去に意図的に導入した仕様や既知問題への対策が複数失われているように見えるため、変更をご検討ください。また、コメントアウトして残しているコードがいくつか散見されますが、不要なものであれば削除、採用すべきものであれば、そちらを採用すべきと考えます。どちらか、ご判断いただければと思います。
以下、見つけた回帰と思われる箇所です(これが全てではないかもしれません):
- PR #77 で追加された、Version/DeleteMarkerの全削除および1000件超のVersion対応が失われているように見えます -
# Check and delete all versions of the file in batchesのコードを削除しており、移植されているように見えない - PR #53 で5GB超のCopyObject問題を理由に無効化したintra-copy/moveが、サイズ制限なしで再有効化されている -
can_intra_copy等のコードが仕様変更されている
また、テストの多くが既存の振る舞いを保証する形ではなく、新しい内部ヘルパーをモックして呼び出しだけを確認する形へ置き換えられているため、CIが成功していても上記の回帰を検出できません。
Resolve the single conflict in waterbutler/providers/s3/provider.py by adopting develop's can_intra_copy/can_intra_move signature and 5 GB threshold guard (PR RCOSDP#97). Keeping feature's signature would break core/provider.py, which passes file_size= whenever ACCEPTS_FILE_SIZE_FOR_INTRA is True.
…t legacy code The merge of upstream/develop auto-merged test_provider.py without a conflict and silently kept this branch's inverted assertions (assert provider.can_intra_move(provider)). Those contradict develop's 5 GB threshold guard, under which a call carrying no file_size must return False. Restore develop's 'assert not' form so the guard is enforced again. Delete the dead commented-out blocks left in the S3 provider: the paginator-based get_folder_metadata and get_object_versions alternatives, the presigned delete_objects / _make_delete_xml alternative, the presigned PUT x-amz-copy-source intra_copy alternative, and the pydevd_pycharm remote-debug hooks. The two Todo lines that pointed at the removed get_folder_metadata block go with them, since they would otherwise dangle. Docstrings and 'Docs:' URL references are kept. Also fix the typo in the test name, test_delete_comfirm_delete -> test_delete_confirm_delete. No behaviour change: tests/providers/s3 remains 87 passed.
…ListObjectVersions Restores the behaviour RCOSDP#77 added for the boto2 provider and that the SigV4 rewrite lost. On a versioned bucket a plain DELETE only writes a new delete marker, so every superseded version survived a delete and kept counting against the user's quota. get_object_versions drove ListObjectVersions but paged it with the ListObjectsV2 contract: it read NextContinuationToken, which that API never returns, and reissued the identical request. Against a real S3 that is an unbounded loop, not merely a lost page. It now continues on NextKeyMarker/NextVersionIdMarker, and stops rather than re-requesting when a page claims IsTruncated but carries no marker to resume from. include_delete_markers is added, off by default: DeleteMarker entries are needed to purge a key but are not restorable revisions, so revisions() must not see them. File delete now lists every version and delete marker of the key and removes them through DeleteObjects. The listing is a prefix match, so entries are filtered down to an exact key match -- without that, deleting 'foo' would also destroy 'foo.bak'. A bucket with versioning off reports its live object as version id 'null', which DeleteObjects takes verbatim. Folder delete does the same over the prefix instead of listing only the live keys with ListObjectsV2, and raises NotFoundError when the prefix holds nothing at all. An empty folder is the 0-byte 'prefix/' key S3 stores for it, which is one version of its own and must be deleted rather than mistaken for a missing folder. delete_s3_bucket_folder_objects was that path's only caller and goes with it. The 1000-object batching is extracted into delete_objects_in_chunks and shared by both paths. Two behaviours move with it: Quiet=False, so per-object Errors reported inside a 200 DeleteObjects body raise DeleteError naming the survivors instead of being discarded; and an empty object list short-circuits, since DeleteObjects rejects it. Listing failures are converted to DeleteError -- WaterButlerErrors keep their status, and ClientError/TimeoutError are caught because they are not WaterButlerErrors and would otherwise escape delete(). Messages carry the exception type, never the raw S3 error document or a presigned URL. The folder-delete tests used to replace the method under test with a coroutine stub and assert the stub had been called, which exercised the dispatch and nothing else. They now inject the listings over aiohttpretty and DeleteObjects at the client boundary so the provider code actually runs, which brings 1001-key batching, a listing spanning two continuation pages, and a partial failure under test for the first time.
…h the destination's credentials Brings server-side copy back in line with what RCOSDP#53 and RCOSDP#97 settled for the boto2 provider. can_intra_copy/can_intra_move keep develop's threshold, but the existing fallback tests only ever passed limit + 1, which cannot tell '>' from '>=' -- whether a file of exactly the limit is copied server side or streamed was never decided by a test. The three points around the boundary are pinned, together with the file_size is None case, for both predicates. intra_copy turned every failure into a 500 and pasted botocore's message -- which embeds the request id and whatever arn or bucket name S3 chose to name -- into what the user sees. ClientError is caught specifically and reported as the exception type plus S3's error code, carrying the provider's own HTTP status. CopyObject can fail with a 200 and an <Error> body: botocore rewrites the response status to 500 so the call raises, but ResponseMetadata still says 200, so anything below 400 is reported as a 500 rather than turning a failed copy into a success at the API layer. Exception types other than ClientError propagate here instead of being relabelled IntraCopyError; converting those is handled later in this branch. CopyObject is now signed with the destination's credentials and region, which is what intra_copy's own docstring promises (the destination's credentials hold read access to the source bucket) and what develop does. Signing with the source's key required a permission nobody documents -- that the source can write to the destination. The region matters for the same reason: SigV4 puts the region in the signing scope and the host in the request, so signing a write to the destination bucket under the source's region is refused before the object is even read. dest_provider._check_region() is awaited explicitly so the destination's region cannot be used unresolved; it is a no-op once resolved. The source's own _check_region() stays, as in develop, for resolving CopySource and for the metrics.
The GRDM file browser walks a folder one page at a time, so metadata() has to be able to stop after a page and hand back a continuation token. With a next_token keyword the listing stops after one page of at most 1000 keys and the continuation token rides along as the last element; without it the listing is drained as before and contains only metadata objects. The API layer always names the keyword and core's internal callers never do, so the file browser gets its cursor while _folder_file_op, zip and ZipStreamGenerator keep getting a complete listing they can read .name off every element of. handle_data() splits the trailing token back off, checking that the last element really is a str rather than assuming it, so a listing that ends without a token cannot lose its last entry. Paging stays on ListObjectsV2 ContinuationToken and reuses get_folder_metadata rather than adding a second listing path. No core provider changes. get_folder_metadata set MaxKeys to the string '1000' on the single-page path. botocore validates parameter types before signing, so every paged folder listing raised ParamValidationError, which generate_generic_presigned_url converts into a 404: the file browser could not open any folder past the first page. The drain path never set MaxKeys, which is why the defect was invisible to everything except the UI. MaxKeys is sent as an int. The paging tests used to hand the provider a stand-in presigner that accepted any parameter and pasted it into a query string, so MaxKeys='1000' read as covered. They now sign with the real generate_generic_presigned_url and answer that exact URL, which needs the signing clock pinned so the registered URL and the one the provider builds are the same string. Only the clock is replaced -- parameter validation, the automatic encoding-type=url and the HMAC all still run.
…lder-id shapes
develop asserts that create_folder validates the path shape before it consults
folder_precheck, so a caller that opts out of the existence check still cannot
create a folder from a file path. Nothing on this branch pinned that ordering;
the case is restored.
generate_generic_presigned_url builds its endpoint from self.region, so the
region decides both the host that gets signed and the credential scope. Measured
in the pinned environment and now pinned as a test:
region unset / '' -> https://s3.amazonaws.com/... scope us-east-1
'us-east-1' -> https://s3.us-east-1.amazonaws.com scope us-east-1
'ap-northeast-1' -> https://s3.ap-northeast-1... scope ap-northeast-1
'eu-west-1' -> https://s3.eu-west-1... scope eu-west-1
A us-east-1 bucket answers GetBucketLocation with an empty LocationConstraint, so
region stays falsy for that bucket's whole lifetime and the global host is used.
_check_region rewrites the legacy 'EU' constraint to 'eu-west-1' before it reaches
the endpoint. The empty-constraint case of test_region_host was commented out and
is enabled again to hold that down.
addons.s3.models.NodeSettings.serialize_waterbutler_settings sends id='bucket:/'
when a bucket is selected with no prefix, which test_base_folder_parsing did not
cover. Added.
Error reporting. One helper, _raise_from_client_error, now stands at every site
that catches a botocore or make_request failure, and reports the exception's type
name and S3's error code and nothing else. The messages it replaces both carry
material that must not reach a client or a log: botocore quotes S3's prose with
its request and host ids, and core's exception_from_response quotes the request
URL, which under SigV4 is a presigned URL carrying X-Amz-Credential -- the access
key id -- and X-Amz-Signature. waterbutler.server.api.v1.core.write_error hands
exc.message to the client, so check_key_existence was answering 404 with the
signature in the body, and _chunked_upload was logging the same through repr().
The two conversion points also raise `from None`: leaving the original on
__context__ meant traceback.format_exception -- reached through log_exception's
exc_info -- printed the very message they refused to copy. The original is still
logged at both sites, by type and status.
The helper re-raises asyncio.CancelledError unchanged. Python 3.6 derives it
from Exception, so each of these broad excepts was reporting a cancelled request
as a provider failure and stopping the cancellation from propagating.
_chunked_upload gets its own guard because it aborts the session before it
reports. get_s3_bucket_object_location had no handler at all: a botocore error
left the provider as itself and the API layer could only answer 500 with no code,
and it is the first request of every operation, so that is the least informative
place to lose the error.
Response parsing. xmltodict keys elements by the name as written, so a listing
whose root is spelled <s3:ListBucketResult>, or a body that is not XML, left
doc.get() giving back {} -- indistinguishable from an empty bucket. A folder
would list as empty and a delete would report success having found no version to
remove. _parse_listing fails closed with a 502 instead.
The commit. S3 sends the status line before it starts assembling a multi-part
upload, so a commit that fails part way through arrives as 200 with an <Error>
body; expects=(200, 201) only looks at the status and read that as a completed
upload. The body is now read, and a commit counts as successful only when the
body says so -- a CompleteMultipartUploadResult carrying an ETag, which is
computed from the assembled object and therefore exists only once the assembly
finished. An empty or scalar <Error>, a body that is not XML, a truncated or
empty body, a result with no ETag and an unknown root element are all evidence
for neither outcome, so they raise rather than reporting a file that may not be
there.
The commit is now sent exactly once: retry=0 stops core's retry loop and
allow_redirects=False stops aiohttp following a 307/308 below it, and neither
substitutes for the other. The scope is the commit alone -- the session
creation, the part uploads and the abort keep core's defaults, where a re-send
changes nothing the caller reads. The accepted cost is that a 503 or 504 on the
commit reaches the user on the first attempt instead of the third; Complete is
not idempotent, and a second attempt after a first that actually succeeded
answers NoSuchUpload, which would be read as a definitive rejection.
A failed commit now tells the user which of two things happened: the file was not
saved, or it may have been saved and the file list should be checked first. The
verdict comes from the S3 error code alone -- a table of the codes that prove
nothing was assembled; every other code, and the case where no code could be
read, is unknown and carries the notice. The asymmetry is deliberate: an
over-reported notice costs a look at the file list, an under-reported one costs a
duplicate object that only an administrator can remove. The HTTP status class is
not consulted and cannot be, since a failed commit arrives as a 200. The table
is transcribed from MinIO measurements; only EntityTooSmall was actually observed
on a CompleteMultipartUpload, and reconciliation against AWS S3 is a separate
piece of work.
This ports RCOSDP#98's three-valued outcome, with two deviations.
RCOSDP#98 suppresses the notice for quota exhaustion, because "you are out of space"
and "it may have completed" contradict each other; s3 has no quota mechanism at
all -- it is absent from ADDON_METHOD_PROVIDER and from website/util/quota.py's
PROVIDERS -- so there is no quota response here to suppress. And the abort
outcome keeps its own wording, with the notice placed between the failure
sentence and it: "is the object there?" and "is there rubbish left behind?" are
different questions and the user needs both answers.
Finally, an abort that itself raised used to replace the verdict entirely.
_abort_chunked_upload returns False only when it read answers and parts were
still there; a DELETE answering 404, 403 or 500 comes back as an exception,
raised after the notice is computed and before it is used, so both the failure
sentence and the notice were discarded and the user heard only about the cleanup.
404 NoSuchUpload on the abort is the worst case and a real answer -- it is what
S3 says when the UploadId is already consumed, which is exactly when the user
most needs to go and look. The abort is wrapped, treated as not aborted, and all
three sentences are reported. CancelledError is re-raised first.
Also fixes a test-isolation defect this work uncovered: a failing
ClientSession._request installed through monkeypatch was undone at teardown,
after the conftest hook had already deactivated aiohttpretty, which put
aiohttpretty's fake back permanently for every later test that needs a real
socket. It is restored inside the test body instead.
aiobotocore 1.2.2 requires botocore<1.19.53,>=1.19.52 -- a one-version window, because it reaches into botocore internals rather than using the public API. Nothing in this file held pip inside that window: resolving it installed botocore 1.19.63, and `pip check` named the conflict on every build. The s3 provider drives botocore through aiobotocore for presigned URLs and for CopyObject, so the window is not advisory. Running outside it means the provider's behaviour depends on whichever botocore pip happened to pick, which is exactly the kind of drift a pinned requirements file exists to stop. Measured in the rebuilt pinned image (Python 3.6.15): before botocore==1.19.63 pip check: 15 violations, 1 of them botocore after botocore==1.19.52 pip check: 14 violations, 0 of them botocore The 14 that remain predate this branch and are unrelated -- pbr/stevedore versions pulled in by the keystone and oslo packages, markupsafe under jinja2 3.0.3, aws-sam-translator under cfn-lint, and furl/yarl under aiohttpretty. They are recorded rather than fixed: each one belongs to a dependency this branch does not touch. No behavioural difference. The full suite is identical on both images: 2169 passed / 7 skipped, flake8 clean.
…sponse declares
A presigned URL already carries the parameters it was signed over, and aiohttp's
ClientRequest extends a URL's query with make_request's `params=` rather than
replacing it. _upload_part passed partNumber and uploadId both ways, so both
went out twice, the canonical query no longer matched the signature and S3
answered SignatureDoesNotMatch: every chunked upload past the first part failed,
which means no file large enough to be uploaded in parts could be stored.
_abort_chunked_upload and _list_uploaded_chunks passed `params=headers`, an empty
dict left over from a copy-paste -- it added nothing, but it is the same mistake
one value away from being live, so it goes too.
aiohttpretty replaces ClientSession._request, which sits above that merge, so it
cannot show the duplicate. The tests for this put the presigner's own URL on a
loopback socket -- only the origin is rewritten, since the signed endpoint is
hard-coded to amazonaws.com -- and read the query string the server actually
received.
Listing decode. botocore asks every listing for encoding-type=url, so key names
arrive percent-encoded; but the unconditional unquote() corrupted any key
containing a "%", and replace('+', ' ') corrupted "a+b" -- under encoding-type=url
a plus is "%2B" and a space is "%20", so there is no "+" to translate.
_decoder_for() now reads EncodingType off the listing itself: url means unquote
the keys, the prefixes and the key markers; anything else means the names are
already verbatim. Markers are decoded for the same reason they are read --
KeyMarker goes back out as a query parameter and the signer encodes it again, so
leaving it encoded asked S3 for key-marker=f%252Fa%252Bb and the second page
never came back. The decoded key set matches what develop's boto2 path returned.
NextVersionIdMarker is the exception and is resumed verbatim. EncodingType=url
encodes only what S3 derived from a key name; a version id is an opaque
identifier S3 minted itself and comes back unencoded, so decoding it asks the
next page to resume from a version that does not exist.
metadata.download_file passes accept_url=('direct' not in query) and redirects
when the return value is a str. This branch ignored that and always streamed
bytes, so every download went through WaterButler instead of being handed off to
S3 -- a change from develop's default behaviour, and one that puts the whole file
through the server for no reason.
download(accept_url=True) returns a presigned URL again. It is signed with the
same parameters as the streaming path (VersionId and ResponseContentDisposition)
and expires with TEMP_URL_SECS. `range` is deliberately not applied: when the
client follows the redirect it re-sends its own Range to S3.
The `?direct` path is unchanged and still streams. The tests drive the real
presigner and compare scheme, host, path and parsed query rather than the URL
string, since the order of the query parameters is not part of the contract, and
assert that no HTTP request is made at all on the accept_url path. The former
test_accepts_url, which never passed accept_url, is replaced.
These hooks are part of COS's Celery serialization work (ENG-7534, WB Upgrade) and have nothing to do with SigV4. GRDM has no caller for them, and `rehydrate` took the `cls` string out of a payload and handed it to importlib.import_module plus getattr, constructing an arbitrary class from data. Following COS here belongs to the separate ticket that updates that base, not to this branch. - waterbutler/core/metadata.py: drop dehydrate / _dehydrate / rehydrate / _rehydrate and the importlib import. core/metadata.py is now identical to develop again, which leaves core/provider.py's file_size hand-off as this branch's only core change. - waterbutler/providers/s3/metadata.py: drop S3FileMetadataHeaders' _dehydrate / _rehydrate overrides, since the base they override is gone -- leaving an override behind would raise the moment it was called, because there would be no super() to reach. Regression guards are added on both sides so that a later merge cannot bring them back unnoticed: BaseMetadata must not carry the four names and waterbutler.core.metadata must not import importlib; S3Metadata and S3FileMetadataHeaders must not carry them either. Measured: 2219 passed / 7 skipped, flake8 clean over core, the s3 provider and both test trees.
…eport folder-delete listing errors - intra_copy caught only ClientError, so failures where S3 never answered -- EndpointConnectionError, ReadTimeoutError, ParamValidationError, aiohttp's ClientPayloadError -- escaped unconverted. The except is widened to Exception, which brings all six aiobotocore call sites into line with each other. - _parse_listing now normalizes the element type of Contents, CommonPrefixes, Version and DeleteMarker: xmltodict gives back a dict for a single element and a list for several, and anything that is neither a dict nor a list of dicts is a 502. Iterating a string instead raised AttributeError part way through. The callers' scattered isinstance corrections are removed. _next_marker is added alongside it, and a page claiming IsTruncated=true with no marker is a 502 as well: a folder listing used to re-send the identical request forever, and a version listing used to stop after one page, which silently left objects undeleted. - _delete_folder's listing failures are converted to DeleteError naming the exception type and the error code. core returns a failure safely only when it carries no data, and here S3's XML body was being carried in the message. - The per-call ERROR log in _upload_parts is removed; it fired on every healthy multipart upload and said nothing but the method's own name. - `# GRDM:` markers are added to the places this branch deliberately differs from upstream: can_intra_copy/can_intra_move, the version-listing continuation, _get_base_folder, the four regional endpoint_url sites and the Bucket default. TestCRUD.test_folder_delete_listing_error is removed because its contract changed here, and is replaced by a test on the error-reporting side that goes through the real presigner; a pointer comment is left where it was.
… at the HTTP boundary
The suite replaced the thing it was meant to measure. The provider fixture
installed hand-written generate_generic_presigned_url and check_key_existence:
the first built https://<bucket>.s3.amazonaws.com/<key> from the path alone,
ignoring the operation and the parameters, and the second re-made the same
string. A second stand-in "presigner" merely urlencoded the query. botocore
therefore never ran, and three of this branch's defects lived in exactly that
gap -- a wrongly typed MaxKeys that botocore rejects before signing, parameters
folded into the signed URL and then added a second time on the wire, and the
encoding-type=url that makes S3 return keys percent-encoded. Each one read as
covered.
The stand-ins are gone. The fixture now pins only the region; a module-wide
autouse fixture freezes the clock botocore signs with, which is what lets a test
name the signed URL in advance, so register_presigned signs the operation for
real and registers that exact URL, signature included. match_querystring=False
is no longer needed anywhere -- the whole URL matches, which is itself the
evidence that the presigner ran. The opt-in helper and its eighteen call sites
go, along with the now-unused URL builders, and the fixture delegates to the raw
one so there is a single construction rather than two.
patch_aiobotocore_client no longer replaces create_client with a mock: it builds
the real client and shadows only the named API method on the instance. Every
remaining get_session patch goes through the real session, including
test_intra_copy, which was the last place a fabricated object was handed in --
meaning the CopySource the provider assembles had been asserted against a client
that would have accepted any shape. The delete and copy expectations move to
botocore's before-send event, so the Delete payload is read back out of the
rest-xml body and the copy source out of the x-amz-copy-source header botocore
renders; one test counts the dispatched operations so that a client-level stub
reappearing there shows up as a zero. Failure injection keeps the client-level
shadow, where shadowing the call is the point, and says so.
test_metadata_file_missing changes the exception it expects, from MetadataError
to NotFoundError. MetadataError was the stand-in's: it called
make_request(throws=...) and stopped. The real check_key_existence wraps that
call and re-raises as NotFoundError, because BaseProvider.exists reads a
NotFoundError of any status as "no" and every caller arrives through it. One
substitution survives: a test cannot listen on s3.amazonaws.com, so the real
presigner runs and only the scheme and host of its output are rewritten -- path,
query and signature are kept whole.
Three behaviours were only ever measured through a substitute and are now
covered for real: an undecodable commit answer, where core's
exception_from_response raises UnicodeDecodeError out of data.decode('utf-8')
before the body is ever parsed; dropping the version-id marker between pages,
which needs three pages so that one boundary repeats it and the next does not;
and a partial DeleteObjects refusal in the second batch rather than the first,
which a loop that checks only one response would miss -- 1001 objects put it
there, serialised, signed and parsed by botocore.
Finally, the chunked-upload query parameters are counted with the case of the
name folded away. parse_qs keys on the exact spelling, so asserting
query['partNumber'] == ['1'] says nothing about a second copy sent as
'PartNumber' -- two entries in the canonical query string, the same
SignatureDoesNotMatch, and it read as a pass. 'uploads' had no test of its own:
it is the marker that makes the POST an initiate and carries no value, so a
duplicate of it is invisible to any check that reads the value.
tishin-endou
force-pushed
the
feature/s3-sigv4
branch
from
October 1, 2026 08:44
cf2f87a to
7f771cd
Compare
Author
|
レビューありがとうございます。ご指摘の 3 点をすべて対応し、あわせて PR 全体を見直しました。対応の要点をまとめます(詳細は PR 本文を更新しました)。 1. PR #77 の Version / DeleteMarker 全削除と 1000 件超対応の消失 — 復元しました。 2. PR #53 の intra copy / move のサイズ制限の消失 — develop の現行実装(PR #97 の 3. 内部ヘルパーをモックして呼び出しだけを確認するテスト — すべて書き直しました。presigner や SDK メソッドの差し替えをやめ、実 presigner が生成した URL に対する HTTP 境界( 見直しで見つかった追加の修正 — テストを実経路に戻した結果、以下が見つかり修正しました: フォルダ一覧の 実機確認 — AWS S3(us-east-1 / ap-northeast-1)に対し、本 PR と RDM-osf.io#746 を同時にデプロイした検証環境で E2E を実施しました(手順は RDM-e2e-test-nb#41)。機関ストレージの登録・アップロード・ダウンロード(presigned URL への 302)、1500 件フォルダのページング(1000+500、欠落なし)、バージョニング有効バケットでの削除(Versions 3 → 0 / DeleteMarkers 0)、同一ストレージ内の copy / move、200MB のマルチパートアップロード(4 パート)を確認しています。 既知の限界 — 旧 なお、レビュー以降のコミットは読みやすいよう論理単位に整理しました(整理前 cf2f87a と整理後 7f771cd のツリーは同一で、差分は変わっていません)。CI は RDM-waterbutler / RDM-osf.io(#746、develop を取り込み済み)とも success です。再レビューをお願いします。 |
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
3 participants
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.
概要
S3 アドオンのプロバイダを boto2 / SigV2 から aiobotocore / SigV4 に移行します。実装は CenterForOpenScience/osf.io の S3 プロバイダ(SigV4 対応済み)を基底とし、GRDM 固有の仕様を差分として重ねています(
# GRDM:コメントで明示、33 箇所)。RDM-osf.io#746 と一体で動作します。GRDM 固有仕様(develop から引き継ぎ)
KeyMarker/VersionIdMarkerでページングACCEPTS_FILE_SIZE_FOR_INTRA、5GB)next_token(UI の遅延読み込み用)accept_urlによる presigned URL への直接ダウンロード(302)bucket:/prefix/形式の folder_id(旧形式bucketも許容)、リージョン別エンドポイントの固定、created/modifiedaiobotocore==1.2.2/botocore==1.19.52の固定(aiobotocore が対応する版に合わせる)堅牢化(旧 s3compatsigv4 プロバイダで確立した対策の適用)
CompleteMultipartUploadは応答本文が成功を示すときだけ成功。明確な拒否コードは失敗、それ以外は「完了している可能性がある」注記付きの失敗(3 値モデル)。commit 要求はリトライせず 1 回だけ送るfrom Noneとし、資格情報・署名を例外本文に出さないテスト方針
実機確認
AWS S3(us-east-1、ap-northeast-1)で E2E 実施(RDM-e2e-test-nb#41)。登録・アップロード・presigned 302 ダウンロード、1500 件ページング、バージョニング有効バケットでの全版削除、copy / move、200MB マルチパート、
CompleteMultipartUpload失敗コードの採取。コミットの構成
レビュー(2026-08-10)以降のコミットは論理単位に整理しました(整理前 cf2f87a と整理後のツリーは同一です)。
bdf20aa1〜446fdc13: SigV4 移行の初版(レビュー時点の内容)d36a23c1: develop(004f407)の取り込みb08dbe74develop のcan_intra_*回帰テスト復元、コメントアウト済み旧実装の削除558f2702削除時の全 Version / DeleteMarker 削除(PR [GRDM-50089] Fixing errors when versioning is enabled on S3 compatible storage #77)、ListObjectVersionsのページングd3ad653bintra copy / move を develop のサイズ上限に揃え、宛先の資格情報で署名(PR Hotfix/s3compat disabled intra copy #53 / Feature/nii grdm 202510 step2/2.1 improve multiple small file uploads rebase #97)a9cc4fccフォルダ一覧の 1 ページ返却(next_token)、MaxKeysを int で送るbeb01515create_folderの precheck ケース復元、エンドポイント・folder_id 形式のテスト9c4b2279エラー報告と multipart commit の堅牢化(資格情報を出さない、commit は 1 回、成功は本文で判定、不明は注記、abort 失敗時も判定保持)1a416210botocore を aiobotocore の対応版に固定df0c3f0fpresigned パラメータの重複送信を解消、EncodingTypeに従った復号(VersionId は復号しない)9718a92daccept_urlによる presigned URL への直接ダウンロードの復元90c8381cdehydrate / rehydrate(SigV4 と無関係な直列化基盤)の撤去、core/は develop と差分なし10731ab7転送系失敗の変換漏れ、一覧応答の形の正規化、フォルダ削除時の一覧失敗の報告7f771cd5全テストを実 presigner 経由にし、注入は HTTP 境界のみに関連