Skip to content
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.

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

query TYPE_A and TYPE_AAAA via Command::Resolve #185

Merged
merged 4 commits into from
Mar 20, 2024
Merged

query TYPE_A and TYPE_AAAA via Command::Resolve #185

merged 4 commits into from
Mar 20, 2024

Conversation

hrzlgnm
Copy link
Contributor

@hrzlgnm hrzlgnm commented Mar 19, 2024

This fixes issues with resolving records published by python-zeroconf or systemd-resolved without an actual A or AAAA address assigned.

Closes #182

Manually tested on linux and windows successfully

@hrzlgnm hrzlgnm changed the title Send A and AAAA queries if we have only a hostname form SRV Send A and AAAA queries if we only got a hostname form a SRV record Mar 19, 2024
Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Thanks for coming up with the fix! Good work! I have two comments:

  1. Should we use the existing Command::Resolve to send this query? The reason is that it is possible that the responses came in more than 1 DnsIncoming datagram. This will give it a chance to process completely. It will also handle retry.

I opened PR #186 to show what I meant.

  1. As an optimization, we can refactor send_query to accept more than 1 type so that we can ask more than 1 questions in the packet. So we can reduce the network traffic. (We / I can do this later, just a comment here)

@hrzlgnm
Copy link
Contributor Author

hrzlgnm commented Mar 20, 2024

Thanks for coming up with the fix! Good work! I have two comments:

1. Should we use the existing `Command::Resolve` to send this query? The reason is that it is possible that the responses came in more than 1 `DnsIncoming` datagram. This will give it a chance to process completely.  It will also handle retry.

I opened PR #186 to show what I meant.

2. As an optimization, we can refactor `send_query` to accept more than 1 type so that we can ask more than 1 questions in the packet. So we can reduce the network traffic. (We  / I can do this later, just a comment here)

Regarding 1. yes that makes more sense to me as we wouldn't retry endlessly if we never receive a reply.

Regarding 2: yes makes sense to send multiple queries in one packet to reduce network traffic.

@keepsimple1
Copy link
Owner

Regarding 1. yes that makes more sense to me as we wouldn't retry endlessly if we never receive a reply.

Regarding 2: yes makes sense to send multiple queries in one packet to reduce network traffic.

I think at least two options we can do:

a) Merge your current changes in this PR and then I will follow up with changes using the other PR. And will need your help to verify your use case still works.

b) You can update this PR with suggestion changes (particularly 1) and we merge it.

I'm good with either approaches. Let me know what you think?

@hrzlgnm
Copy link
Contributor Author

hrzlgnm commented Mar 20, 2024

I'd rather do b to avoid the unclean intermediate state

@hrzlgnm
Copy link
Contributor Author

hrzlgnm commented Mar 20, 2024

I'll update my pr after work

@hrzlgnm
Copy link
Contributor Author

hrzlgnm commented Mar 20, 2024

Update the PR and retested my 2 cases successfully on windows and linux.

@hrzlgnm hrzlgnm requested a review from keepsimple1 March 20, 2024 16:34
@hrzlgnm hrzlgnm changed the title Send A and AAAA queries if we only got a hostname form a SRV record query TYPE_A and TYPE_AAAA via Command::Resolve Mar 20, 2024
@hrzlgnm
Copy link
Contributor Author

hrzlgnm commented Mar 20, 2024

And thank you very much for the explanatory PR, it was very educational 👍

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Thank you for the update! LGTM!

@keepsimple1 keepsimple1 merged commit e6fa83b into keepsimple1:main Mar 20, 2024
3 checks passed
@hrzlgnm hrzlgnm deleted the fix-resolve-zeroconf-systemd-resolved branch March 20, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Services published using python-zerconf or systemd-resolved are not resolved
2 participants