Validate fields mount name and mount path in Dataset

Signed-off-by: ZhangXiaozheng <[email protected]>
fluid-cloudnative · Jan 11, 2024 · d4de617 · d4de617
1 parent 453b086
commit d4de617
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 19 deletions.
diff --git a/pkg/common/env_names.go b/pkg/common/env_names.go
@@ -28,4 +28,6 @@ const (
 	EnvEnableRuntimeInfoCache = "ENABLE_RUNTIMEINFO_CACHE"
 
 	EnvRuntimeInfoCacheTTL = "RUNTIMEINFO_CACHE_TTL"
+
+	EnvEnableMountValidation = "ENABLE_MOUNT_VALIDATION"
 )
diff --git a/pkg/controllers/v1alpha1/dataset/dataset_controller.go b/pkg/controllers/v1alpha1/dataset/dataset_controller.go
@@ -41,6 +41,7 @@ import (
 
 	datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1"
 	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	fluidvalidation "github.com/fluid-cloudnative/fluid/pkg/utils/validation"
 )
 
 const (
@@ -124,14 +125,44 @@ func (r *DatasetReconciler) reconcileDataset(ctx reconcileRequestContext, needRe
 	log := ctx.Log.WithName("reconcileDataset")
 	log.V(1).Info("process the dataset", "dataset", ctx.Dataset)
 
-	// 0. Validate name is prefixed with a number such as "20-hbase".
+	// 0.0 Validate name is prefixed with a number such as "20-hbase".
 	if errs := validation.IsDNS1035Label(ctx.Dataset.ObjectMeta.Name); len(ctx.Dataset.ObjectMeta.Name) > 0 && len(errs) > 0 {
 		err := field.Invalid(field.NewPath("metadata").Child("name"), ctx.Dataset.ObjectMeta.Name, strings.Join(errs, ","))
 		ctx.Log.Error(err, "Failed to create dataset", "DatasetCreateError", ctx)
 		r.Recorder.Eventf(&ctx.Dataset, v1.EventTypeWarning, common.ErrorCreateDataset, "Failed to create dataset because err: %v", err)
 		return utils.RequeueIfError(err)
 	}
 
+	// 0.1 Validate the mount name and mount path
+	// Users can set the environment variable to 'false' to disable this validation
+	// Default is true
+	if utils.GetBoolValueFromEnv(common.EnvEnableMountValidation, true) {
+		var validationErr error = nil
+		for _, mount := range ctx.Dataset.Spec.Mounts {
+			// The field mount.Name and mount.Path is optional
+			// Empty name or path is allowed
+			if len(mount.Name) != 0 {
+				// If users set the mount.Name, it should comply with the DNS1035 rule.
+				if errs := validation.IsDNS1035Label(mount.Name); len(errs) > 0 {
+					validationErr = field.Invalid(field.NewPath("spec").Child("mounts").Child("name"), mount.Name, strings.Join(errs, ","))
+					break
+				}
+			}
+			if len(mount.Path) != 0 {
+				// If users set the mount.Path, check it.
+				if err := fluidvalidation.IsValidMountPath(mount.Path); err != nil {
+					validationErr = field.Invalid(field.NewPath("spec").Child("mounts").Child("path"), mount.Path, err.Error())
+					break
+				}
+			}
+		}
+		if validationErr != nil {
+			ctx.Log.Error(validationErr, "Failed to create dataset", "DatasetCreateError", ctx)
+			r.Recorder.Eventf(&ctx.Dataset, v1.EventTypeWarning, common.ErrorCreateDataset, "Failed to create dataset because err: %v", validationErr)
+			return utils.RequeueIfError(validationErr)
+		}
+	}
+
 	// 1. Check if need to delete dataset
 	if utils.HasDeletionTimestamp(ctx.Dataset.ObjectMeta) {
 		return r.reconcileDatasetDeletion(ctx)

diff --git a/pkg/utils/validation/validation.go b/pkg/utils/validation/validation.go
@@ -24,6 +24,22 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation"
 )
 
+const invalidPartOfPathErrMsg string = "every part of the path shuold follow the relaxed DNS (RFC 1123) rule which additionally allows upper case alphabetic character and character '_'"
+
+func isValidPartsOfPath(partsOfPath []string) error {
+	for _, part := range partsOfPath {
+		// Convert characters to lowercase and replace underscores with hyphens
+		part = strings.ToLower(part)
+		part = strings.Replace(part, "_", "-", -1)
+
+		// If the component fails the DNS 1123 conformity test, the function returns an error
+		if len(validation.IsDNS1123Label(part)) > 0 {
+			return fmt.Errorf(invalidPartOfPathErrMsg)
+		}
+	}
+	return nil
+}
+
 const invalidMountRootErrMsgFmt string = "invalid mount root path '%s': %s"
 
 func IsValidMountRoot(path string) error {
@@ -37,18 +53,32 @@ func IsValidMountRoot(path string) error {
 	// The path is an absolute path and to avoid an empty part, we omit the first '/'
 	parts := strings.Split(filepath.Clean(path)[1:], string(filepath.Separator))
 
-	for _, part := range parts {
-		// Convert characters to lowercase and replace underscores with hyphens
-		part = strings.ToLower(part)
-		part = strings.Replace(part, "_", "-", -1)
+	// Validate every part of the path
+	if err := isValidPartsOfPath(parts); err != nil {
+		return fmt.Errorf(invalidMountRootErrMsgFmt, path, err.Error())
+	}
 
-		// If the component fails the DNS 1123 conformity test, the function returns an error
-		errs := validation.IsDNS1123Label(part)
-		if len(errs) > 0 {
-			return fmt.Errorf(invalidMountRootErrMsgFmt, path, "every directory name in the mount root path shuold follow the relaxed DNS (RFC 1123) rule which additionally allows upper case alphabetic character and character '_'")
-		}
+	// If all components pass the check, the function returns nil
+	return nil
+}
+
+func IsValidMountPath(path string) error {
+	if len(path) == 0 {
+		return fmt.Errorf("the mount path is empty")
+	}
+	// Normalize the path and split it into components
+	path = filepath.Clean(path)
+	if filepath.IsAbs(path) {
+		// The path is an absolute path and to avoid an empty part, we omit the first '/'
+		path = path[1:]
+	}
+	parts := strings.Split(path, string(filepath.Separator))
+
+	// Validate every part of the path
+	if err := isValidPartsOfPath(parts); err != nil {
+		return err
 	}
 
-	// If all components pass the DNS 1123 check, the function returns nil
+	// If all components pass the check, the function returns nil
 	return nil
 }
diff --git a/pkg/utils/validation/validation_test.go b/pkg/utils/validation/validation_test.go
@@ -18,11 +18,10 @@ package validation
 
 import (
 	"fmt"
-	"path/filepath"
 	"testing"
 )
 
-func TestIsSafePathWithSafePath(t *testing.T) {
+func TestIsValidMountRootWithValidPath(t *testing.T) {
 
 	type testCase struct {
 		name  string
@@ -41,16 +40,14 @@ func TestIsSafePathWithSafePath(t *testing.T) {
 	}
 
 	for _, test := range testCases {
-		tt := filepath.Clean(test.input)
-		print(tt)
 		got := IsValidMountRoot(test.input)
 		if got != nil {
 			t.Errorf("testcase %s failed, expect no error happened, but got an error: %s", test.name, got.Error())
 		}
 	}
 }
 
-func TestIsSafePathWithInvalidPath(t *testing.T) {
+func TestIsValidMountRootWithInvalidPath(t *testing.T) {
 
 	type testCase struct {
 		name   string
@@ -62,17 +59,17 @@ func TestIsSafePathWithInvalidPath(t *testing.T) {
 		{
 			name:   "invalidPath-1",
 			input:  "/$test/alluxio/default/hbase",
-			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/$test/alluxio/default/hbase", "every directory name in the mount root path shuold follow the relaxed DNS (RFC 1123) rule which additionally allows upper case alphabetic character and character '_'"),
+			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/$test/alluxio/default/hbase", invalidPartOfPathErrMsg),
 		},
 		{
 			name:   "invalidPath-2",
 			input:  "/test/(alluxio)/default/hbase",
-			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/test/(alluxio)/default/hbase", "every directory name in the mount root path shuold follow the relaxed DNS (RFC 1123) rule which additionally allows upper case alphabetic character and character '_'"),
+			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/test/(alluxio)/default/hbase", invalidPartOfPathErrMsg),
 		},
 		{
 			name:   "invalidPath-3",
 			input:  "/test/alluxio/def;ault/hbase",
-			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/test/alluxio/def;ault/hbase", "every directory name in the mount root path shuold follow the relaxed DNS (RFC 1123) rule which additionally allows upper case alphabetic character and character '_'"),
+			expect: fmt.Errorf(invalidMountRootErrMsgFmt, "/test/alluxio/def;ault/hbase", invalidPartOfPathErrMsg),
 		},
 		{
 			name:   "invalidPath-4",
@@ -96,3 +93,74 @@ func TestIsSafePathWithInvalidPath(t *testing.T) {
 		}
 	}
 }
+
+func TestIsValidMountPathWithValidPath(t *testing.T) {
+	type testCase struct {
+		name  string
+		input string
+	}
+
+	testCases := []testCase{
+		{
+			name:  "validPath-1",
+			input: "/runtime-mnt//alluxio/default/hbase",
+		},
+		{
+			name:  "validPath-2",
+			input: "/opt/20-Runtime-Mnt_1/./alluxio/default/hbase",
+		},
+		{
+			name:  "validPath-3",
+			input: "runtime-mnt/test",
+		},
+	}
+
+	for _, test := range testCases {
+		got := IsValidMountPath(test.input)
+		if got != nil {
+			t.Errorf("testcase %s failed, expect no error happened, but got an error: %s", test.name, got.Error())
+		}
+	}
+}
+
+func TestIsValidMountPathWithInvalidPath(t *testing.T) {
+
+	type testCase struct {
+		name   string
+		input  string
+		expect error
+	}
+
+	testCases := []testCase{
+		{
+			name:   "invalidPath-1",
+			input:  "/$test/alluxio/default/hbase",
+			expect: fmt.Errorf(invalidPartOfPathErrMsg),
+		},
+		{
+			name:   "invalidPath-2",
+			input:  "/test/(alluxio)/default/hbase",
+			expect: fmt.Errorf(invalidPartOfPathErrMsg),
+		},
+		{
+			name:   "invalidPath-3",
+			input:  "/test/alluxio/def;ault/hbase",
+			expect: fmt.Errorf(invalidPartOfPathErrMsg),
+		},
+		{
+			name:   "invalidPath-4",
+			input:  "",
+			expect: fmt.Errorf("the mount path is empty"),
+		},
+	}
+
+	for _, test := range testCases {
+		got := IsValidMountPath(test.input)
+		if got == nil {
+			t.Errorf("testcase %s failed, expect an error happened, but got no error", test.name)
+		}
+		if got.Error() != test.expect.Error() {
+			t.Errorf("testcase %s failed, expect error: %v, but got error: %v", test.name, test.expect, got)
+		}
+	}
+}