Merge pull request #125 from codenrhoden/node_id_required

codenrhoden · web-flow · commit 3e243d0c49aa · 2019-06-20T20:38:50.000-06:00
Make NodeID required where needed
diff --git a/README.md b/README.md
@@ -247,17 +247,6 @@ environment variables:
       <p>Enabling this option sets <code>X_CSI_SPEC_REQ_VALIDATION=true</code></p>
       </td>
     </tr>
-    <tr>
-      <td><code>X_CSI_REQUIRE_NODE_ID</code></td>
-      <td>
-        <p>A flag that enables treating the following fields as required:</p>
-        <ul>
-          <li><code>ControllerPublishVolumeRequest.NodeId</code></li>
-          <li><code>NodeGetIdResponse.NodeId</code></li>
-      </ul>
-      <p>Enabling this option sets <code>X_CSI_SPEC_REQ_VALIDATION=true</code></p>
-      </td>
-    </tr>
     <tr>
       <td><code>X_CSI_REQUIRE_VOL_CONTEXT</code></td>
       <td>
diff --git a/csc/cmd/controller_publish_volume.go b/csc/cmd/controller_publish_volume.go
@@ -73,13 +73,6 @@ func init() {
 		"",
 		"The ID of the node to which to publish the volume")
 
-	controllerPublishVolumeCmd.Flags().BoolVar(
-		&root.withRequiresNodeID,
-		"with-requires-node-id",
-		false,
-		`Marks the request's NodeId field as required.
-        Enabling this option also enables --with-spec-validation.`)
-
 	flagReadOnly(
 		controllerPublishVolumeCmd.Flags(), &controllerPublishVolume.readOnly)
 
