Merge pull request #539 from JoelSpeed/mhc-maxunhealthy-parse

[release-4.4] Bug 1816606: Allow Int in String within MHC MaxUnhealthy

pkg/controller/machinehealthcheck/machinehealthcheck_controller.go

@@ -2,7 +2,11 @@ package machinehealthcheck
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"math"
+	"strconv"
+	"strings"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -228,7 +232,7 @@ func isAllowedRemediation(mhc *mapiv1.MachineHealthCheck) bool {
 	if mhc.Spec.MaxUnhealthy == nil {
 		return true
 	}
-	maxUnhealthy, err := intstr.GetValueFromIntOrPercent(mhc.Spec.MaxUnhealthy, derefInt(mhc.Status.ExpectedMachines), false)
+	maxUnhealthy, err := getValueFromIntOrPercent(mhc.Spec.MaxUnhealthy, derefInt(mhc.Status.ExpectedMachines), false)
 	if err != nil {
 		glog.Errorf("%s: error decoding maxUnhealthy, remediation won't be allowed: %v", namespacedName(mhc), err)
 		return false
@@ -673,3 +677,53 @@ func hasMatchingLabels(machineHealthCheck *mapiv1.MachineHealthCheck, machine *m
 	}
 	return true
 }
+
+// getValueFromIntOrPercent returns the integer number value based on the
+// percentage of the total or absolute number dependent on the IntOrString given
+//
+// The following code is copied from https://github.com/kubernetes/apimachinery/blob/1a505bc60c6dfb15cb18a8cdbfa01db042156fe2/pkg/util/intstr/intstr.go#L154-L185
+// But fixed so that string values aren't always assumed to be percentages
+// See https://github.com/kubernetes/kubernetes/issues/89082 for details
+func getValueFromIntOrPercent(intOrPercent *intstr.IntOrString, total int, roundUp bool) (int, error) {
+	if intOrPercent == nil {
+		return 0, errors.New("nil value for IntOrString")
+	}
+	value, isPercent, err := getIntOrPercentValue(intOrPercent)
+	if err != nil {
+		return 0, fmt.Errorf("invalid value for IntOrString: %v", err)
+	}
+	if isPercent {
+		if roundUp {
+			value = int(math.Ceil(float64(value) * (float64(total)) / 100))
+		} else {
+			value = int(math.Floor(float64(value) * (float64(total)) / 100))
+		}
+	}
+	return value, nil
+}
+
+// getIntOrPercentValue returns the integer value of the IntOrString and
+// determines if this value is a percentage or absolute number
+//
+// The following code is copied from https://github.com/kubernetes/apimachinery/blob/1a505bc60c6dfb15cb18a8cdbfa01db042156fe2/pkg/util/intstr/intstr.go#L154-L185
+// But fixed so that string values aren't always assumed to be percentages
+// See https://github.com/kubernetes/kubernetes/issues/89082 for details
+func getIntOrPercentValue(intOrStr *intstr.IntOrString) (int, bool, error) {
+	switch intOrStr.Type {
+	case intstr.Int:
+		return intOrStr.IntValue(), false, nil
+	case intstr.String:
+		isPercent := false
+		s := intOrStr.StrVal
+		if strings.Contains(s, "%") {
+			isPercent = true
+			s = strings.Replace(intOrStr.StrVal, "%", "", -1)
+		}
+		v, err := strconv.Atoi(s)
+		if err != nil {
+			return 0, isPercent, fmt.Errorf("invalid value %q: %v", intOrStr.StrVal, err)
+		}
+		return int(v), isPercent, nil
+	}
+	return 0, false, fmt.Errorf("invalid type: neither int nor percentage")
+}

pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go

@@ -2411,6 +2411,8 @@ func TestIsAllowedRemediation(t *testing.T) {
 	// short circuit if ever more than 2 out of 5 go unhealthy
 	maxUnhealthyInt := intstr.FromInt(2)
 	maxUnhealthyString := intstr.FromString("40%")
+	maxUnhealthyIntInString := intstr.FromString("2")
+	maxUnhealthyMixedString := intstr.FromString("foo%50")
 
 	testCases := []struct {
 		testCase string
@@ -2501,6 +2503,48 @@ func TestIsAllowedRemediation(t *testing.T) {
 			},
 			expected: false,
 		},
+		{
+			testCase: "not above maxUnhealthy (int in string)",
+			mhc: &mapiv1beta1.MachineHealthCheck{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: namespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "MachineHealthCheck",
+				},
+				Spec: mapiv1beta1.MachineHealthCheckSpec{
+					Selector:     metav1.LabelSelector{},
+					MaxUnhealthy: &maxUnhealthyIntInString,
+				},
+				Status: mapiv1beta1.MachineHealthCheckStatus{
+					ExpectedMachines: IntPtr(5),
+					CurrentHealthy:   IntPtr(3),
+				},
+			},
+			expected: true,
+		},
+		{
+			testCase: "above maxUnhealthy (int in string)",
+			mhc: &mapiv1beta1.MachineHealthCheck{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: namespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "MachineHealthCheck",
+				},
+				Spec: mapiv1beta1.MachineHealthCheckSpec{
+					Selector:     metav1.LabelSelector{},
+					MaxUnhealthy: &maxUnhealthyIntInString,
+				},
+				Status: mapiv1beta1.MachineHealthCheckStatus{
+					ExpectedMachines: IntPtr(5),
+					CurrentHealthy:   IntPtr(2),
+				},
+			},
+			expected: false,
+		},
 		{
 			testCase: "nil values",
 			mhc: &mapiv1beta1.MachineHealthCheck{
@@ -2522,6 +2566,27 @@ func TestIsAllowedRemediation(t *testing.T) {
 			},
 			expected: true,
 		},
+		{
+			testCase: "invalid string value",
+			mhc: &mapiv1beta1.MachineHealthCheck{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: namespace,
+				},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "MachineHealthCheck",
+				},
+				Spec: mapiv1beta1.MachineHealthCheckSpec{
+					Selector:     metav1.LabelSelector{},
+					MaxUnhealthy: &maxUnhealthyMixedString,
+				},
+				Status: mapiv1beta1.MachineHealthCheckStatus{
+					ExpectedMachines: nil,
+					CurrentHealthy:   nil,
+				},
+			},
+			expected: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -2533,6 +2598,74 @@ func TestIsAllowedRemediation(t *testing.T) {
 	}
 }
 
