-
Notifications
You must be signed in to change notification settings - Fork 353
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
Fix connection being made without bridge if there are no bridges #6083
Fix connection being made without bridge if there are no bridges #6083
Conversation
3744234
to
e2e7310
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)
mullvad-relay-selector/src/error.rs
line 31 at r1 (raw file):
}, #[error("No candidates remain")]
Maybe this is a bit vague. "No candidate bridge found"? Or just use NoBridge
.
mullvad-relay-selector/src/relay_selector/mod.rs
line 934 at r1 (raw file):
custom_lists, ) .map(|(settings, relay)| SelectedBridge::Normal { settings, relay });
Maybe Result<Option<_>>
would be slightly better?
let bridge = stuff(...)?;
Ok(Some(bridge))
And there's no need to transpose the option above. But this is probably nitpicky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)
mullvad-relay-selector/src/relay_selector/mod.rs
line 961 at r1 (raw file):
.ok_or(Error::NoRelay), }?; let endpoint = helpers::pick_random_bridge(bridge_data, &bridge).ok_or(Error::NoBridge)?;
This arguably belongs in detailer
. Isn't it really just "filling in details" for a bridge we've already picked?
e2e7310
to
21dad0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dlon)
mullvad-relay-selector/src/relay_selector/mod.rs
line 934 at r1 (raw file):
Previously, dlon (David Lönnhager) wrote…
Maybe
Result<Option<_>>
would be slightly better?let bridge = stuff(...)?; Ok(Some(bridge))
And there's no need to transpose the option above. But this is probably nitpicky.
Done
mullvad-relay-selector/src/error.rs
line 31 at r1 (raw file):
Previously, dlon (David Lönnhager) wrote…
Maybe this is a bit vague. "No candidate bridge found"? Or just use
NoBridge
.
Sure, I'll just re-use NoBridge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
5c19869
to
74b4998
Compare
74b4998
to
abd86d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
This PR addresses a recent regression where if all bridges would be discarded before random selection, the tunnel parameters would still be generated. We should instead fail loudly and don't attempt to establish a tunnel in that case.
This change is