Skip to content

filter DNS query results to only include addresses that our quic endpoint can use #166

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

Jump to bottom
Merged
kixelated merged 3 commits into from
May 30, 2024
Merged

filter DNS query results to only include addresses that our quic endpoint can use #166

kixelated merged 3 commits into from
May 30, 2024

Conversation

@Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented May 30, 2024

Minor bug fix. Without this change it's possible that the connection will attempt to use an IP address which the internal quinn::Endpoint can't actually connect to.

kixelated
kixelated approved these changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@kixelated kixelated left a comment

Choose a reason for hiding this comment

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

thanks!

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented May 30, 2024

I went ahead and fixed the clippy warning. I also adjusted the "context" of the error to more accurately reflect the actual problem.

@kixelated kixelated merged commit 1230cd6 into moq-dev:main May 30, 2024
@Xaeroxe Xaeroxe deleted the fix-dns-query branch May 30, 2024 19:54
@Xaeroxe Xaeroxe restored the fix-dns-query branch May 30, 2024 19:55
@Xaeroxe Xaeroxe deleted the fix-dns-query branch May 30, 2024 19:55
kixelated added a commit that referenced this pull request Jun 3, 2024
@kixelated
Revert "filter DNS query results to only include addresses that our q…
fb362bf 
…uic endpoint can use (#166)"

This breaks my moq-pub client and the bbb stream in production.
I think it's using IPv6 to connect to a IPv4 address which is valid.

This reverts commit 1230cd6.
@kixelated
Copy link
Collaborator

kixelated commented Jun 3, 2024

Yo, reverting this because it breaks production. I think the moq-pub client binds via ipv6 but connects to a ipv4 address, which should still be valid?

@kixelated
Copy link
Collaborator

kixelated commented Jun 3, 2024

But yeah the IPv4 vs IPv6 stuff is a mess.

@github-actions github-actions bot mentioned this pull request Jun 3, 2024
Merged
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jun 3, 2024 •
edited
Loading

I suspect I didn’t account for the IPV6_V6ONLY flag being set to false. The default value for this flag varies by operating system.

This would be easier if moq-rs created the quinn::Endpoint internally rather than accepting one from the caller.

@kixelated
Copy link
Collaborator

kixelated commented Jun 3, 2024

I suspect I didn’t account for the IPV6_V6ONLY flag being set to false. The default value for this flag varies by operating system.

This would be easier if moq-rs created the quinn::Endpoint internally rather than accepting one from the caller.

moq-native creates the quinn::Endpoint, because yeah it's a bit advanced to create your own.

ajay-s-2502 pushed a commit to ajay-s-2502/moq-rs that referenced this pull request Nov 22, 2024
@Xaeroxe
filter DNS query results to only include addresses that our quic endp…
926d5f6 
…oint can use (moq-dev#166)
ajay-s-2502 pushed a commit to ajay-s-2502/moq-rs that referenced this pull request Nov 22, 2024
@kixelated
Revert "filter DNS query results to only include addresses that our q…
35e73e7 
…uic endpoint can use (moq-dev#166)"

This breaks my moq-pub client and the bbb stream in production.
I think it's using IPv6 to connect to a IPv4 address which is valid.

This reverts commit 926d5f6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kixelated kixelated kixelated approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Xaeroxe @kixelated