interfaces: disable udev backend when inside a container #14930
+91
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pedronis
reviewed
Jan 15, 2025
interfaces/backends/backends.go
Outdated
| @@ -33,6 +35,14 @@ import ( | |||
| apparmor_sandbox "github.com/snapcore/snapd/sandbox/apparmor" | |||
| ) | |||
|
|
|||
| func isContainer() bool { | |||
| cmd := exec.Command("systemd-detect-virt", "--container", "--quiet") | |||
There was a problem hiding this comment.
we are doing this from other places, maybe time to have something in osutil?
There was a problem hiding this comment.
Yes, I saw we check for non-contianer virt as well. I'll have a look at a common helper used in both places.
There was a problem hiding this comment.
I've added a helper but one remaining call site is hard to adjust (squashfs) as it introduces dependency loops. I think this is fine for now. Without forcing a package split to resolve that.
pedronis
added
the
Needs security review
|
A quick run locally shows only unrelated failures: |
|
Fri Jan 17 17:28:22 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
bboozzoo
changed the title
We need know if we are running in a container in quite a few places in the code. It's time to add a helper and call it instead of doing it by hand. Signed-off-by: Zygmunt Krynicki <[email protected]>
Signed-off-by: Zygmunt Krynicki <[email protected]>
zyga
force-pushed
the
fix/udev-in-containers-SNAPDENG-24757
branch
from
6748117 to
3b289f6
Compare
Devices are not namespace-aware in Linux, thus udev inside containers does not really do anything sensible and is set to not execute if /sys is not mounted as a writable file system. This solves the issue where snapd is failing to install snaps in LXD, demanding workarounds like installing the snap twice, hoping that once-generated files are intact and not trigger a udev rule reload. Fixes: https://bugs.launchpad.net/snapd/+bug/1712808 Fixes: https://bugs.launchpad.net/snapd/+bug/1865503 Jira: https://warthogs.atlassian.net/browse/SNAPDENG-24757 Signed-off-by: Zygmunt Krynicki <[email protected]>
zyga
force-pushed
the
fix/udev-in-containers-SNAPDENG-24757
branch
from
3b289f6 to
82721e8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14930 +/- ##
==========================================
- Coverage 78.20% 78.06% -0.15%
==========================================
Files 1151 1135 -16
Lines 151396 152387 +991
==========================================
+ Hits 118402 118955 +553
- Misses 25662 26097 +435
- Partials 7332 7335 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bboozzoo
reviewed
Jan 17, 2025
Comment on lines
+26
to
+35
| // IsContainer returns true if the system is running in a container. | ||
| // | ||
| // The implementation calls: systemd-detect-virt --quiet --container. It can | ||
| // be mocked with testutil.MockCommand. The zero exit code indicates that | ||
| // system _is_ running a container. Ensuring that --container is passed is | ||
| // important. | ||
| func IsContainer() bool { | ||
| err := exec.Command("systemd-detect-virt", "--quiet", "--container").Run() | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
Suggested change
| // IsContainer returns true if the system is running in a container. | |
| // | |
| // The implementation calls: systemd-detect-virt --quiet --container. It can | |
| // be mocked with testutil.MockCommand. The zero exit code indicates that | |
| // system _is_ running a container. Ensuring that --container is passed is | |
| // important. | |
| func IsContainer() bool { | |
| err := exec.Command("systemd-detect-virt", "--quiet", "--container").Run() | |
| return err == nil | |
| } | |
| // IsContainer returns true if the system is running in a container as detected | |
| // by systemd helper tool systemd-detect-virt. | |
| func IsContainer() bool { | |
| // it's imperative to pass --container, zero exit code indicates thatsystem _is_ | |
| // running a container | |
| err := exec.Command("systemd-detect-virt", "--quiet", "--container").Run() | |
| return err == nil | |
| } |
There was a problem hiding this comment.
I kept the comment on how to mock it in the documentation because that shows up in IDEs.
| * | ||
| */ | ||
|
|
||
| package systemd |
There was a problem hiding this comment.
not entirely convinced this should be in systemd package. What about osutil?
There was a problem hiding this comment.
Why would it be in osutil? It's literally calling systemd. Osutil is a bag with endless OS code and I'd rather not append to it.
| @@ -149,7 +149,7 @@ func loadAppArmorProfiles() error { | |||
|
|
|||
| func isContainer() bool { | |||
| // systemd's implementation may fail on WSL2 with custom kernels | |||
| return release.OnWSL || (exec.Command("systemd-detect-virt", "--quiet", "--container").Run() == nil) | |||
| return release.OnWSL || systemd.IsContainer() | |||
There was a problem hiding this comment.
there are 2 more call locations: osutil/squashfs/fstype.go and daemon/api_general.go.
There was a problem hiding this comment.
Yes I know.
- squashfs cannot import systemd, I've mentioned that in the comment above.
- daemon doesn't check for containers so I left it out, the signature would be
systemd.VirtualizationTypebut this is orthogonal to this fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Devices are not namespace-aware in Linux, thus udev inside containers does not really do anything sensible and is set to not execute if /sys is not mounted as a writable file system.
This solves the issue where snapd is failing to install snaps in LXD, demanding workarounds like installing the snap twice, hoping that once-generated files are intact and not trigger a udev rule reload.
Fixes: https://bugs.launchpad.net/snapd/+bug/1712808
Fixes: https://bugs.launchpad.net/snapd/+bug/1865503
Jira: https://warthogs.atlassian.net/browse/SNAPDENG-24757