From 0564de277597cf33d450dac7247c07a3e61887c6 Mon Sep 17 00:00:00 2001 From: Matthew Mckenzie Date: Tue, 8 Sep 2026 20:23:58 +0000 Subject: [PATCH] fix: preserve override union fields during strategic merge --- docs/features/overrides.md | 50 +++++ pkg/kubernetes/overrides.go | 39 +++- pkg/kubernetes/overrides_test.go | 242 +++++++++++++++++++++++++ pkg/kubernetes/overrides_validation.go | 121 +++++++++++++ 4 files changed, 450 insertions(+), 2 deletions(-) create mode 100644 pkg/kubernetes/overrides_validation.go diff --git a/docs/features/overrides.md b/docs/features/overrides.md index 227c5d59..65c23110 100644 --- a/docs/features/overrides.md +++ b/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 diff --git a/pkg/kubernetes/overrides.go b/pkg/kubernetes/overrides.go index 12c908c3..d3ba6b2d 100644 --- a/pkg/kubernetes/overrides.go +++ b/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 diff --git a/pkg/kubernetes/overrides_test.go b/pkg/kubernetes/overrides_test.go index c81d6dc9..c8d055a7 100644 --- a/pkg/kubernetes/overrides_test.go +++ b/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 diff --git a/pkg/kubernetes/overrides_validation.go b/pkg/kubernetes/overrides_validation.go new file mode 100644 index 00000000..ec2ef9fd --- /dev/null +++ b/pkg/kubernetes/overrides_validation.go @@ -0,0 +1,121 @@ +// Licensed to Alexandre VILAIN under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Alexandre VILAIN licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package kubernetes + +import ( + "reflect" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// mergeConflictHint is appended to override validation errors: a merged element +// holding several mutually exclusive fields always comes from a strategic merge +// patch which merged an override element into an existing one carrying the same +// merge key. +const mergeConflictHint = `the override was merged into an existing element with the same name; ` + + `explicitly set the conflicting field to null in the override, or use "$patch": "replace"` + +// validatePatchedPodSpec reports the mutually-exclusive field constraints that an +// override merge can violate. Strategic merge patch merges two elements sharing the +// same merge key field by field, which happily produces an EnvVar carrying both +// value and valueFrom, or a Volume carrying two sources. The API server rejects +// those on apply; reporting them here fails the reconcile with a message naming the +// offending field and the way out. +func validatePatchedPodSpec(spec *corev1.PodSpec, path *field.Path) field.ErrorList { + errs := field.ErrorList{} + + containerLists := []struct { + name string + containers []corev1.Container + }{ + {"initContainers", spec.InitContainers}, + {"containers", spec.Containers}, + } + + for _, list := range containerLists { + for i, container := range list.containers { + containerPath := path.Child(list.name).Index(i) + errs = append(errs, validatePatchedEnv(container.Env, containerPath.Child("env"))...) + } + } + + for i, container := range spec.EphemeralContainers { + containerPath := path.Child("ephemeralContainers").Index(i) + errs = append(errs, validatePatchedEnv(container.Env, containerPath.Child("env"))...) + } + + for i, volume := range spec.Volumes { + volumePath := path.Child("volumes").Index(i) + if sources := setFields(volume.VolumeSource); len(sources) > 1 { + errs = append(errs, field.Invalid(volumePath, strings.Join(sources, ", "), + `may not specify more than one volume source: `+mergeConflictHint)) + } + } + + return errs +} + +func validatePatchedEnv(env []corev1.EnvVar, path *field.Path) field.ErrorList { + errs := field.ErrorList{} + + for i, envVar := range env { + envVarPath := path.Index(i) + + if envVar.Value != "" && envVar.ValueFrom != nil { + errs = append(errs, field.Invalid(envVarPath.Child("valueFrom"), envVar.Name, + `may not be specified when "value" is not empty: `+mergeConflictHint)) + + continue + } + + if envVar.ValueFrom != nil { + if sources := setFields(*envVar.ValueFrom); len(sources) > 1 { + errs = append(errs, field.Invalid(envVarPath.Child("valueFrom"), strings.Join(sources, ", "), + `may not specify more than one environment variable source: `+mergeConflictHint)) + } + } + } + + return errs +} + +// setFields returns the json names of the non-nil pointer fields of the provided +// struct. It's used to count how many members of a union-like struct are set. +func setFields(union any) []string { + names := []string{} + + value := reflect.ValueOf(union) + for i := range value.NumField() { + if value.Field(i).Kind() != reflect.Ptr || value.Field(i).IsNil() { + continue + } + + name := value.Type().Field(i).Name + if tag, ok := value.Type().Field(i).Tag.Lookup("json"); ok { + if jsonName, _, _ := strings.Cut(tag, ","); jsonName != "" { + name = jsonName + } + } + + names = append(names, name) + } + + return names +}