diff --git a/changelog/v0.34.2/consistent-validation-output.yaml b/changelog/v0.34.2/consistent-validation-output.yaml new file mode 100644 index 000000000..946dee5a8 --- /dev/null +++ b/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 diff --git a/pkg/api/v2/reporter/reporter.go b/pkg/api/v2/reporter/reporter.go index 6b6eb7fca..00618b259 100644 --- a/pkg/api/v2/reporter/reporter.go +++ b/pkg/api/v2/reporter/reporter.go @@ -3,6 +3,7 @@ package reporter import ( "context" "os" + "slices" "sort" "strings" @@ -192,34 +193,122 @@ 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 resource to the map of resources with the same key + refMap[resKey] = append(refMap[resKey], res) + } + + // Get the list of name-namespace keys + refKeys = make([]string, len(refMap)) + i := 0 + for k := range refMap { + refKeys[i] = k + i++ + } + + // 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 } diff --git a/pkg/api/v2/reporter/reporter_test.go b/pkg/api/v2/reporter/reporter_test.go index 2f82c05d8..54d81f425 100644 --- a/pkg/api/v2/reporter/reporter_test.go +++ b/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, + }), + ) + + }) })