Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make PodTemplateSpec more robust and less error prone #255

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions config/samples/dataplane-konnect.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 0 additions & 4 deletions config/samples/dataplane-sidecar.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ spec:
dataplane-pod-annotation: example
spec:
volumes:
- name: cluster-certificate
- name: sidecar-vector-config-volume
configMap:
name: sidecar-vector-config
Expand All @@ -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:
Expand Down
81 changes: 81 additions & 0 deletions controller/controlplane/controller_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 8 additions & 13 deletions controller/dataplane/controller_reconciler_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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/",
Expand Down Expand Up @@ -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,
},
},
Expand Down
1 change: 0 additions & 1 deletion pkg/utils/kubernetes/resources/deployments.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ func GenerateNewDeploymentForControlPlane(params GenerateNewDeploymentForControl
},
},
}
SetDefaultsPodTemplateSpec(&deployment.Spec.Template)
LabelObjectAsControlPlaneManaged(deployment)

if params.ControlPlane.Spec.Deployment.PodTemplateSpec != nil {
Expand Down
34 changes: 31 additions & 3 deletions pkg/utils/kubernetes/resources/strategicmerge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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)
}
Expand Down
Loading
Loading