tests: new test for cifs-mount interface #5714

sergiocazzolato · 2018-08-23T21:35:22Z

No description provided.

bboozzoo

Looking good. Some comments inline.

bboozzoo · 2018-08-24T05:32:52Z

tests/main/interfaces-cifs-mount/task.yaml

+    snap install test-snapd-cifs-mount
+
+    # Create linux and samba users 
+    sudo useradd -s /bin/true test-cifs


The tests are executed by root anyway, no need to use sudo.

bboozzoo · 2018-08-24T05:35:28Z

tests/main/interfaces-cifs-mount/task.yaml

+
+    # Configure samba
+    cp /etc/samba/smb.conf /etc/samba/smb.conf.back
+    echo "[data]" >> /etc/samba/smb.conf


This should work too:

[data] path=$(pwd)/local_share writable=yes browsable=yes

shellcheck will probably suggest something like this:

(echo "[data]" echo "path=$(pwd)/local_share" echo "writable=yes" echo "browsable=yes" ) > /etc/samba/smb.conf

bboozzoo · 2018-08-24T05:36:12Z

tests/main/interfaces-cifs-mount/task.yaml

+    echo "browsable=yes" >> /etc/samba/smb.conf
+
+    systemctl restart smbd.service
+    systemctl restart nmbd.service


You don't need to touch nmbd, IIRC it's only used for NetBIOS name resolution and shouldn't matter here.

bboozzoo · 2018-08-24T05:39:02Z

tests/main/interfaces-cifs-mount/task.yaml

+    snap connect test-snapd-cifs-mount:cifs-mount
+
+    echo "Then the snap is able to mount and umount a cifs fs"
+    test-snapd-cifs-mount.sh -c "mount -t cifs //127.0.0.1/data /var/snap/test-snapd-cifs-mount/current/local_mount -o user=test-cifs,passowrd=test"


I thin this line and ones below should use $SNAP_DATA, eg:

test-snapd-cifs-mount.sh -c "mount -t cifs //127.0.0.1/data \$SNAP_DATA/local_mount -o user=test-cifs,passowrd=test"

bboozzoo · 2018-08-24T05:40:20Z

tests/main/interfaces-cifs-mount/task.yaml

+    snap connect test-snapd-cifs-mount:cifs-mount
+
+    echo "Then the snap is able to mount and umount a cifs fs"
+    test-snapd-cifs-mount.sh -c "mount -t cifs //127.0.0.1/data /var/snap/test-snapd-cifs-mount/current/local_mount -o user=test-cifs,passowrd=test"


And you probably need to mkdir the mount point first too:

test-snapd-cifs-mount.sh -c "mkdir \$SNAP_DATA/local_mount"

sergiocazzolato · 2018-08-27T15:44:05Z

@bboozzoo thanks for reviewing this, all the commest have been addressed.

bboozzoo · 2018-08-28T06:00:50Z

2018-08-27 16:02:15 Error executing google:ubuntu-16.04-64:tests/main/interfaces-cifs-mount : 
-----
+ echo 'The interface is not connected by default'
The interface is not connected by default
+ MATCH -- '^- +test-snapd-cifs-mount:cifs-mount'
+ snap interfaces -i cifs-mount
+ echo 'When the interface is connected'
When the interface is connected
+ snap connect test-snapd-cifs-mount:cifs-mount
+ echo 'Then the snap is able to mount and umount a cifs fs'
Then the snap is able to mount and umount a cifs fs
+ test-snapd-cifs-mount.sh -c 'mount -t cifs //127.0.0.1/data $SNAP_DATA/local_mount -o user=test-cifs,passowrd=test'
/bin/sh: 1: mount: Permission denied
-----
...
[Mon Aug 27 16:02:14 2018] audit: type=1400 audit(1535385735.584:105): 
   apparmor="DENIED" operation="exec" 
   profile="snap.test-snapd-cifs-mount.sh" name="/bin/mount" 
   pid=25232 comm="sh" requested_mask="x" denied_mask="x" fsuid=0 ouid=0

