Handle edge cases for unsanitized secrets in diff.HideSecretData

pkg/diff/diff.go

@@ -77,11 +77,17 @@
 	o := applyOptions(opts)
 	if config != nil {
 		config = remarshal(config, o)
-		Normalize(config, opts...)
+		err := Normalize(config, opts...)
+		if err != nil {
+			return nil, err
+		}
 	}
 	if live != nil {
 		live = remarshal(live, o)
-		Normalize(live, opts...)
+		err := Normalize(live, opts...)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	if o.serverSideDiff {
@@ -118,7 +124,10 @@
 		o.log.V(1).Info(fmt.Sprintf("Failed to get last applied configuration: %v", err))
 	} else {
 		if orig != nil && config != nil {
-			Normalize(orig, opts...)
+			err := Normalize(orig, opts...)
+			if err != nil {
+				return nil, err
+			}
 			dr, err := ThreeWayDiff(orig, config, live)
 			if err == nil {
 				return dr, nil
@@ -179,7 +188,7 @@
 		}
 	}
 
 	Normalize(predictedLive, opts...)
 	unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields")
 
 	predictedLiveBytes, err := json.Marshal(predictedLive)
@@ -823,9 +832,9 @@
 	return &diffResultList, nil
 }
 
-func Normalize(un *unstructured.Unstructured, opts ...Option) {
+func Normalize(un *unstructured.Unstructured, opts ...Option) error {
 	if un == nil {
-		return
+		return nil
 	}
 	o := applyOptions(opts)
 
@@ -835,7 +844,10 @@
 
 	gvk := un.GroupVersionKind()
 	if gvk.Group == "" && gvk.Kind == "Secret" {
-		NormalizeSecret(un, opts...)
+		err := NormalizeSecret(un)
+		if err != nil {
+			return err
+		}
 	} else if gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role") {
 		normalizeRole(un, o)
 	} else if gvk.Group == "" && gvk.Kind == "Endpoints" {
@@ -846,24 +858,31 @@
 	if err != nil {
 		o.log.Error(err, fmt.Sprintf("Failed to normalize %s/%s/%s", un.GroupVersionKind(), un.GetNamespace(), un.GetName()))
 	}
+
+	return nil
 }
 
 // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to
-// empty strings. If the object is not a secret, or is an invalid secret, then returns the same object.
-func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
+// empty strings. Invalid secrets return appropriate errors. If the object is not a secret, or is
+// an invalid secret, then returns the same object.
+func NormalizeSecret(un *unstructured.Unstructured) error {
 	if un == nil {
-		return
+		return nil
 	}
 	gvk := un.GroupVersionKind()
 	if gvk.Group != "" || gvk.Kind != "Secret" {
-		return
+		return nil
+	}
+	if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil {
+		err := stringifyNestedIntKeys(un, stringData)
+		if err != nil {
+			return err
+		}
 	}
-	o := applyOptions(opts)
 	var secret corev1.Secret
 	err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret)
 	if err != nil {
-		o.log.Error(err, "Failed to convert from unstructured into Secret")
-		return
+		return fmt.Errorf("Failed to convert from unstructured into Secret: %s", err)
 	}
 	// We normalize nils to empty string to handle: https://github.com/argoproj/argo-cd/issues/943
 	for k, v := range secret.Data {
@@ -882,16 +901,16 @@
 	}
 	newObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&secret)
 	if err != nil {
-		o.log.Error(err, "object unable to convert from secret")
-		return
+		return fmt.Errorf("Object unable to convert from Secret: %s", err)
 	}
 	if secret.Data != nil {
 		err = unstructured.SetNestedField(un.Object, newObj["data"], "data")
 		if err != nil {
-			o.log.Error(err, "failed to set secret.data")
-			return
+			return fmt.Errorf("Failed to set Secret data: %s", err)
 		}
 	}
+
+	return nil
 }
 
 // normalizeEndpoint normalizes endpoint meaning that EndpointSubsets are sorted lexicographically
@@ -1003,7 +1022,10 @@
 		if obj == nil {
 			continue
 		}
-		NormalizeSecret(obj)
+		err := NormalizeSecret(obj)
+		if err != nil {
+			return nil, nil, err
+		}
 		if data, found, err := unstructured.NestedMap(obj.Object, "data"); found && err == nil {
 			for k := range data {
 				keys[k] = true
@@ -1114,3 +1136,18 @@
 	unstrBody = jsonutil.RemoveMapFields(obj.Object, unstrBody)
 	return &unstructured.Unstructured{Object: unstrBody}
 }
+
+func stringifyNestedIntKeys(un *unstructured.Unstructured, stringData map[string]interface{}) error {
+	for k, v := range stringData {
+		switch v := v.(type) {
+		case int64:
+			stringData[k] = toString(v)
+		}
+	}
+	err := unstructured.SetNestedField(un.Object, stringData, "stringData")
+	if err != nil {
+		return fmt.Errorf("unstructured.SetNestedField error: %s", err)
+	} else {
+		return nil
+	}
+}

pkg/diff/diff_test.go

@@ -983,6 +983,24 @@ var (
 	replacement3 = strings.Repeat("+", 16)
 )
 
+
+func TestHideSecretStringDataInvalidValues(t *testing.T) {
+	var err error
+	var configUn unstructured.Unstructured
+	err = yaml.Unmarshal([]byte(secretInvalidConfig), &configUn)
+	require.NoError(t, err)
+
+	var liveUn unstructured.Unstructured
+	err = yaml.Unmarshal([]byte(secretInvalidLive), &liveUn)
+	require.NoError(t, err)
+
+	target, live, err := HideSecretData(&configUn, &liveUn)
+	require.NoError(t, err)
+
+	assert.Equal(t, map[string]interface{}{"foo": replacement1}, secretData(target))
+	assert.Equal(t, map[string]interface{}{"foo": replacement2}, secretData(live))
+}
+
 func TestHideSecretDataSameKeysDifferentValues(t *testing.T) {
 	target, live, err := HideSecretData(
 		createSecret(map[string]string{"key1": "test", "key2": "test"}),
@@ -1055,7 +1073,7 @@ func getLiveSecretJsonBytes() []byte {
     },
     "data": {
         "namespace": "ZGVmYXVsdA==",
-        "token": "ZGVmYXVsdAcb=="
+        "token": "ZGVmYXVsdA=="
     }
 }`)
 }