From 70fb5d563abfcd809ce627b5ed35c0b09d55d684 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 28 Oct 2021 09:48:26 -0400 Subject: [PATCH] Get rid of the public expiration_time attribute LeaseInfo now has a getter and a setter for this attribute. LeaseInfo is now also immutable by way of `attrs`. LeaseInfo is now also comparable by way of `attrs`. --- src/allmydata/scripts/debug.py | 8 +-- src/allmydata/storage/immutable.py | 6 +-- src/allmydata/storage/lease.py | 72 ++++++++++++++++++++------ src/allmydata/storage/mutable.py | 7 ++- src/allmydata/test/test_storage.py | 23 +++----- src/allmydata/test/test_storage_web.py | 2 +- 6 files changed, 73 insertions(+), 45 deletions(-) diff --git a/src/allmydata/scripts/debug.py b/src/allmydata/scripts/debug.py index 2d6ba4602..4d3f4cb21 100644 --- a/src/allmydata/scripts/debug.py +++ b/src/allmydata/scripts/debug.py @@ -170,7 +170,7 @@ def dump_immutable_lease_info(f, out): leases = list(f.get_leases()) if leases: for i,lease in enumerate(leases): - when = format_expiration_time(lease.expiration_time) + when = format_expiration_time(lease.get_expiration_time()) print(" Lease #%d: owner=%d, expire in %s" \ % (i, lease.owner_num, when), file=out) else: @@ -223,7 +223,7 @@ def dump_mutable_share(options): print(file=out) print(" Lease #%d:" % leasenum, file=out) print(" ownerid: %d" % lease.owner_num, file=out) - when = format_expiration_time(lease.expiration_time) + when = format_expiration_time(lease.get_expiration_time()) print(" expires in %s" % when, file=out) print(" renew_secret: %s" % str(base32.b2a(lease.renew_secret), "utf-8"), file=out) print(" cancel_secret: %s" % str(base32.b2a(lease.cancel_secret), "utf-8"), file=out) @@ -730,7 +730,7 @@ def describe_share(abs_sharefile, si_s, shnum_s, now, out): m = MutableShareFile(abs_sharefile) WE, nodeid = m._read_write_enabler_and_nodeid(f) data_length = m._read_data_length(f) - expiration_time = min( [lease.expiration_time + expiration_time = min( [lease.get_expiration_time() for (i,lease) in m._enumerate_leases(f)] ) expiration = max(0, expiration_time - now) @@ -811,7 +811,7 @@ def describe_share(abs_sharefile, si_s, shnum_s, now, out): sf = ShareFile(abs_sharefile) bp = ImmediateReadBucketProxy(sf) - expiration_time = min( [lease.expiration_time + expiration_time = min( [lease.get_expiration_time() for lease in sf.get_leases()] ) expiration = max(0, expiration_time - now) diff --git a/src/allmydata/storage/immutable.py b/src/allmydata/storage/immutable.py index b8b18f140..81470eed8 100644 --- a/src/allmydata/storage/immutable.py +++ b/src/allmydata/storage/immutable.py @@ -156,9 +156,9 @@ class ShareFile(object): for i,lease in enumerate(self.get_leases()): if timing_safe_compare(lease.renew_secret, renew_secret): # yup. See if we need to update the owner time. - if new_expire_time > lease.expiration_time: + if new_expire_time > lease.get_expiration_time(): # yes - lease.expiration_time = new_expire_time + lease = lease.renew(new_expire_time) with open(self.home, 'rb+') as f: self._write_lease_record(f, i, lease) return @@ -167,7 +167,7 @@ class ShareFile(object): def add_or_renew_lease(self, lease_info): try: self.renew_lease(lease_info.renew_secret, - lease_info.expiration_time) + lease_info.get_expiration_time()) except IndexError: self.add_lease(lease_info) diff --git a/src/allmydata/storage/lease.py b/src/allmydata/storage/lease.py index 187f32406..594d61cf5 100644 --- a/src/allmydata/storage/lease.py +++ b/src/allmydata/storage/lease.py @@ -13,24 +13,64 @@ if PY2: import struct, time +import attr + +@attr.s(frozen=True) class LeaseInfo(object): - def __init__(self, owner_num=None, renew_secret=None, cancel_secret=None, - expiration_time=None, nodeid=None): - self.owner_num = owner_num - self.renew_secret = renew_secret - self.cancel_secret = cancel_secret - self.expiration_time = expiration_time - if nodeid is not None: - assert isinstance(nodeid, bytes) - assert len(nodeid) == 20 - self.nodeid = nodeid + """ + Represent the details of one lease, a marker which is intended to inform + the storage server how long to store a particular share. + """ + owner_num = attr.ib(default=None) + + # Don't put secrets into the default string representation. This makes it + # slightly less likely the secrets will accidentally be leaked to + # someplace they're not meant to be. + renew_secret = attr.ib(default=None, repr=False) + cancel_secret = attr.ib(default=None, repr=False) + + _expiration_time = attr.ib(default=None) + + nodeid = attr.ib(default=None) + + @nodeid.validator + def _validate_nodeid(self, attribute, value): + if value is not None: + if not isinstance(value, bytes): + raise ValueError( + "nodeid value must be bytes, not {!r}".format(value), + ) + if len(value) != 20: + raise ValueError( + "nodeid value must be 20 bytes long, not {!r}".format(value), + ) + return None def get_expiration_time(self): - return self.expiration_time + # type: () -> float + """ + Retrieve a POSIX timestamp representing the time at which this lease is + set to expire. + """ + return self._expiration_time + + def renew(self, new_expire_time): + # type: (float) -> LeaseInfo + """ + Create a new lease the same as this one but with a new expiration time. + + :param new_expire_time: The new expiration time. + + :return: The new lease info. + """ + return attr.assoc( + self, + _expiration_time=new_expire_time, + ) def get_grant_renew_time_time(self): # hack, based upon fixed 31day expiration period - return self.expiration_time - 31*24*60*60 + return self._expiration_time - 31*24*60*60 def get_age(self): return time.time() - self.get_grant_renew_time_time() @@ -39,7 +79,7 @@ class LeaseInfo(object): (self.owner_num, self.renew_secret, self.cancel_secret, - self.expiration_time) = struct.unpack(">L32s32sL", data) + self._expiration_time) = struct.unpack(">L32s32sL", data) self.nodeid = None return self @@ -47,18 +87,18 @@ class LeaseInfo(object): return struct.pack(">L32s32sL", self.owner_num, self.renew_secret, self.cancel_secret, - int(self.expiration_time)) + int(self._expiration_time)) def to_mutable_data(self): return struct.pack(">LL32s32s20s", self.owner_num, - int(self.expiration_time), + int(self._expiration_time), self.renew_secret, self.cancel_secret, self.nodeid) def from_mutable_data(self, data): (self.owner_num, - self.expiration_time, + self._expiration_time, self.renew_secret, self.cancel_secret, self.nodeid) = struct.unpack(">LL32s32s20s", data) return self diff --git a/src/allmydata/storage/mutable.py b/src/allmydata/storage/mutable.py index 2ef0c3215..53a38fae9 100644 --- a/src/allmydata/storage/mutable.py +++ b/src/allmydata/storage/mutable.py @@ -304,9 +304,9 @@ class MutableShareFile(object): for (leasenum,lease) in self._enumerate_leases(f): if timing_safe_compare(lease.renew_secret, renew_secret): # yup. See if we need to update the owner time. - if new_expire_time > lease.expiration_time: + if new_expire_time > lease.get_expiration_time(): # yes - lease.expiration_time = new_expire_time + lease = lease.renew(new_expire_time) self._write_lease_record(f, leasenum, lease) return accepting_nodeids.add(lease.nodeid) @@ -324,7 +324,7 @@ class MutableShareFile(object): precondition(lease_info.owner_num != 0) # 0 means "no lease here" try: self.renew_lease(lease_info.renew_secret, - lease_info.expiration_time) + lease_info.get_expiration_time()) except IndexError: self.add_lease(lease_info) @@ -454,4 +454,3 @@ def create_mutable_sharefile(filename, my_nodeid, write_enabler, parent): ms.create(my_nodeid, write_enabler) del ms return MutableShareFile(filename, parent) - diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index d18960a1e..8123be2c5 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -835,7 +835,7 @@ class Server(unittest.TestCase): # Start out with single lease created with bucket: renewal_secret, cancel_secret = self.create_bucket_5_shares(ss, b"si0") [lease] = ss.get_leases(b"si0") - self.assertEqual(lease.expiration_time, 123 + DEFAULT_RENEWAL_TIME) + self.assertEqual(lease.get_expiration_time(), 123 + DEFAULT_RENEWAL_TIME) # Time passes: clock.advance(123456) @@ -843,7 +843,7 @@ class Server(unittest.TestCase): # Adding a lease with matching renewal secret just renews it: ss.remote_add_lease(b"si0", renewal_secret, cancel_secret) [lease] = ss.get_leases(b"si0") - self.assertEqual(lease.expiration_time, 123 + 123456 + DEFAULT_RENEWAL_TIME) + self.assertEqual(lease.get_expiration_time(), 123 + 123456 + DEFAULT_RENEWAL_TIME) def test_have_shares(self): """By default the StorageServer has no shares.""" @@ -1230,17 +1230,6 @@ class MutableServer(unittest.TestCase): self.failUnlessEqual(a.cancel_secret, b.cancel_secret) self.failUnlessEqual(a.nodeid, b.nodeid) - def compare_leases(self, leases_a, leases_b): - self.failUnlessEqual(len(leases_a), len(leases_b)) - for i in range(len(leases_a)): - a = leases_a[i] - b = leases_b[i] - self.failUnlessEqual(a.owner_num, b.owner_num) - self.failUnlessEqual(a.renew_secret, b.renew_secret) - self.failUnlessEqual(a.cancel_secret, b.cancel_secret) - self.failUnlessEqual(a.nodeid, b.nodeid) - self.failUnlessEqual(a.expiration_time, b.expiration_time) - def test_leases(self): ss = self.create("test_leases") def secrets(n): @@ -1321,11 +1310,11 @@ class MutableServer(unittest.TestCase): self.failUnlessIn("I have leases accepted by nodeids:", e_s) self.failUnlessIn("nodeids: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' .", e_s) - self.compare_leases(all_leases, list(s0.get_leases())) + self.assertEqual(all_leases, list(s0.get_leases())) # reading shares should not modify the timestamp read(b"si1", [], [(0,200)]) - self.compare_leases(all_leases, list(s0.get_leases())) + self.assertEqual(all_leases, list(s0.get_leases())) write(b"si1", secrets(0), {0: ([], [(200, b"make me bigger")], None)}, []) @@ -1359,7 +1348,7 @@ class MutableServer(unittest.TestCase): "shares", storage_index_to_dir(b"si1")) s0 = MutableShareFile(os.path.join(bucket_dir, "0")) [lease] = s0.get_leases() - self.assertEqual(lease.expiration_time, 235 + DEFAULT_RENEWAL_TIME) + self.assertEqual(lease.get_expiration_time(), 235 + DEFAULT_RENEWAL_TIME) # Time passes... clock.advance(835) @@ -1367,7 +1356,7 @@ class MutableServer(unittest.TestCase): # Adding a lease renews it: ss.remote_add_lease(b"si1", renew_secret, cancel_secret) [lease] = s0.get_leases() - self.assertEqual(lease.expiration_time, + self.assertEqual(lease.get_expiration_time(), 235 + 835 + DEFAULT_RENEWAL_TIME) def test_remove(self): diff --git a/src/allmydata/test/test_storage_web.py b/src/allmydata/test/test_storage_web.py index b3f5fac98..e905b240d 100644 --- a/src/allmydata/test/test_storage_web.py +++ b/src/allmydata/test/test_storage_web.py @@ -490,7 +490,7 @@ class LeaseCrawler(unittest.TestCase, pollmixin.PollMixin): # current lease has), so we have to reach inside it. for i,lease in enumerate(sf.get_leases()): if lease.renew_secret == renew_secret: - lease.expiration_time = new_expire_time + lease = lease.renew(new_expire_time) f = open(sf.home, 'rb+') sf._write_lease_record(f, i, lease) f.close()