From 27f622c5393556892420ec6088f1676095654ff9 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 28 Mar 2026 02:33:22 +0530 Subject: [PATCH 1/4] fix: pass referenced resource sets to generateNewService for proper validation The generateNewService function was creating its own local referencedSecrets, referencedConfigMaps, and referencedPVCs sets internally, but these were never returned to the caller. This meant CheckResourcesArePresent was always called with empty sets, silently skipping resource validation for new service deployments. Fix by passing the caller's sets into generateNewService so they get properly populated by ProcessEnvs and ProcessVolumes. Also adds regression tests verifying: - env vars from -e flag are passed through to the deployer - env vars appear in the generated Knative Service container spec Ref #3514 --- cmd/deploy_test.go | 43 ++++++++++++++++++++++++++++ pkg/knative/deployer.go | 12 +++----- pkg/knative/deployer_test.go | 55 ++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 8 deletions(-) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index a769cf778a..ce9f3b0b04 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -465,6 +465,49 @@ func TestDeploy_Envs(t *testing.T) { } +// TestDeploy_EnvsPassedToDeployer ensures that environment variables provided +// via the -e flag are included in the function passed to the deployer. +// This is a regression test for issue #3514 where env vars were persisted to +// func.yaml but not included in the deployed service spec. +func TestDeploy_EnvsPassedToDeployer(t *testing.T) { + root := FromTempDirectory(t) + ptr := func(s string) *string { return &s } + + _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) + if err != nil { + t.Fatal(err) + } + + deployer := mock.NewDeployer() + var deployedEnvs fn.Envs + deployer.DeployFn = func(_ context.Context, f fn.Function) (fn.DeploymentResult, error) { + deployedEnvs = f.Run.Envs + return fn.DeploymentResult{ + Status: fn.Deployed, + URL: "http://example.com", + Namespace: "default", + }, nil + } + + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + cmd.SetArgs([]string{"--env=MYVAR=myvalue", "--env=OTHER=othervalue"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + if !deployer.DeployInvoked { + t.Fatal("deployer was not invoked") + } + + expected := fn.Envs([]fn.Env{ + {Name: ptr("MYVAR"), Value: ptr("myvalue")}, + {Name: ptr("OTHER"), Value: ptr("othervalue")}, + }) + if !reflect.DeepEqual(deployedEnvs, expected) { + t.Fatalf("Expected deployer to receive envs '%v', got '%v'", expected, deployedEnvs) + } +} + // TestDeploy_FunctionContext ensures that the function contextually relevant // to the current command is loaded and used for flag defaults by spot-checking // the builder setting. diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index ff8367c891..707b30d98b 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -209,7 +209,7 @@ consider setting up the pull secrets and linking it to the default service accou referencedConfigMaps := sets.New[string]() referencedPVCs := sets.New[string]() - service, err := generateNewService(f, d.decorator, daprInstalled) + service, err := generateNewService(f, d.decorator, daprInstalled, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) if err != nil { err = fmt.Errorf("knative deployer failed to generate the Knative Service: %v", err) return fn.DeploymentResult{}, err @@ -411,7 +411,7 @@ func createTriggers(ctx context.Context, f fn.Function, client clientservingv1.K return nil } -func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprInstalled bool) (*servingv1.Service, error) { +func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprInstalled bool, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (*servingv1.Service, error) { container := corev1.Container{ Image: f.Deploy.Image, } @@ -419,18 +419,14 @@ func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprI k8s.SetSecurityContext(&container) k8s.SetHealthEndpoints(f, &container) - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVC := sets.New[string]() - - newEnv, newEnvFrom, err := k8s.ProcessEnvs(f.Run.Envs, &referencedSecrets, &referencedConfigMaps) + newEnv, newEnvFrom, err := k8s.ProcessEnvs(f.Run.Envs, referencedSecrets, referencedConfigMaps) if err != nil { return nil, err } container.Env = newEnv container.EnvFrom = newEnvFrom - newVolumes, newVolumeMounts, err := k8s.ProcessVolumes(f.Run.Volumes, &referencedSecrets, &referencedConfigMaps, &referencedPVC) + newVolumes, newVolumeMounts, err := k8s.ProcessVolumes(f.Run.Volumes, referencedSecrets, referencedConfigMaps, referencedPVCs) if err != nil { return nil, err } diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index 2de1d2d4be..52a9a17b5a 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -24,8 +24,10 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + fn "knative.dev/func/pkg/functions" "knative.dev/pkg/ptr" ) @@ -290,6 +292,59 @@ func setupRegistry(t *testing.T) http.RoundTripper { return trans } +// TestGenerateNewService_Envs ensures that environment variables defined on the +// function are correctly included in the generated Knative Service container spec. +// This is a regression test for issue #3514. +func TestGenerateNewService_Envs(t *testing.T) { + ptr := func(s string) *string { return &s } + + f := fn.Function{ + Name: "test-func", + Deploy: fn.DeploySpec{ + Image: "example.com/test:latest", + }, + Run: fn.RunSpec{ + Envs: fn.Envs{ + {Name: ptr("MYVAR"), Value: ptr("myvalue")}, + {Name: ptr("OTHER"), Value: ptr("othervalue")}, + }, + }, + } + + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + referencedPVCs := sets.New[string]() + + service, err := generateNewService(f, nil, false, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + if err != nil { + t.Fatal(err) + } + + containers := service.Spec.Template.Spec.Containers + if len(containers) != 1 { + t.Fatalf("expected 1 container, got %d", len(containers)) + } + + envVars := containers[0].Env + // ProcessEnvs adds ADDRESS=0.0.0.0 and BUILT= automatically + foundMyVar := false + foundOther := false + for _, env := range envVars { + if env.Name == "MYVAR" && env.Value == "myvalue" { + foundMyVar = true + } + if env.Name == "OTHER" && env.Value == "othervalue" { + foundOther = true + } + } + if !foundMyVar { + t.Errorf("expected MYVAR=myvalue in container env, got: %v", envVars) + } + if !foundOther { + t.Errorf("expected OTHER=othervalue in container env, got: %v", envVars) + } +} + func assertAuth(uname, pwd string, w http.ResponseWriter, r *http.Request) bool { user, pass, ok := r.BasicAuth() if ok && user == uname && pass == pwd { From 31f05f444abd9633a204cbde5ad61939237a82c4 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 3 Apr 2026 15:50:21 +0530 Subject: [PATCH 2/4] test: replace mislabelled regression tests with honest coverage Remove TestGenerateNewService_Envs which tested pre-existing behaviour and was mislabelled as a regression test for #3514. Add TestUpdateService_EnvsPropagated which directly exercises the update path through deployer.go: it calls the updateService closure with a pre-existing service (simulating previousService != nil) and asserts that cp.Env = newEnv at deployer.go:553 correctly replaces the container's env list. This is the path that runs on every redeployment of an existing function. Also correct the doc-comment on TestDeploy_EnvsPassedToDeployer: it is a valid characterisation test, not a regression test for #3514. --- cmd/deploy_test.go | 5 +- pkg/knative/deployer_test.go | 95 +++++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 36 deletions(-) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index ce9f3b0b04..20eadb68e5 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -467,8 +467,9 @@ func TestDeploy_Envs(t *testing.T) { // TestDeploy_EnvsPassedToDeployer ensures that environment variables provided // via the -e flag are included in the function passed to the deployer. -// This is a regression test for issue #3514 where env vars were persisted to -// func.yaml but not included in the deployed service spec. +// It verifies that --env flags are forwarded through the deploy command +// to the deployer implementation (characterisation test; not a regression +// test for a specific issue). func TestDeploy_EnvsPassedToDeployer(t *testing.T) { root := FromTempDirectory(t) ptr := func(s string) *string { return &s } diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index 52a9a17b5a..07a3017c9a 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -24,11 +24,11 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" fn "knative.dev/func/pkg/functions" "knative.dev/pkg/ptr" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) const ( @@ -292,56 +292,83 @@ func setupRegistry(t *testing.T) http.RoundTripper { return trans } -// TestGenerateNewService_Envs ensures that environment variables defined on the -// function are correctly included in the generated Knative Service container spec. -// This is a regression test for issue #3514. -func TestGenerateNewService_Envs(t *testing.T) { - ptr := func(s string) *string { return &s } +// TestUpdateService_EnvsPropagated verifies that environment variables are correctly +// written into the container spec when updating an existing Knative Service. +// This exercises the update path through deployer.go (the updateService closure), +// specifically the `cp.Env = newEnv` assignment that runs when redeploying a function +// that already exists on the cluster. The former test in this slot (TestGenerateNewService_Envs) +// tested pre-existing behaviour unrelated to the actual code change in this commit. +func TestUpdateService_EnvsPropagated(t *testing.T) { + // Simulate an existing deployed service that carries a stale env var. + previousService := &servingv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-func", + Namespace: "default", + }, + Spec: servingv1.ServiceSpec{ + ConfigurationSpec: servingv1.ConfigurationSpec{ + Template: servingv1.RevisionTemplateSpec{ + Spec: servingv1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Image: "example.com/test:v1", + Env: []corev1.EnvVar{{Name: "OLD_VAR", Value: "old"}}, + }, + }, + }, + }, + }, + }, + }, + } f := fn.Function{ Name: "test-func", Deploy: fn.DeploySpec{ - Image: "example.com/test:latest", - }, - Run: fn.RunSpec{ - Envs: fn.Envs{ - {Name: ptr("MYVAR"), Value: ptr("myvalue")}, - {Name: ptr("OTHER"), Value: ptr("othervalue")}, - }, + Image: "example.com/test:v2", }, } - referencedSecrets := sets.New[string]() - referencedConfigMaps := sets.New[string]() - referencedPVCs := sets.New[string]() + // newEnv represents the resolved env vars the deployer computes via + // k8s.ProcessEnvs(f.Run.Envs, ...) before invoking updateService. + newEnv := []corev1.EnvVar{ + {Name: "MYVAR", Value: "myvalue"}, + {Name: "OTHER", Value: "othervalue"}, + } - service, err := generateNewService(f, nil, false, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + // Obtain the update closure — this mirrors what Deploy() does when + // previousService != nil (the service already exists on the cluster). + updateFn := updateService(f, previousService, newEnv, nil, nil, nil, nil, false) + + // Apply the closure to a working copy of the service (mirroring what + // UpdateServiceWithRetry does internally on each attempt). + svcCopy := previousService.DeepCopy() + result, err := updateFn(svcCopy) if err != nil { - t.Fatal(err) + t.Fatalf("updateService closure returned unexpected error: %v", err) } - containers := service.Spec.Template.Spec.Containers + containers := result.Spec.Template.Spec.Containers if len(containers) != 1 { t.Fatalf("expected 1 container, got %d", len(containers)) } - envVars := containers[0].Env - // ProcessEnvs adds ADDRESS=0.0.0.0 and BUILT= automatically - foundMyVar := false - foundOther := false - for _, env := range envVars { - if env.Name == "MYVAR" && env.Value == "myvalue" { - foundMyVar = true - } - if env.Name == "OTHER" && env.Value == "othervalue" { - foundOther = true - } + gotEnv := containers[0].Env + envMap := make(map[string]string, len(gotEnv)) + for _, e := range gotEnv { + envMap[e.Name] = e.Value + } + + if v, ok := envMap["MYVAR"]; !ok || v != "myvalue" { + t.Errorf("expected MYVAR=myvalue in updated container env, got: %v", gotEnv) } - if !foundMyVar { - t.Errorf("expected MYVAR=myvalue in container env, got: %v", envVars) + if v, ok := envMap["OTHER"]; !ok || v != "othervalue" { + t.Errorf("expected OTHER=othervalue in updated container env, got: %v", gotEnv) } - if !foundOther { - t.Errorf("expected OTHER=othervalue in container env, got: %v", envVars) + // OLD_VAR must not survive: updateService replaces (not merges) the env list. + if _, ok := envMap["OLD_VAR"]; ok { + t.Errorf("expected OLD_VAR to be replaced by updateService but it was still present: %v", gotEnv) } } From d1d1899403a46d71d7b065aa6efa05dfe1e36d96 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 3 Apr 2026 21:10:56 +0530 Subject: [PATCH 3/4] test: add create-path regression test and tone down update-path comment Add TestGenerateNewService_ResourceSetsPopulated to pin the first-deploy path: verifies that generateNewService populates the caller-supplied referencedSecrets and referencedConfigMaps sets so CheckResourcesArePresent actually validates them. A regression reintroducing internal sets would cause this test to fail. Tone down the comment on TestUpdateService_EnvsPropagated to accurately reflect that it only exercises the updateService closure directly, not Deploy, ProcessEnvs, or UpdateServiceWithRetry. Ref #3514 --- pkg/knative/deployer_test.go | 47 +++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index 07a3017c9a..4049a9a64c 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" fn "knative.dev/func/pkg/functions" @@ -292,12 +293,10 @@ func setupRegistry(t *testing.T) http.RoundTripper { return trans } -// TestUpdateService_EnvsPropagated verifies that environment variables are correctly -// written into the container spec when updating an existing Knative Service. -// This exercises the update path through deployer.go (the updateService closure), -// specifically the `cp.Env = newEnv` assignment that runs when redeploying a function -// that already exists on the cluster. The former test in this slot (TestGenerateNewService_Envs) -// tested pre-existing behaviour unrelated to the actual code change in this commit. +// TestUpdateService_EnvsPropagated checks that the updateService closure replaces +// (not merges) the container env list with the supplied newEnv slice. +// It calls updateService directly — it does not exercise Deploy, ProcessEnvs, or +// UpdateServiceWithRetry. func TestUpdateService_EnvsPropagated(t *testing.T) { // Simulate an existing deployed service that carries a stale env var. previousService := &servingv1.Service{ @@ -372,6 +371,42 @@ func TestUpdateService_EnvsPropagated(t *testing.T) { } } +// TestGenerateNewService_ResourceSetsPopulated is a regression test for the +// create (first-deploy) path. It proves that generateNewService populates the +// caller-supplied referencedSecrets and referencedConfigMaps sets so that the +// subsequent CheckResourcesArePresent call in Deploy() actually validates them. +// Before the fix, generateNewService allocated its own internal sets and the +// caller's sets remained empty, causing validation to be silently skipped. +func TestGenerateNewService_ResourceSetsPopulated(t *testing.T) { + secretName := "my-secret" + configMapName := "my-configmap" + + f := fn.Function{ + Name: "test-func", + Deploy: fn.DeploySpec{ + Image: "example.com/test:v1", + }, + } + f.Run.Envs.Add("FROM_SECRET", "{{ secret:"+secretName+":key }}") + f.Run.Envs.Add("FROM_CM", "{{ configMap:"+configMapName+":key }}") + + referencedSecrets := sets.New[string]() + referencedConfigMaps := sets.New[string]() + referencedPVCs := sets.New[string]() + + _, err := generateNewService(f, nil, false, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) + if err != nil { + t.Fatalf("generateNewService returned unexpected error: %v", err) + } + + if !referencedSecrets.Has(secretName) { + t.Errorf("expected referencedSecrets to contain %q after generateNewService, got: %v", secretName, referencedSecrets) + } + if !referencedConfigMaps.Has(configMapName) { + t.Errorf("expected referencedConfigMaps to contain %q after generateNewService, got: %v", configMapName, referencedConfigMaps) + } +} + func assertAuth(uname, pwd string, w http.ResponseWriter, r *http.Request) bool { user, pass, ok := r.BasicAuth() if ok && user == uname && pass == pwd { From e1f72ce9b4ee39df09e9935ee697ba71ccc6e24d Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 3 Apr 2026 22:16:33 +0530 Subject: [PATCH 4/4] test: remove redundant TestDeploy_EnvsPassedToDeployer TestDeploy_Envs already covers env var parsing and persistence through the deploy command. The removed test added no coverage beyond what was already there. --- cmd/deploy_test.go | 44 -------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 20eadb68e5..a769cf778a 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -465,50 +465,6 @@ func TestDeploy_Envs(t *testing.T) { } -// TestDeploy_EnvsPassedToDeployer ensures that environment variables provided -// via the -e flag are included in the function passed to the deployer. -// It verifies that --env flags are forwarded through the deploy command -// to the deployer implementation (characterisation test; not a regression -// test for a specific issue). -func TestDeploy_EnvsPassedToDeployer(t *testing.T) { - root := FromTempDirectory(t) - ptr := func(s string) *string { return &s } - - _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) - if err != nil { - t.Fatal(err) - } - - deployer := mock.NewDeployer() - var deployedEnvs fn.Envs - deployer.DeployFn = func(_ context.Context, f fn.Function) (fn.DeploymentResult, error) { - deployedEnvs = f.Run.Envs - return fn.DeploymentResult{ - Status: fn.Deployed, - URL: "http://example.com", - Namespace: "default", - }, nil - } - - cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) - cmd.SetArgs([]string{"--env=MYVAR=myvalue", "--env=OTHER=othervalue"}) - if err := cmd.Execute(); err != nil { - t.Fatal(err) - } - - if !deployer.DeployInvoked { - t.Fatal("deployer was not invoked") - } - - expected := fn.Envs([]fn.Env{ - {Name: ptr("MYVAR"), Value: ptr("myvalue")}, - {Name: ptr("OTHER"), Value: ptr("othervalue")}, - }) - if !reflect.DeepEqual(deployedEnvs, expected) { - t.Fatalf("Expected deployer to receive envs '%v', got '%v'", expected, deployedEnvs) - } -} - // TestDeploy_FunctionContext ensures that the function contextually relevant // to the current command is loaded and used for flag defaults by spot-checking // the builder setting.