Fixed a bug in #4199 that resulted in wrong Out of Sync. (#4248)

* Fixed a bug in #4199 that resulted in wrong Out of Sync.

* Apply cloud drift detector

* Fix diff detail's header

* Revert diff_test

* Fix ignoreAddingMapKeys process

* Fix ignoreAddingMapKeysTest

* Add print debug

* Fix test

* Delete print debug

* Update comment

pkg/app/piped/driftdetector/cloudrun/detector.go

@@ -207,8 +207,8 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application,
 	d.logger.Info(fmt.Sprintf("application %s has a live service manifest", app.Id))
 
 	result, err := provider.Diff(
-		headManifest,
 		liveManifest,
+		headManifest,
 		diff.WithEquateEmpty(),
 		diff.WithIgnoreAddingMapKeys(),
 		diff.WithCompareNumberAndNumericString(),
@@ -308,7 +308,7 @@ func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncS
 
 	var b strings.Builder
 	b.WriteString(fmt.Sprintf("Diff between the defined state in Git at commit %s and actual live state:\n\n", commit))
-	b.WriteString("--- Expected\n+++ Actual\n\n")
+	b.WriteString("--- Actual   (LiveState)\n+++ Expected (Git)\n\n")
 
 	details := r.Render(provider.DiffRenderOptions{
 		// Currently, we do not use the diff command to render the result

pkg/app/piped/driftdetector/kubernetes/detector.go

@@ -199,8 +199,8 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application,
 	}
 
 	result, err := provider.DiffList(
-		headManifests,
 		liveManifests,
+		headManifests,
 		d.logger,
 		diff.WithEquateEmpty(),
 		diff.WithIgnoreAddingMapKeys(),
@@ -361,7 +361,7 @@ func makeSyncState(r *provider.DiffListResult, commit string) model.ApplicationS
 
 	var b strings.Builder
 	b.WriteString(fmt.Sprintf("Diff between the defined state in Git at commit %s and actual state in cluster:\n\n", commit))
-	b.WriteString("--- Expected\n+++ Actual\n\n")
+	b.WriteString("--- Actual   (LiveState)\n+++ Expected (Git)\n\n")
 
 	details := r.Render(provider.DiffRenderOptions{
 		MaskSecret:          true,

pkg/app/piped/driftdetector/terraform/detector.go

@@ -281,7 +281,7 @@ func makeSyncState(r provider.PlanResult, commit string) model.ApplicationSyncSt
 
 	var b strings.Builder
 	b.WriteString(fmt.Sprintf("Diff between the defined state in Git at commit %s and actual live state:\n\n", commit))
-	b.WriteString("+++ Expected (Git)\n--- Actual   (LiveState)\n\n")
+	b.WriteString("--- Actual   (LiveState)\n+++ Expected (Git)\n\n")
 
 	details := r.Render()
 	b.WriteString(details)

pkg/app/piped/platformprovider/kubernetes/diff.go

@@ -53,19 +53,19 @@ type DiffListChange struct {
 func Diff(old, new Manifest, logger *zap.Logger, opts ...diff.Option) (*diff.Result, error) {
 	if old.Key.IsSecret() && new.Key.IsSecret() {
 		var err error
-		new.u, err = normalizeNewSecret(old.u, new.u)
+		old.u, err = normalizeNewSecret(old.u, new.u)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	normalized, err := remarshal(old.u)
+	normalized, err := remarshal(new.u)
 	if err != nil {
 		logger.Info("compare manifests directly since it was unable to remarshal Kubernetes manifest to normalize special fields", zap.Error(err))
 		return diff.DiffUnstructureds(*old.u, *new.u, opts...)
 	}
 
-	return diff.DiffUnstructureds(*normalized, *new.u, opts...)
+	return diff.DiffUnstructureds(*old.u, *normalized, opts...)
 }
 
 func DiffList(olds, news []Manifest, logger *zap.Logger, opts ...diff.Option) (*DiffListResult, error) {
@@ -99,32 +99,32 @@ func normalizeNewSecret(old, new *unstructured.Unstructured) (*unstructured.Unst
 	runtime.DefaultUnstructuredConverter.FromUnstructured(old.Object, &o)
 	runtime.DefaultUnstructuredConverter.FromUnstructured(new.Object, &n)
 
-	// Move as much as possible fields from `n.Data` to `n.StringData` to make `n` close to `o` to minimize the diff.
-	for k, v := range n.Data {
+	// Move as much as possible fields from `o.Data` to `o.StringData` to make `o` close to `n` to minimize the diff.
+	for k, v := range o.Data {
 		// Skip if the field also exists in StringData.
-		if _, ok := n.StringData[k]; ok {
+		if _, ok := o.StringData[k]; ok {
 			continue
 		}
 
-		if _, ok := o.StringData[k]; !ok {
+		if _, ok := n.StringData[k]; !ok {
 			continue
 		}
 
-		if n.StringData == nil {
-			n.StringData = make(map[string]string)
+		if o.StringData == nil {
+			o.StringData = make(map[string]string)
 		}
 
-		// If the field is existing in `o.StringData`, we should move that field from `n.Data` to `n.StringData`
-		n.StringData[k] = string(v)
-		delete(n.Data, k)
+		// If the field is existing in `n.StringData`, we should move that field from `o.Data` to `o.StringData`
+		o.StringData[k] = string(v)
+		delete(o.Data, k)
 	}
 
-	newObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&n)
+	newO, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&o)
 	if err != nil {
 		return nil, err
 	}
 
-	return &unstructured.Unstructured{Object: newObj}, nil
+	return &unstructured.Unstructured{Object: newO}, nil
 }
 
 type DiffRenderOptions struct {

pkg/app/piped/platformprovider/kubernetes/diff_test.go

@@ -248,16 +248,16 @@ metadata:
   name: secret-management
 data:
   password: hoge
-stringData:
-  foo: bar
+  foo: YmFy
 ---
 apiVersion: apps/v1
 kind: Secret
 metadata:
   name: secret-management
 data:
   password: hoge
-  foo: YmFy
+stringData:
+  foo: bar
 `,
 			expected: "",
 			diffNum:  0,
@@ -270,17 +270,17 @@ metadata:
   name: secret-management
 data:
   password: hoge
-  foo: Zm9v
-stringData:
-  foo: bar
+  foo: YmFy
 ---
 apiVersion: apps/v1
 kind: Secret
 metadata:
   name: secret-management
 data:
   password: hoge
-  foo: YmFy
+  foo: Zm9v
+stringData:
+  foo: bar
 `,
 			expected:      "",
 			diffNum:       0,
@@ -293,20 +293,20 @@ kind: Secret
 metadata:
   name: secret-management
 data:
-  password: hoge
-stringData:
-  foo: bar
+  foo: YmFy
 ---
 apiVersion: apps/v1
 kind: Secret
 metadata:
   name: secret-management
 data:
-  foo: YmFy
+  password: hoge
+stringData:
+  foo: bar
 `,
 			expected: `  #data
-- data:
--   password: hoge
++ data:
++   password: hoge
 
 `,
 			diffNum: 1,
@@ -323,7 +323,6 @@ spec:
   containers:
     - name: web
       image: nginx
-      ports:
       resources:
         limits:
           memory: "2Gi"
@@ -338,6 +337,7 @@ spec:
   containers:
     - name: web
       image: nginx
+      ports:
       resources:
         limits:
           memory: "2Gi"
@@ -358,10 +358,9 @@ spec:
   containers:
     - name: web
       image: nginx
-      ports:
       resources:
         limits:
-          memory: "1.5Gi"
+          memory: "1536Mi"
 ---
 apiVersion: v1
 kind: Pod
@@ -373,9 +372,10 @@ spec:
   containers:
     - name: web
       image: nginx
+      ports:
       resources:
         limits:
-          memory: "1536Mi"
+          memory: "1.5Gi"
 `,
 			expected:      "",
 			diffNum:       0,

pkg/diff/diff.go

@@ -200,15 +200,15 @@ func (d *differ) diffMap(path []PathStep, vx, vy reflect.Value) error {
 			continue
 		}
 
-		nextValueX := vx.MapIndex(k)
-		// Don't need to check the key existing in the second one but missing in the first one
+		nextValueY := vy.MapIndex(k)
+		// Don't need to check the key existing in the first(LiveManifest) one but missing in the seccond(GitManifest) one
 		// when IgnoreAddingMapKeys is configured.
-		if d.ignoreAddingMapKeys && !nextValueX.IsValid() {
+		if d.ignoreAddingMapKeys && !nextValueY.IsValid() {
 			continue
 		}
 
 		nextPath := newMapPath(path, k.String())
-		nextValueY := vy.MapIndex(k)
+		nextValueX := vx.MapIndex(k)
 		checks[k.String()] = struct{}{}
 		if err := d.diff(nextPath, nextValueX, nextValueY); err != nil {
 			return err

pkg/diff/testdata/ignore_adding_map_keys.yaml

@@ -3,8 +3,10 @@ kind: Foo
 metadata:
   name: simple
   labels:
-    pipecd.dev/managed-by: piped
     app: simple
+    pipecd.dev/managed-by: piped
+    pipecd.dev/resource-key: apps/v1:Foo:default:simple
+    pipecd.dev/variant: primary
 spec:
   replicas: 2
   selector:
@@ -15,6 +17,9 @@ spec:
       labels:
         app: simple
     spec:
+      newSliceFields:
+        - a
+        - b
       containers:
       - name: helloworld
         image: gcr.io/pipecd/helloworld:v1.0.0
@@ -23,16 +28,16 @@ spec:
           - hello
         ports:
         - containerPort: 9085
+status:
+    desc: ok
 ---
 apiVersion: apps/v1
 kind: Foo
 metadata:
   name: simple
   labels:
-    app: simple
     pipecd.dev/managed-by: piped
-    pipecd.dev/resource-key: apps/v1:Foo:default:simple
-    pipecd.dev/variant: primary
+    app: simple
 spec:
   replicas: 2
   selector:
@@ -43,9 +48,6 @@ spec:
       labels:
         app: simple
     spec:
-      newSliceFields:
-        - a
-        - b
       containers:
       - name: helloworld
         image: gcr.io/pipecd/helloworld:v1.0.0
@@ -54,5 +56,3 @@ spec:
           - hello
         ports:
         - containerPort: 9085
-status:
-    desc: ok