Merge pull request #167 from fluxcd/ssa-drift

ssa: Improve drift detection

ssa/manager_diff.go

@@ -75,7 +75,7 @@ func (m *ResourceManager) Diff(ctx context.Context, object *unstructured.Unstruc
 	return m.changeSetEntry(dryRunObject, UnchangedAction), nil
 }
 
-// hasDrifted detects changes to metadata labels, metadata annotations, spec and webhooks.
+// hasDrifted detects changes to metadata labels, annotations and spec.
 func (m *ResourceManager) hasDrifted(existingObject, dryRunObject *unstructured.Unstructured) bool {
 	if dryRunObject.GetResourceVersion() == "" {
 		return true
@@ -90,32 +90,31 @@ func (m *ResourceManager) hasDrifted(existingObject, dryRunObject *unstructured.
 		return true
 	}
 
-	var found bool
-	for _, field := range []string{"spec", "webhooks", "rules", "subjects", "roleRef", "subsets", "data", "binaryData", "stringData", "immutable"} {
-		if _, ok := existingObject.Object[field]; ok {
-			found = true
-		}
-		if hasFieldDrifted(existingObject, dryRunObject, field) {
-			return true
-		}
-	}
-
-	if !found {
-		if !apiequality.Semantic.DeepDerivative(dryRunObject.Object, existingObject.Object) {
-			return true
-		}
-	}
-
-	return false
+	return hasObjectDrifted(dryRunObject, existingObject)
 }
 
-func hasFieldDrifted(existingObject, dryRunObject *unstructured.Unstructured, field string) bool {
+// hasFieldDrifted performs a semantic equality check of the specified field
+func hasFieldDrifted(dryRunObject, existingObject *unstructured.Unstructured, field string) bool {
 	if _, ok := existingObject.Object[field]; ok {
-		return !apiequality.Semantic.DeepDerivative(dryRunObject.Object[field], existingObject.Object[field])
+		return !apiequality.Semantic.DeepEqual(dryRunObject.Object[field], existingObject.Object[field])
 	}
 	return false
 }
 
+// hasObjectDrifted removes the metadata and status fields from both objects
+// then performs a semantic equality check of the remaining fields
+func hasObjectDrifted(dryRunObject, existingObject *unstructured.Unstructured) bool {
+	dryRunObj := dryRunObject.DeepCopy()
+	unstructured.RemoveNestedField(dryRunObj.Object, "metadata")
+	unstructured.RemoveNestedField(dryRunObj.Object, "status")
+
+	existingObj := existingObject.DeepCopy()
+	unstructured.RemoveNestedField(existingObj.Object, "metadata")
+	unstructured.RemoveNestedField(existingObj.Object, "status")
+
+	return !apiequality.Semantic.DeepEqual(dryRunObj.Object, existingObj.Object)
+}
+
 // validationError formats the given error and hides sensitive data
 // if the error was caused by an invalid Kubernetes secrets.
 func (m *ResourceManager) validationError(object *unstructured.Unstructured, err error) error {

ssa/manager_diff_test.go

@@ -115,3 +115,84 @@ func TestDiff(t *testing.T) {
 		}
 	})
 }
+
+func TestDiff_Removals(t *testing.T) {
+	timeout := 10 * time.Second
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+
+	id := generateName("diff")
+	objects, err := readManifest("testdata/test4.yaml", id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	SetNativeKindsDefaults(objects)
+
+	configMapName, configMap := getFirstObject(objects, "ConfigMap", id)
+
+	if _, err = manager.ApplyAllStaged(ctx, objects, false, timeout); err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("generates empty diff for unchanged object", func(t *testing.T) {
+		changeSetEntry, err := manager.Diff(ctx, configMap)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := cmp.Diff(configMapName, changeSetEntry.Subject); diff != "" {
+			t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+		}
+
+		if diff := cmp.Diff(string(UnchangedAction), changeSetEntry.Action); diff != "" {
+			t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+		}
+
+		if _, err = manager.ApplyAll(ctx, []*unstructured.Unstructured{configMap}, false); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("generates diff for added map entry", func(t *testing.T) {
+		newVal := "diff-test"
+		err = unstructured.SetNestedField(configMap.Object, newVal, "data", "token")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		changeSetEntry, err := manager.Diff(ctx, configMap)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := cmp.Diff(string(ConfiguredAction), changeSetEntry.Action); diff != "" {
+			t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+		}
+
+		if !strings.Contains(changeSetEntry.Diff, newVal) {
+			t.Errorf("Mismatch from expected value, want %s", newVal)
+		}
+
+		if _, err = manager.ApplyAll(ctx, []*unstructured.Unstructured{configMap}, false); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("generates diff for removed map entry", func(t *testing.T) {
+		unstructured.RemoveNestedField(configMap.Object, "data", "token")
+
+		changeSetEntry, err := manager.Diff(ctx, configMap)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := cmp.Diff(string(ConfiguredAction), changeSetEntry.Action); diff != "" {
+			t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+		}
+
+		if _, err = manager.ApplyAll(ctx, []*unstructured.Unstructured{configMap}, false); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+}

ssa/testdata/test3.yaml

@@ -19,8 +19,6 @@ metadata:
   creationTimestamp: null
   labels:
     app: some-operator
-data:
-
 ---
 apiVersion: v1
 kind: Secret

ssa/testdata/test4.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: "%[1]s"
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: "%[1]s"
+  namespace: "%[1]s"