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

Commit cf8d42f

Browse files
authored
fix: server-side diff shows duplicate containerPorts (#791)
* fix: server-side diff shows duplicate containerPorts Signed-off-by: Peter Jiang <[email protected]> * lint Signed-off-by: Peter Jiang <[email protected]> --------- Signed-off-by: Peter Jiang <[email protected]>
1 parent ff8c119 commit cf8d42f

File tree

8 files changed

+515
-22
lines changed

8 files changed

+515
-22
lines changed

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ require (
9797
sigs.k8s.io/kustomize/api v0.20.1 // indirect
9898
sigs.k8s.io/kustomize/kyaml v0.20.1 // indirect
9999
sigs.k8s.io/randfill v1.0.0 // indirect
100+
sigs.k8s.io/structured-merge-diff/v4 v4.7.0 // indirect
100101
)
101102

102103
replace (

go.sum

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ github.com/google/btree v1.1.3 h1:CVpQJjYgC4VbzxeGVHfvZrv1ctoYCAI8vbl07Fcxlyg=
4949
github.com/google/btree v1.1.3/go.mod h1:qOPhT0dTNdNzV6Z/lhRX0YXUafgPLFUh+gZMl761Gm4=
5050
github.com/google/gnostic-models v0.7.0 h1:qwTtogB15McXDaNqTZdzPJRHvaVJlAl+HVQnLmJEJxo=
5151
github.com/google/gnostic-models v0.7.0/go.mod h1:whL5G0m6dmc5cPxKc5bdKdEN3UjI7OUGxBlw57miDrQ=
52+
github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
5253
github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
5354
github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
5455
github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
@@ -249,9 +250,13 @@ sigs.k8s.io/kustomize/api v0.20.1 h1:iWP1Ydh3/lmldBnH/S5RXgT98vWYMaTUL1ADcr+Sv7I
249250
sigs.k8s.io/kustomize/api v0.20.1/go.mod h1:t6hUFxO+Ph0VxIk1sKp1WS0dOjbPCtLJ4p8aADLwqjM=
250251
sigs.k8s.io/kustomize/kyaml v0.20.1 h1:PCMnA2mrVbRP3NIB6v9kYCAc38uvFLVs8j/CD567A78=
251252
sigs.k8s.io/kustomize/kyaml v0.20.1/go.mod h1:0EmkQHRUsJxY8Ug9Niig1pUMSCGHxQ5RklbpV/Ri6po=
253+
sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=
252254
sigs.k8s.io/randfill v1.0.0 h1:JfjMILfT8A6RbawdsK2JXGBR5AQVfd+9TbzrlneTyrU=
253255
sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=
256+
sigs.k8s.io/structured-merge-diff/v4 v4.7.0 h1:qPeWmscJcXP0snki5IYF79Z8xrl8ETFxgMd7wez1XkI=
257+
sigs.k8s.io/structured-merge-diff/v4 v4.7.0/go.mod h1:dDy58f92j70zLsuZVuUX5Wp9vtxXpaZnkPGWeqDfCps=
254258
sigs.k8s.io/structured-merge-diff/v6 v6.3.0 h1:jTijUJbW353oVOd9oTlifJqOGEkUw2jB/fXCbTiQEco=
255259
sigs.k8s.io/structured-merge-diff/v6 v6.3.0/go.mod h1:M3W8sfWvn2HhQDIbGWj3S099YozAsymCo/wrT5ohRUE=
260+
sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=
256261
sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs=
257262
sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=

pkg/diff/diff.go

Lines changed: 51 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
267267
}
268268

269269
// In case any of the removed fields cause schema violations, we will keep those fields
270-
nonArgoFieldsSet = safelyRemoveFieldsSet(typedPredictedLive, nonArgoFieldsSet)
270+
nonArgoFieldsSet = filterOutCompositeKeyFields(typedPredictedLive, nonArgoFieldsSet)
271271
typedPredictedLive = typedPredictedLive.RemoveItems(nonArgoFieldsSet)
272272

273273
// Apply the predicted live state to the live state to get a diff without mutation webhook fields
@@ -289,29 +289,58 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
289289
return &unstructured.Unstructured{Object: pl}, nil
290290
}
291291

