diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 0f534f0c5..33a9c0b0e 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -615,7 +615,7 @@ From RFC 7231:: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Advise the server the data read from the indicated share was corrupt. -The request body includes an human-meaningful string with details about the corruption. +The request body includes an human-meaningful (Unicode) string with details about the corruption. It also includes potentially important details about the share. For example:: @@ -624,6 +624,8 @@ For example:: .. share-type, storage-index, and share-number are inferred from the URL +The response code is OK, or 404 not found if the share couldn't be found. + Reading ~~~~~~~ diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 1610f5433..d0ae4b584 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -354,3 +354,31 @@ class StorageClientImmutables(object): return else: raise ClientException(response.code) + + @inlineCallbacks + def advise_corrupt_share( + self, + storage_index: bytes, + share_number: int, + reason: str, + ): + """Indicate a share has been corrupted, with a human-readable message.""" + assert isinstance(reason, str) + url = self._client.relative_url( + "/v1/immutable/{}/{}/corrupt".format( + _encode_si(storage_index), share_number + ) + ) + message = dumps({"reason": reason}) + response = yield self._client.request( + "POST", + url, + data=message, + headers=Headers({"content-type": ["application/cbor"]}), + ) + if response.code == http.OK: + return + else: + raise ClientException( + response.code, + ) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 2deb03dfd..5d357b846 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -455,3 +455,22 @@ class HTTPServer(object): request.setResponseCode(http.NO_CONTENT) return b"" + + @_authorized_route( + _app, + set(), + "/v1/immutable///corrupt", + methods=["POST"], + ) + def advise_corrupt_share(self, request, authorization, storage_index, share_number): + """Indicate that given share is corrupt, with a text reason.""" + # TODO 3879 test success path + try: + bucket = self._storage_server.get_buckets(storage_index)[share_number] + except KeyError: + # TODO 3879 test this path + raise _HTTPError(http.NOT_FOUND) + + info = loads(request.content.read()) + bucket.advise_corrupt_share(info["reason"].encode("utf-8")) + return b"" diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 0add9806b..7ef7b4d37 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -743,8 +743,9 @@ class StorageServer(service.MultiService): def advise_corrupt_share(self, share_type, storage_index, shnum, reason): - # This is a remote API, I believe, so this has to be bytes for legacy - # protocol backwards compatibility reasons. + # Previously this had to be bytes for legacy protocol backwards + # compatibility reasons. Now that Foolscap layer has been abstracted + # out, we can probably refactor this to be unicode... assert isinstance(share_type, bytes) assert isinstance(reason, bytes), "%r is not bytes" % (reason,) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index ac74f6c67..55b6cfb05 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -77,7 +77,7 @@ from allmydata.util.hashutil import permute_server_hash from allmydata.util.dictutil import BytesKeyDict, UnicodeKeyDict from allmydata.storage.http_client import ( StorageClient, StorageClientImmutables, StorageClientGeneral, - ClientException as HTTPClientException, + ClientException as HTTPClientException ) @@ -1094,7 +1094,10 @@ class _HTTPBucketReader(object): ) def advise_corrupt_share(self, reason): - pass # TODO in later ticket + return self.client.advise_corrupt_share( + self.storage_index, self.share_number, + str(reason, "utf-8", errors="backslashreplace") + ) # WORK IN PROGRESS, for now it doesn't actually implement whole thing. @@ -1124,7 +1127,7 @@ class _HTTPStorageServer(object): cancel_secret, sharenums, allocated_size, - canary, + canary ): upload_secret = urandom(20) immutable_client = StorageClientImmutables(self._http_client) @@ -1148,7 +1151,7 @@ class _HTTPStorageServer(object): @defer.inlineCallbacks def get_buckets( self, - storage_index, + storage_index ): immutable_client = StorageClientImmutables(self._http_client) share_numbers = yield immutable_client.list_shares( @@ -1165,9 +1168,24 @@ class _HTTPStorageServer(object): self, storage_index, renew_secret, - cancel_secret, + cancel_secret ): immutable_client = StorageClientImmutables(self._http_client) return immutable_client.add_or_renew_lease( storage_index, renew_secret, cancel_secret ) + + def advise_corrupt_share( + self, + share_type, + storage_index, + shnum, + reason: bytes + ): + if share_type == b"immutable": + imm_client = StorageClientImmutables(self._http_client) + return imm_client.advise_corrupt_share( + storage_index, shnum, str(reason, "utf-8", errors="backslashreplace") + ) + else: + raise NotImplementedError() # future tickets diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index e85047081..253ff6046 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -1149,12 +1149,6 @@ class HTTPImmutableAPIsTests( ): """HTTP-specific tests for immutable ``IStorageServer`` APIs.""" - # These will start passing in future PRs as HTTP protocol is implemented. - SKIP_TESTS = { - "test_advise_corrupt_share", - "test_bucket_advise_corrupt_share", - } - class FoolscapMutableAPIsTests( _FoolscapMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase