mirror of
https://github.com/argoproj/argo-cd.git
synced 2026-02-20 01:28:45 +01:00
fix: server-side diff shows duplicate containerPorts (#24785)
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
This commit is contained in:
Peter Jiang
@@ -267,7 +267,7 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa
|
||||
}
|
||||
|
||||
// In case any of the removed fields cause schema violations, we will keep those fields
|
||||
nonArgoFieldsSet = safelyRemoveFieldsSet(typedPredictedLive, nonArgoFieldsSet)
|
||||
nonArgoFieldsSet = filterOutCompositeKeyFields(typedPredictedLive, nonArgoFieldsSet)
|
||||
typedPredictedLive = typedPredictedLive.RemoveItems(nonArgoFieldsSet)
|
||||
|
||||
// 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
|
||||
return &unstructured.Unstructured{Object: pl}, nil
|
||||
}
|
||||
|
||||
// safelyRemoveFieldSet will validate if removing the fieldsToRemove set from predictedLive maintains
|
||||
// a valid schema. If removing a field in fieldsToRemove is invalid and breaks the schema, it is not safe
|
||||
// to remove and will be skipped from removal from predictedLive.
|
||||
func safelyRemoveFieldsSet(predictedLive *typed.TypedValue, fieldsToRemove *fieldpath.Set) *fieldpath.Set {
|
||||
// In some cases, we cannot remove fields due to violation of the predicted live schema. In such cases we validate the removal
|
||||
// of each field and only include it if the removal is valid.
|
||||
testPredictedLive := predictedLive.RemoveItems(fieldsToRemove)
|
||||
err := testPredictedLive.Validate()
|
||||
if err != nil {
|
||||
adjustedFieldsToRemove := fieldpath.NewSet()
|
||||
fieldsToRemove.Iterate(func(p fieldpath.Path) {
|
||||
singleFieldSet := fieldpath.NewSet(p)
|
||||
testSingleRemoval := predictedLive.RemoveItems(singleFieldSet)
|
||||
// Check if removing this single field maintains a valid schema
|
||||
if testSingleRemoval.Validate() == nil {
|
||||
// If valid, add this field to the adjusted set to remove
|
||||
adjustedFieldsToRemove.Insert(p)
|
||||
}
|
||||
})
|
||||
return adjustedFieldsToRemove
|
||||
// filterOutCompositeKeyFields filters out fields that are part of composite keys in associative lists.
|
||||
// These fields must be preserved to maintain list element identity during merge operations.
|
||||
func filterOutCompositeKeyFields(typedValue *typed.TypedValue, fieldsToRemove *fieldpath.Set) *fieldpath.Set {
|
||||
filteredFields := fieldpath.NewSet()
|
||||
|
||||
fieldsToRemove.Iterate(func(fieldPath fieldpath.Path) {
|
||||
isCompositeKey := isCompositeKeyField(fieldPath)
|
||||
if !isCompositeKey {
|
||||
// Only keep fields that are NOT composite keys - these are safe to remove
|
||||
filteredFields.Insert(fieldPath)
|
||||
}
|
||||
})
|
||||
|
||||
return filteredFields
|
||||
}
|
||||
|
||||
// isCompositeKeyField checks if a field path represents a field that is part of a composite key
|
||||
// in an associative list by examining the PathElement structure.
|
||||
// Example: .spec.containers[name="nginx"].ports[containerPort=80,protocol="TCP"].protocol
|
||||
// The path elements include:
|
||||
// - PathElement{Key: {name: "nginx"}} - single key (not composite)
|
||||
// - PathElement{Key: {containerPort: 80, protocol: "TCP"}} - composite key with 2 fields
|
||||
func isCompositeKeyField(fieldPath fieldpath.Path) bool {
|
||||
if len(fieldPath) == 0 {
|
||||
return false
|
||||
}
|
||||
// If no violations, return the original set to remove
|
||||
return fieldsToRemove
|
||||
|
||||
// Get the last path element
|
||||
lastElement := fieldPath[len(fieldPath)-1]
|
||||
if lastElement.FieldName == nil {
|
||||
return false
|
||||
}
|
||||
finalFieldName := *lastElement.FieldName
|
||||
|
||||
// Look backwards through the path to find the most recent associative list key
|
||||
for i := len(fieldPath) - 2; i >= 0; i-- {
|
||||
pe := fieldPath[i]
|
||||
if pe.Key == nil {
|
||||
continue
|
||||
}
|
||||
if len(*pe.Key) <= 1 {
|
||||
continue
|
||||
}
|
||||
// This is a composite key
|
||||
for _, keyField := range *pe.Key {
|
||||
if keyField.Name == finalFieldName {
|
||||
return true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error) {
|
||||
|
||||
@@ -1106,6 +1106,66 @@ func TestServerSideDiff(t *testing.T) {
|
||||
// Other fields should still be visible (not ignored)
|
||||
assert.Contains(t, predictedLiveStr, "selector", "Other fields should remain visible")
|
||||
})
|
||||
|
||||
t.Run("will preserve composite key fields during diff", func(t *testing.T) {
|
||||
// given
|
||||
t.Parallel()
|
||||
liveState := StrToUnstructured(testdata.DeploymentCompositeKeyLiveYAMLSSD)
|
||||
desiredState := StrToUnstructured(testdata.DeploymentCompositeKeyConfigYAMLSSD)
|
||||
opts := buildOpts(testdata.DeploymentCompositeKeyPredictedLiveJSONSSD)
|
||||
|
||||
// when
|
||||
result, err := serverSideDiff(desiredState, liveState, opts...)
|
||||
|
||||
// then
|
||||
require.NoError(t, err)
|
||||
assert.NotNil(t, result)
|
||||
assert.True(t, result.Modified)
|
||||
|
||||
predictedDeploy := YamlToDeploy(t, result.PredictedLive)
|
||||
liveDeploy := YamlToDeploy(t, result.NormalizedLive)
|
||||
|
||||
// Verify the nginx container has all 3 ports in predicted live
|
||||
assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers, 2, "Should have 2 containers")
|
||||
nginxContainer := predictedDeploy.Spec.Template.Spec.Containers[0]
|
||||
assert.Equal(t, "nginx", nginxContainer.Name)
|
||||
assert.Len(t, nginxContainer.Ports, 3, "nginx should have 3 ports in predicted")
|
||||
|
||||
// Verify live still has only 2 ports for nginx
|
||||
liveNginxContainer := liveDeploy.Spec.Template.Spec.Containers[0]
|
||||
assert.Len(t, liveNginxContainer.Ports, 2, "nginx should have 2 ports in live")
|
||||
|
||||
// Check that the new port 8080 has protocol field preserved (composite key field)
|
||||
port8080Found := false
|
||||
for _, port := range nginxContainer.Ports {
|
||||
if port.ContainerPort == 8080 {
|
||||
port8080Found = true
|
||||
assert.Equal(t, "metrics", port.Name, "Port 8080 should have name 'metrics'")
|
||||
assert.Equal(t, corev1.ProtocolTCP, port.Protocol, "Port 8080 protocol (composite key field) must be preserved from webhook")
|
||||
}
|
||||
}
|
||||
assert.True(t, port8080Found, "Port 8080 should be present in predicted live")
|
||||
|
||||
// Verify existing ports still have their protocol (also composite key fields)
|
||||
port80Found := false
|
||||
port443Found := false
|
||||
for _, port := range nginxContainer.Ports {
|
||||
if port.ContainerPort == 80 {
|
||||
port80Found = true
|
||||
assert.Equal(t, corev1.ProtocolTCP, port.Protocol)
|
||||
}
|
||||
if port.ContainerPort == 443 {
|
||||
port443Found = true
|
||||
assert.Equal(t, corev1.ProtocolTCP, port.Protocol)
|
||||
}
|
||||
}
|
||||
assert.True(t, port80Found, "Port 80 should be present")
|
||||
assert.True(t, port443Found, "Port 443 should be present")
|
||||
|
||||
// Verify that mutation webhook changes are still filtered out from diff
|
||||
assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig])
|
||||
assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig])
|
||||
})
|
||||
}
|
||||
|
||||
// testIgnoreDifferencesNormalizer implements a simple normalizer that removes specified fields
|
||||
|
||||
9
gitops-engine/pkg/diff/testdata/data.go
vendored
9
gitops-engine/pkg/diff/testdata/data.go
vendored
@@ -77,4 +77,13 @@ var (
|
||||
|
||||
//go:embed ssd-svc-no-label-predicted-live.json
|
||||
ServicePredictedLiveNoLabelJSONSSD string
|
||||
|
||||
//go:embed ssd-deploy-composite-key-config.yaml
|
||||
DeploymentCompositeKeyConfigYAMLSSD string
|
||||
|
||||
//go:embed ssd-deploy-composite-key-live.yaml
|
||||
DeploymentCompositeKeyLiveYAMLSSD string
|
||||
|
||||
//go:embed ssd-deploy-composite-key-predicted-live.json
|
||||
DeploymentCompositeKeyPredictedLiveJSONSSD string
|
||||
)
|
||||
|
||||
33
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-config.yaml
vendored
Normal file
33
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-config.yaml
vendored
Normal file
@@ -0,0 +1,33 @@
|
||||
apiVersion: apps/v1
|
||||
kind: Deployment
|
||||
metadata:
|
||||
name: test-container-ports
|
||||
namespace: default
|
||||
labels:
|
||||
app: test-app
|
||||
spec:
|
||||
replicas: 1
|
||||
selector:
|
||||
matchLabels:
|
||||
app: test-app
|
||||
template:
|
||||
metadata:
|
||||
labels:
|
||||
app: test-app
|
||||
spec:
|
||||
containers:
|
||||
- name: nginx
|
||||
image: nginx:1.21
|
||||
ports:
|
||||
- containerPort: 80
|
||||
name: http
|
||||
- containerPort: 443
|
||||
name: https
|
||||
- containerPort: 8080
|
||||
name: metrics
|
||||
- name: sidecar
|
||||
image: busybox:1.35
|
||||
command: ["sleep", "3600"]
|
||||
ports:
|
||||
- containerPort: 9090
|
||||
name: sidecar-port
|
||||
92
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-live.yaml
vendored
Normal file
92
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-live.yaml
vendored
Normal file
@@ -0,0 +1,92 @@
|
||||
apiVersion: apps/v1
|
||||
kind: Deployment
|
||||
metadata:
|
||||
name: test-container-ports
|
||||
namespace: default
|
||||
labels:
|
||||
app: test-app
|
||||
uid: 12345678-1234-1234-1234-123456789012
|
||||
resourceVersion: "12345"
|
||||
generation: 1
|
||||
creationTimestamp: "2024-01-01T00:00:00Z"
|
||||
managedFields:
|
||||
- apiVersion: apps/v1
|
||||
fieldsType: FieldsV1
|
||||
fieldsV1:
|
||||
f:spec:
|
||||
f:template:
|
||||
f:spec:
|
||||
f:containers:
|
||||
k:{"name":"nginx"}:
|
||||
f:ports:
|
||||
k:{"containerPort":80,"protocol":"TCP"}:
|
||||
.: {}
|
||||
f:containerPort: {}
|
||||
f:name: {}
|
||||
f:protocol: {}
|
||||
k:{"containerPort":443,"protocol":"TCP"}:
|
||||
.: {}
|
||||
f:containerPort: {}
|
||||
f:name: {}
|
||||
f:protocol: {}
|
||||
k:{"name":"sidecar"}:
|
||||
f:ports:
|
||||
k:{"containerPort":9090,"protocol":"TCP"}:
|
||||
.: {}
|
||||
f:containerPort: {}
|
||||
f:name: {}
|
||||
f:protocol: {}
|
||||
manager: argocd-controller
|
||||
operation: Apply
|
||||
time: "2024-01-01T00:00:00Z"
|
||||
spec:
|
||||
replicas: 1
|
||||
selector:
|
||||
matchLabels:
|
||||
app: test-app
|
||||
template:
|
||||
metadata:
|
||||
labels:
|
||||
app: test-app
|
||||
spec:
|
||||
containers:
|
||||
- name: nginx
|
||||
image: nginx:1.21
|
||||
ports:
|
||||
- containerPort: 80
|
||||
name: http
|
||||
protocol: TCP
|
||||
- containerPort: 443
|
||||
name: https
|
||||
protocol: TCP
|
||||
terminationMessagePath: /dev/termination-log
|
||||
terminationMessagePolicy: File
|
||||
imagePullPolicy: IfNotPresent
|
||||
- name: sidecar
|
||||
image: busybox:1.35
|
||||
command: ["sleep", "3600"]
|
||||
ports:
|
||||
- containerPort: 9090
|
||||
name: sidecar-port
|
||||
protocol: TCP
|
||||
terminationMessagePath: /dev/termination-log
|
||||
terminationMessagePolicy: File
|
||||
imagePullPolicy: IfNotPresent
|
||||
restartPolicy: Always
|
||||
terminationGracePeriodSeconds: 30
|
||||
dnsPolicy: ClusterFirst
|
||||
securityContext: {}
|
||||
schedulerName: default-scheduler
|
||||
strategy:
|
||||
type: RollingUpdate
|
||||
rollingUpdate:
|
||||
maxUnavailable: 25%
|
||||
maxSurge: 25%
|
||||
revisionHistoryLimit: 10
|
||||
progressDeadlineSeconds: 600
|
||||
status:
|
||||
observedGeneration: 1
|
||||
replicas: 1
|
||||
updatedReplicas: 1
|
||||
readyReplicas: 1
|
||||
availableReplicas: 1
|
||||
264
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-predicted-live.json
vendored
Normal file
264
gitops-engine/pkg/diff/testdata/ssd-deploy-composite-key-predicted-live.json
vendored
Normal file
@@ -0,0 +1,264 @@
|
||||
{
|
||||
"apiVersion": "apps/v1",
|
||||
"kind": "Deployment",
|
||||
"metadata": {
|
||||
"name": "test-container-ports",
|
||||
"namespace": "default",
|
||||
"labels": {
|
||||
"app": "test-app"
|
||||
},
|
||||
"uid": "84ec73f0-7144-4fd6-9fb1-40485f47b185",
|
||||
"resourceVersion": "2137313",
|
||||
"generation": 3,
|
||||
"creationTimestamp": "2025-09-19T19:23:03Z",
|
||||
"annotations": {
|
||||
"deployment.kubernetes.io/revision": "1"
|
||||
},
|
||||
"managedFields": [
|
||||
{
|
||||
"apiVersion": "apps/v1",
|
||||
"fieldsType": "FieldsV1",
|
||||
"fieldsV1": {
|
||||
"f:metadata": {
|
||||
"f:labels": {
|
||||
"f:app": {}
|
||||
}
|
||||
},
|
||||
"f:spec": {
|
||||
"f:replicas": {},
|
||||
"f:selector": {},
|
||||
"f:template": {
|
||||
"f:metadata": {
|
||||
"f:labels": {
|
||||
"f:app": {}
|
||||
}
|
||||
},
|
||||
"f:spec": {
|
||||
"f:containers": {
|
||||
"k:{\"name\":\"nginx\"}": {
|
||||
".": {},
|
||||
"f:image": {},
|
||||
"f:name": {},
|
||||
"f:ports": {
|
||||
"k:{\"containerPort\":80,\"protocol\":\"TCP\"}": {
|
||||
".": {},
|
||||
"f:containerPort": {},
|
||||
"f:name": {}
|
||||
},
|
||||
"k:{\"containerPort\":443,\"protocol\":\"TCP\"}": {
|
||||
".": {},
|
||||
"f:containerPort": {},
|
||||
"f:name": {}
|
||||
},
|
||||
"k:{\"containerPort\":8080,\"protocol\":\"TCP\"}": {
|
||||
".": {},
|
||||
"f:containerPort": {},
|
||||
"f:name": {}
|
||||
}
|
||||
}
|
||||
},
|
||||
"k:{\"name\":\"sidecar\"}": {
|
||||
".": {},
|
||||
"f:command": {},
|
||||
"f:image": {},
|
||||
"f:name": {},
|
||||
"f:ports": {
|
||||
"k:{\"containerPort\":9090,\"protocol\":\"TCP\"}": {
|
||||
".": {},
|
||||
"f:containerPort": {},
|
||||
"f:name": {}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"manager": "argocd-controller",
|
||||
"operation": "Apply",
|
||||
"time": "2025-09-30T17:52:41Z"
|
||||
},
|
||||
{
|
||||
"apiVersion": "apps/v1",
|
||||
"fieldsType": "FieldsV1",
|
||||
"fieldsV1": {
|
||||
"f:spec": {
|
||||
"f:template": {
|
||||
"f:spec": {
|
||||
"f:containers": {
|
||||
"k:{\"name\":\"nginx\"}": {
|
||||
"f:ports": {
|
||||
"k:{\"containerPort\":80,\"protocol\":\"TCP\"}": {
|
||||
"f:protocol": {}
|
||||
},
|
||||
"k:{\"containerPort\":443,\"protocol\":\"TCP\"}": {
|
||||
"f:protocol": {}
|
||||
},
|
||||
"k:{\"containerPort\":8080,\"protocol\":\"TCP\"}": {
|
||||
"f:protocol": {}
|
||||
}
|
||||
}
|
||||
},
|
||||
"k:{\"name\":\"sidecar\"}": {
|
||||
"f:ports": {
|
||||
"k:{\"containerPort\":9090,\"protocol\":\"TCP\"}": {
|
||||
"f:protocol": {}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"manager": "mutation-webhook",
|
||||
"operation": "Apply",
|
||||
"time": "2025-09-30T17:52:42Z"
|
||||
},
|
||||
{
|
||||
"apiVersion": "apps/v1",
|
||||
"fieldsType": "FieldsV1",
|
||||
"fieldsV1": {
|
||||
"f:metadata": {
|
||||
"f:annotations": {
|
||||
"f:deployment.kubernetes.io/revision": {}
|
||||
}
|
||||
},
|
||||
"f:status": {
|
||||
"f:availableReplicas": {},
|
||||
"f:conditions": {
|
||||
".": {},
|
||||
"k:{\"type\":\"Available\"}": {
|
||||
".": {},
|
||||
"f:lastTransitionTime": {},
|
||||
"f:lastUpdateTime": {},
|
||||
"f:message": {},
|
||||
"f:reason": {},
|
||||
"f:status": {},
|
||||
"f:type": {}
|
||||
},
|
||||
"k:{\"type\":\"Progressing\"}": {
|
||||
".": {},
|
||||
"f:lastTransitionTime": {},
|
||||
"f:lastUpdateTime": {},
|
||||
"f:message": {},
|
||||
"f:reason": {},
|
||||
"f:status": {},
|
||||
"f:type": {}
|
||||
}
|
||||
},
|
||||
"f:observedGeneration": {},
|
||||
"f:readyReplicas": {},
|
||||
"f:replicas": {},
|
||||
"f:updatedReplicas": {}
|
||||
}
|
||||
},
|
||||
"manager": "kube-controller-manager",
|
||||
"operation": "Update",
|
||||
"subresource": "status",
|
||||
"time": "2025-09-30T17:49:16Z"
|
||||
}
|
||||
]
|
||||
},
|
||||
"spec": {
|
||||
"progressDeadlineSeconds": 600,
|
||||
"replicas": 1,
|
||||
"revisionHistoryLimit": 10,
|
||||
"selector": {
|
||||
"matchLabels": {
|
||||
"app": "test-app"
|
||||
}
|
||||
},
|
||||
"strategy": {
|
||||
"type": "RollingUpdate",
|
||||
"rollingUpdate": {
|
||||
"maxSurge": "25%",
|
||||
"maxUnavailable": "25%"
|
||||
}
|
||||
},
|
||||
"template": {
|
||||
"metadata": {
|
||||
"creationTimestamp": null,
|
||||
"labels": {
|
||||
"app": "test-app"
|
||||
}
|
||||
},
|
||||
"spec": {
|
||||
"containers": [
|
||||
{
|
||||
"name": "nginx",
|
||||
"image": "nginx:1.21",
|
||||
"ports": [
|
||||
{
|
||||
"containerPort": 80,
|
||||
"name": "http",
|
||||
"protocol": "TCP"
|
||||
},
|
||||
{
|
||||
"containerPort": 443,
|
||||
"name": "https",
|
||||
"protocol": "TCP"
|
||||
},
|
||||
{
|
||||
"containerPort": 8080,
|
||||
"name": "metrics",
|
||||
"protocol": "TCP"
|
||||
}
|
||||
],
|
||||
"resources": {},
|
||||
"terminationMessagePath": "/dev/termination-log",
|
||||
"terminationMessagePolicy": "File",
|
||||
"imagePullPolicy": "IfNotPresent"
|
||||
},
|
||||
{
|
||||
"name": "sidecar",
|
||||
"image": "busybox:1.35",
|
||||
"command": ["sleep", "3600"],
|
||||
"ports": [
|
||||
{
|
||||
"containerPort": 9090,
|
||||
"name": "sidecar-port",
|
||||
"protocol": "TCP"
|
||||
}
|
||||
],
|
||||
"resources": {},
|
||||
"terminationMessagePath": "/dev/termination-log",
|
||||
"terminationMessagePolicy": "File",
|
||||
"imagePullPolicy": "IfNotPresent"
|
||||
}
|
||||
],
|
||||
"dnsPolicy": "ClusterFirst",
|
||||
"restartPolicy": "Always",
|
||||
"schedulerName": "default-scheduler",
|
||||
"securityContext": {},
|
||||
"terminationGracePeriodSeconds": 30
|
||||
}
|
||||
}
|
||||
},
|
||||
"status": {
|
||||
"availableReplicas": 1,
|
||||
"conditions": [
|
||||
{
|
||||
"type": "Progressing",
|
||||
"status": "True",
|
||||
"lastUpdateTime": "2025-09-19T19:23:04Z",
|
||||
"lastTransitionTime": "2025-09-19T19:23:03Z",
|
||||
"reason": "NewReplicaSetAvailable",
|
||||
"message": "ReplicaSet \"test-container-ports-7bd6dc57d5\" has successfully progressed."
|
||||
},
|
||||
{
|
||||
"type": "Available",
|
||||
"status": "True",
|
||||
"lastUpdateTime": "2025-09-30T17:49:16Z",
|
||||
"lastTransitionTime": "2025-09-30T17:49:16Z",
|
||||
"reason": "MinimumReplicasAvailable",
|
||||
"message": "Deployment has minimum availability."
|
||||
}
|
||||
],
|
||||
"observedGeneration": 2,
|
||||
"readyReplicas": 1,
|
||||
"replicas": 1,
|
||||
"updatedReplicas": 1
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user