Skip to content

Commit 88b804f

Browse files
LucaberLucaber
committed
[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)
1 parent 5fdbb62 commit 88b804f

File tree

6 files changed

+147
-6
lines changed

6 files changed

+147
-6
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
- 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))
4949
- `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))
5050
- Fixed CRD validation generation issue with `status.Conditions`. ([#2739](https://github.com/operator-framework/operator-sdk/pull/2739))
51+
- Fixed helm-operator issue with reconciliation of arrays ([#2777](https://github.com/operator-framework/operator-sdk/pull/2777))
5152

5253
## v0.16.0
5354

‎go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ require (
1818
github.com/jmoiron/sqlx v1.2.0 // indirect
1919
github.com/markbates/inflect v1.0.4
2020
github.com/martinlindhe/base36 v1.0.0
21-
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a
2221
github.com/mattn/go-isatty v0.0.12
2322
github.com/mitchellh/go-homedir v1.1.0
2423
github.com/mitchellh/mapstructure v1.1.2
@@ -40,6 +39,7 @@ require (
4039
github.com/ziutek/mymysql v1.5.4 // indirect
4140
go.uber.org/zap v1.14.1
4241
golang.org/x/tools v0.0.0-20200327195553-82bb89366a1e
42+
gomodules.xyz/jsonpatch/v3 v3.0.1
4343
gopkg.in/gorp.v1 v1.7.2 // indirect
4444
gopkg.in/yaml.v2 v2.2.8
4545
helm.sh/helm/v3 v3.1.2

‎go.sum

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,8 +631,6 @@ github.com/markbates/inflect v1.0.4/go.mod h1:1fR9+pO2KHEO9ZRtto13gDwwZaAKstQzfe
631631
github.com/marstr/guid v1.1.0/go.mod h1:74gB1z2wpxxInTG6yaqA7KrtM0NZ+RbrcqDvYHefzho=
632632
github.com/martinlindhe/base36 v1.0.0 h1:eYsumTah144C0A8P1T/AVSUk5ZoLnhfYFM3OGQxB52A=
633633
github.com/martinlindhe/base36 v1.0.0/go.mod h1:+AtEs8xrBpCeYgSLoY/aJ6Wf37jtBuR0s35750M27+8=
634-
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a h1:+J2gw7Bw77w/fbK7wnNJJDKmw1IbWft2Ul5BzrG1Qm8=
635-
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a/go.mod h1:M1qoD/MqPgTZIk0EWKB38wE28ACRfVcn+cU08jyArI0=
636634
github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
637635
github.com/mattn/go-colorable v0.1.2 h1:/bC9yWikZXAL9uJdulbSfyVNIR3n3trXl+v8+1sx8mU=
638636
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
12061204
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
12071205
gomodules.xyz/jsonpatch/v2 v2.0.1 h1:xyiBuvkD2g5n7cYzx6u2sxQvsAy4QJsZFCzGVdzOXZ0=
12081206
gomodules.xyz/jsonpatch/v2 v2.0.1/go.mod h1:IhYNNY4jnS53ZnfE4PAmpKtDpTCj1JFXc+3mwe7XcUU=
1207+
gomodules.xyz/jsonpatch/v3 v3.0.1 h1:Te7hKxV52TKCbNYq3t84tzKav3xhThdvSsSp/W89IyI=
1208+
gomodules.xyz/jsonpatch/v3 v3.0.1/go.mod h1:CBhndykehEwTOlEfnsfJwvkFQbSN8YZFr9M+cIHAJto=
1209+
gomodules.xyz/orderedmap v0.1.0 h1:fM/+TGh/O1KkqGR5xjTKg6bU8OKBkg7p0Y+x/J9m8Os=
1210+
gomodules.xyz/orderedmap v0.1.0/go.mod h1:g9/TPUCm1t2gwD3j3zfV8uylyYhVdCNSi+xCEIu7yTU=
12091211
gonum.org/v1/gonum v0.0.0-20190331200053-3d26580ed485/go.mod h1:2ltnJ7xHfj0zHS40VVPYEAAMTa3ZGguvHGBSJeRWqE0=
12101212
gonum.org/v1/netlib v0.0.0-20190313105609-8cb42192e0e0/go.mod h1:wa6Ws7BG/ESfp6dHfk7C6KdzKA7wR7u/rKwOGE66zvw=
12111213
gonum.org/v1/netlib v0.0.0-20190331212654-76723241ea4e/go.mod h1:kS+toOQn6AQKjmKJ7gzohV1XkqsFehRA2FbsbkopSuQ=

‎pkg/helm/release/manager.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"fmt"
2323
"strings"
2424

25+
"gomodules.xyz/jsonpatch/v3"
2526
"helm.sh/helm/v3/pkg/action"
2627
cpb "helm.sh/helm/v3/pkg/chart"
2728
"helm.sh/helm/v3/pkg/kube"
@@ -35,7 +36,6 @@ import (
3536
"k8s.io/cli-runtime/pkg/resource"
3637
"k8s.io/client-go/rest"
3738

38-
"github.com/mattbaird/jsonpatch"
3939
"github.com/operator-framework/operator-sdk/pkg/helm/internal/types"
4040
)
4141

@@ -275,9 +275,13 @@ func generatePatch(existing, expected runtime.Object) ([]byte, error) {
275275
// fields added by Kubernetes or by the user after the existing release
276276
// resource has been applied. The goal for this patch is to make sure that
277277
// the fields managed by the Helm chart are applied.
278+
// Add operations without a value (nulll) can be ignored
278279
patchOps := make([]jsonpatch.JsonPatchOperation, 0)
280+
removeOps := make([]jsonpatch.JsonPatchOperation, 0)
279281
for _, op := range ops {
280-
if op.Operation != "remove" {
282+
if op.Operation == "remove" {
283+
removeOps = append(removeOps, op)
284+
} else if !(op.Operation == "add" && op.Value == nil) {
281285
patchOps = append(patchOps, op)
282286
}
283287
}
@@ -289,6 +293,17 @@ func generatePatch(existing, expected runtime.Object) ([]byte, error) {
289293
return nil, nil
290294
}
291295

296+
// We should add "remove" operations if the same path does also have
297+
// a "add" operation
298+
for _, rop := range removeOps {
299+
for _, pop := range patchOps {
300+
if rop.Path == pop.Path && pop.Operation == "add" {
301+
patchOps = append(patchOps, rop)
302+
break
303+
}
304+
}
305+
}
306+
292307
return json.Marshal(patchOps)
293308
}
294309

‎pkg/helm/release/manager_test.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// Copyright 2018 The Operator-SDK 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 release
16+
17+
import (
18+
"encoding/json"
19+
"testing"
20+
21+
"github.com/stretchr/testify/assert"
22+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
23+
)
24+
25+
func newTestDeployment(containers []interface{}) *unstructured.Unstructured {
26+
return &unstructured.Unstructured{
27+
Object: map[string]interface{}{
28+
"kind": "Deployment",
29+
"apiVersion": "apps/v1",
30+
"metadata": map[string]interface{}{
31+
"name": "test",
32+
"namespace": "ns",
33+
},
34+
"spec": map[string]interface{}{
35+
"template": map[string]interface{}{
36+
"spec": map[string]interface{}{
37+
"containers": containers,
38+
},
39+
},
40+
},
41+
},
42+
}
43+
}
44+
45+
func TestManagerGeneratePatch(t *testing.T) {
46+
47+
tests := []struct {
48+
o1 *unstructured.Unstructured
49+
o2 *unstructured.Unstructured
50+
patch []map[string]interface{}
51+
}{
52+
{
53+
o1: newTestDeployment([]interface{}{
54+
map[string]interface{}{
55+
"name": "test1",
56+
},
57+
map[string]interface{}{
58+
"name": "test2",
59+
},
60+
}),
61+
o2: newTestDeployment([]interface{}{
62+
map[string]interface{}{
63+
"name": "test1",
64+
},
65+
}),
66+
patch: []map[string]interface{}{},
67+
},
68+
{
69+
o1: newTestDeployment([]interface{}{
70+
map[string]interface{}{
71+
"name": "test1",
72+
},
73+
}),
74+
o2: newTestDeployment([]interface{}{
75+
map[string]interface{}{
76+
"name": "test1",
77+
},
78+
map[string]interface{}{
79+
"name": "test2",
80+
},
81+
}),
82+
patch: []map[string]interface{}{
83+
{
84+
"op": "add",
85+
"path": "/spec/template/spec/containers/1",
86+
"value": map[string]interface{}{
87+
"name": string("test2"),
88+
},
89+
},
90+
},
91+
},
92+
{
93+
o1: newTestDeployment([]interface{}{
94+
map[string]interface{}{
95+
"name": "test1",
96+
},
97+
}),
98+
o2: newTestDeployment([]interface{}{
99+
map[string]interface{}{
100+
"name": "test2",
101+
},
102+
}),
103+
patch: []map[string]interface{}{
104+
{
105+
"op": "replace",
106+
"path": "/spec/template/spec/containers/0/name",
107+
"value": "test2",
108+
},
109+
},
110+
},
111+
}
112+
113+
for _, test := range tests {
114+
diff, err := generatePatch(test.o1, test.o2)
115+
assert.NoError(t, err)
116+
117+
x := []map[string]interface{}{}
118+
err = json.Unmarshal(diff, &x)
119+
assert.NoError(t, err)
120+
assert.Equal(t, x, test.patch)
121+
}
122+
}

‎test/test-framework/go.sum

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,6 @@ github.com/markbates/inflect v1.0.4 h1:5fh1gzTFhfae06u3hzHYO9xe3l3v3nW5Pwt3naLTP
552552
github.com/markbates/inflect v1.0.4/go.mod h1:1fR9+pO2KHEO9ZRtto13gDwwZaAKstQzferVeWqbgNs=
553553
github.com/marstr/guid v1.1.0/go.mod h1:74gB1z2wpxxInTG6yaqA7KrtM0NZ+RbrcqDvYHefzho=
554554
github.com/martinlindhe/base36 v1.0.0/go.mod h1:+AtEs8xrBpCeYgSLoY/aJ6Wf37jtBuR0s35750M27+8=
555-
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a/go.mod h1:M1qoD/MqPgTZIk0EWKB38wE28ACRfVcn+cU08jyArI0=
556555
github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
557556
github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=
558557
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
10861085
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
10871086
gomodules.xyz/jsonpatch/v2 v2.0.1 h1:xyiBuvkD2g5n7cYzx6u2sxQvsAy4QJsZFCzGVdzOXZ0=
10881087
gomodules.xyz/jsonpatch/v2 v2.0.1/go.mod h1:IhYNNY4jnS53ZnfE4PAmpKtDpTCj1JFXc+3mwe7XcUU=
1088+
gomodules.xyz/jsonpatch/v3 v3.0.1/go.mod h1:CBhndykehEwTOlEfnsfJwvkFQbSN8YZFr9M+cIHAJto=
1089+
gomodules.xyz/orderedmap v0.1.0/go.mod h1:g9/TPUCm1t2gwD3j3zfV8uylyYhVdCNSi+xCEIu7yTU=
10891090
gonum.org/v1/gonum v0.0.0-20190331200053-3d26580ed485/go.mod h1:2ltnJ7xHfj0zHS40VVPYEAAMTa3ZGguvHGBSJeRWqE0=
10901091
gonum.org/v1/netlib v0.0.0-20190313105609-8cb42192e0e0/go.mod h1:wa6Ws7BG/ESfp6dHfk7C6KdzKA7wR7u/rKwOGE66zvw=
10911092
gonum.org/v1/netlib v0.0.0-20190331212654-76723241ea4e/go.mod h1:kS+toOQn6AQKjmKJ7gzohV1XkqsFehRA2FbsbkopSuQ=

0 commit comments

Comments
 (0)