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

Add IPv4-only HTTP client #34

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

robertodauria
Copy link
Contributor

@robertodauria robertodauria commented Aug 22, 2024

This replaces the default HTTP client from the http package with a client that will always connect over IPv4.

The main reason behind this change is that the Autojoin API currently tries to autodetect the node's IPv4 by reading the HTTP request's client IP. On dual-stack machines, the request could randomly happen over IPv6 and the Autojoin API wouldn't have the minimum information required to register the node. This change mitigates that problem.


This change is Reviewable

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 10510110774

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.212%

Totals Coverage Status
Change from base Build 10495869920: 0.0%
Covered Lines: 714
Relevant Lines: 727

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Would it be preferable overall to make ipv4= parameter required? If we eliminated the fallback in the server then clients would have to specify the address at registration time, and the http connection could use v4 or v6.

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor Author

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

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

After discussing this on Slack, do you think it's worth keeping the ipv4 discovery?

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Yes, I think so. Now I'm wondering if it's a mistake to have the ipv4= parameter! :)

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@robertodauria robertodauria merged commit 187b2b8 into main Aug 22, 2024
8 checks passed
@robertodauria robertodauria deleted the sandbox-roberto-ipv4-only-client branch August 22, 2024 16:32
@robertodauria
Copy link
Contributor Author

It would help in handling the case where the server has multiple interfaces/addresses, and the host wants to serve ndt-server on a specific one (which may not be the default route).

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.

3 participants