fix: pass referenced resource sets to generateNewService for proper validation#3579
fix: pass referenced resource sets to generateNewService for proper validation#3579Ankitsinghsisodya wants to merge 4 commits intoknative:mainfrom
Conversation
…alidation 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 knative#3514
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
bot
added
size/L
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a deploy regression where env vars passed via func deploy -e KEY=VALUE were not reliably propagated all the way into the generated Knative Service container spec (particularly on first deploy where referenced resource tracking could be lost).
Changes:
- Pass caller-owned referenced Secret/ConfigMap/PVC sets into the new-service generation path so resource references are tracked consistently.
- Ensure deploy-time envs (
f.Run.Envs) are preserved through Knative Service generation (containerenv/envFrom). - Add regression tests covering both CLI-to-deployer env propagation and Knative Service env generation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/knative/deployer.go | Threads referenced resource sets into generateNewService so first-deploy validation and env/volume processing use the caller-owned sets. |
| pkg/knative/deployer_test.go | Adds a regression test asserting function envs appear in the generated Knative Service container env list. |
| cmd/deploy_test.go | Adds a regression test asserting --env flags are present in the fn.Function passed to the deployer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/knative/deployer_test.go
Outdated
| // 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 } |
There was a problem hiding this comment.
In this test, the local helper variable ptr shadows the imported knative.dev/pkg/ptr package used elsewhere in this file (e.g., ptr.Bool(...)). This makes the test harder to read and can lead to accidental mistakes when editing. Consider renaming the helper (e.g., strPtr) or using ptr.String(...) consistently instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3579 +/- ##
==========================================
+ Coverage 56.24% 56.36% +0.11%
==========================================
Files 180 180
Lines 20465 20461 -4
==========================================
+ Hits 11511 11533 +22
+ Misses 7755 7727 -28
- Partials 1199 1201 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove TestGenerateNewService_Envs which tested pre-existing behaviour and was mislabelled as a regression test for knative#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 knative#3514.
Ankitsinghsisodya
changed the title
func deploy -e env vars reach the Knative Service spec
Ankitsinghsisodya
force-pushed
the
fix/deploy-env-vars-not-applied
branch
from
14971c1 to
31f05f4
Compare
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 knative#3514
Ankitsinghsisodya
changed the title
TestDeploy_Envs already covers env var parsing and persistence through the deploy command. The removed test added no coverage beyond what was already there.
|
cc @gauron99 |
Changes
generateNewServiceinpkg/knative/deployer.gowas allocating its own localreferencedSecrets,referencedConfigMaps, andreferencedPVCssetsinternally. Those sets were never visible to the caller, so
CheckResourcesArePresenton the first-deploy path was always invoked withempty sets, silently passing validation regardless of whether referenced
Secrets, ConfigMaps, or PVCs actually existed on the cluster.
The fix adds the three sets as parameters to
generateNewServiceand removesthe internal allocations.
ProcessEnvsandProcessVolumesnow populate thecaller-owned sets, so
CheckResourcesArePresentreceives the correct data andcan reject a deployment that references missing resources.
On the test side, Two new tests are added:
TestUpdateService_EnvsPropagatedcalls theupdateServiceclosure directlyagainst a service with a stale env var and asserts that the container env list
is replaced, not merged;
TestGenerateNewService_ResourceSetsPopulatedis aregression test for the create path that asserts the caller-supplied
referencedSecretsandreferencedConfigMapssets are populated aftergenerateNewServicereturns — a future regression that reintroduces internalallocation would cause this test to fail.
relates #3514