diff --git a/NOTICE.txt b/NOTICE.txt index 9966dbbdbd..4bc4d196f9 100644 --- a/NOTICE.txt +++ b/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/ diff --git a/pkg/app/piped/cloudprovider/kubernetes/BUILD.bazel b/pkg/app/piped/cloudprovider/kubernetes/BUILD.bazel index 0d3b3d66e4..6fa65d3729 100644 --- a/pkg/app/piped/cloudprovider/kubernetes/BUILD.bazel +++ b/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", diff --git a/pkg/app/piped/cloudprovider/kubernetes/diff.go b/pkg/app/piped/cloudprovider/kubernetes/diff.go index b9dee40a82..c77865468a 100644 --- a/pkg/app/piped/cloudprovider/kubernetes/diff.go +++ b/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,7 +50,7 @@ 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) @@ -57,10 +58,17 @@ func Diff(old, new Manifest, opts ...diff.Option) (*diff.Result, error) { 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 } diff --git a/pkg/app/piped/cloudprovider/kubernetes/diff_test.go b/pkg/app/piped/cloudprovider/kubernetes/diff_test.go index db4943e812..a2f624c4eb 100644 --- a/pkg/app/piped/cloudprovider/kubernetes/diff_test.go +++ b/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)) diff --git a/pkg/app/piped/cloudprovider/kubernetes/diffutil.go b/pkg/app/piped/cloudprovider/kubernetes/diffutil.go new file mode 100644 index 0000000000..b4d5351680 --- /dev/null +++ b/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 +} diff --git a/pkg/app/piped/cloudprovider/kubernetes/diffutil_test.go b/pkg/app/piped/cloudprovider/kubernetes/diffutil_test.go new file mode 100644 index 0000000000..929a29ff6d --- /dev/null +++ b/pkg/app/piped/cloudprovider/kubernetes/diffutil_test.go @@ -0,0 +1,218 @@ +// 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 ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRemoveMapFields(t *testing.T) { + t.Parallel() + + testcases := []struct { + name string + config map[string]interface{} + live map[string]interface{} + expected map[string]interface{} + }{ + { + name: "Empty map", + config: make(map[string]interface{}, 0), + live: make(map[string]interface{}, 0), + expected: make(map[string]interface{}, 0), + }, + { + name: "Not nested 1", + config: map[string]interface{}{ + "key a": "value a", + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": "value b", + }, + expected: map[string]interface{}{ + "key a": "value a", + }, + }, + { + name: "Not nested 2", + config: map[string]interface{}{ + "key a": "value a", + "key b": "value b", + }, + live: map[string]interface{}{ + "key a": "value a", + }, + expected: map[string]interface{}{ + "key a": "value a", + }, + }, + { + name: "Nested live deleted", + config: map[string]interface{}{ + "key a": "value a", + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + }, + }, + { + name: "Nested same", + config: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + }, + { + name: "Nested nested live deleted", + config: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + "nested key b": "nested value b", + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": map[string]interface{}{ + "nested key a": "nested value a", + }, + }, + }, + { + name: "Nested array", + config: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + }, + { + name: "Nested array 2", + config: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, 4, + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + }, + { + name: "Nested array remain", + config: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", map[string]interface{}{ + "aa": "aa", + }, + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", map[string]interface{}{ + "aa": "aa", + }, + }, + }, + }, + { + name: "Nested array same", + config: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "a", "b", 3, + }, + }, + live: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "b", "a", 3, + }, + }, + expected: map[string]interface{}{ + "key a": "value a", + "key b": []interface{}{ + "b", "a", 3, + }, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + removed := removeMapFields(tc.config, tc.live) + assert.Equal(t, tc.expected, removed) + }) + } +} diff --git a/pkg/app/piped/driftdetector/kubernetes/detector.go b/pkg/app/piped/driftdetector/kubernetes/detector.go index 83174482fc..a77e8e04bb 100644 --- a/pkg/app/piped/driftdetector/kubernetes/detector.go +++ b/pkg/app/piped/driftdetector/kubernetes/detector.go @@ -191,6 +191,7 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application, result, err := provider.DiffList( headManifests, liveManifests, + d.logger, diff.WithEquateEmpty(), diff.WithIgnoreAddingMapKeys(), diff.WithCompareNumberAndNumericString(), diff --git a/pkg/app/piped/planner/kubernetes/BUILD.bazel b/pkg/app/piped/planner/kubernetes/BUILD.bazel index e742a747c5..aaeb0cfb51 100644 --- a/pkg/app/piped/planner/kubernetes/BUILD.bazel +++ b/pkg/app/piped/planner/kubernetes/BUILD.bazel @@ -36,5 +36,6 @@ go_test( "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1/unstructured:go_default_library", + "@org_uber_go_zap//:go_default_library", ], ) diff --git a/pkg/app/piped/planner/kubernetes/kubernetes.go b/pkg/app/piped/planner/kubernetes/kubernetes.go index b415b630cb..0ccca05d76 100644 --- a/pkg/app/piped/planner/kubernetes/kubernetes.go +++ b/pkg/app/piped/planner/kubernetes/kubernetes.go @@ -209,7 +209,7 @@ func (p *Planner) Plan(ctx context.Context, in planner.Input) (out planner.Outpu manifestCache.Put(in.MostRecentSuccessfulCommitHash, oldManifests) } - progressive, desc := decideStrategy(oldManifests, newManifests, cfg.Workloads) + progressive, desc := decideStrategy(oldManifests, newManifests, cfg.Workloads, in.Logger) out.Summary = desc if progressive { @@ -225,7 +225,7 @@ func (p *Planner) Plan(ctx context.Context, in planner.Input) (out planner.Outpu // First up, checks to see if the workload's `spec.template` has been changed, // and then checks if the configmap/secret's data. -func decideStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference) (progressive bool, desc string) { +func decideStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (progressive bool, desc string) { oldWorkloads := findWorkloadManifests(olds, workloadRefs) if len(oldWorkloads) == 0 { desc = "Quick sync by applying all manifests because it was unable to find the currently running workloads" @@ -243,7 +243,7 @@ func decideStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sRes for _, w := range workloads { // If the workload's pod template was touched // do progressive deployment with the specified pipeline. - diffResult, err := provider.Diff(w.old, w.new) + diffResult, err := provider.Diff(w.old, w.new, logger) if err != nil { progressive = true desc = fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err) @@ -287,7 +287,7 @@ func decideStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sRes desc = fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Key.Kind, oc.Key.Name) return } - result, err := provider.Diff(oc, nc) + result, err := provider.Diff(oc, nc, logger) if err != nil { progressive = true desc = fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err) diff --git a/pkg/app/piped/planner/kubernetes/kubernetes_test.go b/pkg/app/piped/planner/kubernetes/kubernetes_test.go index 816864372d..8afacb46df 100644 --- a/pkg/app/piped/planner/kubernetes/kubernetes_test.go +++ b/pkg/app/piped/planner/kubernetes/kubernetes_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" provider "github.com/pipe-cd/pipecd/pkg/app/piped/cloudprovider/kubernetes" @@ -391,7 +392,7 @@ func TestDecideStrategy(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - gotProgressive, gotDesc := decideStrategy(tc.olds, tc.news, tc.workloadRefs) + gotProgressive, gotDesc := decideStrategy(tc.olds, tc.news, tc.workloadRefs, zap.NewNop()) assert.Equal(t, tc.wantProgressive, gotProgressive) assert.Equal(t, tc.wantDesc, gotDesc) }) @@ -567,7 +568,7 @@ func TestCheckImageChange(t *testing.T) { workloads := findUpdatedWorkloads(oldManifests, newManifests) for _, w := range workloads { - diffResult, err := provider.Diff(w.old, w.new) + diffResult, err := provider.Diff(w.old, w.new, zap.NewNop()) require.NoError(t, err) diffNodes := diffResult.Nodes() templateDiffs := diffNodes.FindByPrefix("spec.template") diff --git a/pkg/app/piped/planpreview/kubernetesdiff.go b/pkg/app/piped/planpreview/kubernetesdiff.go index ac13c05ee7..f2d827789b 100644 --- a/pkg/app/piped/planpreview/kubernetesdiff.go +++ b/pkg/app/piped/planpreview/kubernetesdiff.go @@ -63,6 +63,7 @@ func (b *builder) kubernetesDiff( result, err := provider.DiffList( oldManifests, newManifests, + b.logger, diff.WithEquateEmpty(), diff.WithCompareNumberAndNumericString(), )