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

snap-exec: fix detection if cups interface is connected #11616

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Apr 1, 2022

The cups interface needs a way to set the environment variable
CUPS_SERVER if the cups interface is connected. However we
have no mechanism for setting environment vars based on interface
connections currently. To workaround this, the cups work added
a workaround in snap-exec that tried to detect if cups is
connected and when it is set the environment. Unfortunately the
code in there was too simplistic because it just checked if
the directory /var/cups exists. However ths dir now always
exists because it's needed as the mount point.

This commit fixes the detection by checking if /var/cups is
a bind mount. This is checked by looking at the stat() data
and the dev_t field in there. If they differ it means the
bind mount exists and the only thing that creates this bind
mount is the cups MountConnectedPlug() code.

This is not great but it fixes the spread failure we see
in the cups-control test (which is a real bug) and should
be good enough until we have a proper interface backend that
can set environment variables.

mvo5 added 2 commits April 1, 2022 13:23
The cups interface needs a way to set the environment variable
`CUPS_SERVER` if the `cups` interface is connected. However we
have no mechanism for setting environment vars based on interface
connections currently. To workaround this, the cups work added
a workaround in `snap-exec` that tried to detect if `cups` is
connected and when it is set the environment. Unfortunately the
code in there was too simplistic because it just checked if
the directory `/var/cups` exists. However ths dir now always
exists because it's needed as the mount point.

This commit fixes the detection by checking if `/var/cups` is
a bind mount. This is checked by looking at the `stat()` data
and the `dev_t` field in there. If they differ it means the
bind mount exists and the only thing that creates this bind
mount is the `cups` `MountConnectedPlug()` code.

This is not great but it fixes the spread failure we see
in the `cups-control` test (which is a real bug) and should
be good enough until we have a proper interface backend that
can set environment variables.
For unknown reasons the error message on unplug changes when I
run this on my 20.04 system so I updated the tests - this may
need more investigation.
@mvo5 mvo5 added this to the 2.55 milestone Apr 1, 2022
@@ -74,4 +72,4 @@ execute: |
echo "Expected error with plug disconnected"
exit 1
fi
MATCH "scheduler not responding" < print.error
MATCH "(scheduler not responding|No default destination)" < print.error
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this seems to be needed because the test-snapd-cups-control-consumer moved to base: core20 very recently.

Copy link
Collaborator

@sergiocazzolato sergiocazzolato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the fix

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

cmd/snap-exec/main.go Show resolved Hide resolved
Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mvo5 mvo5 merged commit 0275484 into canonical:master Apr 4, 2022
@tillkamppeter
Copy link

@mvo5 I have done a quick test:

$ sudo snap refresh --edge snapd
2022-04-04T20:24:17+02:00 INFO Waiting for automatic snapd restart...
snapd (edge) 2.55.2+git188.ge21669c from Canonical✓ refreshed
$ snap list
cups                             2.4.1-3                      x39    latest/edge      -                 -
snapd                            2.55.2+git188.ge21669c       15441  latest/edge      canonical✓        snapd
$ snap version
snap    2.55.2+git188.ge21669c
snapd   2.55.2+git188.ge21669c
series  16
ubuntu  22.04
kernel  5.15.0-22-generic
$ sudo snap stop cups
Stopped.
$ sudo snap start cups
Started.
$ snap run --shell cups.cupsd
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.
$ echo $CUPS_SERVER
/var/cups/cups.sock
$ stat / /var/cups | grep Device
Device: 709h/1801d	Inode: 11786       Links: 21
Device: 709h/1801d	Inode: 11666       Links: 2
$ exit
$

I have updated to today's snapshot of snapd from Edge, which, identifying by the commit ID in the version number is from a commit AFTER the commit which is the merge of this PR. After that I have stopped and started again the CUPS Snap which is on my system, a local build of the current GIT status of the CUPS Snap. Due to the new snapd with this PR merged, the CUPS_SERVER environment variable should NOT be set, but it IS set. I checked also whether there is a bind mount on /var/cups/ and there is no bind mount. So why is the variable set? Something wrong here.

@tillkamppeter
Copy link

@mvo5 I have also a Snap which plugs cups-control and here it seems to be all OK:

$ snap connections | grep cups-control
cups-control              cups-admin-test:cups-control              :cups-control                    manual
$ cups-admin-test.lpstat -r
scheduler is running
$ snap run --shell cups-admin-test.lpstat
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.
$ echo $CUPS_SERVER

$ stat / /var/cups | grep Device
Device: 709h/1801d	Inode: 11786       Links: 21
Device: 709h/1801d	Inode: 11666       Links: 2
$ exit
$

So for Snaps plugging cups-control one does not need to do something different to what we thought out in the beginning. One can also simply check whether there is a bind mount on /var/cups/ and if there is none, one does not set the variable.

I am wondering what the problem with the CUPS Snap is.

@tillkamppeter
Copy link

Locally rebuilding the CUPS Snap did not solve the problem.

@tillkamppeter
Copy link

This is a Snap which actually plugs cups and it does correctly:

$ snap connections | grep cups:cups
cups                      cups-admin-test-no-control:cups           cups:cups                        manual
$ cups-admin-test-no-control.lpstat -r
scheduler is running
$ cups-admin-test-no-control.lpstat -v
device for cljp176n_till_x1yoga: proxy://%2Frun%2Fcups%2Fcups.sock/cljp176n_till_x1yoga
device for commandtops-test: proxy://%2Frun%2Fcups%2Fcups.sock/commandtops-test
[...]
$ cups-admin-test-no-control.lpinfo -v
lpinfo: Forbidden
$ snap run --shell cups-admin-test-no-control.lpstat
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.
$ echo $CUPS_SERVER
/var/cups/cups.sock
$ stat / /var/cups | grep Device
Device: 709h/1801d	Inode: 11786       Links: 21
Device: 10302h/66306d	Inode: 53764177    Links: 2
$ exit
$

All correct here, only the CUPS Snap itself is doing wrong ...

@tillkamppeter
Copy link

In which cases the code in cmd/snap-exec/main_test.go is executed? Only in unit tests or also in production (like starting an actual Snap's app)?

@tillkamppeter
Copy link

I have now done the following:

I have removed all the three Snaps, cups, cups-admin-test, and cups-admin-test-no-control using sudo snap remove --purge SNAPNAME, so removing all configuration, cache, ... from the Snaps, then I re-installed the Snaps (from local *.snap files), manually connected the interfaces (as I did not install from the Snap Store), ...

... and this solved the problem!!!

I retested all the three Snaps as in my posts above, getting the same (correct) result for cups-admin-test which plugs cups-control and for cups-admin-test-no-control which plugs cups, and getting also a correct result for the CUPS Snap, the CUPS_SERVER variable is now NOT set and the commands cups.lpstat -r, cups.lpstat -v, and cups.lpinfo -v show all the correct results.

Conclusions

Principally, the bug is fixed.

But what I am wondering about now is that if at some point in time in a Snap's sandbox an environment variable is set, that it seems to survive restarts, stopping and starting, updates of both the Snap itself and of snapd, ... in some cache where I have no idea where it is (it seems not to be in /var/snap/cups/current/ or /var/snap/cups/common/). Is this true?

Then I would suggest that if snap-exec finds that there is NO bind mount, that it not only not sets CUPS_SERVER but even UNSETS CUPS_SERVER, to be sure to not have any remains from an old, buggy snapd (2.55.2 from stable).

WDYT?

@tillkamppeter
Copy link

And by the way, I tested echo . | cups-admin-test-no-control.lp -d Printer now and it prints!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants