improve error message for duplicate sibling exports (gardener#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
enrico-kaack-comp · Nov 16, 2022 · 4d57055 · 4d57055
1 parent 49fa8d3
commit 4d57055
Show file tree

Hide file tree

Showing 10 changed files with 553 additions and 100 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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...))
 		})
 
 	})