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

fix: online stun test #1065

Merged
merged 6 commits into from
Jun 6, 2023
Merged

fix: online stun test #1065

merged 6 commits into from
Jun 6, 2023

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Jun 2, 2023 •
edited
Loading

maybe it helps against test flakiness of test_google_stun

Servers from https://gist.github.com/mondain/b0ec1cf5f60ae726202e

@rklaehn
fix: add multiple google stun servers
e3351a6 
maybe it helps against test flakiness...
@rklaehn rklaehn requested a review from flub June 2, 2023 08:19
flub
flub approved these changes Jun 2, 2023
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.

I'm unsure, i worry the log output will be more messy and as l.google.com is already loadbalanced between datacenters with each datacenter having multiple servers running. but otoh, who knows?

flub
flub reviewed Jun 2, 2023
View reviewed changes
src/hp/netcheck.rs Outdated
UseIpv6::None,

let stun_servers = vec![
("stun.l.google.com", 19302, 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

can you anchor these at the root? stun.l.google.com.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

because we do DNS lookups if you specify the stun server by hostname. not having it anchored at the root means that the local "search" directives (i forget the official name) of your DNS configuration are applied. e.g. on many home networks that would result in lookups to stun.l.google.com.lan or stun.l.google.com.local, as well as the intended stun.l.google.com.

By using stun.l.google.com. you avoid this.

@rklaehn
Copy link
Contributor Author

rklaehn commented Jun 2, 2023

Hm, doesn't seem to help...

failures:
    hp::netcheck::tests::test_google_stun

@rklaehn
fix: point to derp.iroh.computer. and rename the test
af13dca 
Keep the loop so we can add more stun servers later.
@flub flub changed the title fix: add multiple google stun servers fix: online stun test Jun 5, 2023
flub added 2 commits June 5, 2023 12:19
@flub
Log STUN transaction IDs in derper and netcheck
0c68e69 
This should allow us to match requests across the services.
@flub
remove obsolete comment
d16be25
rklaehn
rklaehn commented Jun 6, 2023
View reviewed changes
src/hp/netcheck/probe.rs
@@ -53,7 +53,7 @@ pub enum Probe {
Https {
delay: Duration,
node: String,
reg: DerpRegion,
region: DerpRegion,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

: +1:

@flub flub merged commit bec1bbe into x-hp Jun 6, 2023
@flub flub deleted the x-hp-multistun branch June 6, 2023 08:31
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

@dignifiedquire dignifiedquire dignifiedquire 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.
3 participants
@rklaehn @flub @dignifiedquire