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

NordVPN improvements | Change up and down script and add item to DietPi-Banner #3084

Merged
merged 22 commits into from
Sep 8, 2019
Merged

Conversation

svh1985
Copy link
Contributor

@svh1985 svh1985 commented Aug 30, 2019

Status: Ready

  • Change up and down to run on route change
  • Add NordVPN entry to DietPi-Banner
  • Add function to NordVPN to pass status to DietPi-Banner

Fixes: #3079

Commit list/description:

  • DietPi-Nordvpn | Changed up to route-up and down to route-pre-down. This will ensure that the scripts will only run after the VPN successfully connected.

Pull latest changes into fork
Changed up to route-up and down to route-pre-down. This will ensure that the scripts will only run after the VPN succesfully connected.
@svh1985 svh1985 changed the title DietPi-NordVPN | Change DietPi-NordVPN | Change up and down script to triggers Aug 30, 2019
First commit.
Add DietPi-NordVPN status to banner.
@svh1985
Copy link
Contributor Author

svh1985 commented Aug 30, 2019

@MichaIng I noticed that the order of the banner items is tied to the array number. How do we handle an item that is added in the middle of array? I thought the NordVPN item should live at index 7. But I guess this messes up the settings file once we push the upgraded DietPi-Banner no?

@svh1985 svh1985 changed the title DietPi-NordVPN | Change up and down script to triggers NordVPN improvements | Change up and down script and add item to DietPi-Banner Aug 30, 2019
Placeholder added in DietPi-Banner for NordVPN
Started work on a function in DietPi-NordVPN to get status from commandline. The retun now includes:
[  OK  ] DietPi-NordVPN | Root access verified.
[  OK  ] DietPi-NordVPN | RootFS R/W access verified.

Need advise to solve this.
Also set NORDVPN_SERVICE=1 manually because it does not sets the global var from the Init('status') function properly.

Banner item NordVPN calls DietPi-NordVPN to get status.
@MichaIng
Copy link
Owner

MichaIng commented Aug 31, 2019

@svh1985
Many thanks for this. Looks good, will test later.

Btw the order of the banner entries does not depend on the banner index, but just on the oder on the code. So no need to change existing indices :).

@MichaIng MichaIng added this to the v6.26 milestone Aug 31, 2019
@MichaIng MichaIng self-requested a review August 31, 2019 08:14
@svh1985
Copy link
Contributor Author

svh1985 commented Aug 31, 2019

Ok good to know, I’ll then have to correct the indexes.

Update to maintain compatibility.
@svh1985
Copy link
Contributor Author

svh1985 commented Sep 1, 2019

@MichaIng do you know the best way I can suppress the:
[ OK ] DietPi-NordVPN | Root access verified.
[ OK ] DietPi-NordVPN | RootFS R/W access verified.
It's now echoed in the banner.

Added service usage info that gets returned when wrong command has been passed as input.
Typo in usage info
@MichaIng
Copy link
Owner

MichaIng commented Sep 1, 2019

@svh1985
Totally forgot about this. Humm... One way would be to write status to a temporary file and read it back from banner. But then using dietpi-nordvpn status from console is somehow awkward.

Okay lets do it like this:

  • I anyway wanted to mute the root user and R/W check in case of success. IMO it is sufficient to do this silently and only print error/prompt if check failed.
  • For status info, neither root permissions nor R/W access are required, thus both checks should be moved to right before the menu loop (Config file read must be done as well after those checks). Otherwise currently, when logging in with non-root user, banner would still print or even prompt error.

I was also thinking about how to avoid doubled code (optional for now):

  • Connection state + in case RX/TX estimation can be done in a separate function. This function could produce related variables.
  • When calling G_INTERACTIVE=0 /DietPi/dietpi/misc/dietpi-nordvpn status, all whiptail dialogs are skipped, questions are answered with "no". With this, "status" does not require an own Init() argument, but it can simply echo from within the existing G_WHIP_YESNO "else" part.
  • By this, also the doubled NORDVPN_SERVICE check is obsolete, as long as Read_Settings is moved to in front of menu while loop.

I will remove the positive root user + r/w access prints now.

@MichaIng
Copy link
Owner

MichaIng commented Sep 2, 2019

@svh1985
Done: 1d25c2d
G_CHECKs succeed silently now.

Changed some status text and added colors to the status.
@svh1985
Copy link
Contributor Author

svh1985 commented Sep 2, 2019

@MichaIng thanks, it works like a charm. I also added colors to the status, what do you think?
image
image

@MichaIng
Copy link
Owner

MichaIng commented Sep 2, 2019

@svh1985
Beautiful!! Yeah the colour makes totally sense for a VPN, so users will recognise quickly, if VPN is up or down. This is especially important as long as we don't have a killswitch integrated. Another task, most likely for v6.27.

@svh1985
Copy link
Contributor Author

svh1985 commented Sep 3, 2019

@MichaIng Do you want to merge or shall we implement your suggestions first? When do you want to release v6.26?

Kill switch would be nice! The Down script with dietpi-service stop qbittorrent is not very stable. I get multiple dietpi-service instance errors, not sure what goes wrong there. qbittorrent takes a long time to quite when having an active transfer maybe a openvpn down script timeout kills the dietpi-service script.

@MichaIng
Copy link
Owner

MichaIng commented Sep 3, 2019

@svh1985

Do you want to merge or shall we implement your suggestions first?

I will have a look later, probably do some fine tuning and then merge.

The Down script with dietpi-service stop qbittorrent is not very stable.

Use systemctl stop qbittorrent instead. DietPi-Services is overkill here. It is mainly useful for either interactive calls where you need formatted output/error handling, or to start/stop all known services automatically in reasonable order, before maintenance tasks like installs, backups and such.

