Support validation again snapshot - consistent order of reporter output

changelog/v0.34.2/consistent-validation-output.yaml

@@ -0,0 +1,9 @@
+changelog:
+  - type: NON_USER_FACING
+    description: >
+      Updated reporter validation:
+      * Added ValidateSeparateWarnings to return warnings separately from errors
+      * Added ValidateSeparateWarningsAndErrors to provide a single method to validate and return warnings and errors separately to keep a consistent interface even if warnings will always be `nil`
+      * Updated all Valdiation to return errors and warnings in a consistent order
+    issueLink: https://github.com/solo-io/gloo/issues/8931
+    resolvesIssue: false

pkg/api/v2/reporter/reporter.go

@@ -3,6 +3,7 @@ package reporter
 import (
 	"context"
 	"os"
+	"slices"
 	"sort"
 	"strings"
 
@@ -192,34 +193,118 @@ func (e ResourceReports) FilterByKind(kind string) ResourceReports {
 	return reports
 }
 
-// ignores warnings
+// refMapAndSortedKeys returns a map of resource refs to resources and a sorted list of resource refs
+// This is used to iterate over the resources in a consistent order.
+// The reports are keyed by references to the resources, so are not sortable.
+// There is no unique key for a resource, so we use the string representation of the name/namespace as the key,
+// and collect all the resources with the same key together.
+// The previous implementation did not guarantee a consistent order when iterating over the resources,
+// so any order used here will be acceptable for backwards compatibility.
+func (e ResourceReports) refMapAndSortedKeys() (map[string][]resources.InputResource, []string) {
+	// refKeys contains all the unique keys for the resources
+	var refKeys []string
+	// refMaps is a map of resource keys to a slice of resources with that key
+	var refMap = make(map[string][]resources.InputResource)
+
+	// Loop over the resources
+	for res := range e {
+
+		// Get a string representation of the resource ref. This is not guaranteed to be unique.
+		resKey := res.GetMetadata().Ref().String()
+
+		// Add the key to the list of keys if it is not already there
+		if !slices.Contains(refKeys, resKey) {
+			refKeys = append(refKeys, resKey)
+		}
+		// Add the resource to the map of resources with the same key
+		refMap[resKey] = append(refMap[resKey], res)
+	}
+
+	// Sort the name-namespace keys. This will allow the reports to be accssed in a consistent order,
+	// except in cases where the name/namespace is not unique. In those cases, the individual validaiton
+	// functions will have to handle consistent ordering.
+	slices.Sort(refKeys)
+	return refMap, refKeys
+}
+
+// sortErrors sorts errors based on string representation
+// Note: because we are using multierror the string representation starts with "x errors occurred".
+// This will be consistent, but possibly counter-intuitive for tests.
+func sortErrors(errs []error) {
+	sort.Slice(errs, func(i, j int) bool {
+		return errs[i].Error() < errs[j].Error()
+	})
+}
+
+// Validate ignores warnings
 func (e ResourceReports) Validate() error {
 	var errs error
-	for res, rpt := range e {
-		if rpt.Errors != nil {
+	refMap, refKeys := e.refMapAndSortedKeys()
+
+	// the refKeys are sorted/sortable and point to the unsortable resources refs that are the keys to the report map
+	for _, refKey := range refKeys {
+		// name/namespace is not unique, so we collect those references together
+		reses := refMap[refKey]
+
+		var errsForKey []error
+		for _, res := range reses {
+			rpt := e[res]
+
+			if rpt.Errors != nil {
+				errsForKey = append(errsForKey, rpt.Errors)
+			}
+		}
+
+		if len(errsForKey) > 0 {
 			if errs == nil {
-				errs = errors.Errorf("invalid resource %v.%v", res.GetMetadata().Namespace, res.GetMetadata().Name)
+				// All of the resources in the group have the same metadata, so use the first one
+				errs = errors.Errorf("invalid resource %v.%v", reses[0].GetMetadata().Namespace, reses[0].GetMetadata().Name)
+			}
+			sortErrors(errsForKey)
+
+			for _, err := range errsForKey {
+				errs = multierror.Append(errs, err)
 			}
-			errs = multierror.Append(errs, rpt.Errors)
 		}
 	}
 	return errs
 }
 
-// does not ignore warnings
+// ValidateStrict does not ignore warnings. If warnings are present, they will be included in the error.
+// If an error is not present but warnings are, an "invalid resource" error will be returned along with each warning.
 func (e ResourceReports) ValidateStrict() error {
 	errs := e.Validate()
-	for res, rpt := range e {
-		if len(rpt.Warnings) > 0 {
+	refMap, refKeys := e.refMapAndSortedKeys()
+
+	for _, refKey := range refKeys {
+		var errsForKey []error
+		reses := refMap[refKey]
+
+		// name/namespace is not unique, so we collect those references together
+		for _, res := range reses {
+			rpt := e[res]
+			if len(rpt.Warnings) > 0 {
+				errsForKey = append(errsForKey, errors.Errorf("WARN: \n  %v", rpt.Warnings))
+
+			}
+		}
+
+		if len(errsForKey) > 0 {
 			if errs == nil {
+				// All of the resources in the group have the same metadata, so use the first one
 				errs = errors.Errorf(
 					"invalid resource %v.%v",
-					res.GetMetadata().GetNamespace(),
-					res.GetMetadata().GetName(),
+					reses[0].GetMetadata().GetNamespace(),
+					reses[0].GetMetadata().GetName(),
 				)
 			}
-			errs = multierror.Append(errs, errors.Errorf("WARN: \n  %v", rpt.Warnings))
+			sortErrors(errsForKey)
+
+			for _, err := range errsForKey {
+				errs = multierror.Append(errs, err)
+			}
 		}
+
 	}
 	return errs
 }

pkg/api/v2/reporter/reporter_test.go

@@ -5,13 +5,13 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/hashicorp/go-multierror"
 	"github.com/solo-io/go-utils/contextutils"
 	"github.com/solo-io/solo-kit/test/matchers"
 
 	"github.com/solo-io/solo-kit/pkg/utils/statusutils"
 
 	"github.com/golang/mock/gomock"
-	"github.com/hashicorp/go-multierror"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/solo-io/solo-kit/pkg/api/v1/clients"
@@ -484,4 +484,135 @@ var _ = Describe("Reporter", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 	})
+
+})
+
+var _ = Describe("Reporter", func() {
+	type expectedReports struct {
+		Validation             func() error
+		StrictValidation       func() error
+		SeparateValidationErr  func() error
+		SeparateValidationWarn func() error
+	}
+
+	var (
+		mockResourceClient, mockResourceClient2, mockResourceClient3 clients.ResourceClient
+	)
+
+	BeforeEach(func() {
+		mockResourceClient = memory.NewResourceClient(memory.NewInMemoryResourceCache(), &v1.MockResource{})
+		mockResourceClient2 = memory.NewResourceClient(memory.NewInMemoryResourceCache(), &v1.MockResource{})
+		mockResourceClient3 = memory.NewResourceClient(memory.NewInMemoryResourceCache(), &v1.MockResource{})
+		// By default, DisableTruncateStatus is false, unless users opt into it
+		// To mirror that in our tests, we explicitly set it to false unless a test requires it
+		rep.DisableTruncateStatus = false
+	})
+
+	Context("Validation", func() {
+		var (
+			r1, r2, r3, r4, r5 resources.Resource
+		)
+		initResources := func() {
+			var err error
+			r1, err = mockResourceClient.Write(v1.NewMockResource("test-ns", "testres1"), clients.WriteOpts{})
+			Expect(err).NotTo(HaveOccurred())
+			r2, err = mockResourceClient.Write(v1.NewMockResource("test-ns", "testres2"), clients.WriteOpts{})
+			Expect(err).NotTo(HaveOccurred())
+			r3, err = mockResourceClient.Write(v1.NewMockResource("test-ns", "testres3"), clients.WriteOpts{})
+			Expect(err).NotTo(HaveOccurred())
+			r4, err = mockResourceClient2.Write(v1.NewMockResource("test-ns", "testres1"), clients.WriteOpts{})
+			Expect(err).NotTo(HaveOccurred())
+			r5, err = mockResourceClient3.Write(v1.NewMockResource("test-ns", "testres1"), clients.WriteOpts{})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(r1).NotTo(BeNil())
+			Expect(r2).NotTo(BeNil())
+			Expect(r3).NotTo(BeNil())
+			Expect(r4).NotTo(BeNil())
+			Expect(r5).NotTo(BeNil())
+			Expect(r3).NotTo(BeNil())
+		}
+
+		// r0, r4, and r1 are all for the same resource key
+		// though the errors are sorted, the `r1` errors come after the `r4` errors because the errors
+		// are multierrors and when compared, are sorted by the number of errors contaiend.
+		// `r1` has 3 errors, so comes after `r0` and `r4`, which have one each.
+		validateReports := func() rep.ResourceReports {
+			return rep.ResourceReports{
+				r1.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r1err1"), fmt.Errorf("r1err2"), fmt.Errorf("r1err0")}}, Warnings: []string{"r1warn1", "r1warn2"}},
+				r2.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r2err1")}}},
+				r3.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r3err1")}}, Warnings: []string{"r3warn1", "r3warn0"}},
+				r4.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r4err1")}}, Warnings: []string{"r4warn1", "r4warn0"}},
+				r5.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r0err1")}}, Warnings: []string{"r0warn1"}},
+			}
+		}
+
+		validateReportsReordered := func() rep.ResourceReports {
+			return rep.ResourceReports{
+				r5.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r0err1")}}, Warnings: []string{"r0warn1"}},
+				r4.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r4err1")}}, Warnings: []string{"r4warn1", "r4warn0"}},
+				r3.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r3err1")}}, Warnings: []string{"r3warn1", "r3warn0"}},
+				r2.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r2err1")}}},
+				r1.(*v1.MockResource): rep.Report{Errors: &multierror.Error{Errors: []error{fmt.Errorf("r1err1"), fmt.Errorf("r1err2"), fmt.Errorf("r1err0")}}, Warnings: []string{"r1warn1", "r1warn2"}},
+			}
+		}
+
+		expectedValidateErrors := func() error {
+			var expectedErr error
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("invalid resource test-ns.testres1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r0err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r4err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err2"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err0"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r2err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r3err1"))
+
+			return expectedErr
+		}
+
+		expectedValidateStrictErrors := func() error {
+			var expectedErr error
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("invalid resource test-ns.testres1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r0err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r4err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err2"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r1err0"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r2err1"))
+			expectedErr = multierror.Append(expectedErr, fmt.Errorf("r3err1"))
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("WARN: \n  %v", []string{"r0warn1"}))
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("WARN: \n  %v", []string{"r1warn1", "r1warn2"}))
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("WARN: \n  %v", []string{"r4warn1", "r4warn0"}))
+			expectedErr = multierror.Append(expectedErr, errors.Errorf("WARN: \n  %v", []string{"r3warn1", "r3warn0"}))
+
+			return expectedErr
+		}
+
+		BeforeEach(func() {
+			initResources()
+		})
+
+		// Run these tests multiple times to ensure that the validation errors are ordered consistently
+		DescribeTable("Validation functions should return the expected reports for a set of reports", MustPassRepeatedly(5), func(getReports func() rep.ResourceReports, expectedResults expectedReports) {
+			reports := getReports()
+			// Validate - Regular validation
+			err := reports.Validate()
+			Expect(err.Error()).To(Equal(expectedResults.Validation().Error()))
+
+			// ValidateStrict - Strict validation
+			err = reports.ValidateStrict()
+			Expect(err.Error()).To(Equal(expectedResults.StrictValidation().Error()))
+
+		},
+			Entry("validate reports", validateReports, expectedReports{
+				Validation:       expectedValidateErrors,
+				StrictValidation: expectedValidateStrictErrors,
+			}),
+			Entry("validate reordered reports", validateReportsReordered, expectedReports{
+				Validation:       expectedValidateErrors,
+				StrictValidation: expectedValidateStrictErrors,
+			}),
+		)
+
+	})
 })