From 0814973e131f37451cc740d6794f3ec389e7d53d Mon Sep 17 00:00:00 2001 From: Luca Berneking Date: Fri, 3 Apr 2020 18:58:33 +0200 Subject: [PATCH] [helm-operator] Fix reconcilation of resources with jsonpatch This PR updates the jsonpatch library to fix an issue during reconcilation of arrays. The old version had an incomplete implementation for json patches of arrays. A diff between ``` containers: - name: container1 - name: container2 ``` and ``` containers: - name: container1 ``` created 3 patch operations: - remove containers/0 - remove containers/1 - add containers/0 This causes an issue because we are ignoring all "remove" operations to allow manual modifications. Helm tries to apply ``` containers: - name: container1 - name: container2 - name: container1 ``` ``` invalid: spec.template.spec.containers[0].name: Duplicate value: "container1" ``` The new jsonpatch version does only return a `remove containers/1` operation (which we are still ignoring because it could be a manual change, but does not fail) --- CHANGELOG.md | 1 + go.mod | 2 +- go.sum | 6 +- pkg/helm/release/manager.go | 5 +- pkg/helm/release/manager_test.go | 140 +++++++++++++++++++++++++++++++ test/test-framework/go.sum | 3 +- 6 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 pkg/helm/release/manager_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ce279c0495c..019248e7894 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ - The command `operator-sdk generate k8s` no longer requires users to explicitly set GOROOT in their environment. Now, GOROOT is detected using `go env GOROOT` and set automatically. ([#2754](https://github.com/operator-framework/operator-sdk/pull/2754)) - `operator-sdk generate csv` and `operator-sdk test local` now parse multi-manifest files correctly. ([#2758](https://github.com/operator-framework/operator-sdk/pull/2758)) - Fixed CRD validation generation issue with `status.Conditions`. ([#2739](https://github.com/operator-framework/operator-sdk/pull/2739)) +- Fix issue faced in the reconciliation when arrays are used in the config YAML files for Helm based-operators. ([#2777](https://github.com/operator-framework/operator-sdk/pull/2777)) ## v0.16.0 diff --git a/go.mod b/go.mod index a9edcbcdb8f..86aee7be57f 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,6 @@ require ( github.com/jmoiron/sqlx v1.2.0 // indirect github.com/markbates/inflect v1.0.4 github.com/martinlindhe/base36 v1.0.0 - github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a github.com/mattn/go-isatty v0.0.12 github.com/mitchellh/go-homedir v1.1.0 github.com/mitchellh/mapstructure v1.1.2 @@ -40,6 +39,7 @@ require ( github.com/ziutek/mymysql v1.5.4 // indirect go.uber.org/zap v1.14.1 golang.org/x/tools v0.0.0-20200327195553-82bb89366a1e + gomodules.xyz/jsonpatch/v3 v3.0.1 gopkg.in/gorp.v1 v1.7.2 // indirect gopkg.in/yaml.v2 v2.2.8 helm.sh/helm/v3 v3.1.2 diff --git a/go.sum b/go.sum index 952977b6882..c061e058ffe 100644 --- a/go.sum +++ b/go.sum @@ -631,8 +631,6 @@ github.com/markbates/inflect v1.0.4/go.mod h1:1fR9+pO2KHEO9ZRtto13gDwwZaAKstQzfe github.com/marstr/guid v1.1.0/go.mod h1:74gB1z2wpxxInTG6yaqA7KrtM0NZ+RbrcqDvYHefzho= github.com/martinlindhe/base36 v1.0.0 h1:eYsumTah144C0A8P1T/AVSUk5ZoLnhfYFM3OGQxB52A= github.com/martinlindhe/base36 v1.0.0/go.mod h1:+AtEs8xrBpCeYgSLoY/aJ6Wf37jtBuR0s35750M27+8= -github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a h1:+J2gw7Bw77w/fbK7wnNJJDKmw1IbWft2Ul5BzrG1Qm8= -github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a/go.mod h1:M1qoD/MqPgTZIk0EWKB38wE28ACRfVcn+cU08jyArI0= github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= github.com/mattn/go-colorable v0.1.2 h1:/bC9yWikZXAL9uJdulbSfyVNIR3n3trXl+v8+1sx8mU= github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= @@ -1206,6 +1204,10 @@ golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IV golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gomodules.xyz/jsonpatch/v2 v2.0.1 h1:xyiBuvkD2g5n7cYzx6u2sxQvsAy4QJsZFCzGVdzOXZ0= gomodules.xyz/jsonpatch/v2 v2.0.1/go.mod h1:IhYNNY4jnS53ZnfE4PAmpKtDpTCj1JFXc+3mwe7XcUU= +gomodules.xyz/jsonpatch/v3 v3.0.1 h1:Te7hKxV52TKCbNYq3t84tzKav3xhThdvSsSp/W89IyI= +gomodules.xyz/jsonpatch/v3 v3.0.1/go.mod h1:CBhndykehEwTOlEfnsfJwvkFQbSN8YZFr9M+cIHAJto= +gomodules.xyz/orderedmap v0.1.0 h1:fM/+TGh/O1KkqGR5xjTKg6bU8OKBkg7p0Y+x/J9m8Os= +gomodules.xyz/orderedmap v0.1.0/go.mod h1:g9/TPUCm1t2gwD3j3zfV8uylyYhVdCNSi+xCEIu7yTU= gonum.org/v1/gonum v0.0.0-20190331200053-3d26580ed485/go.mod h1:2ltnJ7xHfj0zHS40VVPYEAAMTa3ZGguvHGBSJeRWqE0= gonum.org/v1/netlib v0.0.0-20190313105609-8cb42192e0e0/go.mod h1:wa6Ws7BG/ESfp6dHfk7C6KdzKA7wR7u/rKwOGE66zvw= gonum.org/v1/netlib v0.0.0-20190331212654-76723241ea4e/go.mod h1:kS+toOQn6AQKjmKJ7gzohV1XkqsFehRA2FbsbkopSuQ= diff --git a/pkg/helm/release/manager.go b/pkg/helm/release/manager.go index 8cc35a5ebd9..a9275deb6e5 100644 --- a/pkg/helm/release/manager.go +++ b/pkg/helm/release/manager.go @@ -22,6 +22,7 @@ import ( "fmt" "strings" + "gomodules.xyz/jsonpatch/v3" "helm.sh/helm/v3/pkg/action" cpb "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/kube" @@ -35,7 +36,6 @@ import ( "k8s.io/cli-runtime/pkg/resource" "k8s.io/client-go/rest" - "github.com/mattbaird/jsonpatch" "github.com/operator-framework/operator-sdk/pkg/helm/internal/types" ) @@ -275,9 +275,10 @@ func generatePatch(existing, expected runtime.Object) ([]byte, error) { // fields added by Kubernetes or by the user after the existing release // resource has been applied. The goal for this patch is to make sure that // the fields managed by the Helm chart are applied. + // All "add" operations without a value (null) can be ignored patchOps := make([]jsonpatch.JsonPatchOperation, 0) for _, op := range ops { - if op.Operation != "remove" { + if op.Operation != "remove" && !(op.Operation == "add" && op.Value == nil) { patchOps = append(patchOps, op) } } diff --git a/pkg/helm/release/manager_test.go b/pkg/helm/release/manager_test.go new file mode 100644 index 00000000000..a980b1faf91 --- /dev/null +++ b/pkg/helm/release/manager_test.go @@ -0,0 +1,140 @@ +// Copyright 2018 The Operator-SDK 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 release + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func newTestDeployment(containers []interface{}) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "Deployment", + "apiVersion": "apps/v1", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "ns", + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": containers, + }, + }, + }, + }, + } +} + +func TestManagerGeneratePatch(t *testing.T) { + + tests := []struct { + o1 *unstructured.Unstructured + o2 *unstructured.Unstructured + patch []map[string]interface{} + }{ + { + o1: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + map[string]interface{}{ + "name": "test2", + }, + }), + o2: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + }), + patch: []map[string]interface{}{}, + }, + { + o1: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + }), + o2: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + map[string]interface{}{ + "name": "test2", + }, + }), + patch: []map[string]interface{}{ + { + "op": "add", + "path": "/spec/template/spec/containers/1", + "value": map[string]interface{}{ + "name": string("test2"), + }, + }, + }, + }, + { + o1: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + }), + o2: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + "test": nil, + }, + }), + patch: []map[string]interface{}{}, + }, + { + o1: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test1", + }, + }), + o2: newTestDeployment([]interface{}{ + map[string]interface{}{ + "name": "test2", + }, + }), + patch: []map[string]interface{}{ + { + "op": "replace", + "path": "/spec/template/spec/containers/0/name", + "value": "test2", + }, + }, + }, + } + + for _, test := range tests { + diff, err := generatePatch(test.o1, test.o2) + assert.NoError(t, err) + + if len(test.patch) == 0 { + assert.Equal(t, 0, len(test.patch)) + } else { + x := []map[string]interface{}{} + err = json.Unmarshal(diff, &x) + assert.NoError(t, err) + assert.Equal(t, test.patch, x) + } + } +} diff --git a/test/test-framework/go.sum b/test/test-framework/go.sum index bbb5b95fed4..89d33ab7a26 100644 --- a/test/test-framework/go.sum +++ b/test/test-framework/go.sum @@ -552,7 +552,6 @@ github.com/markbates/inflect v1.0.4 h1:5fh1gzTFhfae06u3hzHYO9xe3l3v3nW5Pwt3naLTP github.com/markbates/inflect v1.0.4/go.mod h1:1fR9+pO2KHEO9ZRtto13gDwwZaAKstQzferVeWqbgNs= github.com/marstr/guid v1.1.0/go.mod h1:74gB1z2wpxxInTG6yaqA7KrtM0NZ+RbrcqDvYHefzho= github.com/martinlindhe/base36 v1.0.0/go.mod h1:+AtEs8xrBpCeYgSLoY/aJ6Wf37jtBuR0s35750M27+8= -github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a/go.mod h1:M1qoD/MqPgTZIk0EWKB38wE28ACRfVcn+cU08jyArI0= github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-ieproxy v0.0.0-20190610004146-91bb50d98149/go.mod h1:31jz6HNzdxOmlERGGEc4v/dMssOfmp2p5bT/okiKFFc= @@ -1086,6 +1085,8 @@ golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IV golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gomodules.xyz/jsonpatch/v2 v2.0.1 h1:xyiBuvkD2g5n7cYzx6u2sxQvsAy4QJsZFCzGVdzOXZ0= gomodules.xyz/jsonpatch/v2 v2.0.1/go.mod h1:IhYNNY4jnS53ZnfE4PAmpKtDpTCj1JFXc+3mwe7XcUU= +gomodules.xyz/jsonpatch/v3 v3.0.1/go.mod h1:CBhndykehEwTOlEfnsfJwvkFQbSN8YZFr9M+cIHAJto= +gomodules.xyz/orderedmap v0.1.0/go.mod h1:g9/TPUCm1t2gwD3j3zfV8uylyYhVdCNSi+xCEIu7yTU= gonum.org/v1/gonum v0.0.0-20190331200053-3d26580ed485/go.mod h1:2ltnJ7xHfj0zHS40VVPYEAAMTa3ZGguvHGBSJeRWqE0= gonum.org/v1/netlib v0.0.0-20190313105609-8cb42192e0e0/go.mod h1:wa6Ws7BG/ESfp6dHfk7C6KdzKA7wR7u/rKwOGE66zvw= gonum.org/v1/netlib v0.0.0-20190331212654-76723241ea4e/go.mod h1:kS+toOQn6AQKjmKJ7gzohV1XkqsFehRA2FbsbkopSuQ=