From 3f42c538a11f45569a2ae85b64eb9c004bc901d3 Mon Sep 17 00:00:00 2001 From: "gcp-cherry-pick-bot[bot]" <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Date: Wed, 26 Jul 2023 17:09:15 -0400 Subject: [PATCH] fix: manifest generation error with null annotations (#14336) (#14680) (#14735) * fix: manifest generation error with null annotations * fix test * fix unit tests --------- Signed-off-by: Alexandre Gaudreault Co-authored-by: Alexandre Gaudreault --- reposerver/repository/repository_test.go | 22 ++++++++++++ .../testdata/invalid-metadata/bad.yaml | 17 ++++++++++ .../nil-metadata-accessors.yaml | 8 +++++ util/kube/kube.go | 34 ++++++++++++------- util/kube/kube_test.go | 4 +-- 5 files changed, 71 insertions(+), 14 deletions(-) create mode 100644 reposerver/repository/testdata/invalid-metadata/bad.yaml create mode 100644 reposerver/repository/testdata/nil-metadata-accessors/nil-metadata-accessors.yaml diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index e3e29d0b0c0ad..2465a7dcb6242 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -413,6 +413,28 @@ func TestInvalidManifestsInDir(t *testing.T) { assert.NotNil(t, err) } +func TestInvalidMetadata(t *testing.T) { + service := newService(".") + + src := argoappv1.ApplicationSource{Path: "./testdata/invalid-metadata", Directory: &argoappv1.ApplicationSourceDirectory{Recurse: true}} + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &src, AppLabelKey: "test", AppName: "invalid-metadata", TrackingMethod: "annotation+label"} + _, err := service.GenerateManifest(context.Background(), &q) + assert.Error(t, err) + assert.Contains(t, err.Error(), "contains non-string key in the map") +} + +func TestNilMetadataAccessors(t *testing.T) { + service := newService(".") + expected := "{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{\"argocd.argoproj.io/tracking-id\":\"nil-metadata-accessors:/ConfigMap:/my-map\"},\"labels\":{\"test\":\"nil-metadata-accessors\"},\"name\":\"my-map\"},\"stringData\":{\"foo\":\"bar\"}}" + + src := argoappv1.ApplicationSource{Path: "./testdata/nil-metadata-accessors", Directory: &argoappv1.ApplicationSourceDirectory{Recurse: true}} + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &src, AppLabelKey: "test", AppName: "nil-metadata-accessors", TrackingMethod: "annotation+label"} + res, err := service.GenerateManifest(context.Background(), &q) + assert.NoError(t, err) + assert.Equal(t, len(res.Manifests), 1) + assert.Equal(t, expected, res.Manifests[0]) +} + func TestGenerateJsonnetManifestInDir(t *testing.T) { service := newService(".") diff --git a/reposerver/repository/testdata/invalid-metadata/bad.yaml b/reposerver/repository/testdata/invalid-metadata/bad.yaml new file mode 100644 index 0000000000000..83f48a40dc334 --- /dev/null +++ b/reposerver/repository/testdata/invalid-metadata/bad.yaml @@ -0,0 +1,17 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: my-map-annotation + annotations: + invalid: true +stringData: + foo: bar +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: my-map-label + labels: + invalid: true +stringData: + foo: bar diff --git a/reposerver/repository/testdata/nil-metadata-accessors/nil-metadata-accessors.yaml b/reposerver/repository/testdata/nil-metadata-accessors/nil-metadata-accessors.yaml new file mode 100644 index 0000000000000..53979de769c01 --- /dev/null +++ b/reposerver/repository/testdata/nil-metadata-accessors/nil-metadata-accessors.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: my-map + annotations: + labels: +stringData: + foo: bar diff --git a/util/kube/kube.go b/util/kube/kube.go index ad3dd47e804eb..2e0be641f2a16 100644 --- a/util/kube/kube.go +++ b/util/kube/kube.go @@ -20,8 +20,7 @@ func IsValidResourceName(name string) bool { // SetAppInstanceLabel the recommended app.kubernetes.io/instance label against an unstructured object // Uses the legacy labeling if environment variable is set func SetAppInstanceLabel(target *unstructured.Unstructured, key, val string) error { - // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 - labels, _, err := unstructured.NestedStringMap(target.Object, "metadata", "labels") + labels, _, err := nestedNullableStringMap(target.Object, "metadata", "labels") if err != nil { return err } @@ -100,11 +99,11 @@ func SetAppInstanceLabel(target *unstructured.Unstructured, key, val string) err // SetAppInstanceAnnotation the recommended app.kubernetes.io/instance annotation against an unstructured object // Uses the legacy labeling if environment variable is set func SetAppInstanceAnnotation(target *unstructured.Unstructured, key, val string) error { - // Do not use target.GetAnnotations(), https://github.com/argoproj/argo-cd/issues/13730 - annotations, _, err := unstructured.NestedStringMap(target.Object, "metadata", "annotations") + annotations, _, err := nestedNullableStringMap(target.Object, "metadata", "annotations") if err != nil { - return err + return fmt.Errorf("failed to get annotations from target object %s %s/%s: %w", target.GroupVersionKind().String(), target.GetNamespace(), target.GetName(), err) } + if annotations == nil { annotations = make(map[string]string) } @@ -115,10 +114,9 @@ func SetAppInstanceAnnotation(target *unstructured.Unstructured, key, val string // GetAppInstanceAnnotation returns the application instance name from annotation func GetAppInstanceAnnotation(un *unstructured.Unstructured, key string) (string, error) { - // Do not use target.GetAnnotations(), https://github.com/argoproj/argo-cd/issues/13730 - annotations, _, err := unstructured.NestedStringMap(un.Object, "metadata", "annotations") + annotations, _, err := nestedNullableStringMap(un.Object, "metadata", "annotations") if err != nil { - return "", err + return "", fmt.Errorf("failed to get annotations from target object %s %s/%s: %w", un.GroupVersionKind().String(), un.GetNamespace(), un.GetName(), err) } if annotations != nil { return annotations[key], nil @@ -128,8 +126,7 @@ func GetAppInstanceAnnotation(un *unstructured.Unstructured, key string) (string // GetAppInstanceLabel returns the application instance name from labels func GetAppInstanceLabel(un *unstructured.Unstructured, key string) (string, error) { - // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 - labels, _, err := unstructured.NestedStringMap(un.Object, "metadata", "labels") + labels, _, err := nestedNullableStringMap(un.Object, "metadata", "labels") if err != nil { return "", err } @@ -141,8 +138,7 @@ func GetAppInstanceLabel(un *unstructured.Unstructured, key string) (string, err // RemoveLabel removes label with the specified name func RemoveLabel(un *unstructured.Unstructured, key string) error { - // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 - labels, _, err := unstructured.NestedStringMap(un.Object, "metadata", "labels") + labels, _, err := nestedNullableStringMap(un.Object, "metadata", "labels") if err != nil { return err } @@ -163,3 +159,17 @@ func RemoveLabel(un *unstructured.Unstructured, key string) error { } return nil } + +// nestedNullableStringMap returns a copy of map[string]string value of a nested field. +// Returns false if value is not found and an error if not one of map[string]interface{} or nil, or contains non-string values in the map. +func nestedNullableStringMap(obj map[string]interface{}, fields ...string) (map[string]string, bool, error) { + var m map[string]string + val, found, err := unstructured.NestedFieldNoCopy(obj, fields...) + if err != nil { + return nil, found, err + } + if found && val != nil { + return unstructured.NestedStringMap(obj, fields...) + } + return m, found, err +} diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go index f7fc1607aaa39..21d941b836a83 100644 --- a/util/kube/kube_test.go +++ b/util/kube/kube_test.go @@ -192,7 +192,7 @@ func TestSetAppInstanceAnnotationWithInvalidData(t *testing.T) { assert.Nil(t, err) err = SetAppInstanceAnnotation(&obj, common.LabelKeyAppInstance, "my-app") assert.Error(t, err) - assert.Equal(t, ".metadata.annotations accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) + assert.Equal(t, "failed to get annotations from target object /v1, Kind=Service /my-service: .metadata.annotations accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) } func TestGetAppInstanceAnnotation(t *testing.T) { @@ -218,7 +218,7 @@ func TestGetAppInstanceAnnotationWithInvalidData(t *testing.T) { _, err = GetAppInstanceAnnotation(&obj, "valid-annotation") assert.Error(t, err) - assert.Equal(t, ".metadata.annotations accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) + assert.Equal(t, "failed to get annotations from target object /v1, Kind=Service /my-service: .metadata.annotations accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) } func TestGetAppInstanceLabel(t *testing.T) {