From 1cb9e75ec2c05ff40aaee8bada43675d77c07163 Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Thu, 23 Jul 2020 15:52:22 -0400
Subject: [PATCH] csi: NodePublish should not create target_path, only its
 parent dir (#8505)

The NodePublish workflow currently creates the target path and its parent
directory. However, the CSI specification says that the CO shall ensure the
parent directory of the target path exists, and that the SP shall place the
block device or mounted directory at the target path. Much of our testing has
been with CSI plugins that are more forgiving, but our behavior breaks
spec-compliant CSI plugins.

This changeset ensures we only create the parent directory.
---
 client/pluginmanager/csimanager/volume.go | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/client/pluginmanager/csimanager/volume.go b/client/pluginmanager/csimanager/volume.go
index 4f882a2a7d0e..9f6a5cf2e153 100644
--- a/client/pluginmanager/csimanager/volume.go
+++ b/client/pluginmanager/csimanager/volume.go
@@ -67,7 +67,11 @@ func (v *volumeManager) stagingDirForVolume(root string, volID string, usage *Us
 	return filepath.Join(root, StagingDirName, volID, usage.ToFS())
 }
 
-func (v *volumeManager) allocDirForVolume(root string, volID, allocID string, usage *UsageOptions) string {
+func (v *volumeManager) allocDirForVolume(root string, volID, allocID string) string {
+	return filepath.Join(root, AllocSpecificDirName, allocID, volID)
+}
+
+func (v *volumeManager) targetForVolume(root string, volID, allocID string, usage *UsageOptions) string {
 	return filepath.Join(root, AllocSpecificDirName, allocID, volID, usage.ToFS())
 }
 
@@ -103,21 +107,22 @@ func (v *volumeManager) ensureStagingDir(vol *structs.CSIVolume, usage *UsageOpt
 // Returns whether the directory is a pre-existing mountpoint, the publish path,
 // and any errors that occurred.
 func (v *volumeManager) ensureAllocDir(vol *structs.CSIVolume, alloc *structs.Allocation, usage *UsageOptions) (string, bool, error) {
-	allocPath := v.allocDirForVolume(v.mountRoot, vol.ID, alloc.ID, usage)
+	allocPath := v.allocDirForVolume(v.mountRoot, vol.ID, alloc.ID)
 
 	// Make the alloc path, owned by the Nomad User
 	if err := os.MkdirAll(allocPath, 0700); err != nil && !os.IsExist(err) {
 		return "", false, fmt.Errorf("failed to create allocation directory for volume (%s): %v", vol.ID, err)
 	}
 
-	// Validate that it is not already a mount point
+	// Validate that the target is not already a mount point
+	targetPath := v.targetForVolume(v.mountRoot, vol.ID, alloc.ID, usage)
 	m := mount.New()
-	isNotMount, err := m.IsNotAMountPoint(allocPath)
+	isNotMount, err := m.IsNotAMountPoint(targetPath)
 	if err != nil {
 		return "", false, fmt.Errorf("mount point detection failed for volume (%s): %v", vol.ID, err)
 	}
 
-	return allocPath, !isNotMount, nil
+	return targetPath, !isNotMount, nil
 }
 
 func volumeCapability(vol *structs.CSIVolume, usage *UsageOptions) (*csi.VolumeCapability, error) {
@@ -192,7 +197,7 @@ func (v *volumeManager) publishVolume(ctx context.Context, vol *structs.CSIVolum
 	if err != nil {
 		return nil, err
 	}
-	pluginTargetPath := v.allocDirForVolume(v.containerMountPoint, vol.ID, alloc.ID, usage)
+	pluginTargetPath := v.targetForVolume(v.containerMountPoint, vol.ID, alloc.ID, usage)
 
 	if isMount {
 		logger.Debug("Re-using existing published volume for allocation")
@@ -296,7 +301,7 @@ func combineErrors(maybeErrs ...error) error {
 }
 
 func (v *volumeManager) unpublishVolume(ctx context.Context, volID, remoteID, allocID string, usage *UsageOptions) error {
-	pluginTargetPath := v.allocDirForVolume(v.containerMountPoint, volID, allocID, usage)
+	pluginTargetPath := v.targetForVolume(v.containerMountPoint, volID, allocID, usage)
 
 	// CSI NodeUnpublishVolume errors for timeout, codes.Unavailable and
 	// codes.ResourceExhausted are retried; all other errors are fatal.
@@ -306,7 +311,7 @@ func (v *volumeManager) unpublishVolume(ctx context.Context, volID, remoteID, al
 		grpc_retry.WithBackoff(grpc_retry.BackoffExponential(100*time.Millisecond)),
 	)
 
-	hostTargetPath := v.allocDirForVolume(v.mountRoot, volID, allocID, usage)
+	hostTargetPath := v.targetForVolume(v.mountRoot, volID, allocID, usage)
 	if _, err := os.Stat(hostTargetPath); os.IsNotExist(err) {
 		if rpcErr != nil && strings.Contains(rpcErr.Error(), "no mount point") {
 			// host target path was already destroyed, nothing to do here.