Bug#981458: vtun systemd service

2023-07-04 Thread Andreas Henriksson
Hello again,

Fear we're sliding off-topic here, but will try to follow up anyway
in case my input is in any way helpful.

On Tue, Jul 04, 2023 at 12:08:36PM +0200, Francesco P. Lovergine wrote:
> First of all, sorry for the use of irritual instead of weird (false friend 
> term applies
> in this case, for non native speakers).

I'm not a native speaker myself.

> 
> About the discouraging of EnvironmentFile could you please point where
> it is expressed in the Policy?

By Policy I assume you mean Debian Policy?

My most recent interaction with policy was to to remove policys very
detailed description of how sysvinit worked which was forbidding
things like startpar and insserv, which has been mandatory for decades
in Debian under sysvinit. By then there wasn't a single mention of
systemd. Maybe things have changed in the last couple of years, but
I would definitely not take Debian Policy as a authoritative source
for any systemd related recommendations.

> For sure, Debian has impressive use of the /etc/default/ tree
> which was and still is Debian specific.

The /etc/default pattern is for sure both Debian and sysvinit specific.
The same thing exists as /etc/sysconfig in RH/RPM world.
I guess unifying this was never useful since init scripts where always
very distro specific anyway.

Notably /etc/default/foo is posix shell, while EnvironmentFile= takes
a key=value file (without executing it in shell context).

This means that for example if /etc/default/foo contains:
```
UNAME="`uname -a`"
```

You would get different results with the init script pattern vs
systemd EnvironmentFile=.


> That is probably the origin of those rumors.
> 
> Indeed, enabling/disabling of services by using an option in /etc/default/ 
> (as for a lots
> of services in the past) is considered a bad practice due to the old init
> sysv days. Today, one should enable/disable the unit instead, which is much
> more clean. That make sense.

Now you are talking about the infamous NOSTART anti-pattern, which is a
different problem and should for sure not be used (unrelated ot which
init is used).

> I disagree with a general deprecation
> of Environment entries instead (files or vars), which is optimal mode of 
> solving
> configuration issues without writing whole units or overrides. But on those 
> regards,
> using a non-templated unit as a pseudo-templated is a very strange choice.
> 
> Anyway it is your choice.

For sure not my choice. I'm just providing something in the hope that
things will move forward as they seem to have been stagnant for this
package up until this day.

Regards,
Andreas Henriksson



Bug#981458: vtun systemd service

2023-07-04 Thread Francesco P. Lovergine

On Tue, Jul 04, 2023 at 11:24:21AM +0200, Andreas Henriksson wrote:

Hello Francesco P. Lovergine,

Thanks for your feedback on this!

On Mon, Jul 03, 2023 at 06:17:20PM +0200, Francesco P. Lovergine wrote:

Uhm, it seems to me quite irritual using a template unit file without a 
template variable. Which reflects
the quite strange use of /etc/default/vtun with multiple indexed vars,
instead of multiple configuration files such as:

/etc/vtun.d/config?.vars (or even under /etc/vtun if you prefer so)

and of course you can override the env variables by using an
/etc/vtun.d/%i.vars

where it makes sense in the template file. I think this would be the right 
moment to convert the
insane limited number of env var sets in /etc/default/vtun into multiple 
ordinary configuration
files and using something like that.

EnvironmentFile=-/etc/vtun.d/%i.vars

would override name, host and args variables.

I'm missing something?


While I atleast partially agree with your initial sentence, I'm not
onboard with your suggested solution.
In my understanding use of EnvironmentFile= is discuraged (and if I'm
not mistaken I've even read statements saying it was a mistake to ever
add it as an option).

It seems to me like you're bending over backwards trying to invent
something that actually needs the instance variable.

(I'm however fine with anything that gets things moving forward of using
native units instead of init script. I'm also not even a user of this
package/program as previously stated, so it affects me very little.
Use what ever solution you find acceptable!)

Regards,
Andreas Henriksson



First of all, sorry for the use of irritual instead of weird (false friend term 
applies
in this case, for non native speakers). 


About the discouraging of EnvironmentFile could you please point where
it is expressed in the Policy? For sure, Debian has impressive use of the 
/etc/default/ tree
which was and still is Debian specific. That is probably the origin of those 
rumors.

Indeed, enabling/disabling of services by using an option in /etc/default/ (as 
for a lots
of services in the past) is considered a bad practice due to the old init sysv days. 
Today, one should enable/disable the unit instead, which is much more clean. That make sense. 
I disagree with a general deprecation

of Environment entries instead (files or vars), which is optimal mode of solving
configuration issues without writing whole units or overrides. But on those 
regards,
using a non-templated unit as a pseudo-templated is a very strange choice.

Anyway it is your choice.

--
Francesco P. Lovergine



Bug#981458: vtun systemd service

2023-07-04 Thread Andreas Henriksson
Hello Francesco P. Lovergine,

Thanks for your feedback on this!

