tailscale-android icon indicating copy to clipboard operation
tailscale-android copied to clipboard

Implement USE_EXIT_NODE intent

Open 333fred opened this issue 1 year ago • 17 comments

Closes https://github.com/tailscale/tailscale/issues/8143. I map friendly labels from intent extras to tailscale node IDs, with empty string or not specifying the exitNode intent extra as the "no exit node" action. When an error is encountered, we will push a notification with a friendly message to the status notification channel. The tasker syntax I tested with locally is:

Action: com.tailscale.ipn.USE_EXIT_NODE Package: com.tailscale.ipn Class: com.tailscale.ipn.IPNReceiver Target: Broadcast Receiver Extra: exitNode:exitNodeLabelOrEmpty Extra: allowLanAccess:trueOrFalse

333fred avatar Dec 09 '23 20:12 333fred

@DentonGentry hey, sorry for the ping, just trying to get someone to look at this pr.

333fred avatar Dec 15 '23 16:12 333fred

I understand and do intend to. We are finishing the 1.56 client release this week. We would want something like this to go in at the start of a new development cycle to potentially appear in 1.58, we need to finish 1.56 first.

DentonGentry avatar Dec 15 '23 16:12 DentonGentry

I understand and do intend to. We are finishing the 1.56 client release this week. We would want something like this to go in at the start of a new development cycle to potentially appear in 1.58, we need to finish 1.56 first.

Perfectly fine. I just hadn't heard anything so I was making sure this was on the radar 🙂.

333fred avatar Dec 15 '23 18:12 333fred

Hey @DentonGentry, no pressure intended, but do you have an idea when you'll be taking a look at this?

333fred avatar Jan 16 '24 23:01 333fred

I hope that the pull request will be merged soon. It's a feature I'm eagerly awaiting. I use MacroDroid for automations, depending on wifi connection etc. At home, "Use exit node" should be off, but on the road, please turn it on.

Linutux avatar Jan 31 '24 19:01 Linutux

@DentonGentry @kari-ts, any status updates?

333fred avatar Feb 20 '24 16:02 333fred

@DentonGentry @kari-ts, I'm trying to be patient here, but the radio silence is really starting to get frustrating. Can I please at least get an idea of when you'll look at this? I don't want to keep resolving merge conflicts if this is just never going to be looked at.

333fred avatar Feb 28 '24 17:02 333fred

I'm personally dying for this feature. Can it please get added asap?

nuentes avatar Feb 28 '24 17:02 nuentes

@333fred thanks for the work here.

I'm making some changes that impact exit nodes now (https://github.com/tailscale/tailscale-android/pull/172, https://github.com/tailscale/tailscale-android/pull/176) - right now, whenever we want to make any changes to prefs, we call SetPrefs, which is intended for setting prefs for the first time, rather than editing the prefs. I also would like to prevent users from being able to use an exit node if they're currently running an exit node, and vice versa.

Once we switch to using EditPrefs instead of SetPrefs, if a user is using an exit node and tries to run an exit node or vice versa, we will fail this check. For this PR, this error should be handled and some UI telling the user why the action failed should be shown.

kari-ts avatar Feb 28 '24 18:02 kari-ts

I see. These intents are normally run in the background, while tailscale isn't open, or indeed while the phone screen isn't even on; I'm not sure what UI would be best to try and show. Either toasts or notifications are likely options here. Perhaps a notification would be best, as toasts can easily be missed?

333fred avatar Feb 28 '24 18:02 333fred

Adding @sonovawolf for UI input

kari-ts avatar Feb 28 '24 18:02 kari-ts

@sonovawolf thoughts on the UI suggestions? I won't have time to work on the changes this weekend, but may be able to get to it some evening next week if I have a direction to go.

333fred avatar Mar 01 '24 16:03 333fred

Hey @333fred! We'd prefer to use notifications to communicate those errors.

sonovawolf avatar Mar 04 '24 22:03 sonovawolf

@kari-ts I've had time to address the notification feedback; given the number of changes to the app since I initially submitted this PR, I simply rewrote the UseExitNodeWorker in Kotlin and force-pushed (I was also missing the Signed-off-by in my commits). This is ready for review.

333fred avatar May 03 '24 05:05 333fred

@agottardo strings extracted.

333fred avatar May 04 '24 01:05 333fred

Thanks for the reviews! Anything else you need from me before merging this?

333fred avatar May 06 '24 15:05 333fred

Thanks for the reviews! Anything else you need from me before merging this?

@333fred we'll take care of merging this as soon as QA has signed off on the 1.66 release, to land this PR in the next beta version (and ultimately the 1.68 release). 👍🏻

agottardo avatar May 06 '24 16:05 agottardo

@333fred sorry to bother you, but we had to refactor quite a few files to fix some last-minute bugs for the release. Can you please rebase this fixing the conflicts, and I'll get it merged? Thanks.

agottardo avatar May 16 '24 21:05 agottardo

Sorry, I've been getting ready for vacation and am now actually on vacation, so it'll be a bit before I can get to a real computer to rebase.

333fred avatar May 25 '24 23:05 333fred

@agottardo I'm back from vacation and was able to rebase this. As a general piece of feedback, since the contribution guidelines at this point are "we're working on it", trying to get this in has not been a good experience. I would really like to suggest a branching strategy that allows you to work on features for future versions without having to wait for stabilization of the current version; I'm waiting with a bit of dread for a month from now when you tell me that you're ready to merge my PR, but the underlying architecture has been refactored again and I need to do another rewrite of things, as I've needed to do twice now (once majorly with the app rewrite, and once minorly with the notification and app lifetime changes you made in the past month since I updated my PR again).

333fred avatar Jun 11 '24 03:06 333fred

Thank you @333fred !

barnstar avatar Jun 12 '24 20:06 barnstar

Thanks @barnstar, @agottardo, and @kari-ts! Glad to see this finally make it in!

333fred avatar Jun 12 '24 21:06 333fred