+func TestGetIntOrPercentValue(t *testing.T) {
+	int10 := intstr.FromInt(10)
+	percent20 := intstr.FromString("20%")
+	intInString30 := intstr.FromString("30")
+	invalidStringA := intstr.FromString("a")
+	invalidStringAPercent := intstr.FromString("a%")
+
+	testCases := []struct {
+		name            string
+		in              *intstr.IntOrString
+		expectedValue   int
+		expectedPercent bool
+		expectedError   error
+	}{
+		{
+			name:            "with a integer",
+			in:              &int10,
+			expectedValue:   10,
+			expectedPercent: false,
+			expectedError:   nil,
+		},
+		{
+			name:            "with a percentage",
+			in:              &percent20,
+			expectedValue:   20,
+			expectedPercent: true,
+			expectedError:   nil,
+		},
+		{
+			name:            "with an int in string",
+			in:              &intInString30,
+			expectedValue:   30,
+			expectedPercent: false,
+			expectedError:   nil,
+		},
+		{
+			name:            "with an 'a' string",
+			in:              &invalidStringA,
+			expectedValue:   0,
+			expectedPercent: false,
+			expectedError:   fmt.Errorf("invalid value \"a\": strconv.Atoi: parsing \"a\": invalid syntax"),
+		},
+		{
+			name:            "with an 'a%' string",
+			in:              &invalidStringAPercent,
+			expectedValue:   0,
+			expectedPercent: true,
+			expectedError:   fmt.Errorf("invalid value \"a%%\": strconv.Atoi: parsing \"a\": invalid syntax"),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			value, percent, err := getIntOrPercentValue(tc.in)
+			// Check first if one is nil, and the other isn't, otherwise if not nil, do the messages match
+			if (tc.expectedError != nil) != (err != nil) || err != nil && tc.expectedError.Error() != err.Error() {
+				t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, err, tc.expectedError)
+			}
+			if tc.expectedPercent != percent {
+				t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, percent, tc.expectedPercent)
+			}
+			if tc.expectedValue != value {
+				t.Errorf("Case: %s. Got: %v, expected: %v", tc.name, value, tc.expectedValue)
+			}
+		})
+	}
+}
+
 func IntPtr(i int) *int {
 	return &i
 }