diff --git a/cmd/kubectl-direct_csi/drives_accesstier_set.go b/cmd/kubectl-direct_csi/drives_accesstier_set.go index 4fe51d9a5..8bc8c0e6c 100644 --- a/cmd/kubectl-direct_csi/drives_accesstier_set.go +++ b/cmd/kubectl-direct_csi/drives_accesstier_set.go @@ -82,7 +82,7 @@ func setAccessTier(ctx context.Context, accessTierArg []string) error { return fmt.Errorf("Invalid input arguments. Please use '%s' for examples to set access-tiers", utils.Bold("--help")) } - accessTier, err := utils.ValidateAccessTier(accessTierArg[0]) + accessTier, err := directcsi.ValidateAccessTier(accessTierArg[0]) if err != nil { return err } diff --git a/cmd/kubectl-direct_csi/utils.go b/cmd/kubectl-direct_csi/utils.go index 32eaa7714..5d07f4386 100644 --- a/cmd/kubectl-direct_csi/utils.go +++ b/cmd/kubectl-direct_csi/utils.go @@ -45,7 +45,6 @@ import ( const ( dot = "•" directCSIPartitionInfix = "-part-" - globRegExpPattern = `(^|[^\\])[\*\?\[]` ) type migrateFunc func(ctx context.Context, fromVersion string) error @@ -55,6 +54,9 @@ var ( red = color.New(color.FgRed).SprintFunc() green = color.New(color.FgGreen).SprintFunc() yellow = color.New(color.FgYellow).SprintFunc() + + globRegexp = regexp.MustCompile(`(^|[^\\])[\*\?\[]`) + errMixedSelectorUsage = errors.New("mixed usage of glob and ellipses selectors is not allowed") ) var ( // Default direct csi directory where direct csi audit logs are stored. @@ -74,25 +76,6 @@ func ListVolumesInDrive(drive directcsi.DirectCSIDrive, volumes *directcsi.Direc return vols } -func getAccessTierSet(accessTiers []string) ([]directcsi.AccessTier, error) { - var atSet []directcsi.AccessTier - for i := range accessTiers { - if accessTiers[i] == "*" { - return []directcsi.AccessTier{ - directcsi.AccessTierHot, - directcsi.AccessTierWarm, - directcsi.AccessTierCold, - }, nil - } - at, err := utils.ValidateAccessTier(strings.TrimSpace(accessTiers[i])) - if err != nil { - return atSet, err - } - atSet = append(atSet, at) - } - return atSet, nil -} - func printableString(s string) string { if s == "" { return "-" @@ -124,27 +107,9 @@ func canonicalNameFromPath(val string) string { return strings.ReplaceAll(dr, directCSIPartitionInfix, "") } -func hasGlob(s string) (bool, error) { - re, err := regexp.Compile(globRegExpPattern) - if err != nil { - return false, err - } - return re.MatchString(s), nil -} - func expandSelector(selectors []string) ([]string, error) { var expanded []string for _, selector := range selectors { - globFound, gErr := hasGlob(selector) - if gErr != nil { - return expanded, gErr - } - if globFound { - if !dryRun { - klog.Warning("Glob matches will be deprecated soon. Please use ellipses instead") - } - return nil, nil - } expandedList, err := ellipsis.Expand(selector) if err != nil { return nil, err @@ -155,16 +120,21 @@ func expandSelector(selectors []string) ([]string, error) { return expanded, nil } -func accessTierToString(aTs []directcsi.AccessTier) []string { - var atStringList []string - for _, aT := range aTs { - atStringList = append(atStringList, string(aT)) +func hasGlobSelectors(selectors []string) (bool, error) { + globCount := 0 + for _, selector := range selectors { + if globRegexp.MatchString(selector) { + globCount++ + } + } + if globCount > 0 && globCount != len(selectors) { + return false, errMixedSelectorUsage } - return atStringList + return globCount > 0, nil } -func setIfNil(sliceA, sliceB []string) []string { - if sliceA == nil { +func setIfTrue(cond bool, sliceB []string) []string { + if cond { return sliceB } return nil @@ -179,25 +149,35 @@ func processFilteredDrives( processFunc func(context.Context, *directcsi.DirectCSIDrive) error) error { var resultCh <-chan utils.ListDriveResult var err error - var expandedNodeList, expandedDriveList []string + var hasGlobNodeSelector, hasGlobDriveSelector bool if len(idArgs) > 0 { resultCh = getDrivesByIds(ctx, idArgs) } else { - expandedNodeList, err = expandSelector(nodes) + hasGlobNodeSelector, err = hasGlobSelectors(nodes) + if err != nil { + return err + } + + hasGlobDriveSelector, err = hasGlobSelectors(drives) if err != nil { return err } - expandedDriveList, err = expandSelector(drives) + expandedNodeList, err := expandSelector(setIfTrue(!hasGlobNodeSelector, nodes)) if err != nil { return err } - accessTierSet, err := getAccessTierSet(accessTiers) + expandedDriveList, err := expandSelector(setIfTrue(!hasGlobDriveSelector, drives)) if err != nil { return err } - accessTierSelector := accessTierToString(accessTierSet) + + accessTierSet, err := directcsi.GetAccessTierSet(accessTiers) + if err != nil { + return err + } + accessTierSelector := directcsi.AccessTiersToStrings(accessTierSet) directCSIClient := utils.GetDirectCSIClient() ctx, cancelFunc := context.WithCancel(ctx) @@ -214,13 +194,19 @@ func processFilteredDrives( } } + if hasGlobNodeSelector || hasGlobDriveSelector { + if !dryRun { + klog.Warning("Glob matches will be deprecated soon. Please use ellipses instead") + } + } + return processDrives( ctx, resultCh, func(drive *directcsi.DirectCSIDrive) bool { return drive.MatchGlob( - setIfNil(expandedNodeList, nodes), - setIfNil(expandedDriveList, drives), + setIfTrue(hasGlobNodeSelector, nodes), + setIfTrue(hasGlobDriveSelector, drives), status) && matchFunc(drive) }, applyFunc, @@ -232,20 +218,31 @@ func getFilteredDriveList(ctx context.Context, driveInterface clientset.DirectCS ctx, cancelFunc := context.WithCancel(ctx) defer cancelFunc() - expandedNodeList, err := expandSelector(nodes) + hasGlobNodeSelector, err := hasGlobSelectors(nodes) if err != nil { return nil, err } - expandedDriveList, err := expandSelector(drives) + + hasGlobDriveSelector, err := hasGlobSelectors(drives) if err != nil { return nil, err } - accessTierSet, err := getAccessTierSet(accessTiers) + expandedNodeList, err := expandSelector(setIfTrue(!hasGlobNodeSelector, nodes)) if err != nil { return nil, err } - accessTierSelector := accessTierToString(accessTierSet) + + expandedDriveList, err := expandSelector(setIfTrue(!hasGlobDriveSelector, drives)) + if err != nil { + return nil, err + } + + accessTierSet, err := directcsi.GetAccessTierSet(accessTiers) + if err != nil { + return nil, err + } + accessTierSelector := directcsi.AccessTiersToStrings(accessTierSet) resultCh, err := utils.ListDrives(ctx, driveInterface, @@ -257,14 +254,20 @@ func getFilteredDriveList(ctx context.Context, driveInterface clientset.DirectCS return nil, err } + if hasGlobNodeSelector || hasGlobDriveSelector { + if !dryRun { + klog.Warning("Glob matches will be deprecated soon. Please use ellipses instead") + } + } + filteredDrives := []directcsi.DirectCSIDrive{} for result := range resultCh { if result.Err != nil { return nil, result.Err } if result.Drive.MatchGlob( - setIfNil(expandedNodeList, nodes), - setIfNil(expandedDriveList, drives), + setIfTrue(hasGlobNodeSelector, nodes), + setIfTrue(hasGlobDriveSelector, drives), status) && filterFunc(result.Drive) { filteredDrives = append(filteredDrives, result.Drive) } @@ -277,22 +280,42 @@ func getFilteredVolumeList(ctx context.Context, volumeInterface clientset.Direct ctx, cancelFunc := context.WithCancel(ctx) defer cancelFunc() - expandedNodeList, err := expandSelector(nodes) + hasGlobNodeSelector, err := hasGlobSelectors(nodes) if err != nil { return nil, err } - expandedDriveList, err := expandSelector(drives) + hasGlobDriveSelector, err := hasGlobSelectors(drives) if err != nil { return nil, err } - expandedPodNameList, err := expandSelector(podNames) + hasGlobPodNameSelector, err := hasGlobSelectors(podNames) if err != nil { return nil, err } - expandedPodNssList, err := expandSelector(podNss) + hasGlobPodNsSelector, err := hasGlobSelectors(podNss) + if err != nil { + return nil, err + } + + expandedNodeList, err := expandSelector(setIfTrue(!hasGlobNodeSelector, nodes)) + if err != nil { + return nil, err + } + + expandedDriveList, err := expandSelector(setIfTrue(!hasGlobDriveSelector, drives)) + if err != nil { + return nil, err + } + + expandedPodNameList, err := expandSelector(setIfTrue(!hasGlobPodNameSelector, podNames)) + if err != nil { + return nil, err + } + + expandedPodNssList, err := expandSelector(setIfTrue(!hasGlobPodNsSelector, podNss)) if err != nil { return nil, err } @@ -308,14 +331,20 @@ func getFilteredVolumeList(ctx context.Context, volumeInterface clientset.Direct return nil, err } + if hasGlobNodeSelector || hasGlobDriveSelector || hasGlobPodNameSelector || hasGlobPodNsSelector { + if !dryRun { + klog.Warning("Glob matches will be deprecated soon. Please use ellipses instead") + } + } + filteredVolumes := []directcsi.DirectCSIVolume{} for result := range resultCh { if result.Err != nil { return nil, result.Err } - if result.Volume.MatchNodeDrives(setIfNil(expandedNodeList, nodes), setIfNil(expandedDriveList, drives)) && - result.Volume.MatchPodName(setIfNil(expandedPodNameList, podNames)) && - result.Volume.MatchPodNamespace(setIfNil(expandedPodNssList, podNss)) && + if result.Volume.MatchNodeDrives(setIfTrue(hasGlobNodeSelector, nodes), setIfTrue(hasGlobDriveSelector, drives)) && + result.Volume.MatchPodName(setIfTrue(hasGlobPodNameSelector, podNames)) && + result.Volume.MatchPodNamespace(setIfTrue(hasGlobPodNsSelector, podNss)) && result.Volume.MatchStatus(volumeStatus) && filterFunc(result.Volume) { filteredVolumes = append(filteredVolumes, result.Volume) diff --git a/cmd/kubectl-direct_csi/utils_test.go b/cmd/kubectl-direct_csi/utils_test.go index 0e33c06eb..c259818b5 100644 --- a/cmd/kubectl-direct_csi/utils_test.go +++ b/cmd/kubectl-direct_csi/utils_test.go @@ -77,14 +77,6 @@ func TestExpandSelector(t1 *testing.T) { "/dev/nvmen2p2", }, }, - { - selectors: []string{"/dev/xvd[b-f]"}, - expandedList: nil, - }, - { - selectors: []string{"node-*"}, - expandedList: nil, - }, { selectors: []string{"/dev/xvd{b..c}"}, expandedList: nil, @@ -123,71 +115,85 @@ func TestExpandSelector(t1 *testing.T) { } } -func TestHasGlob(t1 *testing.T) { +func TestHasGlobSelectors(t1 *testing.T) { testCases := []struct { - inputStr string - isGlob bool + selectors []string + hasGlobSelector bool + expectErr bool }{ { - inputStr: "/dev/xvd[b-c]", - isGlob: true, + selectors: []string{"/dev/xvd{a...c}", "/dev/xvd{e...f}"}, + hasGlobSelector: false, + expectErr: false, + }, + { + selectors: []string{"/dev/xvd[a-c]", "/dev/xvd[e-f]"}, + hasGlobSelector: true, + expectErr: false, }, { - inputStr: "/dev/xvd*", - isGlob: true, + selectors: []string{"/dev/xvd[a-c]"}, + hasGlobSelector: true, + expectErr: false, }, { - inputStr: "/dev/node[1-3]", - isGlob: true, + selectors: []string{"/dev/xvda*"}, + hasGlobSelector: true, + expectErr: false, }, { - inputStr: "/dev/node1*", - isGlob: true, + selectors: []string{"/dev/xvd{a...b}"}, + hasGlobSelector: false, + expectErr: false, }, { - inputStr: "/dev/xvd{b...c}", + selectors: nil, + hasGlobSelector: false, + expectErr: false, }, { - inputStr: "/dev/node{1...3}", + selectors: []string{"/dev/xvd{a...c}", "/dev/xvd[e-f]"}, + hasGlobSelector: false, + expectErr: true, }, } for i, testCase := range testCases { - isGlob, err := hasGlob(testCase.inputStr) - if err != nil { - t1.Fatalf("case %v: failed to check for glob: %v", i+1, err) + hasGlob, err := hasGlobSelectors(testCase.selectors) + if err != nil && !testCase.expectErr { + t1.Fatalf("case %v: did not expect error but got: %v", i+1, err) } - if testCase.isGlob != isGlob { - t1.Errorf("case %v: Expected result = %v, got %v", i+1, testCase.isGlob, isGlob) + if testCase.hasGlobSelector != hasGlob { + t1.Errorf("case %v: Expected hasGlob = %v, got %v", i+1, testCase.hasGlobSelector, hasGlob) } } } -func TestSetIfNil(t1 *testing.T) { +func TestSetIfTrue(t1 *testing.T) { testCases := []struct { - sliceA []string + cond bool sliceB []string result []string }{ { - sliceA: []string{"abc"}, + cond: false, sliceB: []string{"def"}, result: nil, }, { - sliceA: nil, + cond: true, sliceB: []string{"def"}, result: []string{"def"}, }, { - sliceA: nil, + cond: true, sliceB: nil, result: nil, }, } for i, testCase := range testCases { - result := setIfNil(testCase.sliceA, testCase.sliceB) + result := setIfTrue(testCase.cond, testCase.sliceB) if !reflect.DeepEqual(result, testCase.result) { t1.Errorf("case %v: Expected result = %v, got %v", i+1, testCase.result, result) } diff --git a/pkg/apis/direct.csi.min.io/v1beta3/drive_matcher.go b/pkg/apis/direct.csi.min.io/v1beta3/drive_matcher.go index 28a63de6f..9e17a49fa 100644 --- a/pkg/apis/direct.csi.min.io/v1beta3/drive_matcher.go +++ b/pkg/apis/direct.csi.min.io/v1beta3/drive_matcher.go @@ -17,10 +17,47 @@ package v1beta3 import ( + "fmt" + "strings" + "github.com/minio/direct-csi/pkg/matcher" ) -func accessTiersToStrings(accessTiers []AccessTier) (slice []string) { +func ValidateAccessTier(at string) (AccessTier, error) { + switch AccessTier(strings.Title(at)) { + case AccessTierWarm: + return AccessTierWarm, nil + case AccessTierHot: + return AccessTierHot, nil + case AccessTierCold: + return AccessTierCold, nil + case AccessTierUnknown: + return AccessTierUnknown, fmt.Errorf("Please set any one among ['hot','warm', 'cold']") + default: + return AccessTierUnknown, fmt.Errorf("Invalid 'access-tier' value, Please set any one among ['hot','warm','cold']") + } +} + +func GetAccessTierSet(accessTiers []string) ([]AccessTier, error) { + var atSet []AccessTier + for i := range accessTiers { + if accessTiers[i] == "*" { + return []AccessTier{ + AccessTierHot, + AccessTierWarm, + AccessTierCold, + }, nil + } + at, err := ValidateAccessTier(strings.TrimSpace(accessTiers[i])) + if err != nil { + return atSet, err + } + atSet = append(atSet, at) + } + return atSet, nil +} + +func AccessTiersToStrings(accessTiers []AccessTier) (slice []string) { for _, accessTier := range accessTiers { slice = append(slice, string(accessTier)) } @@ -32,5 +69,5 @@ func (drive *DirectCSIDrive) MatchGlob(nodes, drives, status []string) bool { } func (drive *DirectCSIDrive) MatchAccessTier(accessTierList []AccessTier) bool { - return len(accessTierList) == 0 || matcher.StringIn(accessTiersToStrings(accessTierList), string(drive.Status.AccessTier)) + return len(accessTierList) == 0 || matcher.StringIn(AccessTiersToStrings(accessTierList), string(drive.Status.AccessTier)) } diff --git a/pkg/controller/utils.go b/pkg/controller/utils.go index 0d0119d86..9fc23d068 100644 --- a/pkg/controller/utils.go +++ b/pkg/controller/utils.go @@ -23,7 +23,6 @@ import ( "sort" directcsi "github.com/minio/direct-csi/pkg/apis/direct.csi.min.io/v1beta3" - "github.com/minio/direct-csi/pkg/utils" "github.com/container-storage-interface/spec/lib/go/csi" "google.golang.org/grpc/codes" @@ -112,7 +111,7 @@ func FilterDrivesByParameters(parameters map[string]string, csiDrives []directcs for k, v := range parameters { switch k { case "direct-csi-min-io/access-tier": - accessT, err := utils.ValidateAccessTier(v) + accessT, err := directcsi.ValidateAccessTier(v) if err != nil { return csiDrives, err } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 4f2b946ba..aee40bcd5 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -17,8 +17,6 @@ package utils import ( - "fmt" - "io" "os" "path/filepath" "reflect" @@ -36,21 +34,6 @@ func BoolToCondition(val bool) metav1.ConditionStatus { return metav1.ConditionFalse } -func ValidateAccessTier(at string) (directcsi.AccessTier, error) { - switch directcsi.AccessTier(strings.Title(at)) { - case directcsi.AccessTierWarm: - return directcsi.AccessTierWarm, nil - case directcsi.AccessTierHot: - return directcsi.AccessTierHot, nil - case directcsi.AccessTierCold: - return directcsi.AccessTierCold, nil - case directcsi.AccessTierUnknown: - return directcsi.AccessTierUnknown, fmt.Errorf("Please set any one among ['hot','warm', 'cold']") - default: - return directcsi.AccessTierUnknown, fmt.Errorf("Invalid 'access-tier' value, Please set any one among ['hot','warm','cold']") - } -} - func defaultIfZero(left, right interface{}) interface{} { lval := reflect.ValueOf(left) if lval.IsZero() {