From 13e66f689728b173f6ad955f93d6a3763bfe0a1f Mon Sep 17 00:00:00 2001 From: Elvand Lie Nababan Date: Wed, 28 Oct 2026 19:11:19 +0700 Subject: [PATCH] fix(k8s): fix error ordering in ProcessEnvs and late validation ProcessEnvs appends the result of createEnvVarSource to the envVars slice before checking the error. Reordered so the error check happens first, following standard Go error handling. createEnvFromSource and createEnvVarSource both validate that sourceName is non-empty only after using it to construct resource references and inserting it into the referenced sets. Moved the empty-name validation before the switch statements. Fixes #3798 Signed-off-by: Elvand Lie Nababan --- pkg/k8s/deployer.go | 30 ++++++++-------------- pkg/k8s/deployer_test.go | 54 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 19 deletions(-) diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index 5b6048d382..b2696e588e 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -1006,10 +1006,10 @@ func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Se if len(slices) == 3 { // ENV from a key in secret/configMap, eg. FOO={{ secret:secretName:key }} FOO={{ configMap:configMapName.key }} valueFrom, err := createEnvVarSource(slices, referencedSecrets, referencedConfigMaps) - envVars = append(envVars, corev1.EnvVar{Name: *env.Name, ValueFrom: valueFrom}) if err != nil { return nil, nil, err } + envVars = append(envVars, corev1.EnvVar{Name: *env.Name, ValueFrom: valueFrom}) continue } else if len(slices) == 2 { // ENV from the local ENV var, eg. FOO={{ env:LOCAL_ENV }} @@ -1340,11 +1340,12 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * typeString := strings.TrimSpace(slices[0]) sourceName := strings.TrimSpace(slices[1]) - var sourceType string + if len(sourceName) == 0 { + return nil, fmt.Errorf("env source name cannot be an empty string") + } switch typeString { case "configMap": - sourceType = "ConfigMap" envVarSource.ConfigMapRef = &corev1.ConfigMapEnvSource{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -1354,7 +1355,6 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * referencedConfigMaps.Insert(sourceName) } case "secret": - sourceType = "Secret" envVarSource.SecretRef = &corev1.SecretEnvSource{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -1366,10 +1366,6 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * return nil, fmt.Errorf("unsupported env source type %q; supported source types are \"configMap\" or \"secret\"", slices[0]) } - if len(sourceName) == 0 { - return nil, fmt.Errorf("the name of %s cannot be an empty string", sourceType) - } - return &envVarSource, nil } @@ -1384,11 +1380,16 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps sourceName := strings.TrimSpace(slices[1]) sourceKey := strings.TrimSpace(slices[2]) - var sourceType string + if len(sourceName) == 0 { + return nil, fmt.Errorf("env source name cannot be an empty string") + } + + if len(sourceKey) == 0 { + return nil, fmt.Errorf("env source key cannot be an empty string") + } switch typeString { case "configMap": - sourceType = "ConfigMap" envVarSource.ConfigMapKeyRef = &corev1.ConfigMapKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -1399,7 +1400,6 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps referencedConfigMaps.Insert(sourceName) } case "secret": - sourceType = "Secret" envVarSource.SecretKeyRef = &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -1413,14 +1413,6 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps return nil, fmt.Errorf("unsupported env source type %q; supported source types are \"configMap\" or \"secret\"", slices[0]) } - if len(sourceName) == 0 { - return nil, fmt.Errorf("the name of %s cannot be an empty string", sourceType) - } - - if len(sourceKey) == 0 { - return nil, fmt.Errorf("the key referenced by resource %s %q cannot be an empty string", sourceType, sourceName) - } - return &envVarSource, nil } diff --git a/pkg/k8s/deployer_test.go b/pkg/k8s/deployer_test.go index 5a86e3237a..b67fbdfa73 100644 --- a/pkg/k8s/deployer_test.go +++ b/pkg/k8s/deployer_test.go @@ -1825,3 +1825,57 @@ func Test_preserveDeploymentSelector(t *testing.T) { } }) } + +func Test_ProcessEnvs_ErrorCheckBeforeAppend(t *testing.T) { + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + + name := "MY_VAR" + // empty secret name in the template should cause an error + value := "{{ secret::key }}" + envs := []fn.Env{{Name: &name, Value: &value}} + + envVars, _, err := ProcessEnvs(envs, &referencedSecrets, &referencedConfigMaps) + if err == nil { + t.Fatal("expected error for empty secret name") + } + if envVars != nil { + t.Fatalf("expected nil envVars on error, got %d entries", len(envVars)) + } +} + +func Test_createEnvFromSource_EmptyName(t *testing.T) { + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + + _, err := createEnvFromSource("{{ secret: }}", &referencedSecrets, &referencedConfigMaps) + if err == nil { + t.Fatal("expected error for empty source name") + } + if referencedSecrets.Has("") { + t.Fatal("empty string was inserted into referencedSecrets before validation") + } +} + +func Test_createEnvVarSource_EmptyName(t *testing.T) { + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + + _, err := createEnvVarSource([]string{"secret", "", "key"}, &referencedSecrets, &referencedConfigMaps) + if err == nil { + t.Fatal("expected error for empty source name") + } + if referencedSecrets.Has("") { + t.Fatal("empty string was inserted into referencedSecrets before validation") + } +} + +func Test_createEnvVarSource_EmptyKey(t *testing.T) { + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + + _, err := createEnvVarSource([]string{"secret", "my-secret", ""}, &referencedSecrets, &referencedConfigMaps) + if err == nil { + t.Fatal("expected error for empty source key") + } +}