Error out if spec is invalid

Closes #66

PiperOrigin-RevId: 202496258
Change-Id: Ib9287c5bf1279ffba1db21ebd9e6b59305cddf34

3 files changed, 138 insertions, 3 deletions

diff --git a/runsc/specutils/BUILD b/runsc/specutils/BUILD
index 1b6d265bc..34c952bdf 100644
--- a/runsc/specutils/BUILD
+++ b/runsc/specutils/BUILD
@@ -22,4 +22,5 @@ go_test(
     size = "small",
     srcs = ["specutils_test.go"],
     embed = [":specutils"],
+    deps = ["@com_github_opencontainers_runtime-spec//specs-go:go_default_library"],
 )
diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go
index c552111f2..0d9e09e9d 100644
--- a/runsc/specutils/specutils.go
+++ b/runsc/specutils/specutils.go
@@ -47,10 +47,28 @@ func LogSpec(spec *specs.Spec) {
 
 // ValidateSpec validates that the spec is compatible with runsc.
 func ValidateSpec(spec *specs.Spec) error {
+	// Mandatory fields.
 	if spec.Process == nil {
-		return fmt.Errorf("Process must be defined")
+		return fmt.Errorf("Spec.Process must be defined: %+v", spec)
 	}
-	if spec.Process.SelinuxLabel != "" {
+	if len(spec.Process.Args) == 0 {
+		return fmt.Errorf("Spec.Process.Arg must be defined: %+v", spec.Process)
+	}
+	if spec.Root == nil {
+		return fmt.Errorf("Spec.Root must be defined: %+v", spec)
+	}
+	if len(spec.Root.Path) == 0 {
+		return fmt.Errorf("Spec.Root.Path must be defined: %+v", spec.Root)
+	}
+
+	// Unsupported fields.
+	if spec.Solaris != nil {
+		return fmt.Errorf("Spec.Solaris is not supported: %+v", spec)
+	}
+	if spec.Windows != nil {
+		return fmt.Errorf("Spec.Windows is not supported: %+v", spec)
+	}
+	if len(spec.Process.SelinuxLabel) != 0 {
 		return fmt.Errorf("SELinux is not supported: %s", spec.Process.SelinuxLabel)
 	}
 
@@ -64,7 +82,7 @@ func ValidateSpec(spec *specs.Spec) error {
 		log.Warningf("Seccomp spec is being ignored")
 	}
 
-	// 2 annotations are use by containerd to support multi-container pods.
+	// Two annotations are use by containerd to support multi-container pods.
 	//   "io.kubernetes.cri.container-type"
 	//   "io.kubernetes.cri.sandbox-id"
 	containerType, hasContainerType := spec.Annotations[ContainerdContainerTypeAnnotation]
@@ -98,6 +116,9 @@ func ReadSpec(bundleDir string) (*specs.Spec, error) {
 	if err := json.Unmarshal(specBytes, &spec); err != nil {
 		return nil, fmt.Errorf("error unmarshaling spec from file %q: %v\n %s", specFile, err, string(specBytes))
 	}
+	if err := ValidateSpec(&spec); err != nil {
+		return nil, err
+	}
 	return &spec, nil
 }
 
diff --git a/runsc/specutils/specutils_test.go b/runsc/specutils/specutils_test.go
index ef293e608..959be3af3 100644
--- a/runsc/specutils/specutils_test.go
+++ b/runsc/specutils/specutils_test.go
@@ -20,6 +20,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 func TestWaitForReadyHappy(t *testing.T) {
@@ -94,3 +96,114 @@ func TestWaitForReadyTimeout(t *testing.T) {
 	}
 	cmd.Process.Kill()
 }
+
+func TestSpecInvalid(t *testing.T) {
+	for _, test := range []struct {
+		name  string
+		spec  specs.Spec
+		error string
+	}{
+		{
+			name: "valid",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "",
+		},
+		{
+			name: "valid+warning",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+					// This is normally set by docker and will just cause warnings to be logged.
+					ApparmorProfile: "someprofile",
+				},
+				// This is normally set by docker and will just cause warnings to be logged.
+				Linux: &specs.Linux{Seccomp: &specs.LinuxSeccomp{}},
+			},
+			error: "",
+		},
+		{
+			name: "no root",
+			spec: specs.Spec{
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "empty root",
+			spec: specs.Spec{
+				Root: &specs.Root{},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "no process",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "empty args",
+			spec: specs.Spec{
+				Root:    &specs.Root{Path: "/"},
+				Process: &specs.Process{},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "selinux",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args:         []string{"/bin/true"},
+					SelinuxLabel: "somelabel",
+				},
+			},
+			error: "is not supported",
+		},
+		{
+			name: "solaris",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+				Solaris: &specs.Solaris{},
+			},
+			error: "is not supported",
+		},
+		{
+			name: "windows",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+				Windows: &specs.Windows{},
+			},
+			error: "is not supported",
+		},
+	} {
+		err := ValidateSpec(&test.spec)
+		if len(test.error) == 0 {
+			if err != nil {
+				t.Errorf("ValidateSpec(%q) failed, err: %v", test.name, err)
+			}
+		} else {
+			if err == nil || !strings.Contains(err.Error(), test.error) {
+				t.Errorf("ValidateSpec(%q) wrong error, got: %v, want: .*%s.*", test.name, err, test.error)
+			}
+		}
+	}
+}