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

Do not fallback to making direct API connections when testing access methods #7091

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Oct 28, 2024

This PR addresses an issue in the logic for testing access methods whre if an access methods fails to resolve to a set of connection details (e.g. if the relay selector / mullvad encrypted dns cache is empty) the daemon would fall back to making a direct API connection. The current behaviour is straight up misleading.


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 force-pushed the do-not-fallback-to-direct-when-testing-edp branch from a41953d to a04a016 Compare October 28, 2024 19:19
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


mullvad-daemon/src/api.rs line 183 at r1 (raw file):
nit:

Tru to resolve...

Typo?

Rawa
Rawa previously approved these changes Oct 29, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

LGTM, probably good if Rust dev takes a look as well 👍

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


mullvad-daemon/src/api.rs line 183 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

nit:

Tru to resolve...

Typo?

Type!

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


mullvad-daemon/src/api.rs line 183 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Type!

Typo* Lol

Copy link
Contributor

@hulthe hulthe left a 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, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98 and @Rawa)


mullvad-daemon/src/api.rs line 604 at r1 (raw file):

                AccessMethod::BuiltIn(BuiltInAccessMethod::Bridge) => {
                    let Some(bridge) = relay_selector.get_bridge_forced() else {
                        log::warn!("Received unexpected proxy settings type. Defaulting to direct API connection");

Is "Defaulting to direct API connection" still accurate?

Copy link
Contributor

@Rawa Rawa left a 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 r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

Rawa
Rawa previously approved these changes Oct 30, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

@MarkusPettersson98 MarkusPettersson98 force-pushed the do-not-fallback-to-direct-when-testing-edp branch 2 times, most recently from 4e3d05e to 24d2184 Compare October 30, 2024 11:47
Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 7 files reviewed, 1 unresolved discussion (waiting on @hulthe and @Rawa)


mullvad-daemon/src/api.rs line 604 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Is "Defaulting to direct API connection" still accurate?

Well spotted, this log message is outdated 😊
Fixed!

@MarkusPettersson98 MarkusPettersson98 force-pushed the do-not-fallback-to-direct-when-testing-edp branch 2 times, most recently from de723fd to 5ceb991 Compare October 31, 2024 15:22
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@MarkusPettersson98 MarkusPettersson98 force-pushed the do-not-fallback-to-direct-when-testing-edp branch from 5ceb991 to 4e57719 Compare October 31, 2024 16:17
@MarkusPettersson98 MarkusPettersson98 merged commit 76d5eb6 into main Oct 31, 2024
60 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the do-not-fallback-to-direct-when-testing-edp branch October 31, 2024 17:05
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