error out instead of warning on invalid FUSE mount extra opt

internal/pkg/util/fs/fuse/fuse_mount_linux.go

@@ -132,7 +132,10 @@ func (i *ImageMount) generateCmdArgs() ([]string, error) {
 		}
 	}()
 
-	opts := i.generateMountOpts()
+	opts, err := i.generateMountOpts()
+	if err != nil {
+		return args, err
+	}
 
 	if len(opts) > 0 {
 		args = append(args, "-o", strings.Join(opts, ","))
@@ -144,13 +147,13 @@ func (i *ImageMount) generateCmdArgs() ([]string, error) {
 	return args, nil
 }
 
-func (i ImageMount) generateMountOpts() []string {
+func (i ImageMount) generateMountOpts() ([]string, error) {
 	// TODO: Think through what makes sense for file ownership in FUSE-mounted
 	// images, vis a vis id-mappings and user-namespaces.
 	opts := []string{"uid=0", "gid=0"}
 
 	// Create a map of the extra mount options that have been requested, so we
-	// can weed out attempts to overwrite builtin struct fields.
+	// can catch attempts to overwrite builtin struct fields.
 	extraOptsMap := lo.SliceToMap(i.ExtraOpts, func(s string) (string, mo.Option[string]) {
 		splitted := strings.SplitN(s, "=", 2)
 		if len(splitted) < 2 {
@@ -160,40 +163,43 @@ func (i ImageMount) generateMountOpts() []string {
 		return strings.ToLower(splitted[0]), mo.Some(splitted[1])
 	})
 
-	weedOutMountOpt(&extraOptsMap, "ro")
-	weedOutMountOpt(&extraOptsMap, "rw")
+	if maps.HasKey(extraOptsMap, "ro") {
+		return opts, fmt.Errorf("cannot pass 'ro' as extra FUSE-mount option, as it is handled by an internal field")
+	}
+	if maps.HasKey(extraOptsMap, "rw") {
+		return opts, fmt.Errorf("cannot pass 'rw' as extra FUSE-mount option, as it is handled by an internal field")
+	}
 	if i.Readonly {
 		// Not strictly necessary as will be read-only in assembled overlay,
 		// however this stops any erroneous writes through the stagingDir.
 		opts = append(opts, "ro")
 	}
 
 	// FUSE defaults to nosuid,nodev - attempt to reverse if AllowDev/Setuid requested.
-	weedOutMountOpt(&extraOptsMap, "dev")
+	if maps.HasKey(extraOptsMap, "dev") {
+		return opts, fmt.Errorf("cannot pass 'dev' as extra FUSE-mount option, as it is handled by an internal field")
+	}
 	if i.AllowDev {
 		opts = append(opts, "dev")
 	}
-	weedOutMountOpt(&extraOptsMap, "suid")
+	if maps.HasKey(extraOptsMap, "suid") {
+		return opts, fmt.Errorf("cannot pass 'suid' as extra FUSE-mount option, as it is handled by an internal field")
+	}
 	if i.AllowSetuid {
 		opts = append(opts, "suid")
 	}
 
-	weedOutMountOpt(&extraOptsMap, "allow_other")
+	if maps.HasKey(extraOptsMap, "allow_other") {
+		return opts, fmt.Errorf("cannot pass 'allow_other' as extra FUSE-mount option, as it is handled by an internal field")
+	}
 	if i.AllowOther {
 		opts = append(opts, "allow_other")
 	}
 
 	filteredExtraOpts := lo.MapToSlice(extraOptsMap, rebuildOpt)
 	opts = append(opts, filteredExtraOpts...)
 
-	return opts
-}
-
-func weedOutMountOpt(extraOptsMap *map[string]mo.Option[string], k string) {
-	if maps.HasKey(*extraOptsMap, k) {
-		sylog.Warningf("Passing extra FUSE-mount option %q would override built-in field; ignoring", rebuildOpt(k, (*extraOptsMap)[k]))
-		delete(*extraOptsMap, k)
-	}
+	return opts, nil
 }
 
 func rebuildOpt(k string, v mo.Option[string]) string {

internal/pkg/util/fs/fuse/fuse_mount_linux_test.go

@@ -6,112 +6,35 @@
 package fuse
 
 import (
-	"strings"
 	"testing"
-
-	"github.com/samber/lo"
-	"github.com/sylabs/singularity/v4/pkg/image"
 )
 
-func TestReadonlyOverride(t *testing.T) {
-	m1 := ImageMount{
-		Type:      image.SQUASHFS,
-		Readonly:  true,
-		ExtraOpts: []string{"rw"},
-	}
-
-	opts := m1.generateMountOpts()
-	if lo.ContainsBy(opts, func(s string) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		return (strings.ToLower(splitted[0]) == "rw")
-	}) {
-		t.Errorf("Failed to weed out 'rw' mount option; opts: %#v", opts)
-	}
-
-	m2 := ImageMount{
-		Type:      image.SQUASHFS,
-		Readonly:  false,
-		ExtraOpts: []string{"ro"},
-	}
-
-	opts = m2.generateMountOpts()
-	if lo.ContainsBy(opts, func(s string) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		return (strings.ToLower(splitted[0]) == "ro")
-	}) {
-		t.Errorf("Failed to weed out 'ro' mount option; opts: %#v", opts)
-	}
-}
-
-func TestDevOverride(t *testing.T) {
-	m := ImageMount{
-		Type:      image.SQUASHFS,
-		AllowDev:  false,
-		ExtraOpts: []string{"dev"},
-	}
-
-	opts := m.generateMountOpts()
-	if lo.ContainsBy(opts, func(s string) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		return (strings.ToLower(splitted[0]) == "dev")
-	}) {
-		t.Errorf("Failed to weed out 'dev' mount option; opts: %#v", opts)
-	}
-}
-
-func TestSetuidOverride(t *testing.T) {
-	m := ImageMount{
-		Type:        image.SQUASHFS,
-		AllowSetuid: false,
-		ExtraOpts:   []string{"suid"},
-	}
-
-	opts := m.generateMountOpts()
-	if lo.ContainsBy(opts, func(s string) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		return (strings.ToLower(splitted[0]) == "suid")
-	}) {
-		t.Errorf("Failed to weed out 'suid' mount option; opts: %#v", opts)
-	}
+func TestExtraOptOverrides(t *testing.T) {
+	testOneOverride(t, "rw")
+	testOneOverride(t, "ro")
+	testOneOverride(t, "dev")
+	testOneOverride(t, "suid")
+	testOneOverride(t, "allow_other")
 }
 
-func TestAllowOtherOverride(t *testing.T) {
+func testOneOverride(t *testing.T, s string) {
 	m := ImageMount{
-		Type:       image.SQUASHFS,
-		AllowOther: false,
-		ExtraOpts:  []string{"allow_other"},
+		ExtraOpts: []string{s},
 	}
 
-	opts := m.generateMountOpts()
-	if lo.ContainsBy(opts, func(s string) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		return (strings.ToLower(splitted[0]) == "allow_other")
-	}) {
-		t.Errorf("Failed to weed out 'allow_other' mount option; opts: %#v", opts)
+	opts, err := m.generateMountOpts()
+	if err == nil {
+		t.Errorf("Failed to weed out %q mount option; opts: %#v", s, opts)
 	}
 }
 
 func TestAllOverridesAtOnce(t *testing.T) {
 	m := ImageMount{
-		Type:        image.SQUASHFS,
-		Readonly:    true,
-		AllowDev:    false,
-		AllowSetuid: false,
-		AllowOther:  false,
-		ExtraOpts:   []string{"suid", "allow_other", "rw", "dev"},
+		ExtraOpts: []string{"suid", "allow_other", "rw", "dev"},
 	}
 
-	opts := m.generateMountOpts()
-	offendingOpts := lo.Filter(opts, func(s string, _ int) bool {
-		splitted := strings.SplitN(s, "=", 2)
-		switch splitted[0] {
-		case "rw", "dev", "suid", "allow_other":
-			return true
-		default:
-			return false
-		}
-	})
-	if len(offendingOpts) > 0 {
-		t.Errorf("Failed to properly filter mount options; opts: %#v (offending options: %#v)", opts, offendingOpts)
+	opts, err := m.generateMountOpts()
+	if err == nil {
+		t.Errorf("Failed to properly filter mount options; opts: %#v", opts)
 	}
 }

internal/pkg/util/fs/squashfs/squashfs.go

@@ -21,7 +21,6 @@ func FUSEMount(ctx context.Context, offset uint64, path, mountPath string) (*fus
 		Readonly:   true,
 		SourcePath: filepath.Clean(path),
 		ExtraOpts: []string{
-			"ro",
 			fmt.Sprintf("offset=%d", offset),
 			fmt.Sprintf("uid=%d", os.Getuid()),
 			fmt.Sprintf("gid=%d", os.Getgid()),