On Mon, Jul 03, 2023 at 06:17:20PM +0200, Francesco P. Lovergine wrote:
> Uhm, it seems to me quite irritual using a template unit file without a 
> template variable. Which reflects
> the quite strange use of /etc/default/vtun with multiple indexed vars,
> instead of multiple configuration files such as:
> 
> /etc/vtun.d/config?.vars (or even under /etc/vtun if you prefer so)
> 
> and of course you can override the env variables by using an
> /etc/vtun.d/%i.vars
> 
> where it makes sense in the template file. I think this would be the right 
> moment to convert the
> insane limited number of env var sets in /etc/default/vtun into multiple 
> ordinary configuration
> files and using something like that.
> 
> EnvironmentFile=-/etc/vtun.d/%i.vars
> 
> would override name, host and args variables.
> 
> I'm missing something?

While I atleast partially agree with your initial sentence, I'm not
onboard with your suggested solution.
In my understanding use of EnvironmentFile= is discuraged (and if I'm
not mistaken I've even read statements saying it was a mistake to ever
add it as an option).

It seems to me like you're bending over backwards trying to invent
something that actually needs the instance variable.

(I'm however fine with anything that gets things moving forward of using
native units instead of init script. I'm also not even a user of this
package/program as previously stated, so it affects me very little.
Use what ever solution you find acceptable!)

Regards,
Andreas Henriksson



Bug#981458: vtun systemd service

2023-07-03 Thread Francesco P. Lovergine

On Mon, Jul 03, 2023 at 04:03:47PM +0200, Andreas Henriksson wrote:

Control: forcemerge -1 1039413
Control: severity -1 important
Control: retitle -1 vtun: please provide vtun@.service and mask init script

Hello,

I'm attaching a patch for the vtun debian package that should hopefully
be a good start in migrating to native systemd units.
The patch is COMPLETELY UNTESTED as I'm not myself a user of vtun.
Please make sure to set FIRST_SYSTEMD_SERVICE_VERSION to the correct
debian package version including these changes.

The attached patch adds a vtun@.service as the init script seems to have
used a home-brew template units style.
The /etc/default/vtun CLIENT$i_* variables are broken up into separate
vtun@.service instances, eg CLIENT1_NAME, CLIENT1_HOST, CLIENT1_ARGS
goes to vtun@CLIENT1.service override as NAME, HOST and OPTIONS.
This is done as a one-time migration
(This also lifts the restrictions of having 0-9 instances.)

You most likely also want to make sure vtun.service (without the @)
is a symlink to /dev/null, to mask the init script.


See also:

https://src.fedoraproject.org/rpms/vtun/tree/81e15b3a03b89bffe0e6076a235720d40f343292

You might also want to provide the socket unit

Regards,
Andreas Henriksson



diff '--color=auto' -uriNp vtun-3.0.4/debian/postinst 
vtun-3.0.4.systemd/debian/postinst
--- vtun-3.0.4/debian/postinst  2019-11-11 01:17:37.0 +0100
+++ vtun-3.0.4.systemd/debian/postinst  2023-07-03 15:47:08.717223223 +0200
@@ -46,6 +46,36 @@ case "$1" in
;;
esac

+# migrate old init.d style settings to systemd
+FIRST_SYSTEMD_SERVICE_VERSION="3.0.4-2.1"
+if [ "$1" = "configure" ] && dpkg --compare-versions "$2" lt-nl 
"$FIRST_SYSTEMD_SERVICE_VERSION~" ; then
+(
+echo "Checking if we need to migrate /etc/default/vtun settings to 
'vtun@.service'."
+if [ -e /etc/default/vtun ]; then
+. /etc/default/vtun
+fi
+
+for i in 0 1 2 3 4 5 6 7 8 9; do
+eval name=\$CLIENT${i}_NAME
+eval host=\$CLIENT${i}_HOST
+eval args=\$CLIENT${i}_ARGS
+if [ -n "$name" ] && [ -n "$host" ]; then
+echo "One-time migration of vtun CLIENT$i settings to 
vtun@CLIENT$i.service"
+mkdir -p "/etc/systemd/system/vtun@CLIENT$i.service.d/"
+echo "[Service]" >> 
"/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"NAME=$name\"" >> 
"/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"HOST=$host\"" >> 
"/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"OPTIONS=$args\"" >> 
"/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+if [ -n "${RUN_SERVER:-}" ] && [ "${RUN_SERVER:-}" != "no" ]; 
then
+deb-systemd-helper enable "vtun@CLIENT$i"
+fi
+fi
+
+done
+)
+fi
+
+
# dh_installdeb will replace this with shell code automatically
# generated by other debhelper scripts.

diff '--color=auto' -uriNp vtun-3.0.4/debian/vtun@.service 
vtun-3.0.4.systemd/debian/vtun@.service
--- vtun-3.0.4/debian/vtun@.service 1970-01-01 01:00:00.0 +0100
+++ vtun-3.0.4.systemd/debian/vtun@.service 2023-07-03 15:23:25.513183684 
+0200
@@ -0,0 +1,12 @@
+[Unit]
+Description=Virtual Tunnels over TCP/IP networks
+After=network.target
+
+[Service]
+ExecStart=/usr/sbin/vtund -n $OPTIONS $NAME $HOST
+# Reload should be synchronous, but signals are not...
+ExecReload=/usr/bin/kill -HUP $MAINPID
+Restart=on-failure
+
+[Install]
+WantedBy=multi-user.target



