diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go index 37f24151b7e..4f783821d22 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go @@ -21,6 +21,7 @@ package nodeinfomanager // import "k8s.io/kubernetes/pkg/volume/csi/nodeinfomana import ( "encoding/json" "fmt" + "strings" csipb "github.com/container-storage-interface/spec/lib/go/csi/v0" "k8s.io/api/core/v1" @@ -458,7 +459,11 @@ func (nim *nodeInfoManager) installDriverToCSINodeInfo( nodeInfo.Spec.Drivers = newDriverSpecs nodeInfo.Status.Drivers = newDriverStatuses - _, err := csiKubeClient.CsiV1alpha1().CSINodeInfos().Update(nodeInfo) + err := validateCSINodeInfo(nodeInfo) + if err != nil { + return err + } + _, err = csiKubeClient.CsiV1alpha1().CSINodeInfos().Update(nodeInfo) return err // do not wrap error } @@ -494,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 @@ -557,3 +565,50 @@ func removeMaxAttachLimit(driverName string) nodeUpdateFunc { return node, true, nil } } + +// validateCSINodeInfo ensures members of CSINodeInfo object satisfies map and set semantics. +// Before calling CSINodeInfoInterface.Update(), validateCSINodeInfo() should be invoked to +// make sure the CSINodeInfo is compliant +func validateCSINodeInfo(nodeInfo *csiv1alpha1.CSINodeInfo) error { + if len(nodeInfo.Status.Drivers) < 1 { + return fmt.Errorf("at least one Driver entry is required in driver statuses") + } + 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 + 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)) + } + 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 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 + } + return fmt.Errorf(strings.Join(errors, ", ")) +} diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go index b58ab813dcd..468fedcc910 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go @@ -637,6 +637,296 @@ func TestInstallCSIDriverExistingAnnotation(t *testing.T) { } } +func TestValidateCSINodeInfo(t *testing.T) { + testcases := []struct { + name string + nodeInfo *csiv1alpha1.CSINodeInfo + expectErr bool + }{ + { + name: "multiple drivers with different node IDs, topology keys and status", + nodeInfo: &csiv1alpha1.CSINodeInfo{ + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1, key2"}, + }, + { + Name: "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 node IDs, topology keys and status", + nodeInfo: &csiv1alpha1.CSINodeInfo{ + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1"}, + }, + { + Name: "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 in driver specs", + nodeInfo: &csiv1alpha1.CSINodeInfo{ + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1", "key2"}, + }, + { + Name: "driver1", + NodeID: "nodeX", + TopologyKeys: []string{"keyA", "keyB"}, + }, + }, + }, + Status: csiv1alpha1.CSINodeInfoStatus{ + Drivers: []csiv1alpha1.CSIDriverInfoStatus{ + { + Name: "driver1", + Available: true, + VolumePluginMechanism: "csi", + }, + }, + }, + }, + expectErr: true, + }, + { + name: "duplicate drivers in driver statuses", + nodeInfo: &csiv1alpha1.CSINodeInfo{ + 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: "single driver with duplicate topology keys in driver specs", + nodeInfo: &csiv1alpha1.CSINodeInfo{ + Spec: csiv1alpha1.CSINodeInfoSpec{ + Drivers: []csiv1alpha1.CSIDriverInfoSpec{ + { + Name: "driver1", + NodeID: "node1", + TopologyKeys: []string{"key1", "key1"}, + }, + }, + }, + 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", + }, + }, + }, + }, + expectErr: true, + }, + } + for _, tc := range testcases { + t.Logf("test case: %q", tc.name) + err := validateCSINodeInfo(tc.nodeInfo) + if err != nil && !tc.expectErr { + t.Errorf("expected no errors from validateCSINodeInfo but got error %v", err) + } + if err == nil && tc.expectErr { + t.Errorf("expected error from validateCSINodeInfo but got no errors") + } + } +} + func test(t *testing.T, addNodeInfo bool, csiNodeInfoEnabled bool, testcases []testcase) { defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSINodeInfo, csiNodeInfoEnabled)() defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AttachVolumeLimit, true)()