Maybe the interface should add (u)mount to the interface?

/usr/bin/mount ixr,
/usr/bin/umount ixr,

Otherwise when you have for eg. a simple Python app, you cannot really use the interface unless you poke the libc via ctypes or somesuch. @zyga @jdstrand wdyt?

niemeyer · 2018-08-28T10:46:55Z

@bboozzoo This interface is paired with mount-observe to have access to reading the current mounts.

@sergiocazzolato You'll probably need that too.

niemeyer

Some comments, and LGTM once you're happy about their consideration.

niemeyer · 2018-08-28T10:52:58Z

tests/main/interfaces-cifs-mount/task.yaml

+
+    # Configure samba
+    cp /etc/samba/smb.conf /etc/samba/smb.conf.back
+    { echo "[data]"; echo "path=$(pwd)/local_share"; echo "writable=yes"; echo "browsable=yes"; } >> /etc/samba/smb.conf


$ echo -e "foo\nbar" foo bar

niemeyer · 2018-08-28T10:53:26Z

tests/lib/snaps/test-snapd-cifs-mount/snapcraft.yaml

+apps:
+  sh:
+    command: sh
+    plugs: [cifs-mount]


Probably needs mount-observe, per comments.

@niemeyer , thanks for reviewing this. I tried by adding the mount-observe plug but still having the same denial. testlog: https://paste.ubuntu.com/p/X56wt4CHrw/

niemeyer · 2018-08-28T10:54:52Z

tests/main/interfaces-cifs-mount/task.yaml

+    The cifs-mount interface allows mounting and unmounting CIFS filesystems.
+
+prepare: |
+    snap install test-snapd-cifs-mount


Shouldn't this be installing the local snap?

As I need some dependencies as part of this snap, I had to create a snap and upload it to the store.

niemeyer · 2018-08-28T10:55:20Z

tests/main/interfaces-cifs-mount/task.yaml

+
+execute: |
+    echo "The interface is not connected by default"
+    snap interfaces -i cifs-mount | MATCH -- '^- +test-snapd-cifs-mount:cifs-mount'


Why the --?

Not strictly needed here because '^.. will be detected as part of the pattern and handled porperly. However, the pattern MATCH '- +test..' will fail because '- will be seen as a -foo switch. So it's either MATCH -- <pattern>. We could use grep .. -e switch which makes grep expec <pattern> as the next argument.

bboozzoo · 2018-08-29T05:59:27Z

This interface is paired with mount-observe to have access to reading the current mounts.

We're not even getting that far. The problem is that currently the only way of doing the mount is via mount(2) because the policy does not allow executing mount|umount. Looking and udisks2, there's the following entry in the policy (which I think we need in cifs-mount too):

# udisksd execs mount/umount to do the actual operations
/bin/mount ixr,
/bin/umount ixr,

# mount/umount (via libmount) track some mount info in these files
/run/mount/utab* wrl,

Edit: and sorry for accidentally closing the PR, I've clicked the wrong button.

chipaca · 2018-10-17T11:20:41Z

Can't we call mount.cifs directly?

sergiocazzolato · 2018-10-26T14:09:14Z

@chipaca, in the test we are already using mount.cifs.

test-snapd-cifs-mount.sh -c "mount.cifs //127.0.0.1/data1 $SNAP_DATA/local_mount -o user=test-cifs,passowrd=test"

zyga

LGTM

zyga · 2018-10-26T15:22:20Z

+ smbpasswd -x test-cifs
/bin/bash: line 64: smbpasswd: command not found

It looks like we need to patch the test to install another package @sergiocazzolato

zyga · 2018-10-26T19:51:03Z

When the interface is connected
+ snap connect test-snapd-cifs-mount:cifs-mount
+ snap connect test-snapd-cifs-mount:network-control
error: snap "test-snapd-cifs-mount" has no plug named "network-control"

