Skip to content
This repository was archived by the owner on Dec 17, 2025. It is now read-only.

Commit 8849c3f

Browse files
andrii-korotkov-verkadaandrii-korotkov-verkada
authored
fix: Server side diff now works correctly with fields removal (#640)
* fix: Server side diff now works correctly with some fields removal Helps with argoproj/argo-cd#20792 Removed and modified sets may only contain the fields that changed, not including key fields like "name". This can cause merge to fail, since it expects those fields to be present if they are present in the predicted live. Fortunately, we can inspect the set and derive the key fields necessary. Then they can be added to the set and used during a merge. Also, have a new test which fails before the fix, but passes now. Failure of the new test before the fix ``` Error: Received unexpected error: error removing non config mutations for resource Deployment/nginx-deployment: error reverting webhook removed fields in predicted live resource: .spec.template.spec.containers: element 0: associative list with keys has an element that omits key field "name" (and doesn't have default value) Test: TestServerSideDiff/will_test_removing_some_field_with_undoing_changes_done_by_webhook ``` Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * Use new version of structured merge diff with a new option Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * Add DCO Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> * Try to fix sonar exclusions config Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> --------- Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
1 parent 0371401 commit 8849c3f

File tree

9 files changed

+362
-5
lines changed

9 files changed

+362
-5
lines changed

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ require (
2323
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340
2424
k8s.io/kubectl v0.31.2
2525
k8s.io/kubernetes v1.31.0
26-
sigs.k8s.io/structured-merge-diff/v4 v4.4.3
26+
sigs.k8s.io/structured-merge-diff/v4 v4.4.4-0.20241211184406-7bf59b3d70ee
2727
sigs.k8s.io/yaml v1.4.0
2828
)
2929

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ sigs.k8s.io/kustomize/api v0.17.2 h1:E7/Fjk7V5fboiuijoZHgs4aHuexi5Y2loXlVOAVAG5g
328328
sigs.k8s.io/kustomize/api v0.17.2/go.mod h1:UWTz9Ct+MvoeQsHcJ5e+vziRRkwimm3HytpZgIYqye0=
329329
sigs.k8s.io/kustomize/kyaml v0.17.1 h1:TnxYQxFXzbmNG6gOINgGWQt09GghzgTP6mIurOgrLCQ=
330330
sigs.k8s.io/kustomize/kyaml v0.17.1/go.mod h1:9V0mCjIEYjlXuCdYsSXvyoy2BTsLESH7TlGV81S282U=
331-
sigs.k8s.io/structured-merge-diff/v4 v4.4.3 h1:sCP7Vv3xx/CWIuTPVN38lUPx0uw0lcLfzaiDa8Ja01A=
332-
sigs.k8s.io/structured-merge-diff/v4 v4.4.3/go.mod h1:N8f93tFZh9U6vpxwRArLiikrE5/2tiu1w1AGfACIGE4=
331+
sigs.k8s.io/structured-merge-diff/v4 v4.4.4-0.20241211184406-7bf59b3d70ee h1:ipT2c6nEOdAfBwiwW1oI0mkrlPabbXEFmJBrg6B+OR8=
332+
sigs.k8s.io/structured-merge-diff/v4 v4.4.4-0.20241211184406-7bf59b3d70ee/go.mod h1:N8f93tFZh9U6vpxwRArLiikrE5/2tiu1w1AGfACIGE4=
333333
sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=
334334
sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=

‎pkg/diff/diff.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
258258
}
259259

