Handle validation of drivers in Spec and Status

Signed-off-by: Deep Debroy <ddebroy@docker.com>

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

@@ -387,10 +387,6 @@ func (nim *nodeInfoManager) CreateCSINodeInfo() (*csiv1alpha1.CSINodeInfo, error
 		},
 	}
 
-	err = validateCSINodeInfo(nodeInfo)
-	if err != nil {
-		return err
-	}
 	return csiKubeClient.CsiV1alpha1().CSINodeInfos().Create(nodeInfo)
 }
 
@@ -503,7 +499,10 @@ func (nim *nodeInfoManager) uninstallDriverFromCSINodeInfo(csiDriverName string)
 			return nil
 		}
 
-		// TODO (verult) make sure CSINodeInfo has validation logic to prevent duplicate driver names
+		err = validateCSINodeInfo(nodeInfo)
+		if err != nil {
+			return err
+		}
 		_, updateErr := nodeInfoClient.Update(nodeInfo)
 		return updateErr // do not wrap error
 
@@ -568,29 +567,46 @@ func removeMaxAttachLimit(driverName string) nodeUpdateFunc {
 }
 
 // validateCSINodeInfo ensures members of CSINodeInfo object satisfies map and set semantics.
-// Before calling CSINodeInfoInterface.Create() or CSINodeInfoInterface.Update()
+// Before calling CSINodeInfoInterface.Update(), validateCSINodeInfo() should be invoked to
-// validateCSINodeInfo() should be invoked to make sure the CSINodeInfo is compliant
+// make sure the CSINodeInfo is compliant
-// TODO: move this logic to an external webhook
 func validateCSINodeInfo(nodeInfo *csiv1alpha1.CSINodeInfo) error {
-	if len(nodeInfo.CSIDrivers) < 1 {
+	if len(nodeInfo.Status.Drivers) < 1 {
-		return fmt.Errorf("at least one CSI Driver entry is required")
+		return fmt.Errorf("at least one Driver entry is required in driver statuses")
 	}
-	// check for duplicate entries for the same driver
+	if len(nodeInfo.Spec.Drivers) < 1 {
+		return fmt.Errorf("at least one Driver entry is required in driver specs")
+	}
+	if len(nodeInfo.Status.Drivers) != len(nodeInfo.Spec.Drivers) {
+		return fmt.Errorf("")
+	}
+	// check for duplicate entries for the same driver in statuses
 	var errors []string
-	driverNames := make(sets.String)
+	driverNamesInStatuses := make(sets.String)
-	for _, driverInfo := range nodeInfo.CSIDrivers {
+	for _, driverInfo := range nodeInfo.Status.Drivers {
-		if driverNames.Has(driverInfo.Driver) {
+		if driverNamesInStatuses.Has(driverInfo.Name) {
-			errors = append(errors, fmt.Sprintf("duplicate entries found for driver %s", driverNames))
+			errors = append(errors, fmt.Sprintf("duplicate entries found for driver: %s in driver statuses", driverInfo.Name))
 		}
-		driverNames.Insert(driverInfo.Driver)
+		driverNamesInStatuses.Insert(driverInfo.Name)
+	}
+	// check for duplicate entries for the same driver in specs
+	driverNamesInSpecs := make(sets.String)
+	for _, driverInfo := range nodeInfo.Spec.Drivers {
+		if driverNamesInSpecs.Has(driverInfo.Name) {
+			errors = append(errors, fmt.Sprintf("duplicate entries found for driver: %s in driver specs", driverInfo.Name))
+		}
+		driverNamesInSpecs.Insert(driverInfo.Name)
 		topoKeys := make(sets.String)
 		for _, key := range driverInfo.TopologyKeys {
 			if topoKeys.Has(key) {
-				errors = append(errors, fmt.Sprintf("duplicate topology keys %s found for driver %s", key, driverInfo.Driver))
+				errors = append(errors, fmt.Sprintf("duplicate topology keys %s found for driver %s in driver specs", key, driverInfo.Name))
 			}
 			topoKeys.Insert(key)
 		}
 	}
+	// check all entries in specs and status match
+	if !driverNamesInSpecs.Equal(driverNamesInStatuses) {
+		errors = append(errors, fmt.Sprintf("list of drivers in specs: %v does not match list of drivers in statuses: %v", driverNamesInSpecs.List(), driverNamesInStatuses.List()))
+	}
 	if len(errors) == 0 {
 		return nil
 	}

pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go

@@ -644,85 +644,271 @@ func TestValidateCSINodeInfo(t *testing.T) {
 		expectErr bool
 	}{
 		{
-			name: "multiple drivers with same ids and different topology keys",
+			name: "multiple drivers with different node IDs, topology keys and status",
 			nodeInfo: &csiv1alpha1.CSINodeInfo{
-				CSIDrivers: []csiv1alpha1.CSIDriverInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
-					{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
-						Driver:       "driver1",
+						{
-						NodeID:       "node1",
+							Name:         "driver1",
-						TopologyKeys: []string{"key1, key2"},
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1, key2"},
+						},
+						{
+							Name:         "driverB",
+							NodeID:       "nodeA",
+							TopologyKeys: []string{"keyA", "keyB"},
+						},
 					},
-					{
+				},
-						Driver:       "driverB",
+				Status: csiv1alpha1.CSINodeInfoStatus{
-						NodeID:       "nodeA",
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
-						TopologyKeys: []string{"keyA", "keyB"},
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "in-tree",
+						},
+						{
+							Name:                  "driverB",
+							Available:             false,
+							VolumePluginMechanism: "csi",
+						},
 					},
 				},
 			},
 			expectErr: false,
 		},
 		{
-			name: "multiple drivers with same ids and similar topology keys",
+			name: "multiple drivers with same node IDs, topology keys and status",
 			nodeInfo: &csiv1alpha1.CSINodeInfo{
-				CSIDrivers: []csiv1alpha1.CSIDriverInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
-					{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
-						Driver:       "driver1",
+						{
-						NodeID:       "node1",
+							Name:         "driver1",
-						TopologyKeys: []string{"key1"},
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
+						{
+							Name:         "driver2",
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
 					},
-					{
+				},
-						Driver:       "driver2",
+				Status: csiv1alpha1.CSINodeInfoStatus{
-						NodeID:       "node1",
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
-						TopologyKeys: []string{"key1"},
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+						{
+							Name:                  "driver2",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
 					},
 				},
 			},
 			expectErr: false,
 		},
 		{
-			name: "duplicate drivers",
+			name: "duplicate drivers in driver specs",
 			nodeInfo: &csiv1alpha1.CSINodeInfo{
-				CSIDrivers: []csiv1alpha1.CSIDriverInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
-					{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
-						Driver:       "driver1",
+						{
-						NodeID:       "node1",
+							Name:         "driver1",
-						TopologyKeys: []string{"key1", "key2"},
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1", "key2"},
+						},
+						{
+							Name:         "driver1",
+							NodeID:       "nodeX",
+							TopologyKeys: []string{"keyA", "keyB"},
+						},
 					},
-					{
+				},
-						Driver:       "driver1",
+				Status: csiv1alpha1.CSINodeInfoStatus{
-						NodeID:       "nodeX",
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
-						TopologyKeys: []string{"keyA", "keyB"},
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
 					},
 				},
 			},
 			expectErr: true,
 		},
 		{
-			name: "single driver with duplicate topology keys",
+			name: "duplicate drivers in driver statuses",
 			nodeInfo: &csiv1alpha1.CSINodeInfo{
-				CSIDrivers: []csiv1alpha1.CSIDriverInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
-					{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
-						Driver:       "driver1",
+						{
-						NodeID:       "node1",
+							Name:         "driver1",
-						TopologyKeys: []string{"key1", "key1"},
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1", "key2"},
+						},
+					},
+				},
+				Status: csiv1alpha1.CSINodeInfoStatus{
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "in-tree",
+						},
+						{
+							Name:                  "driver1",
+							Available:             false,
+							VolumePluginMechanism: "csi",
+						},
 					},
 				},
 			},
 			expectErr: true,
 		},
 		{
-			name: "multiple drivers with one set of duplicate topology keys ",
+			name: "single driver with duplicate topology keys in driver specs",
 			nodeInfo: &csiv1alpha1.CSINodeInfo{
-				CSIDrivers: []csiv1alpha1.CSIDriverInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
-					{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
-						Driver:       "driver1",
+						{
-						NodeID:       "node1",
+							Name:         "driver1",
-						TopologyKeys: []string{"key1"},
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1", "key1"},
+						},
 					},
-					{
+				},
-						Driver:       "driver2",
+				Status: csiv1alpha1.CSINodeInfoStatus{
-						NodeID:       "nodeX",
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
-						TopologyKeys: []string{"keyA", "keyA"},
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "multiple drivers with one set of duplicate topology keys in driver specs",
+			nodeInfo: &csiv1alpha1.CSINodeInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
+						{
+							Name:         "driver1",
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
+						{
+							Name:         "driver2",
+							NodeID:       "nodeX",
+							TopologyKeys: []string{"keyA", "keyA"},
+						},
+					},
+				},
+				Status: csiv1alpha1.CSINodeInfoStatus{
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+						{
+							Name:                  "driver2",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "mismatch between drivers in specs and status (null intersection)",
+			nodeInfo: &csiv1alpha1.CSINodeInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
+						{
+							Name:         "driver1",
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
+						{
+							Name:         "driver2",
+							NodeID:       "nodeX",
+							TopologyKeys: []string{"keyA", "keyA"},
+						},
+					},
+				},
+				Status: csiv1alpha1.CSINodeInfoStatus{
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
+						{
+							Name:                  "driver3",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "mismatch between drivers in specs and status (specs superset of status)",
+			nodeInfo: &csiv1alpha1.CSINodeInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
+						{
+							Name:         "driver1",
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
+						{
+							Name:         "driver2",
+							NodeID:       "nodeX",
+							TopologyKeys: []string{"keyA", "keyA"},
+						},
+					},
+				},
+				Status: csiv1alpha1.CSINodeInfoStatus{
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "mismatch between drivers in specs and status (specs subset of status)",
+			nodeInfo: &csiv1alpha1.CSINodeInfo{
+				Spec: csiv1alpha1.CSINodeInfoSpec{
+					Drivers: []csiv1alpha1.CSIDriverInfoSpec{
+						{
+							Name:         "driver1",
+							NodeID:       "node1",
+							TopologyKeys: []string{"key1"},
+						},
+					},
+				},
+				Status: csiv1alpha1.CSINodeInfoStatus{
+					Drivers: []csiv1alpha1.CSIDriverInfoStatus{
+						{
+							Name:                  "driver1",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
+						{
+							Name:                  "driver2",
+							Available:             true,
+							VolumePluginMechanism: "csi",
+						},
 					},
 				},
 			},