diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 1b43ea0ce..597e71f9b 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -258,7 +258,13 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa } if comparison.Modified != nil && !comparison.Modified.Empty() { - liveModValues := typedLive.ExtractItems(comparison.Modified) + comparisonModifiedForMerge := comparison.Modified + typedPredictedLiveFieldSet, err := typedPredictedLive.ToFieldSet() + if err != nil { + return nil, fmt.Errorf("error reverting webhook modified fields in predicted live resource: %s", err) + } + appendKeyFieldsToRhs(typedPredictedLiveFieldSet, comparisonModifiedForMerge) + liveModValues := typedLive.ExtractItems(comparisonModifiedForMerge) // revert modified fields not owned by any manager typedPredictedLive, err = typedPredictedLive.Merge(liveModValues) if err != nil { @@ -267,7 +273,13 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa } if comparison.Removed != nil && !comparison.Removed.Empty() { - liveRmValues := typedLive.ExtractItems(comparison.Removed) + comparisonRemovedForMerge := comparison.Removed + typedPredictedLiveFieldSet, err := typedPredictedLive.ToFieldSet() + if err != nil { + return nil, fmt.Errorf("error reverting webhook removed fields in predicted live resource: %s", err) + } + appendKeyFieldsToRhs(typedPredictedLiveFieldSet, comparisonRemovedForMerge) + liveRmValues := typedLive.ExtractItems(comparisonRemovedForMerge) // revert removed fields not owned by any manager typedPredictedLive, err = typedPredictedLive.Merge(liveRmValues) if err != nil { @@ -283,6 +295,31 @@ func removeWebhookMutation(predictedLive, live *unstructured.Unstructured, gvkPa return &unstructured.Unstructured{Object: pl}, nil } +// appendKeyFieldsToRhs appends the key fields like "name" to the rhs elements which have some entries keyed by those fields. +// The fields are only appended if they are present in lhs as well, otherwise assuming they have a default value not needed +// to be specified explicitly. +// This is needed to merge properly, see https://github.com/argoproj/argo-cd/issues/20792. +func appendKeyFieldsToRhs(lhs *fieldpath.Set, rhs *fieldpath.Set) { + keyFieldPaths := []fieldpath.Path{} + rhs.Iterate(func(path fieldpath.Path) { + for i := 0; i < len(path); i++ { + if path[i].Key != nil { + for _, keyField := range *path[i].Key { + pathPartCopy := make([]fieldpath.PathElement, i+1) + copy(pathPartCopy, path[:i+1]) + newPath := append(pathPartCopy, fieldpath.PathElement{FieldName: &keyField.Name}) + keyFieldPaths = append(keyFieldPaths, newPath) + } + } + } + }) + for _, path := range keyFieldPaths { + if !rhs.Has(path) && lhs.Has(path) { + rhs.Insert(path) + } + } +} + func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error) { res := make(map[string]interface{}) err := json.Unmarshal([]byte(jsonString), &res) diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index 806919a1c..1e204832d 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -28,6 +28,8 @@ import ( "k8s.io/apimachinery/pkg/util/managedfields" "k8s.io/klog/v2/textlogger" openapiproto "k8s.io/kube-openapi/pkg/util/proto" + "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "sigs.k8s.io/structured-merge-diff/v4/value" "sigs.k8s.io/yaml" ) @@ -933,6 +935,31 @@ func TestServerSideDiff(t *testing.T) { assert.Empty(t, liveSVC.Annotations[AnnotationLastAppliedConfig]) assert.Empty(t, predictedSVC.Labels["event"]) }) + + t.Run("will test removing some field with undoing changes done by webhook", func(t *testing.T) { + // given + t.Parallel() + liveState := StrToUnstructured(testdata.Deployment2LiveYAML) + desiredState := StrToUnstructured(testdata.Deployment2ConfigYAML) + opts := buildOpts(testdata.Deployment2PredictedLiveJSONSSD) + + // 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) + assert.Len(t, predictedDeploy.Spec.Template.Spec.Containers, 1) + assert.Len(t, liveDeploy.Spec.Template.Spec.Containers, 1) + assert.Equal(t, "500m", predictedDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Cpu().String()) + assert.Equal(t, "512Mi", predictedDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Memory().String()) + assert.Equal(t, "500m", liveDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Cpu().String()) + assert.Equal(t, "512Mi", liveDeploy.Spec.Template.Spec.Containers[0].Resources.Requests.Memory().String()) + }) + t.Run("will include mutation webhook modifications", func(t *testing.T) { // given t.Parallel() @@ -1319,6 +1346,73 @@ spec: } } +func TestAppendKeyFieldsToRhs(t *testing.T) { + // Paths for the key field "name", simulating a key field with no default value. + lhsPath1, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test1")}}, + "name", + ) + assert.NoError(t, err) + lhsPath2, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test2")}}, + "name", + ) + assert.NoError(t, err) + lhs := fieldpath.NewSet(lhsPath1, lhsPath2) + + rhsPath1, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test1")}}, + "resources", + ) + assert.NoError(t, err) + // Has some key fields which are assumed to have default values like "protocol" + rhsPath2, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test2")}}, + "ports", + &value.FieldList{ + value.Field{Name: "containerPort", Value: value.NewValueInterface(8080)}, + value.Field{Name: "protocol", Value: value.NewValueInterface("TCP")}, + }, + "containerPort", + ) + assert.NoError(t, err) + + rhs := fieldpath.NewSet(rhsPath1, rhsPath2) + + appendKeyFieldsToRhs(lhs, rhs) + + assert.Equal(t, rhs.Size(), 4) + assert.True(t, rhs.Has(rhsPath1)) + assert.True(t, rhs.Has(rhsPath2)) + + // The paths for the field "name" should be added, but not "protocol". + rhsPath3, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test1")}}, + "name", + ) + assert.NoError(t, err) + rhsPath4, err := fieldpath.MakePath( + "spec", + "containers", + &value.FieldList{value.Field{Name: "name", Value: value.NewValueInterface("test2")}}, + "name", + ) + assert.NoError(t, err) + + assert.True(t, rhs.Has(rhsPath3)) + assert.True(t, rhs.Has(rhsPath4)) +} + func ExampleDiff() { expectedResource := unstructured.Unstructured{} if err := yaml.Unmarshal([]byte(` diff --git a/pkg/diff/testdata/data.go b/pkg/diff/testdata/data.go index 047f3190b..fd2a7ae96 100644 --- a/pkg/diff/testdata/data.go +++ b/pkg/diff/testdata/data.go @@ -24,6 +24,15 @@ var ( //go:embed smd-deploy-config.yaml DeploymentConfigYAML string + //go:embed smd-deploy2-live.yaml + Deployment2LiveYAML string + + //go:embed smd-deploy2-config.yaml + Deployment2ConfigYAML string + + //go:embed smd-deploy2-predicted-live.json + Deployment2PredictedLiveJSONSSD string + // OpenAPIV2Doc is a binary representation of the openapi // document available in a given k8s instance. To update // this file the following commands can be executed: diff --git a/pkg/diff/testdata/smd-deploy2-config.yaml b/pkg/diff/testdata/smd-deploy2-config.yaml new file mode 100644 index 000000000..1c141e3e1 --- /dev/null +++ b/pkg/diff/testdata/smd-deploy2-config.yaml @@ -0,0 +1,36 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: missing + applications.argoproj.io/app-name: nginx + something-else: bla + name: nginx-deployment + namespace: default +spec: + replicas: 2 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + applications.argoproj.io/app-name: nginx + spec: + containers: + - image: 'nginx:1.23.1' + imagePullPolicy: Never + livenessProbe: + exec: + command: + - cat + - non-existent-file + initialDelaySeconds: 5 + periodSeconds: 180 + name: nginx + ports: + - containerPort: 8081 + protocol: UDP + - containerPort: 80 + protocol: TCP diff --git a/pkg/diff/testdata/smd-deploy2-live.yaml b/pkg/diff/testdata/smd-deploy2-live.yaml new file mode 100644 index 000000000..94535a85d --- /dev/null +++ b/pkg/diff/testdata/smd-deploy2-live.yaml @@ -0,0 +1,161 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + annotations: + deployment.kubernetes.io/revision: '1' + creationTimestamp: '2022-09-18T23:50:25Z' + generation: 1 + labels: + app: missing + applications.argoproj.io/app-name: nginx + something-else: bla + managedFields: + - apiVersion: apps/v1 + fieldsType: FieldsV1 + fieldsV1: + 'f:metadata': + 'f:labels': + 'f:app': {} + 'f:applications.argoproj.io/app-name': {} + 'f:something-else': {} + 'f:spec': + 'f:replicas': {} + 'f:selector': {} + 'f:template': + 'f:metadata': + 'f:labels': + 'f:app': {} + 'f:applications.argoproj.io/app-name': {} + 'f:spec': + 'f:containers': + 'k:{"name":"nginx"}': + .: {} + 'f:image': {} + 'f:imagePullPolicy': {} + 'f:livenessProbe': + 'f:exec': + 'f:command': {} + 'f:initialDelaySeconds': {} + 'f:periodSeconds': {} + 'f:name': {} + 'f:ports': + 'k:{"containerPort":80,"protocol":"TCP"}': + .: {} + 'f:containerPort': {} + 'f:protocol': {} + 'f:resources': + 'f:requests': + 'f:cpu': {} + 'f:memory': {} + manager: argocd-controller + operation: Apply + time: '2022-09-18T23:50:25Z' + - 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: '2022-09-23T18:30:59Z' + name: nginx-deployment + namespace: default + resourceVersion: '7492752' + uid: 731f7434-d3d9-47fa-b179-d9368a84f7c9 +spec: + progressDeadlineSeconds: 600 + replicas: 2 + revisionHistoryLimit: 10 + selector: + matchLabels: + app: nginx + strategy: + rollingUpdate: + maxSurge: 25% + maxUnavailable: 25% + type: RollingUpdate + template: + metadata: + creationTimestamp: null + labels: + app: nginx + applications.argoproj.io/app-name: nginx + spec: + containers: + - image: 'nginx:1.23.1' + imagePullPolicy: Never + livenessProbe: + exec: + command: + - cat + - non-existent-file + failureThreshold: 3 + initialDelaySeconds: 5 + periodSeconds: 180 + successThreshold: 1 + timeoutSeconds: 1 + name: nginx + ports: + - containerPort: 80 + protocol: TCP + - containerPort: 8080 + protocol: TCP + - containerPort: 8081 + protocol: UDP + resources: + requests: + memory: 512Mi + cpu: 500m + terminationMessagePath: /dev/termination-log + terminationMessagePolicy: File + dnsPolicy: ClusterFirst + restartPolicy: Always + schedulerName: default-scheduler + securityContext: {} + terminationGracePeriodSeconds: 30 +status: + availableReplicas: 2 + conditions: + - lastTransitionTime: '2022-09-18T23:50:25Z' + lastUpdateTime: '2022-09-18T23:50:26Z' + message: ReplicaSet "nginx-deployment-6d68ff5f86" has successfully progressed. + reason: NewReplicaSetAvailable + status: 'True' + type: Progressing + - lastTransitionTime: '2022-09-23T18:30:59Z' + lastUpdateTime: '2022-09-23T18:30:59Z' + message: Deployment has minimum availability. + reason: MinimumReplicasAvailable + status: 'True' + type: Available + observedGeneration: 1 + readyReplicas: 2 + replicas: 2 + updatedReplicas: 2 diff --git a/pkg/diff/testdata/smd-deploy2-predicted-live.json b/pkg/diff/testdata/smd-deploy2-predicted-live.json new file mode 100644 index 000000000..8f6f26874 --- /dev/null +++ b/pkg/diff/testdata/smd-deploy2-predicted-live.json @@ -0,0 +1,124 @@ +{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": { + "labels": { + "app": "missing", + "applications.argoproj.io/app-name": "nginx", + "something-else": "bla" + }, + "name": "nginx-deployment", + "namespace": "default", + "managedFields": [ + { + "apiVersion": "apps/v1", + "fieldsType": "FieldsV1", + "fieldsV1": { + "f:metadata": { + "f:labels": { + "f:app": {}, + "f:applications.argoproj.io/app-name": {}, + "f:something-else": {} + } + }, + "f:spec": { + "f:replicas": {}, + "f:selector": {}, + "f:template": { + "f:metadata": { + "f:labels": { + "f:app": {}, + "f:applications.argoproj.io/app-name": {} + } + }, + "f:spec": { + "f:containers": { + "k:{\"name\":\"nginx\"}": { + ".": {}, + "f:image": {}, + "f:imagePullPolicy": {}, + "f:livenessProbe": { + "f:exec": { + "f:command": {} + }, + "f:initialDelaySeconds": {}, + "f:periodSeconds": {} + }, + "f:name": {}, + "f:ports": { + "k:{\"containerPort\":80,\"protocol\":\"TCP\"}": { + ".": {}, + "f:containerPort": {}, + "f:protocol": {} + } + }, + "f:resources": { + "f:requests": { + "f:cpu": {}, + "f:memory": {} + } + } + } + } + } + } + } + }, + "manager": "argocd-controller", + "operation": "Apply", + "time": "2022-09-18T23:50:25Z" + } + ] + }, + "spec": { + "replicas": 2, + "selector": { + "matchLabels": { + "app": "nginx" + } + }, + "template": { + "metadata": { + "labels": { + "app": "nginx", + "applications.argoproj.io/app-name": "nginx" + } + }, + "spec": { + "containers": [ + { + "image": "nginx:1.23.1", + "imagePullPolicy": "Never", + "livenessProbe": { + "exec": { + "command": [ + "cat", + "non-existent-file" + ] + }, + "initialDelaySeconds": 5, + "periodSeconds": 180 + }, + "name": "nginx", + "ports": [ + { + "containerPort": 8081, + "protocol": "UDP" + }, + { + "containerPort": 80, + "protocol": "TCP" + } + ], + "resources": { + "requests": { + "memory": "512Mi", + "cpu": "500m" + } + } + } + ] + } + } + } +} diff --git a/sonar-project.properties b/sonar-project.properties new file mode 100644 index 000000000..8e22c6ed5 --- /dev/null +++ b/sonar-project.properties @@ -0,0 +1,2 @@ +# Exclude test manifests from analysis +sonar.kubernetes.exclusions=pkg/diff/testdata/**