diff --git a/config/samples/dataplane-konnect.yaml b/config/samples/dataplane-konnect.yaml index a296f8ed5..ac77ac712 100644 --- a/config/samples/dataplane-konnect.yaml +++ b/config/samples/dataplane-konnect.yaml @@ -66,16 +66,10 @@ spec: initialDelaySeconds: 1 periodSeconds: 1 volumeMounts: - # We need to specify the cluster-certificate volume-mount because otherwise - # strategic merge patch would merge its entry with provided - # konnect-client-tls volume mount. - - name: cluster-certificate - mountPath: /var/cluster-certificate - name: konnect-client-tls mountPath: /etc/secrets/kong-cluster-cert/ readOnly: true volumes: - - name: cluster-certificate - name: konnect-client-tls secret: secretName: konnect-client-tls diff --git a/config/samples/dataplane-sidecar.yaml b/config/samples/dataplane-sidecar.yaml index 00bf32c13..efcc0df48 100644 --- a/config/samples/dataplane-sidecar.yaml +++ b/config/samples/dataplane-sidecar.yaml @@ -26,7 +26,6 @@ spec: dataplane-pod-annotation: example spec: volumes: - - name: cluster-certificate - name: sidecar-vector-config-volume configMap: name: sidecar-vector-config @@ -41,9 +40,6 @@ spec: mountPath: /etc/vector - name: proxy-logs mountPath: /etc/kong/log/ - readinessProbe: - initialDelaySeconds: 1 - periodSeconds: 1 - name: proxy image: kong:3.6 volumeMounts: diff --git a/controller/controlplane/controller_utils_test.go b/controller/controlplane/controller_utils_test.go index 04e4015b6..2729b992a 100644 --- a/controller/controlplane/controller_utils_test.go +++ b/controller/controlplane/controller_utils_test.go @@ -917,6 +917,87 @@ func TestControlPlaneSpecDeepEqual(t *testing.T) { }, equal: true, }, + { + name: "not matching Extensions, different length", + spec1: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test", + }, + }, + }, + }, + spec2: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test", + }, + }, + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test2", + }, + }, + }, + }, + equal: false, + }, + { + name: "matching Extensions, different order", + spec1: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test", + }, + }, + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test2", + }, + }, + }, + }, + spec2: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test2", + }, + }, + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test", + }, + }, + }, + }, + equal: false, + }, + { + name: "not matching Extensions, different names", + spec1: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test", + }, + }, + }, + }, + spec2: &operatorv1beta1.ControlPlaneOptions{ + Extensions: []operatorv1alpha1.ExtensionRef{ + { + NamespacedRef: operatorv1alpha1.NamespacedRef{ + Name: "test2", + }, + }, + }, + }, + equal: false, + }, } for _, tc := range testCases { diff --git a/controller/dataplane/controller_reconciler_utils_test.go b/controller/dataplane/controller_reconciler_utils_test.go index ffd98ad18..385a43661 100644 --- a/controller/dataplane/controller_reconciler_utils_test.go +++ b/controller/dataplane/controller_reconciler_utils_test.go @@ -104,11 +104,6 @@ func TestDeploymentBuilder(t *testing.T) { PodTemplateSpec: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Volumes: []corev1.Volume{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, - }, { Name: "test-volume", VolumeSource: corev1.VolumeSource{ @@ -122,10 +117,6 @@ func TestDeploymentBuilder(t *testing.T) { { Name: consts.DataPlaneProxyContainerName, VolumeMounts: []corev1.VolumeMount{ - { - Name: consts.ClusterCertificateVolume, - MountPath: consts.ClusterCertificateVolumeMountPath, - }, { Name: "test-volume", MountPath: "/var/test/", @@ -187,16 +178,20 @@ func TestDeploymentBuilder(t *testing.T) { k8sresources.SetDefaultsVolume(&testVolume) testVolume.Name = "test-volume" testVolume.VolumeSource.Secret.SecretName = "test-secret" + require.Equal(t, + []corev1.Volume{testVolume, certificateVolume}, + deployment.Spec.Template.Spec.Volumes, + ) require.Equal(t, []corev1.VolumeMount{ { - Name: consts.ClusterCertificateVolume, - MountPath: consts.ClusterCertificateVolumeMountPath, + Name: "test-volume", + MountPath: "/var/test/", ReadOnly: true, }, { - Name: "test-volume", - MountPath: "/var/test/", + Name: consts.ClusterCertificateVolume, + MountPath: consts.ClusterCertificateVolumeMountPath, ReadOnly: true, }, }, diff --git a/pkg/utils/kubernetes/resources/deployments.go b/pkg/utils/kubernetes/resources/deployments.go index 79d14647d..027f62724 100644 --- a/pkg/utils/kubernetes/resources/deployments.go +++ b/pkg/utils/kubernetes/resources/deployments.go @@ -101,7 +101,6 @@ func GenerateNewDeploymentForControlPlane(params GenerateNewDeploymentForControl }, }, } - SetDefaultsPodTemplateSpec(&deployment.Spec.Template) LabelObjectAsControlPlaneManaged(deployment) if params.ControlPlane.Spec.Deployment.PodTemplateSpec != nil { diff --git a/pkg/utils/kubernetes/resources/strategicmerge.go b/pkg/utils/kubernetes/resources/strategicmerge.go index 1d5b5904e..93b9e7e1c 100644 --- a/pkg/utils/kubernetes/resources/strategicmerge.go +++ b/pkg/utils/kubernetes/resources/strategicmerge.go @@ -6,6 +6,7 @@ import ( "github.com/goccy/go-json" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/mergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" pkgapiscorev1 "k8s.io/kubernetes/pkg/apis/core/v1" @@ -19,24 +20,51 @@ func StrategicMergePatchPodTemplateSpec(base, patch *corev1.PodTemplateSpec) (*c return base, nil } + // NOTE: this is needed because without it the patch will wipe out the containers from the base. + if len(patch.Spec.Containers) == 0 { + patch.Spec.Containers = base.Spec.Containers + } + + SetDefaultsPodTemplateSpec(base) baseBytes, err := json.Marshal(base) if err != nil { return nil, fmt.Errorf("failed to marshal JSON for base %s: %w", base.Name, err) } + baseMap := map[string]interface{}{} + if err := json.Unmarshal(baseBytes, &baseMap); err != nil { + return nil, mergepatch.ErrBadJSONDoc + } SetDefaultsPodTemplateSpec(patch) patchBytes, err := json.Marshal(patch) if err != nil { return nil, fmt.Errorf("failed to marshal JSON for patch %s: %w", patch.Name, err) } + patchMap := map[string]interface{}{} + if err := json.Unmarshal(patchBytes, &patchMap); err != nil { + return nil, mergepatch.ErrBadJSONDoc + } + schema, err := strategicpatch.NewPatchMetaFromStruct(&corev1.PodTemplateSpec{}) + if err != nil { + return nil, err + } - // Calculate the patch result. - jsonResultBytes, err := strategicpatch.StrategicMergePatch(baseBytes, patchBytes, &corev1.PodTemplateSpec{}) + out, err := CreateTwoWayMergeMapPatchUsingLookupPatchMeta(baseMap, patchMap, schema) + if err != nil { + return nil, err + } + + outMap, err := strategicpatch.StrategicMergeMapPatch(baseMap, out, &corev1.PodTemplateSpec{}) if err != nil { return nil, fmt.Errorf("failed to generate merge patch for %s: %w", base.Name, err) } - patchResult := base.DeepCopy() + jsonResultBytes, err := json.Marshal(outMap) + if err != nil { + return nil, err + } + + patchResult := &corev1.PodTemplateSpec{} if err := json.Unmarshal(jsonResultBytes, patchResult); err != nil { return nil, fmt.Errorf("failed to unmarshal merged %s: %w", base.Name, err) } diff --git a/pkg/utils/kubernetes/resources/strategicmerge_test.go b/pkg/utils/kubernetes/resources/strategicmerge_test.go index 1b5ef1022..daad87f9c 100644 --- a/pkg/utils/kubernetes/resources/strategicmerge_test.go +++ b/pkg/utils/kubernetes/resources/strategicmerge_test.go @@ -14,6 +14,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + pkgapiscorev1 "k8s.io/kubernetes/pkg/apis/core/v1" operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1" "github.com/kong/gateway-operator/pkg/consts" @@ -54,9 +55,53 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }..., ) + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d, nil } + clusterCertificateVolume := func() corev1.Volume { + v := corev1.Volume{ + Name: consts.ClusterCertificateVolume, + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: "kong-cert-secret", + Items: []corev1.KeyToPath{ + { + Key: "tls.crt", + Path: "tls.crt", + }, + { + Key: "tls.key", + Path: "tls.key", + }, + { + Key: "ca.crt", + Path: "ca.crt", + }, + }, + }, + }, + } + + pkgapiscorev1.SetDefaults_Volume(&v) + return v + } + + clusterCertificateVolumeMount := func() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: consts.ClusterCertificateVolume, + MountPath: consts.ClusterCertificateVolumeMountPath, + ReadOnly: true, + } + } + admissionWebhookVolumeMount := func() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: consts.ControlPlaneAdmissionWebhookVolumeName, + ReadOnly: true, + MountPath: consts.ControlPlaneAdmissionWebhookVolumeMountPath, + } + } + testcases := []struct { Name string Patch *corev1.PodTemplateSpec @@ -198,7 +243,7 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, { - Name: "overwrite and add env vars", + Name: "overwrite 1 base env value and add 1 new env var", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -243,7 +288,7 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, { - Name: "add and overwrite env vars", + Name: "add 1 new env var and overwrite 1 base env value", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -288,20 +333,66 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, { - Name: "append a volume and volume mount", + Name: "patch volume and volume mount prepends them", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Volumes: []corev1.Volume{ { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, + Name: "volume1", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{ + SizeLimit: resource.NewQuantity(1000, resource.DecimalSI), + }, + }, }, + }, + Containers: []corev1.Container{ { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, + Name: "controller", + VolumeMounts: []corev1.VolumeMount{ + { + Name: "volume1", + MountPath: "/volume1", + }, + }, + }, + }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + d.Spec.Template.Spec.Volumes = []corev1.Volume{ + { + Name: "volume1", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{ + SizeLimit: resource.NewQuantity(1000, resource.DecimalSI), + }, }, + }, + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + } + + d.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + { + Name: "volume1", + MountPath: "/volume1", + }, + clusterCertificateVolumeMount(), + admissionWebhookVolumeMount(), + } + + SetDefaultsPodTemplateSpec(&d.Spec.Template) + return d.Spec.Template + }, + }, + { + Name: "patch volume and volume mount restating the base works", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ { Name: "volume1", VolumeSource: corev1.VolumeSource{ @@ -315,18 +406,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { { Name: "controller", VolumeMounts: []corev1.VolumeMount{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, - MountPath: consts.ClusterCertificateVolumeMountPath, - }, - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, - MountPath: consts.ControlPlaneAdmissionWebhookVolumeMountPath, - }, { Name: "volume1", MountPath: "/volume1", @@ -339,22 +418,29 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { Expected: func() corev1.PodTemplateSpec { d, err := makeControlPlaneDeployment() require.NoError(t, err) - d.Spec.Template.Spec.Volumes = append(d.Spec.Template.Spec.Volumes, - corev1.Volume{ + d.Spec.Template.Spec.Volumes = []corev1.Volume{ + { Name: "volume1", VolumeSource: corev1.VolumeSource{ EmptyDir: &corev1.EmptyDirVolumeSource{ SizeLimit: resource.NewQuantity(1000, resource.DecimalSI), }, }, - }) - d.Spec.Template.Spec.Containers[0].VolumeMounts = append(d.Spec.Template.Spec.Containers[0].VolumeMounts, - corev1.VolumeMount{ + }, + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + } + + d.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + { Name: "volume1", MountPath: "/volume1", }, - ) + clusterCertificateVolumeMount(), + admissionWebhookVolumeMount(), + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, @@ -363,11 +449,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: "controller", - }, { Name: "sidecar", Image: "alpine", @@ -400,21 +481,19 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, } - SetDefaultsContainer(&sidecarContainer) - d.Spec.Template.Spec.Containers = append(d.Spec.Template.Spec.Containers, sidecarContainer) + d.Spec.Template.Spec.Containers = []corev1.Container{ + sidecarContainer, + d.Spec.Template.Spec.Containers[0], + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, { - Name: "append a sidecar and a volume mount", + Name: "append a sidecar and a volume", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: "controller", - }, { Name: "sidecar", Image: "alpine", @@ -436,16 +515,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, Volumes: []corev1.Volume{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, - }, - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, - }, { Name: "new_volume", VolumeSource: corev1.VolumeSource{ @@ -479,17 +548,127 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, } - SetDefaultsContainer(&sidecarContainer) - d.Spec.Template.Spec.Containers = append(d.Spec.Template.Spec.Containers, sidecarContainer) - d.Spec.Template.Spec.Volumes = append(d.Spec.Template.Spec.Volumes, corev1.Volume{ - Name: "new_volume", - VolumeSource: corev1.VolumeSource{ - HostPath: &corev1.HostPathVolumeSource{ - Path: "/host/path", - Type: lo.ToPtr(corev1.HostPathUnset), + d.Spec.Template.Spec.Containers = append([]corev1.Container{sidecarContainer}, d.Spec.Template.Spec.Containers...) + d.Spec.Template.Spec.Volumes = []corev1.Volume{ + { + Name: "new_volume", + VolumeSource: corev1.VolumeSource{ + HostPath: &corev1.HostPathVolumeSource{ + Path: "/host/path", + Type: lo.ToPtr(corev1.HostPathUnset), + }, }, }, - }) + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + return d.Spec.Template + }, + }, + { + Name: "append a sidecar and change controller's image", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "sidecar", + Image: "alpine", + Command: []string{ + "sleep", "1000", + }, + Env: []corev1.EnvVar{ + { + Name: "SIDECAR_ENV1", + Value: "SIDECAR_VALUE1", + }, + }, + }, + { + Name: "controller", + Image: "custom:1.0", + }, + }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + + require.Len(t, d.Spec.Template.Spec.Containers, 1) + d.Spec.Template.Spec.Containers[0].Image = "custom:1.0" + + sidecarContainer := corev1.Container{ + Name: "sidecar", + Image: "alpine", + Command: []string{ + "sleep", "1000", + }, + Env: []corev1.EnvVar{ + { + Name: "SIDECAR_ENV1", + Value: "SIDECAR_VALUE1", + }, + }, + } + d.Spec.Template.Spec.Containers = []corev1.Container{ + sidecarContainer, + d.Spec.Template.Spec.Containers[0], + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + return d.Spec.Template + }, + }, + { + Name: "append a sidecar and change controller's image, define sidecar second", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "controller", + Image: "custom:1.0", + }, + { + Name: "sidecar", + Image: "alpine", + Command: []string{ + "sleep", "1000", + }, + Env: []corev1.EnvVar{ + { + Name: "SIDECAR_ENV1", + Value: "SIDECAR_VALUE1", + }, + }, + }, + }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + + require.Len(t, d.Spec.Template.Spec.Containers, 1) + d.Spec.Template.Spec.Containers[0].Image = "custom:1.0" + + sidecarContainer := corev1.Container{ + Name: "sidecar", + Image: "alpine", + Command: []string{ + "sleep", "1000", + }, + Env: []corev1.EnvVar{ + { + Name: "SIDECAR_ENV1", + Value: "SIDECAR_VALUE1", + }, + }, + } + d.Spec.Template.Spec.Containers = []corev1.Container{ + d.Spec.Template.Spec.Containers[0], + sidecarContainer, + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, @@ -557,49 +736,122 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, { - Name: "append a secret volume and volume mount", + Name: "restating the base volumes and volume mounts does not work", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + // NOTE: This was required in 1.2.x and older versions of the operator. + // Now this only checks that this approach still works. + Name: consts.ClusterCertificateVolume, + // 1.3.x introduced a change in how strategic merge patch is applied for PodTemplateSpec + // and the only discovered "breaking change" is that volume entries missing the + // volume source will result in removing the volume source from the base. + // Including the volume source in the patch (even empty like below) will keep the volume source. + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{}, + }, + }, + { + // NOTE: This was required in 1.2.x and older versions of the operator. + // Now this only checks that this approach still works. + Name: consts.ControlPlaneAdmissionWebhookVolumeName, + // 1.3.x introduced a change in how strategic merge patch is applied for PodTemplateSpec + // and the only discovered "breaking change" is that volume entries missing the + // volume source will result in removing the volume source from the base. + // Including the volume source in the patch (even empty like below) will keep the volume source. + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{}, + }, + }, + { + Name: "volume1", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "configmap-1", + }, + }, + }, + }, + }, Containers: []corev1.Container{ { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. Name: "controller", VolumeMounts: []corev1.VolumeMount{ { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. + // NOTE: This was required in 1.2.x and older versions of the operator. + // Now this only checks that this approach still works. Name: consts.ClusterCertificateVolume, MountPath: consts.ClusterCertificateVolumeMountPath, }, { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. + // NOTE: This was required in 1.2.x and older versions of the operator. + // Now this only checks that this approach still works. Name: consts.ControlPlaneAdmissionWebhookVolumeName, MountPath: consts.ControlPlaneAdmissionWebhookVolumeMountPath, }, { - Name: "new_volume", - MountPath: "/new_volume", + Name: "volume1", + MountPath: "/volume1", }, }, }, }, - Volumes: []corev1.Volume{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + volume := corev1.Volume{ + Name: "volume1", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "configmap-1", + }, }, + }, + } + + d.Spec.Template.Spec.Volumes = []corev1.Volume{ + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + volume, + } + d.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + clusterCertificateVolumeMount(), + admissionWebhookVolumeMount(), + { + Name: "volume1", + MountPath: "/volume1", + }, + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + return d.Spec.Template + }, + }, + { + Name: "append a secret volume and volume mount", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, + Name: "controller", + VolumeMounts: []corev1.VolumeMount{ + { + Name: "new_volume", + MountPath: "/new_volume", + }, + }, }, + }, + Volumes: []corev1.Volume{ { Name: "new_volume", VolumeSource: corev1.VolumeSource{ @@ -622,14 +874,21 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, } - SetDefaultsVolume(&volume) - d.Spec.Template.Spec.Volumes = append(d.Spec.Template.Spec.Volumes, volume) - require.Len(t, d.Spec.Template.Spec.Containers, 1) - d.Spec.Template.Spec.Containers[0].VolumeMounts = append(d.Spec.Template.Spec.Containers[0].VolumeMounts, - corev1.VolumeMount{ + + d.Spec.Template.Spec.Volumes = []corev1.Volume{ + volume, + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + } + d.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + { Name: "new_volume", MountPath: "/new_volume", - }) + }, + clusterCertificateVolumeMount(), + admissionWebhookVolumeMount(), + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, @@ -641,18 +900,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { { Name: consts.ControlPlaneControllerContainerName, VolumeMounts: []corev1.VolumeMount{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, - MountPath: consts.ClusterCertificateVolumeMountPath, - }, - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, - MountPath: consts.ControlPlaneAdmissionWebhookVolumeMountPath, - }, { Name: "hostpath-volumemount", MountPath: "/var/log/hostpath", @@ -661,16 +908,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, Volumes: []corev1.Volume{ - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ClusterCertificateVolume, - }, - { - // NOTE: we need to provide the existing entry in the slice - // to prevent merging the provided new entry with existing entries. - Name: consts.ControlPlaneAdmissionWebhookVolumeName, - }, { Name: "hostpath-volume", VolumeSource: corev1.VolumeSource{ @@ -687,70 +924,148 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { require.NoError(t, err) d.Spec.Template.Spec.Volumes = []corev1.Volume{ { - Name: "cluster-certificate", + Name: "hostpath-volume", VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: "kong-cert-secret", - Items: []corev1.KeyToPath{ - { - Key: "tls.crt", - Path: "tls.crt", - }, - { - Key: "tls.key", - Path: "tls.key", - }, - { - Key: "ca.crt", - Path: "ca.crt", + HostPath: &corev1.HostPathVolumeSource{ + Path: "/var/log", + Type: lo.ToPtr(corev1.HostPathUnset), + }, + }, + }, + clusterCertificateVolume(), + controlPlaneAdmissionWebhookCertificateVolume("kong-admission-cert-secret"), + } + d.Spec.Template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ + { + Name: "hostpath-volumemount", + MountPath: "/var/log/hostpath", + }, + clusterCertificateVolumeMount(), + admissionWebhookVolumeMount(), + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + + return d.Spec.Template + }, + }, + { + Name: "add envs with fieldRef prepends it", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: consts.ControlPlaneControllerContainerName, + Env: []corev1.EnvVar{ + { + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + }, }, }, - DefaultMode: lo.ToPtr(corev1.SecretVolumeSourceDefaultMode), }, }, }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + d.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{ { - Name: consts.ControlPlaneAdmissionWebhookVolumeName, - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: "kong-admission-cert-secret", - Items: []corev1.KeyToPath{ - { - Key: "tls.crt", - Path: "tls.crt", - }, - { - Key: "tls.key", - Path: "tls.key", + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + Divisor: *resource.NewQuantity(1, resource.DecimalSI), + }, + }, + }, + { + Name: "ENV1", + Value: "VALUE1", + }, + { + Name: "ENV2", + Value: "VALUE2", + }, + { + Name: "ENV3", + Value: "VALUE3", + }, + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + + return d.Spec.Template + }, + }, + { + Name: "add envs with fieldRef re-stating the base values", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: consts.ControlPlaneControllerContainerName, + Env: []corev1.EnvVar{ + { + Name: "ENV1", + Value: "VALUE1", + }, + { + Name: "ENV2", + Value: "VALUE2", + }, + { + Name: "ENV3", + Value: "VALUE3", + }, + { + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + }, }, }, - DefaultMode: lo.ToPtr(corev1.SecretVolumeSourceDefaultMode), }, }, }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + d.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{ { - Name: "hostpath-volume", - VolumeSource: corev1.VolumeSource{ - HostPath: &corev1.HostPathVolumeSource{ - Path: "/var/log", - Type: lo.ToPtr(corev1.HostPathUnset), + Name: "ENV1", + Value: "VALUE1", + }, + { + Name: "ENV2", + Value: "VALUE2", + }, + { + Name: "ENV3", + Value: "VALUE3", + }, + { + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + Divisor: *resource.NewQuantity(1, resource.DecimalSI), }, }, }, } - d.Spec.Template.Spec.Containers[0].VolumeMounts = append( - d.Spec.Template.Spec.Containers[0].VolumeMounts, - corev1.VolumeMount{ - Name: "hostpath-volumemount", - MountPath: "/var/log/hostpath", - }, - ) + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, { - Name: "add envs with fieldRef", + Name: "add env without restating the base prepends the new env", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -776,11 +1091,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { d.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{ { Name: "LIMIT", - // NOTE: this is an artifact of the strategic merge patch at work. - // This values comes from the first entry in the base patch. - // In order to overcome this we need to re-state the base values - // in the slice. - Value: "VALUE1", ValueFrom: &corev1.EnvVarSource{ ResourceFieldRef: &corev1.ResourceFieldSelector{ Resource: "limits.cpu", @@ -801,12 +1111,13 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { Value: "VALUE3", }, } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, }, { - Name: "add envs with fieldRef re-stating the base", + Name: "add env with restating the base first, appends the new env", Patch: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -825,7 +1136,6 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { Name: "ENV3", Value: "VALUE3", }, - // We're moving the `LIMIT` entry from the 1st to the 4th place in the slice to make sure it will not end up with both `Value` and `ValueFrom` fields. { Name: "LIMIT", ValueFrom: &corev1.EnvVarSource{ @@ -865,6 +1175,71 @@ func TestStrategicMergePatchPodTemplateSpec(t *testing.T) { }, }, } + SetDefaultsPodTemplateSpec(&d.Spec.Template) + + return d.Spec.Template + }, + }, + { + Name: "add env and change the order of the env vars", + Patch: &corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: consts.ControlPlaneControllerContainerName, + Env: []corev1.EnvVar{ + { + Name: "ENV1", + Value: "VALUE1", + }, + { + Name: "ENV3", + Value: "VALUE3", + }, + { + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + }, + }, + }, + { + Name: "ENV2", + Value: "XXX", + }, + }, + }, + }, + }, + }, + Expected: func() corev1.PodTemplateSpec { + d, err := makeControlPlaneDeployment() + require.NoError(t, err) + d.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{ + { + Name: "ENV1", + Value: "VALUE1", + }, + { + Name: "ENV3", + Value: "VALUE3", + }, + { + Name: "LIMIT", + ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{ + Resource: "limits.cpu", + Divisor: *resource.NewQuantity(1, resource.DecimalSI), + }, + }, + }, + { + Name: "ENV2", + Value: "XXX", + }, + } + SetDefaultsPodTemplateSpec(&d.Spec.Template) return d.Spec.Template }, diff --git a/pkg/utils/kubernetes/resources/strategicmerge_twoway.go b/pkg/utils/kubernetes/resources/strategicmerge_twoway.go new file mode 100644 index 000000000..a5952a661 --- /dev/null +++ b/pkg/utils/kubernetes/resources/strategicmerge_twoway.go @@ -0,0 +1,934 @@ +// This is a copy from https://github.com/kubernetes/apimachinery/blob/v0.29.2/pkg/util/strategicpatch/patch.go#L139 +// to make one small change which is not allowed by the API, namely: to set the diff option to ignore deletions. +// +//nolint:gocritic,revive +package resources + +import ( + "fmt" + "reflect" + "sort" + "strings" + + "k8s.io/apimachinery/pkg/util/mergepatch" + "k8s.io/apimachinery/pkg/util/strategicpatch" +) + +const ( + directiveMarker = "$patch" + deleteDirective = "delete" + replaceDirective = "replace" + mergeDirective = "merge" + + retainKeysStrategy = "retainKeys" + + deleteFromPrimitiveListDirectivePrefix = "$deleteFromPrimitiveList" + retainKeysDirective = "$" + retainKeysStrategy + setElementOrderDirectivePrefix = "$setElementOrder" +) + +// CreateTwoWayMergeMapPatchUsingLookupPatchMeta is a copy of +// strategicpatch.CreateTwoWayMergeMapPatchUsingLookupPatchMeta with IgnoreDeletions +// set to true. +func CreateTwoWayMergeMapPatchUsingLookupPatchMeta( + original, modified strategicpatch.JSONMap, + schema strategicpatch.LookupPatchMeta, + fns ...mergepatch.PreconditionFunc, +) (strategicpatch.JSONMap, error) { + diffOptions := strategicpatch.DiffOptions{ + SetElementOrder: true, + // NOTE: This is the only change made in this package. + IgnoreDeletions: true, + } + patchMap, err := diffMaps(original, modified, schema, diffOptions) + if err != nil { + return nil, err + } + + // Apply the preconditions to the patch, and return an error if any of them fail. + for _, fn := range fns { + if !fn(patchMap) { + return nil, mergepatch.NewErrPreconditionFailed(patchMap) + } + } + + return patchMap, nil +} + +// Returns a (recursive) strategic merge patch that yields modified when applied to original. +// Including: +// - Adding fields to the patch present in modified, missing from original +// - Setting fields to the patch present in modified and original with different values +// - Delete fields present in original, missing from modified through +// - IFF map field - set to nil in patch +// - IFF list of maps && merge strategy - use deleteDirective for the elements +// - IFF list of primitives && merge strategy - use parallel deletion list +// - IFF list of maps or primitives with replace strategy (default) - set patch value to the value in modified +// - Build $retainKeys directive for fields with retainKeys patch strategy +func diffMaps( + original, modified map[string]interface{}, + schema strategicpatch.LookupPatchMeta, + diffOptions strategicpatch.DiffOptions, +) (map[string]interface{}, error) { + patch := map[string]interface{}{} + + // This will be used to build the $retainKeys directive sent in the patch + retainKeysList := make([]interface{}, 0, len(modified)) + + // Compare each value in the modified map against the value in the original map + for key, modifiedValue := range modified { + // Get the underlying type for pointers + if diffOptions.BuildRetainKeysDirective && modifiedValue != nil { + retainKeysList = append(retainKeysList, key) + } + + originalValue, ok := original[key] + if !ok { + // Key was added, so add to patch + if !diffOptions.IgnoreChangesAndAdditions { + patch[key] = modifiedValue + } + continue + } + + // The patch may have a patch directive + // TODO: figure out if we need this. This shouldn't be needed by apply. When would the original map have patch directives in it? + foundDirectiveMarker, err := handleDirectiveMarker(key, originalValue, modifiedValue, patch) + if err != nil { + return nil, err + } + if foundDirectiveMarker { + continue + } + + if reflect.TypeOf(originalValue) != reflect.TypeOf(modifiedValue) { + // Types have changed, so add to patch + if !diffOptions.IgnoreChangesAndAdditions { + patch[key] = modifiedValue + } + continue + } + + // Types are the same, so compare values + switch originalValueTyped := originalValue.(type) { + case map[string]interface{}: + modifiedValueTyped := modifiedValue.(map[string]interface{}) + err = handleMapDiff(key, originalValueTyped, modifiedValueTyped, patch, schema, diffOptions) + case []interface{}: + modifiedValueTyped := modifiedValue.([]interface{}) + err = handleSliceDiff(key, originalValueTyped, modifiedValueTyped, patch, schema, diffOptions) + default: + replacePatchFieldIfNotEqual(key, originalValue, modifiedValue, patch, diffOptions) + } + if err != nil { + return nil, err + } + } + + updatePatchIfMissing(original, modified, patch, diffOptions) + // Insert the retainKeysList iff there are values present in the retainKeysList and + // either of the following is true: + // - the patch is not empty + // - there are additional field in original that need to be cleared + if len(retainKeysList) > 0 && + (len(patch) > 0 || hasAdditionalNewField(original, modified)) { + patch[retainKeysDirective] = sortScalars(retainKeysList) + } + return patch, nil +} + +// handleDirectiveMarker handles how to diff directive marker between 2 objects +func handleDirectiveMarker(key string, originalValue, modifiedValue interface{}, patch map[string]interface{}) (bool, error) { + if key == directiveMarker { + originalString, ok := originalValue.(string) + if !ok { + return false, fmt.Errorf("invalid value for special key: %s", directiveMarker) + } + modifiedString, ok := modifiedValue.(string) + if !ok { + return false, fmt.Errorf("invalid value for special key: %s", directiveMarker) + } + if modifiedString != originalString { + patch[directiveMarker] = modifiedValue + } + return true, nil + } + return false, nil +} + +// handleMapDiff diff between 2 maps `originalValueTyped` and `modifiedValue`, +// puts the diff in the `patch` associated with `key` +// key is the key associated with originalValue and modifiedValue. +// originalValue, modifiedValue are the old and new value respectively.They are both maps +// patch is the patch map that contains key and the updated value, and it is the parent of originalValue, modifiedValue +// diffOptions contains multiple options to control how we do the diff. +func handleMapDiff(key string, originalValue, modifiedValue, patch map[string]interface{}, + schema strategicpatch.LookupPatchMeta, + diffOptions strategicpatch.DiffOptions, +) error { + subschema, patchMeta, err := schema.LookupPatchMetadataForStruct(key) + if err != nil { + // We couldn't look up metadata for the field + // If the values are identical, this doesn't matter, no patch is needed + if reflect.DeepEqual(originalValue, modifiedValue) { + return nil + } + // Otherwise, return the error + return err + } + retainKeys, patchStrategy, err := extractRetainKeysPatchStrategy(patchMeta.GetPatchStrategies()) + if err != nil { + return err + } + diffOptions.BuildRetainKeysDirective = retainKeys + switch patchStrategy { + // The patch strategic from metadata tells us to replace the entire object instead of diffing it + case replaceDirective: + if !diffOptions.IgnoreChangesAndAdditions { + patch[key] = modifiedValue + } + default: + patchValue, err := diffMaps(originalValue, modifiedValue, subschema, diffOptions) + if err != nil { + return err + } + // Maps were not identical, use provided patch value + if len(patchValue) > 0 { + patch[key] = patchValue + } + } + return nil +} + +// handleSliceDiff diff between 2 slices `originalValueTyped` and `modifiedValue`, +// puts the diff in the `patch` associated with `key` +// key is the key associated with originalValue and modifiedValue. +// originalValue, modifiedValue are the old and new value respectively.They are both slices +// patch is the patch map that contains key and the updated value, and it is the parent of originalValue, modifiedValue +// diffOptions contains multiple options to control how we do the diff. +func handleSliceDiff(key string, originalValue, modifiedValue []interface{}, patch map[string]interface{}, + schema strategicpatch.LookupPatchMeta, + diffOptions strategicpatch.DiffOptions, +) error { + subschema, patchMeta, err := schema.LookupPatchMetadataForSlice(key) + if err != nil { + // We couldn't look up metadata for the field + // If the values are identical, this doesn't matter, no patch is needed + if reflect.DeepEqual(originalValue, modifiedValue) { + return nil + } + // Otherwise, return the error + return err + } + retainKeys, patchStrategy, err := extractRetainKeysPatchStrategy(patchMeta.GetPatchStrategies()) + if err != nil { + return err + } + switch patchStrategy { + // Merge the 2 slices using mergePatchKey + case mergeDirective: + diffOptions.BuildRetainKeysDirective = retainKeys + addList, deletionList, setOrderList, err := diffLists(originalValue, modifiedValue, subschema, patchMeta.GetPatchMergeKey(), diffOptions) + if err != nil { + return err + } + if len(addList) > 0 { + patch[key] = addList + } + // generate a parallel list for deletion + if len(deletionList) > 0 { + parallelDeletionListKey := fmt.Sprintf("%s/%s", deleteFromPrimitiveListDirectivePrefix, key) + patch[parallelDeletionListKey] = deletionList + } + if len(setOrderList) > 0 { + parallelSetOrderListKey := fmt.Sprintf("%s/%s", setElementOrderDirectivePrefix, key) + patch[parallelSetOrderListKey] = setOrderList + } + default: + replacePatchFieldIfNotEqual(key, originalValue, modifiedValue, patch, diffOptions) + } + return nil +} + +// extractRetainKeysPatchStrategy process patch strategy, which is a string may contains multiple +// patch strategies separated by ",". It returns a boolean var indicating if it has +// retainKeys strategies and a string for the other strategy. +func extractRetainKeysPatchStrategy(strategies []string) (bool, string, error) { + switch len(strategies) { + case 0: + return false, "", nil + case 1: + singleStrategy := strategies[0] + switch singleStrategy { + case retainKeysStrategy: + return true, "", nil + default: + return false, singleStrategy, nil + } + case 2: + switch { + case strategies[0] == retainKeysStrategy: + return true, strategies[1], nil + case strategies[1] == retainKeysStrategy: + return true, strategies[0], nil + default: + return false, "", fmt.Errorf("unexpected patch strategy: %v", strategies) + } + default: + return false, "", fmt.Errorf("unexpected patch strategy: %v", strategies) + } +} + +// replacePatchFieldIfNotEqual updates the patch if original and modified are not deep equal +// if diffOptions.IgnoreChangesAndAdditions is false. +// original is the old value, maybe either the live cluster object or the last applied configuration +// modified is the new value, is always the users new config +func replacePatchFieldIfNotEqual(key string, original, modified interface{}, patch map[string]interface{}, + diffOptions strategicpatch.DiffOptions, +) { + if diffOptions.IgnoreChangesAndAdditions { + // Ignoring changes - do nothing + return + } + if reflect.DeepEqual(original, modified) { + // Contents are identical - do nothing + return + } + // Create a patch to replace the old value with the new one + patch[key] = modified +} + +// updatePatchIfMissing iterates over `original` when ignoreDeletions is false. +// Clear the field whose key is not present in `modified`. +// original is the old value, maybe either the live cluster object or the last applied configuration +// modified is the new value, is always the users new config +func updatePatchIfMissing(original, modified, patch map[string]interface{}, + diffOptions strategicpatch.DiffOptions, +) { + if diffOptions.IgnoreDeletions { + // Ignoring deletion - do nothing + return + } + // Add nils for deleted values + for key := range original { + if _, found := modified[key]; !found { + patch[key] = nil + } + } +} + +// Returns a (recursive) strategic merge patch, a parallel deletion list if necessary and +// another list to set the order of the list +// Only list of primitives with merge strategy will generate a parallel deletion list. +// These two lists should yield modified when applied to original, for lists with merge semantics. +func diffLists(original, modified []interface{}, schema strategicpatch.LookupPatchMeta, mergeKey string, diffOptions strategicpatch.DiffOptions) ([]interface{}, []interface{}, []interface{}, error) { + if len(original) == 0 { + // Both slices are empty - do nothing + if len(modified) == 0 || diffOptions.IgnoreChangesAndAdditions { + return nil, nil, nil, nil + } + + // Old slice was empty - add all elements from the new slice + return modified, nil, nil, nil + } + + elementType, err := sliceElementType(original, modified) + if err != nil { + return nil, nil, nil, err + } + + var patchList, deleteList, setOrderList []interface{} + kind := elementType.Kind() + switch kind { + case reflect.Map: + patchList, deleteList, err = diffListsOfMaps(original, modified, schema, mergeKey, diffOptions) + if err != nil { + return nil, nil, nil, err + } + patchList, err = normalizeSliceOrder(patchList, modified, mergeKey, kind) + if err != nil { + return nil, nil, nil, err + } + orderSame, err := isOrderSame(original, modified, mergeKey) + if err != nil { + return nil, nil, nil, err + } + // append the deletions to the end of the patch list. + patchList = append(patchList, deleteList...) + deleteList = nil + // generate the setElementOrder list when there are content changes or order changes + if diffOptions.SetElementOrder && + ((!diffOptions.IgnoreChangesAndAdditions && (len(patchList) > 0 || !orderSame)) || + (!diffOptions.IgnoreDeletions && len(patchList) > 0)) { + // Generate a list of maps that each item contains only the merge key. + setOrderList = make([]interface{}, len(modified)) + for i, v := range modified { + typedV := v.(map[string]interface{}) + setOrderList[i] = map[string]interface{}{ + mergeKey: typedV[mergeKey], + } + } + } + case reflect.Slice: + // Lists of Lists are not permitted by the api + return nil, nil, nil, mergepatch.ErrNoListOfLists + default: + patchList, deleteList, err = diffListsOfScalars(original, modified, diffOptions) + if err != nil { + return nil, nil, nil, err + } + patchList, err = normalizeSliceOrder(patchList, modified, mergeKey, kind) + // generate the setElementOrder list when there are content changes or order changes + if diffOptions.SetElementOrder && ((!diffOptions.IgnoreDeletions && len(deleteList) > 0) || + (!diffOptions.IgnoreChangesAndAdditions && !reflect.DeepEqual(original, modified))) { + setOrderList = modified + } + } + return patchList, deleteList, setOrderList, err +} + +func sortScalars(s []interface{}) []interface{} { + ss := SortableSliceOfScalars{s} + sort.Sort(ss) + return ss.s +} + +// hasAdditionalNewField returns if original map has additional key with non-nil value than modified. +func hasAdditionalNewField(original, modified map[string]interface{}) bool { + for k, v := range original { + if v == nil { + continue + } + if _, found := modified[k]; !found { + return true + } + } + return false +} + +// Returns the type of the elements of N slice(s). If the type is different, +// another slice or undefined, returns an error. +func sliceElementType(slices ...[]interface{}) (reflect.Type, error) { + var prevType reflect.Type + for _, s := range slices { + // Go through elements of all given slices and make sure they are all the same type. + for _, v := range s { + currentType := reflect.TypeOf(v) + if prevType == nil { + prevType = currentType + // We don't support lists of lists yet. + if prevType.Kind() == reflect.Slice { + return nil, mergepatch.ErrNoListOfLists + } + } else { + if prevType != currentType { + return nil, fmt.Errorf("list element types are not identical: %v", fmt.Sprint(slices)) + } + prevType = currentType + } + } + } + + if prevType == nil { + return nil, fmt.Errorf("no elements in any of the given slices") + } + + return prevType, nil +} + +// diffListsOfMaps takes a pair of lists and +// returns a (recursive) strategic merge patch list contains additions and changes and +// a deletion list contains deletions +func diffListsOfMaps(original, modified []interface{}, schema strategicpatch.LookupPatchMeta, mergeKey string, diffOptions strategicpatch.DiffOptions) ([]interface{}, []interface{}, error) { + patch := make([]interface{}, 0, len(modified)) + deletionList := make([]interface{}, 0, len(original)) + + originalSorted, err := sortMergeListsByNameArray(original, schema, mergeKey, false) + if err != nil { + return nil, nil, err + } + modifiedSorted, err := sortMergeListsByNameArray(modified, schema, mergeKey, false) + if err != nil { + return nil, nil, err + } + + originalIndex, modifiedIndex := 0, 0 + for { + originalInBounds := originalIndex < len(originalSorted) + modifiedInBounds := modifiedIndex < len(modifiedSorted) + bothInBounds := originalInBounds && modifiedInBounds + if !originalInBounds && !modifiedInBounds { + break + } + + var originalElementMergeKeyValueString, modifiedElementMergeKeyValueString string + var originalElementMergeKeyValue, modifiedElementMergeKeyValue interface{} + var originalElement, modifiedElement map[string]interface{} + if originalInBounds { + originalElement, originalElementMergeKeyValue, err = getMapAndMergeKeyValueByIndex(originalIndex, mergeKey, originalSorted) + if err != nil { + return nil, nil, err + } + originalElementMergeKeyValueString = fmt.Sprintf("%v", originalElementMergeKeyValue) + } + if modifiedInBounds { + modifiedElement, modifiedElementMergeKeyValue, err = getMapAndMergeKeyValueByIndex(modifiedIndex, mergeKey, modifiedSorted) + if err != nil { + return nil, nil, err + } + modifiedElementMergeKeyValueString = fmt.Sprintf("%v", modifiedElementMergeKeyValue) + } + + switch { + case bothInBounds && strategicpatch.ItemMatchesOriginalAndModifiedSlice(originalElementMergeKeyValueString, modifiedElementMergeKeyValueString): + // Merge key values are equal, so recurse + patchValue, err := diffMaps(originalElement, modifiedElement, schema, diffOptions) + if err != nil { + return nil, nil, err + } + if len(patchValue) > 0 { + patchValue[mergeKey] = modifiedElementMergeKeyValue + patch = append(patch, patchValue) + } + originalIndex++ + modifiedIndex++ + // only modified is in bound + case !originalInBounds: + fallthrough + // modified has additional map + case bothInBounds && strategicpatch.ItemAddedToModifiedSlice(originalElementMergeKeyValueString, modifiedElementMergeKeyValueString): + if !diffOptions.IgnoreChangesAndAdditions { + patch = append(patch, modifiedElement) + } + modifiedIndex++ + // only original is in bound + case !modifiedInBounds: + fallthrough + // original has additional map + case bothInBounds && strategicpatch.ItemRemovedFromModifiedSlice(originalElementMergeKeyValueString, modifiedElementMergeKeyValueString): + if !diffOptions.IgnoreDeletions { + // Item was deleted, so add delete directive + deletionList = append(deletionList, strategicpatch.CreateDeleteDirective(mergeKey, originalElementMergeKeyValue)) + } + originalIndex++ + } + } + + return patch, deletionList, nil +} + +// normalizeSliceOrder sort `toSort` list by `order` +func normalizeSliceOrder(toSort, order []interface{}, mergeKey string, kind reflect.Kind) ([]interface{}, error) { + var toDelete []interface{} + if kind == reflect.Map { + // make sure each item in toSort, order has merge key + err := validateMergeKeyInLists(mergeKey, toSort, order) + if err != nil { + return nil, err + } + toSort, toDelete, err = extractToDeleteItems(toSort) + if err != nil { + return nil, err + } + } + + sort.SliceStable(toSort, func(i, j int) bool { + if ii := index(order, toSort[i], mergeKey, kind); ii >= 0 { + if ij := index(order, toSort[j], mergeKey, kind); ij >= 0 { + return ii < ij + } + } + return true + }) + toSort = append(toSort, toDelete...) + return toSort, nil +} + +// validateMergeKeyInLists checks if each map in the list has the mentryerge key. +func validateMergeKeyInLists(mergeKey string, lists ...[]interface{}) error { + for _, list := range lists { + for _, item := range list { + m, ok := item.(map[string]interface{}) + if !ok { + return mergepatch.ErrBadArgType(m, item) + } + if _, ok = m[mergeKey]; !ok { + return mergepatch.ErrNoMergeKey(m, mergeKey) + } + } + } + return nil +} + +// index returns the index of the item in the given items, or -1 if it doesn't exist +// l must NOT be a slice of slices, this should be checked before calling. +func index(l []interface{}, valToLookUp interface{}, mergeKey string, kind reflect.Kind) int { + var getValFn func(interface{}) interface{} + // Get the correct `getValFn` based on item `kind`. + // It should return the value of merge key for maps and + // return the item for other kinds. + switch kind { + case reflect.Map: + getValFn = func(item interface{}) interface{} { + typedItem, ok := item.(map[string]interface{}) + if !ok { + return nil + } + val := typedItem[mergeKey] + return val + } + default: + getValFn = func(item interface{}) interface{} { + return item + } + } + + for i, v := range l { + if getValFn(valToLookUp) == getValFn(v) { + return i + } + } + return -1 +} + +// isOrderSame checks if the order in a list has changed +func isOrderSame(original, modified []interface{}, mergeKey string) (bool, error) { + if len(original) != len(modified) { + return false, nil + } + for i, modifiedItem := range modified { + equal, err := mergeKeyValueEqual(original[i], modifiedItem, mergeKey) + if err != nil || !equal { + return equal, err + } + } + return true, nil +} + +func mergeKeyValueEqual(left, right interface{}, mergeKey string) (bool, error) { + if len(mergeKey) == 0 { + return left == right, nil + } + typedLeft, ok := left.(map[string]interface{}) + if !ok { + return false, mergepatch.ErrBadArgType(typedLeft, left) + } + typedRight, ok := right.(map[string]interface{}) + if !ok { + return false, mergepatch.ErrBadArgType(typedRight, right) + } + mergeKeyLeft, ok := typedLeft[mergeKey] + if !ok { + return false, mergepatch.ErrNoMergeKey(typedLeft, mergeKey) + } + mergeKeyRight, ok := typedRight[mergeKey] + if !ok { + return false, mergepatch.ErrNoMergeKey(typedRight, mergeKey) + } + return mergeKeyLeft == mergeKeyRight, nil +} + +// diffListsOfScalars returns 2 lists, the first one is addList and the second one is deletionList. +// Argument diffOptions.IgnoreChangesAndAdditions controls if calculate addList. true means not calculate. +// Argument diffOptions.IgnoreDeletions controls if calculate deletionList. true means not calculate. +// original may be changed, but modified is guaranteed to not be changed +func diffListsOfScalars(original, modified []interface{}, diffOptions strategicpatch.DiffOptions) ([]interface{}, []interface{}, error) { + modifiedCopy := make([]interface{}, len(modified)) + copy(modifiedCopy, modified) + // Sort the scalars for easier calculating the diff + originalScalars := sortScalars(original) + modifiedScalars := sortScalars(modifiedCopy) + + originalIndex, modifiedIndex := 0, 0 + addList := []interface{}{} + deletionList := []interface{}{} + + for { + originalInBounds := originalIndex < len(originalScalars) + modifiedInBounds := modifiedIndex < len(modifiedScalars) + if !originalInBounds && !modifiedInBounds { + break + } + // we need to compare the string representation of the scalar, + // because the scalar is an interface which doesn't support either < or > + // And that's how func sortScalars compare scalars. + var originalString, modifiedString string + var originalValue, modifiedValue interface{} + if originalInBounds { + originalValue = originalScalars[originalIndex] + originalString = fmt.Sprintf("%v", originalValue) + } + if modifiedInBounds { + modifiedValue = modifiedScalars[modifiedIndex] + modifiedString = fmt.Sprintf("%v", modifiedValue) + } + + originalV, modifiedV := compareListValuesAtIndex(originalInBounds, modifiedInBounds, originalString, modifiedString) + switch { + case originalV == nil && modifiedV == nil: + originalIndex++ + modifiedIndex++ + case originalV != nil && modifiedV == nil: + if !diffOptions.IgnoreDeletions { + deletionList = append(deletionList, originalValue) + } + originalIndex++ + case originalV == nil && modifiedV != nil: + if !diffOptions.IgnoreChangesAndAdditions { + addList = append(addList, modifiedValue) + } + modifiedIndex++ + default: + return nil, nil, fmt.Errorf("Unexpected returned value from compareListValuesAtIndex: %v and %v", originalV, modifiedV) + } + } + + return addList, deduplicateScalars(deletionList), nil +} + +// If first return value is non-nil, list1 contains an element not present in list2 +// If second return value is non-nil, list2 contains an element not present in list1 +func compareListValuesAtIndex(list1Inbounds, list2Inbounds bool, list1Value, list2Value string) (interface{}, interface{}) { + bothInBounds := list1Inbounds && list2Inbounds + switch { + // scalars are identical + case bothInBounds && list1Value == list2Value: + return nil, nil + // only list2 is in bound + case !list1Inbounds: + fallthrough + // list2 has additional scalar + case bothInBounds && list1Value > list2Value: + return nil, list2Value + // only original is in bound + case !list2Inbounds: + fallthrough + // original has additional scalar + case bothInBounds && list1Value < list2Value: + return list1Value, nil + default: + return nil, nil + } +} + +func deduplicateScalars(s []interface{}) []interface{} { + // Clever algorithm to deduplicate. + length := len(s) - 1 + for i := 0; i < length; i++ { + for j := i + 1; j <= length; j++ { + if s[i] == s[j] { + s[j] = s[length] + s = s[0:length] + length-- + j-- + } + } + } + + return s +} + +// extractToDeleteItems takes a list and +// returns 2 lists: one contains items that should be kept and the other contains items to be deleted. +func extractToDeleteItems(l []interface{}) ([]interface{}, []interface{}, error) { + var nonDelete, toDelete []interface{} + for _, v := range l { + m, ok := v.(map[string]interface{}) + if !ok { + return nil, nil, mergepatch.ErrBadArgType(m, v) + } + + directive, foundDirective := m[directiveMarker] + if foundDirective && directive == deleteDirective { + toDelete = append(toDelete, v) + } else { + nonDelete = append(nonDelete, v) + } + } + return nonDelete, toDelete, nil +} + +// getMapAndMergeKeyValueByIndex return a map in the list and its merge key value given the index of the map. +func getMapAndMergeKeyValueByIndex(index int, mergeKey string, listOfMaps []interface{}) (map[string]interface{}, interface{}, error) { + m, ok := listOfMaps[index].(map[string]interface{}) + if !ok { + return nil, nil, mergepatch.ErrBadArgType(m, listOfMaps[index]) + } + + val, ok := m[mergeKey] + if !ok { + return nil, nil, mergepatch.ErrNoMergeKey(m, mergeKey) + } + return m, val, nil +} + +// Function sortMergeListsByNameMap recursively sorts the merge lists by its mergeKey in an array. +func sortMergeListsByNameArray(s []interface{}, schema strategicpatch.LookupPatchMeta, mergeKey string, recurse bool) ([]interface{}, error) { + if len(s) == 0 { + return s, nil + } + + // We don't support lists of lists yet. + t, err := sliceElementType(s) + if err != nil { + return nil, err + } + + // If the elements are not maps... + if t.Kind() != reflect.Map { + // Sort the elements, because they may have been merged out of order. + return deduplicateAndSortScalars(s), nil + } + + // Elements are maps - if one of the keys of the map is a map or a + // list, we may need to recurse into it. + newS := []interface{}{} + for _, elem := range s { + if recurse { + typedElem := elem.(map[string]interface{}) + newElem, err := sortMergeListsByNameMap(typedElem, schema) + if err != nil { + return nil, err + } + + newS = append(newS, newElem) + } else { + newS = append(newS, elem) + } + } + + // Sort the maps. + newS = sortMapsBasedOnField(newS, mergeKey) + return newS, nil +} + +func deduplicateAndSortScalars(s []interface{}) []interface{} { + s = deduplicateScalars(s) + return sortScalars(s) +} + +func sortMapsBasedOnField(m []interface{}, fieldName string) []interface{} { + mapM := mapSliceFromSlice(m) + ss := SortableSliceOfMaps{mapM, fieldName} + sort.Sort(ss) + newS := sliceFromMapSlice(ss.s) + return newS +} + +func sliceFromMapSlice(s []map[string]interface{}) []interface{} { + newS := []interface{}{} + for _, v := range s { + newS = append(newS, v) + } + + return newS +} + +func mapSliceFromSlice(m []interface{}) []map[string]interface{} { + newM := []map[string]interface{}{} + for _, v := range m { + vt := v.(map[string]interface{}) + newM = append(newM, vt) + } + + return newM +} + +type SortableSliceOfMaps struct { + s []map[string]interface{} + k string // key to sort on +} + +func (ss SortableSliceOfMaps) Len() int { + return len(ss.s) +} + +func (ss SortableSliceOfMaps) Less(i, j int) bool { + iStr := fmt.Sprintf("%v", ss.s[i][ss.k]) + jStr := fmt.Sprintf("%v", ss.s[j][ss.k]) + return sort.StringsAreSorted([]string{iStr, jStr}) +} + +func (ss SortableSliceOfMaps) Swap(i, j int) { + tmp := ss.s[i] + ss.s[i] = ss.s[j] + ss.s[j] = tmp +} + +// Function sortMergeListsByNameMap recursively sorts the merge lists by its mergeKey in a map. +func sortMergeListsByNameMap(s map[string]interface{}, schema strategicpatch.LookupPatchMeta) (map[string]interface{}, error) { + newS := map[string]interface{}{} + for k, v := range s { + if k == retainKeysDirective { + typedV, ok := v.([]interface{}) + if !ok { + return nil, mergepatch.ErrBadPatchFormatForRetainKeys + } + v = sortScalars(typedV) + } else if strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix) { + typedV, ok := v.([]interface{}) + if !ok { + return nil, mergepatch.ErrBadPatchFormatForPrimitiveList + } + v = sortScalars(typedV) + } else if strings.HasPrefix(k, setElementOrderDirectivePrefix) { + _, ok := v.([]interface{}) + if !ok { + return nil, mergepatch.ErrBadPatchFormatForSetElementOrderList + } + } else if k != directiveMarker { + // recurse for map and slice. + switch typedV := v.(type) { + case map[string]interface{}: + subschema, _, err := schema.LookupPatchMetadataForStruct(k) + if err != nil { + return nil, err + } + v, err = sortMergeListsByNameMap(typedV, subschema) + if err != nil { + return nil, err + } + case []interface{}: + subschema, patchMeta, err := schema.LookupPatchMetadataForSlice(k) + if err != nil { + return nil, err + } + _, patchStrategy, err := extractRetainKeysPatchStrategy(patchMeta.GetPatchStrategies()) + if err != nil { + return nil, err + } + if patchStrategy == mergeDirective { + var err error + v, err = sortMergeListsByNameArray(typedV, subschema, patchMeta.GetPatchMergeKey(), true) + if err != nil { + return nil, err + } + } + } + } + + newS[k] = v + } + + return newS, nil +} + +type SortableSliceOfScalars struct { + s []interface{} +} + +func (ss SortableSliceOfScalars) Len() int { + return len(ss.s) +} + +func (ss SortableSliceOfScalars) Less(i, j int) bool { + iStr := fmt.Sprintf("%v", ss.s[i]) + jStr := fmt.Sprintf("%v", ss.s[j]) + return sort.StringsAreSorted([]string{iStr, jStr}) +} + +func (ss SortableSliceOfScalars) Swap(i, j int) { + tmp := ss.s[i] + ss.s[i] = ss.s[j] + ss.s[j] = tmp +} diff --git a/test/integration/test_dataplane.go b/test/integration/test_dataplane.go index e175df2d0..b8d6ab4c2 100644 --- a/test/integration/test_dataplane.go +++ b/test/integration/test_dataplane.go @@ -673,9 +673,6 @@ func TestDataPlaneVolumeMounts(t *testing.T) { PodTemplateSpec: &corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Volumes: []corev1.Volume{ - { - Name: consts.ClusterCertificateVolume, - }, { Name: "test-volume", VolumeSource: corev1.VolumeSource{ @@ -690,11 +687,6 @@ func TestDataPlaneVolumeMounts(t *testing.T) { Name: consts.DataPlaneProxyContainerName, Image: helpers.GetDefaultDataPlaneImage(), VolumeMounts: []corev1.VolumeMount{ - { - Name: consts.ClusterCertificateVolume, - MountPath: consts.ClusterCertificateVolumeMountPath, - ReadOnly: true, - }, { Name: "test-volume", MountPath: "/var/test",