Do not use MustParse with wrong StorageRequest size

In case of a wrong user input, the operator crashes without any chance
to recover.
The MustParse implementation wraps the ParseQuantity function, that
instead of panic() returns an error that can be easily handled at the
operator level. This change does not alter the current behavior, but
it adds a preliminary check that does not let the validation to continue
if the quantity is not valid. It also moves away from MustParse in favor
of ParseQuantity. By doing this we can get a webhook feedback about the
wrong quantity instead of checking the operators log and see the panic
stacktrace.

Signed-off-by: Francesco Pantano <fpantano@redhat.com>

modules/common/webhook/storage.go

@@ -37,11 +37,27 @@ import (
 //
 //     ValidateStorageRequest(<path>, "500M", "5G", true)
 func ValidateStorageRequest(basePath *field.Path, req string, min string, err bool) (admission.Warnings, field.ErrorList) {
-	storageRequestProd := resource.MustParse(min)
-	storageRequest := resource.MustParse(req)
 	allErrs := field.ErrorList{}
 	allWarn := []string{}
 
+	// If the StorageRequest is a wrong string, we must return
+	// an error. MustParse can't be used in this context as it
+	// generates panic() and we can't recover the operator.
+	storageRequest, parseError := resource.ParseQuantity(req)
+	if parseError != nil {
+		parseQuantityError := fmt.Sprintf("Field %s: %s is invalid",
+			basePath.Child("storageRequest").String(), req)
+		allErrs = append(allErrs, field.Invalid(basePath, req, parseQuantityError))
+		return allWarn, allErrs
+	}
+
+	storageRequestProd, parseError := resource.ParseQuantity(min)
+	if parseError != nil {
+		parseQuantityError := fmt.Sprintf("Invalid %s quantity", min)
+		allErrs = append(allErrs, field.Invalid(basePath, min, parseQuantityError))
+		return allWarn, allErrs
+	}
+
 	if storageRequest.Cmp(storageRequestProd) < 0 {
 		res := fmt.Sprintf("%s: %s is not appropriate for production! For production use at least %s!",
 			basePath.Child("storageRequest").String(), req, min)

modules/common/webhook/storage_test.go

@@ -67,6 +67,31 @@ func TestValidateStorageRequest(t *testing.T) {
 		},
 	}
 
+	wrongQuantityTest := []struct {
+		name     string
+		req      string
+		min      string
+		err      bool
+		wantErr  bool
+		wantWarn bool
+	}{
+		{
+			name: "req is a wrong string",
+			req:  "foo",
+			min:  "500M",
+		},
+		{
+			name: "min is a wrong string",
+			req:  "500M",
+			min:  "foo",
+		},
+		{
+			name: "both are wrong strings",
+			req:  "foo",
+			min:  "bar",
+		},
+	}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
@@ -86,4 +111,15 @@ func TestValidateStorageRequest(t *testing.T) {
 			}
 		})
 	}
+
+	for _, tt := range wrongQuantityTest {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			p := field.NewPath("foo")
+			// we always get an error when the input is a wrong quantity that
+			// can't be parsed
+			_, errs := ValidateStorageRequest(p, tt.req, tt.min, tt.err)
+			g.Expect(errs).To(HaveLen(1))
+		})
+	}
 }