fix: manifest generation error with null annotations (argoproj#14336) (…

…argoproj#14680) * fix: manifest generation error with null annotations Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com> * fix test Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com> * fix unit tests Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com> --------- Signed-off-by: Alexandre Gaudreault <alexandre.gaudreault@logmein.com>
tesla59 · Dec 16, 2023 · e0d2fdb · e0d2fdb
1 parent 091bd46
commit e0d2fdb
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -21,8 +21,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 fmt.Errorf("failed to get labels from target object %s %s/%s: %w", target.GroupVersionKind().String(), target.GetNamespace(), target.GetName(), err)
 	}
@@ -101,11 +100,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)
 	}
@@ -116,10 +115,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
@@ -129,8 +127,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 "", fmt.Errorf("failed to get labels for %s %s/%s: %w", un.GroupVersionKind().String(), un.GetNamespace(), un.GetName(), err)
 	}
@@ -142,8 +139,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 fmt.Errorf("failed to get labels for %s %s/%s: %w", un.GroupVersionKind().String(), un.GetNamespace(), un.GetName(), err)
 	}
@@ -164,3 +160,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
@@ -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: <nil> is of the type <nil>, 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: <nil> is of the type <nil>, 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: <nil> is of the type <nil>, 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: <nil> is of the type <nil>, expected string", err.Error())
 }
 
 func TestGetAppInstanceLabel(t *testing.T) {