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

iOS: Extract network extension management into a separate file #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ailinykh
Copy link
Contributor

@ailinykh ailinykh commented Jan 2, 2023

This is the first step of the ViewController refactoring process described in #139

All the network extension stuff is now moved into a separate component and the ViewController's dependency graph changed from this

flowchart LR
    viewController --> UIViewController
    viewController --> UIButton
    viewController --> UIImageView
    viewController --> UILabel
    viewController --> UserDefaults
    viewController --> NotificationCenter
    viewController --> MMWormhole
    viewController --> NETunnelProviderManager
    viewController --> NWPathMonitor
    viewController --> NEVPNStatus

    subgraph User Iinterface
        UIButton
        UIImageView
        UILabel
        UIViewController
    end

    subgraph Infrastructure
        UserDefaults
        NotificationCenter
        MMWormhole
    end

    subgraph Networking
        NETunnelProviderManager
        NWPathMonitor
        NEVPNStatus
    end
Loading

to this:

flowchart RL
    viewController --> UIViewController
    viewController --> UIButton
    viewController --> UIImageView
    viewController --> UILabel
    viewController --> UserDefaults
    viewController --> NotificationCenter
    viewController --> MMWormhole

    TunnelManagerImpl --> NETunnelProviderManager
    TunnelManagerImpl --> NEVPNStatus
    TunnelManagerImpl -.-> TunnelManager["<TunnelManager>"]
    TunnelManagerDelegate --> TunnelManagerState
    viewController --> TunnelManager
    viewController --> TunnelManagerDelegate

    subgraph User Interface
        UIButton
        UIImageView
        UILabel
        UIViewController
    end

    subgraph Infrastructure
        UserDefaults
        NotificationCenter
        MMWormhole
    end

    subgraph Network Extension
        NETunnelProviderManager
        NWPathMonitor
        NEVPNStatus
    end

    subgraph NewNode
        TunnelManager
        TunnelManagerDelegate
        TunnelManagerState

        TunnelManagerImpl
    end
Loading

There are some benefits of this approach:

  • ViewController does not depend on NetworkExtension, the dependency inversion principle applied
  • TunnelManager can be shared between iOS/macOS platforms without any change
  • We can have multiple implementations for the TunnelManager protocol which allows us to focus on UI improvements on the simulator. Like so:
    private lazy var tunnelManager: TunnelManager = {
#if targetEnvironment(simulator)
        TunnelManagerSimulator(delegate: self)
#else
        TunnelManagerImpl(delegate: self)
#endif
    }()
  • No need to use NWPathMonitor since we can rely on NWConnection state
  • Fixed bug when the connection fails after another VPN enabled

@ghazel
Copy link
Collaborator

ghazel commented Jan 3, 2023

I'm very curious if NWConnection is really sufficient in place of NWPathMonitor. Path Monitor should notice changes independent of the the success of NWConnection. Which NWConnection are you referring to?

@ailinykh
Copy link
Contributor Author

ailinykh commented Jan 3, 2023

NEVPNConnection is more appropriate because it represents the state of your tunnel and the main app can handle all the state transitions while NWPathMonitor notifies you when the network changed and does not reflect the actual tunnel state

The NWPathMonitor is a more low-level API and does not depend on the NETunnelProviderManager's connection.

When NEVPNConnection is in a disconnected state the traffic goes directly (around the network extension) whether NewNode working or not

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.

2 participants