From 61a20e245029ecfa626aa5f522af2eb08b7e19d3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 22 Oct 2021 10:10:53 -0400 Subject: [PATCH 01/89] Add concept of upload secret to immutable uploads. --- docs/proposed/http-storage-node-protocol.rst | 33 +++++++++++++++++--- newsfragments/3820.minor | 0 2 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 newsfragments/3820.minor diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 521bf476d..16db0fed9 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -451,6 +451,7 @@ Details of the buckets to create are encoded in the request body. For example:: {"renew-secret": "efgh", "cancel-secret": "ijkl", + "upload-secret": "xyzf", "share-numbers": [1, 7, ...], "allocated-size": 12345} The response body includes encoded information about the created buckets. @@ -458,6 +459,8 @@ For example:: {"already-have": [1, ...], "allocated": [7, ...]} +The session secret is an opaque _byte_ string. + Discussion `````````` @@ -482,6 +485,13 @@ The response includes ``already-have`` and ``allocated`` for two reasons: This might be because a server has become unavailable and a remaining server needs to store more shares for the upload. It could also just be that the client's preferred servers have changed. +Regarding upload secrets, +the goal is for uploading and aborting (see next sections) to be authenticated by more than just the storage index. +In the future, we will want to generate them in a way that allows resuming/canceling when the client has issues. +In the short term, they can just be a random byte string. +The key security constraint is that each upload to each server has its own, unique upload key, +tied to uploading that particular storage index to this particular server. + ``PATCH /v1/immutable/:storage_index/:share_number`` !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! @@ -498,6 +508,12 @@ If any one of these requests fails then at most 128KiB of upload work needs to b The server must recognize when all of the data has been received and mark the share as complete (which it can do because it was informed of the size when the storage index was initialized). +The request body looks this, with data and upload secret being bytes:: + + { "upload-secret": "xyzf", "data": "thedata" } + +Responses: + * When a chunk that does not complete the share is successfully uploaded the response is ``OK``. The response body indicates the range of share data that has yet to be uploaded. That is:: @@ -522,6 +538,10 @@ The server must recognize when all of the data has been received and mark the sh This cancels an *in-progress* upload. +The request body looks this:: + + { "upload-secret": "xyzf" } + The response code: * When the upload is still in progress and therefore the abort has succeeded, @@ -695,6 +715,7 @@ Immutable Data POST /v1/immutable/AAAAAAAAAAAAAAAA {"renew-secret": "efgh", "cancel-secret": "ijkl", + "upload-secret": "xyzf", "share-numbers": [1, 7], "allocated-size": 48} 200 OK @@ -704,25 +725,29 @@ Immutable Data PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 0-15/48 - + + {"upload-secret": b"xyzf", "data": "first 16 bytes!!" 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 16-31/48 - + + {"upload-secret": "xyzf", "data": "second 16 bytes!" 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 32-47/48 - + + {"upload-secret": "xyzf", "data": "final 16 bytes!!" 201 CREATED #. Download the content of the previously uploaded immutable share ``7``:: - GET /v1/immutable/AAAAAAAAAAAAAAAA?share=7&offset=0&size=48 + GET /v1/immutable/AAAAAAAAAAAAAAAA?share=7 + Range: bytes=0-47 200 OK diff --git a/newsfragments/3820.minor b/newsfragments/3820.minor new file mode 100644 index 000000000..e69de29bb From e0c8bab5d7a97d539a5364c962cd5861430432a0 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 22 Oct 2021 10:32:44 -0400 Subject: [PATCH 02/89] Add proposal on how to generate upload secret. --- docs/proposed/http-storage-node-protocol.rst | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 16db0fed9..d5b6653be 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -459,7 +459,13 @@ For example:: {"already-have": [1, ...], "allocated": [7, ...]} -The session secret is an opaque _byte_ string. +The uplaod secret is an opaque _byte_ string. +It will be generated by hashing a combination of:b + +1. A tag. +2. The storage index, so it's unique across different source files. +3. The server ID, so it's unique across different servers. +4. The convergence secret, so that servers can't guess the upload secret for other servers. Discussion `````````` @@ -492,6 +498,13 @@ In the short term, they can just be a random byte string. The key security constraint is that each upload to each server has its own, unique upload key, tied to uploading that particular storage index to this particular server. +Rejected designs for upload secrets: + +* Upload secret per share number. + In order to make the secret unguessable by attackers, which includes other servers, + it must contain randomness. + Randomness means there is no need to have a secret per share, since adding share-specific content to randomness doesn't actually make the secret any better. + ``PATCH /v1/immutable/:storage_index/:share_number`` !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! From 125c937d466db13e27ab06cc40e5d68aa5d93d28 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 28 Oct 2021 10:49:08 -0400 Subject: [PATCH 03/89] Switch to HTTP header scheme. --- docs/proposed/http-storage-node-protocol.rst | 38 ++++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index d5b6653be..fd1db5c4c 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -450,16 +450,22 @@ A lease is also created for the shares. Details of the buckets to create are encoded in the request body. For example:: - {"renew-secret": "efgh", "cancel-secret": "ijkl", - "upload-secret": "xyzf", - "share-numbers": [1, 7, ...], "allocated-size": 12345} + {"share-numbers": [1, 7, ...], "allocated-size": 12345} + +The request must include ``WWW-Authenticate`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. +Typically this is a header sent by the server, but in Tahoe-LAFS keys are set by the client, so may as well reuse it. +For example:: + + WWW-Authenticate: x-tahoe-renew-secret + WWW-Authenticate: x-tahoe-cancel-secret + WWW-Authenticate: x-tahoe-upload-secret The response body includes encoded information about the created buckets. For example:: {"already-have": [1, ...], "allocated": [7, ...]} -The uplaod secret is an opaque _byte_ string. +The upload secret is an opaque _byte_ string. It will be generated by hashing a combination of:b 1. A tag. @@ -521,9 +527,9 @@ If any one of these requests fails then at most 128KiB of upload work needs to b The server must recognize when all of the data has been received and mark the share as complete (which it can do because it was informed of the size when the storage index was initialized). -The request body looks this, with data and upload secret being bytes:: +The request must include a ``Authorization`` header that includes the upload secret:: - { "upload-secret": "xyzf", "data": "thedata" } + Authorization: x-tahoe-upload-secret Responses: @@ -727,9 +733,11 @@ Immutable Data 1. Create a bucket for storage index ``AAAAAAAAAAAAAAAA`` to hold two immutable shares, discovering that share ``1`` was already uploaded:: POST /v1/immutable/AAAAAAAAAAAAAAAA - {"renew-secret": "efgh", "cancel-secret": "ijkl", - "upload-secret": "xyzf", - "share-numbers": [1, 7], "allocated-size": 48} + WWW-Authenticate: x-tahoe-renew-secret efgh + WWW-Authenticate: x-tahoe-cancel-secret jjkl + WWW-Authenticate: x-tahoe-upload-secret xyzf + + {"share-numbers": [1, 7], "allocated-size": 48} 200 OK {"already-have": [1], "allocated": [7]} @@ -738,22 +746,22 @@ Immutable Data PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 0-15/48 - - {"upload-secret": b"xyzf", "data": "first 16 bytes!!" + Authorization: x-tahoe-upload-secret xyzf + 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 16-31/48 - - {"upload-secret": "xyzf", "data": "second 16 bytes!" + Authorization: x-tahoe-upload-secret xyzf + 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 Content-Range: bytes 32-47/48 - - {"upload-secret": "xyzf", "data": "final 16 bytes!!" + Authorization: x-tahoe-upload-secret xyzf + 201 CREATED From 54bf271fbebdb7d63642cc4d86dcf9507ae839df Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 11:12:08 -0400 Subject: [PATCH 04/89] news fragment --- newsfragments/3833.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3833.minor diff --git a/newsfragments/3833.minor b/newsfragments/3833.minor new file mode 100644 index 000000000..e69de29bb From 66845c9a1786778e145aab65e30fb0068e2f8245 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 11:12:20 -0400 Subject: [PATCH 05/89] Add ShareFile.is_valid_header and use it instead of manual header inspection --- src/allmydata/scripts/debug.py | 2 +- src/allmydata/storage/immutable.py | 15 +++++++++++++++ src/allmydata/storage/server.py | 2 +- src/allmydata/test/test_system.py | 8 ++++---- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/allmydata/scripts/debug.py b/src/allmydata/scripts/debug.py index 4d3f4cb21..71e1ccb41 100644 --- a/src/allmydata/scripts/debug.py +++ b/src/allmydata/scripts/debug.py @@ -795,7 +795,7 @@ def describe_share(abs_sharefile, si_s, shnum_s, now, out): else: print("UNKNOWN mutable %s" % quote_output(abs_sharefile), file=out) - elif struct.unpack(">L", prefix[:4]) == (1,): + elif ShareFile.is_valid_header(prefix): # immutable class ImmediateReadBucketProxy(ReadBucketProxy): diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 7712e568a..407116038 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -57,6 +57,21 @@ class ShareFile(object): LEASE_SIZE = struct.calcsize(">L32s32sL") sharetype = "immutable" + @classmethod + def is_valid_header(cls, header): + # (bytes) -> bool + """ + Determine if the given bytes constitute a valid header for this type of + container. + + :param header: Some bytes from the beginning of a container. + + :return: ``True`` if the bytes could belong to this container, + ``False`` otherwise. + """ + (version,) = struct.unpack(">L", header[:4]) + return version == 1 + def __init__(self, filename, max_size=None, create=False): """ If max_size is not None then I won't allow more than max_size to be written to me. If create=True and max_size must not be None. """ precondition((max_size is not None) or (not create), max_size, create) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 041783a4e..f339c579b 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -378,7 +378,7 @@ class StorageServer(service.MultiService, Referenceable): # note: if the share has been migrated, the renew_lease() # call will throw an exception, with information to help the # client update the lease. - elif header[:4] == struct.pack(">L", 1): + elif ShareFile.is_valid_header(header): sf = ShareFile(filename) else: continue # non-sharefile diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index 087a1c634..72ce4b6ec 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -22,7 +22,7 @@ from twisted.trial import unittest from twisted.internet import defer from allmydata import uri -from allmydata.storage.mutable import MutableShareFile +from allmydata.storage.mutable import ShareFile, MutableShareFile from allmydata.storage.server import si_a2b from allmydata.immutable import offloaded, upload from allmydata.immutable.literal import LiteralFileNode @@ -1290,9 +1290,9 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): # are sharefiles here filename = os.path.join(dirpath, filenames[0]) # peek at the magic to see if it is a chk share - magic = open(filename, "rb").read(4) - if magic == b'\x00\x00\x00\x01': - break + with open(filename, "rb") as f: + if ShareFile.is_valid_header(f.read(32)): + break else: self.fail("unable to find any uri_extension files in %r" % self.basedir) From 1b46ac7a241e719cc0d7ddc4b66fa9fcdca5992d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 11:38:18 -0400 Subject: [PATCH 06/89] add MutableShareFile.is_valid_header and use it --- src/allmydata/scripts/debug.py | 287 ++++++++++++++--------------- src/allmydata/storage/immutable.py | 2 +- src/allmydata/storage/mutable.py | 18 +- src/allmydata/storage/server.py | 2 +- src/allmydata/storage/shares.py | 3 +- src/allmydata/test/common.py | 2 +- src/allmydata/test/test_system.py | 3 +- 7 files changed, 163 insertions(+), 154 deletions(-) diff --git a/src/allmydata/scripts/debug.py b/src/allmydata/scripts/debug.py index 71e1ccb41..ab48b0fd0 100644 --- a/src/allmydata/scripts/debug.py +++ b/src/allmydata/scripts/debug.py @@ -15,15 +15,22 @@ try: except ImportError: pass - -# do not import any allmydata modules at this level. Do that from inside -# individual functions instead. import struct, time, os, sys + from twisted.python import usage, failure from twisted.internet import defer from foolscap.logging import cli as foolscap_cli -from allmydata.scripts.common import BaseOptions +from allmydata.scripts.common import BaseOptions +from allmydata import uri +from allmydata.storage.mutable import MutableShareFile +from allmydata.storage.immutable import ShareFile +from allmydata.mutable.layout import unpack_share +from allmydata.mutable.layout import MDMFSlotReadProxy +from allmydata.mutable.common import NeedMoreDataError +from allmydata.immutable.layout import ReadBucketProxy +from allmydata.util import base32 +from allmydata.util.encodingutil import quote_output class DumpOptions(BaseOptions): def getSynopsis(self): @@ -56,13 +63,11 @@ def dump_share(options): # check the version, to see if we have a mutable or immutable share print("share filename: %s" % quote_output(options['filename']), file=out) - f = open(options['filename'], "rb") - prefix = f.read(32) - f.close() - if prefix == MutableShareFile.MAGIC: - return dump_mutable_share(options) - # otherwise assume it's immutable - return dump_immutable_share(options) + with open(options['filename'], "rb") as f: + if MutableShareFile.is_valid_header(f.read(32)): + return dump_mutable_share(options) + # otherwise assume it's immutable + return dump_immutable_share(options) def dump_immutable_share(options): from allmydata.storage.immutable import ShareFile @@ -712,125 +717,115 @@ def call(c, *args, **kwargs): return results[0] def describe_share(abs_sharefile, si_s, shnum_s, now, out): - from allmydata import uri - from allmydata.storage.mutable import MutableShareFile - from allmydata.storage.immutable import ShareFile - from allmydata.mutable.layout import unpack_share - from allmydata.mutable.common import NeedMoreDataError - from allmydata.immutable.layout import ReadBucketProxy - from allmydata.util import base32 - from allmydata.util.encodingutil import quote_output - import struct - - f = open(abs_sharefile, "rb") - prefix = f.read(32) - - if prefix == MutableShareFile.MAGIC: - # mutable share - m = MutableShareFile(abs_sharefile) - WE, nodeid = m._read_write_enabler_and_nodeid(f) - data_length = m._read_data_length(f) - expiration_time = min( [lease.get_expiration_time() - for (i,lease) in m._enumerate_leases(f)] ) - expiration = max(0, expiration_time - now) - - share_type = "unknown" - f.seek(m.DATA_OFFSET) - version = f.read(1) - if version == b"\x00": - # this slot contains an SMDF share - share_type = "SDMF" - elif version == b"\x01": - share_type = "MDMF" - - if share_type == "SDMF": - f.seek(m.DATA_OFFSET) - data = f.read(min(data_length, 2000)) - - try: - pieces = unpack_share(data) - except NeedMoreDataError as e: - # retry once with the larger size - size = e.needed_bytes - f.seek(m.DATA_OFFSET) - data = f.read(min(data_length, size)) - pieces = unpack_share(data) - (seqnum, root_hash, IV, k, N, segsize, datalen, - pubkey, signature, share_hash_chain, block_hash_tree, - share_data, enc_privkey) = pieces - - print("SDMF %s %d/%d %d #%d:%s %d %s" % \ - (si_s, k, N, datalen, - seqnum, str(base32.b2a(root_hash), "utf-8"), - expiration, quote_output(abs_sharefile)), file=out) - elif share_type == "MDMF": - from allmydata.mutable.layout import MDMFSlotReadProxy - fake_shnum = 0 - # TODO: factor this out with dump_MDMF_share() - class ShareDumper(MDMFSlotReadProxy): - def _read(self, readvs, force_remote=False, queue=False): - data = [] - for (where,length) in readvs: - f.seek(m.DATA_OFFSET+where) - data.append(f.read(length)) - return defer.succeed({fake_shnum: data}) - - p = ShareDumper(None, "fake-si", fake_shnum) - def extract(func): - stash = [] - # these methods return Deferreds, but we happen to know that - # they run synchronously when not actually talking to a - # remote server - d = func() - d.addCallback(stash.append) - return stash[0] - - verinfo = extract(p.get_verinfo) - (seqnum, root_hash, salt_to_use, segsize, datalen, k, N, prefix, - offsets) = verinfo - print("MDMF %s %d/%d %d #%d:%s %d %s" % \ - (si_s, k, N, datalen, - seqnum, str(base32.b2a(root_hash), "utf-8"), - expiration, quote_output(abs_sharefile)), file=out) + with open(abs_sharefile, "rb") as f: + prefix = f.read(32) + if MutableShareFile.is_valid_header(prefix): + _describe_mutable_share(abs_sharefile, f, now, si_s, out) + elif ShareFile.is_valid_header(prefix): + _describe_immutable_share(abs_sharefile, now, si_s, out) else: - print("UNKNOWN mutable %s" % quote_output(abs_sharefile), file=out) + print("UNKNOWN really-unknown %s" % quote_output(abs_sharefile), file=out) - elif ShareFile.is_valid_header(prefix): - # immutable +def _describe_mutable_share(abs_sharefile, f, now, si_s, out): + # mutable share + m = MutableShareFile(abs_sharefile) + WE, nodeid = m._read_write_enabler_and_nodeid(f) + data_length = m._read_data_length(f) + expiration_time = min( [lease.get_expiration_time() + for (i,lease) in m._enumerate_leases(f)] ) + expiration = max(0, expiration_time - now) - class ImmediateReadBucketProxy(ReadBucketProxy): - def __init__(self, sf): - self.sf = sf - ReadBucketProxy.__init__(self, None, None, "") - def __repr__(self): - return "" - def _read(self, offset, size): - return defer.succeed(sf.read_share_data(offset, size)) + share_type = "unknown" + f.seek(m.DATA_OFFSET) + version = f.read(1) + if version == b"\x00": + # this slot contains an SMDF share + share_type = "SDMF" + elif version == b"\x01": + share_type = "MDMF" - # use a ReadBucketProxy to parse the bucket and find the uri extension - sf = ShareFile(abs_sharefile) - bp = ImmediateReadBucketProxy(sf) + if share_type == "SDMF": + f.seek(m.DATA_OFFSET) + data = f.read(min(data_length, 2000)) - expiration_time = min( [lease.get_expiration_time() - for lease in sf.get_leases()] ) - expiration = max(0, expiration_time - now) + try: + pieces = unpack_share(data) + except NeedMoreDataError as e: + # retry once with the larger size + size = e.needed_bytes + f.seek(m.DATA_OFFSET) + data = f.read(min(data_length, size)) + pieces = unpack_share(data) + (seqnum, root_hash, IV, k, N, segsize, datalen, + pubkey, signature, share_hash_chain, block_hash_tree, + share_data, enc_privkey) = pieces - UEB_data = call(bp.get_uri_extension) - unpacked = uri.unpack_extension_readable(UEB_data) + print("SDMF %s %d/%d %d #%d:%s %d %s" % \ + (si_s, k, N, datalen, + seqnum, str(base32.b2a(root_hash), "utf-8"), + expiration, quote_output(abs_sharefile)), file=out) + elif share_type == "MDMF": + fake_shnum = 0 + # TODO: factor this out with dump_MDMF_share() + class ShareDumper(MDMFSlotReadProxy): + def _read(self, readvs, force_remote=False, queue=False): + data = [] + for (where,length) in readvs: + f.seek(m.DATA_OFFSET+where) + data.append(f.read(length)) + return defer.succeed({fake_shnum: data}) - k = unpacked["needed_shares"] - N = unpacked["total_shares"] - filesize = unpacked["size"] - ueb_hash = unpacked["UEB_hash"] - - print("CHK %s %d/%d %d %s %d %s" % (si_s, k, N, filesize, - str(ueb_hash, "utf-8"), expiration, - quote_output(abs_sharefile)), file=out) + p = ShareDumper(None, "fake-si", fake_shnum) + def extract(func): + stash = [] + # these methods return Deferreds, but we happen to know that + # they run synchronously when not actually talking to a + # remote server + d = func() + d.addCallback(stash.append) + return stash[0] + verinfo = extract(p.get_verinfo) + (seqnum, root_hash, salt_to_use, segsize, datalen, k, N, prefix, + offsets) = verinfo + print("MDMF %s %d/%d %d #%d:%s %d %s" % \ + (si_s, k, N, datalen, + seqnum, str(base32.b2a(root_hash), "utf-8"), + expiration, quote_output(abs_sharefile)), file=out) else: - print("UNKNOWN really-unknown %s" % quote_output(abs_sharefile), file=out) + print("UNKNOWN mutable %s" % quote_output(abs_sharefile), file=out) + + +def _describe_immutable_share(abs_sharefile, now, si_s, out): + class ImmediateReadBucketProxy(ReadBucketProxy): + def __init__(self, sf): + self.sf = sf + ReadBucketProxy.__init__(self, None, None, "") + def __repr__(self): + return "" + def _read(self, offset, size): + return defer.succeed(sf.read_share_data(offset, size)) + + # use a ReadBucketProxy to parse the bucket and find the uri extension + sf = ShareFile(abs_sharefile) + bp = ImmediateReadBucketProxy(sf) + + expiration_time = min( [lease.get_expiration_time() + for lease in sf.get_leases()] ) + expiration = max(0, expiration_time - now) + + UEB_data = call(bp.get_uri_extension) + unpacked = uri.unpack_extension_readable(UEB_data) + + k = unpacked["needed_shares"] + N = unpacked["total_shares"] + filesize = unpacked["size"] + ueb_hash = unpacked["UEB_hash"] + + print("CHK %s %d/%d %d %s %d %s" % (si_s, k, N, filesize, + str(ueb_hash, "utf-8"), expiration, + quote_output(abs_sharefile)), file=out) - f.close() def catalog_shares(options): from allmydata.util.encodingutil import listdir_unicode, quote_output @@ -933,34 +928,34 @@ def corrupt_share(options): f.write(d) f.close() - f = open(fn, "rb") - prefix = f.read(32) - f.close() - if prefix == MutableShareFile.MAGIC: - # mutable - m = MutableShareFile(fn) - f = open(fn, "rb") - f.seek(m.DATA_OFFSET) - data = f.read(2000) - # make sure this slot contains an SMDF share - assert data[0:1] == b"\x00", "non-SDMF mutable shares not supported" - f.close() + with open(fn, "rb") as f: + prefix = f.read(32) - (version, ig_seqnum, ig_roothash, ig_IV, ig_k, ig_N, ig_segsize, - ig_datalen, offsets) = unpack_header(data) + if MutableShareFile.is_valid_header(prefix): + # mutable + m = MutableShareFile(fn) + f = open(fn, "rb") + f.seek(m.DATA_OFFSET) + data = f.read(2000) + # make sure this slot contains an SMDF share + assert data[0:1] == b"\x00", "non-SDMF mutable shares not supported" + f.close() - assert version == 0, "we only handle v0 SDMF files" - start = m.DATA_OFFSET + offsets["share_data"] - end = m.DATA_OFFSET + offsets["enc_privkey"] - flip_bit(start, end) - else: - # otherwise assume it's immutable - f = ShareFile(fn) - bp = ReadBucketProxy(None, None, '') - offsets = bp._parse_offsets(f.read_share_data(0, 0x24)) - start = f._data_offset + offsets["data"] - end = f._data_offset + offsets["plaintext_hash_tree"] - flip_bit(start, end) + (version, ig_seqnum, ig_roothash, ig_IV, ig_k, ig_N, ig_segsize, + ig_datalen, offsets) = unpack_header(data) + + assert version == 0, "we only handle v0 SDMF files" + start = m.DATA_OFFSET + offsets["share_data"] + end = m.DATA_OFFSET + offsets["enc_privkey"] + flip_bit(start, end) + else: + # otherwise assume it's immutable + f = ShareFile(fn) + bp = ReadBucketProxy(None, None, '') + offsets = bp._parse_offsets(f.read_share_data(0, 0x24)) + start = f._data_offset + offsets["data"] + end = f._data_offset + offsets["plaintext_hash_tree"] + flip_bit(start, end) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 407116038..24465c1ed 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -59,7 +59,7 @@ class ShareFile(object): @classmethod def is_valid_header(cls, header): - # (bytes) -> bool + # type: (bytes) -> bool """ Determine if the given bytes constitute a valid header for this type of container. diff --git a/src/allmydata/storage/mutable.py b/src/allmydata/storage/mutable.py index de840b89a..1b29b4a65 100644 --- a/src/allmydata/storage/mutable.py +++ b/src/allmydata/storage/mutable.py @@ -67,6 +67,20 @@ class MutableShareFile(object): MAX_SIZE = MAX_MUTABLE_SHARE_SIZE # TODO: decide upon a policy for max share size + @classmethod + def is_valid_header(cls, header): + # type: (bytes) -> bool + """ + Determine if the given bytes constitute a valid header for this type of + container. + + :param header: Some bytes from the beginning of a container. + + :return: ``True`` if the bytes could belong to this container, + ``False`` otherwise. + """ + return header.startswith(cls.MAGIC) + def __init__(self, filename, parent=None): self.home = filename if os.path.exists(self.home): @@ -77,7 +91,7 @@ class MutableShareFile(object): write_enabler_nodeid, write_enabler, data_length, extra_least_offset) = \ struct.unpack(">32s20s32sQQ", data) - if magic != self.MAGIC: + if not self.is_valid_header(data): msg = "sharefile %s had magic '%r' but we wanted '%r'" % \ (filename, magic, self.MAGIC) raise UnknownMutableContainerVersionError(msg) @@ -388,7 +402,7 @@ class MutableShareFile(object): write_enabler_nodeid, write_enabler, data_length, extra_least_offset) = \ struct.unpack(">32s20s32sQQ", data) - assert magic == self.MAGIC + assert self.is_valid_header(data) return (write_enabler, write_enabler_nodeid) def readv(self, readv): diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index f339c579b..0f30dad6a 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -373,7 +373,7 @@ class StorageServer(service.MultiService, Referenceable): for shnum, filename in self._get_bucket_shares(storage_index): with open(filename, 'rb') as f: header = f.read(32) - if header[:32] == MutableShareFile.MAGIC: + if MutableShareFile.is_valid_header(header): sf = MutableShareFile(filename, self) # note: if the share has been migrated, the renew_lease() # call will throw an exception, with information to help the diff --git a/src/allmydata/storage/shares.py b/src/allmydata/storage/shares.py index ec6c0a501..59e7b1539 100644 --- a/src/allmydata/storage/shares.py +++ b/src/allmydata/storage/shares.py @@ -17,8 +17,7 @@ from allmydata.storage.immutable import ShareFile def get_share_file(filename): with open(filename, "rb") as f: prefix = f.read(32) - if prefix == MutableShareFile.MAGIC: + if MutableShareFile.is_valid_header(prefix): return MutableShareFile(filename) # otherwise assume it's immutable return ShareFile(filename) - diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 0f2dc7c62..97368ee92 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -1068,7 +1068,7 @@ def _corrupt_offset_of_uri_extension_to_force_short_read(data, debug=False): def _corrupt_mutable_share_data(data, debug=False): prefix = data[:32] - assert prefix == MutableShareFile.MAGIC, "This function is designed to corrupt mutable shares of v1, and the magic number doesn't look right: %r vs %r" % (prefix, MutableShareFile.MAGIC) + assert MutableShareFile.is_valid_header(prefix), "This function is designed to corrupt mutable shares of v1, and the magic number doesn't look right: %r vs %r" % (prefix, MutableShareFile.MAGIC) data_offset = MutableShareFile.DATA_OFFSET sharetype = data[data_offset:data_offset+1] assert sharetype == b"\x00", "non-SDMF mutable shares not supported" diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index 72ce4b6ec..d859a0e00 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -22,7 +22,8 @@ from twisted.trial import unittest from twisted.internet import defer from allmydata import uri -from allmydata.storage.mutable import ShareFile, MutableShareFile +from allmydata.storage.mutable import MutableShareFile +from allmydata.storage.immutable import ShareFile from allmydata.storage.server import si_a2b from allmydata.immutable import offloaded, upload from allmydata.immutable.literal import LiteralFileNode From 8cb1f4f57cc6591e46573bd214ef0a7c43ad2c04 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 14:25:24 -0400 Subject: [PATCH 07/89] news fragment --- newsfragments/3527.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3527.minor diff --git a/newsfragments/3527.minor b/newsfragments/3527.minor new file mode 100644 index 000000000..e69de29bb From 54d80222c9cdf6662510f67d15de0cf7494a723e Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 14:34:47 -0400 Subject: [PATCH 08/89] switch to monkey-patching from other sources This is not much of an improvement to the tests themselves, unfortunately. However, it does get us one step closer to dropping `mock` as a dependency. --- src/allmydata/test/cli/test_create.py | 144 ++++++++++++++++---------- src/allmydata/test/common.py | 25 +++++ 2 files changed, 115 insertions(+), 54 deletions(-) diff --git a/src/allmydata/test/cli/test_create.py b/src/allmydata/test/cli/test_create.py index 282f26163..609888fb3 100644 --- a/src/allmydata/test/cli/test_create.py +++ b/src/allmydata/test/cli/test_create.py @@ -11,16 +11,24 @@ if PY2: from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 import os -import mock + +try: + from typing import Any, List, Tuple +except ImportError: + pass + from twisted.trial import unittest from twisted.internet import defer, reactor from twisted.python import usage from allmydata.util import configutil +from allmydata.util import tor_provider, i2p_provider from ..common_util import run_cli, parse_cli +from ..common import ( + disable_modules, +) from ...scripts import create_node from ... import client - def read_config(basedir): tahoe_cfg = os.path.join(basedir, "tahoe.cfg") config = configutil.get_config(tahoe_cfg) @@ -105,11 +113,12 @@ class Config(unittest.TestCase): @defer.inlineCallbacks def test_client_hide_ip_no_i2p_txtorcon(self): - # hmm, I must be doing something weird, these don't work as - # @mock.patch decorators for some reason - txi2p = mock.patch('allmydata.util.i2p_provider._import_txi2p', return_value=None) - txtorcon = mock.patch('allmydata.util.tor_provider._import_txtorcon', return_value=None) - with txi2p, txtorcon: + """ + The ``create-client`` sub-command tells the user to install the necessary + dependencies if they have neither tor nor i2p support installed and + they request network location privacy with the ``--hide-ip`` flag. + """ + with disable_modules("txi2p", "txtorcon"): basedir = self.mktemp() rc, out, err = yield run_cli("create-client", "--hide-ip", basedir) self.assertTrue(rc != 0, out) @@ -118,8 +127,7 @@ class Config(unittest.TestCase): @defer.inlineCallbacks def test_client_i2p_option_no_txi2p(self): - txi2p = mock.patch('allmydata.util.i2p_provider._import_txi2p', return_value=None) - with txi2p: + with disable_modules("txi2p"): basedir = self.mktemp() rc, out, err = yield run_cli("create-node", "--listen=i2p", "--i2p-launch", basedir) self.assertTrue(rc != 0) @@ -127,8 +135,7 @@ class Config(unittest.TestCase): @defer.inlineCallbacks def test_client_tor_option_no_txtorcon(self): - txtorcon = mock.patch('allmydata.util.tor_provider._import_txtorcon', return_value=None) - with txtorcon: + with disable_modules("txtorcon"): basedir = self.mktemp() rc, out, err = yield run_cli("create-node", "--listen=tor", "--tor-launch", basedir) self.assertTrue(rc != 0) @@ -145,9 +152,7 @@ class Config(unittest.TestCase): @defer.inlineCallbacks def test_client_hide_ip_no_txtorcon(self): - txtorcon = mock.patch('allmydata.util.tor_provider._import_txtorcon', - return_value=None) - with txtorcon: + with disable_modules("txtorcon"): basedir = self.mktemp() rc, out, err = yield run_cli("create-client", "--hide-ip", basedir) self.assertEqual(0, rc) @@ -295,11 +300,10 @@ class Config(unittest.TestCase): def test_node_slow_tor(self): basedir = self.mktemp() d = defer.Deferred() - with mock.patch("allmydata.util.tor_provider.create_config", - return_value=d): - d2 = run_cli("create-node", "--listen=tor", basedir) - d.callback(({}, "port", "location")) - rc, out, err = yield d2 + self.patch(tor_provider, "create_config", lambda *a, **kw: d) + d2 = run_cli("create-node", "--listen=tor", basedir) + d.callback(({}, "port", "location")) + rc, out, err = yield d2 self.assertEqual(rc, 0) self.assertIn("Node created", out) self.assertEqual(err, "") @@ -308,11 +312,10 @@ class Config(unittest.TestCase): def test_node_slow_i2p(self): basedir = self.mktemp() d = defer.Deferred() - with mock.patch("allmydata.util.i2p_provider.create_config", - return_value=d): - d2 = run_cli("create-node", "--listen=i2p", basedir) - d.callback(({}, "port", "location")) - rc, out, err = yield d2 + self.patch(i2p_provider, "create_config", lambda *a, **kw: d) + d2 = run_cli("create-node", "--listen=i2p", basedir) + d.callback(({}, "port", "location")) + rc, out, err = yield d2 self.assertEqual(rc, 0) self.assertIn("Node created", out) self.assertEqual(err, "") @@ -353,6 +356,27 @@ class Config(unittest.TestCase): self.assertIn("is not empty", err) self.assertIn("To avoid clobbering anything, I am going to quit now", err) +def fake_config(testcase, module, result): + # type: (unittest.TestCase, Any, Any) -> List[Tuple] + """ + Monkey-patch a fake configuration function into the given module. + + :param testcase: The test case to use to do the monkey-patching. + + :param module: The module into which to patch the fake function. + + :param result: The return value for the fake function. + + :return: A list of tuples of the arguments the fake function was called + with. + """ + calls = [] + def fake_config(reactor, cli_config): + calls.append((reactor, cli_config)) + return result + testcase.patch(module, "create_config", fake_config) + return calls + class Tor(unittest.TestCase): def test_default(self): basedir = self.mktemp() @@ -360,12 +384,14 @@ class Tor(unittest.TestCase): tor_port = "ghi" tor_location = "jkl" config_d = defer.succeed( (tor_config, tor_port, tor_location) ) - with mock.patch("allmydata.util.tor_provider.create_config", - return_value=config_d) as co: - rc, out, err = self.successResultOf( - run_cli("create-node", "--listen=tor", basedir)) - self.assertEqual(len(co.mock_calls), 1) - args = co.mock_calls[0][1] + + calls = fake_config(self, tor_provider, config_d) + rc, out, err = self.successResultOf( + run_cli("create-node", "--listen=tor", basedir), + ) + + self.assertEqual(len(calls), 1) + args = calls[0] self.assertIdentical(args[0], reactor) self.assertIsInstance(args[1], create_node.CreateNodeOptions) self.assertEqual(args[1]["listen"], "tor") @@ -380,12 +406,15 @@ class Tor(unittest.TestCase): tor_port = "ghi" tor_location = "jkl" config_d = defer.succeed( (tor_config, tor_port, tor_location) ) - with mock.patch("allmydata.util.tor_provider.create_config", - return_value=config_d) as co: - rc, out, err = self.successResultOf( - run_cli("create-node", "--listen=tor", "--tor-launch", - basedir)) - args = co.mock_calls[0][1] + + calls = fake_config(self, tor_provider, config_d) + rc, out, err = self.successResultOf( + run_cli( + "create-node", "--listen=tor", "--tor-launch", + basedir, + ), + ) + args = calls[0] self.assertEqual(args[1]["listen"], "tor") self.assertEqual(args[1]["tor-launch"], True) self.assertEqual(args[1]["tor-control-port"], None) @@ -396,12 +425,15 @@ class Tor(unittest.TestCase): tor_port = "ghi" tor_location = "jkl" config_d = defer.succeed( (tor_config, tor_port, tor_location) ) - with mock.patch("allmydata.util.tor_provider.create_config", - return_value=config_d) as co: - rc, out, err = self.successResultOf( - run_cli("create-node", "--listen=tor", "--tor-control-port=mno", - basedir)) - args = co.mock_calls[0][1] + + calls = fake_config(self, tor_provider, config_d) + rc, out, err = self.successResultOf( + run_cli( + "create-node", "--listen=tor", "--tor-control-port=mno", + basedir, + ), + ) + args = calls[0] self.assertEqual(args[1]["listen"], "tor") self.assertEqual(args[1]["tor-launch"], False) self.assertEqual(args[1]["tor-control-port"], "mno") @@ -434,12 +466,13 @@ class I2P(unittest.TestCase): i2p_port = "ghi" i2p_location = "jkl" dest_d = defer.succeed( (i2p_config, i2p_port, i2p_location) ) - with mock.patch("allmydata.util.i2p_provider.create_config", - return_value=dest_d) as co: - rc, out, err = self.successResultOf( - run_cli("create-node", "--listen=i2p", basedir)) - self.assertEqual(len(co.mock_calls), 1) - args = co.mock_calls[0][1] + + calls = fake_config(self, i2p_provider, dest_d) + rc, out, err = self.successResultOf( + run_cli("create-node", "--listen=i2p", basedir), + ) + self.assertEqual(len(calls), 1) + args = calls[0] self.assertIdentical(args[0], reactor) self.assertIsInstance(args[1], create_node.CreateNodeOptions) self.assertEqual(args[1]["listen"], "i2p") @@ -461,12 +494,15 @@ class I2P(unittest.TestCase): i2p_port = "ghi" i2p_location = "jkl" dest_d = defer.succeed( (i2p_config, i2p_port, i2p_location) ) - with mock.patch("allmydata.util.i2p_provider.create_config", - return_value=dest_d) as co: - rc, out, err = self.successResultOf( - run_cli("create-node", "--listen=i2p", "--i2p-sam-port=mno", - basedir)) - args = co.mock_calls[0][1] + + calls = fake_config(self, i2p_provider, dest_d) + rc, out, err = self.successResultOf( + run_cli( + "create-node", "--listen=i2p", "--i2p-sam-port=mno", + basedir, + ), + ) + args = calls[0] self.assertEqual(args[1]["listen"], "i2p") self.assertEqual(args[1]["i2p-launch"], False) self.assertEqual(args[1]["i2p-sam-port"], "mno") diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 0f2dc7c62..2e6da9801 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -26,8 +26,14 @@ __all__ = [ "PIPE", ] +try: + from typing import Tuple, ContextManager +except ImportError: + pass + import sys import os, random, struct +from contextlib import contextmanager import six import tempfile from tempfile import mktemp @@ -1213,6 +1219,25 @@ class ConstantAddresses(object): raise Exception("{!r} has no client endpoint.") return self._handler +@contextmanager +def disable_modules(*names): + # type: (Tuple[str]) -> ContextManager + """ + A context manager which makes modules appear to be missing while it is + active. + + :param *names: The names of the modules to disappear. + """ + missing = object() + modules = list(sys.modules.get(n, missing) for n in names) + for n in names: + sys.modules[n] = None + yield + for n, original in zip(names, modules): + if original is missing: + del sys.modules[n] + else: + sys.modules[n] = original class _TestCaseMixin(object): """ From 8d5727977b9a1a7865954db30f9d4771518b97c0 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 14:47:42 -0400 Subject: [PATCH 09/89] it doesn't typecheck, nevermind --- src/allmydata/test/common.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 2e6da9801..8e97fa598 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -26,11 +26,6 @@ __all__ = [ "PIPE", ] -try: - from typing import Tuple, ContextManager -except ImportError: - pass - import sys import os, random, struct from contextlib import contextmanager @@ -1221,7 +1216,6 @@ class ConstantAddresses(object): @contextmanager def disable_modules(*names): - # type: (Tuple[str]) -> ContextManager """ A context manager which makes modules appear to be missing while it is active. From 78dbe7699403dbe38f94d574367f5d5e95916f4a Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 15:20:44 -0400 Subject: [PATCH 10/89] remove unused import --- src/allmydata/storage/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 0f30dad6a..3e2d3b5c6 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -14,7 +14,7 @@ if PY2: else: from typing import Dict -import os, re, struct, time +import os, re, time import six from foolscap.api import Referenceable From 8b976b441e793f45b50e5d5ebcb4314beba889ee Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 12:05:34 -0400 Subject: [PATCH 11/89] add LeaseInfo.is_renew_secret and use it --- src/allmydata/storage/immutable.py | 2 +- src/allmydata/storage/lease.py | 12 +++++++++ src/allmydata/storage/mutable.py | 2 +- src/allmydata/test/test_storage.py | 39 ++++++++++++++++++++++-------- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 24465c1ed..c9b8995b5 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -180,7 +180,7 @@ class ShareFile(object): secret. """ for i,lease in enumerate(self.get_leases()): - if timing_safe_compare(lease.renew_secret, renew_secret): + if lease.is_renew_secret(renew_secret): # yup. See if we need to update the owner time. if allow_backdate or new_expire_time > lease.get_expiration_time(): # yes diff --git a/src/allmydata/storage/lease.py b/src/allmydata/storage/lease.py index 17683a888..2132048ce 100644 --- a/src/allmydata/storage/lease.py +++ b/src/allmydata/storage/lease.py @@ -15,6 +15,8 @@ import struct, time import attr +from allmydata.util.hashutil import timing_safe_compare + @attr.s(frozen=True) class LeaseInfo(object): """ @@ -68,6 +70,16 @@ class LeaseInfo(object): _expiration_time=new_expire_time, ) + def is_renew_secret(self, candidate_secret): + # type: (bytes) -> bool + """ + Check a string to see if it is the correct renew secret. + + :return: ``True`` if it is the correct renew secret, ``False`` + otherwise. + """ + return timing_safe_compare(self.renew_secret, candidate_secret) + def get_grant_renew_time_time(self): # hack, based upon fixed 31day expiration period return self._expiration_time - 31*24*60*60 diff --git a/src/allmydata/storage/mutable.py b/src/allmydata/storage/mutable.py index 1b29b4a65..017f2dbb7 100644 --- a/src/allmydata/storage/mutable.py +++ b/src/allmydata/storage/mutable.py @@ -327,7 +327,7 @@ class MutableShareFile(object): accepting_nodeids = set() with open(self.home, 'rb+') as f: for (leasenum,lease) in self._enumerate_leases(f): - if timing_safe_compare(lease.renew_secret, renew_secret): + if lease.is_renew_secret(renew_secret): # yup. See if we need to update the owner time. if allow_backdate or new_expire_time > lease.get_expiration_time(): # yes diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 8123be2c5..005309f87 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -755,28 +755,28 @@ class Server(unittest.TestCase): # Create a bucket: rs0, cs0 = self.create_bucket_5_shares(ss, b"si0") - leases = list(ss.get_leases(b"si0")) - self.failUnlessEqual(len(leases), 1) - self.failUnlessEqual(set([l.renew_secret for l in leases]), set([rs0])) + (lease,) = ss.get_leases(b"si0") + self.assertTrue(lease.is_renew_secret(rs0)) rs1, cs1 = self.create_bucket_5_shares(ss, b"si1") # take out a second lease on si1 rs2, cs2 = self.create_bucket_5_shares(ss, b"si1", 5, 0) - leases = list(ss.get_leases(b"si1")) - self.failUnlessEqual(len(leases), 2) - self.failUnlessEqual(set([l.renew_secret for l in leases]), set([rs1, rs2])) + (lease1, lease2) = ss.get_leases(b"si1") + self.assertTrue(lease1.is_renew_secret(rs1)) + self.assertTrue(lease2.is_renew_secret(rs2)) # and a third lease, using add-lease rs2a,cs2a = (hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)), hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret))) ss.remote_add_lease(b"si1", rs2a, cs2a) - leases = list(ss.get_leases(b"si1")) - self.failUnlessEqual(len(leases), 3) - self.failUnlessEqual(set([l.renew_secret for l in leases]), set([rs1, rs2, rs2a])) + (lease1, lease2, lease3) = ss.get_leases(b"si1") + self.assertTrue(lease1.is_renew_secret(rs1)) + self.assertTrue(lease2.is_renew_secret(rs2)) + self.assertTrue(lease3.is_renew_secret(rs2a)) # add-lease on a missing storage index is silently ignored - self.failUnlessEqual(ss.remote_add_lease(b"si18", b"", b""), None) + self.assertIsNone(ss.remote_add_lease(b"si18", b"", b"")) # check that si0 is readable readers = ss.remote_get_buckets(b"si0") @@ -3028,3 +3028,22 @@ class ShareFileTests(unittest.TestCase): sf = self.get_sharefile() with self.assertRaises(IndexError): sf.cancel_lease(b"garbage") + + def test_renew_secret(self): + """ + A lease loaded from a share file can have its renew secret verified. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + expiration_time = 2 ** 31 + + sf = self.get_sharefile() + lease = LeaseInfo( + owner_num=0, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + expiration_time=expiration_time, + ) + sf.add_lease(lease) + (loaded_lease,) = sf.get_leases() + self.assertTrue(loaded_lease.is_renew_secret(renew_secret)) From b5f882ffa60574f193a18e70e3c310077a2f097e Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 12:21:22 -0400 Subject: [PATCH 12/89] introduce and use LeaseInfo.is_cancel_secret --- src/allmydata/storage/immutable.py | 2 +- src/allmydata/storage/lease.py | 10 ++++++++++ src/allmydata/storage/mutable.py | 2 +- src/allmydata/test/test_storage.py | 19 +++++++++++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index c9b8995b5..4f6a1c9c7 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -209,7 +209,7 @@ class ShareFile(object): leases = list(self.get_leases()) num_leases_removed = 0 for i,lease in enumerate(leases): - if timing_safe_compare(lease.cancel_secret, cancel_secret): + if lease.is_cancel_secret(cancel_secret): leases[i] = None num_leases_removed += 1 if not num_leases_removed: diff --git a/src/allmydata/storage/lease.py b/src/allmydata/storage/lease.py index 2132048ce..ff96ebaf4 100644 --- a/src/allmydata/storage/lease.py +++ b/src/allmydata/storage/lease.py @@ -80,6 +80,16 @@ class LeaseInfo(object): """ return timing_safe_compare(self.renew_secret, candidate_secret) + def is_cancel_secret(self, candidate_secret): + # type: (bytes) -> bool + """ + Check a string to see if it is the correct cancel secret. + + :return: ``True`` if it is the correct cancel secret, ``False`` + otherwise. + """ + return timing_safe_compare(self.cancel_secret, candidate_secret) + def get_grant_renew_time_time(self): # hack, based upon fixed 31day expiration period return self._expiration_time - 31*24*60*60 diff --git a/src/allmydata/storage/mutable.py b/src/allmydata/storage/mutable.py index 017f2dbb7..9480a3c03 100644 --- a/src/allmydata/storage/mutable.py +++ b/src/allmydata/storage/mutable.py @@ -371,7 +371,7 @@ class MutableShareFile(object): with open(self.home, 'rb+') as f: for (leasenum,lease) in self._enumerate_leases(f): accepting_nodeids.add(lease.nodeid) - if timing_safe_compare(lease.cancel_secret, cancel_secret): + if lease.is_cancel_secret(cancel_secret): self._write_lease_record(f, leasenum, blank_lease) modified += 1 else: diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 005309f87..aac40362c 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -3047,3 +3047,22 @@ class ShareFileTests(unittest.TestCase): sf.add_lease(lease) (loaded_lease,) = sf.get_leases() self.assertTrue(loaded_lease.is_renew_secret(renew_secret)) + + def test_cancel_secret(self): + """ + A lease loaded from a share file can have its cancel secret verified. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + expiration_time = 2 ** 31 + + sf = self.get_sharefile() + lease = LeaseInfo( + owner_num=0, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + expiration_time=expiration_time, + ) + sf.add_lease(lease) + (loaded_lease,) = sf.get_leases() + self.assertTrue(loaded_lease.is_cancel_secret(cancel_secret)) From 696a260ddfc02be35a63ed1446eda0b5434cc86f Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 29 Oct 2021 09:00:38 -0400 Subject: [PATCH 13/89] news fragment --- newsfragments/3836.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3836.minor diff --git a/newsfragments/3836.minor b/newsfragments/3836.minor new file mode 100644 index 000000000..e69de29bb From 892b4683654cd1281be8feeaa65a2ef946ed4f5a Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 29 Oct 2021 09:03:37 -0400 Subject: [PATCH 14/89] use the port assigner to assign a port for the main tub --- src/allmydata/test/common_system.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 9d14c8642..874c7f6ba 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -672,11 +672,14 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): """ iv_dir = self.getdir("introducer") if not os.path.isdir(iv_dir): - _, port_endpoint = self.port_assigner.assign(reactor) + _, web_port_endpoint = self.port_assigner.assign(reactor) + main_location_hint, main_port_endpoint = self.port_assigner.assign(reactor) introducer_config = ( u"[node]\n" u"nickname = introducer \N{BLACK SMILING FACE}\n" + - u"web.port = {}\n".format(port_endpoint) + u"web.port = {}\n".format(web_port_endpoint) + + u"tub.port = {}\n".format(main_port_endpoint) + + u"tub.location = {}\n".format(main_location_hint) ).encode("utf-8") fileutil.make_dirs(iv_dir) From 39c4a2c4eb1963b2035644d97c1760b649c21278 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 2 Nov 2021 15:10:54 -0400 Subject: [PATCH 15/89] tidy up some corners --- src/allmydata/scripts/debug.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/allmydata/scripts/debug.py b/src/allmydata/scripts/debug.py index ab48b0fd0..260cca55b 100644 --- a/src/allmydata/scripts/debug.py +++ b/src/allmydata/scripts/debug.py @@ -746,6 +746,13 @@ def _describe_mutable_share(abs_sharefile, f, now, si_s, out): if share_type == "SDMF": f.seek(m.DATA_OFFSET) + + # Read at least the mutable header length, if possible. If there's + # less data than that in the share, don't try to read more (we won't + # be able to unpack the header in this case but we surely don't want + # to try to unpack bytes *following* the data section as if they were + # header data). Rather than 2000 we could use HEADER_LENGTH from + # allmydata/mutable/layout.py, probably. data = f.read(min(data_length, 2000)) try: @@ -810,8 +817,8 @@ def _describe_immutable_share(abs_sharefile, now, si_s, out): sf = ShareFile(abs_sharefile) bp = ImmediateReadBucketProxy(sf) - expiration_time = min( [lease.get_expiration_time() - for lease in sf.get_leases()] ) + expiration_time = min(lease.get_expiration_time() + for lease in sf.get_leases()) expiration = max(0, expiration_time - now) UEB_data = call(bp.get_uri_extension) @@ -934,9 +941,10 @@ def corrupt_share(options): if MutableShareFile.is_valid_header(prefix): # mutable m = MutableShareFile(fn) - f = open(fn, "rb") - f.seek(m.DATA_OFFSET) - data = f.read(2000) + with open(fn, "rb") as f: + f.seek(m.DATA_OFFSET) + # Read enough data to get a mutable header to unpack. + data = f.read(2000) # make sure this slot contains an SMDF share assert data[0:1] == b"\x00", "non-SDMF mutable shares not supported" f.close() From b3d1acd14a1f602df5bba424214070a4643a8bab Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 09:55:16 -0400 Subject: [PATCH 16/89] try skipping Tor integration tests on Python 2 --- integration/test_tor.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/integration/test_tor.py b/integration/test_tor.py index 15d888e36..b0419f0d2 100644 --- a/integration/test_tor.py +++ b/integration/test_tor.py @@ -35,6 +35,9 @@ from allmydata.test.common import ( if sys.platform.startswith('win'): pytest.skip('Skipping Tor tests on Windows', allow_module_level=True) +if PY2: + pytest.skip('Skipping Tor tests on Python 2 because dependencies are hard to come by', allow_module_level=True) + @pytest_twisted.inlineCallbacks def test_onion_service_storage(reactor, request, temp_dir, flog_gatherer, tor_network, tor_introducer_furl): yield _create_anonymous_node(reactor, 'carol', 8008, request, temp_dir, flog_gatherer, tor_network, tor_introducer_furl) From 4606c3c9dde91de11d769bf9d8c6fd6f2fd1f877 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 09:59:19 -0400 Subject: [PATCH 17/89] news fragment --- newsfragments/3837.other | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3837.other diff --git a/newsfragments/3837.other b/newsfragments/3837.other new file mode 100644 index 000000000..a9e4e6986 --- /dev/null +++ b/newsfragments/3837.other @@ -0,0 +1 @@ +Tahoe-LAFS no longer runs its Tor integration test suite on Python 2 due to the increased complexity of obtaining compatible versions of necessary dependencies. From 8e150cce6a27b6616db54cfd4c2ac08fbdd13794 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 13:14:55 -0400 Subject: [PATCH 18/89] add explicit direct tests for the new methods --- src/allmydata/test/test_storage.py | 61 ++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index aac40362c..460653bd0 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -3066,3 +3066,64 @@ class ShareFileTests(unittest.TestCase): sf.add_lease(lease) (loaded_lease,) = sf.get_leases() self.assertTrue(loaded_lease.is_cancel_secret(cancel_secret)) + + +class LeaseInfoTests(unittest.TestCase): + """ + Tests for ``allmydata.storage.lease.LeaseInfo``. + """ + def test_is_renew_secret(self): + """ + ``LeaseInfo.is_renew_secret`` returns ``True`` if the value given is the + renew secret. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + lease = LeaseInfo( + owner_num=1, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + ) + self.assertTrue(lease.is_renew_secret(renew_secret)) + + def test_is_not_renew_secret(self): + """ + ``LeaseInfo.is_renew_secret`` returns ``False`` if the value given is not + the renew secret. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + lease = LeaseInfo( + owner_num=1, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + ) + self.assertFalse(lease.is_renew_secret(cancel_secret)) + + def test_is_cancel_secret(self): + """ + ``LeaseInfo.is_cancel_secret`` returns ``True`` if the value given is the + cancel secret. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + lease = LeaseInfo( + owner_num=1, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + ) + self.assertTrue(lease.is_cancel_secret(cancel_secret)) + + def test_is_not_cancel_secret(self): + """ + ``LeaseInfo.is_cancel_secret`` returns ``False`` if the value given is not + the cancel secret. + """ + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 + lease = LeaseInfo( + owner_num=1, + renew_secret=renew_secret, + cancel_secret=cancel_secret, + ) + self.assertFalse(lease.is_cancel_secret(renew_secret)) From 7335b2a5977752c0805a7fd9c7759cafa8ac31b1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 13:16:15 -0400 Subject: [PATCH 19/89] remove unused import --- src/allmydata/storage/immutable.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 4f6a1c9c7..8a7a5a966 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -24,7 +24,6 @@ from allmydata.interfaces import ( ) from allmydata.util import base32, fileutil, log from allmydata.util.assertutil import precondition -from allmydata.util.hashutil import timing_safe_compare from allmydata.storage.lease import LeaseInfo from allmydata.storage.common import UnknownImmutableContainerVersionError From 86ca463c3198746e31b61569f79e860f4a6e7d6d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 13:24:04 -0400 Subject: [PATCH 20/89] news fragment --- newsfragments/3834.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3834.minor diff --git a/newsfragments/3834.minor b/newsfragments/3834.minor new file mode 100644 index 000000000..e69de29bb From 797e0994596dd916a978b5fc8a757d15322b3100 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:05:28 -0400 Subject: [PATCH 21/89] make create_introducer_webish assign a main tub port --- src/allmydata/test/web/test_introducer.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/allmydata/test/web/test_introducer.py b/src/allmydata/test/web/test_introducer.py index 4b5850cbc..69309d35b 100644 --- a/src/allmydata/test/web/test_introducer.py +++ b/src/allmydata/test/web/test_introducer.py @@ -83,12 +83,18 @@ def create_introducer_webish(reactor, port_assigner, basedir): with the node and its webish service. """ node.create_node_dir(basedir, "testing") - _, port_endpoint = port_assigner.assign(reactor) + main_tub_location, main_tub_endpoint = port_assigner.assign(reactor) + _, web_port_endpoint = port_assigner.assign(reactor) with open(join(basedir, "tahoe.cfg"), "w") as f: f.write( "[node]\n" - "tub.location = 127.0.0.1:1\n" + - "web.port = {}\n".format(port_endpoint) + "tub.port = {main_tub_endpoint}\n" + "tub.location = {main_tub_location}\n" + "web.port = {web_port_endpoint}\n".format( + main_tub_endpoint=main_tub_endpoint, + main_tub_location=main_tub_location, + web_port_endpoint=web_port_endpoint, + ) ) intro_node = yield create_introducer(basedir) From 31649890ef47c2169a0aedee2d7488b8f6da6959 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:08:08 -0400 Subject: [PATCH 22/89] Teach UseNode to use a port assigner for tub.port Then use it to assign ports for tub.port unless the caller supplied their own value. --- src/allmydata/test/common.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 97368ee92..e0472edce 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -267,8 +267,12 @@ class UseNode(object): node_config = attr.ib(default=attr.Factory(dict)) config = attr.ib(default=None) + reactor = attr.ib(default=None) def setUp(self): + self.assigner = SameProcessStreamEndpointAssigner() + self.assigner.setUp() + def format_config_items(config): return "\n".join( " = ".join((key, value)) @@ -292,6 +296,23 @@ class UseNode(object): "default", self.introducer_furl, ) + + node_config = self.node_config.copy() + if "tub.port" not in node_config: + if "tub.location" in node_config: + raise ValueError( + "UseNode fixture does not support specifying tub.location " + "without tub.port" + ) + + # Don't use the normal port auto-assignment logic. It produces + # collisions and makes tests fail spuriously. + tub_location, tub_endpoint = self.assigner.assign(self.reactor) + node_config.update({ + "tub.port": tub_endpoint, + "tub.location": tub_location, + }) + self.config = config_from_string( self.basedir.asTextMode().path, "tub.port", @@ -304,7 +325,7 @@ storage.plugins = {storage_plugin} {plugin_config_section} """.format( storage_plugin=self.storage_plugin, - node_config=format_config_items(self.node_config), + node_config=format_config_items(node_config), plugin_config_section=plugin_config_section, ) ) @@ -316,7 +337,7 @@ storage.plugins = {storage_plugin} ) def cleanUp(self): - pass + self.assigner.tearDown() def getDetails(self): From 5a71774bf875a71c8ddbfb8b4fcfcb2dda7a4f9d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:10:32 -0400 Subject: [PATCH 23/89] use port assigner and UseNode more in test_node.py --- src/allmydata/test/test_node.py | 36 +++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/src/allmydata/test/test_node.py b/src/allmydata/test/test_node.py index cf5fa27f3..c6cff1bab 100644 --- a/src/allmydata/test/test_node.py +++ b/src/allmydata/test/test_node.py @@ -69,6 +69,8 @@ import allmydata.test.common_util as testutil from .common import ( ConstantAddresses, + SameProcessStreamEndpointAssigner, + UseNode, ) def port_numbers(): @@ -80,11 +82,10 @@ class LoggingMultiService(service.MultiService): # see https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2946 -def testing_tub(config_data=''): +def testing_tub(reactor, config_data=''): """ Creates a 'main' Tub for testing purposes, from config data """ - from twisted.internet import reactor basedir = 'dummy_basedir' config = config_from_string(basedir, 'DEFAULT_PORTNUMFILE_BLANK', config_data) fileutil.make_dirs(os.path.join(basedir, 'private')) @@ -112,6 +113,9 @@ class TestCase(testutil.SignalMixin, unittest.TestCase): # try to bind the port. We'll use a low-numbered one that's likely to # conflict with another service to prove it. self._available_port = 22 + self.port_assigner = SameProcessStreamEndpointAssigner() + self.port_assigner.setUp() + self.addCleanup(self.port_assigner.tearDown) def _test_location( self, @@ -137,11 +141,23 @@ class TestCase(testutil.SignalMixin, unittest.TestCase): :param local_addresses: If not ``None`` then a list of addresses to supply to the system under test as local addresses. """ + from twisted.internet import reactor + basedir = self.mktemp() create_node_dir(basedir, "testing") + if tub_port is None: + # Always configure a usable tub.port address instead of relying on + # the automatic port assignment. The automatic port assignment is + # prone to collisions and spurious test failures. + _, tub_port = self.port_assigner.assign(reactor) + config_data = "[node]\n" - if tub_port: - config_data += "tub.port = {}\n".format(tub_port) + config_data += "tub.port = {}\n".format(tub_port) + + # If they wanted a certain location, go for it. This probably won't + # agree with the tub.port value we set but that only matters if + # anything tries to use this to establish a connection ... which + # nothing in this test suite will. if tub_location is not None: config_data += "tub.location = {}\n".format(tub_location) @@ -149,7 +165,7 @@ class TestCase(testutil.SignalMixin, unittest.TestCase): self.patch(iputil, 'get_local_addresses_sync', lambda: local_addresses) - tub = testing_tub(config_data) + tub = testing_tub(reactor, config_data) class Foo(object): pass @@ -431,7 +447,12 @@ class TestCase(testutil.SignalMixin, unittest.TestCase): @defer.inlineCallbacks def test_logdir_is_str(self): - basedir = "test_node/test_logdir_is_str" + from twisted.internet import reactor + + basedir = FilePath(self.mktemp()) + fixture = UseNode(None, None, basedir, "pb://introducer/furl", {}, reactor=reactor) + fixture.setUp() + self.addCleanup(fixture.cleanUp) ns = Namespace() ns.called = False @@ -440,8 +461,7 @@ class TestCase(testutil.SignalMixin, unittest.TestCase): self.failUnless(isinstance(logdir, str), logdir) self.patch(foolscap.logging.log, 'setLogDir', call_setLogDir) - create_node_dir(basedir, "nothing to see here") - yield client.create_client(basedir) + yield fixture.create_node() self.failUnless(ns.called) def test_set_config_unescaped_furl_hash(self): From 5caa80fe383630aab8afa8a9a1667fb3d4cd8f60 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:11:08 -0400 Subject: [PATCH 24/89] use UseNode more in test_client.py Also make write_introducer more lenient about filesystem state --- src/allmydata/scripts/common.py | 4 +++- src/allmydata/test/test_client.py | 16 +++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/allmydata/scripts/common.py b/src/allmydata/scripts/common.py index 0a9ab8714..c9fc8e031 100644 --- a/src/allmydata/scripts/common.py +++ b/src/allmydata/scripts/common.py @@ -141,7 +141,9 @@ def write_introducer(basedir, petname, furl): """ if isinstance(furl, bytes): furl = furl.decode("utf-8") - basedir.child(b"private").child(b"introducers.yaml").setContent( + private = basedir.child(b"private") + private.makedirs(ignoreExistingDirectory=True) + private.child(b"introducers.yaml").setContent( safe_dump({ "introducers": { petname: { diff --git a/src/allmydata/test/test_client.py b/src/allmydata/test/test_client.py index fd2837f1d..a2572e735 100644 --- a/src/allmydata/test/test_client.py +++ b/src/allmydata/test/test_client.py @@ -89,6 +89,7 @@ from .common import ( UseTestPlugins, MemoryIntroducerClient, get_published_announcements, + UseNode, ) from .matchers import ( MatchesSameElements, @@ -953,13 +954,14 @@ class Run(unittest.TestCase, testutil.StallMixin): @defer.inlineCallbacks def test_reloadable(self): - basedir = FilePath("test_client.Run.test_reloadable") - private = basedir.child("private") - private.makedirs() + from twisted.internet import reactor + dummy = "pb://wl74cyahejagspqgy4x5ukrvfnevlknt@127.0.0.1:58889/bogus" - write_introducer(basedir, "someintroducer", dummy) - basedir.child("tahoe.cfg").setContent(BASECONFIG. encode("ascii")) - c1 = yield client.create_client(basedir.path) + fixture = UseNode(None, None, FilePath(self.mktemp()), dummy, reactor=reactor) + fixture.setUp() + self.addCleanup(fixture.cleanUp) + + c1 = yield fixture.create_node() c1.setServiceParent(self.sparent) # delay to let the service start up completely. I'm not entirely sure @@ -981,7 +983,7 @@ class Run(unittest.TestCase, testutil.StallMixin): # also change _check_exit_trigger to use it instead of a raw # reactor.stop, also instrument the shutdown event in an # attribute that we can check.) - c2 = yield client.create_client(basedir.path) + c2 = yield fixture.create_node() c2.setServiceParent(self.sparent) yield c2.disownServiceParent() From 780be2691b9ca4a7b1b2d08c6d0bb44b11d8b9a1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:11:28 -0400 Subject: [PATCH 25/89] assign a tub.port to all system test nodes --- src/allmydata/test/common_system.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 874c7f6ba..0c424136a 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -767,13 +767,15 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def _generate_config(self, which, basedir): config = {} - except1 = set(range(self.numclients)) - {1} + allclients = set(range(self.numclients)) + except1 = allclients - {1} feature_matrix = { ("client", "nickname"): except1, - # client 1 has to auto-assign an address. - ("node", "tub.port"): except1, - ("node", "tub.location"): except1, + # Auto-assigning addresses is extremely failure prone and not + # amenable to automated testing in _this_ manner. + ("node", "tub.port"): allclients, + ("node", "tub.location"): allclients, # client 0 runs a webserver and a helper # client 3 runs a webserver but no helper @@ -855,7 +857,13 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): # connection-lost code basedir = FilePath(self.getdir("client%d" % client_num)) basedir.makedirs() - config = "[client]\n" + config = ( + "[node]\n" + "tub.location = {}\n" + "tub.port = {}\n" + "[client]\n" + ).format(*self.port_assigner.assign(reactor)) + if helper_furl: config += "helper.furl = %s\n" % helper_furl basedir.child("tahoe.cfg").setContent(config.encode("utf-8")) From b4bc95cb5a36b7507ce3745cfc3a273b5eedecb6 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 3 Nov 2021 16:15:38 -0400 Subject: [PATCH 26/89] news fragment --- newsfragments/3838.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3838.minor diff --git a/newsfragments/3838.minor b/newsfragments/3838.minor new file mode 100644 index 000000000..e69de29bb From 8dd4aaebb6e579b4874951bc4b6c6218ed667b79 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 10 Nov 2021 14:42:22 -0500 Subject: [PATCH 27/89] More consistent header system. --- docs/proposed/http-storage-node-protocol.rst | 106 +++++++++++-------- 1 file changed, 63 insertions(+), 43 deletions(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index fd1db5c4c..2a392fb20 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -363,11 +363,11 @@ one branch contains all of the share data; another branch contains all of the lease data; etc. -Authorization is required for all endpoints. +An ``Authorization`` header in requests is required for all endpoints. The standard HTTP authorization protocol is used. The authentication *type* used is ``Tahoe-LAFS``. The swissnum from the NURL used to locate the storage service is used as the *credentials*. -If credentials are not presented or the swissnum is not associated with a storage service then no storage processing is performed and the request receives an ``UNAUTHORIZED`` response. +If credentials are not presented or the swissnum is not associated with a storage service then no storage processing is performed and the request receives an ``401 UNAUTHORIZED`` response. General ~~~~~~~ @@ -396,17 +396,26 @@ For example:: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Either renew or create a new lease on the bucket addressed by ``storage_index``. -The details of the lease are encoded in the request body. + +For a renewal, the renew secret and cancellation secret should be included as ``X-Tahoe-Authorization`` headers. For example:: - {"renew-secret": "abcd", "cancel-secret": "efgh"} + X-Tahoe-Authorization: lease-renew-secret + X-Tahoe-Authorization: lease-cancel-secret -If the ``renew-secret`` value matches an existing lease -then the expiration time of that lease will be changed to 31 days after the time of this operation. -If it does not match an existing lease -then a new lease will be created with this ``renew-secret`` which expires 31 days after the time of this operation. +For a new lease, ``X-Tahoe-Set-Authorization`` headers should be used instead. +For example:: -``renew-secret`` and ``cancel-secret`` values must be 32 bytes long. + X-Tahoe-Set-Authorization: lease-renew-secret + X-Tahoe-Set-Authorization: lease-cancel-secret + +For renewal, the expiration time of that lease will be changed to 31 days after the time of this operation. +If the renewal secret does not match, a new lease will be created, but clients should still not rely on this behavior if possible, and instead use the appropriate new lease headers. + +For the creation path, +then a new lease will be created with this ``lease-renew-secret`` which expires 31 days after the time of this operation. + +``lease-renew-secret`` and ``lease-cancel-secret`` values must be 32 bytes long. The server treats them as opaque values. :ref:`Share Leases` gives details about how the Tahoe-LAFS storage client constructs these values. @@ -423,7 +432,7 @@ In these cases the server takes no action and returns ``NOT FOUND``. Discussion `````````` -We considered an alternative where ``renew-secret`` and ``cancel-secret`` are placed in query arguments on the request path. +We considered an alternative where ``lease-renew-secret`` and ``lease-cancel-secret`` are placed in query arguments on the request path. We chose to put these values into the request body to make the URL simpler. Several behaviors here are blindly copied from the Foolscap-based storage server protocol. @@ -452,13 +461,13 @@ For example:: {"share-numbers": [1, 7, ...], "allocated-size": 12345} -The request must include ``WWW-Authenticate`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. +The request must include ``X-Tahoe-Set-Authorization`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. Typically this is a header sent by the server, but in Tahoe-LAFS keys are set by the client, so may as well reuse it. For example:: - WWW-Authenticate: x-tahoe-renew-secret - WWW-Authenticate: x-tahoe-cancel-secret - WWW-Authenticate: x-tahoe-upload-secret + X-Tahoe-Set-Authorization: lease-renew-secret + X-Tahoe-Set-Authorization: lease-cancel-secret + X-Tahoe-Set-Authorization: upload-secret The response body includes encoded information about the created buckets. For example:: @@ -527,9 +536,9 @@ If any one of these requests fails then at most 128KiB of upload work needs to b The server must recognize when all of the data has been received and mark the share as complete (which it can do because it was informed of the size when the storage index was initialized). -The request must include a ``Authorization`` header that includes the upload secret:: +The request must include a ``X-Tahoe-Authorization`` header that includes the upload secret:: - Authorization: x-tahoe-upload-secret + X-Tahoe-Authorization: upload-secret Responses: @@ -557,9 +566,9 @@ Responses: This cancels an *in-progress* upload. -The request body looks this:: +The request must include a ``Authorization`` header that includes the upload secret:: - { "upload-secret": "xyzf" } + X-Tahoe-Authorization: upload-secret The response code: @@ -658,16 +667,16 @@ The first write operation on a mutable storage index creates it (that is, there is no separate "create this storage index" operation as there is for the immutable storage index type). -The request body includes the secrets necessary to rewrite to the shares -along with test, read, and write vectors for the operation. +The request must include ``X-Tahoe-Authorization`` headers with write enabler and lease secrets:: + + X-Tahoe-Authorization: write-enabler + X-Tahoe-Authorization: lease-lease-cancel-secret + X-Tahoe-Authorization: lease-renew-secret + +The request body includes test, read, and write vectors for the operation. For example:: { - "secrets": { - "write-enabler": "abcd", - "lease-renew": "efgh", - "lease-cancel": "ijkl" - }, "test-write-vectors": { 0: { "test": [{ @@ -733,9 +742,10 @@ Immutable Data 1. Create a bucket for storage index ``AAAAAAAAAAAAAAAA`` to hold two immutable shares, discovering that share ``1`` was already uploaded:: POST /v1/immutable/AAAAAAAAAAAAAAAA - WWW-Authenticate: x-tahoe-renew-secret efgh - WWW-Authenticate: x-tahoe-cancel-secret jjkl - WWW-Authenticate: x-tahoe-upload-secret xyzf + Authorization: Tahoe-LAFS nurl-swissnum + X-Tahoe-Set-Authorization: lease-renew-secret efgh + X-Tahoe-Set-Authorization: lease-cancel-secret jjkl + X-Tahoe-Set-Authorization: upload-secret xyzf {"share-numbers": [1, 7], "allocated-size": 48} @@ -745,22 +755,25 @@ Immutable Data #. Upload the content for immutable share ``7``:: PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 + Authorization: Tahoe-LAFS nurl-swissnum Content-Range: bytes 0-15/48 - Authorization: x-tahoe-upload-secret xyzf + X-Tahoe-Authorization: upload-secret xyzf 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 + Authorization: Tahoe-LAFS nurl-swissnum Content-Range: bytes 16-31/48 - Authorization: x-tahoe-upload-secret xyzf + X-Tahoe-Authorization: upload-secret xyzf 200 OK PATCH /v1/immutable/AAAAAAAAAAAAAAAA/7 + Authorization: Tahoe-LAFS nurl-swissnum Content-Range: bytes 32-47/48 - Authorization: x-tahoe-upload-secret xyzf + X-Tahoe-Authorization: upload-secret xyzf 201 CREATED @@ -768,6 +781,7 @@ Immutable Data #. Download the content of the previously uploaded immutable share ``7``:: GET /v1/immutable/AAAAAAAAAAAAAAAA?share=7 + Authorization: Tahoe-LAFS nurl-swissnum Range: bytes=0-47 200 OK @@ -776,7 +790,9 @@ Immutable Data #. Renew the lease on all immutable shares in bucket ``AAAAAAAAAAAAAAAA``:: PUT /v1/lease/AAAAAAAAAAAAAAAA - {"renew-secret": "efgh", "cancel-secret": "ijkl"} + Authorization: Tahoe-LAFS nurl-swissnum + X-Tahoe-Authorization: lease-cancel-secret jjkl + X-Tahoe-Authorization: upload-secret xyzf 204 NO CONTENT @@ -789,12 +805,12 @@ if there is no existing share, otherwise it will read a byte which won't match `b""`:: POST /v1/mutable/BBBBBBBBBBBBBBBB/read-test-write + Authorization: Tahoe-LAFS nurl-swissnum + X-Tahoe-Authorization: write-enabler abcd + X-Tahoe-Authorization: lease-cancel-secret efgh + X-Tahoe-Authorization: lease-renew-secret ijkl + { - "secrets": { - "write-enabler": "abcd", - "lease-renew": "efgh", - "lease-cancel": "ijkl" - }, "test-write-vectors": { 3: { "test": [{ @@ -821,12 +837,12 @@ otherwise it will read a byte which won't match `b""`:: #. Safely rewrite the contents of a known version of mutable share number ``3`` (or fail):: POST /v1/mutable/BBBBBBBBBBBBBBBB/read-test-write + Authorization: Tahoe-LAFS nurl-swissnum + X-Tahoe-Authorization: write-enabler abcd + X-Tahoe-Authorization: lease-cancel-secret efgh + X-Tahoe-Authorization: lease-renew-secret ijkl + { - "secrets": { - "write-enabler": "abcd", - "lease-renew": "efgh", - "lease-cancel": "ijkl" - }, "test-write-vectors": { 3: { "test": [{ @@ -853,12 +869,16 @@ otherwise it will read a byte which won't match `b""`:: #. Download the contents of share number ``3``:: GET /v1/mutable/BBBBBBBBBBBBBBBB?share=3&offset=0&size=10 + Authorization: Tahoe-LAFS nurl-swissnum + #. Renew the lease on previously uploaded mutable share in slot ``BBBBBBBBBBBBBBBB``:: PUT /v1/lease/BBBBBBBBBBBBBBBB - {"renew-secret": "efgh", "cancel-secret": "ijkl"} + Authorization: Tahoe-LAFS nurl-swissnum + X-Tahoe-Authorization: lease-cancel-secret efgh + X-Tahoe-Authorization: lease-renew-secret ijkl 204 NO CONTENT From 7faec6e5a0bb53f5d58a4253795047144a58d62d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 15:48:58 -0500 Subject: [PATCH 28/89] news fragment --- newsfragments/3842.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3842.minor diff --git a/newsfragments/3842.minor b/newsfragments/3842.minor new file mode 100644 index 000000000..e69de29bb From 9af81d21c5af232c8e02e874b09ac33202cb5158 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 16:08:40 -0500 Subject: [PATCH 29/89] add a way to turn off implicit bucket lease renewal too --- src/allmydata/storage/server.py | 50 ++++++++++++++++++++----- src/allmydata/test/test_storage.py | 59 +++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 3e2d3b5c6..d142646a8 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -57,9 +57,23 @@ DEFAULT_RENEWAL_TIME = 31 * 24 * 60 * 60 @implementer(RIStorageServer, IStatsProducer) class StorageServer(service.MultiService, Referenceable): + """ + A filesystem-based implementation of ``RIStorageServer``. + + :ivar bool _implicit_bucket_lease_renewal: If and only if this is ``True`` + then ``allocate_buckets`` will renew leases on existing shares + associated with the storage index it operates on. + + :ivar bool _implicit_slot_lease_renewal: If and only if this is ``True`` + then ``slot_testv_and_readv_and_writev`` will renew leases on shares + associated with the slot it operates on. + """ name = 'storage' LeaseCheckerClass = LeaseCheckingCrawler + _implicit_bucket_lease_renewal = True + _implicit_slot_lease_renewal = True + def __init__(self, storedir, nodeid, reserved_space=0, discard_storage=False, readonly_storage=False, stats_provider=None, @@ -135,6 +149,29 @@ class StorageServer(service.MultiService, Referenceable): def __repr__(self): return "" % (idlib.shortnodeid_b2a(self.my_nodeid),) + def set_implicit_bucket_lease_renewal(self, enabled): + # type: (bool) -> None + """ + Control the behavior of implicit lease renewal by *allocate_buckets*. + + :param enabled: If and only if ``True`` then future *allocate_buckets* + calls will renew leases on shares that already exist in the bucket. + """ + self._implicit_bucket_lease_renewal = enabled + + def set_implicit_slot_lease_renewal(self, enabled): + # type: (bool) -> None + """ + Control the behavior of implicit lease renewal by + *slot_testv_and_readv_and_writev*. + + :param enabled: If and only if ``True`` then future + *slot_testv_and_readv_and_writev* calls will renew leases on + shares that still exist in the slot after the writev is applied + and which were touched by the writev. + """ + self._implicit_slot_lease_renewal = enabled + def have_shares(self): # quick test to decide if we need to commit to an implicit # permutation-seed or if we should use a new one @@ -319,8 +356,9 @@ class StorageServer(service.MultiService, Referenceable): # file, they'll want us to hold leases for this file. for (shnum, fn) in self._get_bucket_shares(storage_index): alreadygot.add(shnum) - sf = ShareFile(fn) - sf.add_or_renew_lease(lease_info) + if self._implicit_bucket_lease_renewal: + sf = ShareFile(fn) + sf.add_or_renew_lease(lease_info) for shnum in sharenums: incominghome = os.path.join(self.incomingdir, si_dir, "%d" % shnum) @@ -625,15 +663,10 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, - renew_leases, ): """ Read data from shares and conditionally write some data to them. - :param bool renew_leases: If and only if this is ``True`` and the test - vectors pass then shares in this slot will also have an updated - lease applied to them. - See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. """ @@ -673,7 +706,7 @@ class StorageServer(service.MultiService, Referenceable): test_and_write_vectors, shares, ) - if renew_leases: + if self._implicit_slot_lease_renewal: lease_info = self._make_lease_info(renew_secret, cancel_secret) self._add_or_renew_leases(remaining_shares, lease_info) @@ -690,7 +723,6 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, - renew_leases=True, ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 460653bd0..efa889f8d 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -608,6 +608,61 @@ class Server(unittest.TestCase): for i,wb in writers.items(): wb.remote_abort() + def test_allocate_without_lease_renewal(self): + """ + ``remote_allocate_buckets`` does not renew leases on existing shares if + ``set_implicit_bucket_lease_renewal(False)`` is called first. + """ + first_lease = 456 + second_lease = 543 + storage_index = b"allocate" + + clock = Clock() + clock.advance(first_lease) + ss = self.create( + "test_allocate_without_lease_renewal", + get_current_time=clock.seconds, + ) + ss.set_implicit_bucket_lease_renewal(False) + + # Put a share on there + already, writers = self.allocate(ss, storage_index, [0], 1) + (writer,) = writers.values() + writer.remote_write(0, b"x") + writer.remote_close() + + # It should have a lease granted at the current time. + shares = dict(ss._get_bucket_shares(storage_index)) + self.assertEqual( + [first_lease], + list( + lease.get_grant_renew_time_time() + for lease + in ShareFile(shares[0]).get_leases() + ), + ) + + # Let some time pass so we can tell if the lease on share 0 is + # renewed. + clock.advance(second_lease) + + # Put another share on there. + already, writers = self.allocate(ss, storage_index, [1], 1) + (writer,) = writers.values() + writer.remote_write(0, b"x") + writer.remote_close() + + # The first share's lease expiration time is unchanged. + shares = dict(ss._get_bucket_shares(storage_index)) + self.assertEqual( + [first_lease], + list( + lease.get_grant_renew_time_time() + for lease + in ShareFile(shares[0]).get_leases() + ), + ) + def test_bad_container_version(self): ss = self.create("test_bad_container_version") a,w = self.allocate(ss, b"si1", [0], 10) @@ -1408,9 +1463,10 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``False`` is passed for the ``renew_leases`` parameter. + leases if ``set_implicit_bucket_lease_renewal(False)`` is called first. """ ss = self.create("test_writev_without_renew_lease") + ss.set_implicit_slot_lease_renewal(False) storage_index = b"si2" secrets = ( @@ -1429,7 +1485,6 @@ class MutableServer(unittest.TestCase): sharenum: ([], datav, None), }, read_vector=[], - renew_leases=False, ) leases = list(ss.get_slot_leases(storage_index)) self.assertEqual([], leases) From 2742de6f7c1fa6cf77e35ecc5854bcf7db3e5963 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 16:08:53 -0500 Subject: [PATCH 30/89] drop some ancient cruft allocated_size not used anywhere, so why have it --- src/allmydata/storage/server.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index d142646a8..36cf06d0e 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -617,10 +617,8 @@ class StorageServer(service.MultiService, Referenceable): else: if sharenum not in shares: # allocate a new share - allocated_size = 2000 # arbitrary, really share = self._allocate_slot_share(bucketdir, secrets, sharenum, - allocated_size, owner_num=0) shares[sharenum] = share shares[sharenum].writev(datav, new_length) @@ -726,7 +724,7 @@ class StorageServer(service.MultiService, Referenceable): ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, - allocated_size, owner_num=0): + owner_num=0): (write_enabler, renew_secret, cancel_secret) = secrets my_nodeid = self.my_nodeid fileutil.make_dirs(bucketdir) From c270a346c6c7c247db08bf107bef93c4cccc7ced Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Nov 2021 11:02:51 -0500 Subject: [PATCH 31/89] Remove typo. --- docs/proposed/http-storage-node-protocol.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 2a392fb20..19a64f5ca 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -670,7 +670,7 @@ there is no separate "create this storage index" operation as there is for the i The request must include ``X-Tahoe-Authorization`` headers with write enabler and lease secrets:: X-Tahoe-Authorization: write-enabler - X-Tahoe-Authorization: lease-lease-cancel-secret + X-Tahoe-Authorization: lease-cancel-secret X-Tahoe-Authorization: lease-renew-secret The request body includes test, read, and write vectors for the operation. From 24646c56d0aae56bd18d2d2ffa2acf1616cc2a62 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Nov 2021 11:29:05 -0500 Subject: [PATCH 32/89] Updates based on review. --- docs/proposed/http-storage-node-protocol.rst | 43 ++++++++------------ 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 19a64f5ca..44bda1205 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -397,22 +397,15 @@ For example:: Either renew or create a new lease on the bucket addressed by ``storage_index``. -For a renewal, the renew secret and cancellation secret should be included as ``X-Tahoe-Authorization`` headers. +The renew secret and cancellation secret should be included as ``X-Tahoe-Authorization`` headers. For example:: X-Tahoe-Authorization: lease-renew-secret X-Tahoe-Authorization: lease-cancel-secret -For a new lease, ``X-Tahoe-Set-Authorization`` headers should be used instead. -For example:: - - X-Tahoe-Set-Authorization: lease-renew-secret - X-Tahoe-Set-Authorization: lease-cancel-secret - -For renewal, the expiration time of that lease will be changed to 31 days after the time of this operation. -If the renewal secret does not match, a new lease will be created, but clients should still not rely on this behavior if possible, and instead use the appropriate new lease headers. - -For the creation path, +If the ``lease-renew-secret`` value matches an existing lease +then the expiration time of that lease will be changed to 31 days after the time of this operation. +If it does not match an existing lease then a new lease will be created with this ``lease-renew-secret`` which expires 31 days after the time of this operation. ``lease-renew-secret`` and ``lease-cancel-secret`` values must be 32 bytes long. @@ -433,7 +426,9 @@ Discussion `````````` We considered an alternative where ``lease-renew-secret`` and ``lease-cancel-secret`` are placed in query arguments on the request path. -We chose to put these values into the request body to make the URL simpler. +This increases chances of leaking secrets in logs. +Putting the secrets in the body reduces the chances of leaking secrets, +but eventually we chose headers as the least likely information to be logged. Several behaviors here are blindly copied from the Foolscap-based storage server protocol. @@ -461,13 +456,13 @@ For example:: {"share-numbers": [1, 7, ...], "allocated-size": 12345} -The request must include ``X-Tahoe-Set-Authorization`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. +The request must include ``X-Tahoe-Authorization`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. Typically this is a header sent by the server, but in Tahoe-LAFS keys are set by the client, so may as well reuse it. For example:: - X-Tahoe-Set-Authorization: lease-renew-secret - X-Tahoe-Set-Authorization: lease-cancel-secret - X-Tahoe-Set-Authorization: upload-secret + X-Tahoe-Authorization: lease-renew-secret + X-Tahoe-Authorization: lease-cancel-secret + X-Tahoe-Authorization: upload-secret The response body includes encoded information about the created buckets. For example:: @@ -475,12 +470,6 @@ For example:: {"already-have": [1, ...], "allocated": [7, ...]} The upload secret is an opaque _byte_ string. -It will be generated by hashing a combination of:b - -1. A tag. -2. The storage index, so it's unique across different source files. -3. The server ID, so it's unique across different servers. -4. The convergence secret, so that servers can't guess the upload secret for other servers. Discussion `````````` @@ -508,7 +497,7 @@ The response includes ``already-have`` and ``allocated`` for two reasons: Regarding upload secrets, the goal is for uploading and aborting (see next sections) to be authenticated by more than just the storage index. -In the future, we will want to generate them in a way that allows resuming/canceling when the client has issues. +In the future, we may want to generate them in a way that allows resuming/canceling when the client has issues. In the short term, they can just be a random byte string. The key security constraint is that each upload to each server has its own, unique upload key, tied to uploading that particular storage index to this particular server. @@ -566,7 +555,7 @@ Responses: This cancels an *in-progress* upload. -The request must include a ``Authorization`` header that includes the upload secret:: +The request must include a ``X-Tahoe-Authorization`` header that includes the upload secret:: X-Tahoe-Authorization: upload-secret @@ -743,9 +732,9 @@ Immutable Data POST /v1/immutable/AAAAAAAAAAAAAAAA Authorization: Tahoe-LAFS nurl-swissnum - X-Tahoe-Set-Authorization: lease-renew-secret efgh - X-Tahoe-Set-Authorization: lease-cancel-secret jjkl - X-Tahoe-Set-Authorization: upload-secret xyzf + X-Tahoe-Authorization: lease-renew-secret efgh + X-Tahoe-Authorization: lease-cancel-secret jjkl + X-Tahoe-Authorization: upload-secret xyzf {"share-numbers": [1, 7], "allocated-size": 48} From bea4cf18a0d7d91dece9fb4a45bb39c5b41b8e9d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 11:19:29 -0500 Subject: [PATCH 33/89] News file. --- newsfragments/3843.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3843.minor diff --git a/newsfragments/3843.minor b/newsfragments/3843.minor new file mode 100644 index 000000000..e69de29bb From e7a5d14c0e8c0077880e2a9ffbd1e3db3738dd93 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 11:25:10 -0500 Subject: [PATCH 34/89] New requirements. --- nix/tahoe-lafs.nix | 2 +- setup.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/nix/tahoe-lafs.nix b/nix/tahoe-lafs.nix index e42afc57f..f691677f6 100644 --- a/nix/tahoe-lafs.nix +++ b/nix/tahoe-lafs.nix @@ -4,7 +4,7 @@ , setuptools, setuptoolsTrial, pyasn1, zope_interface , service-identity, pyyaml, magic-wormhole, treq, appdirs , beautifulsoup4, eliot, autobahn, cryptography, netifaces -, html5lib, pyutil, distro, configparser +, html5lib, pyutil, distro, configparser, klein, treq }: python.pkgs.buildPythonPackage rec { # Most of the time this is not exactly the release version (eg 1.16.0). diff --git a/setup.py b/setup.py index 8c6396937..3d9f5a509 100644 --- a/setup.py +++ b/setup.py @@ -140,6 +140,10 @@ install_requires = [ # For the RangeMap datastructure. "collections-extended", + + # HTTP server and client + "klein", + "treq", ] setup_requires = [ @@ -397,7 +401,6 @@ setup(name="tahoe-lafs", # also set in __init__.py # Python 2.7. "decorator < 5", "hypothesis >= 3.6.1", - "treq", "towncrier", "testtools", "fixtures", From 777d630f481e3010c399d0cc2e872bacd572e700 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 12:00:07 -0500 Subject: [PATCH 35/89] Another dependency. --- nix/tahoe-lafs.nix | 2 +- setup.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/nix/tahoe-lafs.nix b/nix/tahoe-lafs.nix index f691677f6..a6a8a69ec 100644 --- a/nix/tahoe-lafs.nix +++ b/nix/tahoe-lafs.nix @@ -4,7 +4,7 @@ , setuptools, setuptoolsTrial, pyasn1, zope_interface , service-identity, pyyaml, magic-wormhole, treq, appdirs , beautifulsoup4, eliot, autobahn, cryptography, netifaces -, html5lib, pyutil, distro, configparser, klein, treq +, html5lib, pyutil, distro, configparser, klein, treq, cbor2 }: python.pkgs.buildPythonPackage rec { # Most of the time this is not exactly the release version (eg 1.16.0). diff --git a/setup.py b/setup.py index 3d9f5a509..7e7a955c6 100644 --- a/setup.py +++ b/setup.py @@ -144,6 +144,7 @@ install_requires = [ # HTTP server and client "klein", "treq", + "cbor2" ] setup_requires = [ From a32c6be978f0c857ee0465cf123b56058178a21e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 12:02:58 -0500 Subject: [PATCH 36/89] A sketch of what the HTTP server will look like. --- src/allmydata/storage/http_server.py | 66 ++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 src/allmydata/storage/http_server.py diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py new file mode 100644 index 000000000..87edda999 --- /dev/null +++ b/src/allmydata/storage/http_server.py @@ -0,0 +1,66 @@ +""" +HTTP server for storage. +""" + +from functools import wraps + +from klein import Klein +from twisted.web import http + +# Make sure to use pure Python versions: +from cbor2.encoder import dumps +from cbor2.decoder import loads + +from .server import StorageServer + + +def _authorization_decorator(f): + """ + Check the ``Authorization`` header, and (TODO: in later revision of code) + extract ``X-Tahoe-Authorization`` headers and pass them in. + """ + + @wraps(f) + def route(self, request, *args, **kwargs): + if request.headers["Authorization"] != self._swissnum: + request.setResponseCode(http.NOT_ALLOWED) + return b"" + # authorization = request.headers.getRawHeaders("X-Tahoe-Authorization", []) + # For now, just a placeholder: + authorization = None + return f(self, request, authorization, *args, **kwargs) + + +def _route(app, *route_args, **route_kwargs): + """ + Like Klein's @route, but with additional support for checking the + ``Authorization`` header as well as ``X-Tahoe-Authorization`` headers. The + latter will (TODO: in later revision of code) get passed in as second + argument to wrapped functions. + """ + + def decorator(f): + @app.route(*route_args, **route_kwargs) + @_authorization_decorator + def handle_route(*args, **kwargs): + return f(*args, **kwargs) + + return handle_route + + return decorator + + +class HTTPServer(object): + """ + A HTTP interface to the storage server. + """ + + _app = Klein() + + def __init__(self, storage_server: StorageServer, swissnum): + self._storage_server = storage_server + self._swissnum = swissnum + + @_route(_app, "/v1/version", methods=["GET"]) + def version(self, request, authorization): + return dumps(self._storage_server.remote_get_version()) From ddd2780bd243436d3630fdcee8b0340480736e27 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 12:51:52 -0500 Subject: [PATCH 37/89] First sketch of HTTP client. --- src/allmydata/storage/http_client.py | 36 ++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 src/allmydata/storage/http_client.py diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py new file mode 100644 index 000000000..ca80b704e --- /dev/null +++ b/src/allmydata/storage/http_client.py @@ -0,0 +1,36 @@ +""" +HTTP client that talks to the HTTP storage server. +""" + +# Make sure to import Python version: +from cbor2.encoder import loads +from cbor2.decoder import loads + +from twisted.internet.defer import inlineCallbacks, returnValue +from hyperlink import DecodedURL +import treq + + +def _decode_cbor(response): + """Given HTTP response, return decoded CBOR body.""" + return treq.content(response).addCallback(loads) + + +class StorageClient(object): + """ + HTTP client that talks to the HTTP storage server. + """ + + def __init__(self, url: DecodedURL, swissnum, treq=treq): + self._base_url = url + self._swissnum = swissnum + self._treq = treq + + @inlineCallbacks + def get_version(self): + """ + Return the version metadata for the server. + """ + url = self._base_url.child("v1", "version") + response = _decode_cbor((yield self._treq.get(url))) + returnValue(response) From 12cbf8a90109548aaba570d977863bacc2e8fdad Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 13:03:53 -0500 Subject: [PATCH 38/89] First sketch of HTTP testing infrastructure. --- src/allmydata/test/test_storage_http.py | 38 +++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 src/allmydata/test/test_storage_http.py diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py new file mode 100644 index 000000000..589cfdddf --- /dev/null +++ b/src/allmydata/test/test_storage_http.py @@ -0,0 +1,38 @@ +""" +Tests for HTTP storage client + server. +""" + +from twisted.trial.unittest import TestCase +from twisted.internet.defer import inlineCallbacks + +from treq.testing import StubTreq +from hyperlink import DecodedURL + +from ..storage.server import StorageServer +from ..storage.http_server import HTTPServer +from ..storage.http_client import StorageClient + + +class HTTPTests(TestCase): + """ + Tests of HTTP client talking to the HTTP server. + """ + + def setUp(self): + self.storage_server = StorageServer(self.mktemp(), b"\x00" * 20) + # TODO what should the swissnum _actually_ be? + self._http_server = HTTPServer(self._storage_server, b"abcd") + self.client = StorageClient( + DecodedURL.from_text("http://example.com"), + b"abcd", + treq=StubTreq(self._http_server.get_resource()), + ) + + @inlineCallbacks + def test_version(self): + """ + The client can return the version. + """ + version = yield self.client.get_version() + expected_version = self.storage_server.remote_get_version() + self.assertEqual(version, expected_version) From c101dd4dc9e33190da63daedd1963a1fb0e9f7cf Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Nov 2021 13:13:19 -0500 Subject: [PATCH 39/89] Closer to first passing test. --- src/allmydata/storage/http_client.py | 20 +++++++++++++------- src/allmydata/storage/http_server.py | 20 +++++++++++++++----- src/allmydata/test/test_storage_http.py | 2 +- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index ca80b704e..e593fd379 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -2,18 +2,23 @@ HTTP client that talks to the HTTP storage server. """ -# Make sure to import Python version: -from cbor2.encoder import loads -from cbor2.decoder import loads +# TODO Make sure to import Python version? +from cbor2 import loads, dumps -from twisted.internet.defer import inlineCallbacks, returnValue +from twisted.internet.defer import inlineCallbacks, returnValue, fail from hyperlink import DecodedURL import treq +class ClientException(Exception): + """An unexpected error.""" + + def _decode_cbor(response): """Given HTTP response, return decoded CBOR body.""" - return treq.content(response).addCallback(loads) + if response.code > 199 and response.code < 300: + return treq.content(response).addCallback(loads) + return fail(ClientException(response.code, response.phrase)) class StorageClient(object): @@ -32,5 +37,6 @@ class StorageClient(object): Return the version metadata for the server. """ url = self._base_url.child("v1", "version") - response = _decode_cbor((yield self._treq.get(url))) - returnValue(response) + response = yield self._treq.get(url) + decoded_response = yield _decode_cbor(response) + returnValue(decoded_response) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 87edda999..b862fe7b1 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -7,9 +7,8 @@ from functools import wraps from klein import Klein from twisted.web import http -# Make sure to use pure Python versions: -from cbor2.encoder import dumps -from cbor2.decoder import loads +# TODO Make sure to use pure Python versions? +from cbor2 import loads, dumps from .server import StorageServer @@ -22,14 +21,19 @@ def _authorization_decorator(f): @wraps(f) def route(self, request, *args, **kwargs): - if request.headers["Authorization"] != self._swissnum: + if ( + request.requestHeaders.getRawHeaders("Authorization", [None])[0] + != self._swissnum + ): request.setResponseCode(http.NOT_ALLOWED) return b"" - # authorization = request.headers.getRawHeaders("X-Tahoe-Authorization", []) + # authorization = request.requestHeaders.getRawHeaders("X-Tahoe-Authorization", []) # For now, just a placeholder: authorization = None return f(self, request, authorization, *args, **kwargs) + return route + def _route(app, *route_args, **route_kwargs): """ @@ -53,6 +57,8 @@ def _route(app, *route_args, **route_kwargs): class HTTPServer(object): """ A HTTP interface to the storage server. + + TODO returning CBOR should set CBOR content-type """ _app = Klein() @@ -61,6 +67,10 @@ class HTTPServer(object): self._storage_server = storage_server self._swissnum = swissnum + def get_resource(self): + """Return twisted.web Resource for this object.""" + return self._app.resource() + @_route(_app, "/v1/version", methods=["GET"]) def version(self, request, authorization): return dumps(self._storage_server.remote_get_version()) diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index 589cfdddf..663675f40 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -21,7 +21,7 @@ class HTTPTests(TestCase): def setUp(self): self.storage_server = StorageServer(self.mktemp(), b"\x00" * 20) # TODO what should the swissnum _actually_ be? - self._http_server = HTTPServer(self._storage_server, b"abcd") + self._http_server = HTTPServer(self.storage_server, b"abcd") self.client = StorageClient( DecodedURL.from_text("http://example.com"), b"abcd", From c3cb0ebaeaa196c24272ac1fd834ed3c30baa377 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:20:27 -0500 Subject: [PATCH 40/89] Switch to per-call parameter for controlling lease renewal behavior This is closer to an implementation where you could have two frontends, say a Foolscap frontend and an HTTP frontend or even just two different HTTP frontends, which had different opinions about what the behaviour should be. --- src/allmydata/storage/server.py | 51 ++++++------------------ src/allmydata/test/test_storage.py | 63 +++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 53 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 36cf06d0e..70d71f841 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -59,21 +59,10 @@ DEFAULT_RENEWAL_TIME = 31 * 24 * 60 * 60 class StorageServer(service.MultiService, Referenceable): """ A filesystem-based implementation of ``RIStorageServer``. - - :ivar bool _implicit_bucket_lease_renewal: If and only if this is ``True`` - then ``allocate_buckets`` will renew leases on existing shares - associated with the storage index it operates on. - - :ivar bool _implicit_slot_lease_renewal: If and only if this is ``True`` - then ``slot_testv_and_readv_and_writev`` will renew leases on shares - associated with the slot it operates on. """ name = 'storage' LeaseCheckerClass = LeaseCheckingCrawler - _implicit_bucket_lease_renewal = True - _implicit_slot_lease_renewal = True - def __init__(self, storedir, nodeid, reserved_space=0, discard_storage=False, readonly_storage=False, stats_provider=None, @@ -149,29 +138,6 @@ class StorageServer(service.MultiService, Referenceable): def __repr__(self): return "" % (idlib.shortnodeid_b2a(self.my_nodeid),) - def set_implicit_bucket_lease_renewal(self, enabled): - # type: (bool) -> None - """ - Control the behavior of implicit lease renewal by *allocate_buckets*. - - :param enabled: If and only if ``True`` then future *allocate_buckets* - calls will renew leases on shares that already exist in the bucket. - """ - self._implicit_bucket_lease_renewal = enabled - - def set_implicit_slot_lease_renewal(self, enabled): - # type: (bool) -> None - """ - Control the behavior of implicit lease renewal by - *slot_testv_and_readv_and_writev*. - - :param enabled: If and only if ``True`` then future - *slot_testv_and_readv_and_writev* calls will renew leases on - shares that still exist in the slot after the writev is applied - and which were touched by the writev. - """ - self._implicit_slot_lease_renewal = enabled - def have_shares(self): # quick test to decide if we need to commit to an implicit # permutation-seed or if we should use a new one @@ -314,9 +280,12 @@ class StorageServer(service.MultiService, Referenceable): def _allocate_buckets(self, storage_index, renew_secret, cancel_secret, sharenums, allocated_size, - owner_num=0): + owner_num=0, renew_leases=True): """ Generic bucket allocation API. + + :param bool renew_leases: If and only if this is ``True`` then + renew leases on existing shares in this bucket. """ # owner_num is not for clients to set, but rather it should be # curried into the PersonalStorageServer instance that is dedicated @@ -356,7 +325,7 @@ class StorageServer(service.MultiService, Referenceable): # file, they'll want us to hold leases for this file. for (shnum, fn) in self._get_bucket_shares(storage_index): alreadygot.add(shnum) - if self._implicit_bucket_lease_renewal: + if renew_leases: sf = ShareFile(fn) sf.add_or_renew_lease(lease_info) @@ -399,7 +368,7 @@ class StorageServer(service.MultiService, Referenceable): """Foolscap-specific ``allocate_buckets()`` API.""" alreadygot, bucketwriters = self._allocate_buckets( storage_index, renew_secret, cancel_secret, sharenums, allocated_size, - owner_num=owner_num, + owner_num=owner_num, renew_leases=True, ) # Abort BucketWriters if disconnection happens. for bw in bucketwriters.values(): @@ -661,12 +630,17 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, + renew_leases, ): """ Read data from shares and conditionally write some data to them. See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. + + :param bool renew_leases: If and only if this is ``True`` then renew + leases on all shares mentioned in ``test_and_write_vectors` that + still exist after the changes are made. """ start = self._get_current_time() self.count("writev") @@ -704,7 +678,7 @@ class StorageServer(service.MultiService, Referenceable): test_and_write_vectors, shares, ) - if self._implicit_slot_lease_renewal: + if renew_leases: lease_info = self._make_lease_info(renew_secret, cancel_secret) self._add_or_renew_leases(remaining_shares, lease_info) @@ -721,6 +695,7 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, + renew_leases=True, ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index efa889f8d..a6c1ac2c2 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -468,14 +468,19 @@ class Server(unittest.TestCase): sv1 = ver[b'http://allmydata.org/tahoe/protocols/storage/v1'] self.failUnlessIn(b'available-space', sv1) - def allocate(self, ss, storage_index, sharenums, size, canary=None): + def allocate(self, ss, storage_index, sharenums, size, renew_leases=True): + """ + Call directly into the storage server's allocate_buckets implementation, + skipping the Foolscap layer. + """ renew_secret = hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)) cancel_secret = hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)) - if not canary: - canary = FakeCanary() - return ss.remote_allocate_buckets(storage_index, - renew_secret, cancel_secret, - sharenums, size, canary) + return ss._allocate_buckets( + storage_index, + renew_secret, cancel_secret, + sharenums, size, + renew_leases=renew_leases, + ) def test_large_share(self): syslow = platform.system().lower() @@ -611,7 +616,7 @@ class Server(unittest.TestCase): def test_allocate_without_lease_renewal(self): """ ``remote_allocate_buckets`` does not renew leases on existing shares if - ``set_implicit_bucket_lease_renewal(False)`` is called first. + ``renew_leases`` is ``False``. """ first_lease = 456 second_lease = 543 @@ -623,10 +628,11 @@ class Server(unittest.TestCase): "test_allocate_without_lease_renewal", get_current_time=clock.seconds, ) - ss.set_implicit_bucket_lease_renewal(False) # Put a share on there - already, writers = self.allocate(ss, storage_index, [0], 1) + already, writers = self.allocate( + ss, storage_index, [0], 1, renew_leases=False, + ) (writer,) = writers.values() writer.remote_write(0, b"x") writer.remote_close() @@ -647,7 +653,9 @@ class Server(unittest.TestCase): clock.advance(second_lease) # Put another share on there. - already, writers = self.allocate(ss, storage_index, [1], 1) + already, writers = self.allocate( + ss, storage_index, [1], 1, renew_leases=False, + ) (writer,) = writers.values() writer.remote_write(0, b"x") writer.remote_close() @@ -684,8 +692,17 @@ class Server(unittest.TestCase): def test_disconnect(self): # simulate a disconnection ss = self.create("test_disconnect") + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 canary = FakeCanary() - already,writers = self.allocate(ss, b"disconnect", [0,1,2], 75, canary) + already,writers = ss.remote_allocate_buckets( + b"disconnect", + renew_secret, + cancel_secret, + sharenums=[0,1,2], + allocated_size=75, + canary=canary, + ) self.failUnlessEqual(already, set()) self.failUnlessEqual(set(writers.keys()), set([0,1,2])) for (f,args,kwargs) in list(canary.disconnectors.values()): @@ -717,8 +734,17 @@ class Server(unittest.TestCase): # the size we request. OVERHEAD = 3*4 LEASE_SIZE = 4+32+32+4 + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 canary = FakeCanary() - already, writers = self.allocate(ss, b"vid1", [0,1,2], 1000, canary) + already, writers = ss.remote_allocate_buckets( + b"vid1", + renew_secret, + cancel_secret, + sharenums=[0,1,2], + allocated_size=1000, + canary=canary, + ) self.failUnlessEqual(len(writers), 3) # now the StorageServer should have 3000 bytes provisionally # allocated, allowing only 2000 more to be claimed @@ -751,7 +777,14 @@ class Server(unittest.TestCase): # now there should be ALLOCATED=1001+12+72=1085 bytes allocated, and # 5000-1085=3915 free, therefore we can fit 39 100byte shares canary3 = FakeCanary() - already3, writers3 = self.allocate(ss, b"vid3", list(range(100)), 100, canary3) + already3, writers3 = ss.remote_allocate_buckets( + b"vid3", + renew_secret, + cancel_secret, + sharenums=list(range(100)), + allocated_size=100, + canary=canary3, + ) self.failUnlessEqual(len(writers3), 39) self.failUnlessEqual(len(ss._bucket_writers), 39) @@ -1463,10 +1496,9 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``set_implicit_bucket_lease_renewal(False)`` is called first. + leases if ``renew_leases```` is ``False``. """ ss = self.create("test_writev_without_renew_lease") - ss.set_implicit_slot_lease_renewal(False) storage_index = b"si2" secrets = ( @@ -1485,6 +1517,7 @@ class MutableServer(unittest.TestCase): sharenum: ([], datav, None), }, read_vector=[], + renew_leases=False, ) leases = list(ss.get_slot_leases(storage_index)) self.assertEqual([], leases) From 85977e48a7dde8ea29e196a6d466ae8685c2f6fc Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:23:15 -0500 Subject: [PATCH 41/89] put this comment back and merge info from the two versions --- src/allmydata/storage/server.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 70d71f841..9b73963ae 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -635,12 +635,13 @@ class StorageServer(service.MultiService, Referenceable): """ Read data from shares and conditionally write some data to them. + :param bool renew_leases: If and only if this is ``True`` and the test + vectors pass then shares mentioned in ``test_and_write_vectors`` + that still exist after the changes are made will also have an + updated lease applied to them. + See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. - - :param bool renew_leases: If and only if this is ``True`` then renew - leases on all shares mentioned in ``test_and_write_vectors` that - still exist after the changes are made. """ start = self._get_current_time() self.count("writev") From dece67ee3ac8d2bd06b42e07a01492e3c4497ae6 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:24:29 -0500 Subject: [PATCH 42/89] it is not the remote interface that varies anymore --- src/allmydata/test/test_storage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index a6c1ac2c2..076e9f3d1 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -615,8 +615,8 @@ class Server(unittest.TestCase): def test_allocate_without_lease_renewal(self): """ - ``remote_allocate_buckets`` does not renew leases on existing shares if - ``renew_leases`` is ``False``. + ``StorageServer._allocate_buckets`` does not renew leases on existing + shares if ``renew_leases`` is ``False``. """ first_lease = 456 second_lease = 543 From 6c2e85e99145652625ff7a4d6791a410ce13c742 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:25:36 -0500 Subject: [PATCH 43/89] put the comment back --- src/allmydata/test/test_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 076e9f3d1..4e40a76a5 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -1496,7 +1496,7 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``renew_leases```` is ``False``. + leases if ``False`` is passed for the ``renew_leases`` parameter. """ ss = self.create("test_writev_without_renew_lease") From ad6017e63df94dbac7916f4673332b33deb8d5be Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 15 Nov 2021 08:08:14 -0500 Subject: [PATCH 44/89] clarify renew_leases docs on allocate_buckets --- src/allmydata/storage/server.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 9b73963ae..bfbc10b59 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -284,8 +284,10 @@ class StorageServer(service.MultiService, Referenceable): """ Generic bucket allocation API. - :param bool renew_leases: If and only if this is ``True`` then - renew leases on existing shares in this bucket. + :param bool renew_leases: If and only if this is ``True`` then renew a + secret-matching lease on (or, if none match, add a new lease to) + existing shares in this bucket. Any *new* shares are given a new + lease regardless. """ # owner_num is not for clients to set, but rather it should be # curried into the PersonalStorageServer instance that is dedicated From 84c19f5468b04279e1826ed77dc1e7d4b4ae00e8 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 15 Nov 2021 08:12:07 -0500 Subject: [PATCH 45/89] clarify renew_leases docs on slot_testv_and_readv_and_writev --- src/allmydata/storage/server.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index bfbc10b59..ee2ea1c61 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -639,8 +639,9 @@ class StorageServer(service.MultiService, Referenceable): :param bool renew_leases: If and only if this is ``True`` and the test vectors pass then shares mentioned in ``test_and_write_vectors`` - that still exist after the changes are made will also have an - updated lease applied to them. + that still exist after the changes are made will also have a + secret-matching lease renewed (or, if none match, a new lease + added). See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. From fcd634fc43c42c838ac415767bd6eeb05172c82b Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 15 Nov 2021 13:34:46 -0500 Subject: [PATCH 46/89] some direct tests for the new utility function --- src/allmydata/test/common.py | 7 ++- src/allmydata/test/test_common_util.py | 78 +++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 8e97fa598..76127fb57 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -1220,8 +1220,13 @@ def disable_modules(*names): A context manager which makes modules appear to be missing while it is active. - :param *names: The names of the modules to disappear. + :param *names: The names of the modules to disappear. Only top-level + modules are supported (that is, "." is not allowed in any names). + This is an implementation shortcoming which could be lifted if + desired. """ + if any("." in name for name in names): + raise ValueError("Names containing '.' are not supported.") missing = object() modules = list(sys.modules.get(n, missing) for n in names) for n in names: diff --git a/src/allmydata/test/test_common_util.py b/src/allmydata/test/test_common_util.py index 55986d123..c141adc8d 100644 --- a/src/allmydata/test/test_common_util.py +++ b/src/allmydata/test/test_common_util.py @@ -10,16 +10,30 @@ from future.utils import PY2 if PY2: from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 +import sys import random -import unittest +from hypothesis import given +from hypothesis.strategies import lists, sampled_from +from testtools.matchers import Equals +from twisted.python.reflect import ( + ModuleNotFound, + namedAny, +) + +from .common import ( + SyncTestCase, + disable_modules, +) from allmydata.test.common_util import flip_one_bit -class TestFlipOneBit(unittest.TestCase): +class TestFlipOneBit(SyncTestCase): def setUp(self): - random.seed(42) # I tried using version=1 on PY3 to avoid the if below, to no avail. + super(TestFlipOneBit, self).setUp() + # I tried using version=1 on PY3 to avoid the if below, to no avail. + random.seed(42) def test_accepts_byte_string(self): actual = flip_one_bit(b'foo') @@ -27,3 +41,61 @@ class TestFlipOneBit(unittest.TestCase): def test_rejects_unicode_string(self): self.assertRaises(AssertionError, flip_one_bit, u'foo') + + + +def some_existing_modules(): + """ + Build the names of modules (as native strings) that exist and can be + imported. + """ + candidates = sorted( + name + for name + in sys.modules + if "." not in name + and sys.modules[name] is not None + ) + return sampled_from(candidates) + +class DisableModulesTests(SyncTestCase): + """ + Tests for ``disable_modules``. + """ + def setup_example(self): + return sys.modules.copy() + + def teardown_example(self, safe_modules): + sys.modules.update(safe_modules) + + @given(lists(some_existing_modules(), unique=True)) + def test_importerror(self, module_names): + """ + While the ``disable_modules`` context manager is active any import of the + modules identified by the names passed to it result in ``ImportError`` + being raised. + """ + def get_modules(): + return list( + namedAny(name) + for name + in module_names + ) + before_modules = get_modules() + + with disable_modules(*module_names): + for name in module_names: + with self.assertRaises(ModuleNotFound): + namedAny(name) + + after_modules = get_modules() + self.assertThat(before_modules, Equals(after_modules)) + + def test_dotted_names_rejected(self): + """ + If names with "." in them are passed to ``disable_modules`` then + ``ValueError`` is raised. + """ + with self.assertRaises(ValueError): + with disable_modules("foo.bar"): + pass From 304b0269e3afe6499eaa1a92abd4856c970da60b Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 10:14:04 -0500 Subject: [PATCH 47/89] Apply suggestions from code review Co-authored-by: Jean-Paul Calderone --- docs/proposed/http-storage-node-protocol.rst | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index 44bda1205..bc109ac7e 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -400,8 +400,8 @@ Either renew or create a new lease on the bucket addressed by ``storage_index``. The renew secret and cancellation secret should be included as ``X-Tahoe-Authorization`` headers. For example:: - X-Tahoe-Authorization: lease-renew-secret - X-Tahoe-Authorization: lease-cancel-secret + X-Tahoe-Authorization: lease-renew-secret + X-Tahoe-Authorization: lease-cancel-secret If the ``lease-renew-secret`` value matches an existing lease then the expiration time of that lease will be changed to 31 days after the time of this operation. @@ -457,7 +457,6 @@ For example:: {"share-numbers": [1, 7, ...], "allocated-size": 12345} The request must include ``X-Tahoe-Authorization`` HTTP headers that set the various secrets—upload, lease renewal, lease cancellation—that will be later used to authorize various operations. -Typically this is a header sent by the server, but in Tahoe-LAFS keys are set by the client, so may as well reuse it. For example:: X-Tahoe-Authorization: lease-renew-secret @@ -499,7 +498,7 @@ Regarding upload secrets, the goal is for uploading and aborting (see next sections) to be authenticated by more than just the storage index. In the future, we may want to generate them in a way that allows resuming/canceling when the client has issues. In the short term, they can just be a random byte string. -The key security constraint is that each upload to each server has its own, unique upload key, +The primary security constraint is that each upload to each server has its own unique upload key, tied to uploading that particular storage index to this particular server. Rejected designs for upload secrets: @@ -527,7 +526,7 @@ The server must recognize when all of the data has been received and mark the sh The request must include a ``X-Tahoe-Authorization`` header that includes the upload secret:: - X-Tahoe-Authorization: upload-secret + X-Tahoe-Authorization: upload-secret Responses: @@ -557,7 +556,7 @@ This cancels an *in-progress* upload. The request must include a ``X-Tahoe-Authorization`` header that includes the upload secret:: - X-Tahoe-Authorization: upload-secret + X-Tahoe-Authorization: upload-secret The response code: @@ -658,7 +657,7 @@ there is no separate "create this storage index" operation as there is for the i The request must include ``X-Tahoe-Authorization`` headers with write enabler and lease secrets:: - X-Tahoe-Authorization: write-enabler + X-Tahoe-Authorization: write-enabler X-Tahoe-Authorization: lease-cancel-secret X-Tahoe-Authorization: lease-renew-secret From 7caffce8d509e1293248cb83d89e81e030b88e16 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 10:14:19 -0500 Subject: [PATCH 48/89] Another review suggestion Co-authored-by: Jean-Paul Calderone --- docs/proposed/http-storage-node-protocol.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst index bc109ac7e..490d3f3ca 100644 --- a/docs/proposed/http-storage-node-protocol.rst +++ b/docs/proposed/http-storage-node-protocol.rst @@ -780,7 +780,7 @@ Immutable Data PUT /v1/lease/AAAAAAAAAAAAAAAA Authorization: Tahoe-LAFS nurl-swissnum X-Tahoe-Authorization: lease-cancel-secret jjkl - X-Tahoe-Authorization: upload-secret xyzf + X-Tahoe-Authorization: lease-renew-secret efgh 204 NO CONTENT From 41ec63f7586124eaaf9ca65bb4d6c4884e16b48f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 10:56:21 -0500 Subject: [PATCH 49/89] Passing first tests. --- src/allmydata/storage/http_client.py | 22 ++++++++++++++++++++-- src/allmydata/storage/http_server.py | 8 ++++---- src/allmydata/test/test_storage_http.py | 19 +++++++++++++++++-- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index e593fd379..412bf9cec 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -2,9 +2,13 @@ HTTP client that talks to the HTTP storage server. """ +import base64 + # TODO Make sure to import Python version? from cbor2 import loads, dumps + +from twisted.web.http_headers import Headers from twisted.internet.defer import inlineCallbacks, returnValue, fail from hyperlink import DecodedURL import treq @@ -21,6 +25,11 @@ def _decode_cbor(response): return fail(ClientException(response.code, response.phrase)) +def swissnum_auth_header(swissnum): + """Return value for ``Authentication`` header.""" + return b"Tahoe-LAFS " + base64.encodestring(swissnum).strip() + + class StorageClient(object): """ HTTP client that talks to the HTTP storage server. @@ -31,12 +40,21 @@ class StorageClient(object): self._swissnum = swissnum self._treq = treq + def _get_headers(self): + """Return the basic headers to be used by default.""" + headers = Headers() + headers.addRawHeader( + "Authorization", + swissnum_auth_header(self._swissnum), + ) + return headers + @inlineCallbacks def get_version(self): """ Return the version metadata for the server. """ - url = self._base_url.child("v1", "version") - response = yield self._treq.get(url) + url = self._base_url.click("/v1/version") + response = yield self._treq.get(url, headers=self._get_headers()) decoded_response = yield _decode_cbor(response) returnValue(decoded_response) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index b862fe7b1..2d6308baf 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -11,6 +11,7 @@ from twisted.web import http from cbor2 import loads, dumps from .server import StorageServer +from .http_client import swissnum_auth_header def _authorization_decorator(f): @@ -21,11 +22,10 @@ def _authorization_decorator(f): @wraps(f) def route(self, request, *args, **kwargs): - if ( - request.requestHeaders.getRawHeaders("Authorization", [None])[0] - != self._swissnum + if request.requestHeaders.getRawHeaders("Authorization", [None])[0] != str( + swissnum_auth_header(self._swissnum), "ascii" ): - request.setResponseCode(http.NOT_ALLOWED) + request.setResponseCode(http.UNAUTHORIZED) return b"" # authorization = request.requestHeaders.getRawHeaders("X-Tahoe-Authorization", []) # For now, just a placeholder: diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index 663675f40..b659a6ace 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -10,7 +10,7 @@ from hyperlink import DecodedURL from ..storage.server import StorageServer from ..storage.http_server import HTTPServer -from ..storage.http_client import StorageClient +from ..storage.http_client import StorageClient, ClientException class HTTPTests(TestCase): @@ -23,11 +23,26 @@ class HTTPTests(TestCase): # TODO what should the swissnum _actually_ be? self._http_server = HTTPServer(self.storage_server, b"abcd") self.client = StorageClient( - DecodedURL.from_text("http://example.com"), + DecodedURL.from_text("http://127.0.0.1"), b"abcd", treq=StubTreq(self._http_server.get_resource()), ) + @inlineCallbacks + def test_bad_authentication(self): + """ + If the wrong swissnum is used, an ``Unauthorized`` response code is + returned. + """ + client = StorageClient( + DecodedURL.from_text("http://127.0.0.1"), + b"something wrong", + treq=StubTreq(self._http_server.get_resource()), + ) + with self.assertRaises(ClientException) as e: + yield client.get_version() + self.assertEqual(e.exception.args[0], 401) + @inlineCallbacks def test_version(self): """ From 671b670154f62cb6c7876c707f254a6c7b3a2f4f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:09:08 -0500 Subject: [PATCH 50/89] Some type annotations. --- src/allmydata/storage/http_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 412bf9cec..8e14d1137 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -25,7 +25,7 @@ def _decode_cbor(response): return fail(ClientException(response.code, response.phrase)) -def swissnum_auth_header(swissnum): +def swissnum_auth_header(swissnum): # type: (bytes) -> bytes """Return value for ``Authentication`` header.""" return b"Tahoe-LAFS " + base64.encodestring(swissnum).strip() @@ -40,7 +40,7 @@ class StorageClient(object): self._swissnum = swissnum self._treq = treq - def _get_headers(self): + def _get_headers(self): # type: () -> Headers """Return the basic headers to be used by default.""" headers = Headers() headers.addRawHeader( From 171d1053ec803f2d2de57f0970fbad049d49f2da Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:09:17 -0500 Subject: [PATCH 51/89] CBOR content-type on responses. --- src/allmydata/storage/http_server.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 2d6308baf..91387c58f 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -57,8 +57,6 @@ def _route(app, *route_args, **route_kwargs): class HTTPServer(object): """ A HTTP interface to the storage server. - - TODO returning CBOR should set CBOR content-type """ _app = Klein() @@ -71,6 +69,12 @@ class HTTPServer(object): """Return twisted.web Resource for this object.""" return self._app.resource() + def _cbor(self, request, data): + """Return CBOR-encoded data.""" + request.setHeader("Content-Type", "application/cbor") + # TODO if data is big, maybe want to use a temporary file eventually... + return dumps(data) + @_route(_app, "/v1/version", methods=["GET"]) def version(self, request, authorization): - return dumps(self._storage_server.remote_get_version()) + return self._cbor(request, self._storage_server.remote_get_version()) From c195f895db7bd3ec7a8618956a71e67152e32df7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:16:26 -0500 Subject: [PATCH 52/89] Python 2 support. --- src/allmydata/storage/http_client.py | 19 ++++++++++++++++++- src/allmydata/storage/http_server.py | 18 ++++++++++++++++-- src/allmydata/test/test_storage_http.py | 12 ++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 8e14d1137..4a143a60b 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -2,6 +2,21 @@ HTTP client that talks to the HTTP storage server. """ +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function +from __future__ import unicode_literals + +from future.utils import PY2 + +if PY2: + # fmt: off + from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 + # fmt: on +else: + from typing import Union + from treq.testing import StubTreq + import base64 # TODO Make sure to import Python version? @@ -35,7 +50,9 @@ class StorageClient(object): HTTP client that talks to the HTTP storage server. """ - def __init__(self, url: DecodedURL, swissnum, treq=treq): + def __init__( + self, url, swissnum, treq=treq + ): # type: (DecodedURL, bytes, Union[treq,StubTreq]) -> None self._base_url = url self._swissnum = swissnum self._treq = treq diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 91387c58f..373d31e2e 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -2,6 +2,18 @@ HTTP server for storage. """ +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function +from __future__ import unicode_literals + +from future.utils import PY2 + +if PY2: + # fmt: off + from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 + # fmt: on + from functools import wraps from klein import Klein @@ -61,12 +73,14 @@ class HTTPServer(object): _app = Klein() - def __init__(self, storage_server: StorageServer, swissnum): + def __init__( + self, storage_server, swissnum + ): # type: (StorageServer, bytes) -> None self._storage_server = storage_server self._swissnum = swissnum def get_resource(self): - """Return twisted.web Resource for this object.""" + """Return twisted.web ``Resource`` for this object.""" return self._app.resource() def _cbor(self, request, data): diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index b659a6ace..9ba8adf21 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -2,6 +2,18 @@ Tests for HTTP storage client + server. """ +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function +from __future__ import unicode_literals + +from future.utils import PY2 + +if PY2: + # fmt: off + from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 + # fmt: on + from twisted.trial.unittest import TestCase from twisted.internet.defer import inlineCallbacks From a64778ddb0fb774ea43fa8a3c59be67b84e957ff Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:28:13 -0500 Subject: [PATCH 53/89] Flakes. --- src/allmydata/storage/http_client.py | 2 +- src/allmydata/storage/http_server.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 4a143a60b..d5ca6caec 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -20,7 +20,7 @@ else: import base64 # TODO Make sure to import Python version? -from cbor2 import loads, dumps +from cbor2 import loads from twisted.web.http_headers import Headers diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 373d31e2e..3baa336fa 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -20,7 +20,7 @@ from klein import Klein from twisted.web import http # TODO Make sure to use pure Python versions? -from cbor2 import loads, dumps +from cbor2 import dumps from .server import StorageServer from .http_client import swissnum_auth_header From e5b5b50602268314e035c89e56b740c745b85c84 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:28:19 -0500 Subject: [PATCH 54/89] Duplicate package. --- nix/tahoe-lafs.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nix/tahoe-lafs.nix b/nix/tahoe-lafs.nix index a6a8a69ec..8092dfaa7 100644 --- a/nix/tahoe-lafs.nix +++ b/nix/tahoe-lafs.nix @@ -95,7 +95,7 @@ EOF propagatedBuildInputs = with python.pkgs; [ twisted foolscap zfec appdirs setuptoolsTrial pyasn1 zope_interface - service-identity pyyaml magic-wormhole treq + service-identity pyyaml magic-wormhole eliot autobahn cryptography netifaces setuptools future pyutil distro configparser collections-extended ]; From a1424e90e18ae1dfbed245277120fdf3f0aaedc8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:34:44 -0500 Subject: [PATCH 55/89] Another duplicate. --- nix/tahoe-lafs.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nix/tahoe-lafs.nix b/nix/tahoe-lafs.nix index 8092dfaa7..df12f21d4 100644 --- a/nix/tahoe-lafs.nix +++ b/nix/tahoe-lafs.nix @@ -4,7 +4,7 @@ , setuptools, setuptoolsTrial, pyasn1, zope_interface , service-identity, pyyaml, magic-wormhole, treq, appdirs , beautifulsoup4, eliot, autobahn, cryptography, netifaces -, html5lib, pyutil, distro, configparser, klein, treq, cbor2 +, html5lib, pyutil, distro, configparser, klein, cbor2 }: python.pkgs.buildPythonPackage rec { # Most of the time this is not exactly the release version (eg 1.16.0). From f549488bb508a8377d968d16addb07a98559d8fd Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 16 Nov 2021 11:47:09 -0500 Subject: [PATCH 56/89] Don't use a deprecated API. --- src/allmydata/storage/http_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index d5ca6caec..e1743343d 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -42,7 +42,7 @@ def _decode_cbor(response): def swissnum_auth_header(swissnum): # type: (bytes) -> bytes """Return value for ``Authentication`` header.""" - return b"Tahoe-LAFS " + base64.encodestring(swissnum).strip() + return b"Tahoe-LAFS " + base64.b64encode(swissnum).strip() class StorageClient(object): From 6a78703675e9ce9e42a8401ac38410d78357a86e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 17 Nov 2021 10:53:51 -0500 Subject: [PATCH 57/89] News file. --- newsfragments/3807.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3807.feature diff --git a/newsfragments/3807.feature b/newsfragments/3807.feature new file mode 100644 index 000000000..f82363ffd --- /dev/null +++ b/newsfragments/3807.feature @@ -0,0 +1 @@ +If uploading an immutable hasn't had a write for 30 minutes, the storage server will abort the upload. \ No newline at end of file From 92c36a67d8c98436e2cc2616d89ff0135307858c Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 17 Nov 2021 11:01:04 -0500 Subject: [PATCH 58/89] Use IReactorTime instead of ad-hoc solutions. --- src/allmydata/storage/server.py | 39 ++++++++++++----------- src/allmydata/test/test_istorageserver.py | 10 +++--- src/allmydata/test/test_storage.py | 16 +++++----- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index ee2ea1c61..499d47276 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -20,6 +20,7 @@ import six from foolscap.api import Referenceable from foolscap.ipb import IRemoteReference from twisted.application import service +from twisted.internet import reactor from zope.interface import implementer from allmydata.interfaces import RIStorageServer, IStatsProducer @@ -71,7 +72,7 @@ class StorageServer(service.MultiService, Referenceable): expiration_override_lease_duration=None, expiration_cutoff_date=None, expiration_sharetypes=("mutable", "immutable"), - get_current_time=time.time): + clock=reactor): service.MultiService.__init__(self) assert isinstance(nodeid, bytes) assert len(nodeid) == 20 @@ -122,7 +123,7 @@ class StorageServer(service.MultiService, Referenceable): expiration_cutoff_date, expiration_sharetypes) self.lease_checker.setServiceParent(self) - self._get_current_time = get_current_time + self._clock = clock # Currently being-written Bucketwriters. For Foolscap, lifetime is tied # to connection: when disconnection happens, the BucketWriters are @@ -292,7 +293,7 @@ class StorageServer(service.MultiService, Referenceable): # owner_num is not for clients to set, but rather it should be # curried into the PersonalStorageServer instance that is dedicated # to a particular owner. - start = self._get_current_time() + start = self._clock.seconds() self.count("allocate") alreadygot = set() bucketwriters = {} # k: shnum, v: BucketWriter @@ -305,7 +306,7 @@ class StorageServer(service.MultiService, Referenceable): # goes into the share files themselves. It could also be put into a # separate database. Note that the lease should not be added until # the BucketWriter has been closed. - expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME + expire_time = self._clock.seconds() + DEFAULT_RENEWAL_TIME lease_info = LeaseInfo(owner_num, renew_secret, cancel_secret, expire_time, self.my_nodeid) @@ -360,7 +361,7 @@ class StorageServer(service.MultiService, Referenceable): if bucketwriters: fileutil.make_dirs(os.path.join(self.sharedir, si_dir)) - self.add_latency("allocate", self._get_current_time() - start) + self.add_latency("allocate", self._clock.seconds() - start) return alreadygot, bucketwriters def remote_allocate_buckets(self, storage_index, @@ -395,26 +396,26 @@ class StorageServer(service.MultiService, Referenceable): def remote_add_lease(self, storage_index, renew_secret, cancel_secret, owner_num=1): - start = self._get_current_time() + start = self._clock.seconds() self.count("add-lease") - new_expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME + new_expire_time = self._clock.seconds() + DEFAULT_RENEWAL_TIME lease_info = LeaseInfo(owner_num, renew_secret, cancel_secret, new_expire_time, self.my_nodeid) for sf in self._iter_share_files(storage_index): sf.add_or_renew_lease(lease_info) - self.add_latency("add-lease", self._get_current_time() - start) + self.add_latency("add-lease", self._clock.seconds() - start) return None def remote_renew_lease(self, storage_index, renew_secret): - start = self._get_current_time() + start = self._clock.seconds() self.count("renew") - new_expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME + new_expire_time = self._clock.seconds() + DEFAULT_RENEWAL_TIME found_buckets = False for sf in self._iter_share_files(storage_index): found_buckets = True sf.renew_lease(renew_secret, new_expire_time) - self.add_latency("renew", self._get_current_time() - start) + self.add_latency("renew", self._clock.seconds() - start) if not found_buckets: raise IndexError("no such lease to renew") @@ -441,7 +442,7 @@ class StorageServer(service.MultiService, Referenceable): pass def remote_get_buckets(self, storage_index): - start = self._get_current_time() + start = self._clock.seconds() self.count("get") si_s = si_b2a(storage_index) log.msg("storage: get_buckets %r" % si_s) @@ -449,7 +450,7 @@ class StorageServer(service.MultiService, Referenceable): for shnum, filename in self._get_bucket_shares(storage_index): bucketreaders[shnum] = BucketReader(self, filename, storage_index, shnum) - self.add_latency("get", self._get_current_time() - start) + self.add_latency("get", self._clock.seconds() - start) return bucketreaders def get_leases(self, storage_index): @@ -608,7 +609,7 @@ class StorageServer(service.MultiService, Referenceable): :return LeaseInfo: Information for a new lease for a share. """ ownerid = 1 # TODO - expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME + expire_time = self._clock.seconds() + DEFAULT_RENEWAL_TIME lease_info = LeaseInfo(ownerid, renew_secret, cancel_secret, expire_time, self.my_nodeid) @@ -646,7 +647,7 @@ class StorageServer(service.MultiService, Referenceable): See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. """ - start = self._get_current_time() + start = self._clock.seconds() self.count("writev") si_s = si_b2a(storage_index) log.msg("storage: slot_writev %r" % si_s) @@ -687,7 +688,7 @@ class StorageServer(service.MultiService, Referenceable): self._add_or_renew_leases(remaining_shares, lease_info) # all done - self.add_latency("writev", self._get_current_time() - start) + self.add_latency("writev", self._clock.seconds() - start) return (testv_is_good, read_data) def remote_slot_testv_and_readv_and_writev(self, storage_index, @@ -713,7 +714,7 @@ class StorageServer(service.MultiService, Referenceable): return share def remote_slot_readv(self, storage_index, shares, readv): - start = self._get_current_time() + start = self._clock.seconds() self.count("readv") si_s = si_b2a(storage_index) lp = log.msg("storage: slot_readv %r %r" % (si_s, shares), @@ -722,7 +723,7 @@ class StorageServer(service.MultiService, Referenceable): # shares exist if there is a file for them bucketdir = os.path.join(self.sharedir, si_dir) if not os.path.isdir(bucketdir): - self.add_latency("readv", self._get_current_time() - start) + self.add_latency("readv", self._clock.seconds() - start) return {} datavs = {} for sharenum_s in os.listdir(bucketdir): @@ -736,7 +737,7 @@ class StorageServer(service.MultiService, Referenceable): datavs[sharenum] = msf.readv(readv) log.msg("returning shares %s" % (list(datavs.keys()),), facility="tahoe.storage", level=log.NOISY, parent=lp) - self.add_latency("readv", self._get_current_time() - start) + self.add_latency("readv", self._clock.seconds() - start) return datavs def remote_advise_corrupt_share(self, share_type, storage_index, shnum, diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index fe494a9d4..a17264713 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -21,6 +21,7 @@ if PY2: from random import Random from twisted.internet.defer import inlineCallbacks, returnValue +from twisted.internet.task import Clock from foolscap.api import Referenceable, RemoteException @@ -1017,16 +1018,17 @@ class _FoolscapMixin(SystemTestMixin): self.server = s break assert self.server is not None, "Couldn't find StorageServer" - self._current_time = 123456 - self.server._get_current_time = self.fake_time + self._clock = Clock() + self._clock.advance(123456) + self.server._clock = self._clock def fake_time(self): """Return the current fake, test-controlled, time.""" - return self._current_time + return self._clock.seconds() def fake_sleep(self, seconds): """Advance the fake time by the given number of seconds.""" - self._current_time += seconds + self._clock.advance(seconds) @inlineCallbacks def tearDown(self): diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 4e40a76a5..e143bec63 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -23,7 +23,7 @@ from uuid import uuid4 from twisted.trial import unittest -from twisted.internet import defer +from twisted.internet import defer, reactor from twisted.internet.task import Clock from hypothesis import given, strategies @@ -438,11 +438,11 @@ class Server(unittest.TestCase): basedir = os.path.join("storage", "Server", name) return basedir - def create(self, name, reserved_space=0, klass=StorageServer, get_current_time=time.time): + def create(self, name, reserved_space=0, klass=StorageServer, clock=reactor): workdir = self.workdir(name) ss = klass(workdir, b"\x00" * 20, reserved_space=reserved_space, stats_provider=FakeStatsProvider(), - get_current_time=get_current_time) + clock=clock) ss.setServiceParent(self.sparent) return ss @@ -626,7 +626,7 @@ class Server(unittest.TestCase): clock.advance(first_lease) ss = self.create( "test_allocate_without_lease_renewal", - get_current_time=clock.seconds, + clock=clock, ) # Put a share on there @@ -918,7 +918,7 @@ class Server(unittest.TestCase): """ clock = Clock() clock.advance(123) - ss = self.create("test_immutable_add_lease_renews", get_current_time=clock.seconds) + ss = self.create("test_immutable_add_lease_renews", clock=clock) # Start out with single lease created with bucket: renewal_secret, cancel_secret = self.create_bucket_5_shares(ss, b"si0") @@ -1032,10 +1032,10 @@ class MutableServer(unittest.TestCase): basedir = os.path.join("storage", "MutableServer", name) return basedir - def create(self, name, get_current_time=time.time): + def create(self, name, clock=reactor): workdir = self.workdir(name) ss = StorageServer(workdir, b"\x00" * 20, - get_current_time=get_current_time) + clock=clock) ss.setServiceParent(self.sparent) return ss @@ -1420,7 +1420,7 @@ class MutableServer(unittest.TestCase): clock = Clock() clock.advance(235) ss = self.create("test_mutable_add_lease_renews", - get_current_time=clock.seconds) + clock=clock) def secrets(n): return ( self.write_enabler(b"we1"), self.renew_secret(b"we1-%d" % n), From bf7d03310fc1bd730ba12449112144f3e6935020 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 17 Nov 2021 11:09:45 -0500 Subject: [PATCH 59/89] Hide all _trial_temp. --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 50a1352a2..7c7fa2afd 100644 --- a/.gitignore +++ b/.gitignore @@ -29,7 +29,7 @@ zope.interface-*.egg .pc /src/allmydata/test/plugins/dropin.cache -/_trial_temp* +**/_trial_temp* /tmp* /*.patch /dist/ From 45c00e93c9709e216fe87410783b0a6125e159e7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 17 Nov 2021 11:12:40 -0500 Subject: [PATCH 60/89] Use clock in BucketWriter. --- src/allmydata/storage/immutable.py | 11 ++++++----- src/allmydata/storage/server.py | 3 ++- src/allmydata/test/test_storage.py | 12 ++++++------ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 8a7a5a966..7cfb7a1bf 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -233,7 +233,7 @@ class ShareFile(object): @implementer(RIBucketWriter) class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 - def __init__(self, ss, incominghome, finalhome, max_size, lease_info): + def __init__(self, ss, incominghome, finalhome, max_size, lease_info, clock): self.ss = ss self.incominghome = incominghome self.finalhome = finalhome @@ -245,12 +245,13 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 # added by simultaneous uploaders self._sharefile.add_lease(lease_info) self._already_written = RangeMap() + self._clock = clock def allocated_size(self): return self._max_size def remote_write(self, offset, data): - start = time.time() + start = self._clock.seconds() precondition(not self.closed) if self.throw_out_all_data: return @@ -268,12 +269,12 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 self._sharefile.write_share_data(offset, data) self._already_written.set(True, offset, end) - self.ss.add_latency("write", time.time() - start) + self.ss.add_latency("write", self._clock.seconds() - start) self.ss.count("write") def remote_close(self): precondition(not self.closed) - start = time.time() + start = self._clock.seconds() fileutil.make_dirs(os.path.dirname(self.finalhome)) fileutil.rename(self.incominghome, self.finalhome) @@ -306,7 +307,7 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 filelen = os.stat(self.finalhome)[stat.ST_SIZE] self.ss.bucket_writer_closed(self, filelen) - self.ss.add_latency("close", time.time() - start) + self.ss.add_latency("close", self._clock.seconds() - start) self.ss.count("close") def disconnected(self): diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 499d47276..080c1aea1 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -347,7 +347,8 @@ class StorageServer(service.MultiService, Referenceable): elif (not limited) or (remaining_space >= max_space_per_bucket): # ok! we need to create the new share file. bw = BucketWriter(self, incominghome, finalhome, - max_space_per_bucket, lease_info) + max_space_per_bucket, lease_info, + clock=self._clock) if self.no_storage: bw.throw_out_all_data = True bucketwriters[shnum] = bw diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index e143bec63..36c776fba 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -128,7 +128,7 @@ class Bucket(unittest.TestCase): def test_create(self): incoming, final = self.make_workdir("test_create") - bw = BucketWriter(self, incoming, final, 200, self.make_lease()) + bw = BucketWriter(self, incoming, final, 200, self.make_lease(), Clock()) bw.remote_write(0, b"a"*25) bw.remote_write(25, b"b"*25) bw.remote_write(50, b"c"*25) @@ -137,7 +137,7 @@ class Bucket(unittest.TestCase): def test_readwrite(self): incoming, final = self.make_workdir("test_readwrite") - bw = BucketWriter(self, incoming, final, 200, self.make_lease()) + bw = BucketWriter(self, incoming, final, 200, self.make_lease(), Clock()) bw.remote_write(0, b"a"*25) bw.remote_write(25, b"b"*25) bw.remote_write(50, b"c"*7) # last block may be short @@ -155,7 +155,7 @@ class Bucket(unittest.TestCase): incoming, final = self.make_workdir( "test_write_past_size_errors-{}".format(i) ) - bw = BucketWriter(self, incoming, final, 200, self.make_lease()) + bw = BucketWriter(self, incoming, final, 200, self.make_lease(), Clock()) with self.assertRaises(DataTooLargeError): bw.remote_write(offset, b"a" * length) @@ -174,7 +174,7 @@ class Bucket(unittest.TestCase): expected_data = b"".join(bchr(i) for i in range(100)) incoming, final = self.make_workdir("overlapping_writes_{}".format(uuid4())) bw = BucketWriter( - self, incoming, final, length, self.make_lease(), + self, incoming, final, length, self.make_lease(), Clock() ) # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes. bw.remote_write(10, expected_data[10:20]) @@ -212,7 +212,7 @@ class Bucket(unittest.TestCase): length = 100 incoming, final = self.make_workdir("overlapping_writes_{}".format(uuid4())) bw = BucketWriter( - self, incoming, final, length, self.make_lease(), + self, incoming, final, length, self.make_lease(), Clock() ) # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes. bw.remote_write(10, b"1" * 10) @@ -312,7 +312,7 @@ class BucketProxy(unittest.TestCase): final = os.path.join(basedir, "bucket") fileutil.make_dirs(basedir) fileutil.make_dirs(os.path.join(basedir, "tmp")) - bw = BucketWriter(self, incoming, final, size, self.make_lease()) + bw = BucketWriter(self, incoming, final, size, self.make_lease(), Clock()) rb = RemoteBucket(bw) return bw, rb, final From 5e341ad43a85444ffc3c12c685463171a53838ff Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 17 Nov 2021 11:29:34 -0500 Subject: [PATCH 61/89] New tests to write. --- src/allmydata/test/test_storage.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 36c776fba..93779bb29 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -285,6 +285,22 @@ class Bucket(unittest.TestCase): result_of_read = br.remote_read(0, len(share_data)+1) self.failUnlessEqual(result_of_read, share_data) + def test_bucket_expires_if_no_writes_for_30_minutes(self): + pass + + def test_bucket_writes_delay_timeout(self): + pass + + def test_bucket_finishing_writiing_cancels_timeout(self): + pass + + def test_bucket_closing_cancels_timeout(self): + pass + + def test_bucket_aborting_cancels_timeout(self): + pass + + class RemoteBucket(object): def __init__(self, target): @@ -559,7 +575,6 @@ class Server(unittest.TestCase): writer.remote_abort() self.failUnlessEqual(ss.allocated_size(), 0) - def test_allocate(self): ss = self.create("test_allocate") From 8c8e377466bcf2659029f7d59636d5039e12abf7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 14:35:04 -0500 Subject: [PATCH 62/89] Implement timeout and corresponding tests. --- src/allmydata/storage/immutable.py | 28 +++++++++++++-- src/allmydata/test/test_storage.py | 58 ++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 7cfb7a1bf..8a7519b7b 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -246,11 +246,17 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 self._sharefile.add_lease(lease_info) self._already_written = RangeMap() self._clock = clock + self._timeout = clock.callLater(30 * 60, self._abort_due_to_timeout) def allocated_size(self): return self._max_size def remote_write(self, offset, data): + self.write(offset, data) + + def write(self, offset, data): + # Delay the timeout, since we received data: + self._timeout.reset(30 * 60) start = self._clock.seconds() precondition(not self.closed) if self.throw_out_all_data: @@ -273,7 +279,11 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 self.ss.count("write") def remote_close(self): + self.close() + + def close(self): precondition(not self.closed) + self._timeout.cancel() start = self._clock.seconds() fileutil.make_dirs(os.path.dirname(self.finalhome)) @@ -312,15 +322,23 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 def disconnected(self): if not self.closed: - self._abort() + self.abort() + + def _abort_due_to_timeout(self): + """ + Called if we run out of time. + """ + log.msg("storage: aborting sharefile %s due to timeout" % self.incominghome, + facility="tahoe.storage", level=log.UNUSUAL) + self.abort() def remote_abort(self): log.msg("storage: aborting sharefile %s" % self.incominghome, facility="tahoe.storage", level=log.UNUSUAL) - self._abort() + self.abort() self.ss.count("abort") - def _abort(self): + def abort(self): if self.closed: return @@ -338,6 +356,10 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 self.closed = True self.ss.bucket_writer_closed(self, 0) + # Cancel timeout if it wasn't already cancelled. + if self._timeout.active(): + self._timeout.cancel() + @implementer(RIBucketReader) class BucketReader(Referenceable): # type: ignore # warner/foolscap#78 diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 93779bb29..18dca9856 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -285,20 +285,64 @@ class Bucket(unittest.TestCase): result_of_read = br.remote_read(0, len(share_data)+1) self.failUnlessEqual(result_of_read, share_data) + def _assert_timeout_only_after_30_minutes(self, clock, bw): + """ + The ``BucketWriter`` times out and is closed after 30 minutes, but not + sooner. + """ + self.assertFalse(bw.closed) + # 29 minutes pass. Everything is fine. + for i in range(29): + clock.advance(60) + self.assertFalse(bw.closed, "Bucket closed after only %d minutes" % (i + 1,)) + # After the 30th minute, the bucket is closed due to lack of writes. + clock.advance(60) + self.assertTrue(bw.closed) + def test_bucket_expires_if_no_writes_for_30_minutes(self): - pass + """ + If a ``BucketWriter`` receives no writes for 30 minutes, it is removed. + """ + incoming, final = self.make_workdir("test_bucket_expires") + clock = Clock() + bw = BucketWriter(self, incoming, final, 200, self.make_lease(), clock) + self._assert_timeout_only_after_30_minutes(clock, bw) def test_bucket_writes_delay_timeout(self): - pass - - def test_bucket_finishing_writiing_cancels_timeout(self): - pass + """ + So long as the ``BucketWriter`` receives writes, the the removal + timeout is put off. + """ + incoming, final = self.make_workdir("test_bucket_writes_delay_timeout") + clock = Clock() + bw = BucketWriter(self, incoming, final, 200, self.make_lease(), clock) + # 20 minutes pass, getting close to the timeout... + clock.advance(29 * 60) + # .. but we receive a write! So that should delay the timeout. + bw.write(0, b"hello") + self._assert_timeout_only_after_30_minutes(clock, bw) def test_bucket_closing_cancels_timeout(self): - pass + """ + Closing cancels the ``BucketWriter`` timeout. + """ + incoming, final = self.make_workdir("test_bucket_close_timeout") + clock = Clock() + bw = BucketWriter(self, incoming, final, 10, self.make_lease(), clock) + self.assertTrue(clock.getDelayedCalls()) + bw.close() + self.assertFalse(clock.getDelayedCalls()) def test_bucket_aborting_cancels_timeout(self): - pass + """ + Closing cancels the ``BucketWriter`` timeout. + """ + incoming, final = self.make_workdir("test_bucket_abort_timeout") + clock = Clock() + bw = BucketWriter(self, incoming, final, 10, self.make_lease(), clock) + self.assertTrue(clock.getDelayedCalls()) + bw.abort() + self.assertFalse(clock.getDelayedCalls()) class RemoteBucket(object): From 1827faf36b60751811302d1d20ba87348b7e32c4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 14:45:44 -0500 Subject: [PATCH 63/89] Fix issue with leaked-past-end-of-test DelayedCalls. --- src/allmydata/test/test_storage.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 18dca9856..977ed768f 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -498,7 +498,9 @@ class Server(unittest.TestCase): basedir = os.path.join("storage", "Server", name) return basedir - def create(self, name, reserved_space=0, klass=StorageServer, clock=reactor): + def create(self, name, reserved_space=0, klass=StorageServer, clock=None): + if clock is None: + clock = Clock() workdir = self.workdir(name) ss = klass(workdir, b"\x00" * 20, reserved_space=reserved_space, stats_provider=FakeStatsProvider(), @@ -1091,8 +1093,10 @@ class MutableServer(unittest.TestCase): basedir = os.path.join("storage", "MutableServer", name) return basedir - def create(self, name, clock=reactor): + def create(self, name, clock=None): workdir = self.workdir(name) + if clock is None: + clock = Clock() ss = StorageServer(workdir, b"\x00" * 20, clock=clock) ss.setServiceParent(self.sparent) From 5d915afe1c00d5832fec59d9a1599482d66a9e85 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 15:42:54 -0500 Subject: [PATCH 64/89] Clean up BucketWriters on shutdown (also preventing DelayedCalls leaks in tests). --- src/allmydata/storage/server.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 080c1aea1..6e3d6f683 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -136,6 +136,12 @@ class StorageServer(service.MultiService, Referenceable): # Canaries and disconnect markers for BucketWriters created via Foolscap: self._bucket_writer_disconnect_markers = {} # type: Dict[BucketWriter,(IRemoteReference, object)] + def stopService(self): + # Cancel any in-progress uploads: + for bw in list(self._bucket_writers.values()): + bw.disconnected() + return service.MultiService.stopService(self) + def __repr__(self): return "" % (idlib.shortnodeid_b2a(self.my_nodeid),) From bd645edd9e68efef5c21b5864957b6e2858acc12 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 15:44:51 -0500 Subject: [PATCH 65/89] Fix flake. --- src/allmydata/test/test_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 977ed768f..92de63f0d 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -23,7 +23,7 @@ from uuid import uuid4 from twisted.trial import unittest -from twisted.internet import defer, reactor +from twisted.internet import defer from twisted.internet.task import Clock from hypothesis import given, strategies From e2636466b584fff59dcd513866e254443417e771 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 15:47:25 -0500 Subject: [PATCH 66/89] Fix a flake. --- src/allmydata/storage/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 6e3d6f683..7dc277e39 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -14,7 +14,7 @@ if PY2: else: from typing import Dict -import os, re, time +import os, re import six from foolscap.api import Referenceable From 4c111773876bb97192dc2604e6a32e14f7898c16 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 18 Nov 2021 15:58:55 -0500 Subject: [PATCH 67/89] Fix a problem with typechecking. Using remote_write() isn't quite right given move to HTTP, but can fight that battle another day. --- src/allmydata/storage/immutable.py | 3 --- src/allmydata/test/test_storage.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index 8a7519b7b..08b83cd87 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -252,9 +252,6 @@ class BucketWriter(Referenceable): # type: ignore # warner/foolscap#78 return self._max_size def remote_write(self, offset, data): - self.write(offset, data) - - def write(self, offset, data): # Delay the timeout, since we received data: self._timeout.reset(30 * 60) start = self._clock.seconds() diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 92de63f0d..7fbe8f87b 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -319,7 +319,7 @@ class Bucket(unittest.TestCase): # 20 minutes pass, getting close to the timeout... clock.advance(29 * 60) # .. but we receive a write! So that should delay the timeout. - bw.write(0, b"hello") + bw.remote_write(0, b"hello") self._assert_timeout_only_after_30_minutes(clock, bw) def test_bucket_closing_cancels_timeout(self): From c341a86abdd4aa2b4244d0adeff53d5893be9a03 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 10:01:03 -0500 Subject: [PATCH 68/89] Correct the comment. --- src/allmydata/test/test_storage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 7fbe8f87b..bc87e168d 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -316,9 +316,10 @@ class Bucket(unittest.TestCase): incoming, final = self.make_workdir("test_bucket_writes_delay_timeout") clock = Clock() bw = BucketWriter(self, incoming, final, 200, self.make_lease(), clock) - # 20 minutes pass, getting close to the timeout... + # 29 minutes pass, getting close to the timeout... clock.advance(29 * 60) - # .. but we receive a write! So that should delay the timeout. + # .. but we receive a write! So that should delay the timeout again to + # another 30 minutes. bw.remote_write(0, b"hello") self._assert_timeout_only_after_30_minutes(clock, bw) From 6c514dfda57bfc2ede45719db24acecfbfee3ed1 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 10:33:45 -0500 Subject: [PATCH 69/89] Add klein. --- nix/klein.nix | 18 ++++++++++++++++++ nix/overlays.nix | 3 +++ 2 files changed, 21 insertions(+) create mode 100644 nix/klein.nix diff --git a/nix/klein.nix b/nix/klein.nix new file mode 100644 index 000000000..aa109e3d1 --- /dev/null +++ b/nix/klein.nix @@ -0,0 +1,18 @@ +{ lib, buildPythonPackage, fetchPypi }: +buildPythonPackage rec { + pname = "klein"; + version = "21.8.0"; + + src = fetchPypi { + sha256 = "09i1x5ppan3kqsgclbz8xdnlvzvp3amijbmdzv0kik8p5l5zswxa"; + inherit pname version; + }; + + doCheck = false; + + meta = with lib; { + homepage = https://github.com/twisted/klein; + description = "Nicer web server for Twisted"; + license = licenses.mit; + }; +} diff --git a/nix/overlays.nix b/nix/overlays.nix index fbd0ce3bb..011d8dd6b 100644 --- a/nix/overlays.nix +++ b/nix/overlays.nix @@ -28,6 +28,9 @@ self: super: { packageOverrides = python-self: python-super: { # collections-extended is not part of nixpkgs at this time. collections-extended = python-super.pythonPackages.callPackage ./collections-extended.nix { }; + + # klein is not in nixpkgs 21.05, at least: + klein = python-super.pythonPackages.callPackage ./klein.nix { }; }; }; } From c921b153f4990e98a32dde1286d3a9c11d5fd2e4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 10:39:15 -0500 Subject: [PATCH 70/89] A better name for the API. --- src/allmydata/storage/http_server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 3baa336fa..327892ecd 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -47,7 +47,7 @@ def _authorization_decorator(f): return route -def _route(app, *route_args, **route_kwargs): +def _authorized_route(app, *route_args, **route_kwargs): """ Like Klein's @route, but with additional support for checking the ``Authorization`` header as well as ``X-Tahoe-Authorization`` headers. The @@ -89,6 +89,6 @@ class HTTPServer(object): # TODO if data is big, maybe want to use a temporary file eventually... return dumps(data) - @_route(_app, "/v1/version", methods=["GET"]) + @_authorized_route(_app, "/v1/version", methods=["GET"]) def version(self, request, authorization): return self._cbor(request, self._storage_server.remote_get_version()) From a593095dc935b6719e266e0bf3a996b39047d9c0 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 10:39:53 -0500 Subject: [PATCH 71/89] Explain why it's a conditional import. --- src/allmydata/storage/http_client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index e1743343d..f8a7590aa 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -14,6 +14,8 @@ if PY2: from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 # fmt: on else: + # typing module not available in Python 2, and we only do type checking in + # Python 3 anyway. from typing import Union from treq.testing import StubTreq From 8abc1ad8f4e43a244d0bcead201a133f3cf8b0c1 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 10:44:45 -0500 Subject: [PATCH 72/89] cbor2 for Python 2 on Nix. --- nix/cbor2.nix | 18 ++++++++++++++++++ nix/overlays.nix | 3 +++ 2 files changed, 21 insertions(+) create mode 100644 nix/cbor2.nix diff --git a/nix/cbor2.nix b/nix/cbor2.nix new file mode 100644 index 000000000..02c810e1e --- /dev/null +++ b/nix/cbor2.nix @@ -0,0 +1,18 @@ +{ lib, buildPythonPackage, fetchPypi }: +buildPythonPackage rec { + pname = "cbor2"; + version = "5.2.0"; + + src = fetchPypi { + sha256 = "1mmmncfbsx7cbdalcrsagp9hx7wqfawaz9361gjkmsk3lp6chd5w"; + inherit pname version; + }; + + doCheck = false; + + meta = with lib; { + homepage = https://github.com/agronholm/cbor2; + description = "CBOR encoder/decoder"; + license = licenses.mit; + }; +} diff --git a/nix/overlays.nix b/nix/overlays.nix index 011d8dd6b..5cfab200c 100644 --- a/nix/overlays.nix +++ b/nix/overlays.nix @@ -21,6 +21,9 @@ self: super: { # collections-extended is not part of nixpkgs at this time. collections-extended = python-super.pythonPackages.callPackage ./collections-extended.nix { }; + + # cbor2 is not part of nixpkgs at this time. + cbor2 = python-super.pythonPackages.callPackage ./cbor2.nix { }; }; }; From 30511ea8502dc04848f9ed3715b5517c51444c96 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 Nov 2021 11:39:51 -0500 Subject: [PATCH 73/89] Add more build inputs. --- nix/tahoe-lafs.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/nix/tahoe-lafs.nix b/nix/tahoe-lafs.nix index df12f21d4..59864d36d 100644 --- a/nix/tahoe-lafs.nix +++ b/nix/tahoe-lafs.nix @@ -98,6 +98,7 @@ EOF service-identity pyyaml magic-wormhole eliot autobahn cryptography netifaces setuptools future pyutil distro configparser collections-extended + klein cbor2 treq ]; checkInputs = with python.pkgs; [ From 5fef83078d863843ef1f5f8a35990bfc3fcdb338 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:08:11 -0500 Subject: [PATCH 74/89] news fragment --- newsfragments/3847.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3847.minor diff --git a/newsfragments/3847.minor b/newsfragments/3847.minor new file mode 100644 index 000000000..e69de29bb From 66a0c6f3f43ecd018cdbb571ebd6eab740b6cca7 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:43:06 -0500 Subject: [PATCH 75/89] add a direct test for the non-utf-8 bytestring behavior --- src/allmydata/test/test_eliotutil.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/allmydata/test/test_eliotutil.py b/src/allmydata/test/test_eliotutil.py index 3f915ecd2..00110530c 100644 --- a/src/allmydata/test/test_eliotutil.py +++ b/src/allmydata/test/test_eliotutil.py @@ -78,6 +78,9 @@ from .common import ( class EliotLoggedTestTests(AsyncTestCase): + """ + Tests for the automatic log-related provided by ``EliotLoggedRunTest``. + """ def test_returns_none(self): Message.log(hello="world") @@ -95,6 +98,12 @@ class EliotLoggedTestTests(AsyncTestCase): # We didn't start an action. We're not finishing an action. return d.result + def test_logs_non_utf_8_byte(self): + """ + If an Eliot message is emitted that contains a non-UTF-8 byte string then + the test nevertheless passes. + """ + Message.log(hello=b"\xFF") class ParseDestinationDescriptionTests(SyncTestCase): From f40da7dc27d089e3bdfdca48e51aec25aea282c0 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:23:59 -0500 Subject: [PATCH 76/89] Put the choice of JSON encoder for Eliot into its own module and use it in a few places --- src/allmydata/test/__init__.py | 4 ++-- src/allmydata/test/test_eliotutil.py | 4 ++-- src/allmydata/util/_eliot_updates.py | 28 ++++++++++++++++++++++++++++ src/allmydata/util/eliotutil.py | 7 ++++--- 4 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 src/allmydata/util/_eliot_updates.py diff --git a/src/allmydata/test/__init__.py b/src/allmydata/test/__init__.py index 893aa15ce..ad245ca77 100644 --- a/src/allmydata/test/__init__.py +++ b/src/allmydata/test/__init__.py @@ -125,5 +125,5 @@ if sys.platform == "win32": initialize() from eliot import to_file -from allmydata.util.jsonbytes import AnyBytesJSONEncoder -to_file(open("eliot.log", "wb"), encoder=AnyBytesJSONEncoder) +from allmydata.util.eliotutil import eliot_json_encoder +to_file(open("eliot.log", "wb"), encoder=eliot_json_encoder) diff --git a/src/allmydata/test/test_eliotutil.py b/src/allmydata/test/test_eliotutil.py index 00110530c..0be02b277 100644 --- a/src/allmydata/test/test_eliotutil.py +++ b/src/allmydata/test/test_eliotutil.py @@ -65,11 +65,11 @@ from twisted.internet.task import deferLater from twisted.internet import reactor from ..util.eliotutil import ( + eliot_json_encoder, log_call_deferred, _parse_destination_description, _EliotLogging, ) -from ..util.jsonbytes import AnyBytesJSONEncoder from .common import ( SyncTestCase, @@ -118,7 +118,7 @@ class ParseDestinationDescriptionTests(SyncTestCase): reactor = object() self.assertThat( _parse_destination_description("file:-")(reactor), - Equals(FileDestination(stdout, encoder=AnyBytesJSONEncoder)), + Equals(FileDestination(stdout, encoder=eliot_json_encoder)), ) diff --git a/src/allmydata/util/_eliot_updates.py b/src/allmydata/util/_eliot_updates.py new file mode 100644 index 000000000..4300f2be8 --- /dev/null +++ b/src/allmydata/util/_eliot_updates.py @@ -0,0 +1,28 @@ +""" +Bring in some Eliot updates from newer versions of Eliot than we can +depend on in Python 2. + +Every API in this module (except ``eliot_json_encoder``) should be obsolete as +soon as we depend on Eliot 1.14 or newer. + +Ported to Python 3. +""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function +from __future__ import unicode_literals + +from future.utils import PY2 +if PY2: + from builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 + +from .jsonbytes import AnyBytesJSONEncoder + +# There are currently a number of log messages that include non-UTF-8 bytes. +# Allow these, at least for now. Later when the whole test suite has been +# converted to our SyncTestCase or AsyncTestCase it will be easier to turn +# this off and then attribute log failures to specific codepaths so they can +# be fixed (and then not regressed later) because those instances will result +# in test failures instead of only garbage being written to the eliot log. +eliot_json_encoder = AnyBytesJSONEncoder diff --git a/src/allmydata/util/eliotutil.py b/src/allmydata/util/eliotutil.py index 4e48fbb9f..ff858531d 100644 --- a/src/allmydata/util/eliotutil.py +++ b/src/allmydata/util/eliotutil.py @@ -87,8 +87,9 @@ from twisted.internet.defer import ( ) from twisted.application.service import Service -from .jsonbytes import AnyBytesJSONEncoder - +from ._eliot_updates import ( + eliot_json_encoder, +) def validateInstanceOf(t): """ @@ -306,7 +307,7 @@ class _DestinationParser(object): rotateLength=rotate_length, maxRotatedFiles=max_rotated_files, ) - return lambda reactor: FileDestination(get_file(), AnyBytesJSONEncoder) + return lambda reactor: FileDestination(get_file(), eliot_json_encoder) _parse_destination_description = _DestinationParser().parse From 3eb1a5e7cb6bc227feda6f254b43e35b1807446d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:25:03 -0500 Subject: [PATCH 77/89] Add a MemoryLogger that prefers our encoder and use it instead of Eliot's --- src/allmydata/test/eliotutil.py | 16 ++----- src/allmydata/util/_eliot_updates.py | 62 +++++++++++++++++++++++++++- src/allmydata/util/eliotutil.py | 2 + 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/src/allmydata/test/eliotutil.py b/src/allmydata/test/eliotutil.py index 1685744fd..dd21f1e9d 100644 --- a/src/allmydata/test/eliotutil.py +++ b/src/allmydata/test/eliotutil.py @@ -42,7 +42,6 @@ from zope.interface import ( from eliot import ( ActionType, Field, - MemoryLogger, ILogger, ) from eliot.testing import ( @@ -54,8 +53,9 @@ from twisted.python.monkey import ( MonkeyPatcher, ) -from ..util.jsonbytes import AnyBytesJSONEncoder - +from ..util.eliotutil import ( + MemoryLogger, +) _NAME = Field.for_types( u"name", @@ -71,14 +71,6 @@ RUN_TEST = ActionType( ) -# On Python 3, we want to use our custom JSON encoder when validating messages -# can be encoded to JSON: -if PY2: - _memory_logger = MemoryLogger -else: - _memory_logger = lambda: MemoryLogger(encoder=AnyBytesJSONEncoder) - - @attr.s class EliotLoggedRunTest(object): """ @@ -170,7 +162,7 @@ def with_logging( """ @wraps(test_method) def run_with_logging(*args, **kwargs): - validating_logger = _memory_logger() + validating_logger = MemoryLogger() original = swap_logger(None) try: swap_logger(_TwoLoggers(original, validating_logger)) diff --git a/src/allmydata/util/_eliot_updates.py b/src/allmydata/util/_eliot_updates.py index 4300f2be8..4ff0caf4d 100644 --- a/src/allmydata/util/_eliot_updates.py +++ b/src/allmydata/util/_eliot_updates.py @@ -1,6 +1,7 @@ """ Bring in some Eliot updates from newer versions of Eliot than we can -depend on in Python 2. +depend on in Python 2. The implementations are copied from Eliot 1.14 and +only changed enough to add Python 2 compatibility. Every API in this module (except ``eliot_json_encoder``) should be obsolete as soon as we depend on Eliot 1.14 or newer. @@ -17,6 +18,13 @@ from future.utils import PY2 if PY2: from builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 +import json as pyjson +from functools import partial + +from eliot import ( + MemoryLogger as _MemoryLogger, +) + from .jsonbytes import AnyBytesJSONEncoder # There are currently a number of log messages that include non-UTF-8 bytes. @@ -26,3 +34,55 @@ from .jsonbytes import AnyBytesJSONEncoder # be fixed (and then not regressed later) because those instances will result # in test failures instead of only garbage being written to the eliot log. eliot_json_encoder = AnyBytesJSONEncoder + +class _CustomEncoderMemoryLogger(_MemoryLogger): + """ + Override message validation from the Eliot-supplied ``MemoryLogger`` to + use our chosen JSON encoder. + + This is only necessary on Python 2 where we use an old version of Eliot + that does not parameterize the encoder. + """ + def __init__(self, encoder=eliot_json_encoder): + """ + @param encoder: A JSONEncoder subclass to use when encoding JSON. + """ + self._encoder = encoder + super(_CustomEncoderMemoryLogger, self).__init__() + + def _validate_message(self, dictionary, serializer): + """Validate an individual message. + + As a side-effect, the message is replaced with its serialized contents. + + @param dictionary: A message C{dict} to be validated. Might be mutated + by the serializer! + + @param serializer: C{None} or a serializer. + + @raises TypeError: If a field name is not unicode, or the dictionary + fails to serialize to JSON. + + @raises eliot.ValidationError: If serializer was given and validation + failed. + """ + if serializer is not None: + serializer.validate(dictionary) + for key in dictionary: + if not isinstance(key, str): + if isinstance(key, bytes): + key.decode("utf-8") + else: + raise TypeError(dictionary, "%r is not unicode" % (key,)) + if serializer is not None: + serializer.serialize(dictionary) + + try: + pyjson.dumps(dictionary, cls=self._encoder) + except Exception as e: + raise TypeError("Message %s doesn't encode to JSON: %s" % (dictionary, e)) + +if PY2: + MemoryLogger = partial(_CustomEncoderMemoryLogger, encoder=eliot_json_encoder) +else: + MemoryLogger = partial(_MemoryLogger, encoder=eliot_json_encoder) diff --git a/src/allmydata/util/eliotutil.py b/src/allmydata/util/eliotutil.py index ff858531d..5067876c5 100644 --- a/src/allmydata/util/eliotutil.py +++ b/src/allmydata/util/eliotutil.py @@ -16,6 +16,7 @@ from __future__ import ( ) __all__ = [ + "MemoryLogger", "inline_callbacks", "eliot_logging_service", "opt_eliot_destination", @@ -88,6 +89,7 @@ from twisted.internet.defer import ( from twisted.application.service import Service from ._eliot_updates import ( + MemoryLogger, eliot_json_encoder, ) From 20e0626e424276c83cd1f2eb42fdddb7cf56072e Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:27:17 -0500 Subject: [PATCH 78/89] add capture_logging that parameterizes JSON encoder --- src/allmydata/util/_eliot_updates.py | 100 ++++++++++++++++++++++++++- src/allmydata/util/eliotutil.py | 13 +--- 2 files changed, 102 insertions(+), 11 deletions(-) diff --git a/src/allmydata/util/_eliot_updates.py b/src/allmydata/util/_eliot_updates.py index 4ff0caf4d..8e3beca45 100644 --- a/src/allmydata/util/_eliot_updates.py +++ b/src/allmydata/util/_eliot_updates.py @@ -19,12 +19,17 @@ if PY2: from builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 import json as pyjson -from functools import partial +from functools import wraps, partial from eliot import ( MemoryLogger as _MemoryLogger, ) +from eliot.testing import ( + check_for_errors, + swap_logger, +) + from .jsonbytes import AnyBytesJSONEncoder # There are currently a number of log messages that include non-UTF-8 bytes. @@ -86,3 +91,96 @@ if PY2: MemoryLogger = partial(_CustomEncoderMemoryLogger, encoder=eliot_json_encoder) else: MemoryLogger = partial(_MemoryLogger, encoder=eliot_json_encoder) + +def validateLogging( + assertion, *assertionArgs, **assertionKwargs +): + """ + Decorator factory for L{unittest.TestCase} methods to add logging + validation. + + 1. The decorated test method gets a C{logger} keyword argument, a + L{MemoryLogger}. + 2. All messages logged to this logger will be validated at the end of + the test. + 3. Any unflushed logged tracebacks will cause the test to fail. + + For example: + + from unittest import TestCase + from eliot.testing import assertContainsFields, validateLogging + + class MyTests(TestCase): + def assertFooLogging(self, logger): + assertContainsFields(self, logger.messages[0], {"key": 123}) + + + @param assertion: A callable that will be called with the + L{unittest.TestCase} instance, the logger and C{assertionArgs} and + C{assertionKwargs} once the actual test has run, allowing for extra + logging-related assertions on the effects of the test. Use L{None} if you + want the cleanup assertions registered but no custom assertions. + + @param assertionArgs: Additional positional arguments to pass to + C{assertion}. + + @param assertionKwargs: Additional keyword arguments to pass to + C{assertion}. + + @param encoder_: C{json.JSONEncoder} subclass to use when validating JSON. + """ + encoder_ = assertionKwargs.pop("encoder_", eliot_json_encoder) + def decorator(function): + @wraps(function) + def wrapper(self, *args, **kwargs): + skipped = False + + kwargs["logger"] = logger = MemoryLogger(encoder=encoder_) + self.addCleanup(check_for_errors, logger) + # TestCase runs cleanups in reverse order, and we want this to + # run *before* tracebacks are checked: + if assertion is not None: + self.addCleanup( + lambda: skipped + or assertion(self, logger, *assertionArgs, **assertionKwargs) + ) + try: + return function(self, *args, **kwargs) + except self.skipException: + skipped = True + raise + + return wrapper + + return decorator + +# PEP 8 variant: +validate_logging = validateLogging + +def capture_logging( + assertion, *assertionArgs, **assertionKwargs +): + """ + Capture and validate all logging that doesn't specify a L{Logger}. + + See L{validate_logging} for details on the rest of its behavior. + """ + encoder_ = assertionKwargs.pop("encoder_", eliot_json_encoder) + def decorator(function): + @validate_logging( + assertion, *assertionArgs, encoder_=encoder_, **assertionKwargs + ) + @wraps(function) + def wrapper(self, *args, **kwargs): + logger = kwargs["logger"] + previous_logger = swap_logger(logger) + + def cleanup(): + swap_logger(previous_logger) + + self.addCleanup(cleanup) + return function(self, *args, **kwargs) + + return wrapper + + return decorator diff --git a/src/allmydata/util/eliotutil.py b/src/allmydata/util/eliotutil.py index 5067876c5..789ef38ff 100644 --- a/src/allmydata/util/eliotutil.py +++ b/src/allmydata/util/eliotutil.py @@ -23,6 +23,7 @@ __all__ = [ "opt_help_eliot_destinations", "validateInstanceOf", "validateSetMembership", + "capture_logging", ] from future.utils import PY2 @@ -33,7 +34,7 @@ from six import ensure_text from sys import ( stdout, ) -from functools import wraps, partial +from functools import wraps from logging import ( INFO, Handler, @@ -67,8 +68,6 @@ from eliot.twisted import ( DeferredContext, inline_callbacks, ) -from eliot.testing import capture_logging as eliot_capture_logging - from twisted.python.usage import ( UsageError, ) @@ -91,6 +90,7 @@ from twisted.application.service import Service from ._eliot_updates import ( MemoryLogger, eliot_json_encoder, + capture_logging, ) def validateInstanceOf(t): @@ -330,10 +330,3 @@ def log_call_deferred(action_type): return DeferredContext(d).addActionFinish() return logged_f return decorate_log_call_deferred - -# On Python 3, encoding bytes to JSON doesn't work, so we have a custom JSON -# encoder we want to use when validating messages. -if PY2: - capture_logging = eliot_capture_logging -else: - capture_logging = partial(eliot_capture_logging, encoder_=AnyBytesJSONEncoder) From 7626a02bdb84013e4f45bad81c8a3f5ba4586401 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 29 Nov 2021 13:27:28 -0500 Subject: [PATCH 79/89] remove redundant assertion --- src/allmydata/test/test_util.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/allmydata/test/test_util.py b/src/allmydata/test/test_util.py index a03845ed6..9a0af1e06 100644 --- a/src/allmydata/test/test_util.py +++ b/src/allmydata/test/test_util.py @@ -553,11 +553,6 @@ class JSONBytes(unittest.TestCase): o, cls=jsonbytes.AnyBytesJSONEncoder)), expected, ) - self.assertEqual( - json.loads(jsonbytes.dumps(o, any_bytes=True)), - expected - ) - class FakeGetVersion(object): From b01478659ea9868164aa9f7b7368f295f2d47921 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:18:18 -0500 Subject: [PATCH 80/89] Apparently I generated wrong hashes. --- nix/cbor2.nix | 2 +- nix/klein.nix | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nix/cbor2.nix b/nix/cbor2.nix index 02c810e1e..1bd9920e6 100644 --- a/nix/cbor2.nix +++ b/nix/cbor2.nix @@ -4,7 +4,7 @@ buildPythonPackage rec { version = "5.2.0"; src = fetchPypi { - sha256 = "1mmmncfbsx7cbdalcrsagp9hx7wqfawaz9361gjkmsk3lp6chd5w"; + sha256 = "1gwlgjl70vlv35cgkcw3cg7b5qsmws36hs4mmh0l9msgagjs4fm3"; inherit pname version; }; diff --git a/nix/klein.nix b/nix/klein.nix index aa109e3d1..0bb025cf8 100644 --- a/nix/klein.nix +++ b/nix/klein.nix @@ -4,7 +4,7 @@ buildPythonPackage rec { version = "21.8.0"; src = fetchPypi { - sha256 = "09i1x5ppan3kqsgclbz8xdnlvzvp3amijbmdzv0kik8p5l5zswxa"; + sha256 = "1mpydmz90d0n9dwa7mr6pgj5v0kczfs05ykssrasdq368dssw7ch"; inherit pname version; }; From 1fc77504aeec738acac315d65b68b4a7e01db095 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:39:42 -0500 Subject: [PATCH 81/89] List dependencies. --- nix/klein.nix | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nix/klein.nix b/nix/klein.nix index 0bb025cf8..196f95e88 100644 --- a/nix/klein.nix +++ b/nix/klein.nix @@ -10,6 +10,8 @@ buildPythonPackage rec { doCheck = false; + propagatedBuildInputs = [ attrs hyperlink incremental Tubes Twisted typing_extensions Werkzeug zope.interface ]; + meta = with lib; { homepage = https://github.com/twisted/klein; description = "Nicer web server for Twisted"; From c65a13e63228fada255a94b99ca4e61a1e9e58dc Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:47:28 -0500 Subject: [PATCH 82/89] Rip out klein, maybe not necessary. --- nix/klein.nix | 20 -------------------- nix/overlays.nix | 3 --- 2 files changed, 23 deletions(-) delete mode 100644 nix/klein.nix diff --git a/nix/klein.nix b/nix/klein.nix deleted file mode 100644 index 196f95e88..000000000 --- a/nix/klein.nix +++ /dev/null @@ -1,20 +0,0 @@ -{ lib, buildPythonPackage, fetchPypi }: -buildPythonPackage rec { - pname = "klein"; - version = "21.8.0"; - - src = fetchPypi { - sha256 = "1mpydmz90d0n9dwa7mr6pgj5v0kczfs05ykssrasdq368dssw7ch"; - inherit pname version; - }; - - doCheck = false; - - propagatedBuildInputs = [ attrs hyperlink incremental Tubes Twisted typing_extensions Werkzeug zope.interface ]; - - meta = with lib; { - homepage = https://github.com/twisted/klein; - description = "Nicer web server for Twisted"; - license = licenses.mit; - }; -} diff --git a/nix/overlays.nix b/nix/overlays.nix index 5cfab200c..92f36e93e 100644 --- a/nix/overlays.nix +++ b/nix/overlays.nix @@ -31,9 +31,6 @@ self: super: { packageOverrides = python-self: python-super: { # collections-extended is not part of nixpkgs at this time. collections-extended = python-super.pythonPackages.callPackage ./collections-extended.nix { }; - - # klein is not in nixpkgs 21.05, at least: - klein = python-super.pythonPackages.callPackage ./klein.nix { }; }; }; } From 2f4d1079aa3b0621e4ad5991f810a5baf32c23db Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:51:36 -0500 Subject: [PATCH 83/89] Needs setuptools_scm --- nix/cbor2.nix | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nix/cbor2.nix b/nix/cbor2.nix index 1bd9920e6..4d9734a8b 100644 --- a/nix/cbor2.nix +++ b/nix/cbor2.nix @@ -1,4 +1,4 @@ -{ lib, buildPythonPackage, fetchPypi }: +{ lib, buildPythonPackage, fetchPypi , setuptools_scm }: buildPythonPackage rec { pname = "cbor2"; version = "5.2.0"; @@ -10,6 +10,8 @@ buildPythonPackage rec { doCheck = false; + nativeBuildInputs = [ setuptools_scm ]; + meta = with lib; { homepage = https://github.com/agronholm/cbor2; description = "CBOR encoder/decoder"; From 136bf95bdfcf0819285f7c4ed937f4de64a99125 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:58:02 -0500 Subject: [PATCH 84/89] Simpler way. --- nix/cbor2.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nix/cbor2.nix b/nix/cbor2.nix index 4d9734a8b..ace5e13c6 100644 --- a/nix/cbor2.nix +++ b/nix/cbor2.nix @@ -1,4 +1,4 @@ -{ lib, buildPythonPackage, fetchPypi , setuptools_scm }: +{ lib, buildPythonPackage, fetchPypi }: buildPythonPackage rec { pname = "cbor2"; version = "5.2.0"; @@ -10,7 +10,7 @@ buildPythonPackage rec { doCheck = false; - nativeBuildInputs = [ setuptools_scm ]; + buildInputs = [ setuptools_scm ]; meta = with lib; { homepage = https://github.com/agronholm/cbor2; From f2b52f368d63059ebe559109b4dbe8c4720bdd2f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 30 Nov 2021 13:58:22 -0500 Subject: [PATCH 85/89] Another way. --- nix/cbor2.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nix/cbor2.nix b/nix/cbor2.nix index ace5e13c6..0544b1eb1 100644 --- a/nix/cbor2.nix +++ b/nix/cbor2.nix @@ -10,7 +10,7 @@ buildPythonPackage rec { doCheck = false; - buildInputs = [ setuptools_scm ]; + propagatedBuildInputs = [ setuptools_scm ]; meta = with lib; { homepage = https://github.com/agronholm/cbor2; From d985d1062295e2f816214bcbedb6746838d7a67d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 1 Dec 2021 09:24:03 -0500 Subject: [PATCH 86/89] Update nix/cbor2.nix Co-authored-by: Jean-Paul Calderone --- nix/cbor2.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nix/cbor2.nix b/nix/cbor2.nix index 0544b1eb1..16ca8ff63 100644 --- a/nix/cbor2.nix +++ b/nix/cbor2.nix @@ -1,4 +1,4 @@ -{ lib, buildPythonPackage, fetchPypi }: +{ lib, buildPythonPackage, fetchPypi, setuptools_scm }: buildPythonPackage rec { pname = "cbor2"; version = "5.2.0"; From 18a5966f1d27791d3129690926791a623957472c Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 1 Dec 2021 09:38:56 -0500 Subject: [PATCH 87/89] Don't bother running HTTP server tests on Python 2, since it's going away any day now. --- src/allmydata/test/test_storage_http.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index 9ba8adf21..442e154a0 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -14,6 +14,8 @@ if PY2: from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 # fmt: on +from unittest import SkipTest + from twisted.trial.unittest import TestCase from twisted.internet.defer import inlineCallbacks @@ -31,6 +33,8 @@ class HTTPTests(TestCase): """ def setUp(self): + if PY2: + raise SkipTest("Not going to bother supporting Python 2") self.storage_server = StorageServer(self.mktemp(), b"\x00" * 20) # TODO what should the swissnum _actually_ be? self._http_server = HTTPServer(self.storage_server, b"abcd") From 90d1e90a14b0a3455e2e5ac86cde814a8a81b378 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 1 Dec 2021 15:05:29 -0500 Subject: [PATCH 88/89] rewrite the Eliot interaction tests to make expected behavior clearer and to have explicit assertions about that behavior --- src/allmydata/test/test_eliotutil.py | 113 ++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 21 deletions(-) diff --git a/src/allmydata/test/test_eliotutil.py b/src/allmydata/test/test_eliotutil.py index 0be02b277..cabe599b3 100644 --- a/src/allmydata/test/test_eliotutil.py +++ b/src/allmydata/test/test_eliotutil.py @@ -27,13 +27,12 @@ from fixtures import ( ) from testtools import ( TestCase, -) -from testtools import ( TestResult, ) from testtools.matchers import ( Is, IsInstance, + Not, MatchesStructure, Equals, HasLength, @@ -77,33 +76,105 @@ from .common import ( ) -class EliotLoggedTestTests(AsyncTestCase): +def passes(): """ - Tests for the automatic log-related provided by ``EliotLoggedRunTest``. + Create a matcher that matches a ``TestCase`` that runs without failures or + errors. """ - def test_returns_none(self): - Message.log(hello="world") + def run(case): + result = TestResult() + case.run(result) + return result.wasSuccessful() + return AfterPreprocessing(run, Equals(True)) - def test_returns_fired_deferred(self): - Message.log(hello="world") - return succeed(None) - def test_returns_unfired_deferred(self): - Message.log(hello="world") - # @eliot_logged_test automatically gives us an action context but it's - # still our responsibility to maintain it across stack-busting - # operations. - d = DeferredContext(deferLater(reactor, 0.0, lambda: None)) - d.addCallback(lambda ignored: Message.log(goodbye="world")) - # We didn't start an action. We're not finishing an action. - return d.result +class EliotLoggedTestTests(TestCase): + """ + Tests for the automatic log-related provided by ``AsyncTestCase``. + + This class uses ``testtools.TestCase`` because it is inconvenient to nest + ``AsyncTestCase`` inside ``AsyncTestCase`` (in particular, Eliot messages + emitted by the inner test case get observed by the outer test case and if + an inner case emits invalid messages they cause the outer test case to + fail). + """ + def test_fails(self): + """ + A test method of an ``AsyncTestCase`` subclass can fail. + """ + class UnderTest(AsyncTestCase): + def test_it(self): + self.fail("make sure it can fail") + + self.assertThat(UnderTest("test_it"), Not(passes())) + + def test_unserializable_fails(self): + """ + A test method of an ``AsyncTestCase`` subclass that logs an unserializable + value with Eliot fails. + """ + class world(object): + """ + an unserializable object + """ + + class UnderTest(AsyncTestCase): + def test_it(self): + Message.log(hello=world) + + self.assertThat(UnderTest("test_it"), Not(passes())) def test_logs_non_utf_8_byte(self): """ - If an Eliot message is emitted that contains a non-UTF-8 byte string then - the test nevertheless passes. + A test method of an ``AsyncTestCase`` subclass can log a message that + contains a non-UTF-8 byte string and return ``None`` and pass. """ - Message.log(hello=b"\xFF") + class UnderTest(AsyncTestCase): + def test_it(self): + Message.log(hello=b"\xFF") + + self.assertThat(UnderTest("test_it"), passes()) + + def test_returns_none(self): + """ + A test method of an ``AsyncTestCase`` subclass can log a message and + return ``None`` and pass. + """ + class UnderTest(AsyncTestCase): + def test_it(self): + Message.log(hello="world") + + self.assertThat(UnderTest("test_it"), passes()) + + def test_returns_fired_deferred(self): + """ + A test method of an ``AsyncTestCase`` subclass can log a message and + return an already-fired ``Deferred`` and pass. + """ + class UnderTest(AsyncTestCase): + def test_it(self): + Message.log(hello="world") + return succeed(None) + + self.assertThat(UnderTest("test_it"), passes()) + + def test_returns_unfired_deferred(self): + """ + A test method of an ``AsyncTestCase`` subclass can log a message and + return an unfired ``Deferred`` and pass when the ``Deferred`` fires. + """ + class UnderTest(AsyncTestCase): + def test_it(self): + Message.log(hello="world") + # @eliot_logged_test automatically gives us an action context + # but it's still our responsibility to maintain it across + # stack-busting operations. + d = DeferredContext(deferLater(reactor, 0.0, lambda: None)) + d.addCallback(lambda ignored: Message.log(goodbye="world")) + # We didn't start an action. We're not finishing an action. + return d.result + + self.assertThat(UnderTest("test_it"), passes()) class ParseDestinationDescriptionTests(SyncTestCase): From eee1f0975d5bd32acbb5d1c481623235558ae47c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 1 Dec 2021 15:16:16 -0500 Subject: [PATCH 89/89] note about how to clean this up later --- src/allmydata/util/_eliot_updates.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/allmydata/util/_eliot_updates.py b/src/allmydata/util/_eliot_updates.py index 8e3beca45..81db566a4 100644 --- a/src/allmydata/util/_eliot_updates.py +++ b/src/allmydata/util/_eliot_updates.py @@ -6,6 +6,15 @@ only changed enough to add Python 2 compatibility. Every API in this module (except ``eliot_json_encoder``) should be obsolete as soon as we depend on Eliot 1.14 or newer. +When that happens: + +* replace ``capture_logging`` + with ``partial(eliot.testing.capture_logging, encoder_=eliot_json_encoder)`` +* replace ``validateLogging`` + with ``partial(eliot.testing.validateLogging, encoder_=eliot_json_encoder)`` +* replace ``MemoryLogger`` + with ``partial(eliot.MemoryLogger, encoder=eliot_json_encoder)`` + Ported to Python 3. """