zyga · 2018-10-26T21:15:37Z

This is now failing on:

+ test-snapd-cifs-mount.sh -c 'mount.cifs //127.0.0.1/data1 $SNAP_DATA/local_mount -o user=test-cifs,passowrd=test'
/bin/sh: 1: mount.cifs: not found

@sergiocazzolato can you look at this next week please. Feels like we are missing something :)

sergiocazzolato · 2018-11-28T13:08:00Z

It is blocked until we find a solution for the denial trying to access to /run/mount/utab

sergiocazzolato · 2018-12-12T18:49:07Z

Closeing the PR until a solution to the denial is found

New test for cifs-mount interface

99af3a2

bboozzoo reviewed Aug 24, 2018

View reviewed changes

shellchecks fixed and SNAP_DATA dir used as mountpoint

798429d

niemeyer approved these changes Aug 28, 2018

View reviewed changes

Improve echo command

ae84d99

bboozzoo closed this Aug 29, 2018

bboozzoo reopened this Aug 29, 2018

New way using mount.cifs from the system

c79604e

Merge branch 'master' into tests-interface-cifs-mount

e81aecf

zyga approved these changes Oct 26, 2018

View reviewed changes

Undo installing local snap

012d32e

New local snap for cifs mount

b8497d7

zyga approved these changes Oct 29, 2018

View reviewed changes

sergiocazzolato added 3 commits November 26, 2018 11:06

Merge branch 'master' into tests-interface-cifs-mount

9184491

Merge branch 'master' into tests-interface-cifs-mount

8b34ea9

Use test-snapd-sh snap instead

843205f

sergiocazzolato added the ⛔ Blocked label Nov 28, 2018

sergiocazzolato closed this Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: new test for cifs-mount interface #5714

tests: new test for cifs-mount interface #5714

sergiocazzolato commented Aug 23, 2018

bboozzoo left a comment

bboozzoo Aug 24, 2018

bboozzoo Aug 24, 2018

bboozzoo Aug 24, 2018

bboozzoo Aug 24, 2018

bboozzoo Aug 24, 2018

sergiocazzolato commented Aug 27, 2018

bboozzoo commented Aug 28, 2018 •

edited

niemeyer commented Aug 28, 2018

niemeyer left a comment

niemeyer Aug 28, 2018

sergiocazzolato Aug 28, 2018

niemeyer Aug 28, 2018

sergiocazzolato Aug 28, 2018

niemeyer Aug 28, 2018

sergiocazzolato Aug 28, 2018

niemeyer Aug 28, 2018

bboozzoo Aug 28, 2018

bboozzoo commented Aug 29, 2018 •

edited

chipaca commented Oct 17, 2018

sergiocazzolato commented Oct 26, 2018

zyga left a comment

zyga commented Oct 26, 2018

zyga commented Oct 26, 2018

zyga commented Oct 26, 2018

sergiocazzolato commented Nov 28, 2018

sergiocazzolato commented Dec 12, 2018

tests: new test for cifs-mount interface #5714

tests: new test for cifs-mount interface #5714

Conversation

sergiocazzolato commented Aug 23, 2018

bboozzoo 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

sergiocazzolato commented Aug 27, 2018

bboozzoo commented Aug 28, 2018 • edited

niemeyer commented Aug 28, 2018

niemeyer 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

bboozzoo commented Aug 29, 2018 • edited

chipaca commented Oct 17, 2018

sergiocazzolato commented Oct 26, 2018

zyga left a comment

Choose a reason for hiding this comment

zyga commented Oct 26, 2018

zyga commented Oct 26, 2018

zyga commented Oct 26, 2018

sergiocazzolato commented Nov 28, 2018

sergiocazzolato commented Dec 12, 2018

bboozzoo commented Aug 28, 2018 •

edited

bboozzoo commented Aug 29, 2018 •

edited