[helm-operator] Fix reconciliation of resources with jsonpatch #2777
Conversation
|
@Lucaber Thanks for reporting this and contributing a fix! This definitely looks like an area ripe for bugs, but I haven't been able to reproduce it. I'm probably not fully understanding the scenario correctly. What is the array that helm is managing via the CR and its release, and what is the manual change? I'm also wondering if we could make use of |
|
Scratch that, I reproduced by creating an operator from the default nginx helm chart, running it and deploying the default CR, and then manually editing the Deployment to add an extra container. Looks like your patch fixes it! If you're interested in researching the strategic merge patch option, I can leave this open. Otherwise, I'm good with this approach and researching the strategic merge patch option later. |
I added some tests for the
This sounds perfect, but i'm not familiar with the k8s apis at all. I will take a look at this later. Maybe we could merge this fix first? We ran into this issue because we are using the jaeger-operator which injects a tracing sidecar container into every deployment. |
Lucaber
force-pushed
the
fix/jsonpatch
branch
3 times, most recently
from
88b804f
to
0cb2481
Compare
pkg/helm/release/manager.go
Outdated
| if op.Operation != "remove" { | ||
| if op.Operation == "remove" { | ||
| removeOps = append(removeOps, op) | ||
| } else if !(op.Operation == "add" && op.Value == nil) { |
There was a problem hiding this comment.
Should not be just else?
What happens if it faces a conditional that is not defined here?
There was a problem hiding this comment.
I had issues with chart templates that contain fields with a value of null. During every reconciliation, the operator tried to apply these changes to Kubernetes, but Kubernetes ignores null values completely and they do not appear in for example kubectl get -o yaml. So one minute later, the operator tries it again and again..
There was a problem hiding this comment.
I will add a test case for this
pkg/helm/release/manager.go
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It was added after :
// If there are no patch operations, return nil. Callers are expected
// to check for a nil response and skip the patch operation to avoid
// unnecessary chatter with the API server.
if len(patchOps) == 0 {
return nil, nil
}
Should not be before?
There was a problem hiding this comment.
If the patchOps slice is empty, the next for-loop wont change it anyway, so we can already return here
pkg/helm/release/manager.go
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Also, see that the above is not covered by the tests. See: https://coveralls.io/builds/29873112/source?filename=pkg/helm/release/manager.go#L304
There was a problem hiding this comment.
added a new test case to cover this
There was a problem hiding this comment.
well no, i didn't, i cant replicate this issue with the new jsonpath library, should i remove this?
There was a problem hiding this comment.
Am I understanding correctly that it seems like the new library correctly chooses replace rather than adding both an add and remove?
Does that just mean this PR becomes just the dependency change to use the new library?
There was a problem hiding this comment.
Yes, i removed the additional for-loop. I think this issue is fixed in the new jsonpatch library
Lucaber
force-pushed
the
fix/jsonpatch
branch
4 times, most recently
from
6cc7647
to
40afb0f
Compare
There was a problem hiding this comment.
HI @Lucaber,
Needs fix the lint issue:
Running golangci-lint
pkg/helm/release/manager.go:283:22: SA4010: this result of append is never used, except maybe in other appends (staticcheck)
removeOps = append(removeOps, op)
^
To test it locally you can run make lint
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)
| patchOps := make([]jsonpatch.JsonPatchOperation, 0) | ||
| for _, op := range ops { | ||
| if op.Operation != "remove" { | ||
| if op.Operation != "remove" && !(op.Operation == "add" && op.Value == nil) { |
…ative k8s objects (#2869) A follow-up PR to #2808 and #2777. @joelanford In this PR we are using the Kubernetes strategic three-way merge patch for patches by the helm-operator for native Kubernetes objects. This ensures that we are using the correct merge strategy that is defined in the OpenAPI schema of the object. #2808 However, we do not have the required schema information for custom resources(CRs). So we need to fall back to the previous manual json-patch. To detect CRs we are using the same method as helm https://github.com/helm/helm/blob/b21b00978539ea8270e6b672bc1e6bc07af61475/pkg/kube/client.go#L391 Co-authored-by: Joe Lanford <joe.lanford@gmail.com> Co-authored-by: Henning Surmeier <h.surmeier@mittwald.de>
This PR updates the jsonpatch library to fix an issue during reconciliation of arrays.
The old version had an incomplete implementation for json patches of arrays.
A diff between
and
created 3 patch operations:
This causes an issue because we are ignoring all "remove" operations to allow manual modifications.
Helm tries to apply
The new jsonpatch version does only return a
remove containers/1operation (which we are still ignoring because it could be a manual
change, but does not fail)