fix: preserve env and volume override union fields

docs/features/overrides.md

@@ -311,6 +311,56 @@ spec:
                     key: test
 ```
 
+The same override can be expressed as a strategic merge patch:
+
+```yaml
+apiVersion: temporal.io/v1beta1
+kind: TemporalCluster
+metadata:
+  name: prod
+spec:
+  # [...]
+  services:
+    frontend:
+      overrides:
+        deployment:
+          spec:
+            template:
+              spec:
+                containers:
+                  - name: service
+                    env:
+                      - name: TEST
+                        valueFrom:
+                          secretKeyRef:
+                            name: test-secret
+                            key: test
+```
+
+### Replacing a field of an existing env var or volume
+
+A strategic merge patch merges list elements sharing the same merge key (`name`, for
+`env`, `volumes` and `volumeMounts`) field by field. When the override targets an entry
+that already exists and replaces one member of a mutually exclusive group — `value` and
+`valueFrom` on an env var, or the source of a volume — the field being replaced has to
+be nulled explicitly, otherwise both end up set and the result is invalid:
+
+```yaml
+                    env:
+                      # "PROMETHEUS_ENDPOINT" already exists with a "value".
+                      - name: PROMETHEUS_ENDPOINT
+                        value: null
+                        valueFrom:
+                          secretKeyRef:
+                            name: test-secret
+                            key: endpoint
+```
+
+`$patch: replace` on the element works too. The operator validates the merge result and
+fails the reconcile with the offending field path when a merged env var carries both
+`value` and `valueFrom`, or a merged volume carries more than one source, instead of
+letting the API server reject the deployment later on.
+
 Read more in [Strategic Merge Patch](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md#strategic-merge-patch).
 
 ## Override UI deployment

pkg/kubernetes/overrides.go

@@ -27,6 +27,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 // PatchPodSpecWithOverride patches the provided pod spec with the provided pod spec override.
@@ -56,6 +57,10 @@ func PatchPodSpecWithOverride(spec, override *corev1.PodSpec) (*corev1.PodSpec,
 		return nil, fmt.Errorf("can't unmarshal patched pod spec: %w", err)
 	}
 
+	if errs := validatePatchedPodSpec(patchedSpec, field.NewPath("spec")); len(errs) > 0 {
+		return nil, fmt.Errorf("invalid patched pod spec: %w", errs.ToAggregate())
+	}
+
 	return patchedSpec, nil
 }
 
@@ -83,7 +88,22 @@ func ApplyPodTemplateSpecOverrides(podTemplate *corev1.PodTemplateSpec, override
 		if err != nil {
 			return fmt.Errorf("can't patch pod template spec: %w", err)
 		}
-		return json.Unmarshal(patched, &podTemplate.Spec)
+		// Decode into a fresh PodSpec: the strategic merge reorders merge-key lists
+		// (env, volumes, ...) so unmarshalling in place would decode a merged element
+		// on top of an unrelated existing one and keep its stale fields, producing
+		// invalid results such as an EnvVar holding both value and valueFrom.
+		patchedSpec := corev1.PodSpec{}
+		if err := json.Unmarshal(patched, &patchedSpec); err != nil {
+			return fmt.Errorf("can't unmarshal patched pod template spec: %w", err)
+		}
+
+		if errs := validatePatchedPodSpec(&patchedSpec, field.NewPath("spec")); len(errs) > 0 {
+			return fmt.Errorf("invalid patched pod template spec: %w", errs.ToAggregate())
+		}
+
+		podTemplate.Spec = patchedSpec
+
+		return nil
 	}
 	return nil
 }
@@ -126,7 +146,22 @@ func ApplyDeploymentOverrides(deployment *appsv1.Deployment, override *v1beta1.D
 		if err != nil {
 			return fmt.Errorf("can't apply json patch: %w", err)
 		}
-		return json.Unmarshal(patched, &deployment)
+		// Decode into a fresh Deployment for the same reason as above: a json patch
+		// can reorder or remove list elements, and unmarshalling in place would leave
+		// stale fields behind on the elements it decodes over.
+		patchedDeployment := appsv1.Deployment{}
+		if err := json.Unmarshal(patched, &patchedDeployment); err != nil {
+			return fmt.Errorf("can't unmarshal patched deployment: %w", err)
+		}
+
+		errs := validatePatchedPodSpec(&patchedDeployment.Spec.Template.Spec, field.NewPath("spec", "template", "spec"))
+		if len(errs) > 0 {
+			return fmt.Errorf("invalid patched deployment: %w", errs.ToAggregate())
+		}
+
+		*deployment = patchedDeployment
+
+		return nil
 	}
 
 	return nil

pkg/kubernetes/overrides_test.go

@@ -656,6 +656,180 @@ func TestApplyPodTemplateSpecOverrides(t *testing.T) {
 				},
 			},
 		},
+		// Regression test for https://github.com/alexandrevilain/temporal-operator/issues/793:
+		// the strategic merge sorts the override's env var first, and the merged result used
+		// to be decoded on top of the existing entry, leaving its "value" next to the new
+		// "valueFrom". The override name is chosen so the merged list is reordered: a case
+		// where the override sorts last passes even with the bug.
+		"add env var with valueFrom to existing env": {
+			original: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							Env: []corev1.EnvVar{
+								{
+									Name:  "zzz",
+									Value: "already-here",
+								},
+							},
+						},
+					},
+				},
+			},
+			override: &v1beta1.PodTemplateSpecOverride{
+				Spec: &apiextensionsv1.JSON{
+					Raw: []byte(`{"containers":[{"name":"service","env":[{"name":"aaa","valueFrom":{"secretKeyRef":{"name":"test-secret","key":"test"}}}]}]}`),
+				},
+			},
+			expected: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							Env: []corev1.EnvVar{
+								{
+									Name: "aaa",
+									ValueFrom: &corev1.EnvVarSource{
+										SecretKeyRef: &corev1.SecretKeySelector{
+											LocalObjectReference: corev1.LocalObjectReference{
+												Name: "test-secret",
+											},
+											Key: "test",
+										},
+									},
+								},
+								{
+									Name:  "zzz",
+									Value: "already-here",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		// Same regression, on volumes: the added secret volume used to keep the configMap
+		// source of the entry it was decoded over, giving a volume with two sources.
+		"add secret volume to existing volumes": {
+			original: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							VolumeMounts: []corev1.VolumeMount{
+								{
+									Name:      "zzz-config",
+									MountPath: "/etc/zzz",
+								},
+							},
+						},
+					},
+					Volumes: []corev1.Volume{
+						{
+							Name: "zzz-config",
+							VolumeSource: corev1.VolumeSource{
+								ConfigMap: &corev1.ConfigMapVolumeSource{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "test-config",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			override: &v1beta1.PodTemplateSpecOverride{
+				Spec: &apiextensionsv1.JSON{
+					Raw: []byte(`{"containers":[{"name":"service","volumeMounts":[{"name":"aaa-secret","mountPath":"/etc/aaa"}]}],"volumes":[{"name":"aaa-secret","secret":{"secretName":"test-secret"}}]}`),
+				},
+			},
+			expected: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							VolumeMounts: []corev1.VolumeMount{
+								{
+									Name:      "aaa-secret",
+									MountPath: "/etc/aaa",
+								},
+								{
+									Name:      "zzz-config",
+									MountPath: "/etc/zzz",
+								},
+							},
+						},
+					},
+					Volumes: []corev1.Volume{
+						{
+							Name: "aaa-secret",
+							VolumeSource: corev1.VolumeSource{
+								Secret: &corev1.SecretVolumeSource{
+									SecretName: "test-secret",
+								},
+							},
+						},
+						{
+							Name: "zzz-config",
+							VolumeSource: corev1.VolumeSource{
+								ConfigMap: &corev1.ConfigMapVolumeSource{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "test-config",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		// Replacing the source of an env var that already exists: the strategic merge merges
+		// both elements field by field, so the override has to null the field it replaces.
+		"replace an existing env var value by a valueFrom": {
+			original: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							Env: []corev1.EnvVar{
+								{
+									Name:  "a",
+									Value: "already-here",
+								},
+							},
+						},
+					},
+				},
+			},
+			override: &v1beta1.PodTemplateSpecOverride{
+				Spec: &apiextensionsv1.JSON{
+					Raw: []byte(`{"containers":[{"name":"service","env":[{"name":"a","value":null,"valueFrom":{"secretKeyRef":{"name":"test-secret","key":"test"}}}]}]}`),
+				},
+			},
+			expected: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							Env: []corev1.EnvVar{
+								{
+									Name: "a",
+									ValueFrom: &corev1.EnvVarSource{
+										SecretKeyRef: &corev1.SecretKeySelector{
+											LocalObjectReference: corev1.LocalObjectReference{
+												Name: "test-secret",
+											},
+											Key: "test",
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for name, test := range tests {
@@ -667,6 +841,74 @@ func TestApplyPodTemplateSpecOverrides(t *testing.T) {
 	}
 }
 
+func TestApplyPodTemplateSpecOverridesReportsInvalidMerges(t *testing.T) {
+	tests := map[string]struct {
+		original      *corev1.PodTemplateSpec
+		override      *v1beta1.PodTemplateSpecOverride
+		expectedError string
+	}{
+		"env var merged into an existing value": {
+			original: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "service",
+							Env: []corev1.EnvVar{
+								{
+									Name:  "a",
+									Value: "already-here",
+								},
+							},
+						},
+					},
+				},
+			},
+			override: &v1beta1.PodTemplateSpecOverride{
+				Spec: &apiextensionsv1.JSON{
+					Raw: []byte(`{"containers":[{"name":"service","env":[{"name":"a","valueFrom":{"secretKeyRef":{"name":"test-secret","key":"test"}}}]}]}`),
+				},
+			},
+			expectedError: `spec.containers[0].env[0].valueFrom: Invalid value: "a": may not be specified when "value" is not empty`,
+		},
+		"volume merged into an existing source": {
+			original: &corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Volumes: []corev1.Volume{
+						{
+							Name: "a",
+							VolumeSource: corev1.VolumeSource{
+								ConfigMap: &corev1.ConfigMapVolumeSource{
+									LocalObjectReference: corev1.LocalObjectReference{
+										Name: "test-config",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			override: &v1beta1.PodTemplateSpecOverride{
+				Spec: &apiextensionsv1.JSON{
+					Raw: []byte(`{"volumes":[{"name":"a","secret":{"secretName":"test-secret"}}]}`),
+				},
+			},
+			expectedError: `spec.volumes[0]: Invalid value: "secret, configMap": may not specify more than one volume source`,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(tt *testing.T) {
+			original := test.original.DeepCopy()
+
+			err := kubernetes.ApplyPodTemplateSpecOverrides(test.original, test.override)
+			require.Error(tt, err)
+			assert.Contains(tt, err.Error(), test.expectedError)
+			// The pod template is left untouched when the merge result is rejected.
+			assert.True(tt, equality.Semantic.DeepEqual(test.original, original))
+		})
+	}
+}
+
 func TestPatchPodSpecWithOverride(t *testing.T) {
 	tests := map[string]struct {
 		original *corev1.PodSpec