Merge pull request #742 from kolyshkin/fix-fix

Followup fixes to validator

Author: Mrunal Patel

cmd/runtimetest/main.go

@@ -548,8 +548,22 @@ func (c *complianceTester) validateRootfsPropagation(spec *rspec.Spec) error {
 	}
 	defer os.RemoveAll(targetDir)
 
+	mountErr := unix.Mount("/", targetDir, "", unix.MS_BIND|unix.MS_REC, "")
+	if mountErr == unix.EPERM { //nolint:errcheck // unix errors are bare
+		// This test needs CAP_SYS_ADMIN to perform mounts.
+		// EPERM most probably means it was not granted.
+		c.harness.Skip(1, "unable to perform mount (test requires CAP_SYS_ADMIN)")
+		return nil
+	}
+	if err == nil {
+		defer unix.Unmount(targetDir, unix.MNT_DETACH) //nolint:errcheck
+	}
+
 	switch spec.Linux.RootfsPropagation {
 	case "shared", "slave", "private":
+		if mountErr != nil {
+			return fmt.Errorf("bind-mount / %s: %w", targetDir, err)
+		}
 		mountDir, err := ioutil.TempDir("/", "mount")
 		if err != nil {
 			return err
@@ -568,12 +582,8 @@ func (c *complianceTester) validateRootfsPropagation(spec *rspec.Spec) error {
 		}
 		defer os.Remove(tmpfile.Name())
 
-		if err := unix.Mount("/", targetDir, "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
-			return err
-		}
-		defer unix.Unmount(targetDir, unix.MNT_DETACH) //nolint:errcheck
 		if err := unix.Mount(testDir, mountDir, "", unix.MS_BIND|unix.MS_REC, ""); err != nil {
-			return err
+			return fmt.Errorf("bind-mount %s %s: %w", testDir, mountDir, err)
 		}
 		defer unix.Unmount(mountDir, unix.MNT_DETACH) //nolint:errcheck
 		targetFile := filepath.Join(targetDir, filepath.Join(mountDir, filepath.Base(tmpfile.Name())))
@@ -595,14 +605,12 @@ func (c *complianceTester) validateRootfsPropagation(spec *rspec.Spec) error {
 			)
 		}
 	case "unbindable":
-		err = unix.Mount("/", targetDir, "", unix.MS_BIND|unix.MS_REC, "")
-		if err == syscall.EINVAL {
+		if mountErr == syscall.EINVAL {
 			c.harness.Pass("root propagation is unbindable")
 			return nil
-		} else if err != nil {
-			return err
+		} else if mountErr != nil {
+			return fmt.Errorf("bind-mount / %s: %w", targetDir, err)
 		}
-		defer unix.Unmount(targetDir, unix.MNT_DETACH) //nolint:errcheck
 		c.harness.Fail("root propagation is unbindable")
 		return nil
 	default:

validation/create/create.go

@@ -5,12 +5,12 @@ import (
 	"os/exec"
 	"runtime"
 
+	"github.com/google/uuid"
 	"github.com/mndrix/tap-go"
 	rspecs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/opencontainers/runtime-tools/specerror"
 	"github.com/opencontainers/runtime-tools/validation/util"
-	"github.com/google/uuid"
 )
 
 func main() {
@@ -33,7 +33,7 @@ func main() {
 	if err != nil {
 		util.Fatal(err)
 	}
-	defer r.Clean(true, true)
+	defer r.Clean()
 
 	err = r.SetConfig(&g)
 	if err != nil {

validation/linux_rootfs_propagation/linux_rootfs_propagation.go

@@ -10,9 +10,13 @@ func testLinuxRootPropagation(t *tap.T, propMode string) error {
 	if err != nil {
 		util.Fatal(err)
 	}
-	g.SetupPrivileged(true)
+	// Test case validateRootfsPropagation needs CAP_SYS_ADMIN to perform mounts.
+	g.AddProcessCapability("CAP_SYS_ADMIN")
+	// The generated seccomp profile does not enable mount/umount/umount2 syscalls.
+	g.Config.Linux.Seccomp = nil
+
 	g.SetLinuxRootPropagation(propMode)
-	g.AddAnnotation("TestName", "check root propagation")
+	g.AddAnnotation("TestName", "check root propagation: "+propMode)
 	return util.RuntimeInsideValidate(g, t, nil)
 }
 

validation/util/container.go

@@ -8,7 +8,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"time"
 
 	"github.com/google/uuid"
 	rspecs "github.com/opencontainers/runtime-spec/specs-go"
@@ -175,10 +174,22 @@ func (r *Runtime) Kill(sig string) (err error) {
 	return execWithStderrFallbackToStdout(cmd)
 }
 
-// Delete a container
-func (r *Runtime) Delete() (err error) {
+// Delete removes a (stopped) container.
+func (r *Runtime) Delete() error {
+	return r.del(false)
+}
+
+// ForceDelete removes a container (killing it if necessary).
+func (r *Runtime) ForceDelete() error {
+	return r.del(true)
+}
+
+func (r *Runtime) del(force bool) (err error) {
 	var args []string
 	args = append(args, "delete")
+	if force {
+		args = append(args, "--force")
+	}
 	if r.ID != "" {
 		args = append(args, r.ID)
 	}
@@ -187,28 +198,16 @@ func (r *Runtime) Delete() (err error) {
 	return execWithStderrFallbackToStdout(cmd)
 }
 
-// Clean deletes the container.  If removeBundle is set, the bundle
-// directory is removed after the container is deleted successfully or, if
-// forceRemoveBundle is true, after the deletion attempt regardless of
-// whether it was successful or not.
-func (r *Runtime) Clean(removeBundle bool, forceRemoveBundle bool) {
-	if err := r.Kill("KILL"); err != nil {
-		fmt.Fprintf(os.Stderr, "Clean: Kill: %v", err)
-	}
-	if err := WaitingForStatus(*r, LifecycleStatusStopped, time.Second*10, time.Second/10); err != nil {
-		fmt.Fprintf(os.Stderr, "Clean: %v", err)
-	}
-
-	err := r.Delete()
+// Clean kills and removes the container and its bundle directory.
+func (r *Runtime) Clean() {
+	err := r.ForceDelete()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Clean: Delete: %v", err)
+		fmt.Fprintln(os.Stderr, "Clean: Delete: ", err)
 	}
 
-	if removeBundle && (err == nil || forceRemoveBundle) {
-		err := os.RemoveAll(r.bundleDir())
-		if err != nil {
-			fmt.Fprintf(os.Stderr, "Clean: %v", err)
-		}
+	err = os.RemoveAll(r.bundleDir())
+	if err != nil {
+		fmt.Fprintln(os.Stderr, "Clean: ", err)
 	}
 }
 

validation/util/test.go

@@ -11,18 +11,16 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/uuid"
 	"github.com/mndrix/tap-go"
 	"github.com/mrunalp/fileutils"
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/opencontainers/runtime-tools/specerror"
-	"github.com/google/uuid"
 )
 
-var (
-	// RuntimeCommand is the default runtime command.
-	RuntimeCommand = "runc"
-)
+// RuntimeCommand is the default runtime command.
+var RuntimeCommand = "runc"
 
 // LifecycleAction defines the phases will be called.
 type LifecycleAction int
@@ -194,7 +192,7 @@ func RuntimeInsideValidate(g *generate.Generator, t *tap.T, f PreFunc) (err erro
 		os.RemoveAll(bundleDir)
 		return err
 	}
-	defer r.Clean(true, true)
+	defer r.Clean()
 	err = r.SetConfig(g)
 	if err != nil {
 		return err
@@ -271,7 +269,7 @@ func RuntimeOutsideValidate(g *generate.Generator, t *tap.T, f AfterFunc) error
 		os.RemoveAll(bundleDir)
 		return err
 	}
-	defer r.Clean(true, true)
+	defer r.Clean()
 	err = r.SetConfig(g)
 	if err != nil {
 		return err