292-
// safelyRemoveFieldSet will validate if removing the fieldsToRemove set from predictedLive maintains
293-
// a valid schema. If removing a field in fieldsToRemove is invalid and breaks the schema, it is not safe
294-
// to remove and will be skipped from removal from predictedLive.
295-
func safelyRemoveFieldsSet(predictedLive *typed.TypedValue, fieldsToRemove *fieldpath.Set) *fieldpath.Set {
296-
// In some cases, we cannot remove fields due to violation of the predicted live schema. In such cases we validate the removal
297-
// of each field and only include it if the removal is valid.
298-
testPredictedLive := predictedLive.RemoveItems(fieldsToRemove)
299-
err := testPredictedLive.Validate()
300-
if err != nil {
301-
adjustedFieldsToRemove := fieldpath.NewSet()
302-
fieldsToRemove.Iterate(func(p fieldpath.Path) {
303-
singleFieldSet := fieldpath.NewSet(p)
304-
testSingleRemoval := predictedLive.RemoveItems(singleFieldSet)
305-
// Check if removing this single field maintains a valid schema
306-
if testSingleRemoval.Validate() == nil {
307-
// If valid, add this field to the adjusted set to remove
308-
adjustedFieldsToRemove.Insert(p)
292+
// filterOutCompositeKeyFields filters out fields that are part of composite keys in associative lists.
293+
// These fields must be preserved to maintain list element identity during merge operations.
294+
func filterOutCompositeKeyFields(_ *typed.TypedValue, fieldsToRemove *fieldpath.Set) *fieldpath.Set {
295+
filteredFields := fieldpath.NewSet()
296+
297+
fieldsToRemove.Iterate(func(fieldPath fieldpath.Path) {
298+
isCompositeKey := isCompositeKeyField(fieldPath)
299+
if !isCompositeKey {
300+
// Only keep fields that are NOT composite keys - these are safe to remove
301+
filteredFields.Insert(fieldPath)
302+
}
303+
})
304+
305+
return filteredFields
306+
}
307+
308+
// isCompositeKeyField checks if a field path represents a field that is part of a composite key
309+
// in an associative list by examining the PathElement structure.
310+
// Example: .spec.containers[name="nginx"].ports[containerPort=80,protocol="TCP"].protocol
311+
// The path elements include:
312+
// - PathElement{Key: {name: "nginx"}} - single key (not composite)
313+
// - PathElement{Key: {containerPort: 80, protocol: "TCP"}} - composite key with 2 fields
314+
func isCompositeKeyField(fieldPath fieldpath.Path) bool {
315+
if len(fieldPath) == 0 {
316+
return false
317+
}
318+
319+
// Get the last path element
320+
lastElement := fieldPath[len(fieldPath)-1]
321+
if lastElement.FieldName == nil {
322+
return false
323+
}
324+
finalFieldName := *lastElement.FieldName
325+
326+
// Look backwards through the path to find the most recent associative list key
327+
for i := len(fieldPath) - 2; i >= 0; i-- {
328+
pe := fieldPath[i]
329+
if pe.Key == nil {
330+
continue
331+
}
332+
if len(*pe.Key) <= 1 {
333+
continue
334+
}
335+
// This is a composite key
336+
for _, keyField := range *pe.Key {
337+
if keyField.Name == finalFieldName {
338+
return true
309339
}
310-
})
311-
return adjustedFieldsToRemove
340+
}
312341
}
313-
// If no violations, return the original set to remove
314-
return fieldsToRemove
342+
343+
return false
315344
}
316345

317346
func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error) {

pkg/diff/diff_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1106,6 +1106,66 @@ func TestServerSideDiff(t *testing.T) {
11061106
// Other fields should still be visible (not ignored)
11071107
assert.Contains(t, predictedLiveStr, "selector", "Other fields should remain visible")
11081108
})
1109+
1110+
t.Run("will preserve composite key fields during diff", func(t *testing.T) {
1111+
// given
1112+
t.Parallel()
1113+
liveState := StrToUnstructured(testdata.DeploymentCompositeKeyLiveYAMLSSD)
1114+
desiredState := StrToUnstructured(testdata.DeploymentCompositeKeyConfigYAMLSSD)
1115+
opts := buildOpts(testdata.DeploymentCompositeKeyPredictedLiveJSONSSD)
1116+
1117+
// when
1118+
result, err := serverSideDiff(desiredState, liveState, opts...)
1119+
1120+
// then
1121+
require.NoError(t, err)
1122+
assert.NotNil(t, result)
1123+
assert.True(t, result.Modified)
1124+
1125+
predictedDeploy := YamlToDeploy(t, result.PredictedLive)
1126+
liveDeploy := YamlToDeploy(t, result.NormalizedLive)
1127+
1128+
// Verify the nginx container has all 3 ports in predicted live
1129+
assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers, 2, "Should have 2 containers")
1130+
nginxContainer := predictedDeploy.Spec.Template.Spec.Containers[0]
1131+
assert.Equal(t, "nginx", nginxContainer.Name)
1132+
assert.Len(t, nginxContainer.Ports, 3, "nginx should have 3 ports in predicted")
1133+
1134+
// Verify live still has only 2 ports for nginx
1135+
liveNginxContainer := liveDeploy.Spec.Template.Spec.Containers[0]
1136+
assert.Len(t, liveNginxContainer.Ports, 2, "nginx should have 2 ports in live")
1137+
1138+
// Check that the new port 8080 has protocol field preserved (composite key field)
1139+
port8080Found := false
1140+
for _, port := range nginxContainer.Ports {
1141+
if port.ContainerPort == 8080 {
1142+
port8080Found = true
1143+
assert.Equal(t, "metrics", port.Name, "Port 8080 should have name 'metrics'")
1144+
assert.Equal(t, corev1.ProtocolTCP, port.Protocol, "Port 8080 protocol (composite key field) must be preserved from webhook")
1145+
}
1146+
}
1147+
assert.True(t, port8080Found, "Port 8080 should be present in predicted live")
1148+
1149+
// Verify existing ports still have their protocol (also composite key fields)
1150+
port80Found := false
1151+
port443Found := false
1152+
for _, port := range nginxContainer.Ports {
1153+
if port.ContainerPort == 80 {
1154+
port80Found = true
1155+
assert.Equal(t, corev1.ProtocolTCP, port.Protocol)
1156+
}
1157+
if port.ContainerPort == 443 {
1158+
port443Found = true
1159+
assert.Equal(t, corev1.ProtocolTCP, port.Protocol)
1160+
}
1161+
}
1162+
assert.True(t, port80Found, "Port 80 should be present")
1163+
assert.True(t, port443Found, "Port 443 should be present")
1164+
1165+
// Verify that mutation webhook changes are still filtered out from diff
1166+
assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig])
1167+
assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig])
1168+
})
11091169
}
11101170