Uhm, it seems to me quite irritual using a template unit file without a 
template variable. Which reflects
the quite strange use of /etc/default/vtun with multiple indexed vars,
instead of multiple configuration files such as:

/etc/vtun.d/config?.vars (or even under /etc/vtun if you prefer so)

and of course you can override the env variables by using an /etc/vtun.d/%i.vars 


where it makes sense in the template file. I think this would be the right 
moment to convert the
insane limited number of env var sets in /etc/default/vtun into multiple 
ordinary configuration
files and using something like that.

EnvironmentFile=-/etc/vtun.d/%i.vars

would override name, host and args variables.

I'm missing something?


--
Francesco P. Lovergine



Bug#981458: vtun systemd service

2023-07-03 Thread Andreas Henriksson
Control: forcemerge -1 1039413
Control: severity -1 important
Control: retitle -1 vtun: please provide vtun@.service and mask init script

Hello,

I'm attaching a patch for the vtun debian package that should hopefully
be a good start in migrating to native systemd units.
The patch is COMPLETELY UNTESTED as I'm not myself a user of vtun.
Please make sure to set FIRST_SYSTEMD_SERVICE_VERSION to the correct
debian package version including these changes.

The attached patch adds a vtun@.service as the init script seems to have
used a home-brew template units style.
The /etc/default/vtun CLIENT$i_* variables are broken up into separate
vtun@.service instances, eg CLIENT1_NAME, CLIENT1_HOST, CLIENT1_ARGS
goes to vtun@CLIENT1.service override as NAME, HOST and OPTIONS.
This is done as a one-time migration
(This also lifts the restrictions of having 0-9 instances.)

You most likely also want to make sure vtun.service (without the @)
is a symlink to /dev/null, to mask the init script.


See also:

https://src.fedoraproject.org/rpms/vtun/tree/81e15b3a03b89bffe0e6076a235720d40f343292

You might also want to provide the socket unit

Regards,
Andreas Henriksson
diff '--color=auto' -uriNp vtun-3.0.4/debian/postinst vtun-3.0.4.systemd/debian/postinst
--- vtun-3.0.4/debian/postinst	2019-11-11 01:17:37.0 +0100
+++ vtun-3.0.4.systemd/debian/postinst	2023-07-03 15:47:08.717223223 +0200
@@ -46,6 +46,36 @@ case "$1" in
 ;;
 esac
 
+# migrate old init.d style settings to systemd
+FIRST_SYSTEMD_SERVICE_VERSION="3.0.4-2.1"
+if [ "$1" = "configure" ] && dpkg --compare-versions "$2" lt-nl "$FIRST_SYSTEMD_SERVICE_VERSION~" ; then
+(
+echo "Checking if we need to migrate /etc/default/vtun settings to 'vtun@.service'."
+if [ -e /etc/default/vtun ]; then
+. /etc/default/vtun
+fi
+
+for i in 0 1 2 3 4 5 6 7 8 9; do
+eval name=\$CLIENT${i}_NAME
+eval host=\$CLIENT${i}_HOST
+eval args=\$CLIENT${i}_ARGS
+if [ -n "$name" ] && [ -n "$host" ]; then
+echo "One-time migration of vtun CLIENT$i settings to vtun@CLIENT$i.service"
+mkdir -p "/etc/systemd/system/vtun@CLIENT$i.service.d/"
+echo "[Service]" >> "/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"NAME=$name\"" >> "/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"HOST=$host\"" >> "/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+echo "Environment=\"OPTIONS=$args\"" >> "/etc/systemd/system/vtun@CLIENT$i.service.d/override.conf"
+if [ -n "${RUN_SERVER:-}" ] && [ "${RUN_SERVER:-}" != "no" ]; then
+deb-systemd-helper enable "vtun@CLIENT$i"
+fi
+fi
+
+done
+)
+fi
+
+
 # dh_installdeb will replace this with shell code automatically
 # generated by other debhelper scripts.
 
diff '--color=auto' -uriNp vtun-3.0.4/debian/vtun@.service vtun-3.0.4.systemd/debian/vtun@.service
--- vtun-3.0.4/debian/vtun@.service	1970-01-01 01:00:00.0 +0100
+++ vtun-3.0.4.systemd/debian/vtun@.service	2023-07-03 15:23:25.513183684 +0200
@@ -0,0 +1,12 @@
+[Unit]
+Description=Virtual Tunnels over TCP/IP networks
+After=network.target
+
+[Service]
+ExecStart=/usr/sbin/vtund -n $OPTIONS $NAME $HOST
+# Reload should be synchronous, but signals are not...
+ExecReload=/usr/bin/kill -HUP $MAINPID
+Restart=on-failure
+
+[Install]
+WantedBy=multi-user.target