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

cmd,dirs: fix various issues discovered by a Fedora base snap #5620

Merged
merged 3 commits into from Aug 10, 2018

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Aug 9, 2018

This patch fixes the algorithm that dertermines the mount point for snaps. The
old code used to work because it was looking at /etc/os-release and looking for
specific ID or ID_LIKE values. With the introduction of base snaps that can
have arbitrary os-release files we cannot rely on this information alone.

When a process is inside a snap mount namespace that has a base snap mounted as
the root filesystem then the os-release level information is not useful and
must not be used anymore. Inside that environment the snaps are always mounted
at /snap.

On the outside of a mount namespace the current algorithm is correct, it will
identify the distribution and apply any differences mandated by the
distribution policy.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

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

This patch fixes the algorithm that dertermines the mount point for snaps.  The
old code used to work because it was looking at /etc/os-release and looking for
specific ID or ID_LIKE values. With the introduction of base snaps that can
have arbitrary os-release files we cannot rely on this information alone.

When a process is inside a snap mount namespace that has a base snap mounted as
the root filesystem then the os-release level information is not useful and
must not be used anymore. Inside that environment the snaps are always mounted
at /snap.

On the outside of a mount namespace the current algorithm is correct, it will
identify the distribution and apply any differences mandated by the
distribution policy.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM. A spread test with fedora base snap?

@zyga
Copy link
Collaborator Author

zyga commented Aug 9, 2018

Oh, indeed! I should be able to use the hello-fedora test from the store for this.

This patch allows snap-confine to correctly identify non-Ubuntu-based
base snaps as such. This essentially allows snap-confine to care less
about the distribution ID and more about the concept of "snappy"
vs "classic" rootfs.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@zyga zyga changed the title dirs: fix SnapMountDir inside a Fedora base snap cmd,dirs: fix various issues discovered by a Fedora base snap Aug 9, 2018
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@mvo5 mvo5 added this to the 2.35 milestone Aug 10, 2018
@codecov-io
Copy link

Codecov Report

Merging #5620 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5620      +/-   ##
==========================================
+ Coverage   78.93%   78.97%   +0.03%     
==========================================
  Files         520      520              
  Lines       39567    39634      +67     
==========================================
+ Hits        31234    31300      +66     
- Misses       5789     5790       +1     
  Partials     2544     2544
Impacted Files Coverage Δ
dirs/dirs.go 97.95% <100%> (+0.13%) ⬆️
overlord/ifacestate/ifacestate.go 79.87% <0%> (+0.12%) ⬆️
overlord/snapstate/snapstate.go 81.97% <0%> (+0.72%) ⬆️
overlord/hookstate/hookmgr.go 74.51% <0%> (+0.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab24a15...09f7463. Read the comment docs.

@mvo5 mvo5 merged commit 0f97726 into snapcore:master Aug 10, 2018
@zyga zyga deleted the fix/inside-fedora-base branch August 10, 2018 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants