From 5caed5b90ac08af141ccd195f8c7c264148320a2 Mon Sep 17 00:00:00 2001 From: Mengqi Yu Date: Wed, 17 Nov 2021 14:06:20 -0800 Subject: [PATCH] kyaml/fn/framework ensures the annotation output format matches the input If the input only contains legacy format anntations (path, index, id), the output will be the same. --- kyaml/fn/framework/framework.go | 13 ++ kyaml/kio/byteio_writer_test.go | 3 - kyaml/kio/kio.go | 275 ++++++++++++++++++-------------- kyaml/kio/kio_test.go | 82 ---------- kyaml/kio/kioutil/kioutil.go | 31 ++-- 5 files changed, 186 insertions(+), 218 deletions(-) diff --git a/kyaml/fn/framework/framework.go b/kyaml/fn/framework/framework.go index b1ddbc331..62b11a9b0 100644 --- a/kyaml/fn/framework/framework.go +++ b/kyaml/fn/framework/framework.go @@ -116,8 +116,21 @@ func Execute(p ResourceListProcessor, rlSource *kio.ByteReadWriter) error { } rl.FunctionConfig = rlSource.FunctionConfig + // We store the original + nodeAnnos, err := kio.StoreInternalAnnotations(rl.Items) + if err != nil { + return err + } + retErr := p.Process(&rl) + // If either the internal annotations for path, index, and id OR the legacy + // annotations for path, index, and id are changed, we have to update the other. + err = kio.ReconcileInternalAnnotations(rl.Items, nodeAnnos) + if err != nil { + return err + } + // Write the results // Set the ResourceList.results for validating functions if len(rl.Results) > 0 { diff --git a/kyaml/kio/byteio_writer_test.go b/kyaml/kio/byteio_writer_test.go index 835ed20cb..4897287b0 100644 --- a/kyaml/kio/byteio_writer_test.go +++ b/kyaml/kio/byteio_writer_test.go @@ -400,7 +400,6 @@ metadata: "a": "a long string that would certainly see a newline introduced by the YAML marshaller abcd123", "metadata": { "annotations": { - "config.kubernetes.io/path": "test.json", "internal.config.kubernetes.io/path": "test.json" } } @@ -429,7 +428,6 @@ metadata: "a": "a long string that would certainly see a newline introduced by the YAML marshaller abcd123", "metadata": { "annotations": { - "config.kubernetes.io/path": "test.json", "internal.config.kubernetes.io/path": "test.json" } } @@ -449,7 +447,6 @@ metadata: "a": "b", "metadata": { "annotations": { - "config.kubernetes.io/path": "test.json", "internal.config.kubernetes.io/path": "test.json" } } diff --git a/kyaml/kio/kio.go b/kyaml/kio/kio.go index 410c0a23f..e2361e693 100644 --- a/kyaml/kio/kio.go +++ b/kyaml/kio/kio.go @@ -7,7 +7,6 @@ package kio import ( "fmt" - "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/kio/kioutil" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -114,13 +113,11 @@ func (p Pipeline) ExecuteWithCallback(callback PipelineExecuteCallbackFunc) erro } // apply operations - var err error for i := range p.Filters { // Not all RNodes passed through kio.Pipeline have metadata nor should // they all be required to. - var nodeAnnos map[string]map[string]string - nodeAnnos, err = storeInternalAnnotations(result) - if err != nil && err != yaml.ErrMissingMetadata { + nodeAnnos, err := StoreInternalAnnotations(result) + if err != nil { return err } @@ -138,8 +135,8 @@ func (p Pipeline) ExecuteWithCallback(callback PipelineExecuteCallbackFunc) erro // If either the internal annotations for path, index, and id OR the legacy // annotations for path, index, and id are changed, we have to update the other. - err = reconcileInternalAnnotations(result, nodeAnnos) - if err != nil && err != yaml.ErrMissingMetadata { + err = reconcileInternalAnnotations(result, nodeAnnos, false) + if err != nil { return err } } @@ -166,55 +163,51 @@ func FilterAll(filter yaml.Filter) Filter { }) } -// Store the original path, index, and id annotations so that we can reconcile +// StoreInternalAnnotations stores the original path, index, and id annotations so that we can reconcile // it later. This is necessary because currently both internal-prefixed annotations // and legacy annotations are currently supported, and a change to one must be // reflected in the other. -func storeInternalAnnotations(result []*yaml.RNode) (map[string]map[string]string, error) { - nodeAnnosMap := make(map[string]map[string]string) +func StoreInternalAnnotations(result []*yaml.RNode) (map[nodeAnnotations]map[string]string, error) { + nodeAnnosMap := make(map[nodeAnnotations]map[string]string) for i := range result { + id := kioutil.GetIdAnnotation(result[i]) + path, index, _ := kioutil.GetFileAnnotations(result[i]) + annoKey := nodeAnnotations{ + path: path, + index: index, + id: id, + } + nodeAnnosMap[annoKey] = kioutil.GetInternalAnnotations(result[i]) if err := kioutil.CopyLegacyAnnotations(result[i]); err != nil { return nil, err } - meta, err := result[i].GetMeta() - if err != nil { + + if err := checkMismatchedAnnos(result[i].GetAnnotations()); err != nil { return nil, err } - if err := checkMismatchedAnnos(meta); err != nil { - return nil, err - } - - path := meta.Annotations[kioutil.PathAnnotation] - index := meta.Annotations[kioutil.IndexAnnotation] - id := meta.Annotations[kioutil.IdAnnotation] - - if _, ok := nodeAnnosMap[path]; !ok { - nodeAnnosMap[path] = make(map[string]string) - } - nodeAnnosMap[path][index] = id } return nodeAnnosMap, nil } -func checkMismatchedAnnos(meta yaml.ResourceMeta) error { - path := meta.Annotations[kioutil.PathAnnotation] - index := meta.Annotations[kioutil.IndexAnnotation] - id := meta.Annotations[kioutil.IdAnnotation] +func checkMismatchedAnnos(annotations map[string]string) error { + path := annotations[kioutil.PathAnnotation] + index := annotations[kioutil.IndexAnnotation] + id := annotations[kioutil.IdAnnotation] - legacyPath := meta.Annotations[kioutil.LegacyPathAnnotation] - legacyIndex := meta.Annotations[kioutil.LegacyIndexAnnotation] - legacyId := meta.Annotations[kioutil.LegacyIdAnnotation] + legacyPath := annotations[kioutil.LegacyPathAnnotation] + legacyIndex := annotations[kioutil.LegacyIndexAnnotation] + legacyId := annotations[kioutil.LegacyIdAnnotation] // if prior to running the functions, the legacy and internal annotations differ, // throw an error as we cannot infer the user's intent. - if path != legacyPath { + if path != "" && legacyPath != "" && path != legacyPath { return fmt.Errorf("resource input to function has mismatched legacy and internal path annotations") } - if index != legacyIndex { + if index != "" && legacyIndex != "" && index != legacyIndex { return fmt.Errorf("resource input to function has mismatched legacy and internal index annotations") } - if id != legacyId { + if id != "" && legacyId != "" && id != legacyId { return fmt.Errorf("resource input to function has mismatched legacy and internal id annotations") } return nil @@ -226,30 +219,41 @@ type nodeAnnotations struct { id string } -func reconcileInternalAnnotations(result []*yaml.RNode, nodeAnnosMap map[string]map[string]string) error { - for _, node := range result { - meta, err := node.GetMeta() - if err != nil { +// ReconcileInternalAnnotations reconciles the annotation format for path, index and id annotations. +func ReconcileInternalAnnotations(result []*yaml.RNode, nodeAnnosMap map[nodeAnnotations]map[string]string) error { + return reconcileInternalAnnotations(result, nodeAnnosMap, true) +} + +func reconcileInternalAnnotations(result []*yaml.RNode, nodeAnnosMap map[nodeAnnotations]map[string]string, enforceAnnotationsFormat bool) error { + var useInternal, useLegacy bool + var err error + if enforceAnnotationsFormat { + if useInternal, useLegacy, err = determineAnnotationsFormat(nodeAnnosMap); err != nil { return err } + } + + for i := range result { // if only one annotation is set, set the other. - err = missingInternalOrLegacyAnnotations(node, meta) + err := missingInternalOrLegacyAnnotations(result[i]) if err != nil { return err } // we must check to see if the function changed either the new internal annotations // or the old legacy annotations. If one is changed, the change must be reflected // in the other. - err = checkAnnotationsAltered(node, meta, nodeAnnosMap) + err = checkAnnotationsAltered(result[i], nodeAnnosMap) if err != nil { return err } + if enforceAnnotationsFormat { + err = enforceConsistentAnnotationFormat(result[i], useInternal, useLegacy) + if err != nil { + return err + } + } // if the annotations are still somehow out of sync, throw an error - meta, err = node.GetMeta() - if err != nil { - return err - } - err = checkMismatchedAnnos(meta) + err = checkMismatchedAnnos(result[i].GetAnnotations()) if err != nil { return err } @@ -257,22 +261,63 @@ func reconcileInternalAnnotations(result []*yaml.RNode, nodeAnnosMap map[string] return nil } -func missingInternalOrLegacyAnnotations(rn *yaml.RNode, meta yaml.ResourceMeta) error { - if err := missingInternalOrLegacyAnnotation(rn, meta, kioutil.PathAnnotation, kioutil.LegacyPathAnnotation); err != nil { +// determineAnnotationsFormat determines if the resources are using one of the internal and legacy annotation format or both of them. +func determineAnnotationsFormat(nodeAnnosMap map[nodeAnnotations]map[string]string) (useInternal, useLegacy bool, err error) { + if len(nodeAnnosMap) == 0 { + return true, true, nil + } + + var internal, legacy *bool + for _, annos := range nodeAnnosMap { + _, foundPath := annos[kioutil.PathAnnotation] + _, foundIndex := annos[kioutil.IndexAnnotation] + _, foundId := annos[kioutil.IdAnnotation] + foundOneOf := foundPath || foundIndex || foundId + if internal == nil { + internal = &foundOneOf + } + if (foundOneOf && !*internal) || (!foundOneOf && *internal) { + err = fmt.Errorf("the formatting in the input resources is not consistent") + return + } + + _, foundPath = annos[kioutil.LegacyPathAnnotation] + _, foundIndex = annos[kioutil.LegacyIndexAnnotation] + _, foundId = annos[kioutil.LegacyIdAnnotation] + foundOneOf = foundPath || foundIndex || foundId + if legacy == nil { + legacy = &foundOneOf + } + if (foundOneOf && !*legacy) || (!foundOneOf && *legacy) { + err = fmt.Errorf("the formatting in the input resources is not consistent") + return + } + } + if internal != nil { + useInternal = *internal + } + if legacy != nil { + useLegacy = *legacy + } + return +} + +func missingInternalOrLegacyAnnotations(rn *yaml.RNode) error { + if err := missingInternalOrLegacyAnnotation(rn, kioutil.PathAnnotation, kioutil.LegacyPathAnnotation); err != nil { return err } - if err := missingInternalOrLegacyAnnotation(rn, meta, kioutil.IndexAnnotation, kioutil.LegacyIndexAnnotation); err != nil { + if err := missingInternalOrLegacyAnnotation(rn, kioutil.IndexAnnotation, kioutil.LegacyIndexAnnotation); err != nil { return err } - if err := missingInternalOrLegacyAnnotation(rn, meta, kioutil.IdAnnotation, kioutil.LegacyIdAnnotation); err != nil { + if err := missingInternalOrLegacyAnnotation(rn, kioutil.IdAnnotation, kioutil.LegacyIdAnnotation); err != nil { return err } return nil } -func missingInternalOrLegacyAnnotation(rn *yaml.RNode, meta yaml.ResourceMeta, newKey string, legacyKey string) error { - value := meta.Annotations[newKey] - legacyValue := meta.Annotations[legacyKey] +func missingInternalOrLegacyAnnotation(rn *yaml.RNode, newKey string, legacyKey string) error { + value := rn.GetAnnotations()[newKey] + legacyValue := rn.GetAnnotations()[legacyKey] if value == "" && legacyValue == "" { // do nothing @@ -293,98 +338,82 @@ func missingInternalOrLegacyAnnotation(rn *yaml.RNode, meta yaml.ResourceMeta, n return nil } -func checkAnnotationsAltered(rn *yaml.RNode, meta yaml.ResourceMeta, nodeAnnosMap map[string]map[string]string) error { +func checkAnnotationsAltered(rn *yaml.RNode, nodeAnnosMap map[nodeAnnotations]map[string]string) error { + annotations := rn.GetAnnotations() // get the resource's current path, index, and ids from the new annotations internal := nodeAnnotations{ - path: meta.Annotations[kioutil.PathAnnotation], - index: meta.Annotations[kioutil.IndexAnnotation], - id: meta.Annotations[kioutil.IdAnnotation], + path: annotations[kioutil.PathAnnotation], + index: annotations[kioutil.IndexAnnotation], + id: annotations[kioutil.IdAnnotation], } // get the resource's current path, index, and ids from the legacy annotations legacy := nodeAnnotations{ - path: meta.Annotations[kioutil.LegacyPathAnnotation], - index: meta.Annotations[kioutil.LegacyIndexAnnotation], - id: meta.Annotations[kioutil.LegacyIdAnnotation], + path: annotations[kioutil.LegacyPathAnnotation], + index: annotations[kioutil.LegacyIndexAnnotation], + id: annotations[kioutil.LegacyIdAnnotation], } - if internal.path == legacy.path && - internal.index == legacy.index && - internal.id == legacy.id { - // none of the annotations differ, so no reconciliation is needed - return nil + originalAnnotations, found := nodeAnnosMap[internal] + if !found { + originalAnnotations, found = nodeAnnosMap[legacy] } - - // nodeAnnosMap is a map of structure path -> index -> id that stores - // all of the resources' path/index/id annotations prior to the functions - // being run. We use that to check whether the legacy or new internal - // annotations have been changed, and make sure the change is reflected - // in the other. - - // first, check if the internal annotations are found in nodeAnnosMap - if indexIdMap, ok := nodeAnnosMap[internal.path]; ok { - if id, ok := indexIdMap[internal.index]; ok { - if id == internal.id { - // the internal annotations of the resource match the ones stored in - // nodeAnnosMap, so we should copy the legacy annotations to the - // internal ones - if err := updateAnnotations(rn, meta, - []string{ - kioutil.PathAnnotation, - kioutil.IndexAnnotation, - kioutil.IdAnnotation, - }, - []string{ - legacy.path, - legacy.index, - legacy.id, - }); err != nil { - return err - } + originalPath, found := originalAnnotations[kioutil.PathAnnotation] + if !found { + originalPath = originalAnnotations[kioutil.LegacyPathAnnotation] + } + if originalPath != "" { + if originalPath != internal.path { + if _, err := rn.Pipe(yaml.SetAnnotation(kioutil.LegacyPathAnnotation, internal.path)); err != nil { + return err + } + } else if originalPath != legacy.path { + if _, err := rn.Pipe(yaml.SetAnnotation(kioutil.PathAnnotation, legacy.path)); err != nil { + return err } } } - // check the opposite, to see if the legacy annotations are in nodeAnnosMap - if indexIdMap, ok := nodeAnnosMap[legacy.path]; ok { - if id, ok := indexIdMap[legacy.index]; ok { - if id == legacy.id { - // the legacy annotations of the resource match the ones stored in - // nodeAnnosMap, so we should copy the internal annotations to the - // legacy ones - if err := updateAnnotations(rn, meta, - []string{ - kioutil.LegacyPathAnnotation, - kioutil.LegacyIndexAnnotation, - kioutil.LegacyIdAnnotation, - }, - []string{ - internal.path, - internal.index, - internal.id, - }); err != nil { - return err - } + originalIndex, found := originalAnnotations[kioutil.IndexAnnotation] + if !found { + originalIndex = originalAnnotations[kioutil.LegacyIndexAnnotation] + } + if originalIndex != "" { + if originalIndex != internal.index { + if _, err := rn.Pipe(yaml.SetAnnotation(kioutil.LegacyIndexAnnotation, internal.index)); err != nil { + return err + } + } else if originalIndex != legacy.index { + if _, err := rn.Pipe(yaml.SetAnnotation(kioutil.IndexAnnotation, legacy.index)); err != nil { + return err } } } return nil } -func updateAnnotations(rn *yaml.RNode, meta yaml.ResourceMeta, keys []string, values []string) error { - if len(keys) != len(values) { - return fmt.Errorf("keys is not same length as values") - } - for i := range keys { - _, ok := meta.Annotations[keys[i]] - if values[i] == "" && !ok { - // don't set "" if annotation is not already there - continue - } - if err := rn.PipeE(yaml.SetAnnotation(keys[i], values[i])); err != nil { +func enforceConsistentAnnotationFormat(rn *yaml.RNode, useInternal, useLegacy bool) error { + if !useInternal { + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.IdAnnotation)); err != nil { + return err + } + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.PathAnnotation)); err != nil { + return err + } + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.IndexAnnotation)); err != nil { + return err + } + } + if !useLegacy { + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.LegacyIdAnnotation)); err != nil { + return err + } + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.LegacyPathAnnotation)); err != nil { + return err + } + if err := rn.PipeE(yaml.ClearAnnotation(kioutil.LegacyIndexAnnotation)); err != nil { return err } - } return nil } diff --git a/kyaml/kio/kio_test.go b/kyaml/kio/kio_test.go index 8fccf4b07..b63b59b01 100644 --- a/kyaml/kio/kio_test.go +++ b/kyaml/kio/kio_test.go @@ -217,22 +217,6 @@ func TestLegacyAnnotationReconciliation(t *testing.T) { } return nodes, nil } - changeLegacyId := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { - for _, rn := range nodes { - if err := rn.PipeE(yaml.SetAnnotation(kioutil.LegacyIdAnnotation, "new")); err != nil { - return nil, err - } - } - return nodes, nil - } - changeInternalId := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { - for _, rn := range nodes { - if err := rn.PipeE(yaml.SetAnnotation(kioutil.IdAnnotation, "new")); err != nil { - return nil, err - } - } - return nodes, nil - } changeBothPathAnnos := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { for _, rn := range nodes { if err := rn.PipeE(yaml.SetAnnotation(kioutil.LegacyPathAnnotation, "legacy")); err != nil { @@ -251,8 +235,6 @@ func TestLegacyAnnotationReconciliation(t *testing.T) { } internal := []Filter{FilterFunc(changeInternalAnnos)} legacy := []Filter{FilterFunc(changeLegacyAnnos)} - legacyId := []Filter{FilterFunc(changeLegacyId)} - internalId := []Filter{FilterFunc(changeInternalId)} changeBoth := []Filter{FilterFunc(changeBothPathAnnos), FilterFunc(noopFilter1)} testCases := map[string]struct { @@ -457,70 +439,6 @@ metadata: internal.config.kubernetes.io/index: 'new' data: grpcPort: 8081 -`, - }, - // the orchestrator should detect that the new internal id annotation - // has been changed, and copy it over to the legacy one, and also - // copy the path and index legacy annotations to the new internal - // ones - "change only internal id when original legacy set": { - input: `apiVersion: v1 -kind: ConfigMap -metadata: - name: ports-from - annotations: - config.kubernetes.io/path: 'configmap.yaml' - config.kubernetes.io/index: '0' - config.k8s.io/id: '1' -data: - grpcPort: 8080 -`, - filters: internalId, - expected: `apiVersion: v1 -kind: ConfigMap -metadata: - name: ports-from - annotations: - config.kubernetes.io/path: 'configmap.yaml' - config.kubernetes.io/index: '0' - config.k8s.io/id: 'new' - internal.config.kubernetes.io/path: 'configmap.yaml' - internal.config.kubernetes.io/index: '0' - internal.config.kubernetes.io/id: 'new' -data: - grpcPort: 8080 -`, - }, - // the orchestrator should detect that the legacy id annotation - // has been changed, and copy it over to the internal one, and also - // copy the path and index internal annotations to the legacy - // ones - "change only legacy id when internal legacy set": { - input: `apiVersion: v1 -kind: ConfigMap -metadata: - name: ports-from - annotations: - internal.config.kubernetes.io/path: 'configmap.yaml' - internal.config.kubernetes.io/index: '0' - internal.config.kubernetes.io/id: '1' -data: - grpcPort: 8080 -`, - filters: legacyId, - expected: `apiVersion: v1 -kind: ConfigMap -metadata: - name: ports-from - annotations: - internal.config.kubernetes.io/path: 'configmap.yaml' - internal.config.kubernetes.io/index: '0' - internal.config.kubernetes.io/id: 'new' - config.kubernetes.io/path: 'configmap.yaml' - config.kubernetes.io/index: '0' - config.k8s.io/id: 'new' -data: - grpcPort: 8080 `, }, // the function changes both the legacy and new path annotation, diff --git a/kyaml/kio/kioutil/kioutil.go b/kyaml/kio/kioutil/kioutil.go index 12cf85b11..800dcfe45 100644 --- a/kyaml/kio/kioutil/kioutil.go +++ b/kyaml/kio/kioutil/kioutil.go @@ -44,18 +44,27 @@ const ( ) func GetFileAnnotations(rn *yaml.RNode) (string, string, error) { - if err := CopyLegacyAnnotations(rn); err != nil { - return "", "", err + annotations := rn.GetAnnotations() + path, found := annotations[PathAnnotation] + if !found { + path = annotations[LegacyPathAnnotation] } - meta, err := rn.GetMeta() - if err != nil { - return "", "", err + index, found := annotations[IndexAnnotation] + if !found { + index = annotations[LegacyIndexAnnotation] } - path := meta.Annotations[PathAnnotation] - index := meta.Annotations[IndexAnnotation] return path, index, nil } +func GetIdAnnotation(rn *yaml.RNode) string { + annotations := rn.GetAnnotations() + id, found := annotations[IdAnnotation] + if !found { + id = annotations[LegacyIdAnnotation] + } + return id +} + func CopyLegacyAnnotations(rn *yaml.RNode) error { meta, err := rn.GetMeta() if err != nil { @@ -377,13 +386,15 @@ func ConfirmInternalAnnotationUnchanged(r1 *yaml.RNode, r2 *yaml.RNode, exclusio return nil } -// GetInternalAnnotations returns a map of all the annotations of the provided RNode that begin -// with the prefix `internal.config.kubernetes.io` +// GetInternalAnnotations returns a map of all the annotations of the provided +// RNode that satisfies one of the following: 1) begin with the prefix +// `internal.config.kubernetes.io` 2) is one of `config.kubernetes.io/path`, +// `config.kubernetes.io/index` and `config.k8s.io/id`. func GetInternalAnnotations(rn *yaml.RNode) map[string]string { annotations := rn.GetAnnotations() result := make(map[string]string) for k, v := range annotations { - if strings.HasPrefix(k, internalPrefix) { + if strings.HasPrefix(k, internalPrefix) || k == LegacyPathAnnotation || k == LegacyIndexAnnotation || k == LegacyIdAnnotation { result[k] = v } }