Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap,tests/main: add confinement switch instead of spread system blacklisting #3274
Conversation
zyga
requested changes
May 8, 2017
One fundamental comment about asking snapd vs telling locally. Others are simple suggestions.
| + } | ||
| + | ||
| + fmt.Println("Before check") | ||
| + fmt.Fprintf(Stdout, "%v\n", release.ReleaseInfo.ForceDevMode()) |
zyga
May 8, 2017
Contributor
I think this should be asking snapd (via the /info) URL as snap and snapd may disagree, if one is local and one is remote.
| + | ||
| + fmt.Println("Before check") | ||
| + fmt.Fprintf(Stdout, "%v\n", release.ReleaseInfo.ForceDevMode()) | ||
| + return nil |
zyga
May 8, 2017
Contributor
In addition, to simplify scripting I would convey the result in the exit code. If we are in forced devmode return zero, return nonzero otherwise.
| - exit 1 | ||
| - fi | ||
| - grep -q "Permission denied" fuse.error | ||
| + if [ "$(snap forced-devmode)" = "false" ]; then |
zyga
May 8, 2017
Contributor
You can then use more natural if snap !forced-devmode; then ... fi` syntax.
| @@ -66,6 +62,10 @@ execute: | | ||
| echo "Then the service is accessible by a client" | ||
| nc -w 2 -q 2 localhost "$PORT" < $REQUEST_FILE | grep -Pqz "ok\n" | ||
| + if ! snap forced-devmode ; then |
| @@ -5,8 +5,6 @@ systems: | ||
| # no support for fuse on 14.04 | ||
| - -ubuntu-14.04-64 | ||
| - -ubuntu-14.04-32 | ||
| - # No confinement (AppArmor, Seccomp) available on these systems | ||
| - - -debian-* |
| @@ -1,9 +1,5 @@ | ||
| summary: Ensure that the network-bind interface works | ||
| -systems: |
| @@ -2,8 +2,6 @@ summary: Ensure that the openvswitch interface works. | ||
| systems: | ||
| - -ubuntu-core-* | ||
| - # No confinement (AppArmor, Seccomp) available on these systems | ||
| - - -debian-* |
| @@ -1,9 +1,5 @@ | ||
| summary: Ensure that the process-control interface works. | ||
| -systems: |
| @@ -4,8 +4,6 @@ systems: | ||
| - -ubuntu-core-16-* | ||
| # ppc64el disabled because of https://github.com/snapcore/snapd/issues/2504 | ||
| - -ubuntu-*-ppc64el | ||
| - # No confinement (AppArmor, Seccomp) available on these systems |
zyga
approved these changes
May 9, 2017
Looks good, though the diff is painful to read as it is so repetitive :-)
added some commits
May 5, 2017
Some checks were not successful
|
The code looks great. My main wondering is if should move this new command under |
Good idea. I am open for any of these and will wait for @niemeyer to comment. |
|
I would actually prefer the original approach. It seems much simpler, even in terms of terseness, and also means we know for sure what is being tested where, instead of relying runtime behavior of the system at test. |
niemeyer
approved these changes
May 18, 2017
Simon commented that the point is having tests partially executing, so here is an updated review. LGTM assuming these points are addressed.
| @@ -256,6 +256,7 @@ func sysInfo(c *Command, r *http.Request, user *auth.UserState) Response { | ||
| "os-release": release.ReleaseInfo, | ||
| "on-classic": release.OnClassic, | ||
| "managed": len(users) > 0, | ||
| + "forced-devmode": release.ReleaseInfo.ForceDevMode(), |
niemeyer
May 18, 2017
Contributor
This is a mechanism to get us going but that we want to kill over time. It's also pretty bogus, because lack of apparmor does not mean lack of entire confinement, and definitely doesn't mean devmode. So we shouldn't be making it official by exposing it in a public API.
Here is a suggested alternative: let's have a command named:
$ snap debug confinement
the output should be one of: strict, partial, or none
For the implementation, let's call release.ReleaseInfo.ForceDevMode directly in the client command, instead of exposing it via the API. That buys us time to polish this without promising a public API.
morphis
May 19, 2017
Contributor
OK, will rework this and will introduce a new debug command for the snap binary.
| - # if confinement is supported on the platform we're running. | ||
| - if [[ "$SPREAD_SYSTEM" = debian-* ]]; then | ||
| + if snap forced-devmode; then | ||
| + echo "WARNING: Skipping confinement checks as snapd runs in forced devmode" |
niemeyer
May 18, 2017
Contributor
These warnings may be dropped. They won't go anywhere useful, so it's just adding noise to the test text.
| + expected="^test-snapd-devmode +.* +jailmode" | ||
| + snap list | grep -Pq "$expected" | ||
| + else | ||
| + echo "WARNING: Skipping confinement checks as snapd runs in forced devmode" |
| - echo "When the plug is disconnected" | ||
| - snap disconnect home-consumer:home | ||
| - snap interfaces | grep -Pzq "$DISCONNECTED_PATTERN" | ||
| + if ! snap forced-devmode ; then |
niemeyer
May 18, 2017
Contributor
We can just exit 0 here as done in prior tasks, instead of indenting everything else.
added some commits
May 19, 2017
All checks have passed
Some checks were not successful
Some checks were not successful
codecov-io
commented
May 19, 2017
Codecov Report
@@ Coverage Diff @@
## master #3274 +/- ##
==========================================
+ Coverage 77.59% 77.72% +0.13%
==========================================
Files 364 366 +2
Lines 24980 25445 +465
==========================================
+ Hits 19382 19778 +396
- Misses 3871 3905 +34
- Partials 1727 1762 +35
Continue to review full report at Codecov.
|
morphis commentedMay 5, 2017
•
Edited 1 time
-
morphis
May 5, 2017
No description provided.