Merge ~ahasenack/ubuntu/+source/nfs-utils:disco-fix-rpc.svcgssd-args-1616123 into ubuntu/+source/nfs-utils:ubuntu/devel

[Andreas Hasenack]

There is a mismatch between a variable name defined in /etc/default/nfs-kernel-server and the systemd service file that uses it: the config file sets RPCSVCGSSDOPTS, and the systemd service file expects SVCGSSDOPTS (sans the RPC prefix).

/etc/default/nfs-kernel-server:
RPCSVCGSSDOPTS

nfs-utils_env.sh: generates the config file sourced by rpc-svcgssd.service:
"export RPCSVCGSSDARGS=$RPCSVCGSSDOPTS" and others into /run/sysconfig/nfs-utils

rpc-svcgssd.service:
sources /run/sysconfig/nfs-utils and runs /usr/sbin/rpc.svcgssd $SVCGSSDARGS

The .service file comes from the upstream tarball, whereas nfs-utils_env.sh comes from debian.

The best place to fix this is in the nfs-utils_env.sh script.

PPA with test packages: https://launchpad.net/~ahasenack/+archive/ubuntu/nfs-utils-rpc-svcgssd-var-name-1614261/

TESTING (to be added to the bug once I start the SRU)
* install nfs-server and a kerberos server. Use "EXAMPLE.LOCAL" for the realm,
  and "localhost" for the servers, when prompted:
sudo apt install nfs-server krb5-kdc krb5-user krb5-admin-server

* create the EXAMPLE.LOCAL realm. Use any password you want for the database
  master key, it won't be requested again:
sudo krb5_newrealm

* create a principal for the nfs service:
sudo kadmin.local -q "addprinc -randkey nfs/$(hostname -f)"

* extract the key into the system wide keytab:
sudo kadmin.local -q "ktadd -k /etc/krb5.keytab nfs/$(hostname -f)"

* edit /etc/default/nfs-common and enable gssd:
NEED_GSSD=y

* edit /etc/default/nfs-kernel-server and add an option to RPCSVCGSSDOPTS:
RPCSVCGSSDOPTS="-v"

* restart nfs-server
sudo systemctl restart nfs-server

* verify if /run/sysconfig/nfs-utils has the option we added above:
$ cat /run/sysconfig/nfs-utils
PIPEFS_MOUNTPOINT=/run/rpc_pipefs
RPCNFSDARGS=" 8"
RPCMOUNTDARGS="--manage-gids"
STATDARGS=""
RPCSVCGSSDARGS="-v"

* Verify the running rpc.gssd process. Without the fix, it won't have the "-v" option:
ps axw|grep svcgssd|grep -v grep
 4285 ? Ss 0:00 /usr/sbin/rpc.svcgssd

With the fix, right after installing the udpated packages, the option we added
to /etc/default/nfs-kernel-server will show up:
ps axw|grep svcgssd|grep -v grep
 5656 ? Ss 0:00 /usr/sbin/rpc.svcgssd -v

[Christian Ehrhardt  (paelzer)]

The change itself seems fine, there is no changelog change thou ?!

In addition I have seen that you have tagged the bug for SRUs.
I've had such cases in the past and just wanted to share some lessons learned.

By default NEED_SVCGSSD= is "no" so it doesn't even start. And even if it is "yes" then RPCSVCGSSDOPTS= is empty to it would make no difference. That is good so all of it only matters for a small amount of people.

But if somebody played with the RPCSVCGSSDOPTS= option and left it in any state that is not "" then this SRU will suddenly change behavior for him.
I know that is exactly the fix you want to achieve, but just saying that for "regression potential" this has to be considered. You know better than I do what people usually put in there and if that might be a problem.

One thing that can be done is to parse the file on update and warn about changing behavior if it will be triggered (but people unfortuantely ignore such messages, so it is barely worth).

I checked and as you have already mentioned (it is generated) /etc/default/nfs-kernel-server is no conffile. So I'm wondering how this will behave on an upgrade - is it only generated "once" on install - if so the fix will only help new installs (which is bad) but in return will not break existing setups (which is good).
The install path with the fix seems to be great and fixed, but I haven't looked deeper into the "how does this work on upgrades". Fortunately rbasak has grabbed the full review - so I'll leave that part for him :-P

review: Needs Fixing

[Andreas Hasenack (ahasenack)]

> The change itself seems fine, there is no changelog change thou ?!
>
> In addition I have seen that you have tagged the bug for SRUs.
> I've had such cases in the past and just wanted to share some lessons learned.
>
> By default NEED_SVCGSSD= is "no" so it doesn't even start. And even if it is
> "yes" then RPCSVCGSSDOPTS= is empty to it would make no difference. That is
> good so all of it only matters for a small amount of people.
>
> But if somebody played with the RPCSVCGSSDOPTS= option and left it in any
> state that is not "" then this SRU will suddenly change behavior for him.
> I know that is exactly the fix you want to achieve, but just saying that for
> "regression potential" this has to be considered. You know better than I do
> what people usually put in there and if that might be a problem.

Will do it in the sru template.

> One thing that can be done is to parse the file on update and warn about
> changing behavior if it will be triggered (but people unfortuantely ignore
> such messages, so it is barely worth).

Probably not worth it.

> I checked and as you have already mentioned (it is generated) /etc/default
> /nfs-kernel-server is no conffile. So I'm wondering how this will behave on an

The file that is generated is /run/sysconfig/nfs-utils, and the systemd service files source that one (instead of the ones in /etc/default). This is upstream even.

> upgrade - is it only generated "once" on install - if so the fix will only
> help new installs (which is bad) but in return will not break existing setups
> (which is good).
> The install path with the fix seems to be great and fixed, but I haven't
> looked deeper into the "how does this work on upgrades". Fortunately rbasak
> has grabbed the full review - so I'll leave that part for him :-P

People bitten by this bug could have fixed it in a few ways.

a) change the rpc-svcgssd systemd service file to reference RPCSVCGSSDARGS. When they upgrade, the new systemd service file will reference the correct variable (SVCGSSDARGS). No dpkg prompt, since this is not a config file
b) same as (a), but via a systemd override file. I think these will be broken, because no RPCSVCGSSDARGS variable will exist anymore. To cope with this, we could export both variables in debian/nfs-utils_env.sh. I actually had that before, but thought it wasn't necessary
c) people who changed /etc/default/nfs-kernel-server will have found out that it doesn't fix the bug unless they also changed nfs-utils_env.sh

So (a) works, (b) can be coped with if we want to, and (c) are SOL.

[Robie Basak (racb)]

Thank you for looking at this!

My general feeling is that this has the potential to cause upgrade path users depending on the choices we make. To minimise that, I'm wondering whether we can get the bug fixed in Debian first, to avoid a potential extra upgrade path problem when we try to remove our delta later if Debian doesn't take our patch as-is.

This isn't a hard -1; I'm open to further discussion (or if other Ubuntu devs think otherwise, go ahead and upload). I see no problem with the fix itself apart from potential multiple present and collisions with existing user configurations.

[Andreas Hasenack (ahasenack)]

Summarizing our IRC conversation, I will implement (b), submit that to salsa, and refer to it in the debian bug.

[Andreas Hasenack (ahasenack)]

Debian PR: https://salsa.debian.org/debian/nfs-utils/merge_requests/2

[Andreas Hasenack (ahasenack)]

Testing upgrade scenarios, assuming the user worked around the problem by different means.

a) user replaced SVCGSSDARGS in /lib/systemd/system/rpc-svcgssd.service with RPCSVCGSSDARGS:

prior to the upgrade (just showing the relevant lines for brevity)

ubuntu@ubuntu:~$ systemctl cat rpc-svcgssd.service
# /lib/systemd/system/rpc-svcgssd.service
(...)
ExecStart=/usr/sbin/rpc.svcgssd $RPCSVCGSSDARGS <---- workaround

Upgrading:
...
Setting up nfs-kernel-server (1:1.3.4-2.3ubuntu2~ppa3) ...
Processing triggers for man-db (2.8.5-2) ...
Processing triggers for systemd (240-6ubuntu2) ...

Service:
ubuntu@ubuntu:~$ systemctl cat rpc-svcgssd.service
# /lib/systemd/system/rpc-svcgssd.service
(...)
ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS <--- workaround gone

Argument preserved:
ubuntu@ubuntu:~$ ps axw|grep svcgssd|grep -v grep
10402 ? Ss 0:00 /usr/sbin/rpc.svcgssd -v

b) user used a proper systemd override file, like this:
root@ubuntu:~# systemctl cat rpc-svcgssd.service
# /lib/systemd/system/rpc-svcgssd.service
(...)
ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS <--- original service file

# /etc/systemd/system/rpc-svcgssd.service.d/override.conf <---- override
[Service]
ExecStart=
ExecStart=/usr/sbin/rpc.svcgssd $RPCSVCGSSDARGS <---- workaround

Workaround is working as expected:
ubuntu@ubuntu:~$ ps axw|grep svcgssd|grep -v grep
  520 ? Ss 0:00 /usr/sbin/rpc.svcgssd -v

Now we upgrade. Right after the upgrade, systemctl cat output is the same:
ubuntu@ubuntu:~$ systemctl cat rpc-svcgssd.service
# /lib/systemd/system/rpc-svcgssd.service
(...)

ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS

