From a6f7813f8de10e338e58e570534faa05516f7f99 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 22 Oct 2021 09:58:31 -1000 Subject: [PATCH 1/4] Ensure ServiceInfo orders newest addreses first - In order to comply with rfc6762 section 10.2, cache flushed records expire 1s in the future. If a ServiceInfo request is made while the records are still in the cache and not yet expired, the first address would be returned which was the address that was about to be expunged from the cache. We now return the newest address first to reduce the risk consumers of ServiceInfo will try the old address. This effectively restores the behavior before 1s cache flush expire was added for callers that rely on this. --- tests/services/test_info.py | 26 +++++++++++++-- tests/test_exceptions.py | 4 +-- zeroconf/_services/info.py | 65 ++++++++++++++++++++++++------------- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/tests/services/test_info.py b/tests/services/test_info.py index a72d82f9..9c9bfa02 100644 --- a/tests/services/test_info.py +++ b/tests/services/test_info.py @@ -560,7 +560,27 @@ def test_multiple_addresses(): # This test uses asyncio because it needs to access the cache directly # which is not threadsafe @pytest.mark.asyncio -async def test_multiple_a_addresses(): +async def test_multiple_a_addresses_newest_address_first(): + """Test that info.addresses returns the newest seen address first.""" + type_ = "_http._tcp.local." + registration_name = "multiarec.%s" % type_ + desc = {'path': '/~paulsm/'} + aiozc = AsyncZeroconf(interfaces=['127.0.0.1']) + cache = aiozc.zeroconf.cache + host = "multahost.local." + record1 = r.DNSAddress(host, const._TYPE_A, const._CLASS_IN, 1000, b'\x7f\x00\x00\x01') + record2 = r.DNSAddress(host, const._TYPE_A, const._CLASS_IN, 1000, b'\x7f\x00\x00\x02') + cache.async_add_records([record1, record2]) + + # New kwarg way + info = ServiceInfo(type_, registration_name, 80, 0, 0, desc, host) + info.load_from_cache(aiozc.zeroconf) + assert info.addresses == [b'\x7f\x00\x00\x02', b'\x7f\x00\x00\x01'] + await aiozc.async_close() + + +@pytest.mark.asyncio +async def test_invalid_a_addresses(caplog): type_ = "_http._tcp.local." registration_name = "multiarec.%s" % type_ desc = {'path': '/~paulsm/'} @@ -574,7 +594,9 @@ async def test_multiple_a_addresses(): # New kwarg way info = ServiceInfo(type_, registration_name, 80, 0, 0, desc, host) info.load_from_cache(aiozc.zeroconf) - assert set(info.addresses) == set([b'a', b'b']) + assert not info.addresses + assert "Encountered invalid address while processing record" in caplog.text + await aiozc.async_close() diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 47e68b75..6b682093 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -141,11 +141,11 @@ def test_invalid_addresses(self): name = "xxxyyy" registration_name = f"{name}.{type_}" - bad = ('127.0.0.1', '::1', 42) + bad = (b'127.0.0.1', b'::1') for addr in bad: self.assertRaisesRegex( TypeError, - 'Addresses must be bytes', + 'Addresses must either ', ServiceInfo, type_, registration_name, diff --git a/zeroconf/_services/info.py b/zeroconf/_services/info.py index beaf0678..e968e149 100644 --- a/zeroconf/_services/info.py +++ b/zeroconf/_services/info.py @@ -27,6 +27,7 @@ from .._dns import DNSAddress, DNSPointer, DNSQuestionType, DNSRecord, DNSService, DNSText from .._exceptions import BadTypeInNameException +from .._logger import log from .._protocol.outgoing import DNSOutgoing from .._updates import RecordUpdate, RecordUpdateListener from .._utils.asyncio import get_running_loop, run_coro_with_timeout @@ -124,19 +125,12 @@ def __init__( self.type = type_ self._name = name self.key = name.lower() + self._ipv4_addresses: List[ipaddress.IPv4Address] = [] + self._ipv6_addresses: List[ipaddress.IPv6Address] = [] if addresses is not None: - self._addresses = addresses + self.addresses = addresses elif parsed_addresses is not None: - self._addresses = [_encode_address(a) for a in parsed_addresses] - else: - self._addresses = [] - # This results in an ugly error when registering, better check now - invalid = [a for a in self._addresses if not isinstance(a, bytes) or len(a) not in (4, 16)] - if invalid: - raise TypeError( - 'Addresses must be bytes, got %s. Hint: convert string addresses ' - 'with socket.inet_pton' % invalid - ) + self.addresses = [_encode_address(a) for a in parsed_addresses] self.port = port self.weight = weight self.priority = priority @@ -178,7 +172,20 @@ def addresses(self, value: List[bytes]) -> None: This replaces all currently stored addresses, both IPv4 and IPv6. """ - self._addresses = value + self._ipv4_addresses.clear() + self._ipv6_addresses.clear() + + for address in value: + try: + addr = ipaddress.ip_address(address) + except ValueError: + raise TypeError( + f'Addresses must either be IPv4 or IPv6 strings, bytes, or integers; got {address}. Hint: convert string addresses with socket.inet_pton' + ) + if addr.version == 4: + self._ipv4_addresses.append(addr) + else: + self._ipv6_addresses.append(addr) @property def properties(self) -> Dict: @@ -194,10 +201,13 @@ def properties(self) -> Dict: def addresses_by_version(self, version: IPVersion) -> List[bytes]: """List addresses matching IP version.""" if version == IPVersion.V4Only: - return [addr for addr in self._addresses if not _is_v6_address(addr)] + return [addr.packed for addr in self._ipv4_addresses] if version == IPVersion.V6Only: - return list(filter(_is_v6_address, self._addresses)) - return self._addresses + return [addr.packed for addr in self._ipv6_addresses] + return [ + *(addr.packed for addr in self._ipv4_addresses), + *(addr.packed for addr in self._ipv6_addresses), + ] def parsed_addresses(self, version: IPVersion = IPVersion.All) -> List[str]: """List addresses in their parsed string form.""" @@ -220,7 +230,7 @@ def is_link_local(addr_str: str) -> Any: ll_addrs = list(filter(is_link_local, self.parsed_addresses(version))) other_addrs = list(filter(lambda addr: not is_link_local(addr), self.parsed_addresses(version))) - return ["{}%{}".format(addr, self.interface_index) for addr in ll_addrs] + other_addrs + return [f"{addr}%{self.interface_index}" for addr in ll_addrs] + other_addrs def _set_properties(self, properties: Dict) -> None: """Sets properties and text of this info from a dictionary""" @@ -315,9 +325,20 @@ def _process_record_threadsafe(self, record: DNSRecord, now: float) -> None: return if isinstance(record, DNSAddress): - if record.key == self.server_key and record.address not in self._addresses: - self._addresses.append(record.address) - if record.type is _TYPE_AAAA and ipaddress.IPv6Address(record.address).is_link_local: + if record.key != self.server_key: + return + try: + ip_addr = ipaddress.ip_address(record.address) + except ValueError as ex: + log.warning("Encountered invalid address while processing %s: %s", record, ex) + return + if ip_addr.version == 4: + if ip_addr not in self._ipv4_addresses: + self._ipv4_addresses.insert(0, ip_addr) + return + if ip_addr not in self._ipv6_addresses: + self._ipv6_addresses.insert(0, ip_addr) + if ip_addr.is_link_local: self.interface_index = record.scope_id return @@ -422,7 +443,7 @@ def load_from_cache(self, zc: 'Zeroconf') -> bool: @property def _is_complete(self) -> bool: """The ServiceInfo has all expected properties.""" - return not (self.text is None or not self._addresses) + return self.text is not None and (self._ipv4_addresses or self._ipv6_addresses) def request( self, zc: 'Zeroconf', timeout: float, question_type: Optional[DNSQuestionType] = None @@ -494,10 +515,10 @@ def __eq__(self, other: object) -> bool: def __repr__(self) -> str: """String representation""" - return '%s(%s)' % ( + return '{}({})'.format( type(self).__name__, ', '.join( - '%s=%r' % (name, getattr(self, name)) + '{}={!r}'.format(name, getattr(self, name)) for name in ( 'type', 'name', From 005a1ca267c8863f33693aa831a38bbe90148d73 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 22 Oct 2021 10:03:21 -1000 Subject: [PATCH 2/4] flake8 --- zeroconf/_services/info.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zeroconf/_services/info.py b/zeroconf/_services/info.py index e968e149..731434ec 100644 --- a/zeroconf/_services/info.py +++ b/zeroconf/_services/info.py @@ -180,7 +180,8 @@ def addresses(self, value: List[bytes]) -> None: addr = ipaddress.ip_address(address) except ValueError: raise TypeError( - f'Addresses must either be IPv4 or IPv6 strings, bytes, or integers; got {address}. Hint: convert string addresses with socket.inet_pton' + 'Addresses must either be IPv4 or IPv6 strings, bytes, or integers;' + f' got {address}. Hint: convert string addresses with socket.inet_pton' ) if addr.version == 4: self._ipv4_addresses.append(addr) From d4cfc385a11642639662d3d7ba01ee8b64741aa4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 22 Oct 2021 10:10:16 -1000 Subject: [PATCH 3/4] mypy --- zeroconf/_services/info.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/zeroconf/_services/info.py b/zeroconf/_services/info.py index 731434ec..8b520688 100644 --- a/zeroconf/_services/info.py +++ b/zeroconf/_services/info.py @@ -180,8 +180,8 @@ def addresses(self, value: List[bytes]) -> None: addr = ipaddress.ip_address(address) except ValueError: raise TypeError( - 'Addresses must either be IPv4 or IPv6 strings, bytes, or integers;' - f' got {address}. Hint: convert string addresses with socket.inet_pton' + "Addresses must either be IPv4 or IPv6 strings, bytes, or integers;" + f" got %s. Hint: convert string addresses with socket.inet_pton" # type: ignore ) if addr.version == 4: self._ipv4_addresses.append(addr) @@ -444,7 +444,7 @@ def load_from_cache(self, zc: 'Zeroconf') -> bool: @property def _is_complete(self) -> bool: """The ServiceInfo has all expected properties.""" - return self.text is not None and (self._ipv4_addresses or self._ipv6_addresses) + return bool(self.text is not None and (self._ipv4_addresses or self._ipv6_addresses)) def request( self, zc: 'Zeroconf', timeout: float, question_type: Optional[DNSQuestionType] = None From 17b12f3774cd15bdfda9a7d788c4dd9e0c49cd01 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 22 Oct 2021 10:11:41 -1000 Subject: [PATCH 4/4] mypy --- zeroconf/_services/info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeroconf/_services/info.py b/zeroconf/_services/info.py index 8b520688..2558f726 100644 --- a/zeroconf/_services/info.py +++ b/zeroconf/_services/info.py @@ -181,7 +181,7 @@ def addresses(self, value: List[bytes]) -> None: except ValueError: raise TypeError( "Addresses must either be IPv4 or IPv6 strings, bytes, or integers;" - f" got %s. Hint: convert string addresses with socket.inet_pton" # type: ignore + f" got {address}. Hint: convert string addresses with socket.inet_pton" # type: ignore ) if addr.version == 4: self._ipv4_addresses.append(addr)