Skip to content

Class Unique fix #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jstasiak merged 6 commits into from
Feb 2, 2020
Merged

Class Unique fix #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9a5a45
Ensure all TXT, SRV, A records are unique
mattsaxon Jan 19, 2020
d725c4e
Add tests for uniqueness
mattsaxon Jan 19, 2020
ec16abb
linting
mattsaxon Jan 19, 2020
d21314e
Update test.py
mattsaxon Jan 20, 2020
2751e70
fix a couple of unit tests to be consistent with spec
mattsaxon Feb 1, 2020
c6472e2
Remove unwanted unittest block
jstasiak Feb 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 48 additions & 9 deletions zeroconf/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,13 +887,33 @@ def add_question(self, record: DNSQuestion) -> None:
self.questions.append(record)

def add_answer(self, inp: DNSIncoming, record: DNSRecord) -> None:

"""Only support for unique answers"""
if (
record.type == _TYPE_TXT
or record.type == _TYPE_SRV
or record.type == _TYPE_A
or record.type == _TYPE_AAAA
):
assert record.unique

"""Adds an answer"""
if not record.suppressed_by(inp):
self.add_answer_at_time(record, 0)

def add_answer_at_time(self, record: Optional[DNSRecord], now: Union[float, int]) -> None:
"""Adds an answer if it does not expire by a certain time"""
if record is not None:

"""Only support for unique answers"""
if (
record.type == _TYPE_TXT
or record.type == _TYPE_SRV
or record.type == _TYPE_A
or record.type == _TYPE_AAAA
):
assert record.unique

@jstasiak jstasiak Feb 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd think about putting those if/assert blocks in a function, but it's no biggie. I'd at least remove the comments, the code is crystal clear on what it's going on here.

@mattsaxon mattsaxon Feb 2, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to remove comment, do you want me to resubmit a PR?

@jstasiak jstasiak Feb 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's up to you, we can merge as-is.

@jstasiak jstasiak Feb 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and merged it, we can clean it up later.

@mattsaxon mattsaxon Feb 3, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New PR submitted t address this #225


if now == 0 or not record.is_expired(now):
self.answers.append((record, now))

Expand Down Expand Up @@ -937,6 +957,15 @@ def add_additional_answer(self, record: DNSRecord) -> None:
o All address records (type "A" and "AAAA") named in the SRV rdata.

"""
"""Only support for unique answers"""
if (
record.type == _TYPE_TXT
or record.type == _TYPE_SRV
or record.type == _TYPE_A
or record.type == _TYPE_AAAA
):
assert record.unique

self.additionals.append(record)

def pack(self, format_: Union[bytes, str], value: Any) -> None:
Expand Down Expand Up @@ -2244,7 +2273,7 @@ def _broadcast_service(self, info: ServiceInfo) -> None:
DNSService(
info.name,
_TYPE_SRV,
_CLASS_IN,
_CLASS_IN | _CLASS_UNIQUE,
info.host_ttl,
info.priority,
info.weight,
Expand All @@ -2254,10 +2283,14 @@ def _broadcast_service(self, info: ServiceInfo) -> None:
0,
)

out.add_answer_at_time(DNSText(info.name, _TYPE_TXT, _CLASS_IN, info.other_ttl, info.text), 0)
out.add_answer_at_time(
DNSText(info.name, _TYPE_TXT, _CLASS_IN | _CLASS_UNIQUE, info.other_ttl, info.text), 0
)
for address in info.addresses_by_version(IPVersion.All):
type_ = _TYPE_AAAA if _is_v6_address(address) else _TYPE_A
out.add_answer_at_time(DNSAddress(info.server, type_, _CLASS_IN, info.host_ttl, address), 0)
out.add_answer_at_time(
DNSAddress(info.server, type_, _CLASS_IN | _CLASS_UNIQUE, info.host_ttl, address), 0
)
self.send(out)
i += 1
next_time += _REGISTER_TIME
Expand Down Expand Up @@ -2286,7 +2319,7 @@ def unregister_service(self, info: ServiceInfo) -> None:
DNSService(
info.name,
_TYPE_SRV,
_CLASS_IN,
_CLASS_IN | _CLASS_UNIQUE,
0,
info.priority,
info.weight,
Expand All @@ -2295,11 +2328,13 @@ def unregister_service(self, info: ServiceInfo) -> None:
),
0,
)
out.add_answer_at_time(DNSText(info.name, _TYPE_TXT, _CLASS_IN, 0, info.text), 0)
out.add_answer_at_time(DNSText(info.name, _TYPE_TXT, _CLASS_IN | _CLASS_UNIQUE, 0, info.text), 0)

for address in info.addresses_by_version(IPVersion.All):
type_ = _TYPE_AAAA if _is_v6_address(address) else _TYPE_A
out.add_answer_at_time(DNSAddress(info.server, type_, _CLASS_IN, 0, address), 0)
out.add_answer_at_time(
DNSAddress(info.server, type_, _CLASS_IN | _CLASS_UNIQUE, 0, address), 0
)
self.send(out)
i += 1
next_time += _UNREGISTER_TIME
Expand All @@ -2322,7 +2357,7 @@ def unregister_all_services(self) -> None:
DNSService(
info.name,
_TYPE_SRV,
_CLASS_IN,
_CLASS_IN | _CLASS_UNIQUE,
0,
info.priority,
info.weight,
Expand All @@ -2331,10 +2366,14 @@ def unregister_all_services(self) -> None:
),
0,
)
out.add_answer_at_time(DNSText(info.name, _TYPE_TXT, _CLASS_IN, 0, info.text), 0)
out.add_answer_at_time(
DNSText(info.name, _TYPE_TXT, _CLASS_IN | _CLASS_UNIQUE, 0, info.text), 0
)
for address in info.addresses_by_version(IPVersion.All):
type_ = _TYPE_AAAA if _is_v6_address(address) else _TYPE_A
out.add_answer_at_time(DNSAddress(info.server, type_, _CLASS_IN, 0, address), 0)
out.add_answer_at_time(
DNSAddress(info.server, type_, _CLASS_IN | _CLASS_UNIQUE, 0, address), 0
)
self.send(out)
i += 1
next_time += _UNREGISTER_TIME
Expand Down
54 changes: 43 additions & 11 deletions zeroconf/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,17 @@ def test_parse_own_packet_question(self):
def test_parse_own_packet_response(self):
generated = r.DNSOutgoing(r._FLAGS_QR_RESPONSE)
generated.add_answer_at_time(
r.DNSService("æøå.local.", r._TYPE_SRV, r._CLASS_IN, r._DNS_HOST_TTL, 0, 0, 80, "foo.local."), 0
r.DNSService(
"æøå.local.",
r._TYPE_SRV,
r._CLASS_IN | r._CLASS_UNIQUE,
r._DNS_HOST_TTL,
0,
0,
80,
"foo.local.",
),
0,
)
parsed = r.DNSIncoming(generated.packet())
self.assertEqual(len(generated.answers), 1)
Expand All @@ -166,13 +176,34 @@ def test_suppress_answer(self):
question = r.DNSQuestion("testname.local.", r._TYPE_SRV, r._CLASS_IN)
query_generated.add_question(question)
answer1 = r.DNSService(
"testname1.local.", r._TYPE_SRV, r._CLASS_IN, r._DNS_HOST_TTL, 0, 0, 80, "foo.local."
"testname1.local.",
r._TYPE_SRV,
r._CLASS_IN | r._CLASS_UNIQUE,
r._DNS_HOST_TTL,
0,
0,
80,
"foo.local.",
)
staleanswer2 = r.DNSService(
"testname2.local.", r._TYPE_SRV, r._CLASS_IN, r._DNS_HOST_TTL / 2, 0, 0, 80, "foo.local."
"testname2.local.",
r._TYPE_SRV,
r._CLASS_IN | r._CLASS_UNIQUE,
r._DNS_HOST_TTL / 2,
0,
0,
80,
"foo.local.",
)
answer2 = r.DNSService(
"testname2.local.", r._TYPE_SRV, r._CLASS_IN, r._DNS_HOST_TTL, 0, 0, 80, "foo.local."
"testname2.local.",
r._TYPE_SRV,
r._CLASS_IN | r._CLASS_UNIQUE,
r._DNS_HOST_TTL,
0,
0,
80,
"foo.local.",
)
query_generated.add_answer_at_time(answer1, 0)
query_generated.add_answer_at_time(staleanswer2, 0)
Expand Down Expand Up @@ -444,7 +475,8 @@ def generate_host(zc, host_name, type_):
out = r.DNSOutgoing(r._FLAGS_QR_RESPONSE | r._FLAGS_AA)
out.add_answer_at_time(r.DNSPointer(type_, r._TYPE_PTR, r._CLASS_IN, r._DNS_OTHER_TTL, name), 0)
out.add_answer_at_time(
r.DNSService(type_, r._TYPE_SRV, r._CLASS_IN, r._DNS_HOST_TTL, 0, 0, 80, name), 0
r.DNSService(type_, r._TYPE_SRV, r._CLASS_IN | r._CLASS_UNIQUE, r._DNS_HOST_TTL, 0, 0, 80, name),
0,
)
zc.send(out)

Expand Down Expand Up @@ -487,7 +519,7 @@ def mock_incoming_msg(service_state_change: r.ServiceStateChange) -> r.DNSIncomi
ttl = 0

generated.add_answer_at_time(
r.DNSPointer(service_type, r._TYPE_PTR, r._CLASS_IN | r._CLASS_UNIQUE, ttl, service_name), 0
r.DNSPointer(service_type, r._TYPE_PTR, r._CLASS_IN, ttl, service_name), 0
)
generated.add_answer_at_time(
r.DNSService(
Expand Down Expand Up @@ -670,7 +702,7 @@ def test_incoming_ipv6(self):
addr = "2606:2800:220:1:248:1893:25c8:1946" # example.com
packed = socket.inet_pton(socket.AF_INET6, addr)
generated = r.DNSOutgoing(0)
answer = r.DNSAddress('domain', r._TYPE_AAAA, r._CLASS_IN, 1, packed)
answer = r.DNSAddress('domain', r._TYPE_AAAA, r._CLASS_IN | r._CLASS_UNIQUE, 1, packed)
generated.add_additional_answer(answer)
packet = generated.packet()
parsed = r.DNSIncoming(packet)
Expand Down Expand Up @@ -886,6 +918,7 @@ def test_integration_with_listener_class(self):
service_added = Event()
service_removed = Event()
service_updated = Event()
service_updated2 = Event()

subtype_name = "My special Subtype"
type_ = "_http._tcp.local."
Expand All @@ -902,7 +935,7 @@ def remove_service(self, zeroconf, type, name):
service_removed.set()

def update_service(self, zeroconf, type, name):
pass
service_updated2.set()

class MySubListener(r.ServiceListener):
def add_service(self, zeroconf, type, name):
Expand Down Expand Up @@ -966,7 +999,7 @@ def update_service(self, zeroconf, type, name):
assert info is not None
assert info.properties[b'prop_none'] is False

# Begin material test addition
# test TXT record update
sublistener = MySubListener()
zeroconf_browser.add_service_listener(registration_name, sublistener)
properties['prop_blank'] = b'an updated string'
Expand All @@ -981,7 +1014,6 @@ def update_service(self, zeroconf, type, name):
info = zeroconf_browser.get_service_info(type_, registration_name)
assert info is not None
assert info.properties[b'prop_blank'] == properties['prop_blank']
# End material test addition

zeroconf_registrar.unregister_service(info_service)
service_removed.wait(1)
Expand Down Expand Up @@ -1043,7 +1075,7 @@ def mock_incoming_msg(service_state_change: r.ServiceStateChange) -> r.DNSIncomi
ttl = 0

generated.add_answer_at_time(
r.DNSPointer(service_type, r._TYPE_PTR, r._CLASS_IN | r._CLASS_UNIQUE, ttl, service_name), 0
r.DNSPointer(service_type, r._TYPE_PTR, r._CLASS_IN, ttl, service_name), 0
)
generated.add_answer_at_time(
r.DNSService(
Expand Down