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

change: use DERP port from host_name URL #1143

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Conversation

Frando
Copy link
Member

@Frando Frando commented Jun 28, 2023

Now that DerpNode::host_name is an URL, the DERP port can be part of that URL, so we don't need a separate derp_port field anymore.

This makes setting custom DERP config or accepting a DERP server from the command line much simpler, because you need only a single URL string and not multiple options.

It looks a bit weird in the test code because oftenly those use http://derp.invalid:{port} now. I think we could just put the full IP in the address also and not set UseIpv4::Some().

dignifiedquire
dignifiedquire previously approved these changes Jun 28, 2023
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

yesss, much better

@dignifiedquire
Copy link
Contributor

looks like some tests are failing though

@Frando
Copy link
Member Author

Frando commented Jun 30, 2023

The remaining fail is in netsim - never worked with those, how do I debug this?

@Arqu
Copy link
Collaborator

Arqu commented Jun 30, 2023

So looking at both, netsim has a derpmap provided. Unsure where the issue comes from, but in the logs, seems like both the server and client fail handshakes (presumably with the derper). My assumption here is that now that the derp_port is obsolete it tries to do just http but some port got misplaced and now it fails to establish a connection.

Note that the test that is failing is where the provide side is behind a NAT, so some confusion might be happening due to the setup. @dignifiedquire already dealt with a similar bug before.

TLDR; either it's a config issue and you might be able to help me understand where and I can update netsim or we re-introduced a bug we closed in #1128

Frando added a commit to Frando/chuck that referenced this pull request Jun 30, 2023
Change that goes along with n0-computer/iroh#1143
@Frando
Copy link
Member Author

Frando commented Jun 30, 2023

Ah, so the netsim config would need to change to this with/after this PR:
n0-computer/chuck@main...Frando:chuck:patch-1

How are changes between chunk and iroh synchronized currently?

@Frando
Copy link
Member Author

Frando commented Jun 30, 2023

Something else: Now that host_name is a URL with optional port, host_name is not really the fitting term anymore, is it? Maybe we should change it to derp_url or derp_server or something similar right away?

@dignifiedquire
Copy link
Contributor

host_name is not really the fitting term anymore

Maybe just host or url?

@dignifiedquire
Copy link
Contributor

merge conflict

@Frando Frando force-pushed the Frando/derp-port-from-url branch 2 times, most recently from 31927ed to bb370ca Compare July 3, 2023 21:38
@Frando
Copy link
Member Author

Frando commented Jul 3, 2023

Rebased and changed the field name to url.

I was undecided about naming the field host vs url because neither is fully correct, but URL seemed "more correct" as we support 3 parts of a typical URL (scheme, host, port) and just not any path parts, but more than just a hostname. But can ofc change again if something else is deemed better.

@Frando Frando force-pushed the Frando/derp-port-from-url branch from bb370ca to b8a3f8c Compare July 3, 2023 21:42
@Arqu
Copy link
Collaborator

Arqu commented Jul 4, 2023

I'm happy with where it's at. Once approved, we just need to bump the netsim derp map config too.

@Arqu Arqu merged commit fbeec14 into main Jul 5, 2023
@Arqu Arqu deleted the Frando/derp-port-from-url branch July 5, 2023 14:03
@Frando Frando mentioned this pull request Jul 10, 2023
2 tasks
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