From 0ddaa7f7c9f860ea8b35777bb8bb7104c8247b24 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 7 Jun 2023 15:55:31 -0700 Subject: [PATCH] Fix warnings on "duplicate" env vars Some use-cases are not actually wrong --- pkg/api/pod/warnings.go | 20 +++++++++++++++++++- pkg/api/pod/warnings_test.go | 34 ++++++++++++++++++++++++---------- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 31a66b116be..8571dd0cf71 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -246,7 +246,25 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta items := sets.NewString() for i, item := range c.Env { if items.Has(item.Name) { - warnings = append(warnings, fmt.Sprintf("%s: duplicate name %q", p.Child("env").Index(i).Child("name"), item.Name)) + // a previous value exists, but it might be OK + bad := false + ref := fmt.Sprintf("$(%s)", item.Name) // what does a ref to this name look like + // if we are replacing it with a valueFrom, warn + if item.ValueFrom != nil { + bad = true + } + // if this is X="$(X)", warn + if item.Value == ref { + bad = true + } + // if the new value does not contain a reference to the old + // value (e.g. X="abc"; X="$(X)123"), warn + if !strings.Contains(item.Value, ref) { + bad = true + } + if bad { + warnings = append(warnings, fmt.Sprintf("%s: hides previous definition of %q", p.Child("env").Index(i), item.Name)) + } } else { items.Insert(item.Name) } diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index e849fb37bef..9bfd105f825 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -272,21 +272,35 @@ func TestWarnings(t *testing.T) { name: "duplicate env", template: &api.PodTemplateSpec{Spec: api.PodSpec{ InitContainers: []api.Container{{Env: []api.EnvVar{ - {Name: "a"}, - {Name: "a"}, - {Name: "a"}, + {Name: "a", Value: "a"}, + {Name: "a", Value: "a"}, + {Name: "a", Value: "other"}, + {Name: "a", Value: ""}, + {Name: "a", Value: "$(a)"}, + {Name: "a", ValueFrom: &api.EnvVarSource{}}, + {Name: "a", Value: "$(a) $(a)"}, // no warning }}}, Containers: []api.Container{{Env: []api.EnvVar{ - {Name: "b"}, - {Name: "b"}, - {Name: "b"}, + {Name: "b", Value: "b"}, + {Name: "b", Value: "b"}, + {Name: "b", Value: "other"}, + {Name: "b", Value: ""}, + {Name: "b", Value: "$(b)"}, + {Name: "b", ValueFrom: &api.EnvVarSource{}}, + {Name: "b", Value: "$(b) $(b)"}, // no warning }}}, }}, expected: []string{ - `spec.initContainers[0].env[1].name: duplicate name "a"`, - `spec.initContainers[0].env[2].name: duplicate name "a"`, - `spec.containers[0].env[1].name: duplicate name "b"`, - `spec.containers[0].env[2].name: duplicate name "b"`, + `spec.initContainers[0].env[1]: hides previous definition of "a"`, + `spec.initContainers[0].env[2]: hides previous definition of "a"`, + `spec.initContainers[0].env[3]: hides previous definition of "a"`, + `spec.initContainers[0].env[4]: hides previous definition of "a"`, + `spec.initContainers[0].env[5]: hides previous definition of "a"`, + `spec.containers[0].env[1]: hides previous definition of "b"`, + `spec.containers[0].env[2]: hides previous definition of "b"`, + `spec.containers[0].env[3]: hides previous definition of "b"`, + `spec.containers[0].env[4]: hides previous definition of "b"`, + `spec.containers[0].env[5]: hides previous definition of "b"`, }, }, {