strict staging and target directory

The CSI spec requires the CO (= csi-sanity) to create the staging
directory and the *parent* of the target
directory. Kubernetes (kubernetes/kubernetes#75535)
and csi-sanit have handled the target directory incorrectly by
creating it.

Now directory handling is standard-compliant and more strict, to catch
mistakes:
- the specified target directory is treated as the parent which gets
  created by csi-sanity, while the actual NodePublishVolume call then
  specifies an entry inside that directory; this was done to keep the
  code simple (handling both directories is the same) and to minimize
  API changes
- os.Mkdir and os.Remove are used instead of the more forgiving
  os.MkdirAll and os.RemoveAll; as a result, both directories
  now must be different (which is what they should have been in the
  anyway, it just happened to work when they were identical)
- the mock driver checks for non-existence of the target directory

cmd/csi-sanity/sanity_test.go

@@ -38,8 +38,8 @@ func init() {
 	flag.StringVar(&config.Address, prefix+"endpoint", "", "CSI endpoint")
 	flag.StringVar(&config.ControllerAddress, prefix+"controllerendpoint", "", "CSI controller endpoint")
 	flag.BoolVar(&version, prefix+"version", false, "Version of this program")
-	flag.StringVar(&config.TargetPath, prefix+"mountdir", os.TempDir()+"/csi", "Mount point for NodePublish")
-	flag.StringVar(&config.StagingPath, prefix+"stagingdir", os.TempDir()+"/csi", "Mount point for NodeStage if staging is supported")
+	flag.StringVar(&config.TargetPath, prefix+"mountdir", os.TempDir()+"/csi-mount", "Mount point for NodePublish")
+	flag.StringVar(&config.StagingPath, prefix+"stagingdir", os.TempDir()+"/csi-staging", "Mount point for NodeStage if staging is supported")
 	flag.StringVar(&config.CreateTargetPathCmd, prefix+"createmountpathcmd", "", "Command to run for target path creation")
 	flag.StringVar(&config.CreateStagingPathCmd, prefix+"createstagingpathcmd", "", "Command to run for staging path creation")
 	flag.IntVar(&config.CreatePathCmdTimeout, prefix+"createpathcmdtimeout", 10, "Timeout for the commands to create target and staging paths, in seconds")

hack/_apitest/api_test.go

@@ -9,8 +9,8 @@ import (
 
 func TestMyDriver(t *testing.T) {
 	config := &sanity.Config{
-		TargetPath:                os.TempDir() + "/csi",
-		StagingPath:               os.TempDir() + "/csi",
+		TargetPath:                os.TempDir() + "/csi-target",
+		StagingPath:               os.TempDir() + "/csi-staging",
 		Address:                   "/tmp/e2e-csi-sanity.sock",
 		TestNodeVolumeAttachLimit: true,
 	}

hack/_embedded/embedded_test.go

@@ -26,8 +26,8 @@ var _ = AfterSuite(func() {})
 var _ = Describe("MyCSIDriver", func() {
 	Context("Config A", func() {
 		config := &sanity.Config{
-			TargetPath:                os.TempDir() + "/csi",
-			StagingPath:               os.TempDir() + "/csi",
+			TargetPath:                os.TempDir() + "/csi-target",
+			StagingPath:               os.TempDir() + "/csi-staging",
 			Address:                   "/tmp/e2e-csi-sanity.sock",
 			TestNodeVolumeAttachLimit: true,
 		}

mock/service/node.go

@@ -136,7 +136,7 @@ func (s *service) NodePublishVolume(
 		return nil, status.Error(codes.InvalidArgument, "Volume Capability cannot be empty")
 	}
 
-	if err := checkTargetExists(req.TargetPath); err != nil {
+	if err := checkTargetNotExists(req.TargetPath); err != nil {
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 
@@ -353,8 +353,24 @@ func (s *service) NodeGetVolumeStats(ctx context.Context,
 // does not exists.
 func checkTargetExists(targetPath string) error {
 	_, err := os.Stat(targetPath)
-	if err != nil && os.IsNotExist(err) {
+	if err == nil {
+		return nil
+	}
+	if os.IsNotExist(err) {
 		return status.Errorf(codes.Internal, "target path %s does not exists", targetPath)
 	}
-	return nil
+	return status.Errorf(codes.Internal, "stat target path %s: %s", targetPath, err)
+}
+
+// checkTargetNotExists checks if a given path does not exist and returns error if the path
+// does exist.
+func checkTargetNotExists(targetPath string) error {
+	_, err := os.Stat(targetPath)
+	if err == nil {
+		return status.Errorf(codes.Internal, "target path %s does exist", targetPath)
+	}
+	if os.IsNotExist(err) {
+		return nil
+	}
+	return status.Errorf(codes.Internal, "stat target path %s: %s", targetPath, err)
 }

pkg/sanity/node.go

@@ -186,7 +186,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 				context.Background(),
 				&csi.NodePublishVolumeRequest{
 					VolumeId:   "id",
-					TargetPath: sc.targetPath,
+					TargetPath: sc.targetPath + "/target",
 					Secrets:    sc.Secrets.NodePublishVolumeSecret,
 				},
 			)
@@ -534,7 +534,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 				context.Background(),
 				&csi.NodePublishVolumeRequest{
 					VolumeId:          vol.GetVolume().GetVolumeId(),
-					TargetPath:        sc.targetPath,
+					TargetPath:        sc.targetPath + "/target",
 					StagingTargetPath: stagingPath,
 					VolumeCapability: &csi.VolumeCapability{
 						AccessType: &csi.VolumeCapability_Mount{
@@ -573,7 +573,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 				context.Background(),
 				&csi.NodeUnpublishVolumeRequest{
 					VolumeId:   vol.GetVolume().GetVolumeId(),
-					TargetPath: sc.targetPath,
+					TargetPath: sc.targetPath + "/target",
 				})
 			Expect(err).NotTo(HaveOccurred())
 			Expect(nodeunpubvol).NotTo(BeNil())
@@ -718,7 +718,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 			context.Background(),
 			&csi.NodePublishVolumeRequest{
 				VolumeId:          vol.GetVolume().GetVolumeId(),
-				TargetPath:        sc.targetPath,
+				TargetPath:        sc.targetPath + "/target",
 				StagingTargetPath: stagingPath,
 				VolumeCapability: &csi.VolumeCapability{
 					AccessType: &csi.VolumeCapability_Mount{
@@ -743,7 +743,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 				context.Background(),
 				&csi.NodeGetVolumeStatsRequest{
 					VolumeId:   vol.GetVolume().GetVolumeId(),
-					VolumePath: sc.targetPath,
+					VolumePath: sc.targetPath + "/target",
 				},
 			)
 			Expect(err).ToNot(HaveOccurred())
@@ -756,7 +756,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) {
 			context.Background(),
 			&csi.NodeUnpublishVolumeRequest{
 				VolumeId:   vol.GetVolume().GetVolumeId(),
-				TargetPath: sc.targetPath,
+				TargetPath: sc.targetPath + "/target",
 			})
 		Expect(err).NotTo(HaveOccurred())
 		Expect(nodeunpubvol).NotTo(BeNil())

pkg/sanity/sanity.go

@@ -53,8 +53,14 @@ type CSISecrets struct {
 // Config provides the configuration for the sanity tests. It
 // needs to be initialized by the user of the sanity package.
 type Config struct {
-	TargetPath        string
-	StagingPath       string
+	// TargetPath is the *parent* directory for NodePublishVolumeRequest.target_path.
+	// It gets created and removed by csi-sanity.
+	TargetPath string
+
+	// StagingPath is the NodeStageVolumeRequest.staging_target_path.
+	// It gets created and removed by csi-sanity.
+	StagingPath string
+
 	Address           string
 	ControllerAddress string
 	SecretsFile       string
@@ -70,13 +76,30 @@ type Config struct {
 	// directories. Returns the new paths for mount and staging.
 	// If not defined, directories are created in the default way at TargetPath
 	// and StagingPath on the host.
+	//
+	// Both functions can replace the suggested path. What the test then uses
+	// is the path returned by them.
+	//
+	// Note that target and staging directory have different
+	// semantics in the CSI spec: for NodeStateVolume,
+	// CreateTargetDir must create the directory and return the
+	// full path to it. For NodePublishVolume, CreateStagingDir
+	// must create the *parent* directory of `path` (or some other
+	// directory) and return the full path for an entry inside
+	// that created directory.
 	CreateTargetDir  func(path string) (string, error)
 	CreateStagingDir func(path string) (string, error)
 
 	// Callback functions to customize the removal of the target and staging
 	// directories.
 	// If not defined, directories are removed in the default way at TargetPath
 	// and StagingPath on the host.
+	//
+	// Both functions are passed the actual paths as used during the test.
+	//
+	// Note that RemoveTargetPath only needs to remove the *parent* of the
+	// given path. The CSI driver should have removed the entry at that path
+	// already.
 	RemoveTargetPath  func(path string) error
 	RemoveStagingPath func(path string) error
 
@@ -253,20 +276,11 @@ func createMountTargetLocation(targetPath string, createPathCmd string, customCr
 		}
 		newTargetPath = newpath
 	} else {
-		// Create the target path using mkdir.
-		fileInfo, err := os.Stat(targetPath)
-		if err != nil {
-			if !os.IsNotExist(err) {
-				return "", err
-			}
-			if err := os.MkdirAll(targetPath, 0755); err != nil {
-				return "", err
-			}
-			return targetPath, nil
-		}
-
-		if !fileInfo.IsDir() {
-			return "", fmt.Errorf("Target location %s is not a directory", targetPath)
+		// Create the target path. Only the directory itself
+		// and not its parents get created, and it is an error
+		// if the directory already exists.
+		if err := os.Mkdir(targetPath, 0755); err != nil {
+			return "", err
 		}
 		newTargetPath = targetPath
 	}
@@ -297,7 +311,8 @@ func removeMountTargetLocation(targetPath string, removePathCmd string, customRe
 			return err
 		}
 	} else {
-		return os.RemoveAll(targetPath)
+		// It's an error if the directory is not empty by now.
+		return os.Remove(targetPath)
 	}
 	return nil
 }