interfaces/builtin/mount_control: add support for nfs mounts #14694

bboozzoo · 2024-11-06T15:26:15Z

Extend the mount-control interface with support for nfs mounts. The nfs entries cannot specify any other filesystem types, nor contain the 'what' attribute.

Related: SNAPDENG-34139 LP#2086601

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

codecov · 2024-11-06T15:47:26Z

Codecov Report

Attention: Patch coverage is 94.82759% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.03%. Comparing base (96ea7b0) to head (b7a283e).
Report is 80 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/hookstate/ctlcmd/mount.go	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14694      +/-   ##
==========================================
+ Coverage   78.95%   79.03%   +0.08%     
==========================================
  Files        1084     1087       +3     
  Lines      146638   147666    +1028     
==========================================
+ Hits       115773   116706     +933     
- Misses      23667    23731      +64     
- Partials     7198     7229      +31

Flag	Coverage Δ
unittests	`79.03% <94.82%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder

Looks good, though I'm curious about a single nfs entry allowing connections to any nfs source. Would we want to allow the declaration to restrict valid sources?

olivercalder · 2024-11-06T16:00:04Z

interfaces/builtin/mount_control.go

@@ -583,6 +601,15 @@ func (iface *mountControlInterface) AppArmorConnectedPlug(spec *apparmor.Specifi
 			target = expanded + target[variableEnd:]
 		}

+		if mountInfo.isType("nfs") {
+			// override NFS share source, also see 'nfs-mount' interface
+			source = "*:**"


Does this mean that there can only be one entry for nfs in the mount-control plug? Does this mean that we allow any nfs source, so the snap could mount any source(s) it wants, mediated by one rule?

Would it be feasible to specify the source one wants to allow connections to (and deny others), or is that beyond the scope of the interface to specify?

The host address and the share path describe some remote system and are not controlled by snap in any way, so I'm not sure we can expect to have something reliable here. I would prefer not to rebuild a snap or need to regenerate an assertion each time this changes. We have a similar rule in nfs-mount and cifs-mount interfaces.

I assume this was agreed with Samuele?

bboozzoo · 2024-11-06T16:12:49Z

note for myself, file a ticket for allowing cifs mounts in mount-control.

pedronis

this looks reasonable to me

pedronis · 2024-11-07T16:16:07Z

interfaces/builtin/mount_control.go

+
+		allowNoSource := false
+		if strutil.ListContains(types, "nfs") {
+			if len(types) > 1 {


maybe relatedly to this we should have a comment here and maybe in the doc that supporting multiple types in one entry is mostly for situation when the filesystem is not provided but guessed or a single mount is expected to be performed with a few possible filesystems but under program control with other means

bboozzoo · 2024-11-12T14:20:40Z

I'm adding a spread test. I've already registered and received approval for test-snapd-mount-control-nfs snap. I'm waiting for allow-installation to be issued: https://forum.snapcraft.io/t/request-for-allow-installation-of-test-snapd-mount-control-nfs/43956

pedronis

thanks, I only reviewed the changes in interfaces/bultin, so please get a couple of reviews to cover better that and the rest

pedronis · 2024-11-12T15:38:48Z

interfaces/builtin/mount_control.go

-		if t == "tmpfs" {
-			includesTmpfs = true
+
+		if strutil.ListContains(exclusiveFsTypes, t) {


should we do && exclusiveType == "", so we capture the first and not the last of these?

alexmurray · 2024-11-13T00:19:00Z

FYI I have granted the use of mount-control for the new test snap in the store and so it should now be available for the spread tests etc.

zyga

Looks good but not quite ready:

tests have some things to improve
we should either reject nfs4 or support it all the way

zyga · 2024-11-08T15:52:04Z

interfaces/builtin/mount_control.go

@@ -125,6 +125,7 @@ var allowedFilesystemSpecificMountOptions = map[string][]string{
 	"jfs":        {"iocharset=", "resize=", "nointegrity", "integrity", "errors=", "noquota", "quota", "usrquota", "grpquota"},
 	"msdos":      {"blocksize=", "uid=", "gid=", "umask=", "dmask=", "fmask=", "allow_utime=", "check=", "codepage=", "conv=", "cvf_format=", "cvf_option", "debug", "discard", "dos1xfloppy", "errors=", "fat=", "iocharset=", "nfs=", "tz=", "time_offset=", "quiet", "rodir", "showexec", "sys_immutable", "flush", "usefree", "dots", "nodots", "dotsOK="},
 	"nfs":        {"nfsvers=", "vers=", "soft", "hard", "softreval", "nosoftreval", "intr", "nointr", "timeo=", "retrans=", "rsize=", "wsize=", "ac", "noac", "acregmin=", "acregmax=", "acdirmin=", "acdirmax=", "actimeo=", "bg", "fg", "nconnect=", "max_connect=", "rdirplus", "nordirplus", "retry=", "sec=", "sharecache", "nosharecache", "revsport", "norevsport", "lookupcache=", "fsc", "nofsc", "sloppy", "proto=", "udp", "tcp", "rdma", "port=", "mountport=", "mountproto=", "mounthost=", "mountvers=", "namlen=", "lock", "nolock", "cto", "nocto", "acl", "noacl", "local_lock=", "minorversion=", "clientaddr=", "migration", "nomigration"},
+	// TODO: drop nfs4?


Why would we drop nfs4?

I think so, from what I gather nfs defaults to 4 since nfs-utils 1.2 (15 years ago) and kernels 2.6.33. It's also deprecated from use in /etc/fstab. From manpages https://www.man7.org/linux/man-pages/man5/nfs.5.html when no nfsvers is specified, the client will start with 4.2 and then negotiate down until a compatible version is found (3 being the lowest anyway).

Also:

maciek@galeon:~ ls -l `which mount.nfs4` lrwxrwxrwx 1 root root 9 10-20 09:17 /usr/bin/mount.nfs4 -> mount.nfs

I'll just proceed to drop nfs4 from mount-control. There are no users, since nobody was able to specify the right attributes anyway.

interfaces/builtin/mount_control.go

zyga · 2024-11-14T10:17:59Z

interfaces/builtin/mount_control.go

+		}
+
+		allowNoSource := false
+		if strutil.ListContains(types, "nfs") {


overlord/hookstate/ctlcmd/mount.go

tests/lib/snaps/store/test-snapd-mount-control-nfs/snapcraft.yaml

tests/lib/snaps/store/test-snapd-mount-control-nfs/bin/cmd

tests/main/interfaces-mount-control-nfs/task.yaml

zyga · 2024-11-14T10:42:17Z

tests/main/interfaces-mount-control-nfs/test-snapd-sh/meta/snap.yaml

+version: 1.0
+
+apps:
+    cmd:


This should be called sh, not cmd. We have two styles of shell like tests, one that is like cmd and runs $@, effectively, and one that is sh and runs /bin/sh "$@". Those are not the same and we should be clear what is it that we run.

tests/lib/snaps/store/test-snapd-mount-control-nfs/bin/cmd

Extend the mount-control interface with support for nfs mounts. The nfs entries cannot specify any other filesystem types, nor contain the 'what' attribute. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

Add snap for testing mount-control with NFS Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

Add support for performing NFS mounts through `snapctl mount` command. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

… for nfs and tmpfs Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

…ve FS type during validation Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

The explicit 'nfs4' filesystem is deprecated and mounts should use the 'nfs' type which defaults to NFSv4 and supports all required options for handling NFSv3 and NFSv4. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

zyga

I left some more comments on .sh/.cmd in tests. NFS4 removal looks good

zyga · 2024-11-14T12:07:46Z

interfaces/builtin/mount_control_test.go

+		{
+			// deprecated nfs4
+			"mount:\n  - where: /media/foo\n    type: [nfs4]\n    options: [rw]\n    what: 127.0.0.1:/share",
+			`mount-control "what" attribute is invalid: must start with / and not contain special characters`,


I would have expected this to say "nfs4" is not supported.

it keeps the current status quo wrt nfs4, i.e. it's not possible to construct an mount-control entry which would work

This might be more confusing than helpful, what is the impact of changing this to be more accurate.

zyga · 2024-11-14T12:10:46Z

tests/lib/snaps/store/test-snapd-mount-control-nfs/bin/sh

+#!/bin/sh
+PS1='$ '
+
+exec "$@"


This is not /bin/sh "$@", the name is misleading and it differs from other "sh" apps in other tests. If you want to keep the ".cmd" suffix and semantics just do so.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

zyga

LGTM! Thank you.

alexmurray

LGTM!

ernestl

First pass, looks good, some comments for consideration.

ernestl · 2024-11-18T07:01:24Z

tests/main/interfaces-mount-control-nfs/task.yaml

+
+# limit to systems where we know NFS works without much hassle
+systems:
+  - ubuntu-24.04-*


The actual bug is reported for core22.
What is the risk when limiting to release 24.04?

The dependencies we need for exporting NFS shared as not available on Ubuntu Core.

tests/main/interfaces-mount-control-nfs/task.yaml

interfaces/builtin/mount_control.go

ernestl · 2024-11-18T08:30:13Z

interfaces/builtin/mount_control.go

-	if includesTmpfs && len(types) > 1 {
-		return errors.New(`mount-control filesystem type "tmpfs" cannot be listed with other types`)
+	if exclusiveFsType != "" && len(types) > 1 {
+		return fmt.Errorf(`mount-control filesystem type %q cannot be listed with other types`, exclusiveFsType)


Should something about this be documented?
https://snapcraft.io/docs/mount-control-interface

yes, but editing pages in the forum does not work atm

ernestl · 2024-11-18T08:47:19Z

interfaces/builtin/mount_control_test.go

+		{
+			// deprecated nfs4
+			"mount:\n  - where: /media/foo\n    type: [nfs4]\n    options: [rw]\n    what: 127.0.0.1:/share",
+			`mount-control "what" attribute is invalid: must start with / and not contain special characters`,


This might be more confusing than helpful, what is the impact of changing this to be more accurate.

ernestl · 2024-11-18T09:12:24Z

interfaces/builtin/mount_control.go

@@ -583,6 +601,15 @@ func (iface *mountControlInterface) AppArmorConnectedPlug(spec *apparmor.Specifi
 			target = expanded + target[variableEnd:]
 		}

+		if mountInfo.isType("nfs") {
+			// override NFS share source, also see 'nfs-mount' interface
+			source = "*:**"


I assume this was agreed with Samuele?

ernestl · 2024-11-18T09:13:31Z

interfaces/builtin/mount_control_test.go

+		// possible construct a valid and useful specification using this
+		// type
+		{
+			// deprecated nfs4


Should we update nfs_mount.go ?

mount fstype=nfs{,4} *:** -> ...

In a separate PR perhaps

interfaces/builtin/mount_control.go

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

…retry Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

ernestl

Thanks

* master: (44 commits) wrappers: do not reload activation units (canonical#14724) gadget/install: support for no{exec,dev,suid} mount flags interfaces/builtin/mount_control: add support for nfs mounts (canonical#14694) tests: use gojq - part 1 (canonical#14686) interfaces/desktop-legacy: allow DBus access to com.canonical.dbusmenu interfaces/builtin/fwupd.go: allow access to nvmem for thunderbolt plugin tests: removing uc16 executions (canonical#14575) tests: Added arm github runner to build snapd (canonical#14504) tests: no need to run spread when there are not tests matching the filter (canonical#14728) tests/lib/tools/store-state: exit on errors, update relevant tests (canonical#14725) tests: udpate the github workflow to run tests suggested by spread-filter tool (canonical#14519) testtime: add mockable timers for use in tests (canonical#14672) interface/screen_inhibit_control: Improve screen inhibit control for use on core (canonical#14134) tests: use images with 20G disk in openstack (canonical#14720) i/builtin: allow @ in custom-device filepaths (canonical#14651) tests: refactor test-snapd-desktop-layout-with-content tests: fix broken app definition tests: capitalize sentences in comments tests: wrap very long shell line tests: fix raciness in async startup and sync install ...

bboozzoo added the Needs Samuele review Needs a review from Samuele before it can land label Nov 6, 2024

bboozzoo requested a review from pedronis November 6, 2024 15:26

olivercalder reviewed Nov 6, 2024

View reviewed changes

pedronis reviewed Nov 7, 2024

View reviewed changes

bboozzoo added this to the 2.67 milestone Nov 12, 2024

bboozzoo force-pushed the bboozzoo/mount-control-nfs branch from bd27458 to 27f6ceb Compare November 12, 2024 15:14

bboozzoo marked this pull request as ready for review November 12, 2024 15:14

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Nov 12, 2024

bboozzoo added Needs security review Can only be merged once security gave a :+1: and removed Needs Documentation -auto- Label automatically added which indicates the change needs documentation labels Nov 12, 2024

bboozzoo assigned alexmurray and unassigned alexmurray Nov 12, 2024

bboozzoo requested review from alexmurray, olivercalder and pedronis November 12, 2024 15:14

pedronis approved these changes Nov 12, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/mount-control-nfs branch from 27f6ceb to b30c74d Compare November 13, 2024 08:43

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Nov 13, 2024

bboozzoo removed the Needs Samuele review Needs a review from Samuele before it can land label Nov 13, 2024

bboozzoo requested a review from zyga November 14, 2024 10:15

zyga reviewed Nov 14, 2024

View reviewed changes

bboozzoo added 5 commits November 14, 2024 12:44

interfaces/builtin/mount_control: add support for nfs mounts

bb216e5

Extend the mount-control interface with support for nfs mounts. The nfs entries cannot specify any other filesystem types, nor contain the 'what' attribute. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

tests/lib/snaps/store/test-snapd-mount-control-nfs: add test snap

4a06f96

Add snap for testing mount-control with NFS Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

tests/main/interfaces-mount-control-nfs: spread test

ce74fca

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

overlord/hookstate: support for mount-control with nfs

145bc32

Add support for performing NFS mounts through `snapctl mount` command. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

interfaces/builtin/mount-control: improve validation and doc comments…

977fca9

… for nfs and tmpfs Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo added 3 commits November 14, 2024 12:44

interfaces/builtin: complain about the first occurrence of an exclusi…

eac9a8c

…ve FS type during validation Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

interfaces/builtin/mount_control: drop nfs4

5ad9605

The explicit 'nfs4' filesystem is deprecated and mounts should use the 'nfs' type which defaults to NFSv4 and supports all required options for handling NFSv3 and NFSv4. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

interfaces/builtin/mount_control: improve coverage of validation code

ebaac84

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo force-pushed the bboozzoo/mount-control-nfs branch from 889ebdf to 1b9f9b4 Compare November 14, 2024 11:58

bboozzoo requested review from zyga and ernestl November 14, 2024 11:59

zyga reviewed Nov 14, 2024

View reviewed changes

bboozzoo added 2 commits November 14, 2024 13:32

tests/lib/snaps/store/test-snapd-mount-control-nfs: tweak snapcraft

7257a3f

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

tests/main/interfaces-mount-control-nfs: improve cleanup

6529d53

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo force-pushed the bboozzoo/mount-control-nfs branch from 1b9f9b4 to 6529d53 Compare November 14, 2024 13:06

bboozzoo requested a review from zyga November 14, 2024 13:07

zyga approved these changes Nov 14, 2024

View reviewed changes

alexmurray approved these changes Nov 18, 2024

View reviewed changes

alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Nov 18, 2024

ernestl reviewed Nov 18, 2024

View reviewed changes

bboozzoo added 4 commits November 18, 2024 11:44

interfaces/builtin: comment tweak

37fe977

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

interfaces/builtin/mount_control: naming tweak

8aa27f2

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

interfaces/builtin/mount_control: consider nfs4 to be deprecated

990e336

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

tests/main/interfaces-mount-control-nfs: enable Ubuntu 22.04, better …

b7a283e

…retry Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

bboozzoo requested a review from ernestl November 18, 2024 11:33

ernestl approved these changes Nov 18, 2024

View reviewed changes

ernestl merged commit 0543dff into canonical:master Nov 18, 2024
53 of 56 checks passed

bboozzoo deleted the bboozzoo/mount-control-nfs branch November 19, 2024 12:35

interfaces/builtin/mount_control: add support for nfs mounts #14694

interfaces/builtin/mount_control: add support for nfs mounts #14694

Conversation

bboozzoo commented Nov 6, 2024 • edited by jira bot Loading

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Nov 6, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Nov 12, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmurray commented Nov 13, 2024

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

ernestl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestl left a comment

Choose a reason for hiding this comment

bboozzoo commented Nov 6, 2024 •

edited by jira bot

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

zyga Nov 14, 2024 •

edited

Loading