# /etc/systemd/system/rpc-svcgssd.service.d/override.conf
[Service]
ExecStart=
ExecStart=/usr/sbin/rpc.svcgssd $RPCSVCGSSDARGS

Process still has the "-v" argument:
ubuntu@ubuntu:~$ ps axw|grep svcgssd|grep -v grep
 1651 ? Ss 0:00 /usr/sbin/rpc.svcgssd -v

We can now delete the override even:
ubuntu@ubuntu:~$ sudo rm /etc/systemd/system/rpc-svcgssd.service.d/override.conf
ubuntu@ubuntu:~$ sudo systemctl daemon-reload
ubuntu@ubuntu:~$ systemctl cat rpc-svcgssd.service
# /lib/systemd/system/rpc-svcgssd.service
(...)
ExecStart=/usr/sbin/rpc.svcgssd $SVCGSSDARGS

Restart, and the process still has the "-v" option:
ubuntu@ubuntu:~$ sudo systemctl restart nfs-server
ubuntu@ubuntu:~$ ps axw|grep svcgssd|grep -v grep
 1852 ? Ss 0:00 /usr/sbin/rpc.svcgssd -v

Upgrade paths (a) and (b) work correctly.

[Christian Ehrhardt  (paelzer)]

Thanks for implementing the compat as well, I like that outcome of our discussions.
I'm re-reviewing the change itself now ...

[Christian Ehrhardt  (paelzer)]

Yeah, a minor comment for the changelog, but it LGTM
+1

review: Approve

[Andreas Hasenack (ahasenack)]

> Yeah, a minor comment for the changelog, but it LGTM
> +1

You are right, that is too much info. I was afraid of losing context, but then I remembered the actual commit fixing the bug adds a good deal of commentary, so we are good.

[Andreas Hasenack (ahasenack)]

Thanks, tagged and uploaded:

$ git push pkg upload/1%1.3.4-2.3ubuntu2
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 4 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.48 KiB | 101.00 KiB/s, done.
Total 9 (delta 6), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/nfs-utils
 * [new tag] upload/1%1.3.4-2.3ubuntu2 -> upload/1%1.3.4-2.3ubuntu2

$ dput ubuntu ../nfs-utils_1.3.4-2.3ubuntu2_source.changes
Checking signature on .changes
gpg: ../nfs-utils_1.3.4-2.3ubuntu2_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../nfs-utils_1.3.4-2.3ubuntu2.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading nfs-utils_1.3.4-2.3ubuntu2.dsc: done.
  Uploading nfs-utils_1.3.4-2.3ubuntu2.debian.tar.xz: done.
  Uploading nfs-utils_1.3.4-2.3ubuntu2_source.buildinfo: done.
  Uploading nfs-utils_1.3.4-2.3ubuntu2_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index ebf3f67..bc8f74d 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+nfs-utils (1:1.3.4-2.3ubuntu2) disco; urgency=medium
+
+  * d/nfs-utils_env.sh: alongside RPCSVCGSSDARGS, also export SVCGSSDARGS,
+    which is the variable name expected by the rpc-svcgssd systemd service.
+    (LP: #1616123)
+
+ -- Andreas Hasenack <[e-mail]>  Mon, 25 Mar 2019 09:24:29 -0300
+
 nfs-utils (1:1.3.4-2.3ubuntu1) disco; urgency=low
 
   * Merge from Debian unstable.  Remaining changes:
diff --git a/debian/nfs-utils_env.sh b/debian/nfs-utils_env.sh
index d48eb51..fa7ad84 100644
--- a/debian/nfs-utils_env.sh
+++ b/debian/nfs-utils_env.sh
@@ -12,7 +12,15 @@ echo PIPEFS_MOUNTPOINT=/run/rpc_pipefs
 echo RPCNFSDARGS=\"$RPCNFSDOPTS ${RPCNFSDCOUNT:-8}\"
 echo RPCMOUNTDARGS=\"$RPCMOUNTDOPTS\"
 echo STATDARGS=\"$STATDOPTS\"
+# The rpc-svcgssd.service systemd file uses SVCGSSDARGS, not
+# RPCSVCGSSDARGS, but for a long time just the latter was exported.
+# To not break upgrades for people who have worked around this by
+# overriding the systemd service to use RPCSVCGSSDARGS, both variables
+# are being exported now.
+# See https://bugs.launchpad.net/bugs/1616123 and
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=892654 for more details.
 echo RPCSVCGSSDARGS=\"$RPCSVCGSSDOPTS\"
+echo SVCGSSDARGS=\"$RPCSVCGSSDOPTS\"
 } > /run/sysconfig/nfs-utils
 
 # the following are supported by the systemd units, but not exposed in default files