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

Implement feature indicators in daemon #6481

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

hulthe
Copy link
Contributor

@hulthe hulthe commented Jul 18, 2024

  • Add a GetFeatureIndicators gRPC call that get's the current set of active "features" that should be shown in the UI.
  • Extend the DaemonEvent gRPC stream with a FeatureIndicators variant that has the same info.

This change is Reviewable

Copy link

linear bot commented Jul 18, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch from a9b3239 to b60260d Compare July 24, 2024 09:41
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review July 24, 2024 09:56
@MarkusPettersson98 MarkusPettersson98 requested a review from Rawa July 24, 2024 11:23
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:

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


android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 226 at r1 (raw file):

                        ManagementInterface.DaemonEvent.EventCase.REMOVE_DEVICE -> {}
                        ManagementInterface.DaemonEvent.EventCase.EVENT_NOT_SET -> {}
                        ManagementInterface.DaemonEvent.EventCase.FEATURE_INDICATORS -> {}

:lgtm:

Comment on lines 10 to 23
pub struct FeatureIndicators {
pub active_features: HashSet<FeatureIndicator>,
}

Copy link
Contributor

@Serock3 Serock3 Jul 29, 2024

Choose a reason for hiding this comment

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

Maybe this should just be a tuple struct so we avoid having to name the only field? I.e. FeatureIndicators(HashSet<FeatureIndicator>).

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well tbh

mullvad-daemon/src/lib.rs Outdated Show resolved Hide resolved
@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch 4 times, most recently from 485d2f3 to 027f72a Compare July 30, 2024 09:19
@MarkusPettersson98 MarkusPettersson98 requested a review from Rawa July 30, 2024 10:41
@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch 2 times, most recently from f2a40b0 to b9ab1f8 Compare July 30, 2024 13:55
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

There seems to be no code for triggering a new TunnelState when a setting changes that affects feature indicators, but not the tunnelstate itself, e.g. lockdown mode. You likely still have to listen to changes to the settings to capture this.

Reviewed 8 of 11 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hulthe)


mullvad-cli/src/format.rs line 25 at r4 (raw file):

            endpoint,
            location,
            feature_indicators: _,

What do you think about printing the feature indicators as a part of the CLI?


mullvad-management-interface/src/client.rs line 54 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Dropped FeatureIndicators from the DaemonEvent enum, think it turned out better!

Great work! 😄

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch 2 times, most recently from 79442fa to afd0588 Compare July 31, 2024 13:41
Copy link
Contributor

@Serock3 Serock3 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 2 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hulthe)


mullvad-daemon/src/lib.rs line 1116 at r6 (raw file):

    /// Update the set of feature indicators.
    fn handle_feature_indicator_event(&self) {

Perhaps this function could take the new settings, convert them to feature indicators and only notify about a new tunnel state if they are different from new_indicators?

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch from afd0588 to ccf77ad Compare August 1, 2024 09:01
Copy link
Contributor

@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.

Done

Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @hulthe and @Serock3)

Copy link
Contributor

@Serock3 Serock3 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 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hulthe)


mullvad-daemon/src/lib.rs line 1123 at r7 (raw file):

            let new_feature_indicators = self.get_feature_indicators();
            if *current_feature_indicators != new_feature_indicators {
                // Make sure to update the daemon's actual tunnel state. Otherwise feature indicator changes won't be persisted.

Good catch!

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch from ccf77ad to b9a635c Compare August 1, 2024 11:56
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hulthe)

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch 2 times, most recently from dcb7067 to b8ac868 Compare August 1, 2024 12:34
Copy link
Contributor

@Serock3 Serock3 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 r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hulthe)

- Add a GetFeatureIndicators gRPC call that get's the current set of
  active "features" that should be shown in the UI.

- Extend the TunnelState with a FeatureIndicators value. Clients who
  listens for TunnelState events will get updates automatically.
@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-feature-list-in-rpc-des-1082 branch from b01d3d0 to 7992055 Compare August 1, 2024 13:27
@MarkusPettersson98 MarkusPettersson98 merged commit d567fe1 into main Aug 1, 2024
49 of 50 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the implement-feature-list-in-rpc-des-1082 branch August 1, 2024 13:40
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.

4 participants