OCM-6858 | fix: add validator for node drain grace period in interact…

…ive mode
openshift · Mar 22, 2024 · 76e5f4a · 76e5f4a
1 parent 709d754
commit 76e5f4a
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 20 deletions.
diff --git a/cmd/create/machinepool/nodepool.go b/cmd/create/machinepool/nodepool.go
@@ -393,6 +393,9 @@ func addNodePool(cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, r
 			Help:     cmd.Flags().Lookup("node-drain-grace-period").Usage,
 			Default:  nodeDrainGracePeriod,
 			Required: false,
+			Validators: []interactive.Validator{
+				machinepools.ValidateNodeDrainGracePeriod,
+			},
 		})
 		if err != nil {
 			r.Reporter.Errorf("Expected a valid value for Node drain grace period: %s", err)

diff --git a/pkg/helper/machinepools/helpers.go b/pkg/helper/machinepools/helpers.go
@@ -18,7 +18,17 @@ import (
 )
 
 // To clear existing labels in interactive mode, the user enters "" as an empty list value
-const interactiveModeEmptyLabels = `""`
+const (
+	interactiveModeEmptyLabels = `""`
+	nodeDrainUnitMinute        = "minute"
+	nodeDrainUnitMinutes       = "minutes"
+	nodeDrainUnitHour          = "hour"
+	nodeDrainUnitHours         = "hours"
+	nodeDrainUnits             = nodeDrainUnitMinute + "|" + nodeDrainUnitMinutes + "|" +
+		nodeDrainUnitHour + "|" + nodeDrainUnitHours
+	MaxNodeDrainTimeInMinutes = 10080
+	MaxNodeDrainTimeInHours   = 168
+)
 
 func MinNodePoolReplicaValidator(autoscaling bool) interactive.Validator {
 	return func(val interface{}) error {
@@ -230,26 +240,59 @@ func CreateNodeDrainGracePeriodBuilder(nodeDrainGracePeriod string) (*cmv1.Value
 	}
 
 	nodeDrainParsed := strings.Split(nodeDrainGracePeriod, " ")
-	if len(nodeDrainParsed) > 2 {
-		return nil, fmt.Errorf("Invalid value for node drain grace period. Expects only the duration and " +
-			"the unit (minute|minutes|hour|hours).")
-	}
 	nodeDrainValue, err := strconv.ParseFloat(nodeDrainParsed[0], commonUtils.MaxByteSize)
 	if err != nil {
 		return nil, fmt.Errorf("Invalid time for the node drain grace period: %s", err)
 	}
 
 	// Default to minutes if no unit is specified
 	if len(nodeDrainParsed) > 1 {
-		if nodeDrainParsed[1] != "hours" && nodeDrainParsed[1] != "hour" &&
-			nodeDrainParsed[1] != "minutes" && nodeDrainParsed[1] != "minute" {
-			return nil, fmt.Errorf("Invalid unit for the node drain grace period. Valid value is `minute|minutes|hour|hours`")
-		}
-		if nodeDrainParsed[1] == "hours" || nodeDrainParsed[1] == "hour" {
+		if nodeDrainParsed[1] == nodeDrainUnitHours || nodeDrainParsed[1] == nodeDrainUnitHour {
 			nodeDrainValue = nodeDrainValue * 60
 		}
 	}
 
 	valueBuilder.Value(nodeDrainValue).Unit("minutes")
 	return valueBuilder, nil
 }
+
+func ValidateNodeDrainGracePeriod(val interface{}) error {
+	nodeDrainGracePeriod := val.(string)
+	if nodeDrainGracePeriod == "" {
+		return nil
+	}
+
+	nodeDrainParsed := strings.Split(nodeDrainGracePeriod, " ")
+	if len(nodeDrainParsed) > 2 {
+		return fmt.Errorf("Expected format to include the duration and "+
+			"the unit (%s).", nodeDrainUnits)
+	}
+	nodeDrainValue, err := strconv.ParseInt(nodeDrainParsed[0], 10, 64)
+	if err != nil {
+		return fmt.Errorf("Invalid value '%s', the duration must be an integer.",
+			nodeDrainParsed[0])
+	}
+
+	// Default to minutes if no unit is specified
+	if len(nodeDrainParsed) > 1 {
+		if nodeDrainParsed[1] != nodeDrainUnitHours && nodeDrainParsed[1] != nodeDrainUnitHour &&
+			nodeDrainParsed[1] != "minutes" && nodeDrainParsed[1] != "minute" {
+			return fmt.Errorf("Invalid unit '%s', value unit is '%s'", nodeDrainParsed[1], nodeDrainUnits)
+		}
+		if nodeDrainParsed[1] == nodeDrainUnitHours || nodeDrainParsed[1] == nodeDrainUnitHour {
+			if nodeDrainValue > MaxNodeDrainTimeInHours {
+				return fmt.Errorf("Value '%v' cannot exceed the maximum of %d hours "+
+					"(1 week)", nodeDrainValue, MaxNodeDrainTimeInHours)
+			}
+			nodeDrainValue = nodeDrainValue * 60
+		}
+	}
+	if nodeDrainValue < 0 {
+		return fmt.Errorf("Value '%v' cannot be negative", nodeDrainValue)
+	}
+	if nodeDrainValue > MaxNodeDrainTimeInMinutes {
+		return fmt.Errorf("Value '%v' cannot exceed the maximum of %d minutes "+
+			"(1 week)", nodeDrainValue, MaxNodeDrainTimeInMinutes)
+	}
+	return nil
+}
diff --git a/pkg/helper/machinepools/helpers_test.go b/pkg/helper/machinepools/helpers_test.go
@@ -167,45 +167,96 @@ var _ = Describe("Label validations", func() {
 
 var _ = Describe("Create node drain grace period builder validations", func() {
 	DescribeTable("Create node drain grace period builder validations",
-		func(period string, hasError bool) {
+		func(period string, errMsg string) {
 			_, err := CreateNodeDrainGracePeriodBuilder(period)
-			if hasError {
+			if errMsg != "" {
 				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring(errMsg))
 			} else {
 				Expect(err).ToNot(HaveOccurred())
 			}
 		},
 		Entry("Should not error with empty value",
 			"",
-			false,
+			"",
 		),
 		Entry("Should not error with 0 value",
 			"0",
-			false,
+			"",
 		),
 		Entry("Should not error with lower limit value",
 			"1 minute",
-			false,
+			"",
+		),
+		Entry("Should not error with hour unit",
+			"1 hour",
+			"",
+		),
+		Entry("Should error if the time is not a numeric value",
+			"hour",
+			"Invalid time for the node drain grace period",
+		),
+	)
+})
+
+var _ = Describe("Validate node drain grace period", func() {
+	DescribeTable("Validate node drain grace period",
+		func(period interface{}, errMsg string) {
+			err := ValidateNodeDrainGracePeriod(period)
+			if errMsg != "" {
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring(errMsg))
+			} else {
+				Expect(err).ToNot(HaveOccurred())
+			}
+		},
+		Entry("Should not error with empty value",
+			"",
+			"",
+		),
+		Entry("Should not error with 0 value",
+			"0",
+			"",
+		),
+		Entry("Should not error with lower limit value",
+			"1 minute",
+			"",
 		),
 		Entry("Should not error with upper limit value",
 			"10080 minutes",
-			false,
+			"",
 		),
 		Entry("Should not error with hour unit",
 			"1 hour",
-			false,
+			"",
 		),
 		Entry("Should not error with hours unit",
 			"168 hours",
-			false,
+			"",
 		),
 		Entry("Should error with invalid number of tokens",
 			"1 minute later",
-			true,
+			"Expected format to include the duration",
 		),
 		Entry("Should error with invalid unit",
 			"1 day",
-			true,
+			"Invalid unit",
+		),
+		Entry("Should error with float value",
+			"1.1",
+			"duration must be an integer",
+		),
+		Entry("Should error with float value",
+			"-1 minute",
+			"cannot be negative",
+		),
+		Entry("Should error above upper limit minutes",
+			"10081 minutes",
+			"cannot exceed the maximum of 10080 minutes",
+		),
+		Entry("Should error above upper limit hours",
+			"169 hours",
+			"cannot exceed the maximum of 168 hours",
 		),
 	)
 })