fix: SafeUpdateValidationResult not handling all edge cases (#104)

* fix: SafeUpdateValidationResult edge case returning Pass -> Fail -> PASS instead of Pass -> Fail -> FAIL Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com> * test: test SafeUpdateValidationResult; add codecov action Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com> --------- Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
validator-labs · Nov 12, 2023 · 8f34e2f · 8f34e2f
1 parent 21b3808
commit 8f34e2f
Show file tree

Hide file tree

Showing 3 changed files with 170 additions and 33 deletions.
diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
@@ -32,6 +32,11 @@ jobs:
       - name: Test
         run: make test
 
+      - name: Codecov
+        uses: codecov/codecov-action@v3
+        with:
+          file: ./cover.out
+
       - name: Update coverage report
         uses: ncruces/go-coverage-report@a12281c449c691078b2f4b6d39f2b3ae51a680cc # v0
         with:

diff --git a/pkg/validationresult/validation_result.go b/pkg/validationresult/validation_result.go
@@ -2,7 +2,6 @@ package validationresult
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -17,6 +16,8 @@ import (
 	"github.com/spectrocloud-labs/validator/pkg/util/ptr"
 )
 
+const validationErrorMsg = "Validation failed with an unexpected error"
+
 // HandleExistingValidationResult processes a preexisting validation result for the active validator
 func HandleExistingValidationResult(nn ktypes.NamespacedName, vr *v1alpha1.ValidationResult, l logr.Logger) {
 	switch vr.Status.State {
@@ -76,26 +77,51 @@ func HandleNewValidationResult(c client.Client, plugin string, expectedResults i
 
 // SafeUpdateValidationResult updates the overall validation result, ensuring
 // that the overall validation status remains failed if a single rule fails
-func SafeUpdateValidationResult(c client.Client, nn ktypes.NamespacedName, res *types.ValidationResult, err error, l logr.Logger) {
-	if err != nil {
-		res.State = ptr.Ptr(v1alpha1.ValidationFailed)
-		res.Condition.Status = corev1.ConditionFalse
-		res.Condition.Message = "Validation failed with an unexpected error"
-		res.Condition.Failures = append(res.Condition.Failures, err.Error())
+func SafeUpdateValidationResult(c client.Client, nn ktypes.NamespacedName, res *types.ValidationResult, resErr error, l logr.Logger) {
+
+	vr := &v1alpha1.ValidationResult{}
+	if err := c.Get(context.Background(), nn, vr); err != nil {
+		l.V(0).Error(err, "failed to get ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
+		return
 	}
-	if err := updateValidationResult(c, nn, res, l); err != nil {
-		l.V(0).Error(err, "failed to update ValidationResult")
+
+	updateValidationResult(vr, res, resErr)
+
+	if err := c.Status().Update(context.Background(), vr); err != nil {
+		l.V(0).Error(err, "failed to update ValidationResult", "name", nn.Name, "namespace", nn.Namespace)
+		return
 	}
+
+	l.V(0).Info(
+		"Updated ValidationResult", "state", res.State, "reason", res.Condition.ValidationRule,
+		"message", res.Condition.Message, "details", res.Condition.Details,
+		"failures", res.Condition.Failures, "time", res.Condition.LastValidationTime,
+	)
 }
 
 // updateValidationResult updates the ValidationResult for the active validation rule
-func updateValidationResult(c client.Client, nn ktypes.NamespacedName, res *types.ValidationResult, l logr.Logger) error {
-	vr := &v1alpha1.ValidationResult{}
-	if err := c.Get(context.Background(), nn, vr); err != nil {
-		return fmt.Errorf("failed to get ValidationResult %s in namespace %s: %v", nn.Name, nn.Namespace, err)
+func updateValidationResult(vr *v1alpha1.ValidationResult, res *types.ValidationResult, resErr error) {
+
+	// Finalize result State and Condition in the event of an unexpected error
+	if resErr != nil {
+		res.State = ptr.Ptr(v1alpha1.ValidationFailed)
+		res.Condition.Status = corev1.ConditionFalse
+		res.Condition.Message = validationErrorMsg
+		res.Condition.Failures = append(res.Condition.Failures, resErr.Error())
 	}
 
-	// reset to State to ValidationFailed if any conditions failed
+	// Update and/or insert the ValidationResult's Conditions with the latest Condition
+	idx := getConditionIndexByValidationRule(vr.Status.Conditions, res.Condition.ValidationRule)
+	if idx == -1 {
+		vr.Status.Conditions = append(vr.Status.Conditions, *res.Condition)
+	} else {
+		vr.Status.Conditions[idx] = *res.Condition
+	}
+
+	// Set State to:
+	// - ValidationFailed if ANY condition failed
+	// - ValidationSucceeded if ALL conditions succeeded
+	// - ValidationInProgress otherwise
 	vr.Status.State = *res.State
 	for _, c := range vr.Status.Conditions {
 		if c.Status == corev1.ConditionTrue {
@@ -106,25 +132,6 @@ func updateValidationResult(c client.Client, nn ktypes.NamespacedName, res *type
 			break
 		}
 	}
-
-	idx := getConditionIndexByValidationRule(vr.Status.Conditions, res.Condition.ValidationRule)
-	if idx == -1 {
-		vr.Status.Conditions = append(vr.Status.Conditions, *res.Condition)
-	} else {
-		vr.Status.Conditions[idx] = *res.Condition
-	}
-
-	if err := c.Status().Update(context.Background(), vr); err != nil {
-		l.V(0).Error(err, "failed to update ValidationResult")
-		return err
-	}
-	l.V(0).Info(
-		"Updated ValidationResult", "state", res.State, "reason", res.Condition.ValidationRule,
-		"message", res.Condition.Message, "details", res.Condition.Details,
-		"failures", res.Condition.Failures, "time", res.Condition.LastValidationTime,
-	)
-
-	return nil
 }
 
 // getInvalidConditions filters a ValidationCondition array and returns all conditions corresponding to a failed validation

diff --git a/pkg/validationresult/validation_result_test.go b/pkg/validationresult/validation_result_test.go
@@ -0,0 +1,125 @@
+package validationresult
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+
+	"github.com/spectrocloud-labs/validator/api/v1alpha1"
+	"github.com/spectrocloud-labs/validator/pkg/types"
+	"github.com/spectrocloud-labs/validator/pkg/util/ptr"
+)
+
+var err = errors.New("error")
+
+func res(s corev1.ConditionStatus, state v1alpha1.ValidationState) *types.ValidationResult {
+	return &types.ValidationResult{
+		Condition: &v1alpha1.ValidationCondition{
+			Status: s,
+		},
+		State: ptr.Ptr(state),
+	}
+}
+
+func vr(cs []corev1.ConditionStatus, state v1alpha1.ValidationState, err error) *v1alpha1.ValidationResult {
+	vr := &v1alpha1.ValidationResult{
+		Status: v1alpha1.ValidationResultStatus{
+			Conditions: make([]v1alpha1.ValidationCondition, 0),
+			State:      state,
+		},
+	}
+	for _, c := range cs {
+		condition := v1alpha1.ValidationCondition{
+			Status: c,
+		}
+		if err != nil {
+			condition.Message = validationErrorMsg
+			condition.Failures = append(condition.Failures, err.Error())
+		}
+		vr.Status.Conditions = append(vr.Status.Conditions, condition)
+	}
+	return vr
+}
+
+func TestUpdateValidationResult(t *testing.T) {
+	cs := []struct {
+		name       string
+		res        *types.ValidationResult
+		resErr     error
+		vrCurr     *v1alpha1.ValidationResult
+		vrExpected *v1alpha1.ValidationResult
+	}{
+		{
+			name:       "nil -> Pass -> PASS",
+			res:        res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr:     nil,
+			vrCurr:     vr(nil, v1alpha1.ValidationInProgress, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+		},
+		{
+			name:       "nil -> Error -> FAIL",
+			res:        res(corev1.ConditionFalse, v1alpha1.ValidationFailed),
+			resErr:     err,
+			vrCurr:     vr(nil, v1alpha1.ValidationInProgress, err),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, err),
+		},
+		{
+			name:       "nil -> Fail -> FAIL",
+			res:        res(corev1.ConditionFalse, v1alpha1.ValidationFailed),
+			resErr:     nil,
+			vrCurr:     vr(nil, v1alpha1.ValidationInProgress, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+		},
+		{
+			name:       "Pass -> Pass -> PASS",
+			res:        res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+		},
+		{
+			name:       "Pass -> Fail -> PASS",
+			res:        res(corev1.ConditionFalse, v1alpha1.ValidationFailed),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+		},
+		{
+			name:       "Fail -> Pass -> PASS",
+			res:        res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+		},
+		{
+			name:       "[Pass, Pass] -> Fail -> FAIL",
+			res:        res(corev1.ConditionFalse, v1alpha1.ValidationFailed),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionTrue}, v1alpha1.ValidationFailed, nil),
+		},
+		{
+			name:       "[Fail, Fail] -> Pass -> FAIL",
+			res:        res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionFalse}, v1alpha1.ValidationFailed, nil),
+		},
+		{
+			name:       "[Fail, Pass] -> Pass -> PASS",
+			res:        res(corev1.ConditionTrue, v1alpha1.ValidationSucceeded),
+			resErr:     nil,
+			vrCurr:     vr([]corev1.ConditionStatus{corev1.ConditionFalse, corev1.ConditionTrue}, v1alpha1.ValidationFailed, nil),
+			vrExpected: vr([]corev1.ConditionStatus{corev1.ConditionTrue, corev1.ConditionTrue}, v1alpha1.ValidationSucceeded, nil),
+		},
+	}
+	for _, c := range cs {
+		t.Log(c.name)
+		updateValidationResult(c.vrCurr, c.res, c.resErr)
+		if !reflect.DeepEqual(c.vrCurr.Hash(), c.vrExpected.Hash()) {
+			t.Errorf("expected (%+v), got (%+v)", c.vrExpected, c.vrCurr)
+		}
+	}
+}