fix: apply resource validation to first-deploy path in k8s and keda deployers#3671
fix: apply resource validation to first-deploy path in k8s and keda deployers#3671Ankitsinghsisodya wants to merge 2 commits intoknative:mainfrom
Conversation
…generation logic - Introduced TestInt_ResourceValidationOnFirstDeploy to ensure that deploying a function referencing a non-existent Secret fails, while subsequent deploys succeed after the Secret is created. - Updated the generateDeployment function to accept referenced resource sets (secrets, config maps, PVCs) for proper validation during deployment. - Adjusted related tests to utilize the new validation logic, ensuring comprehensive coverage for resource checks on first deployments. 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
the
size/M
|
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. |
knative-prow
Bot
added
the
needs-ok-to-test
|
cc @matejvasek |
There was a problem hiding this comment.
Pull request overview
Fixes a gap in the Kubernetes and KEDA deployers where referenced Secrets/ConfigMaps/PVCs were not validated on the first-deploy path due to referenced-resource sets being allocated inside generateDeployment and never surfaced to the caller.
Changes:
- Pass caller-owned referenced-resource sets into
k8s.Deployer.generateDeploymentso env/volume processing populates them. - Invoke
CheckResourcesArePresenton both create (first deploy) and update (redeploy) paths before hitting the Kubernetes API. - Add integration coverage for first-deploy resource validation for both k8s and keda deployers; update unit tests for the new signature.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/k8s/deployer.go | Plumbs referenced-resource sets through deployment generation and enforces resource validation on create/update paths. |
| pkg/k8s/deployer_test.go | Updates unit tests to match the new generateDeployment signature. |
| pkg/k8s/deployer_int_test.go | Adds integration test to ensure first-deploy validation rejects missing referenced resources. |
| pkg/keda/deployer_int_test.go | Adds the same integration coverage for the KEDA deployer (delegates to k8s deployer). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| referencedSecrets := sets.New[string]() | ||
| referencedConfigMaps := sets.New[string]() | ||
| referencedPVCs := sets.New[string]() | ||
|
|
||
| deployment, err := d.generateDeployment(f, namespace, daprInstalled, &referencedSecrets, &referencedConfigMaps, &referencedPVCs) | ||
| if err != nil { |
Updated error messages in the Deploy function to clarify that failures are due to validation of referenced resources rather than deployment creation or updates. This change enhances the clarity of error reporting when resources are missing or invalid. Ref knative#3514
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3671 +/- ##
==========================================
+ Coverage 56.61% 56.65% +0.04%
==========================================
Files 181 181
Lines 20853 20862 +9
==========================================
+ Hits 11806 11820 +14
+ Misses 7833 7830 -3
+ Partials 1214 1212 -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:
|
matejvasek
requested review from
creydr,
gauron99 and
lkingland
and removed request for
dsimansk and
jrangelramos
Problem
generateDeploymentinpkg/k8s/deployer.goallocated its own localreferencedSecrets,referencedConfigMaps, andreferencedPVCssetsinternally. Those sets were never visible to the caller, so
CheckResourcesArePresentwas never called on the k8s deploy path —silently passing validation regardless of whether referenced Secrets,
ConfigMaps, or PVCs actually existed on the cluster.
The keda deployer is a thin wrapper around the k8s deployer and inherited
the same gap.
This is the same class of bug fixed for the knative deployer in #3579.
Pass the three sets as parameters into
generateDeploymentinstead ofallocating them locally.
Deploy()now:generateDeploymentCheckResourcesArePresentaftergenerateDeploymenton both thecreate path (first deploy) and the update path (redeployment), before
touching the API
The keda deployer gets the fix for free since it delegates to the k8s
deployer.
Tests
TestInt_ResourceValidationOnFirstDeploywired inpkg/k8s/deployer_int_test.goand
pkg/keda/deployer_int_test.go— exercises the same shared scenariofrom
pkg/deployer/testing/integration_test_helper.goused by theknative deployer since fix: pass referenced resource sets to generateNewService for proper validation #3579
pkg/k8s/deployer_test.goupdated to match thenew
generateDeploymentsignature