diff --git a/csc/cmd/interceptors.go b/csc/cmd/interceptors.go
@@ -42,7 +42,6 @@ func getClientInterceptorsDialOpt() grpc.DialOption {
 	// Configure the spec validator.
 	root.withSpecValidator = root.withSpecValidator ||
 		root.withRequiresCreds ||
-		root.withRequiresNodeID ||
 		root.withRequiresVolContext ||
 		root.withRequiresPubContext
 	if root.withSpecValidator {
@@ -57,11 +56,6 @@ func getClientInterceptorsDialOpt() grpc.DialOption {
 				specvalidator.WithRequiresNodePublishVolumeSecrets())
 			log.Debug("enabled spec validator opt: requires creds")
 		}
-		if root.withRequiresNodeID {
-			specOpts = append(specOpts,
-				specvalidator.WithRequiresNodeID())
-			log.Debug("enabled spec validator opt: requires node ID")
-		}
 		if root.withRequiresVolContext {
 			specOpts = append(specOpts,
 				specvalidator.WithRequiresVolumeContext())
diff --git a/csc/cmd/node_get_info.go b/csc/cmd/node_get_info.go
@@ -29,10 +29,4 @@ var nodeGetInfoCmd = &cobra.Command{
 
 func init() {
 	nodeCmd.AddCommand(nodeGetInfoCmd)
-
-	nodeGetInfoCmd.Flags().BoolVar(
-		&root.withRequiresNodeID,
-		"with-requires-node-id",
-		false,
-		"marks the response's node ID as a required field")
 }
diff --git a/csc/cmd/root.go b/csc/cmd/root.go
@@ -39,7 +39,6 @@ var root struct {
 
 	withSpecValidator      bool
 	withRequiresCreds      bool
-	withRequiresNodeID     bool
 	withRequiresVolContext bool
 	withRequiresPubContext bool
 }
diff --git a/envvars.go b/envvars.go
@@ -114,12 +114,6 @@ const (
 	// a code of "Internal."
 	EnvVarSpecRepValidation = "X_CSI_SPEC_REP_VALIDATION"
 
-	// EnvVarRequireNodeID is the name of the environment variable used
-	// to determine whether or not the node ID value is required for
-	// requests that accept it and responses that return it such as
-	// ControllerPublishVolume and GetNodeId.
-	EnvVarRequireNodeID = "X_CSI_REQUIRE_NODE_ID"
-
 	// EnvVarRequireStagingTargetPath is the name of the environment variable
 	// used to determine whether or not the NodePublishVolume request field
 	// StagingTargetPath is required.
diff --git a/middleware.go b/middleware.go
@@ -28,7 +28,6 @@ func (sp *StoragePlugin) initInterceptors(ctx context.Context) {
 		withSerialVol          = sp.getEnvBool(ctx, EnvVarSerialVolAccess)
 		withSpec               = sp.getEnvBool(ctx, EnvVarSpecValidation)
 		withStgTgtPath         = sp.getEnvBool(ctx, EnvVarRequireStagingTargetPath)
-		withNodeID             = sp.getEnvBool(ctx, EnvVarRequireNodeID)
 		withVolContext         = sp.getEnvBool(ctx, EnvVarRequireVolContext)
 		withPubContext         = sp.getEnvBool(ctx, EnvVarRequirePubContext)
 		withCreds              = sp.getEnvBool(ctx, EnvVarCreds)
@@ -62,7 +61,6 @@ func (sp *StoragePlugin) initInterceptors(ctx context.Context) {
 	if !withSpecReq {
 		withSpecReq = withCreds ||
 			withStgTgtPath ||
-			withNodeID ||
 			withVolContext ||
 			withPubContext
 		log.WithField("withSpecReq", withSpecReq).Debug(
@@ -162,11 +160,6 @@ func (sp *StoragePlugin) initInterceptors(ctx context.Context) {
 			log.Debug("enabled spec validator opt: " +
 				"requires starging target path")
 		}
-		if withNodeID {
-			specOpts = append(specOpts,
-				specvalidator.WithRequiresNodeID())
-			log.Debug("enabled spec validator opt: requires node ID")
-		}
 		if withVolContext {
 			specOpts = append(specOpts,
 				specvalidator.WithRequiresVolumeContext())
diff --git a/middleware/specvalidator/spec_validator.go b/middleware/specvalidator/spec_validator.go
@@ -25,7 +25,6 @@ type opts struct {
 	reqValidation               bool
 	repValidation               bool
 	requiresStagingTargetPath   bool
-	requiresNodeID              bool
 	requiresVolContext          bool
 	requiresPubContext          bool
 	requiresCtlrNewVolSecrets   bool
@@ -50,15 +49,6 @@ func WithResponseValidation() Option {
 	}
 }
 
-// WithRequiresNodeID is a Option that indicates
-// ControllerPublishVolume requests and NodeGetInfo responses must
-// contain non-empty node ID data.
-func WithRequiresNodeID() Option {
-	return func(o *opts) {
-		o.requiresNodeID = true
-	}
-}
-
 // WithRequiresStagingTargetPath is a Option that indicates
 // NodePublishVolume requests must have non-empty StagingTargetPath
 // fields.
@@ -264,9 +254,6 @@ func (s *interceptor) handle(
 type interceptorHasVolumeID interface {
 	GetVolumeId() string
 }
-type interceptorHasNodeID interface {
-	NodeGetId() string
-}
 type interceptorHasUserCredentials interface {
 	GetUserCredentials() map[string]string
 }
@@ -302,17 +289,6 @@ func (s *interceptor) validateRequest(
 		}
 	}
 
-	// Check to see if the request has a node ID and if it is set.
-	// If the node ID is not set then return an error.
-	if s.opts.requiresNodeID {
-		if treq, ok := req.(interceptorHasNodeID); ok {
-			if treq.NodeGetId() == "" {
-				return status.Error(
-					codes.InvalidArgument, "required: NodeID")
-			}
-		}
-	}
-
 	// Check to see if the request has volume context and if they're
 	// required. If the volume context is required by no attributes are
 	// specified then return an error.
@@ -459,6 +435,11 @@ func (s *interceptor) validateControllerPublishVolumeRequest(
 		}
 	}
 
+	if req.NodeId == "" {
+		return status.Error(
+			codes.InvalidArgument, "required: NodeID")
+	}
+
 	return validateVolumeCapabilityArg(req.VolumeCapability, true)
 }
 
@@ -674,9 +655,10 @@ func (s *interceptor) validateGetPluginInfoResponse(
 func (s *interceptor) validateNodeGetInfoResponse(
 	ctx context.Context,
 	rep csi.NodeGetInfoResponse) error {
-	if s.opts.requiresNodeID && rep.NodeId == "" {
+	if rep.NodeId == "" {
 		return status.Error(codes.Internal, "empty: NodeID")
 	}
+
 	return nil
 }
 
diff --git a/mock/provider/provider.go b/mock/provider/provider.go
@@ -39,11 +39,6 @@ func New() gocsi.StoragePluginProvider {
 			// Enable request and response validation.
 			gocsi.EnvVarSpecValidation + "=true",
 
-			// Treat the following fields as required:
-			//    * ControllerPublishVolumeRequest.NodeId
-			//    * NodeGetNodeIdResponse.NodeId
-			gocsi.EnvVarRequireNodeID + "=true",
-
 			// Treat the following fields as required:
 			//   * ControllerPublishVolumeResponse.PublishContext
 			//   * NodeStageVolumeRequest.PublishContext
diff --git a/mock/service/controller.go b/mock/service/controller.go
@@ -5,6 +5,7 @@ import (
 	"math"
 	"path"
 	"strconv"
+	"strings"
 
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
@@ -125,13 +126,24 @@ func (s *service) ControllerUnpublishVolume(
 	// to the specified node.
 	devPathKey := path.Join(req.NodeId, "dev")
 
-	// Check to see if the volume is already unpublished.
-	if v.VolumeContext[devPathKey] == "" {
-		return &csi.ControllerUnpublishVolumeResponse{}, nil
-	}
+	// if NodeID is not blank, unpublish from just that node
+	if req.NodeId != "" {
+		// Check to see if the volume is already unpublished.
+		if v.VolumeContext[devPathKey] == "" {
+			return &csi.ControllerUnpublishVolumeResponse{}, nil
+		}
 
-	// Unpublish the volume.
-	delete(v.VolumeContext, devPathKey)
+		// Unpublish the volume.
+		delete(v.VolumeContext, devPathKey)
+	} else {
+		// NodeID is blank, unpublish from all nodes, which can be identified by
+		// ending with "/dev"
+		for k, _ := range v.VolumeContext {
+			if strings.HasSuffix(k, devPathKey) {
+				delete(v.VolumeContext, k)
+			}
+		}
+	}
 	s.vols[i] = v
 
 	return &csi.ControllerUnpublishVolumeResponse{}, nil
diff --git a/testing/controller_test.go b/testing/controller_test.go
@@ -39,6 +39,7 @@ var _ = Describe("Controller", func() {
 		params   map[string]string
 		// userCreds map[string]string
 		pubInfo map[string]string
+		nodeId  string
 	)
 	BeforeEach(func() {
 		ctx = context.Background()
@@ -53,6 +54,7 @@ var _ = Describe("Controller", func() {
 		mntFlags = []string{"-o noexec"}
 		params = map[string]string{"tag": "gold"}
 		// userCreds = map[string]string{"beour": "guest"}
+		nodeId = service.Name
 	})
 	JustBeforeEach(func() {
 		gclient, stopMock, err = startMockServer(ctx)
@@ -78,6 +80,7 @@ var _ = Describe("Controller", func() {
 		mntFlags = nil
 		params = nil
 		pubInfo = nil
+		nodeId = ""
 	})
 
 	listVolumes := func() (vols []csi.Volume, err error) {
@@ -614,58 +617,99 @@ var _ = Describe("Controller", func() {
 
 	Describe("Publication", func() {
 
+		var (
+			res *csi.ControllerPublishVolumeResponse
+			err error
+		)
+
 		devPathKey := path.Join(service.Name, "dev")
 
 		publishVolume := func() {
 			req := &csi.ControllerPublishVolumeRequest{
 				VolumeId: "1",
-				NodeId:   service.Name,
+				NodeId:   nodeId,
 				Readonly: true,
 				VolumeCapability: utils.NewMountCapability(
 					csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
 					"mock"),
 			}
-			res, err := client.ControllerPublishVolume(ctx, req)
-			Ω(err).ShouldNot(HaveOccurred())
-			pubInfo = res.PublishContext
+			res, err = client.ControllerPublishVolume(ctx, req)
 		}
 
 		shouldBePublished := func() {
+			Ω(err).ShouldNot(HaveOccurred())
+			pubInfo = res.PublishContext
 			Ω(err).ShouldNot(HaveOccurred())
 			Ω(pubInfo).ShouldNot(BeNil())
 			Ω(pubInfo["device"]).Should(Equal("/dev/mock"))
 		}
 
+		unpublishVolume := func() {
+			_, err := client.ControllerUnpublishVolume(
+				ctx,
+				&csi.ControllerUnpublishVolumeRequest{
+					VolumeId: "1",
+					NodeId:   nodeId,
+				})
+			Ω(err).ShouldNot(HaveOccurred())
+		}
+
 		JustBeforeEach(func() {
 			publishVolume()
 		})
+
 		Context("PublishVolume", func() {
-			It("Should Be Valid", func() {
-				shouldBePublished()
-				vols, err := listVolumes()
-				Ω(err).ShouldNot(HaveOccurred())
-				Ω(vols).Should(HaveLen(3))
-				Ω(vols[0].VolumeContext[devPathKey]).Should(Equal("/dev/mock"))
+
+			Context("Valid Node ID", func() {
+				It("Should Be Valid", func() {
+					shouldBePublished()
+					vols, err := listVolumes()
+					Ω(err).ShouldNot(HaveOccurred())
+					Ω(vols).Should(HaveLen(3))
+					Ω(vols[0].VolumeContext[devPathKey]).Should(Equal("/dev/mock"))
+				})
 			})
+
+			Context("Invalid Node ID", func() {
+
+				BeforeEach(func() {
+					nodeId = ""
+				})
+
+				It("Should Error", func() {
+					Ω(err).Should(HaveOccurred())
+					Ω(err).Should(ΣCM(codes.InvalidArgument, "required: NodeID"))
+				})
+			})
+
 		})
 
 		Context("UnpublishVolume", func() {
 			JustBeforeEach(func() {
 				shouldBePublished()
-				_, err := client.ControllerUnpublishVolume(
-					ctx,
-					&csi.ControllerUnpublishVolumeRequest{
-						VolumeId: "1",
-						NodeId:   service.Name,
-					})
-				Ω(err).ShouldNot(HaveOccurred())
 			})
-			It("Should Be Unpublished", func() {
-				vols, err := listVolumes()
-				Ω(err).ShouldNot(HaveOccurred())
-				Ω(vols).Should(HaveLen(3))
-				_, ok := vols[0].VolumeContext[devPathKey]
-				Ω(ok).Should(BeFalse())
+
+			Context("Valid NodeID", func() {
+				It("Should Be Unpublished", func() {
+					unpublishVolume()
+					vols, err := listVolumes()
+					Ω(err).ShouldNot(HaveOccurred())
+					Ω(vols).Should(HaveLen(3))
+					_, ok := vols[0].VolumeContext[devPathKey]
+					Ω(ok).Should(BeFalse())
+				})
+			})
+
+			Context("No NodeID", func() {
+				It("Should Still Be Unpublished", func() {
+					nodeId = ""
+					unpublishVolume()
+					vols, err := listVolumes()
+					Ω(err).ShouldNot(HaveOccurred())
+					Ω(vols).Should(HaveLen(3))
+					_, ok := vols[0].VolumeContext[devPathKey]
+					Ω(ok).Should(BeFalse())
+				})
 			})
 		})
 	})
diff --git a/testing/gocsi_test.go b/testing/gocsi_test.go
diff --git a/usage.go b/usage.go

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,6 @@ var root struct {`
`39`	`39`
`40`	`40`	`withSpecValidator bool`
`41`	`41`	`withRequiresCreds bool`
`42`		`- withRequiresNodeID bool`
`43`	`42`	`withRequiresVolContext bool`
`44`	`43`	`withRequiresPubContext bool`
`45`	`44`	`}`