260260
if comparison.Modified != nil && !comparison.Modified.Empty() {
261-
liveModValues := typedLive.ExtractItems(comparison.Modified)
261+
liveModValues := typedLive.ExtractItems(comparison.Modified, typed.WithAppendKeyFields())
262262
// revert modified fields not owned by any manager
263263
typedPredictedLive, err = typedPredictedLive.Merge(liveModValues)
264264
if err != nil {
@@ -267,7 +267,7 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
267267
}
268268

269269
if comparison.Removed != nil && !comparison.Removed.Empty() {
270-
liveRmValues := typedLive.ExtractItems(comparison.Removed)
270+
liveRmValues := typedLive.ExtractItems(comparison.Removed, typed.WithAppendKeyFields())
271271
// revert removed fields not owned by any manager
272272
typedPredictedLive, err = typedPredictedLive.Merge(liveRmValues)
273273
if err != nil {

‎pkg/diff/diff_test.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -933,6 +933,31 @@ func TestServerSideDiff(t *testing.T) {
933933
assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig])
934934
assert.Empty(t, predictedSVC.Labels["event"])
935935
})
936+
937+
t.Run("will test removing some field with undoing changes done by webhook", func(t *testing.T) {
938+
// given
939+
t.Parallel()
940+
liveState := StrToUnstructured(testdata.Deployment2LiveYAML)
941+
desiredState := StrToUnstructured(testdata.Deployment2ConfigYAML)
942+
opts := buildOpts(testdata.Deployment2PredictedLiveJSONSSD)
943+
944+
// when
945+
result, err := serverSideDiff(desiredState, liveState, opts...)
946+
947+
// then
948+
require.NoError(t, err)
949+
assert.NotNil(t, result)
950+
assert.True(t, result.Modified)
951+
predictedDeploy := YamlToDeploy(t, result.PredictedLive)
952+
liveDeploy := YamlToDeploy(t, result.NormalizedLive)
953+
assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers, 1)
954+
assert.Len(t, liveDeploy.Spec.Template.Spec.Containers, 1)
955+
assert.Equal(t, "500m", predictedDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Cpu().String())
956+
assert.Equal(t, "512Mi", predictedDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Memory().String())
957+
assert.Equal(t, "500m", liveDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Cpu().String())
958+
assert.Equal(t, "512Mi", liveDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Memory().String())
959+
})
960+
936961
t.Run("will include mutation webhook modifications", func(t *testing.T) {
937962
// given
938963
t.Parallel()

‎pkg/diff/testdata/data.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,15 @@ var (
2424
//go:embed smd-deploy-config.yaml
2525
DeploymentConfigYAML string
2626

27+
//go:embed smd-deploy2-live.yaml
28+
Deployment2LiveYAML string
29+
30+
//go:embed smd-deploy2-config.yaml
31+
Deployment2ConfigYAML string
32+
33+
//go:embed smd-deploy2-predicted-live.json
34+
Deployment2PredictedLiveJSONSSD string
35+
2736
// OpenAPIV2Doc is a binary representation of the openapi
2837
// document available in a given k8s instance. To update
2938
// this file the following commands can be executed:

‎pkg/diff/testdata/smd-deploy2-config.yaml‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
apiVersion: apps/v1
2+
kind: Deployment
3+
metadata:
4+
labels:
5+
app: missing
6+
applications.argoproj.io/app-name: nginx
7+
something-else: bla
8+
name: nginx-deployment
9+
namespace: default
10+
spec:
11+
replicas: 2
12+
selector:
13+
matchLabels:
14+
app: nginx
15+
template:
16+
metadata:
17+
labels:
18+
app: nginx
19+
applications.argoproj.io/app-name: nginx
20+
spec:
21+
containers:
22+
- image: 'nginx:1.23.1'
23+
imagePullPolicy: Never
24+
livenessProbe:
25+
exec:
26+
command:
27+
- cat
28+
- non-existent-file
29+
initialDelaySeconds: 5
30+
periodSeconds: 180
31+
name: nginx
32+
ports:
33+
- containerPort: 8081
34+
protocol: UDP
35+
- containerPort: 80
36+
protocol: TCP

‎pkg/diff/testdata/smd-deploy2-live.yaml‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
apiVersion: apps/v1
2+
kind: Deployment
3+
metadata:
4+
annotations:
5+
deployment.kubernetes.io/revision: '1'
6+
creationTimestamp: '2022-09-18T23:50:25Z'
7+
generation: 1
8+
labels:
9+
app: missing
10+
applications.argoproj.io/app-name: nginx
11+
something-else: bla
12+
managedFields:
13+
- apiVersion: apps/v1
14+
fieldsType: FieldsV1
15+
fieldsV1:
16+
'f:metadata':
17+
'f:labels':
18+
'f:app': {}
19+
'f:applications.argoproj.io/app-name': {}
20+
'f:something-else': {}
21+
'f:spec':
22+
'f:replicas': {}
23+
'f:selector': {}
24+
'f:template':
25+
'f:metadata':
26+
'f:labels':
27+
'f:app': {}
28+
'f:applications.argoproj.io/app-name': {}
29+
'f:spec':
30+
'f:containers':
31+
'k:{"name":"nginx"}':
32+
.: {}
33+
'f:image': {}
34+
'f:imagePullPolicy': {}
35+
'f:livenessProbe':
36+
'f:exec':
37+
'f:command': {}
38+
'f:initialDelaySeconds': {}
39+
'f:periodSeconds': {}
40+
'f:name': {}
41+
'f:ports':
42+
'k:{"containerPort":80,"protocol":"TCP"}':
43+
.: {}
44+
'f:containerPort': {}
45+
'f:protocol': {}
46+
'f:resources':
47+
'f:requests':
48+
'f:cpu': {}
49+
'f:memory': {}
50+
manager: argocd-controller
51+
operation: Apply
52+
time: '2022-09-18T23:50:25Z'
53+
- apiVersion: apps/v1
54+
fieldsType: FieldsV1
55+
fieldsV1:
56+
'f:metadata':
57+
'f:annotations':
58+
.: {}
59+
'f:deployment.kubernetes.io/revision': {}
60+
'f:status':
61+
'f:availableReplicas': {}
62+
'f:conditions':
63+
.: {}
64+
'k:{"type":"Available"}':
65+
.: {}
66+
'f:lastTransitionTime': {}
67+
'f:lastUpdateTime': {}
68+
'f:message': {}
69+
'f:reason': {}
70+
'f:status': {}
71+
'f:type': {}
72+
'k:{"type":"Progressing"}':
73+
.: {}
74+
'f:lastTransitionTime': {}
75+
'f:lastUpdateTime': {}
76+
'f:message': {}
77+
'f:reason': {}
78+
'f:status': {}
79+
'f:type': {}
80+
'f:observedGeneration': {}
81+
'f:readyReplicas': {}
82+
'f:replicas': {}
83+
'f:updatedReplicas': {}
84+
manager: kube-controller-manager
85+
operation: Update
86+
subresource: status
87+
time: '2022-09-23T18:30:59Z'
88+
name: nginx-deployment
89+
namespace: default
90+
resourceVersion: '7492752'
91+
uid: 731f7434-d3d9-47fa-b179-d9368a84f7c9
92+
spec:
93+
progressDeadlineSeconds: 600
94+
replicas: 2
95+
revisionHistoryLimit: 10
96+
selector:
97+
matchLabels:
98+
app: nginx
99+
strategy:
100+
rollingUpdate:
101+
maxSurge: 25%
102+
maxUnavailable: 25%
103+
type: RollingUpdate
104+
template:
105+
metadata:
106+
creationTimestamp: null
107+
labels:
108+
app: nginx
109+
applications.argoproj.io/app-name: nginx
110+
spec:
111+
containers:
112+
- image: 'nginx:1.23.1'
113+
imagePullPolicy: Never
114+
livenessProbe:
115+
exec:
116+
command:
117+
- cat
118+
- non-existent-file
119+
failureThreshold: 3
120+
initialDelaySeconds: 5
121+
periodSeconds: 180
122+
successThreshold: 1
123+
timeoutSeconds: 1
124+
name: nginx
125+
ports:
126+
- containerPort: 80
127+
protocol: TCP
128+
- containerPort: 8080
129+
protocol: TCP
130+
- containerPort: 8081
131+
protocol: UDP
132+
resources:
133+
requests:
134+
memory: 512Mi
135+
cpu: 500m
136+
terminationMessagePath: /dev/termination-log
137+
terminationMessagePolicy: File
138+
dnsPolicy: ClusterFirst
139+
restartPolicy: Always
140+
schedulerName: default-scheduler
141+
securityContext: {}
142+
terminationGracePeriodSeconds: 30
143+
status:
144+
availableReplicas: 2
145+
conditions:
146+
- lastTransitionTime: '2022-09-18T23:50:25Z'
147+
lastUpdateTime: '2022-09-18T23:50:26Z'
148+
message: ReplicaSet "nginx-deployment-6d68ff5f86" has successfully progressed.
149+
reason: NewReplicaSetAvailable
150+
status: 'True'
151+
type: Progressing
152+
- lastTransitionTime: '2022-09-23T18:30:59Z'
153+
lastUpdateTime: '2022-09-23T18:30:59Z'
154+
message: Deployment has minimum availability.
155+
reason: MinimumReplicasAvailable
156+
status: 'True'
157+
type: Available
158+
observedGeneration: 1
159+
readyReplicas: 2
160+
replicas: 2
161+
updatedReplicas: 2

0 commit comments

Comments
 (0)