Remarshal manifest to decrease false-positive

NOTICE.txt

@@ -20,3 +20,8 @@ This product contains a modified part of perf, distributed by golang:
 
   * License: https://github.com/golang/perf/blob/master/LICENSE
   * Homepage: https://github.com/golang/perf
+
+This product contains a modified part of gitops-engine, distributed by argoproj:
+
+  * License: https://github.com/argoproj/gitops-engine/blob/master/LICENSE (Apache License v2.0)
+  * Homepage: https://argo-cd.readthedocs.io/

pkg/app/piped/cloudprovider/kubernetes/BUILD.bazel

@@ -6,6 +6,7 @@ go_library(
         "cache.go",
         "deployment.go",
         "diff.go",
+        "diffutil.go",
         "hasher.go",
         "helm.go",
         "kubectl.go",
@@ -47,6 +48,7 @@ go_test(
     srcs = [
         "deployment_test.go",
         "diff_test.go",
+        "diffutil_test.go",
         "hasher_test.go",
         "helm_test.go",
         "kubernetes_test.go",

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

@@ -22,6 +22,7 @@ import (
 	"sort"
 	"strings"
 
+	"go.uber.org/zap"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -49,18 +50,25 @@ type DiffListChange struct {
 	Diff *diff.Result
 }
 
-func Diff(old, new Manifest, opts ...diff.Option) (*diff.Result, error) {
+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)
 		if err != nil {
 			return nil, err
 		}
 	}
-	return diff.DiffUnstructureds(*old.u, *new.u, opts...)
+
+	normalized, err := remarshal(old.u)
+	if err != nil {
+		logger.Info("Unable to remarshal Kubernetes manifest, the raw data will be used to calculate the diff", zap.Error(err))
+		return diff.DiffUnstructureds(*old.u, *new.u, opts...)
+	}
+
+	return diff.DiffUnstructureds(*normalized, *new.u, opts...)
 }
 
-func DiffList(olds, news []Manifest, opts ...diff.Option) (*DiffListResult, error) {
+func DiffList(olds, news []Manifest, logger *zap.Logger, opts ...diff.Option) (*DiffListResult, error) {
 	adds, deletes, newChanges, oldChanges := groupManifests(olds, news)
 	cr := &DiffListResult{
 		Adds:    adds,
@@ -69,7 +77,7 @@ func DiffList(olds, news []Manifest, opts ...diff.Option) (*DiffListResult, erro
 	}
 
 	for i := 0; i < len(newChanges); i++ {
-		result, err := Diff(oldChanges[i], newChanges[i], opts...)
+		result, err := Diff(oldChanges[i], newChanges[i], logger, opts...)
 		if err != nil {
 			return nil, err
 		}

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

@@ -19,11 +19,14 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
 
 	"github.com/pipe-cd/pipecd/pkg/diff"
 )
 
 func TestGroupManifests(t *testing.T) {
+	t.Parallel()
+
 	testcases := []struct {
 		name               string
 		olds               []Manifest
@@ -118,6 +121,8 @@ func TestGroupManifests(t *testing.T) {
 }
 
 func TestDiffByCommand(t *testing.T) {
+	t.Parallel()
+
 	testcases := []struct {
 		name        string
 		command     string
@@ -188,6 +193,8 @@ func TestDiffByCommand(t *testing.T) {
 }
 
 func TestDiff(t *testing.T) {
+	t.Parallel()
+
 	testcases := []struct {
 		name          string
 		manifests     string
@@ -304,6 +311,76 @@ data:
 `,
 			diffNum: 1,
 		},
+		{
+			name: "Pod no diff 1",
+			manifests: `apiVersion: v1
+kind: Pod
+metadata:
+  name: static-web
+  labels:
+    role: myrole
+spec:
+  containers:
+    - name: web
+      image: nginx
+      ports:
+      resources:
+        limits:
+          memory: "2Gi"
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: static-web
+  labels:
+    role: myrole
+spec:
+  containers:
+    - name: web
+      image: nginx
+      resources:
+        limits:
+          memory: "2Gi"
+`,
+			expected:      "",
+			diffNum:       0,
+			falsePositive: false,
+		},
+		{
+			name: "Pod no diff 2",
+			manifests: `apiVersion: v1
+kind: Pod
+metadata:
+  name: static-web
+  labels:
+    role: myrole
+spec:
+  containers:
+    - name: web
+      image: nginx
+      ports:
+      resources:
+        limits:
+          memory: "1.5Gi"
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: static-web
+  labels:
+    role: myrole
+spec:
+  containers:
+    - name: web
+      image: nginx
+      resources:
+        limits:
+          memory: "1536Mi"
+`,
+			expected:      "",
+			diffNum:       0,
+			falsePositive: false,
+		},
 	}
 
 	for _, tc := range testcases {
@@ -313,7 +390,7 @@ data:
 			require.Equal(t, 2, len(manifests))
 			old, new := manifests[0], manifests[1]
 
-			result, err := Diff(old, new, diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString())
+			result, err := Diff(old, new, zap.NewNop(), diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString())
 			require.NoError(t, err)
 
 			renderer := diff.NewRenderer(diff.WithLeftPadding(1))

pkg/app/piped/cloudprovider/kubernetes/diffutil.go

@@ -0,0 +1,120 @@
+// Copyright 2022 The PipeCD Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package kubernetes
+
+import (
+	"bytes"
+	"encoding/json"
+	"reflect"
+
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
+)
+
+// All functions in this file is borrowed from argocd/gitops-engine and modified
+// All function except `remarshal` is borrowed from
+// https://github.com/argoproj/gitops-engine/blob/0bc2f8c395f67123156d4ce6b667bf730618307f/pkg/utils/json/json.go
+// and `remarshal` function is borrowed from
+// https://github.com/argoproj/gitops-engine/blob/b0c5e00ccfa5d1e73087a18dc59e2e4c72f5f175/pkg/diff/diff.go#L685-L723
+
+// https://github.com/ksonnet/ksonnet/blob/master/pkg/kubecfg/diff.go
+func removeFields(config, live interface{}) interface{} {
+	switch c := config.(type) {
+	case map[string]interface{}:
+		l, ok := live.(map[string]interface{})
+		if ok {
+			return removeMapFields(c, l)
+		}
+		return live
+	case []interface{}:
+		l, ok := live.([]interface{})
+		if ok {
+			return removeListFields(c, l)
+		}
+		return live
+	default:
+		return live
+	}
+
+}
+
+// removeMapFields remove all non-existent fields in the live that don't exist in the config
+func removeMapFields(config, live map[string]interface{}) map[string]interface{} {
+	result := map[string]interface{}{}
+	for k, v1 := range config {
+		v2, ok := live[k]
+		if !ok {
+			continue
+		}
+		if v2 != nil {
+			v2 = removeFields(v1, v2)
+		}
+		result[k] = v2
+	}
+	return result
+}
+
+func removeListFields(config, live []interface{}) []interface{} {
+	// If live is longer than config, then the extra elements at the end of the
+	// list will be returned as-is so they appear in the diff.
+	result := make([]interface{}, 0, len(live))
+	for i, v2 := range live {
+		if len(config) > i {
+			if v2 != nil {
+				v2 = removeFields(config[i], v2)
+			}
+			result = append(result, v2)
+		} else {
+			result = append(result, v2)
+		}
+	}
+	return result
+}
+
+// remarshal checks resource kind and version and re-marshal using corresponding struct custom marshaller.
+// This ensures that expected resource state is formatter same as actual resource state in kubernetes
+// and allows to find differences between actual and target states more accurately.
+// Remarshalling also strips any type information (e.g. float64 vs. int) from the unstructured
+// object. This is important for diffing since it will cause godiff to report a false difference.
+func remarshal(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	data, err := json.Marshal(obj)
+	if err != nil {
+		return nil, err
+	}
+	item, err := scheme.Scheme.New(obj.GroupVersionKind())
+	if err != nil {
+		// This is common. the scheme is not registered
+		return nil, err
+	}
+	// This will drop any omitempty fields, perform resource conversion etc...
+	unmarshalledObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
+	// Unmarshal data into unmarshalledObj, but detect if there are any unknown fields that are not
+	// found in the target GVK object.
+	decoder := json.NewDecoder(bytes.NewReader(data))
+	decoder.DisallowUnknownFields()
+	if err := decoder.Decode(&unmarshalledObj); err != nil {
+		// Likely a field present in obj that is not present in the GVK type, or user
+		// may have specified an invalid spec in git, so return original object
+		return nil, err
+	}
+	unstrBody, err := runtime.DefaultUnstructuredConverter.ToUnstructured(unmarshalledObj)
+	if err != nil {
+		return nil, err
+	}
+	// Remove all default values specified by custom formatter (e.g. creationTimestamp)
+	unstrBody = removeMapFields(obj.Object, unstrBody)
+	return &unstructured.Unstructured{Object: unstrBody}, nil
+}