From 4d57055434a575f1b235788167b1d0f7e0c87c79 Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Wed, 16 Nov 2022 08:43:57 +0100 Subject: [PATCH] improve error message for duplicate sibling exports (#636) * improve error message for duplicate sibling exports * enhance graph tests to verify the found cycle * refactor cyclic dependency detection * improve error message for cyclic dependencies in subinstallations * test duplicate export detection and error message * improve error message for duplicate installationTemplate exports --- apis/core/validation/blueprint.go | 26 +- apis/core/validation/blueprint_test.go | 8 +- pkg/utils/dependencies/dependencies.go | 91 +++++-- pkg/utils/dependencies/dependencies_test.go | 40 ++++ pkg/utils/dependencies/graph.go | 77 +++--- pkg/utils/dependencies/graph_test.go | 225 ++++++++++++++++-- pkg/utils/dependencies/queue/queue.go | 59 +++++ .../dependencies/queue/queue_suite_test.go | 99 ++++++++ pkg/utils/landscaper/blueprint.go | 2 +- .../apis/core/validation/blueprint.go | 26 +- 10 files changed, 553 insertions(+), 100 deletions(-) create mode 100644 pkg/utils/dependencies/queue/queue.go create mode 100644 pkg/utils/dependencies/queue/queue_suite_test.go diff --git a/apis/core/validation/blueprint.go b/apis/core/validation/blueprint.go index c754ed6c7..4485a6fb7 100644 --- a/apis/core/validation/blueprint.go +++ b/apis/core/validation/blueprint.go @@ -255,9 +255,9 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co allErrs = field.ErrorList{} names = sets.NewString() importedDataObjects = make([]Import, 0) - exportedDataObjects = sets.NewString() + exportedDataObjects = map[string]string{} importedTargets = make([]Import, 0) - exportedTargets = sets.NewString() + exportedTargets = map[string]string{} blueprintDataImports = sets.NewString() blueprintTargetImports = sets.NewString() @@ -308,14 +308,15 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co //} for i, do := range instTmpl.Exports.Data { - dataPath := instPath.Child("exports").Child("data").Index(i).Key(do.Name) - if exportedDataObjects.Has(do.DataRef) { - allErrs = append(allErrs, field.Forbidden(dataPath, "export is already exported by another installation")) + dataPath := instPath.Child("exports").Child("data").Index(i).Key(fmt.Sprintf("%s/%s", do.Name, do.DataRef)) + if dup, ok := exportedDataObjects[do.DataRef]; ok { + allErrs = append(allErrs, field.Forbidden(dataPath, fmt.Sprintf("data export '%s' is already exported by %s", do.DataRef, dup))) + } else { + exportedDataObjects[do.DataRef] = dataPath.String() } if blueprintDataImports.Has(do.DataRef) { allErrs = append(allErrs, field.Forbidden(dataPath, "export is imported by its parent")) } - exportedDataObjects.Insert(do.DataRef) } for i, do := range instTmpl.Imports.Data { importedDataObjects = append(importedDataObjects, Import{ @@ -324,14 +325,15 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co }) } for i, target := range instTmpl.Exports.Targets { - targetPath := instPath.Child("exports").Child("targets").Index(i).Key(target.Name) - if exportedTargets.Has(target.Target) { - allErrs = append(allErrs, field.Forbidden(targetPath, "export is already exported by another installation")) + targetPath := instPath.Child("exports").Child("targets").Index(i).Key(fmt.Sprintf("%s/%s", target.Name, target.Target)) + if dup, ok := exportedTargets[target.Target]; ok { + allErrs = append(allErrs, field.Forbidden(targetPath, fmt.Sprintf("target export '%s' is already exported by %s", target.Target, dup))) + } else { + exportedTargets[target.Target] = targetPath.String() } if blueprintTargetImports.Has(target.Target) { allErrs = append(allErrs, field.Forbidden(targetPath, "export is imported by its parent")) } - exportedTargets.Insert(target.Target) } for i, target := range instTmpl.Imports.Targets { impPath := instPath.Child("imports").Child("targets").Index(i).Key(target.Name) @@ -364,8 +366,8 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co } // validate that all imported values are either satisfied by the blueprint or by another sibling - allErrs = append(allErrs, ValidateSatisfiedImports(blueprintDataImports, exportedDataObjects, importedDataObjects)...) - allErrs = append(allErrs, ValidateSatisfiedImports(blueprintTargetImports, exportedTargets, importedTargets)...) + allErrs = append(allErrs, ValidateSatisfiedImports(blueprintDataImports, sets.StringKeySet(exportedDataObjects), importedDataObjects)...) + allErrs = append(allErrs, ValidateSatisfiedImports(blueprintTargetImports, sets.StringKeySet(exportedTargets), importedTargets)...) return allErrs } diff --git a/apis/core/validation/blueprint_test.go b/apis/core/validation/blueprint_test.go index 7b32beebc..0c02a072c 100644 --- a/apis/core/validation/blueprint_test.go +++ b/apis/core/validation/blueprint_test.go @@ -449,11 +449,11 @@ var _ = Describe("Blueprint", func() { []*core.InstallationTemplate{tmpl1, tmpl2}) Expect(allErrs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeForbidden), - "Field": Equal("b[0].exports.data[0][myimport]"), + "Field": Equal("b[0].exports.data[0][myimport/myimportref]"), })))) Expect(allErrs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeForbidden), - "Field": Equal("b[1].exports.data[0][mysecondexport]"), + "Field": Equal("b[1].exports.data[0][mysecondexport/mysecondexportref]"), })))) }) @@ -494,11 +494,11 @@ var _ = Describe("Blueprint", func() { []*core.InstallationTemplate{tmpl1, tmpl2}) Expect(allErrs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeForbidden), - "Field": Equal("b[0].exports.targets[0][myimport]"), + "Field": Equal("b[0].exports.targets[0][myimport/myimportref]"), })))) Expect(allErrs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{ "Type": Equal(field.ErrorTypeForbidden), - "Field": Equal("b[1].exports.targets[0][mysecondexport]"), + "Field": Equal("b[1].exports.targets[0][mysecondexport/mysecondexportref]"), })))) }) }) diff --git a/pkg/utils/dependencies/dependencies.go b/pkg/utils/dependencies/dependencies.go index 4698fb4f4..c35b2bca3 100644 --- a/pkg/utils/dependencies/dependencies.go +++ b/pkg/utils/dependencies/dependencies.go @@ -1,7 +1,13 @@ +// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Gardener contributors. +// +// SPDX-License-Identifier: Apache-2.0 + package dependencies import ( + "errors" "fmt" + "strings" "k8s.io/apimachinery/pkg/util/sets" @@ -17,7 +23,7 @@ func FetchPredecessorsFromInstallation(installation *lsv1alpha1.Installation, otherNodes = append(otherNodes, newInstallationNodeFromInstallation(next)) } - predecessors, _ := instNode.fetchPredecessors(otherNodes, false) + predecessors, _ := instNode.fetchPredecessors(otherNodes) return predecessors } @@ -30,9 +36,9 @@ func CheckForCyclesAndDuplicateExports(instTemplates []*lsv1alpha1.InstallationT edges := map[string]sets.String{} for _, next := range instNodes { - predecessors, hasDublicateExports := next.fetchPredecessors(instNodes, true) - if hasDublicateExports { - return nil, fmt.Errorf("the installation %s gets imports with same name from different predecessors", next.name) + predecessors, err := next.fetchPredecessors(instNodes) + if err != nil { + return nil, err } edges[next.name] = predecessors } @@ -42,7 +48,7 @@ func CheckForCyclesAndDuplicateExports(instTemplates []*lsv1alpha1.InstallationT hasCycle, cycle := g.hasCycle() if hasCycle { - return nil, fmt.Errorf("the subinstallations have a cycle: %s", cycle) + return nil, fmt.Errorf("the subinstallations have a cycle: %s", strings.Join(cycle, " -{depends_on}-> ")) } if computeOrder { @@ -88,11 +94,33 @@ func newInstallationNodeFromInstallationTemplate(installation *lsv1alpha1.Instal } } -func (r *installationNode) fetchPredecessors(otherNodes []*installationNode, checkDuplicateExports bool) (sets.String, bool) { - dataExports, targetExports, hasDuplicateExports := r.getExportMaps(otherNodes, checkDuplicateExports) +func (r *installationNode) fetchPredecessors(otherNodes []*installationNode) (sets.String, error) { + dataExports, targetExports, hasDuplicateExports := r.getExportMaps(otherNodes) if hasDuplicateExports { - return nil, hasDuplicateExports + msg := strings.Builder{} + msg.WriteString("the following exports are exported by multiple nested installations:") + dupExpFound := false + for exp, sources := range dataExports { + if sources.Len() > 1 { + if !dupExpFound { + dupExpFound = true + msg.WriteString("\n data exports:") + } + msg.WriteString(fmt.Sprintf("\n '%s' is exported by [%s]", exp, strings.Join(sources.List(), ", "))) + } + } + dupExpFound = false + for exp, sources := range targetExports { + if sources.Len() > 1 { + if !dupExpFound { + dupExpFound = true + msg.WriteString("\n target exports:") + } + msg.WriteString(fmt.Sprintf("\n '%s' is exported by [%s]", exp, strings.Join(sources.List(), ", "))) + } + } + return nil, errors.New(msg.String()) } predecessors := sets.NewString() @@ -101,11 +129,11 @@ func (r *installationNode) fetchPredecessors(otherNodes []*installationNode, che // only dataRef imports can refer to sibling exports continue } - source, ok := dataExports[imp.DataRef] + sources, ok := dataExports[imp.DataRef] if ok { // no sibling exports this import, it has to come from the parent // this is already checked by validation, no need to verify it here - predecessors.Insert(source) + predecessors.Insert(sources.UnsortedList()...) } } @@ -122,22 +150,25 @@ func (r *installationNode) fetchPredecessors(otherNodes []*installationNode, che } for _, target := range targets { - source, ok := targetExports[target] + sources, ok := targetExports[target] if !ok { // no sibling exports this import, it has to come from the parent // this is already checked by validation, no need to verify it here continue } - predecessors.Insert(source) + predecessors.Insert(sources.UnsortedList()...) } } - return predecessors, hasDuplicateExports + return predecessors, nil } -func (r *installationNode) getExportMaps(otherNodes []*installationNode, checkDuplicateExports bool) (map[string]string, map[string]string, bool) { - dataExports := map[string]string{} - targetExports := map[string]string{} +// getExportMaps returns a mapping from sibling export names to the exporting siblings' names. +// If for any given key the length of its value (a set) is greater than 1, this means that two or more siblings define the same export. +// The third returned parameter indicates whether this has happened or not (true in case of duplicate exports). +func (r *installationNode) getExportMaps(otherNodes []*installationNode) (map[string]sets.String, map[string]sets.String, bool) { + dataExports := map[string]sets.String{} + targetExports := map[string]sets.String{} hasDuplicateExports := false for _, sibling := range otherNodes { @@ -146,18 +177,30 @@ func (r *installationNode) getExportMaps(otherNodes []*installationNode, checkDu continue } for _, exp := range sibling.exports.Data { - _, ok := dataExports[exp.DataRef] - if checkDuplicateExports && ok { - return nil, nil, true + var de sets.String + var ok bool + de, ok = dataExports[exp.DataRef] + if !ok { + de = sets.NewString() + } + de.Insert(sibling.name) + if !hasDuplicateExports && de.Len() > 1 { + hasDuplicateExports = true } - dataExports[exp.DataRef] = sibling.name + dataExports[exp.DataRef] = de } for _, exp := range sibling.exports.Targets { - _, ok := targetExports[exp.Target] - if checkDuplicateExports && ok { - return nil, nil, true + var te sets.String + var ok bool + te, ok = targetExports[exp.Target] + if !ok { + te = sets.NewString() + } + te.Insert(sibling.name) + if !hasDuplicateExports && te.Len() > 1 { + hasDuplicateExports = true } - targetExports[exp.Target] = sibling.name + targetExports[exp.Target] = te } } diff --git a/pkg/utils/dependencies/dependencies_test.go b/pkg/utils/dependencies/dependencies_test.go index 747f3e017..18435d332 100644 --- a/pkg/utils/dependencies/dependencies_test.go +++ b/pkg/utils/dependencies/dependencies_test.go @@ -13,6 +13,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/types" ) var _ = Describe("Cyclic Dependency Determination Tests", func() { @@ -108,6 +109,45 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { sortInstallationTemplatesAlphabetically(tmpls) _, err := CheckForCyclesAndDuplicateExports(tmpls, true) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(SatisfyAll(ContainSubstring("c -{depends_on}-> e"), ContainSubstring("d -{depends_on}-> c"), ContainSubstring("e -{depends_on}-> d"))) + }) + + It("should detect duplicate data exports", func() { + deps := map[string][]string{ + "a": nil, + "b": {"a"}, + "c": nil, + } + tmpls := generateSubinstallationTemplates(deps, newDependencyProvider(dataDependency)) + sortInstallationTemplatesAlphabetically(tmpls) + tmpls[0].Exports.Data = append(tmpls[0].Exports.Data, lsv1alpha1.DataExport{Name: "addExp", DataRef: "addExp"}) + tmpls[len(tmpls)-1].Exports.Data = tmpls[0].Exports.Data + _, err := CheckForCyclesAndDuplicateExports(tmpls, true) + Expect(err).To(HaveOccurred()) + matchers := []types.GomegaMatcher{} + for _, exp := range tmpls[0].Exports.Data { + matchers = append(matchers, ContainSubstring("'%s' is exported by [%s]", exp.DataRef, strings.Join([]string{tmpls[0].Name, tmpls[len(tmpls)-1].Name}, ", "))) + } + Expect(err.Error()).To(SatisfyAll(matchers...)) + }) + + It("should detect duplicate target exports", func() { + deps := map[string][]string{ + "a": nil, + "b": {"a"}, + "c": nil, + } + tmpls := generateSubinstallationTemplates(deps, newDependencyProvider(targetDependency)) + sortInstallationTemplatesAlphabetically(tmpls) + tmpls[0].Exports.Targets = append(tmpls[0].Exports.Targets, lsv1alpha1.TargetExport{Name: "addExp", Target: "addExp"}) + tmpls[len(tmpls)-1].Exports.Targets = tmpls[0].Exports.Targets + _, err := CheckForCyclesAndDuplicateExports(tmpls, true) + Expect(err).To(HaveOccurred()) + matchers := []types.GomegaMatcher{} + for _, exp := range tmpls[0].Exports.Targets { + matchers = append(matchers, ContainSubstring("'%s' is exported by [%s]", exp.Target, strings.Join([]string{tmpls[0].Name, tmpls[len(tmpls)-1].Name}, ", "))) + } + Expect(err.Error()).To(SatisfyAll(matchers...)) }) }) diff --git a/pkg/utils/dependencies/graph.go b/pkg/utils/dependencies/graph.go index 9f196a6f5..2d8557770 100644 --- a/pkg/utils/dependencies/graph.go +++ b/pkg/utils/dependencies/graph.go @@ -1,9 +1,15 @@ +// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Gardener contributors. +// +// SPDX-License-Identifier: Apache-2.0 + package dependencies import ( "fmt" "k8s.io/apimachinery/pkg/util/sets" + + "github.com/gardener/landscaper/pkg/utils/dependencies/queue" ) type graph struct { @@ -14,6 +20,22 @@ type graph struct { alreadyCheckedElements sets.String } +// nodeWithPath stores a node of a graph as well as the path that led to this node. +// This is a helper struct for the breadth first search below. +type nodeWithPath struct { + node string + path []string +} + +func (nwp nodeWithPath) hasVisitedBefore(elem string) bool { + for _, p := range nwp.path { + if p == elem { + return true + } + } + return false +} + func newGraph(edges map[string]sets.String) *graph { return &graph{ edges: edges, @@ -22,44 +44,37 @@ func newGraph(edges map[string]sets.String) *graph { } func (g *graph) hasCycle() (bool, []string) { + graphNodes := queue.New[nodeWithPath]() for node := range g.edges { - visited := make(map[string]bool, 0) - hasCycle, cycle := g.canReachCycle(node, visited) - if hasCycle { - return true, append(cycle, node) - } + graphNodes.Append(nodeWithPath{ + node: node, + path: []string{node}, + }) } - - return false, nil + // start a breadth first search for cycles from all graph nodes simultaneously + shortestCycle := g.breadthFirstSearchForCycles(graphNodes) + return shortestCycle != nil, shortestCycle } -// canReachCycle returns true if starting from the given node one can reach an element of a cycle. -// Assumptions: 1. from each visited element there exists a path to the given node, and -// 2. the node itself is not a visited element. -func (g *graph) canReachCycle(node string, visited map[string]bool) (bool, []string) { - if g.alreadyCheckedElements.Has(node) { - return false, nil - } - - visited[node] = true - - successors := g.edges[node] - for succ := range successors { - if visited[succ] { - return true, []string{succ} - } else { - hasCycle, cycle := g.canReachCycle(succ, visited) - if hasCycle { - return true, append(cycle, succ) +// breadthFirstSearchForCycles searches the graph for cycles. +// It returns the first cycle found. +// If no cycle is found, nil is returned. +func (g *graph) breadthFirstSearchForCycles(todo queue.Queue[nodeWithPath]) []string { + for !todo.IsEmpty() { + cur, _ := todo.Pop() + successors := g.edges[cur.node] + for _, succ := range successors.List() { + newPath := append(cur.path, succ) + if cur.hasVisitedBefore(succ) { + return newPath } + todo.Append(nodeWithPath{ + node: succ, + path: newPath, + }) } } - - visited[node] = false - - g.alreadyCheckedElements.Insert(node) - - return false, nil + return nil } func (g *graph) getReverseOrder() ([]string, error) { diff --git a/pkg/utils/dependencies/graph_test.go b/pkg/utils/dependencies/graph_test.go index d7d157595..c58c4de18 100644 --- a/pkg/utils/dependencies/graph_test.go +++ b/pkg/utils/dependencies/graph_test.go @@ -6,9 +6,13 @@ package dependencies import ( "fmt" + "reflect" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" + gomegamatchers "github.com/onsi/gomega/matchers" + "github.com/onsi/gomega/types" "k8s.io/apimachinery/pkg/util/sets" ) @@ -16,16 +20,6 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { Context("DetermineCyclicDependencies", func() { - It("should not detect cyclic dependencies in one element cycle", func() { - deps := map[string]sets.String{ - "a": sets.NewString().Insert("a"), - } - - hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) - Expect(hasCycle).To(BeTrue()) - }) - It("should not detect cyclic dependencies if there aren't any", func() { deps := map[string]sets.String{ "c": sets.NewString().Insert("b"), @@ -34,8 +28,8 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { } hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) Expect(hasCycle).To(BeFalse()) + Expect(cycle).To(BeEmpty()) }) It("should detect simple cyclic dependencies", func() { @@ -46,8 +40,8 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { } hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) Expect(hasCycle).To(BeTrue()) + Expect(cycle).To(CyclicEqual("c", "b", "a")) }) It("should detect one-elemented cyclic dependencies", func() { @@ -56,8 +50,8 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { } hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) Expect(hasCycle).To(BeTrue()) + Expect(cycle).To(CyclicEqual("a")) }) It("should detect multiple independent cycles", func() { @@ -69,8 +63,8 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { } hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) Expect(hasCycle).To(BeTrue()) + Expect(cycle).To(SatisfyAny(CyclicEqual("c", "d"), CyclicEqual("a", "b"))) }) It("should detect multiple connected cycles", func() { @@ -82,11 +76,12 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { } hasCycle, cycle := newGraph(deps).hasCycle() - fmt.Println(cycle) Expect(hasCycle).To(BeTrue()) + // logic detects only the shortest cycle + Expect(cycle).To(CyclicEqual("c", "d")) }) - It("should detect multiple connected cycles", func() { + It("should order the graph correctly according to its edges", func() { deps := map[string]sets.String{ "a": sets.NewString().Insert("c"), "b": sets.NewString().Insert("f", "e"), @@ -118,3 +113,201 @@ var _ = Describe("Cyclic Dependency Determination Tests", func() { }) }) }) + +type CyclicEqualMatcher struct { + Elements []interface{} + lengthMismatch bool + firstExpectedNotFound bool + first, actualSecond, expectedSecond interface{} +} + +func (ce CyclicEqualMatcher) String() string { + return fmt.Sprintf("{cyclically equal to %s}", presentable(ce.Elements)) +} + +// CyclicEquals succeeds if both arrays/slices contain the same elements in the same order. +// The arrays/slices are considered to not have a beginning or end, but be wrapped around, +// so [a, b, c] and [c, a, b] would be considered equal, while [a, c, b] is different from both. +// If the first and the last element of either actual or expected are equal, the last element is removed from the +// respective array/slice before comparison. +// The Equal matcher will be used for comparison. +func CyclicEqual(elements ...interface{}) types.GomegaMatcher { + return &CyclicEqualMatcher{ + Elements: elements, + lengthMismatch: false, + firstExpectedNotFound: false, + } +} + +func (ce *CyclicEqualMatcher) Match(actual interface{}) (success bool, err error) { + if !isArrayOrSlice(actual) { + return false, fmt.Errorf("CyclicEqual matcher expects an array/slice. Got:\n%s", format.Object(actual, 1)) + } + expecteds := ce.Elements + actuals := valuesOf(actual) + + if len(expecteds) == 0 { + if len(actuals) == 0 { + return true, nil + } + ce.lengthMismatch = true + return false, nil + } + + // if the cycle array has the same element at beginning and end, cut one off + if len(actuals) > 1 { + first := actuals[0] + last := actuals[len(actuals)-1] + succ, err := equals(first, last) + if err != nil { + return false, err + } + if succ { + // first and last element are equal, cut last element off + actuals = actuals[:len(actuals)-1] + } + } + if len(expecteds) > 1 { + first := expecteds[0] + last := expecteds[len(expecteds)-1] + succ, err := equals(first, last) + if err != nil { + return false, err + } + if succ { + // first and last element are equal, cut last element off + expecteds = expecteds[:len(expecteds)-1] + } + } + + if len(expecteds) != len(actuals) { + ce.lengthMismatch = true + return false, nil + } + + // Idea: iterate over actual until the first element of expected is found. + // Then iterate over both, comparing the elements. If the end of actual is reached, + // continue from the beginning. + // If any two elements don't match, the cycles are not identical. + skew := 0 + for ; skew < len(actuals); skew++ { + succ, err := equals(actuals[skew], expecteds[0]) + if err != nil { + return false, err + } + if succ { + break + } + } + if skew == len(actuals) { + ce.firstExpectedNotFound = true + return false, nil + } + i := 1 + for ; i < len(expecteds); i++ { + j := (skew + i) % len(actuals) + succ, err := equals(actuals[j], expecteds[i]) + if err != nil { + return false, err + } + if !succ { + ce.first = expecteds[i-1] + ce.actualSecond = actuals[j] + ce.expectedSecond = expecteds[i] + return false, nil + } + } + + return true, nil +} + +func (ce *CyclicEqualMatcher) FailureMessage(actual interface{}) string { + message := format.Message(actual, "to be cyclically equal to", presentable(ce.Elements)) + if ce.lengthMismatch { + message = fmt.Sprintf("%s\nbut the amount of unique elements in both cycles is different", message) + } else if ce.firstExpectedNotFound { + message = fmt.Sprintf("%s\nbut the actual value is missing the first expected element\n%s", message, format.Object(ce.Elements[0], 1)) + } else { + message = fmt.Sprintf("%s\nbut element\n%s\nis followed by element\n%s\nbut it was expected to be followed by element\n%s", + message, + format.Object(ce.first, 1), + format.Object(ce.actualSecond, 1), + format.Object(ce.expectedSecond, 1)) + } + return message +} + +func (ce *CyclicEqualMatcher) NegatedFailureMessage(actual interface{}) string { + return format.Message(actual, "not to be cyclically equal to", presentable(ce.Elements)) +} + +// copied from gomega library +func isArrayOrSlice(a interface{}) bool { + if a == nil { + return false + } + switch reflect.TypeOf(a).Kind() { + case reflect.Array, reflect.Slice: + return true + default: + return false + } +} + +// copied from gomega library and modified +func valuesOf(actual interface{}) []interface{} { + value := reflect.ValueOf(actual) + values := []interface{}{} + for i := 0; i < value.Len(); i++ { + values = append(values, value.Index(i).Interface()) + } + + return values +} + +// copied from gomega library +func presentable(elems []interface{}) interface{} { + elems = flatten(elems) + + if len(elems) == 0 { + return []interface{}{} + } + + sv := reflect.ValueOf(elems) + tt := sv.Index(0).Elem().Type() + for i := 1; i < sv.Len(); i++ { + if sv.Index(i).Elem().Type() != tt { + return elems + } + } + + ss := reflect.MakeSlice(reflect.SliceOf(tt), sv.Len(), sv.Len()) + for i := 0; i < sv.Len(); i++ { + ss.Index(i).Set(sv.Index(i).Elem()) + } + + return ss.Interface() +} + +// copied from gomega library +func flatten(elems []interface{}) []interface{} { + if len(elems) != 1 || !isArrayOrSlice(elems[0]) { + return elems + } + + value := reflect.ValueOf(elems[0]) + flattened := make([]interface{}, value.Len()) + for i := 0; i < value.Len(); i++ { + flattened[i] = value.Index(i).Interface() + } + return flattened +} + +func equals(actual, expected interface{}) (bool, error) { + em := gomegamatchers.EqualMatcher{Expected: expected} + succ, err := em.Match(actual) + if err != nil { + return succ, fmt.Errorf("error comparing elements %v and %v: %w", actual, expected, err) + } + return succ, nil +} diff --git a/pkg/utils/dependencies/queue/queue.go b/pkg/utils/dependencies/queue/queue.go new file mode 100644 index 000000000..bacd27e09 --- /dev/null +++ b/pkg/utils/dependencies/queue/queue.go @@ -0,0 +1,59 @@ +// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Gardener contributors. +// +// SPDX-License-Identifier: Apache-2.0 + +package queue + +import ( + "fmt" +) + +type Queue[T any] struct { + elems []T +} + +// New creates a new Queue. +func New[T any](elems ...T) Queue[T] { + return Queue[T]{elems: elems} +} + +// Append adds new elements to the queue. +func (sq *Queue[T]) Append(elems ...T) { + sq.elems = append(sq.elems, elems...) +} + +// Pop returns the first element and removes it from the queue. +// Returns an error if called on an empty queue. +func (sq *Queue[T]) Pop() (T, error) { + res, err := sq.Peek() + if err != nil { + return res, err + } + sq.elems = sq.elems[1:] + return res, nil +} + +// Peek returns the first element of the queue without removing it. +// Returns an error if called on an empty queue. +func (sq *Queue[T]) Peek() (T, error) { + if len(sq.elems) < 1 { + var zero T + return zero, fmt.Errorf("queue is empty") + } + return sq.elems[0], nil +} + +// Len returns the length of the queue. +func (sq *Queue[_]) Len() int { + return len(sq.elems) +} + +// IsEmpty returns true if the queue is empty. +func (sq *Queue[_]) IsEmpty() bool { + return sq.Len() == 0 +} + +// Copy returns a copy of the queue. +func (sq *Queue[T]) Copy() Queue[T] { + return New(sq.elems...) +} diff --git a/pkg/utils/dependencies/queue/queue_suite_test.go b/pkg/utils/dependencies/queue/queue_suite_test.go new file mode 100644 index 000000000..92fe5993f --- /dev/null +++ b/pkg/utils/dependencies/queue/queue_suite_test.go @@ -0,0 +1,99 @@ +// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Gardener contributors. +// +// SPDX-License-Identifier: Apache-2.0 + +package queue + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestConfig(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Queue Test Suite") +} + +var _ = Describe("Queue Implementation Tests", func() { + + var ( + q Queue[string] + data []string + ) + + BeforeEach(func() { + data = []string{"a", "b", "c"} + q = New(data...) + }) + + Context("NewString", func() { + It("should create a new queue from a given slice", func() { + Expect(q.elems).To(Equal(data)) + }) + }) + + Context("Pop", func() { + It("should return and remove the first element of the queue", func() { + elem, err := q.Pop() + Expect(err).ToNot(HaveOccurred()) + Expect(elem).To(Equal(data[0])) + data = data[1:] + Expect(q.elems).To(Equal(data)) + elem, err = q.Pop() + Expect(err).ToNot(HaveOccurred()) + Expect(elem).To(Equal(data[0])) + data = data[1:] + Expect(q.elems).To(Equal(data)) + elem, err = q.Pop() + Expect(err).ToNot(HaveOccurred()) + Expect(elem).To(Equal(data[0])) + data = data[1:] + Expect(q.elems).To(Equal(data)) + _, err = q.Pop() + Expect(err).To(HaveOccurred()) + Expect(q.Len()).To(Equal(0)) + }) + }) + + Context("Peek", func() { + It("should return the first element of the queue without removing it", func() { + elem, err := q.Peek() + Expect(err).ToNot(HaveOccurred()) + Expect(elem).To(Equal(data[0])) + Expect(q.elems).To(Equal(data)) + }) + }) + + Context("Len", func() { + It("should return the length of the queue", func() { + Expect(q.Len()).To(Equal(len(q.elems))) + }) + }) + + Context("Append", func() { + It("should append new elements", func() { + addData := []string{"d", "e", "f"} + q.Append(addData...) + newData := append(data, addData...) + Expect(q.elems).To(Equal(newData)) + + // ensure that data as well as addData have not been modified + Expect(data).To(HaveLen(3)) + Expect(addData).To(HaveLen(3)) + }) + }) + + Context("Copy", func() { + It("should return an independent copy", func() { + q2 := q.Copy() + Expect(q2.elems).To(Equal(q.elems)) + elem, err := q2.Pop() + Expect(err).ToNot(HaveOccurred()) + Expect(elem).To(Equal(q.elems[0])) + Expect(q2.elems).To(Equal(q.elems[1:])) + }) + }) + +}) diff --git a/pkg/utils/landscaper/blueprint.go b/pkg/utils/landscaper/blueprint.go index 9a7205c14..30cfff6ba 100644 --- a/pkg/utils/landscaper/blueprint.go +++ b/pkg/utils/landscaper/blueprint.go @@ -277,7 +277,7 @@ func (r *BlueprintRenderer) renderSubInstallations(input *ResolvedInstallation, if len(installationTemplates) > 0 { installationTemplates, err = dependencies.CheckForCyclesAndDuplicateExports(installationTemplates, true) if err != nil { - return nil, nil, fmt.Errorf("unable for order subinstallations of blueprint: %w", err) + return nil, nil, fmt.Errorf("unable to order subinstallations of blueprint: %w", err) } } diff --git a/vendor/github.com/gardener/landscaper/apis/core/validation/blueprint.go b/vendor/github.com/gardener/landscaper/apis/core/validation/blueprint.go index c754ed6c7..4485a6fb7 100644 --- a/vendor/github.com/gardener/landscaper/apis/core/validation/blueprint.go +++ b/vendor/github.com/gardener/landscaper/apis/core/validation/blueprint.go @@ -255,9 +255,9 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co allErrs = field.ErrorList{} names = sets.NewString() importedDataObjects = make([]Import, 0) - exportedDataObjects = sets.NewString() + exportedDataObjects = map[string]string{} importedTargets = make([]Import, 0) - exportedTargets = sets.NewString() + exportedTargets = map[string]string{} blueprintDataImports = sets.NewString() blueprintTargetImports = sets.NewString() @@ -308,14 +308,15 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co //} for i, do := range instTmpl.Exports.Data { - dataPath := instPath.Child("exports").Child("data").Index(i).Key(do.Name) - if exportedDataObjects.Has(do.DataRef) { - allErrs = append(allErrs, field.Forbidden(dataPath, "export is already exported by another installation")) + dataPath := instPath.Child("exports").Child("data").Index(i).Key(fmt.Sprintf("%s/%s", do.Name, do.DataRef)) + if dup, ok := exportedDataObjects[do.DataRef]; ok { + allErrs = append(allErrs, field.Forbidden(dataPath, fmt.Sprintf("data export '%s' is already exported by %s", do.DataRef, dup))) + } else { + exportedDataObjects[do.DataRef] = dataPath.String() } if blueprintDataImports.Has(do.DataRef) { allErrs = append(allErrs, field.Forbidden(dataPath, "export is imported by its parent")) } - exportedDataObjects.Insert(do.DataRef) } for i, do := range instTmpl.Imports.Data { importedDataObjects = append(importedDataObjects, Import{ @@ -324,14 +325,15 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co }) } for i, target := range instTmpl.Exports.Targets { - targetPath := instPath.Child("exports").Child("targets").Index(i).Key(target.Name) - if exportedTargets.Has(target.Target) { - allErrs = append(allErrs, field.Forbidden(targetPath, "export is already exported by another installation")) + targetPath := instPath.Child("exports").Child("targets").Index(i).Key(fmt.Sprintf("%s/%s", target.Name, target.Target)) + if dup, ok := exportedTargets[target.Target]; ok { + allErrs = append(allErrs, field.Forbidden(targetPath, fmt.Sprintf("target export '%s' is already exported by %s", target.Target, dup))) + } else { + exportedTargets[target.Target] = targetPath.String() } if blueprintTargetImports.Has(target.Target) { allErrs = append(allErrs, field.Forbidden(targetPath, "export is imported by its parent")) } - exportedTargets.Insert(target.Target) } for i, target := range instTmpl.Imports.Targets { impPath := instPath.Child("imports").Child("targets").Index(i).Key(target.Name) @@ -364,8 +366,8 @@ func ValidateInstallationTemplates(fldPath *field.Path, blueprintImportDefs []co } // validate that all imported values are either satisfied by the blueprint or by another sibling - allErrs = append(allErrs, ValidateSatisfiedImports(blueprintDataImports, exportedDataObjects, importedDataObjects)...) - allErrs = append(allErrs, ValidateSatisfiedImports(blueprintTargetImports, exportedTargets, importedTargets)...) + allErrs = append(allErrs, ValidateSatisfiedImports(blueprintDataImports, sets.StringKeySet(exportedDataObjects), importedDataObjects)...) + allErrs = append(allErrs, ValidateSatisfiedImports(blueprintTargetImports, sets.StringKeySet(exportedTargets), importedTargets)...) return allErrs }