From c8a66fc82128621c57a0a32309b72c55c7a8eabd Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Tue, 10 Oct 2023 11:32:06 +0200 Subject: [PATCH 1/9] Move Kubernetes assests into subdirectories Move Kubernetes assetes into the respective subdirectories. --- .../configmaps}/expected-dyff-spruce.human | 0 .../configmaps}/from.yml | 0 .../configmaps}/to.yml | 0 .../pods => kubernetes/lists}/from.yml | 0 .../pods => kubernetes/lists}/to.yml | 0 assets/kubernetes/multi-docs-versioned-crds/from.yml | 11 +++++++++++ assets/kubernetes/multi-docs-versioned-crds/to.yml | 11 +++++++++++ .../multi-docs}/README.md | 0 .../multi-docs}/from.yml | 0 .../{kubernetes-yaml => kubernetes/multi-docs}/to.yml | 0 pkg/dyff/compare_test.go | 6 +++--- pkg/dyff/output_human_test.go | 6 +++--- 12 files changed, 28 insertions(+), 6 deletions(-) rename assets/{kubernetes-configmaps => kubernetes/configmaps}/expected-dyff-spruce.human (100%) rename assets/{kubernetes-configmaps => kubernetes/configmaps}/from.yml (100%) rename assets/{kubernetes-configmaps => kubernetes/configmaps}/to.yml (100%) rename assets/{kubernetes-lists/pods => kubernetes/lists}/from.yml (100%) rename assets/{kubernetes-lists/pods => kubernetes/lists}/to.yml (100%) create mode 100644 assets/kubernetes/multi-docs-versioned-crds/from.yml create mode 100644 assets/kubernetes/multi-docs-versioned-crds/to.yml rename assets/{kubernetes-yaml => kubernetes/multi-docs}/README.md (100%) rename assets/{kubernetes-yaml => kubernetes/multi-docs}/from.yml (100%) rename assets/{kubernetes-yaml => kubernetes/multi-docs}/to.yml (100%) diff --git a/assets/kubernetes-configmaps/expected-dyff-spruce.human b/assets/kubernetes/configmaps/expected-dyff-spruce.human similarity index 100% rename from assets/kubernetes-configmaps/expected-dyff-spruce.human rename to assets/kubernetes/configmaps/expected-dyff-spruce.human diff --git a/assets/kubernetes-configmaps/from.yml b/assets/kubernetes/configmaps/from.yml similarity index 100% rename from assets/kubernetes-configmaps/from.yml rename to assets/kubernetes/configmaps/from.yml diff --git a/assets/kubernetes-configmaps/to.yml b/assets/kubernetes/configmaps/to.yml similarity index 100% rename from assets/kubernetes-configmaps/to.yml rename to assets/kubernetes/configmaps/to.yml diff --git a/assets/kubernetes-lists/pods/from.yml b/assets/kubernetes/lists/from.yml similarity index 100% rename from assets/kubernetes-lists/pods/from.yml rename to assets/kubernetes/lists/from.yml diff --git a/assets/kubernetes-lists/pods/to.yml b/assets/kubernetes/lists/to.yml similarity index 100% rename from assets/kubernetes-lists/pods/to.yml rename to assets/kubernetes/lists/to.yml diff --git a/assets/kubernetes/multi-docs-versioned-crds/from.yml b/assets/kubernetes/multi-docs-versioned-crds/from.yml new file mode 100644 index 0000000..b024ee1 --- /dev/null +++ b/assets/kubernetes/multi-docs-versioned-crds/from.yml @@ -0,0 +1,11 @@ +--- +apiVersion: v1beta1 +kind: some-kind +metadata: + name: some-item + +--- +apiVersion: v1alpha1 +kind: some-kind +metadata: + name: some-item diff --git a/assets/kubernetes/multi-docs-versioned-crds/to.yml b/assets/kubernetes/multi-docs-versioned-crds/to.yml new file mode 100644 index 0000000..ecaf5c5 --- /dev/null +++ b/assets/kubernetes/multi-docs-versioned-crds/to.yml @@ -0,0 +1,11 @@ +--- +apiVersion: v1alpha1 +kind: some-kind +metadata: + name: some-item + +--- +apiVersion: v1beta1 +kind: some-kind +metadata: + name: some-item diff --git a/assets/kubernetes-yaml/README.md b/assets/kubernetes/multi-docs/README.md similarity index 100% rename from assets/kubernetes-yaml/README.md rename to assets/kubernetes/multi-docs/README.md diff --git a/assets/kubernetes-yaml/from.yml b/assets/kubernetes/multi-docs/from.yml similarity index 100% rename from assets/kubernetes-yaml/from.yml rename to assets/kubernetes/multi-docs/from.yml diff --git a/assets/kubernetes-yaml/to.yml b/assets/kubernetes/multi-docs/to.yml similarity index 100% rename from assets/kubernetes-yaml/to.yml rename to assets/kubernetes/multi-docs/to.yml diff --git a/pkg/dyff/compare_test.go b/pkg/dyff/compare_test.go index fb62689..a1c0d4c 100644 --- a/pkg/dyff/compare_test.go +++ b/pkg/dyff/compare_test.go @@ -636,7 +636,6 @@ listY: [ Yo, Yo, Yo ] }) It("should return all differences between the files with multiple documents", func() { - results, err := dyff.CompareInputFiles(file("../../assets/kubernetes-yaml/from.yml"), file("../../assets/kubernetes-yaml/to.yml")) expected := []dyff.Diff{ singleDiff("#0/spec/template/spec/containers/name=registry/resources/limits/cpu", dyff.MODIFICATION, "100m", "1000m"), singleDiff("#0/spec/template/spec/containers/name=registry/resources/limits/memory", dyff.MODIFICATION, "100Mi", "10Gi"), @@ -644,6 +643,7 @@ listY: [ Yo, Yo, Yo ] singleDiff("#1/spec/ports", dyff.ADDITION, nil, list(`[ {name: backdoor, port: 5001, protocol: TCP} ]`)), } + results, err := dyff.CompareInputFiles(file(assets("kubernetes/multi-docs/from.yml")), file(assets("kubernetes/multi-docs/to.yml"))) Expect(err).To(BeNil()) Expect(results).NotTo(BeNil()) Expect(results.Diffs).NotTo(BeNil()) @@ -821,8 +821,8 @@ b: bar Context("two YAML structures with Kubernetes lists", func() { It("should identify individual list entries based on the nested name field in the respective entry metadata", func() { from, to := loadFiles( - assets("kubernetes-lists", "pods", "from.yml"), - assets("kubernetes-lists", "pods", "to.yml"), + assets("kubernetes", "lists", "from.yml"), + assets("kubernetes", "lists", "to.yml"), ) results, err := dyff.CompareInputFiles(from, to, dyff.KubernetesEntityDetection(true)) diff --git a/pkg/dyff/output_human_test.go b/pkg/dyff/output_human_test.go index b65eb70..5ec04a2 100644 --- a/pkg/dyff/output_human_test.go +++ b/pkg/dyff/output_human_test.go @@ -193,9 +193,9 @@ variables.ROUTER_TLS_PEM.options It("should use human friendly compact diff of multiline text differences", func() { compareAgainstExpected( - assets("kubernetes-configmaps/from.yml"), - assets("kubernetes-configmaps/to.yml"), - assets("kubernetes-configmaps/expected-dyff-spruce.human"), + assets("kubernetes/configmaps/from.yml"), + assets("kubernetes/configmaps/to.yml"), + assets("kubernetes/configmaps/expected-dyff-spruce.human"), false, ) }) From 4e3d7f13b90c3569927689a82c33700fe79eec56 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Tue, 10 Oct 2023 11:33:20 +0200 Subject: [PATCH 2/9] Add `apiVersion` to Kubernetes look-up To identify documents in a YAML file, use the `apiVersion`, `kind`, `namespace` and `name` of a Kubernetes resource. --- internal/cmd/cmds_test.go | 2 +- pkg/dyff/core.go | 21 +++++++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/internal/cmd/cmds_test.go b/internal/cmd/cmds_test.go index 289148c..1ebd070 100644 --- a/internal/cmd/cmds_test.go +++ b/internal/cmd/cmds_test.go @@ -534,7 +534,7 @@ yaml.map.whitespaces It("should show a report when filtering and a document has been removed between inputs", func() { expected := ` -spec.replicas (Deployment/default/test) +spec.replicas (apps/v1/Deployment/test) ± value change - 2 + 3 diff --git a/pkg/dyff/core.go b/pkg/dyff/core.go index 823ece9..6fa64a6 100644 --- a/pkg/dyff/core.go +++ b/pkg/dyff/core.go @@ -881,28 +881,41 @@ func getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (ListItemI return "", fmt.Errorf("not all entities appear to have %q fields", key) } -// fqrn returns something like a fully qualified Kubernetes resource name, which contains its kind, namespace and name +// fqrn returns something like a fully qualified Kubernetes resource name, which +// contains its apiVersion, kind, namespace, and name where the namespace is +// only included if it is defined func fqrn(node *yamlv3.Node) (string, error) { if node.Kind != yamlv3.MappingNode { return "", fmt.Errorf("name look-up for Kubernetes resources does only work with mapping nodes") } + var elem []string + + apiVersion, err := nameFromPath(node, "apiVersion") + if err != nil { + return "", err + } + elem = append(elem, apiVersion) + kind, err := nameFromPath(node, "kind") if err != nil { return "", err } + elem = append(elem, kind) + // namespace is optional and will be omited if not set namespace, err := nameFromPath(node, "metadata.namespace") - if err != nil { - namespace = "default" + if err == nil { + elem = append(elem, namespace) } name, err := nameFromPath(node, "metadata.name") if err != nil { return "", err } + elem = append(elem, name) - return fmt.Sprintf("%s/%s/%s", kind, namespace, name), nil + return strings.Join(elem, "/"), nil } // isEmptyDocument returns true in case the given YAML node is an empty document From a6f5c9603d73a7362865259895941eeb1f34c429 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Tue, 10 Oct 2023 21:10:38 +0200 Subject: [PATCH 3/9] Use `assets` function in test cases Use `assets` function in test cases. --- pkg/dyff/compare_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/dyff/compare_test.go b/pkg/dyff/compare_test.go index a1c0d4c..d161aa9 100644 --- a/pkg/dyff/compare_test.go +++ b/pkg/dyff/compare_test.go @@ -524,8 +524,8 @@ resource_pools: Context("Given two files", func() { It("should return differences in raw texts", func() { - from := file("../../assets/raw-text/from.txt") - to := file("../../assets/raw-text/to.txt") + from := file(assets("raw-text/from.txt")) + to := file(assets("raw-text/to.txt")) Expect(from.Documents).To(HaveLen(1)) Expect(to.Documents).To(HaveLen(1)) @@ -664,8 +664,8 @@ listY: [ Yo, Yo, Yo ] It("should return differences in named lists even if no standard identifier is used", func() { results, err := dyff.CompareInputFiles( - file("../../assets/prometheus/from.yml"), - file("../../assets/prometheus/to.yml"), + file(assets("prometheus/from.yml")), + file(assets("prometheus/to.yml")), ) Expect(err).To(BeNil()) @@ -713,8 +713,8 @@ listY: [ Yo, Yo, Yo ] It("should fail to find the non-standard identifier if the threshold is too high", func() { report, err := dyff.CompareInputFiles( - file("../../assets/prometheus/from.yml"), - file("../../assets/prometheus/to.yml"), + file(assets("prometheus/from.yml")), + file(assets("prometheus/to.yml")), dyff.NonStandardIdentifierGuessCountThreshold(8), ) From 83043b7ae3a4896232e4f7c751bbe23d72b31163 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 11 Oct 2023 14:12:47 +0200 Subject: [PATCH 4/9] Refactor core code Unify code by using the `compare` struct rather than relying on funtion arguments to make sure that similar functions do look more alike. --- pkg/dyff/core.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/dyff/core.go b/pkg/dyff/core.go index 6fa64a6..a4114e9 100644 --- a/pkg/dyff/core.go +++ b/pkg/dyff/core.go @@ -437,25 +437,22 @@ func (compare *compare) sequenceNodes(path ytbx.Path, from *yamlv3.Node, to *yam return []Diff{}, nil } + // check if a known identifier (e.g. name, or id) can be used if identifier, err := compare.getIdentifierFromNamedLists(from, to); err == nil { return compare.namedEntryLists(path, identifier, from, to) } - if identifier := getNonStandardIdentifierFromNamedLists(from, to, compare.settings.NonStandardIdentifierGuessCountThreshold); identifier != "" { - d, err := compare.namedEntryLists(path, identifier, from, to) - if err != nil { - return nil, fmt.Errorf("sequenceNodes(nonstd): %w", err) - } - - return d, nil + // check if there is a field in all entries that could serve as an identifier + if identifier := compare.getNonStandardIdentifierFromNamedLists(from, to); identifier != "" { + return compare.namedEntryLists(path, identifier, from, to) } - if compare.settings.KubernetesEntityDetection { - if identifier, err := getIdentifierFromKubernetesEntityList(from, to); err == nil { - return compare.namedEntryLists(path, identifier, from, to) - } + // check if Kubernetes resource fields can be used to identify items + if identifier, err := compare.getIdentifierFromKubernetesEntityList(from, to); err == nil { + return compare.namedEntryLists(path, identifier, from, to) } + // in any other case, compare lists as simple lists by relying on hashes return compare.simpleLists(path, from, to) } @@ -856,7 +853,11 @@ func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) ( } // getIdentifierFromKubernetesEntityList returns 'metadata.name' as a field identifier if the provided objects all have the key. -func getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (ListItemIdentifierField, error) { +func (compare *compare) getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (ListItemIdentifierField, error) { + if !compare.settings.KubernetesEntityDetection { + return "", fmt.Errorf("entity detection for Kubernetes resource is not enabled") + } + key := ListItemIdentifierField("metadata.name") allHaveMetadataName := func(sequenceNode *yamlv3.Node) bool { numWithMetadata := 0 @@ -934,7 +935,7 @@ func isEmptyDocument(node *yamlv3.Node) bool { return false } -func getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node, nonStandardIdentifierGuessCountThreshold int) ListItemIdentifierField { +func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node) ListItemIdentifierField { createKeyCountMap := func(list *yamlv3.Node) map[string]int { tmp := map[string]map[string]struct{}{} for _, entry := range list.Content { @@ -970,7 +971,7 @@ func getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node, nonStanda for keyA, countA := range counterA { if countB, ok := counterB[keyA]; ok { - if countA == listALength && countB == listBLength && countA > nonStandardIdentifierGuessCountThreshold { + if countA == listALength && countB == listBLength && countA > compare.settings.NonStandardIdentifierGuessCountThreshold { return ListItemIdentifierField(keyA) } } From 4be6bb655436bfae5a4482f32ae8f3292cee854f Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 11 Oct 2023 14:33:18 +0200 Subject: [PATCH 5/9] Refactor core code by sorting functions Move functions that have similar tasks to be together. --- pkg/dyff/core.go | 116 +++++++++++++++++++++++------------------------ 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/pkg/dyff/core.go b/pkg/dyff/core.go index a4114e9..ccd418b 100644 --- a/pkg/dyff/core.go +++ b/pkg/dyff/core.go @@ -547,6 +547,19 @@ func nameFromPath(node *yamlv3.Node, field ListItemIdentifierField) (string, err return nameFromPath(val, ListItemIdentifierField(parts[1])) } +// getEntryFromNamedList returns the entry that is identified by the identifier +// key and a name, for example: `name: one` where name is the identifier key and +// one the name. Function will return nil with bool false if there is no entry. +func getEntryFromNamedList(sequenceNode *yamlv3.Node, identifier ListItemIdentifierField, name string) (*yamlv3.Node, bool) { + for _, mappingNode := range sequenceNode.Content { + nodeName, _ := nameFromPath(mappingNode, identifier) + if nodeName == name { + return mappingNode, true + } + } + return nil, false +} + func (compare *compare) namedEntryLists(path ytbx.Path, identifier ListItemIdentifierField, from *yamlv3.Node, to *yamlv3.Node) ([]Diff, error) { removals := make([]*yamlv3.Node, 0) additions := make([]*yamlv3.Node, 0) @@ -775,19 +788,6 @@ func getValueByKey(mappingNode *yamlv3.Node, key string) (*yamlv3.Node, error) { return nil, fmt.Errorf("no key '%s' found in map and also failed to get a list of key for this map", key) } -// getEntryFromNamedList returns the entry that is identified by the identifier -// key and a name, for example: `name: one` where name is the identifier key and -// one the name. Function will return nil with bool false if there is no entry. -func getEntryFromNamedList(sequenceNode *yamlv3.Node, identifier ListItemIdentifierField, name string) (*yamlv3.Node, bool) { - for _, mappingNode := range sequenceNode.Content { - nodeName, _ := nameFromPath(mappingNode, identifier) - if nodeName == name { - return mappingNode, true - } - } - return nil, false -} - func (compare *compare) listItemIdentifierCandidates() []ListItemIdentifierField { // Set default candidates that are most widly used var candidates = []ListItemIdentifierField{"name", "key", "id"} @@ -852,6 +852,51 @@ func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) ( return "", fmt.Errorf("unable to find a key that can serve as an unique identifier") } +func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node) ListItemIdentifierField { + createKeyCountMap := func(list *yamlv3.Node) map[string]int { + tmp := map[string]map[string]struct{}{} + for _, entry := range list.Content { + if entry.Kind != yamlv3.MappingNode { + return map[string]int{} + } + + for i := 0; i < len(entry.Content); i += 2 { + k, v := followAlias(entry.Content[i]), followAlias(entry.Content[i+1]) + if k.Kind == yamlv3.ScalarNode && k.Tag == "!!str" && + v.Kind == yamlv3.ScalarNode && v.Tag == "!!str" { + if _, ok := tmp[k.Value]; !ok { + tmp[k.Value] = map[string]struct{}{} + } + + tmp[k.Value][v.Value] = struct{}{} + } + } + } + + result := map[string]int{} + for key, value := range tmp { + result[key] = len(value) + } + + return result + } + + listALength := len(listA.Content) + listBLength := len(listB.Content) + counterA := createKeyCountMap(listA) + counterB := createKeyCountMap(listB) + + for keyA, countA := range counterA { + if countB, ok := counterB[keyA]; ok { + if countA == listALength && countB == listBLength && countA > compare.settings.NonStandardIdentifierGuessCountThreshold { + return ListItemIdentifierField(keyA) + } + } + } + + return "" +} + // getIdentifierFromKubernetesEntityList returns 'metadata.name' as a field identifier if the provided objects all have the key. func (compare *compare) getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (ListItemIdentifierField, error) { if !compare.settings.KubernetesEntityDetection { @@ -935,51 +980,6 @@ func isEmptyDocument(node *yamlv3.Node) bool { return false } -func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node) ListItemIdentifierField { - createKeyCountMap := func(list *yamlv3.Node) map[string]int { - tmp := map[string]map[string]struct{}{} - for _, entry := range list.Content { - if entry.Kind != yamlv3.MappingNode { - return map[string]int{} - } - - for i := 0; i < len(entry.Content); i += 2 { - k, v := followAlias(entry.Content[i]), followAlias(entry.Content[i+1]) - if k.Kind == yamlv3.ScalarNode && k.Tag == "!!str" && - v.Kind == yamlv3.ScalarNode && v.Tag == "!!str" { - if _, ok := tmp[k.Value]; !ok { - tmp[k.Value] = map[string]struct{}{} - } - - tmp[k.Value][v.Value] = struct{}{} - } - } - } - - result := map[string]int{} - for key, value := range tmp { - result[key] = len(value) - } - - return result - } - - listALength := len(listA.Content) - listBLength := len(listB.Content) - counterA := createKeyCountMap(listA) - counterB := createKeyCountMap(listB) - - for keyA, countA := range counterA { - if countB, ok := counterB[keyA]; ok { - if countA == listALength && countB == listBLength && countA > compare.settings.NonStandardIdentifierGuessCountThreshold { - return ListItemIdentifierField(keyA) - } - } - } - - return "" -} - func (compare *compare) createLookUpMap(sequenceNode *yamlv3.Node) map[uint64][]int { result := make(map[uint64][]int, len(sequenceNode.Content)) for idx, entry := range sequenceNode.Content { From a4859f7988d4c753db723de8bd90ed3a7e2203a7 Mon Sep 17 00:00:00 2001 From: Smeb Date: Sat, 14 Oct 2023 20:24:15 +0200 Subject: [PATCH 6/9] Provide Kubernetes lists example Provide AWS charts based example. --- .../kubernetes/lists-versioned-crds/from.yml | 18 ++++++++++++++++++ assets/kubernetes/lists-versioned-crds/to.yml | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 assets/kubernetes/lists-versioned-crds/from.yml create mode 100644 assets/kubernetes/lists-versioned-crds/to.yml diff --git a/assets/kubernetes/lists-versioned-crds/from.yml b/assets/kubernetes/lists-versioned-crds/from.yml new file mode 100644 index 0000000..07f16ea --- /dev/null +++ b/assets/kubernetes/lists-versioned-crds/from.yml @@ -0,0 +1,18 @@ +--- +apiVersion: v1 +kind: List +metadata: + name: ingress-class +items: + - apiVersion: v2 + kind: kindv2 + metadata: + name: item + labels: + id: 2 + - apiVersion: v1 + kind: kindv1 + metadata: + name: item + labels: + id: 1 \ No newline at end of file diff --git a/assets/kubernetes/lists-versioned-crds/to.yml b/assets/kubernetes/lists-versioned-crds/to.yml new file mode 100644 index 0000000..bd4e23a --- /dev/null +++ b/assets/kubernetes/lists-versioned-crds/to.yml @@ -0,0 +1,18 @@ +--- +apiVersion: v1 +kind: List +metadata: + name: ingress-class +items: + - apiVersion: v1 + kind: kindv1 + metadata: + name: item + labels: + id: 1 + - apiVersion: v2 + kind: kindv2 + metadata: + name: item + labels: + id: 2 \ No newline at end of file From 9a9c164942940c84b138f1fcc0a72612e278367d Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Sat, 14 Oct 2023 20:19:44 +0200 Subject: [PATCH 7/9] Introduce identifier interface Introduce interface to describe an identifier in order to implement multiple independent list item identifiers. Add single field identifier for use cases like `name`, or `id`. Add Kubernetes identifier that now includes the `apiVersion` and `kind` for its look-ups. --- pkg/dyff/compare_test.go | 24 ++++- pkg/dyff/core.go | 171 ++++++++++-------------------------- pkg/dyff/core_identifier.go | 141 +++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 128 deletions(-) create mode 100644 pkg/dyff/core_identifier.go diff --git a/pkg/dyff/compare_test.go b/pkg/dyff/compare_test.go index d161aa9..771d822 100644 --- a/pkg/dyff/compare_test.go +++ b/pkg/dyff/compare_test.go @@ -832,16 +832,34 @@ b: bar Expect(results.Diffs[0]).To(BeSameDiffAs(singleDiff( "#0/items", dyff.ORDERCHANGE, - dyff.AsSequenceNode("foo-2", "foo-1"), - dyff.AsSequenceNode("foo-1", "foo-2"), + dyff.AsSequenceNode("v1/Pod/foobar/foo-2", "v1/Pod/foobar/foo-1"), + dyff.AsSequenceNode("v1/Pod/foobar/foo-1", "v1/Pod/foobar/foo-2"), ))) Expect(results.Diffs[1]).To(BeSameDiffAs(singleDiff( - "/items/metadata.name=foo-1/metadata/labels/foo", + "/items/resource=v1\\/Pod\\/foobar\\/foo-1/metadata/labels/foo", dyff.MODIFICATION, "bAr", "bar", ))) }) + + It("should detect Kubernetes items by their apiVersion, kind, and name", func() { + from, to := loadFiles( + assets("kubernetes", "lists-versioned-crds", "from.yml"), + assets("kubernetes", "lists-versioned-crds", "to.yml"), + ) + + results, err := dyff.CompareInputFiles(from, to, dyff.KubernetesEntityDetection(true)) + Expect(err).ToNot(HaveOccurred()) + + Expect(results.Diffs).To(HaveLen(1)) + Expect(results.Diffs[0]).To(BeSameDiffAs(singleDiff( + "#0/items", + dyff.ORDERCHANGE, + dyff.AsSequenceNode("v2/kindv2/item", "v1/kindv1/item"), + dyff.AsSequenceNode("v1/kindv1/item", "v2/kindv2/item"), + ))) + }) }) Context("checking known issues of compare", func() { diff --git a/pkg/dyff/core.go b/pkg/dyff/core.go index ccd418b..9e440b7 100644 --- a/pkg/dyff/core.go +++ b/pkg/dyff/core.go @@ -40,23 +40,18 @@ type compareSettings struct { NonStandardIdentifierGuessCountThreshold int IgnoreOrderChanges bool KubernetesEntityDetection bool - AdditionalIdentifiers []ListItemIdentifierField + AdditionalIdentifiers []string } type compare struct { settings compareSettings } -// ListItemIdentifierField names the field that identifies a list. -type ListItemIdentifierField string - // AdditionalIdentifiers specifies additional identifiers that will be -// used as the key for matcing maps from source to target. -func AdditionalIdentifiers(ids ...string) CompareOption { +// used as the key for matching maps from source to target. +func AdditionalIdentifiers(fieldNames ...string) CompareOption { return func(settings *compareSettings) { - for _, i := range ids { - settings.AdditionalIdentifiers = append(settings.AdditionalIdentifiers, ListItemIdentifierField(i)) - } + settings.AdditionalIdentifiers = append(settings.AdditionalIdentifiers, fieldNames...) } } @@ -111,7 +106,7 @@ func CompareInputFiles(from ytbx.InputFile, to ytbx.InputFile, compareOptions .. for i := range from.Documents { if entry := from.Documents[i]; !isEmptyDocument(entry) { fromDocs = append(fromDocs, entry) - if name, err := fqrn(entry.Content[0]); err == nil { + if name, err := k8sItem.Name(entry.Content[0]); err == nil { fromNames = append(fromNames, name) } } @@ -120,7 +115,7 @@ func CompareInputFiles(from ytbx.InputFile, to ytbx.InputFile, compareOptions .. for i := range to.Documents { if entry := to.Documents[i]; !isEmptyDocument(entry) { toDocs = append(toDocs, entry) - if name, err := fqrn(entry.Content[0]); err == nil { + if name, err := k8sItem.Name(entry.Content[0]); err == nil { toNames = append(toNames, name) } } @@ -256,7 +251,7 @@ func (compare *compare) documentNodes(from, to ytbx.InputFile) ([]Diff, error) { for i, document := range inputFile.Documents { node := document.Content[0] - name, err := fqrn(node) + name, err := k8sItem.Name(node) if err != nil { return nil, nil, err } @@ -443,7 +438,7 @@ func (compare *compare) sequenceNodes(path ytbx.Path, from *yamlv3.Node, to *yam } // check if there is a field in all entries that could serve as an identifier - if identifier := compare.getNonStandardIdentifierFromNamedLists(from, to); identifier != "" { + if identifier := compare.getNonStandardIdentifierFromNamedLists(from, to); identifier != nil { return compare.namedEntryLists(path, identifier, from, to) } @@ -534,33 +529,7 @@ func (compare *compare) simpleLists(path ytbx.Path, from *yamlv3.Node, to *yamlv return packChangesAndAddToResult([]Diff{}, path, orderChanges, additions, removals) } -func nameFromPath(node *yamlv3.Node, field ListItemIdentifierField) (string, error) { - parts := strings.SplitN(string(field), ".", 2) - key := parts[0] - val, err := getValueByKey(node, key) - if err != nil { - return "", fmt.Errorf("nameFromPath issue: %w", err) - } - if len(parts) == 1 { - return val.Value, nil - } - return nameFromPath(val, ListItemIdentifierField(parts[1])) -} - -// getEntryFromNamedList returns the entry that is identified by the identifier -// key and a name, for example: `name: one` where name is the identifier key and -// one the name. Function will return nil with bool false if there is no entry. -func getEntryFromNamedList(sequenceNode *yamlv3.Node, identifier ListItemIdentifierField, name string) (*yamlv3.Node, bool) { - for _, mappingNode := range sequenceNode.Content { - nodeName, _ := nameFromPath(mappingNode, identifier) - if nodeName == name { - return mappingNode, true - } - } - return nil, false -} - -func (compare *compare) namedEntryLists(path ytbx.Path, identifier ListItemIdentifierField, from *yamlv3.Node, to *yamlv3.Node) ([]Diff, error) { +func (compare *compare) namedEntryLists(path ytbx.Path, identifier listItemIdentifier, from *yamlv3.Node, to *yamlv3.Node) ([]Diff, error) { removals := make([]*yamlv3.Node, 0) additions := make([]*yamlv3.Node, 0) @@ -574,12 +543,12 @@ func (compare *compare) namedEntryLists(path ytbx.Path, identifier ListItemIdent // Find entries that are common to both lists to compare them separately, and // find entries that are only in from, but not to and are therefore removed for _, fromEntry := range from.Content { - name, err := nameFromPath(fromEntry, identifier) + name, err := identifier.Name(fromEntry) if err != nil { - return nil, fmt.Errorf("nameEntryList from issue: %w", err) + return nil, fmt.Errorf("failed to identify name: %w", err) } - if toEntry, ok := getEntryFromNamedList(to, identifier, name); ok { + if toEntry, err := identifier.FindNodeByName(to, name); err == nil { // `from` and `to` have the same entry identified by identifier and name -> require comparison diffs, err := compare.objects( ytbx.NewPathWithNamedListElement(path, identifier, name), @@ -600,12 +569,12 @@ func (compare *compare) namedEntryLists(path ytbx.Path, identifier ListItemIdent // Find entries that are only in to, but not from and are therefore added for _, toEntry := range to.Content { - name, err := nameFromPath(toEntry, identifier) + name, err := identifier.Name(toEntry) if err != nil { - return nil, fmt.Errorf("nameEntryList to issue: %w", err) + return nil, fmt.Errorf("failed to identify name: %w", err) } - if _, ok := getEntryFromNamedList(from, identifier, name); ok { + if _, err := identifier.FindNodeByName(from, name); err == nil { // `to` and `from` have the same entry identified by identifier and name (comparison already covered by previous range) toNames = append(toNames, name) @@ -770,27 +739,9 @@ func findValueByKey(mappingNode *yamlv3.Node, key string) (*yamlv3.Node, bool) { return nil, false } -// getValueByKey returns the value for a given key in a provided mapping node, -// or nil with an error if there is no such entry. This is comparable to getting -// a value from a map with `foobar[key]`. -func getValueByKey(mappingNode *yamlv3.Node, key string) (*yamlv3.Node, error) { - for i := 0; i < len(mappingNode.Content); i += 2 { - k, v := followAlias(mappingNode.Content[i]), followAlias(mappingNode.Content[i+1]) - if k.Value == key { - return v, nil - } - } - - if names, err := ytbx.ListStringKeys(mappingNode); err == nil { - return nil, fmt.Errorf("no key '%s' found in map, available keys are: %s", key, strings.Join(names, ", ")) - } - - return nil, fmt.Errorf("no key '%s' found in map and also failed to get a list of key for this map", key) -} - -func (compare *compare) listItemIdentifierCandidates() []ListItemIdentifierField { +func (compare *compare) listItemIdentifierCandidates() []string { // Set default candidates that are most widly used - var candidates = []ListItemIdentifierField{"name", "key", "id"} + var candidates = []string{"name", "key", "id"} // Add user supplied additional candidates (taking precedence over defaults) candidates = append(compare.settings.AdditionalIdentifiers, candidates...) @@ -803,7 +754,7 @@ func (compare *compare) listItemIdentifierCandidates() []ListItemIdentifierField return candidates } -func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) (ListItemIdentifierField, error) { +func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) (listItemIdentifier, error) { isCandidate := func(node *yamlv3.Node) bool { if node.Kind == yamlv3.ScalarNode { for _, entry := range compare.listItemIdentifierCandidates() { @@ -816,19 +767,19 @@ func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) ( return false } - createKeyCountMap := func(sequenceNode *yamlv3.Node) map[ListItemIdentifierField]map[string]struct{} { - result := map[ListItemIdentifierField]map[string]struct{}{} + createKeyCountMap := func(sequenceNode *yamlv3.Node) map[string]map[string]struct{} { + result := map[string]map[string]struct{}{} for _, entry := range sequenceNode.Content { switch entry.Kind { case yamlv3.MappingNode: for i := 0; i < len(entry.Content); i += 2 { k, v := followAlias(entry.Content[i]), followAlias(entry.Content[i+1]) if isCandidate(k) { - if _, found := result[ListItemIdentifierField(k.Value)]; !found { - result[ListItemIdentifierField(k.Value)] = map[string]struct{}{} + if _, found := result[k.Value]; !found { + result[k.Value] = map[string]struct{}{} } - result[ListItemIdentifierField(k.Value)][v.Value] = struct{}{} + result[k.Value][v.Value] = struct{}{} } } } @@ -844,15 +795,15 @@ func (compare *compare) getIdentifierFromNamedLists(listA, listB *yamlv3.Node) ( for _, identifier := range compare.listItemIdentifierCandidates() { if countA, okA := counterA[identifier]; okA && len(countA) == len(listA.Content) { if countB, okB := counterB[identifier]; okB && len(countB) == len(listB.Content) { - return identifier, nil + return &singleField{identifier}, nil } } } - return "", fmt.Errorf("unable to find a key that can serve as an unique identifier") + return nil, fmt.Errorf("unable to find a key that can serve as an unique identifier") } -func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node) ListItemIdentifierField { +func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yamlv3.Node) listItemIdentifier { createKeyCountMap := func(list *yamlv3.Node) map[string]int { tmp := map[string]map[string]struct{}{} for _, entry := range list.Content { @@ -889,79 +840,39 @@ func (compare *compare) getNonStandardIdentifierFromNamedLists(listA, listB *yam for keyA, countA := range counterA { if countB, ok := counterB[keyA]; ok { if countA == listALength && countB == listBLength && countA > compare.settings.NonStandardIdentifierGuessCountThreshold { - return ListItemIdentifierField(keyA) + return &singleField{keyA} } } } - return "" + return nil } // getIdentifierFromKubernetesEntityList returns 'metadata.name' as a field identifier if the provided objects all have the key. -func (compare *compare) getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (ListItemIdentifierField, error) { +func (compare *compare) getIdentifierFromKubernetesEntityList(listA, listB *yamlv3.Node) (listItemIdentifier, error) { if !compare.settings.KubernetesEntityDetection { - return "", fmt.Errorf("entity detection for Kubernetes resource is not enabled") + return nil, fmt.Errorf("entity detection for Kubernetes resource is not enabled") } - key := ListItemIdentifierField("metadata.name") allHaveMetadataName := func(sequenceNode *yamlv3.Node) bool { - numWithMetadata := 0 + var numWithMetadata int for _, entry := range sequenceNode.Content { switch entry.Kind { case yamlv3.MappingNode: - _, err := nameFromPath(entry, key) - if err == nil { + if _, err := k8sItem.Name(entry); err == nil { numWithMetadata++ } } } - return numWithMetadata == len(sequenceNode.Content) - } - listAHasKey := allHaveMetadataName(listA) - listBHasKey := allHaveMetadataName(listB) - if listAHasKey && listBHasKey { - return key, nil - } - - return "", fmt.Errorf("not all entities appear to have %q fields", key) -} - -// fqrn returns something like a fully qualified Kubernetes resource name, which -// contains its apiVersion, kind, namespace, and name where the namespace is -// only included if it is defined -func fqrn(node *yamlv3.Node) (string, error) { - if node.Kind != yamlv3.MappingNode { - return "", fmt.Errorf("name look-up for Kubernetes resources does only work with mapping nodes") - } - - var elem []string - - apiVersion, err := nameFromPath(node, "apiVersion") - if err != nil { - return "", err - } - elem = append(elem, apiVersion) - - kind, err := nameFromPath(node, "kind") - if err != nil { - return "", err - } - elem = append(elem, kind) - - // namespace is optional and will be omited if not set - namespace, err := nameFromPath(node, "metadata.namespace") - if err == nil { - elem = append(elem, namespace) + return numWithMetadata == len(sequenceNode.Content) } - name, err := nameFromPath(node, "metadata.name") - if err != nil { - return "", err + if allHaveMetadataName(listA) && allHaveMetadataName(listB) { + return k8sItem, nil } - elem = append(elem, name) - return strings.Join(elem, "/"), nil + return nil, fmt.Errorf("unable to verify list entries to be Kubernetes resources using Kubernetes fields") } // isEmptyDocument returns true in case the given YAML node is an empty document @@ -1170,3 +1081,13 @@ func pathToString(path *ytbx.Path, useGoPatchPaths bool, showPathRoot bool) stri return result } + +func grab(node *yamlv3.Node, pathString string) (*yamlv3.Node, error) { + return ytbx.Grab( + &yamlv3.Node{ + Kind: yamlv3.DocumentNode, + Content: []*yamlv3.Node{node}, + }, + pathString, + ) +} diff --git a/pkg/dyff/core_identifier.go b/pkg/dyff/core_identifier.go new file mode 100644 index 0000000..3623d5f --- /dev/null +++ b/pkg/dyff/core_identifier.go @@ -0,0 +1,141 @@ +// Copyright © 2023 The Homeport Team +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package dyff + +import ( + "fmt" + "strings" + + yamlv3 "gopkg.in/yaml.v3" +) + +// listItemIdentifier defines the contract for an list item identifier +type listItemIdentifier interface { + // Name returns an unique name from the given entry, or an error in case this + // is not possible due to missing fields + Name(mappingNode *yamlv3.Node) (string, error) + + // FindNodeByName returns the node that matches the given name, or an error in + // case it cannot find such an entry or required fields are missing + FindNodeByName(sequenceNode *yamlv3.Node, name string) (*yamlv3.Node, error) + + // String returns a reprensation or explanation of the identifier itself + String() string +} + +// --- --- --- + +// singleField is an list item identifier that relies on one field to serve as +// the defining field to differentiate between list items, e.g. 'name', or 'id' +type singleField struct { + IdentifierFieldName string +} + +var _ listItemIdentifier = &singleField{} + +func (sf *singleField) FindNodeByName(sequenceNode *yamlv3.Node, name string) (*yamlv3.Node, error) { + for _, mappingNode := range sequenceNode.Content { + nameOfNode, err := sf.Name(mappingNode) + if err != nil { + return nil, err + } + + if nameOfNode == name { + return mappingNode, nil + } + } + + return nil, fmt.Errorf("failed to find mapping entry with name %q", name) +} + +func (sf *singleField) Name(mappingNode *yamlv3.Node) (string, error) { + result, err := grab(mappingNode, sf.IdentifierFieldName) + if err != nil { + return "", err + } + + return followAlias(result).Value, nil +} + +func (sf *singleField) String() string { + return sf.IdentifierFieldName +} + +// --- --- --- + +// k8sItemIdentifier is an identifier aiming for Kubernetes items that have an +// api version, kind, and name field to be used +type k8sItemIdentifier struct{} + +var k8sItem listItemIdentifier = &k8sItemIdentifier{} + +func (i *k8sItemIdentifier) FindNodeByName(sequenceNode *yamlv3.Node, name string) (*yamlv3.Node, error) { + for _, mappingNode := range sequenceNode.Content { + nameOfNode, err := i.Name(mappingNode) + if err != nil { + return nil, err + } + + if nameOfNode == name { + return mappingNode, nil + } + } + + return nil, fmt.Errorf("failed to find mapping entry with name %q", name) +} + +func (i *k8sItemIdentifier) Name(node *yamlv3.Node) (string, error) { + if node.Kind != yamlv3.MappingNode { + return "", fmt.Errorf("provided node is not a mapping node") + } + + var elem []string + + apiVersion, err := grab(node, "apiVersion") + if err != nil { + return "", err + } + elem = append(elem, apiVersion.Value) + + kind, err := grab(node, "kind") + if err != nil { + return "", err + } + elem = append(elem, kind.Value) + + // namespace is optional and will be omitted if not set + namespace, err := grab(node, "metadata.namespace") + if err == nil { + elem = append(elem, namespace.Value) + } + + name, err := grab(node, "metadata.name") + if err != nil { + return "", err + } + elem = append(elem, name.Value) + + return strings.Join(elem, "/"), nil +} + +func (lf *k8sItemIdentifier) String() string { + return "resource" +} From e79693ecf67b52db157a1db671fca65c19f2eba0 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Sat, 14 Oct 2023 22:08:45 +0200 Subject: [PATCH 8/9] Add test case to check new if branch Add test case to check newly introduced branch in the `getIdentifierFromKubernetesEntityList` function. --- pkg/dyff/compare_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/dyff/compare_test.go b/pkg/dyff/compare_test.go index 771d822..65ba56a 100644 --- a/pkg/dyff/compare_test.go +++ b/pkg/dyff/compare_test.go @@ -635,6 +635,15 @@ listY: [ Yo, Yo, Yo ] Expect(isSameDetail(actual, expected)).To(BeTrue()) }) + It("should process simple lists independent of other compare settings", func() { + from := yml(`list: [ A, B, C, D, E ]`) + to := yml(`list: [ A, B, C, D, E ]`) + + results, err := compare(from, to, dyff.KubernetesEntityDetection(false)) + Expect(err).To(BeNil()) + Expect(results).To(BeNil()) + }) + It("should return all differences between the files with multiple documents", func() { expected := []dyff.Diff{ singleDiff("#0/spec/template/spec/containers/name=registry/resources/limits/cpu", dyff.MODIFICATION, "100m", "1000m"), From badc1b5d757d5d6f0dce251cf3ed7f0ba4df485c Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Sat, 14 Oct 2023 22:23:53 +0200 Subject: [PATCH 9/9] Add test case where from or to is null Add test case to check branch that deals with changes where one of the from or to item is null. --- pkg/dyff/compare_test.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/dyff/compare_test.go b/pkg/dyff/compare_test.go index 65ba56a..dfe4eea 100644 --- a/pkg/dyff/compare_test.go +++ b/pkg/dyff/compare_test.go @@ -24,11 +24,18 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/gonvenience/ytbx" - "github.com/homeport/dyff/pkg/dyff" + + "github.com/gonvenience/ytbx" + yamlv3 "gopkg.in/yaml.v3" ) +var nullNode = &yamlv3.Node{ + Kind: yamlv3.ScalarNode, + Tag: "!!null", + Value: "null", +} + var _ = Describe("Core/Compare", func() { Describe("Difference between YAMLs", func() { Context("Given two simple YAML structures", func() { @@ -605,6 +612,21 @@ listY: [ Yo, Yo, Yo ] })) }) + It("should return changes where one of the items is null", func() { + from := yml(`foo: null`) + to := yml(`foo: "bar"`) + results, err := compare(from, to) + Expect(err).To(BeNil()) + Expect(results).NotTo(BeNil()) + Expect(results).To(HaveLen(1)) + Expect(results[0]).To(BeSameDiffAs(singleDiff( + "/foo", + dyff.MODIFICATION, + nullNode, + "bar", + ))) + }) + It("should not return order changes in named entry lists in case the ignore option is enabled", func() { results, err := compare( yml(`list: [ {name: A}, {name: C}, {name: B}, {name: D}, {name: E} ]`),