fix(appset): return error on invalid annotations

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>
argoproj · May 25, 2023 · dce871d · dce871d
1 parent 0a8a71e
commit dce871d
Show file tree

Hide file tree

Showing 5 changed files with 143 additions and 26 deletions.
diff --git a/util/argo/resource_tracking.go b/util/argo/resource_tracking.go
@@ -61,7 +61,10 @@ func IsOldTrackingMethod(trackingMethod string) bool {
 }
 
 func (rt *resourceTracking) getAppInstanceValue(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) *AppInstanceValue {
-	appInstanceAnnotation := argokube.GetAppInstanceAnnotation(un, common.AnnotationKeyAppInstance)
+	appInstanceAnnotation, err := argokube.GetAppInstanceAnnotation(un, common.AnnotationKeyAppInstance)
+	if err != nil {
+		return nil
+	}
 	value, err := rt.ParseAppInstanceValue(appInstanceAnnotation)
 	if err != nil {
 		return nil
@@ -80,13 +83,21 @@ func (rt *resourceTracking) GetAppName(un *unstructured.Unstructured, key string
 	}
 	switch trackingMethod {
 	case TrackingMethodLabel:
-		return argokube.GetAppInstanceLabel(un, key)
+		label, err := argokube.GetAppInstanceLabel(un, key)
+		if err != nil {
+			return ""
+		}
+		return label
 	case TrackingMethodAnnotationAndLabel:
 		return retrieveAppInstanceValue()
 	case TrackingMethodAnnotation:
 		return retrieveAppInstanceValue()
 	default:
-		return argokube.GetAppInstanceLabel(un, key)
+		label, err := argokube.GetAppInstanceLabel(un, key)
+		if err != nil {
+			return ""
+		}
+		return label
 	}
 }
 
@@ -185,19 +196,32 @@ func (rt *resourceTracking) Normalize(config, live *unstructured.Unstructured, l
 		return nil
 	}
 
-	label := kube.GetAppInstanceLabel(live, labelKey)
+	label, err := kube.GetAppInstanceLabel(live, labelKey)
+	if err != nil {
+		return err
+	}
 	if label == "" {
 		return nil
 	}
 
-	annotation := argokube.GetAppInstanceAnnotation(config, common.AnnotationKeyAppInstance)
-	err := argokube.SetAppInstanceAnnotation(live, common.AnnotationKeyAppInstance, annotation)
+	annotation, err := argokube.GetAppInstanceAnnotation(config, common.AnnotationKeyAppInstance)
+	if err != nil {
+		return err
+	}
+	err = argokube.SetAppInstanceAnnotation(live, common.AnnotationKeyAppInstance, annotation)
 	if err != nil {
 		return err
 	}
 
-	if argokube.GetAppInstanceLabel(config, labelKey) == "" {
-		argokube.RemoveLabel(live, labelKey)
+	label, err = argokube.GetAppInstanceLabel(config, labelKey)
+	if err != nil {
+		return err
+	}
+	if label == "" {
+		err = argokube.RemoveLabel(live, labelKey)
+		if err != nil {
+			return err
+		}
 	}
 
 	return nil

diff --git a/util/argo/resource_tracking_test.go b/util/argo/resource_tracking_test.go
@@ -136,7 +136,8 @@ func TestResourceIdNormalizer_Normalize(t *testing.T) {
 	_ = rt.Normalize(configObj, liveObj, common.LabelKeyAppInstance, string(TrackingMethodAnnotation))
 
 	// the normalization should affect add the new style annotation and drop old tracking label from live object
-	annotation := kube.GetAppInstanceAnnotation(configObj, common.AnnotationKeyAppInstance)
+	annotation, err := kube.GetAppInstanceAnnotation(configObj, common.AnnotationKeyAppInstance)
+	assert.Nil(t, err)
 	assert.Equal(t, liveObj.GetAnnotations()[common.AnnotationKeyAppInstance], annotation)
 	_, hasOldLabel := liveObj.GetLabels()[common.LabelKeyAppInstance]
 	assert.False(t, hasOldLabel)
@@ -160,7 +161,8 @@ func TestResourceIdNormalizer_Normalize_ConfigHasOldLabel(t *testing.T) {
 	_ = rt.Normalize(configObj, liveObj, common.LabelKeyAppInstance, string(TrackingMethodAnnotation))
 
 	// the normalization should affect add the new style annotation and drop old tracking label from live object
-	annotation := kube.GetAppInstanceAnnotation(configObj, common.AnnotationKeyAppInstance)
+	annotation, err := kube.GetAppInstanceAnnotation(configObj, common.AnnotationKeyAppInstance)
+	assert.Nil(t, err)
 	assert.Equal(t, liveObj.GetAnnotations()[common.AnnotationKeyAppInstance], annotation)
 	_, hasOldLabel := liveObj.GetLabels()[common.LabelKeyAppInstance]
 	assert.True(t, hasOldLabel)

diff --git a/util/kube/kube.go b/util/kube/kube.go
@@ -20,7 +20,11 @@ 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 {
-	labels := target.GetLabels()
+	// Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730
+	labels, _, err := unstructured.NestedStringMap(target.Object, "metadata", "labels")
+	if err != nil {
+		return err
+	}
 	if labels == nil {
 		labels = make(map[string]string)
 	}
@@ -96,7 +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 {
-	annotations := target.GetAnnotations()
+	// Do not use target.GetAnnotations(), https://github.com/argoproj/argo-cd/issues/13730
+	annotations, _, err := unstructured.NestedStringMap(target.Object, "metadata", "annotations")
+	if err != nil {
+		return err
+	}
 	if annotations == nil {
 		annotations = make(map[string]string)
 	}
@@ -106,26 +114,40 @@ func SetAppInstanceAnnotation(target *unstructured.Unstructured, key, val string
 }
 
 // GetAppInstanceAnnotation returns the application instance name from annotation
-func GetAppInstanceAnnotation(un *unstructured.Unstructured, key string) string {
-	if annotations := un.GetAnnotations(); annotations != nil {
-		return annotations[key]
+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")
+	if err != nil {
+		return "", err
 	}
-	return ""
+	if annotations != nil {
+		return annotations[key], nil
+	}
+	return "", nil
 }
 
 // GetAppInstanceLabel returns the application instance name from labels
-func GetAppInstanceLabel(un *unstructured.Unstructured, key string) string {
-	if labels := un.GetLabels(); labels != nil {
-		return labels[key]
+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")
+	if err != nil {
+		return "", err
+	}
+	if labels != nil {
+		return labels[key], nil
 	}
-	return ""
+	return "", nil
 }
 
 // RemoveLabel removes label with the specified name
-func RemoveLabel(un *unstructured.Unstructured, key string) {
-	labels := un.GetLabels()
+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")
+	if err != nil {
+		return err
+	}
 	if labels == nil {
-		return
+		return nil
 	}
 
 	for k := range labels {
@@ -139,4 +161,5 @@ func RemoveLabel(un *unstructured.Unstructured, key string) {
 			break
 		}
 	}
+	return nil
 }
diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go
@@ -184,6 +184,17 @@ func TestSetAppInstanceAnnotation(t *testing.T) {
 	assert.Equal(t, "my-app", s.ObjectMeta.Annotations[common.LabelKeyAppInstance])
 }
 
+func TestSetAppInstanceAnnotationWithInvalidData(t *testing.T) {
+	yamlBytes, err := os.ReadFile("testdata/svc-with-invalid-data.yaml")
+	assert.Nil(t, err)
+	var obj unstructured.Unstructured
+	err = yaml.Unmarshal(yamlBytes, &obj)
+	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())
+}
+
 func TestGetAppInstanceAnnotation(t *testing.T) {
 	yamlBytes, err := os.ReadFile("testdata/svc.yaml")
 	assert.Nil(t, err)
@@ -193,7 +204,21 @@ func TestGetAppInstanceAnnotation(t *testing.T) {
 	err = SetAppInstanceAnnotation(&obj, common.LabelKeyAppInstance, "my-app")
 	assert.Nil(t, err)
 
-	assert.Equal(t, "my-app", GetAppInstanceAnnotation(&obj, common.LabelKeyAppInstance))
+	annotation, err := GetAppInstanceAnnotation(&obj, common.LabelKeyAppInstance)
+	assert.Nil(t, err)
+	assert.Equal(t, "my-app", annotation)
+}
+
+func TestGetAppInstanceAnnotationWithInvalidData(t *testing.T) {
+	yamlBytes, err := os.ReadFile("testdata/svc-with-invalid-data.yaml")
+	assert.Nil(t, err)
+	var obj unstructured.Unstructured
+	err = yaml.Unmarshal(yamlBytes, &obj)
+	assert.Nil(t, err)
+
+	_, 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())
 }
 
 func TestGetAppInstanceLabel(t *testing.T) {
@@ -204,7 +229,20 @@ func TestGetAppInstanceLabel(t *testing.T) {
 	assert.Nil(t, err)
 	err = SetAppInstanceLabel(&obj, common.LabelKeyAppInstance, "my-app")
 	assert.Nil(t, err)
-	assert.Equal(t, "my-app", GetAppInstanceLabel(&obj, common.LabelKeyAppInstance))
+	label, err := GetAppInstanceLabel(&obj, common.LabelKeyAppInstance)
+	assert.Nil(t, err)
+	assert.Equal(t, "my-app", label)
+}
+
+func TestGetAppInstanceLabelWithInvalidData(t *testing.T) {
+	yamlBytes, err := os.ReadFile("testdata/svc-with-invalid-data.yaml")
+	assert.Nil(t, err)
+	var obj unstructured.Unstructured
+	err = yaml.Unmarshal(yamlBytes, &obj)
+	assert.Nil(t, err)
+	_, err = GetAppInstanceLabel(&obj, "valid-label")
+	assert.Error(t, err)
+	assert.Equal(t, ".metadata.labels accessor error: contains non-string key in the map: <nil> is of the type <nil>, expected string", err.Error())
 }
 
 func TestRemoveLabel(t *testing.T) {
@@ -215,7 +253,20 @@ func TestRemoveLabel(t *testing.T) {
 	assert.Nil(t, err)
 	obj.SetLabels(map[string]string{"test": "value"})
 
-	RemoveLabel(&obj, "test")
+	err = RemoveLabel(&obj, "test")
+	assert.Nil(t, err)
 
 	assert.Nil(t, obj.GetLabels())
 }
+
+func TestRemoveLabelWithInvalidData(t *testing.T) {
+	yamlBytes, err := os.ReadFile("testdata/svc-with-invalid-data.yaml")
+	assert.Nil(t, err)
+	var obj unstructured.Unstructured
+	err = yaml.Unmarshal(yamlBytes, &obj)
+	assert.Nil(t, err)
+
+	err = RemoveLabel(&obj, "valid-label")
+	assert.Error(t, err)
+	assert.Equal(t, ".metadata.labels accessor error: contains non-string key in the map: <nil> is of the type <nil>, expected string", err.Error())
+}
diff --git a/util/kube/testdata/svc-with-invalid-data.yaml b/util/kube/testdata/svc-with-invalid-data.yaml
@@ -0,0 +1,17 @@
+kind: Service
+apiVersion: v1
+metadata:
+  name: my-service
+  annotations:
+    valid-annotation: existing-value
+    invalid-annotation: null
+  labels:
+    valid-label: existing-value
+    invalid-label: null
+spec:
+  selector:
+    app: MyApp
+  ports:
+  - protocol: TCP
+    port: 80
+    targetPort: 9376