11111171
// testIgnoreDifferencesNormalizer implements a simple normalizer that removes specified fields

pkg/diff/testdata/data.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,13 @@ var (
7777

7878
//go:embed ssd-svc-no-label-predicted-live.json
7979
ServicePredictedLiveNoLabelJSONSSD string
80+
81+
//go:embed ssd-deploy-composite-key-config.yaml
82+
DeploymentCompositeKeyConfigYAMLSSD string
83+
84+
//go:embed ssd-deploy-composite-key-live.yaml
85+
DeploymentCompositeKeyLiveYAMLSSD string
86+
87+
//go:embed ssd-deploy-composite-key-predicted-live.json
88+
DeploymentCompositeKeyPredictedLiveJSONSSD string
8089
)
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
apiVersion: apps/v1
2+
kind: Deployment
3+
metadata:
4+
name: test-container-ports
5+
namespace: default
6+
labels:
7+
app: test-app
8+
spec:
9+
replicas: 1
10+
selector:
11+
matchLabels:
12+
app: test-app
13+
template:
14+
metadata:
15+
labels:
16+
app: test-app
17+
spec:
18+
containers:
19+
- name: nginx
20+
image: nginx:1.21
21+
ports:
22+
- containerPort: 80
23+
name: http
24+
- containerPort: 443
25+
name: https
26+
- containerPort: 8080
27+
name: metrics
28+
- name: sidecar
29+
image: busybox:1.35
30+
command: ["sleep", "3600"]
31+
ports:
32+
- containerPort: 9090
33+
name: sidecar-port
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
apiVersion: apps/v1
2+
kind: Deployment
3+
metadata:
4+
name: test-container-ports
5+
namespace: default
6+
labels:
7+
app: test-app
8+
uid: 12345678-1234-1234-1234-123456789012
9+
resourceVersion: "12345"
10+
generation: 1
11+
creationTimestamp: "2024-01-01T00:00:00Z"
12+
managedFields:
13+
- apiVersion: apps/v1
14+
fieldsType: FieldsV1
15+
fieldsV1:
16+
f:spec:
17+
f:template:
18+
f:spec:
19+
f:containers:
20+
k:{"name":"nginx"}:
21+
f:ports:
22+
k:{"containerPort":80,"protocol":"TCP"}:
23+
.: {}
24+
f:containerPort: {}
25+
f:name: {}
26+
f:protocol: {}
27+
k:{"containerPort":443,"protocol":"TCP"}:
28+
.: {}
29+
f:containerPort: {}
30+
f:name: {}
31+
f:protocol: {}
32+
k:{"name":"sidecar"}:
33+
f:ports:
34+
k:{"containerPort":9090,"protocol":"TCP"}:
35+
.: {}
36+
f:containerPort: {}
37+
f:name: {}
38+
f:protocol: {}
39+
manager: argocd-controller
40+
operation: Apply
41+
time: "2024-01-01T00:00:00Z"
42+
spec:
43+
replicas: 1
44+
selector:
45+
matchLabels:
46+
app: test-app
47+
template:
48+
metadata:
49+
labels:
50+
app: test-app
51+
spec:
52+
containers:
53+
- name: nginx
54+
image: nginx:1.21
55+
ports:
56+
- containerPort: 80
57+
name: http
58+
protocol: TCP
59+
- containerPort: 443
60+
name: https
61+
protocol: TCP
62+
terminationMessagePath: /dev/termination-log
63+
terminationMessagePolicy: File
64+
imagePullPolicy: IfNotPresent
65+
- name: sidecar
66+
image: busybox:1.35
67+
command: ["sleep", "3600"]
68+
ports:
69+
- containerPort: 9090
70+
name: sidecar-port
71+
protocol: TCP
72+
terminationMessagePath: /dev/termination-log
73+
terminationMessagePolicy: File
74+
imagePullPolicy: IfNotPresent
75+
restartPolicy: Always
76+
terminationGracePeriodSeconds: 30
77+
dnsPolicy: ClusterFirst
78+
securityContext: {}
79+
schedulerName: default-scheduler
80+
strategy:
81+
type: RollingUpdate
82+
rollingUpdate:
83+
maxUnavailable: 25%
84+
maxSurge: 25%
85+
revisionHistoryLimit: 10
86+
progressDeadlineSeconds: 600
87+
status:
88+
observedGeneration: 1
89+
replicas: 1
90+
updatedReplicas: 1
91+
readyReplicas: 1
92+
availableReplicas: 1

0 commit comments

Comments
 (0)