Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add validation of CSINodeInfo fields before Create/Update actions #70742

Merged
merged 2 commits into from
Nov 14, 2018
Merged

Add validation of CSINodeInfo fields before Create/Update actions #70742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af73d7b
Add validation of CSINodeInfo before Create/Update calls
ddebroy Nov 13, 2018
05759a9
Handle validation of drivers in Spec and Status
ddebroy Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 57 additions & 2 deletions pkg/volume/csi/nodeinfomanager/nodeinfomanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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, ", "))
}
290 changes: 290 additions & 0 deletions pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)()
Expand Down