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.

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

refactor(iroh-relay)!: Remove relay_endpoint config option & rename /derp route to /relay #2419

Merged
merged 8 commits into from
Jul 2, 2024
Merged

refactor(iroh-relay)!: Remove relay_endpoint config option & rename /derp route to /relay #2419

merged 8 commits into from
Jul 2, 2024

Conversation

matheus23
Copy link
Contributor

@matheus23 matheus23 commented Jun 26, 2024 •
edited
Loading

Description

  • Removes relay_endpoint config option from ServerBuilder
  • Makes iroh-relay accept both /derp and /relay as paths for relaying
  • Makes /relay the default path used in the iroh-net client

TODO:

  • Test both URLs

Breaking Changes

  • iroh_net::relay::http::ServerBuilder: Removed relay_endpoint function

Notes & open questions

Closes #2378

✅ Merge this after #2387

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@matheus23 matheus23 self-assigned this Jun 26, 2024
@matheus23 matheus23 mentioned this pull request Jun 26, 2024
Merged
10 tasks
@dignifiedquire
Copy link
Contributor

any chance we can have a small test, to make sure both urls work?

@matheus23 matheus23 marked this pull request as draft June 26, 2024 19:25
@matheus23
Copy link
Contributor Author

Yes! Had that planned but forgot about it?

I remember looking into it, probably means I'll need to make the URL it dials configurable in the code - possibly passing it in at the ClientBuilder level or so.

Which also reminds me: Would it make sense to include the /relay bit in the RelayUrl? I.e. a relay URL would be e.g. something like ws://relay.iroh.net/relay instead of ws://relay.iroh.net.

@dignifiedquire
Copy link
Contributor

Which also reminds me: Would it make sense to include the /relay bit in the RelayUrl? I.e. a relay URL would be e.g. something like ws://relay.iroh.net/relay instead of ws://relay.iroh.net.

I think this would actually be really good, as it would allow more interesting deployments setups

@dignifiedquire
Copy link
Contributor

actually though, I am not sure, as we hit other endpoints on a relay server, so that URL would be somewhat confusing

cc @ramfox @flub for some thoughts

@matheus23
Copy link
Contributor Author

Let's continue the discussion here.
However, if it turns out we'll want to do this - I'll create an issue ✌️

@matheus23 matheus23 marked this pull request as ready for review June 27, 2024 14:52
@dignifiedquire dignifiedquire added this to the v0.20.0 milestone Jun 28, 2024
Base automatically changed from matheus23/relay-websockets to main June 28, 2024 16:29
@flub
Copy link
Contributor

flub commented Jul 1, 2024

actually though, I am not sure, as we hit other endpoints on a relay server, so that URL would be somewhat confusing

cc @ramfox @flub for some thoughts

Yeah, I think RelayUrl should be a "root" under which we expect to find several paths. E.g. the client code does relay_url.join("derp"), relay_url.join("generate_204") etc. So if someone is using https://example.com/some/path/ I expect them to have done some reverse-proxy stuff to host it all under a path prefix. Does that make sense?

So I would not put the /relay or /derp into the RelayUrl.

flub
flub reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

LGTM but I wonder if this should be split up in two PRs: the server-side and switching the client. We need to be pretty sure that we have deployed the new relay server before we release with the client switched. I'd even do them in two subsequent releases. /cc @Arqu

iroh-net/src/relay/http/client.rs Outdated Show resolved Hide resolved
iroh-net/src/relay/http/server.rs Outdated Show resolved Hide resolved
@matheus23
Copy link
Contributor Author

I wonder if this should be split up in two PRs: the server-side and switching the client

Good idea. Probably not hard to do.
That'd also test that the "new" relay works with the old code (at least it'd test that for a week).

@matheus23 matheus23 mentioned this pull request Jul 2, 2024
Merged
4 tasks
@matheus23
Copy link
Contributor Author

I split out #2441

flub
flub approved these changes Jul 2, 2024
View reviewed changes
iroh-net/src/relay/http.rs
/// (over websockets and a custom upgrade protocol).
pub const RELAY_PATH: &str = "/relay";
/// The HTTP path under which the relay allows doing latency queries for testing.
pub const RELAY_PROBE_PATH: &str = "/relay/probe";
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is just bikesheeding, but nesting /relay/probe is a bit odd, given the other handlers are all on the root (I appreciate this was copied from tailscale). I'd move this to /probe so it is on the same level as /generate_204 etc. We can definitely do this for the new route. We can even remove /derp/probe if we want as it's not yet used (hi https-probe that still lives in a half-dead branch 😓 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep it in this PR as a refactor instead of a change - let's talk about moving it when we're making use of it :)

@matheus23 matheus23 added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit d4fe155 Jul 2, 2024
25 of 26 checks passed
matheus23 added a commit that referenced this pull request Jul 11, 2024
@matheus23
refactor(iroh-net): Switch to new iroh-relay route /relay instead o…
f4f1a89 
…f `/derp` (#2441)

Split out from #2419 

Splitting this out means we can deploy the relay changes one release in
advance to the changes in the client.

Otherwise, we'd need to update the relays ASAP when we release 0.20, as
clients will expect to be able to connect to the `/relay` route (which
may not be deployed immediately after the release).

## Description

- Switches the client to connect to `/relay` instead of `/derp`.

## Breaking Changes

None

## Notes & open questions

Also a PR related to #2378 

I allowed myself to add in another `derive_more::Debug` instance instead
of a custom, outdated `impl Debug`, if that's okay with you 😁

## Change checklist

- [X] Self-review.
- [X] Documentation updates if relevant.
- [X] Tests if relevant.
- [X] All breaking changes documented.
@matheus23 matheus23 mentioned this pull request Jul 11, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jul 18, 2024
@matheus23
refactor(iroh-net): Switch to new iroh-relay route /relay instead o…
b7b493d 
…f `/derp` (#2489)

Split out from #2419, copy of #2441, because I forgot to switch the base
branch 🙃

Splitting this out means we can deploy the relay changes one release in
advance to the changes in the client.

Otherwise, we'd need to update the relays ASAP when we release 0.20, as
clients will expect to be able to connect to the `/relay` route (which
may not be deployed immediately after the release).

## Description

- Switches the client to connect to `/relay` instead of `/derp`.

## Breaking Changes

None

## Notes & open questions

Also a PR related to #2378 

I allowed myself to add in another `derive_more::Debug` instance instead
of a custom, outdated `impl Debug`, if that's okay with you 😁

## Change checklist

- [X] Self-review.
- [X] Documentation updates if relevant.
- [X] Tests if relevant.
- [X] All breaking changes documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flub flub flub approved these changes

Assignees

@matheus23 matheus23

Labels
None yet
Projects
iroh
Archived in project
Milestone
v0.20.0
Development

Successfully merging this pull request may close these issues.

Move relay HTTP handler to be served on /relay
3 participants
@matheus23 @dignifiedquire @flub