From 015a7dae13e6e2df8e3ed5373d020e0612fd1d55 Mon Sep 17 00:00:00 2001 From: "elvandlie@gmail.com" Date: Thu, 21 May 2026 02:52:22 +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 --- 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 1341515ac8..175b50c4ab 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -633,10 +633,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 }} @@ -709,11 +709,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, @@ -723,7 +724,6 @@ func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps * referencedConfigMaps.Insert(sourceName) } case "secret": - sourceType = "Secret" envVarSource.SecretRef = &corev1.SecretEnvSource{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -735,10 +735,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 } @@ -753,11 +749,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, @@ -768,7 +769,6 @@ func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps referencedConfigMaps.Insert(sourceName) } case "secret": - sourceType = "Secret" envVarSource.SecretKeyRef = &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ Name: sourceName, @@ -782,14 +782,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 ec48829493..50fb89beab 100644 --- a/pkg/k8s/deployer_test.go +++ b/pkg/k8s/deployer_test.go @@ -424,3 +424,57 @@ func TestGenerateTriggerName_DifferentBrokers(t *testing.T) { t.Errorf("Different brokers should produce different names: %s, %s, %s", name1, name2, name3) } } + +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") + } +}