Just to verify:

time dietpi-services stop qbittorrent
G_USER_INPUTS=0 dietpi-services stop qbittorrent
  • These both work from console, right?
  • I wrapped the first into time so you can see how long it takes. Perhaps we can find out the timeout, if present, which would be good to know of course.

@svh1985
Copy link
Contributor Author

svh1985 commented Sep 3, 2019

I think that the OpenVPN down command does not want to wait for 1m30s and just kills the wait leaving dietpi-services in "not finished" state.

The commands you wanted me to run (directly from prompt):
First command:

root@RockPi:~# time dietpi-services stop qbittorrent

 DietPi-Services
─────────────────────────────────────────────────────
 Mode: stop qbittorrent

[  OK  ] DietPi-Services | stop : qbittorrent

real	1m30.368s
user	0m0.660s
sys	0m1.476s

Second:

root@RockPi:~# G_USER_INPUTS=0 dietpi-services stop qbittorrent

 DietPi-Services
─────────────────────────────────────────────────────
 Mode: stop qbittorrent

[  OK  ] DietPi-Services | stop : qbittorrent

@MichaIng
Copy link
Owner

MichaIng commented Sep 4, 2019

@svh1985
Hmm but this is not usual that qbittorrent takes 1.5 minutes to shutdown. Actually this is the timeout of the systemd unit, before it simply kills the process.
Do you have very much torrents active or something? Or does qbittorrent log contain some error message during shutdown/stopping it?

@svh1985
Copy link
Contributor Author

svh1985 commented Sep 4, 2019

@MichaIng Just one transfer. Ill check the log later and report it back once im home.

+ DietPi-NordVPN | Minor alignment
+ DietPi-NordVPN | DietPi scripts don't need to be executed with bash command, since they all contain a bash shebang.
+ DietPi-Banner | Fix output entry indices
+ DietPi-NordVPN | Do root user and rootfs R/W check only when menu is to be loaded, otherwise printing status would prompt error messages even that both is not required. This allows to load DietPi-Globals as well just for menu, which speeds up status print.
+ DietPi-NordVPN | Force non-interactive mode on status print, to skip any whiptail prompt
+ DietPi-NordVPN | Update WAN IP only on before first menu load, on connect, disconnect and refresh, to speed up all other navigation steps, which do not affect the connection state. This is due the WAN IP check can take up to two seconds, which adds a significant delay.
+ DietPi-NordVPN | Move RX/TX obtaining into an own function and remove DietPi-Globals dependency from it. This allows to share very most code between menu mode and status print.
+ DietPi-NordVPN | Further coding and wording enhancements
@MichaIng
Copy link
Owner

MichaIng commented Sep 5, 2019

@svh1985
I made some changes, so that root user and rootfs R/W check is not done for status print. Otherwise the banner would throw an error message when logging in as non-root user.

Then I could not hold myself to redo the NordVPN install, configuration and network state/info, so that most code is now shared between status print and menu mode. And did some other coding enhancements, that are not related to your PR. Perfectionist here 😅.

I hope you are okay with that and I did not destroy anything. Will run tests now to see if banner print works as before my changes (where it worked like a charm already, as root user), then from my side it is ready to merge. Is there anything you still want to do about it (cause status was still WIP)?


Two little fixes added, now testing passed from my side 👍.
EDIT: Ah nope, G_WHIP_YESNO is called before globals are loaded... hmm
EDIT2: Solved.

+ DietPi-NordVPN | Print connection test result to console as well, to stops prior process animation
+ DietPi-NordVPN | Print correct status text if service is not configured
+ DietPi-NordVPN | Now that globals are only loaded on menu mode, G_INTERACTIVE has no effect and G_WHIP_YESNO from Init() must only be called on menu mode.
+ DietPi-NordVPN | Tiny
+ DietPi-NordVPN | Resolve G_WHIP_YESNO being called before globals are loaded
@svh1985
Copy link
Contributor Author

svh1985 commented Sep 6, 2019

@MichaIng nice improvements! Everything worked during my testing. I'll just push one fix for a small menu alignment issue.

I have to admit I now miss the connection state "Connected" in the dietpi-nordvpn menu. Do you mind if we put it back? I can push it along with the menu fix. Then I think the PR is ready to be merged 🎉

Like this:
image

Misallignment in menu, changed usage to traffic and added the connected state again.
+ DietPi-NordVPN | 
+ DietPi-NordVPN |
@MichaIng
Copy link
Owner

MichaIng commented Sep 8, 2019

Accidental commit before filling its comment:

  • Moved systemd unit to /etc/systemd/system, since /lib/systemd/system should be reserved for distribution package manager (APT). A DietPi-Patch will be added to move units on existing installs.
    EDIT: 9e726b5
  • Fixed Check_Connected loop when applying settings, so that a check is done as well after waiting the last second. Currently when index reaches $MAX_WAIT_FOR_CONNECTION, it is waited for another second, but no check is done afterwards, which renders this last second a waste of time.
  • Reordered menu status text as discussed above.
  • Minor coding

nordvpn

@MichaIng MichaIng self-requested a review September 8, 2019 11:03
@MichaIng MichaIng merged commit 67a4ca9 into MichaIng:dev Sep 8, 2019
MichaIng added a commit that referenced this pull request Sep 8, 2019
+ DietPi-Patch | Move DietPi-NordVPN service to /etc/systemd/system: #3084
@MichaIng
Copy link
Owner

MichaIng commented Sep 8, 2019

Changelog entry: 5d0f793

@MichaIng MichaIng mentioned this pull request Sep 16, 2019
@MichaIng MichaIng mentioned this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants