From 96bc3756b0554d5af0d6264dcea22f2833b3e6ca Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Fri, 14 Apr 2023 10:53:17 -0700
Subject: [PATCH] Ignore all annotations for a given volume if any required
 field is missing.

Mount annotations require that `share`, `type` and `source` fields are all
specified via annotations. If any one is missing, then all the remaining
annotations for a given volume should be ignored. Earlier we would error out.
Now we just log a warning.

PiperOrigin-RevId: 524332461
---
 runsc/boot/mount_hints.go      | 12 ++---
 runsc/boot/mount_hints_test.go | 97 ++++++++++++++++------------------
 2 files changed, 50 insertions(+), 59 deletions(-)

diff --git a/runsc/boot/mount_hints.go b/runsc/boot/mount_hints.go
index 373a84a2d..61a076350 100644
--- a/runsc/boot/mount_hints.go
+++ b/runsc/boot/mount_hints.go
@@ -107,14 +107,10 @@ func newPodMountHints(spec *specs.Spec) (*podMountHints, error) {
 	// Validate all the parsed hints.
 	for name, m := range mnts {
 		log.Infof("Mount annotation found, name: %s, source: %q, type: %s, share: %v", name, m.mount.Source, m.mount.Type, m.share)
-		if m.share == invalid {
-			return nil, fmt.Errorf("share field for %q has not been set", m.name)
-		}
-		if len(m.mount.Source) == 0 {
-			return nil, fmt.Errorf("source field for %q has not been set", m.name)
-		}
-		if len(m.mount.Type) == 0 {
-			return nil, fmt.Errorf("type field for %q has not been set", m.name)
+		if m.share == invalid || len(m.mount.Source) == 0 || len(m.mount.Type) == 0 {
+			log.Warningf("ignoring mount annotations for %q because of missing required field(s)", name)
+			delete(mnts, name)
+			continue
 		}
 
 		// Check for duplicate mount sources.
diff --git a/runsc/boot/mount_hints_test.go b/runsc/boot/mount_hints_test.go
index 16dc83d12..321fcb2b6 100644
--- a/runsc/boot/mount_hints_test.go
+++ b/runsc/boot/mount_hints_test.go
@@ -96,57 +96,6 @@ func TestPodMountHintsErrors(t *testing.T) {
 			},
 			error: "invalid mount name",
 		},
-		{
-			name: "missing source",
-			annotations: map[string]string{
-				MountPrefix + "mount1.type":  "tmpfs",
-				MountPrefix + "mount1.share": "pod",
-			},
-			error: "source field",
-		},
-		{
-			name: "missing type",
-			annotations: map[string]string{
-				MountPrefix + "mount1.source": "foo",
-				MountPrefix + "mount1.share":  "pod",
-			},
-			error: "type field",
-		},
-		{
-			name: "missing share",
-			annotations: map[string]string{
-				MountPrefix + "mount1.source": "foo",
-				MountPrefix + "mount1.type":   "tmpfs",
-			},
-			error: "share field",
-		},
-		{
-			name: "invalid source",
-			annotations: map[string]string{
-				MountPrefix + "mount1.source": "",
-				MountPrefix + "mount1.type":   "tmpfs",
-				MountPrefix + "mount1.share":  "pod",
-			},
-			error: "source field for \"mount1\" has not been set",
-		},
-		{
-			name: "invalid type",
-			annotations: map[string]string{
-				MountPrefix + "mount1.source": "foo",
-				MountPrefix + "mount1.type":   "invalid-type",
-				MountPrefix + "mount1.share":  "pod",
-			},
-			error: "type field for \"mount1\" has not been set",
-		},
-		{
-			name: "invalid share",
-			annotations: map[string]string{
-				MountPrefix + "mount1.source": "foo",
-				MountPrefix + "mount1.type":   "tmpfs",
-				MountPrefix + "mount1.share":  "invalid-share",
-			},
-			error: "share field for \"mount1\" has not been set",
-		},
 		{
 			name: "duplicate source",
 			annotations: map[string]string{
@@ -174,6 +123,52 @@ func TestPodMountHintsErrors(t *testing.T) {
 	}
 }
 
+// Tests that when a required mount annotation is missing, the entire mount
+// hint is omitted and ignored.
+func TestPodMountHintsIgnore(t *testing.T) {
+	for _, tst := range []struct {
+		name        string
+		annotations map[string]string
+	}{
+		{
+			name: "invalid source",
+			annotations: map[string]string{
+				MountPrefix + "mount1.source": "",
+				MountPrefix + "mount1.type":   "tmpfs",
+				MountPrefix + "mount1.share":  "pod",
+			},
+		},
+		{
+			name: "invalid type",
+			annotations: map[string]string{
+				MountPrefix + "mount1.source": "foo",
+				MountPrefix + "mount1.type":   "invalid",
+				MountPrefix + "mount1.share":  "pod",
+			},
+		},
+		{
+			name: "invalid share",
+			annotations: map[string]string{
+				MountPrefix + "mount1.source": "foo",
+				MountPrefix + "mount1.type":   "tmpfs",
+				MountPrefix + "mount1.share":  "invalid",
+			},
+		},
+	} {
+		t.Run(tst.name, func(t *testing.T) {
+			spec := &specs.Spec{Annotations: tst.annotations}
+			podHints, err := newPodMountHints(spec)
+			if err != nil {
+				t.Errorf("newPodMountHints() failed: %v", err)
+			} else if podHints != nil {
+				if hint, ok := podHints.mounts["mount1"]; ok {
+					t.Errorf("hint was provided when it should have been omitted: %+v", hint)
+				}
+			}
+		})
+	}
+}
+
 func TestHintsCheckCompatible(t *testing.T) {
 	for _, tc := range []struct {
 		name        string