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

snapd-generator: set standard snapfuse options when generating units for containers #9613

Merged
merged 2 commits into from Nov 10, 2020

Conversation

stolowski
Copy link
Contributor

Set standard snapfuse options (specifically, allow_other) in snapd-generator units for containers. If not set, the options from /etc/systemd/system/ mount unit is used, which in case of squashfs, doesn't have allow_other set.

For backgound: https://forum.snapcraft.io/t/cannot-locate-base-snap-core18-permission-denied/20796/19

@stolowski stolowski added the Bug label Nov 9, 2020
@stolowski stolowski added this to the 2.48 milestone Nov 9, 2020
@stolowski stolowski changed the title snapd-generator: set standard snapfuse options when generating mount units containers snapd-generator: set standard snapfuse options when generating units for containers Nov 9, 2020
@@ -185,7 +185,7 @@ int ensure_fusesquashfs_inside_container(const char *normal_dir)
fprintf(stderr, "cannot open %s: %m\n", fname);
return 2;
}
fprintf(f, "[Mount]\nType=%s\n", fstype);
fprintf(f, "[Mount]\nType=%s\nOptions=nodev,ro,x-gdu.hide,allow_other\nLazyUnmount=yes\n", fstype);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a link to the code before this was a generator and what options we set back then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is spread over systemd.go (see mountUnitTemplate and fsMountOptions) and fstype.go in a bit convoluted way; I've copied them from mount options of a manually installed snap inside the container.

Copy link
Contributor

Choose a reason for hiding this comment

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

this will be applied to all types, yet i thought in gocode the "allow_other" at least is scoped to fuse mount types only.

I somehow expected something like if strcmp() ==0 || strcmp == 0 to fprintf(",allow_other)" or some such (pseudo code).

Or it is safe to always pass allow_other?

It would be nice if the test would be somehow interactive.... can we compare the generated units in /run/systemd/generator with the ones in /etc/systemd/system ? Such that if and when mount options are changed anywhere in the gocode, the generator unittest fails due to any differences?

I guess as long as unit tests do run under lxd and non-lxd at least sometimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xnox this part of the generator is run for squashfuse/snapfuse only (fstype is either "fuse.squashfuse" or "fuse.snapfuse"). Both support "allow_other".

The idea for the test is nice, I'll see what I can do here or in a followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, found it in the code

Copy link
Contributor Author

@stolowski stolowski Nov 9, 2020

Choose a reason for hiding this comment

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

So the extra test check (comparing Options of snapd-generator unit & snapd-generated unit) is possible and I'm working on it but fighting some issues around these checks, I think I'll leave it for a followup tomorrow, don't want to block this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a followup #9617

Copy link
Contributor

@mvo5 mvo5 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!

@@ -185,7 +185,7 @@ int ensure_fusesquashfs_inside_container(const char *normal_dir)
fprintf(stderr, "cannot open %s: %m\n", fname);
return 2;
}
fprintf(f, "[Mount]\nType=%s\n", fstype);
fprintf(f, "[Mount]\nType=%s\nOptions=nodev,ro,x-gdu.hide,allow_other\nLazyUnmount=yes\n", fstype);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, found it in the code

Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

PR looks good to me, thanks for digging deep on this and writing a spread test. One comment about making the spread test more robust however (but that can be for a followup).

VERSION_ID="$(. /etc/os-release && echo "$VERSION_ID" )"
lxd.lxc launch --quiet "ubuntu:$VERSION_ID" ubuntu

lxd.lxc file push --quiet "$GOHOME"/snapd_*.deb "ubuntu/root/"
Copy link
Member

Choose a reason for hiding this comment

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

we have some logic inside another script for setting up a container with the new snapd, is there a reason we're not using that here? I'm worried that for some reason something here will eventually need internet access inside the container and if we're not setting up the proxy it will fail, etc. or there will be issues installing the snapd deb here that are avoided with the workarounds in that script etc.

To be clear, this isn't a blocker, I'm just worried that this test might be a bit flaky without those extra things setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't use that script (in fact I tried to add my checks first to existing lxd test, but then decided to add a new test) is it's a bit blunt and first purges snapd (and with that removes all snaps, e.g. core18 that come preseeded with lxd). I can add http_proxy logic here but I wasn't sure what is the expected use case for proxy?

Copy link
Member

Choose a reason for hiding this comment

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

thanks for explaining that, makes sense now.

as to the proxy, I'm not sure, I always seem to see random lxd things fail around internet issues so I just implicitly assume that things in the container need internet access and as such setting up a proxy makes sense, but perhaps this case is straight forward enough it won't need a proxy setup at all.

@mvo5 mvo5 merged commit ea7f470 into snapcore:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants