YunoHost-Apps / vpnclient_ynh

VPN Client app for YunoHost
GNU Affero General Public License v3.0
41 stars 25 forks source link

Openvpn hooks #118

Closed hidrarga closed 9 months ago

hidrarga commented 10 months ago

Problem

As said in #116, I'm moving what's doing ynh-vpnclient service to OpenVPN hooks.

Solution

There are a lot of changes, sorry :/

I had to remove the yunohost firewall reload stuff because there are lock issues with it. Instead I'm adding / removing rules manually. This isn't too complex I think, and it's faster, so why not?

I'm also improving some stuff, such as the .ynh-vpnclient-started file which wasn't properly erased on error, or which wasn't checked by the ynh-vpnclient-checker timer…

I'm removing the ynh-vpnclient status, and I moved it's content inside the hooks. Honestly I don't even know if this used, or maybe for debugging purpose?

Finally, I changed the priority of the hook 20-set-ipv6 in order to run it after the firewall rules and the DNS configuration.

PR Status

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

hidrarga commented 10 months ago

!testme

yunohost-bot commented 10 months ago

Alrighty! Test Badge

hidrarga commented 10 months ago

For the firewall issue, the trick is to use

yunohost service stop ynh-vpnclient

I needed that for upgrading the app… But otherwise it's not needed since I'm adding the VPN rules manually, which is faster than reloading the firewall. Unless we don't want updating firewall rules that way?

alexAubin commented 10 months ago

I had to remove the yunohost firewall reload stuff because there are lock issues with it. Instead I'm adding / removing rules manually. This isn't too complex I think, and it's faster, so why not?

Sounds like an interesting twist ... I don't have enough insight to tell if there was a good reason for the initial design ... to me naively it sounds like using yunohost firewall reload had an "already implemented hook system" so it was somewhat practical to use it. If that's it, then your new proposed designed sounds indeed more efficient ?

ping @zamentur in case you have time and insight about this ...

zamentur commented 10 months ago

if i understand, the post-iptables hook is keeped in order to manage the case when the admin reload the firewall, and in more we have openvpn hooks. LGTM (it's a very quick review, i know, sadly i don't think i will have more time to review more)

Thanks to all people who work on this :)

hidrarga commented 9 months ago

!testme

yunohost-bot commented 9 months ago

:carousel_horse: Test Badge

hidrarga commented 9 months ago

!testme

yunohost-bot commented 9 months ago

Alrighty! Test Badge

hidrarga commented 9 months ago

!testme

yunohost-bot commented 9 months ago

Fingers crossed! Test Badge