Skip to content

Commit 3791ebc

Browse files
authored
Remarshal manifest to decrease false-positive (#3370)
**What this PR does / why we need it**: To reduce wrong diff result, this PR introduce `remarshal` function that marshal the object to struct that defined by k8s, and then return it to our object. This function is copied from [argoproj/gitops-engin](https://github.com/argoproj/gitops-engine/) and modified. **Which issue(s) this PR fixes**: Fixes #3334 **Does this PR introduce a user-facing change?**: <!-- If no, just write "NONE" in the release-note block below. --> ```release-note NONE ``` This PR was merged by Kapetanios.
1 parent 6d85d5e commit 3791ebc

File tree

11 files changed

+439
-11
lines changed

11 files changed

+439
-11
lines changed

NOTICE.txt

+5
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,8 @@ This product contains a modified part of perf, distributed by golang:
2020

2121
* License: https://github.com/golang/perf/blob/master/LICENSE
2222
* Homepage: https://github.com/golang/perf
23+
24+
This product contains a modified part of gitops-engine, distributed by argoproj:
25+
26+
* License: https://github.com/argoproj/gitops-engine/blob/master/LICENSE (Apache License v2.0)
27+
* Homepage: https://argo-cd.readthedocs.io/

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

+2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ go_library(
66
"cache.go",
77
"deployment.go",
88
"diff.go",
9+
"diffutil.go",
910
"hasher.go",
1011
"helm.go",
1112
"kubectl.go",
@@ -47,6 +48,7 @@ go_test(
4748
srcs = [
4849
"deployment_test.go",
4950
"diff_test.go",
51+
"diffutil_test.go",
5052
"hasher_test.go",
5153
"helm_test.go",
5254
"kubernetes_test.go",

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

+12-4
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"sort"
2323
"strings"
2424

25+
"go.uber.org/zap"
2526
v1 "k8s.io/api/core/v1"
2627
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2728
"k8s.io/apimachinery/pkg/runtime"
@@ -49,18 +50,25 @@ type DiffListChange struct {
4950
Diff *diff.Result
5051
}
5152

52-
func Diff(old, new Manifest, opts ...diff.Option) (*diff.Result, error) {
53+
func Diff(old, new Manifest, logger *zap.Logger, opts ...diff.Option) (*diff.Result, error) {
5354
if old.Key.IsSecret() && new.Key.IsSecret() {
5455
var err error
5556
new.u, err = normalizeNewSecret(old.u, new.u)
5657
if err != nil {
5758
return nil, err
5859
}
5960
}
60-
return diff.DiffUnstructureds(*old.u, *new.u, opts...)
61+
62+
normalized, err := remarshal(old.u)
63+
if err != nil {
64+
logger.Info("Unable to remarshal Kubernetes manifest, the raw data will be used to calculate the diff", zap.Error(err))
65+
return diff.DiffUnstructureds(*old.u, *new.u, opts...)
66+
}
67+
68+
return diff.DiffUnstructureds(*normalized, *new.u, opts...)
6169
}
6270

63-
func DiffList(olds, news []Manifest, opts ...diff.Option) (*DiffListResult, error) {
71+
func DiffList(olds, news []Manifest, logger *zap.Logger, opts ...diff.Option) (*DiffListResult, error) {
6472
adds, deletes, newChanges, oldChanges := groupManifests(olds, news)
6573
cr := &DiffListResult{
6674
Adds: adds,
@@ -69,7 +77,7 @@ func DiffList(olds, news []Manifest, opts ...diff.Option) (*DiffListResult, erro
6977
}
7078

7179
for i := 0; i < len(newChanges); i++ {
72-
result, err := Diff(oldChanges[i], newChanges[i], opts...)
80+
result, err := Diff(oldChanges[i], newChanges[i], logger, opts...)
7381
if err != nil {
7482
return nil, err
7583
}

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

+72-1
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919

2020
"github.com/stretchr/testify/assert"
2121
"github.com/stretchr/testify/require"
22+
"go.uber.org/zap"
2223

2324
"github.com/pipe-cd/pipecd/pkg/diff"
2425
)
@@ -310,6 +311,76 @@ data:
310311
`,
311312
diffNum: 1,
312313
},
314+
{
315+
name: "Pod no diff 1",
316+
manifests: `apiVersion: v1
317+
kind: Pod
318+
metadata:
319+
name: static-web
320+
labels:
321+
role: myrole
322+
spec:
323+
containers:
324+
- name: web
325+
image: nginx
326+
ports:
327+
resources:
328+
limits:
329+
memory: "2Gi"
330+
---
331+
apiVersion: v1
332+
kind: Pod
333+
metadata:
334+
name: static-web
335+
labels:
336+
role: myrole
337+
spec:
338+
containers:
339+
- name: web
340+
image: nginx
341+
resources:
342+
limits:
343+
memory: "2Gi"
344+
`,
345+
expected: "",
346+
diffNum: 0,
347+
falsePositive: false,
348+
},
349+
{
350+
name: "Pod no diff 2",
351+
manifests: `apiVersion: v1
352+
kind: Pod
353+
metadata:
354+
name: static-web
355+
labels:
356+
role: myrole
357+
spec:
358+
containers:
359+
- name: web
360+
image: nginx
361+
ports:
362+
resources:
363+
limits:
364+
memory: "1.5Gi"
365+
---
366+
apiVersion: v1
367+
kind: Pod
368+
metadata:
369+
name: static-web
370+
labels:
371+
role: myrole
372+
spec:
373+
containers:
374+
- name: web
375+
image: nginx
376+
resources:
377+
limits:
378+
memory: "1536Mi"
379+
`,
380+
expected: "",
381+
diffNum: 0,
382+
falsePositive: false,
383+
},
313384
}
314385

315386
for _, tc := range testcases {
@@ -319,7 +390,7 @@ data:
319390
require.Equal(t, 2, len(manifests))
320391
old, new := manifests[0], manifests[1]
321392

322-
result, err := Diff(old, new, diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString())
393+
result, err := Diff(old, new, zap.NewNop(), diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString())
323394
require.NoError(t, err)
324395

325396
renderer := diff.NewRenderer(diff.WithLeftPadding(1))
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
// Copyright 2022 The PipeCD Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package kubernetes
16+
17+
import (
18+
"bytes"
19+
"encoding/json"
20+
"reflect"
21+
22+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
23+
"k8s.io/apimachinery/pkg/runtime"
24+
"k8s.io/client-go/kubernetes/scheme"
25+
)
26+
27+
// All functions in this file is borrowed from argocd/gitops-engine and modified
28+
// All function except `remarshal` is borrowed from
29+
// https://github.com/argoproj/gitops-engine/blob/0bc2f8c395f67123156d4ce6b667bf730618307f/pkg/utils/json/json.go
30+
// and `remarshal` function is borrowed from
31+
// https://github.com/argoproj/gitops-engine/blob/b0c5e00ccfa5d1e73087a18dc59e2e4c72f5f175/pkg/diff/diff.go#L685-L723
32+
33+
// https://github.com/ksonnet/ksonnet/blob/master/pkg/kubecfg/diff.go
34+
func removeFields(config, live interface{}) interface{} {
35+
switch c := config.(type) {
36+
case map[string]interface{}:
37+
l, ok := live.(map[string]interface{})
38+
if ok {
39+
return removeMapFields(c, l)
40+
}
41+
return live
42+
case []interface{}:
43+
l, ok := live.([]interface{})
44+
if ok {
45+
return removeListFields(c, l)
46+
}
47+
return live
48+
default:
49+
return live
50+
}
51+
52+
}
53+
54+
// removeMapFields remove all non-existent fields in the live that don't exist in the config
55+
func removeMapFields(config, live map[string]interface{}) map[string]interface{} {
56+
result := map[string]interface{}{}
57+
for k, v1 := range config {
58+
v2, ok := live[k]
59+
if !ok {
60+
continue
61+
}
62+
if v2 != nil {
63+
v2 = removeFields(v1, v2)
64+
}
65+
result[k] = v2
66+
}
67+
return result
68+
}
69+
70+
func removeListFields(config, live []interface{}) []interface{} {
71+
// If live is longer than config, then the extra elements at the end of the
72+
// list will be returned as-is so they appear in the diff.
73+
result := make([]interface{}, 0, len(live))
74+
for i, v2 := range live {
75+
if len(config) > i {
76+
if v2 != nil {
77+
v2 = removeFields(config[i], v2)
78+
}
79+
result = append(result, v2)
80+
} else {
81+
result = append(result, v2)
82+
}
83+
}
84+
return result
85+
}
86+
87+
// remarshal checks resource kind and version and re-marshal using corresponding struct custom marshaller.
88+
// This ensures that expected resource state is formatter same as actual resource state in kubernetes
89+
// and allows to find differences between actual and target states more accurately.
90+
// Remarshalling also strips any type information (e.g. float64 vs. int) from the unstructured
91+
// object. This is important for diffing since it will cause godiff to report a false difference.
92+
func remarshal(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
93+
data, err := json.Marshal(obj)
94+
if err != nil {
95+
return nil, err
96+
}
97+
item, err := scheme.Scheme.New(obj.GroupVersionKind())
98+
if err != nil {
99+
// This is common. the scheme is not registered
100+
return nil, err
101+
}
102+
// This will drop any omitempty fields, perform resource conversion etc...
103+
unmarshalledObj := reflect.New(reflect.TypeOf(item).Elem()).Interface()
104+
// Unmarshal data into unmarshalledObj, but detect if there are any unknown fields that are not
105+
// found in the target GVK object.
106+
decoder := json.NewDecoder(bytes.NewReader(data))
107+
decoder.DisallowUnknownFields()
108+
if err := decoder.Decode(&unmarshalledObj); err != nil {
109+
// Likely a field present in obj that is not present in the GVK type, or user
110+
// may have specified an invalid spec in git, so return original object
111+
return nil, err
112+
}
113+
unstrBody, err := runtime.DefaultUnstructuredConverter.ToUnstructured(unmarshalledObj)
114+
if err != nil {
115+
return nil, err
116+
}
117+
// Remove all default values specified by custom formatter (e.g. creationTimestamp)
118+
unstrBody = removeMapFields(obj.Object, unstrBody)
119+
return &unstructured.Unstructured{Object: unstrBody}, nil
120+
}

0 commit comments

Comments
 (0)