diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go index a27e296691..4f783821d2 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go +++ b/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() -// validateCSINodeInfo() should be invoked to make sure the CSINodeInfo is compliant -// TODO: move this logic to an external webhook +// Before calling CSINodeInfoInterface.Update(), validateCSINodeInfo() should be invoked to +// make sure the CSINodeInfo is compliant func validateCSINodeInfo(nodeInfo *csiv1alpha1.CSINodeInfo) error { - if len(nodeInfo.CSIDrivers) < 1 { - return fmt.Errorf("at least one CSI Driver entry is required") + if len(nodeInfo.Status.Drivers) < 1 { + 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) - for _, driverInfo := range nodeInfo.CSIDrivers { - if driverNames.Has(driverInfo.Driver) { - errors = append(errors, fmt.Sprintf("duplicate entries found for driver %s", driverNames)) + driverNamesInStatuses := make(sets.String) + for _, driverInfo := range nodeInfo.Status.Drivers { + if driverNamesInStatuses.Has(driverInfo.Name) { + 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 } diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go index f9803d745b..468fedcc91 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go +++ b/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{ - { - Driver: "driver1", - NodeID: "node1", - TopologyKeys: []string{"key1, key2"}, + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1, key2"}, + }, + { + Name: "driverB", + NodeID: "nodeA", + TopologyKeys: []string{"keyA", "keyB"}, + }, }, - { - Driver: "driverB", - NodeID: "nodeA", - TopologyKeys: []string{"keyA", "keyB"}, + }, + Status: csiv1alpha1.CSINodeInfoStatus{ + Drivers: []csiv1alpha1.CSIDriverInfoStatus{ + { + 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{ - { - Driver: "driver1", - NodeID: "node1", - TopologyKeys: []string{"key1"}, + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1"}, + }, + { + Name: "driver2", + NodeID: "node1", + TopologyKeys: []string{"key1"}, + }, }, - { - Driver: "driver2", - NodeID: "node1", - TopologyKeys: []string{"key1"}, + }, + Status: csiv1alpha1.CSINodeInfoStatus{ + Drivers: []csiv1alpha1.CSIDriverInfoStatus{ + { + 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{ - { - Driver: "driver1", - NodeID: "node1", - TopologyKeys: []string{"key1", "key2"}, + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1", "key2"}, + }, + { + Name: "driver1", + NodeID: "nodeX", + TopologyKeys: []string{"keyA", "keyB"}, + }, }, - { - Driver: "driver1", - NodeID: "nodeX", - TopologyKeys: []string{"keyA", "keyB"}, + }, + Status: csiv1alpha1.CSINodeInfoStatus{ + Drivers: []csiv1alpha1.CSIDriverInfoStatus{ + { + 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{ - { - Driver: "driver1", - NodeID: "node1", - TopologyKeys: []string{"key1", "key1"}, + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + 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{ - { - Driver: "driver1", - NodeID: "node1", - TopologyKeys: []string{"key1"}, + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1", "key1"}, + }, }, - { - Driver: "driver2", - NodeID: "nodeX", - TopologyKeys: []string{"keyA", "keyA"}, + }, + Status: csiv1alpha1.CSINodeInfoStatus{ + Drivers: []csiv1alpha1.CSIDriverInfoStatus{ + { + 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", + }, }, }, },