Extend NodeUnpublish test to verify cleanup of TargetPath (#336)

The CSI spec states that the SP MUST delete the file or directory that
it created as part of NodeUnpublishVolume. This adds a new scenario to
the sanity test to verify that NodeUnpublishVolume removes TargetPath.

Author: dobsonj

cmd/csi-sanity/main.go

@@ -81,6 +81,8 @@ func main() {
 	stringVar(&config.RemoveTargetPathCmd, "removemountpathcmd", "Command to run for target path removal")
 	stringVar(&config.RemoveStagingPathCmd, "removestagingpathcmd", "Command to run for staging path removal")
 	durationVar(&config.RemovePathCmdTimeout, "removepathcmdtimeout", "Timeout for the commands to remove target and staging paths, in seconds")
+	stringVar(&config.CheckPathCmd, "checkpathcmd", "Command to run to check a given path. It must print 'file', 'directory', 'not_found', or 'other' on stdout.")
+	durationVar(&config.CheckPathCmdTimeout, "checkpathcmdtimeout", "Timeout for the command to check a given path, in seconds")
 	stringVar(&config.SecretsFile, "secrets", "CSI secrets file")
 	stringVar(&config.TestVolumeAccessType, "testvolumeaccesstype", "Volume capability access type, valid values are mount or block")
 	int64Var(&config.TestVolumeSize, "testvolumesize", "Base volume size used for provisioned volumes")

hack/e2e.sh

@@ -101,9 +101,23 @@ echo $targetpath
 rm -rf $@
 ' > custompathremoval.bash
 
-	chmod +x custompathcreation.bash custompathremoval.bash
+	# Create a script for custom target path check.
+	echo '#!/bin/bash
+if [ -f "$1" ]; then
+    echo "file"
+elif [ -d "$1" ]; then
+    echo "directory"
+elif [ -e "$1" ]; then
+    echo "other"
+else
+    echo "not_found"
+fi
+' > custompathcheck.bash
+
 	local creationscriptpath="$PWD/custompathcreation.bash"
 	local removalscriptpath="$PWD/custompathremoval.bash"
+	local checkscriptpath="$PWD/custompathcheck.bash"
+	chmod +x $creationscriptpath $removalscriptpath $checkscriptpath
 
 	CSI_ENDPOINT=$1 ./bin/mock-driver &
 	local pid=$!
@@ -116,7 +130,8 @@ rm -rf $@
 		--csi.createmountpathcmd=$creationscriptpath \
 		--csi.createstagingpathcmd=$creationscriptpath \
 		--csi.removemountpathcmd=$removalscriptpath \
-		--csi.removestagingpathcmd=$removalscriptpath
+		--csi.removestagingpathcmd=$removalscriptpath \
+		--csi.checkpathcmd=$checkscriptpath \
 )
 
 make

pkg/sanity/node.go

@@ -449,6 +449,64 @@ var _ = DescribeSanity("Node Service", func(sc *TestContext) {
 			Expect(ok).To(BeTrue())
 			Expect(serverError.Code()).To(Equal(codes.InvalidArgument), "unexpected error: %s", serverError.Message())
 		})
+
+		It("should remove target path", func() {
+			// This test may break for consumers that are using
+			// custom target path functions if they have not yet
+			// implemented similar functionality to check if the
+			// path exists. Skip this test if there is a custom
+			// command or function provided to create the path,
+			// but not yet provided to check the path.
+			if sc.Config.CreateTargetPathCmd != "" && sc.Config.CheckPathCmd == "" {
+				Skip("CreateTargetPathCmd was set, but CheckPathCmd was not. Please update your testing configuration to enable CheckPathCmd.")
+			}
+			if sc.Config.CreateTargetDir != nil && sc.Config.CheckPath == nil {
+				Skip("CreateTargetDir was set, but CheckPath was not. Please update your testing configuration to enable CheckPath.")
+			}
+
+			name := UniqueString("sanity-node-unpublish-volume")
+			vol := createVolume(name)
+			volid := vol.GetVolume().GetVolumeId()
+			volpath := sc.TargetPath + "/target"
+
+			By("Getting a node id")
+			nid, err := r.NodeGetInfo(
+				context.Background(),
+				&csi.NodeGetInfoRequest{})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(nid).NotTo(BeNil())
+			Expect(nid.GetNodeId()).NotTo(BeEmpty())
+
+			By("Staging and publishing a volume")
+			conpubvol := controllerPublishVolume(name, vol, nid)
+			_ = nodeStageVolume(name, vol, conpubvol)
+			_ = nodePublishVolume(name, vol, conpubvol)
+
+			// Verify that the path exists before calling
+			// NodeUnpublishVolume.
+			By("Checking the target path exists")
+			pa, err := CheckPath(volpath, sc.Config)
+			Expect(err).NotTo(HaveOccurred(), "checking path %q", volpath)
+			Expect(pa).NotTo(Equal(PathIsNotFound), "path %q should have been created by CSI driver and the test config should be enabling testing for that path", volpath)
+
+			By("Unpublishing the volume")
+			_, err = r.NodeUnpublishVolume(
+				context.Background(),
+				&csi.NodeUnpublishVolumeRequest{
+					VolumeId:   volid,
+					TargetPath: volpath,
+				},
+			)
+			Expect(err).NotTo(HaveOccurred())
+
+			// The CSI spec states that the SP MUST delete
+			// the file or directory it created at this path
+			// as part of NodeUnpublishVolume.
+			By("Checking the target path was removed")
+			pa, err = CheckPath(volpath, sc.Config)
+			Expect(err).NotTo(HaveOccurred(), "checking path %q", volpath)
+			Expect(pa).To(Equal(PathIsNotFound), "path %q should have been removed by the CSI driver during NodeUnpublishVolume", volpath)
+		})
 	})
 
 	Describe("NodeStageVolume", func() {

pkg/sanity/sanity.go

@@ -164,6 +164,14 @@ type TestConfig struct {
 	// n > 0: repeat each call n times
 	// NewTestConfig() by default enables idempotency testing.
 	IdempotentCount int
+
+	// CheckPath is a callback function to check whether the given path exists.
+	// If this is not set, then defaultCheckPath will be used instead.
+	CheckPath func(path string) (PathKind, error)
+	// Command to be executed for a customized way to check a given path.
+	CheckPathCmd string
+	// Timeout for the executed command to check a given path.
+	CheckPathCmdTimeout time.Duration
 }
 
 // TestContext gets initialized by the sanity package before each test
@@ -194,6 +202,7 @@ func NewTestConfig() TestConfig {
 		TestVolumeAccessType: "mount",
 		IDGen:                &DefaultIDGenerator{},
 		IdempotentCount:      10,
+		CheckPathCmdTimeout:  10 * time.Second,
 
 		DialOptions:           []grpc.DialOption{grpc.WithInsecure()},
 		ControllerDialOptions: []grpc.DialOption{grpc.WithInsecure()},
@@ -449,3 +458,89 @@ func PseudoUUID() string {
 func UniqueString(prefix string) string {
 	return prefix + uniqueSuffix
 }
+
+// Return codes for CheckPath
+type PathKind string
+
+const (
+	PathIsFile     PathKind = "file"
+	PathIsDir      PathKind = "directory"
+	PathIsNotFound PathKind = "not_found"
+	PathIsOther    PathKind = "other"
+)
+
+// IsPathKind validates that the input string matches one of the defined
+// PathKind values above. If successful, it returns the corresponding
+// PathKind type. Otherwise, it returns an error.
+func IsPathKind(in string) (PathKind, error) {
+	pk := PathKind(in)
+	switch pk {
+	case PathIsFile, PathIsDir, PathIsNotFound, PathIsOther:
+		return pk, nil
+	default:
+		return pk, fmt.Errorf("invalid PathType: %s", pk)
+	}
+}
+
+// defaultCheckPath runs os.Stat against the provided path and returns
+// a code indicating whether it's a file, directory, not found, or other.
+// If an error occurs, it returns an empty string along with the error.
+func defaultCheckPath(path string) (PathKind, error) {
+	var pk PathKind
+	fi, err := os.Stat(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return PathIsNotFound, nil
+		} else {
+			return "", err
+		}
+	}
+	switch mode := fi.Mode(); {
+	case mode.IsRegular():
+		pk = PathIsFile
+	case mode.IsDir():
+		pk = PathIsDir
+	default:
+		pk = PathIsOther
+	}
+	return pk, nil
+}
+
+// CheckPath takes a path parameter and returns a code indicating whether
+// it's a file, directory, not found, or other. This can be done using a
+// custom command, custom function, or by the defaultCheckPath function.
+// If an error occurs, it returns an empty string along with the error.
+func CheckPath(path string, config *TestConfig) (PathKind, error) {
+	if path == "" {
+		return "", fmt.Errorf("path argument must not be empty")
+	}
+	if config == nil {
+		return "", fmt.Errorf("config argument must not be nil")
+	}
+
+	if config.CheckPathCmd != "" {
+		// Check the provided path using the check path command.
+		ctx, cancel := context.WithTimeout(context.Background(), config.CheckPathCmdTimeout)
+		defer cancel()
+
+		cmd := exec.CommandContext(ctx, config.CheckPathCmd, path)
+		cmd.Stderr = os.Stderr
+		out, err := cmd.Output()
+		if err != nil {
+			return "", fmt.Errorf("check path command %s failed: %v", config.CheckPathCmd, err)
+		}
+		// The output of this command is expected to match the value for
+		// PathIsFile, PathIsDir, PathIsNotFound, or PathIsOther.
+		pk, err := IsPathKind(strings.TrimSpace(string(out)))
+		if err != nil {
+			return "", fmt.Errorf("check path command %s failed: %v", config.CheckPathCmd, err)
+		}
+		return pk, nil
+	} else if config.CheckPath != nil {
+		// Check the path using a custom callback function.
+		return config.CheckPath(path)
+	} else {
+		// Use defaultCheckPath if no custom function was provided.
+		return defaultCheckPath(path)
+	}
+}