refactor: consolidate reference tracking with Tracker in deployer#3830
Conversation
|
[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
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors how referenced Kubernetes resources (Secrets/ConfigMaps/PVCs) are tracked during manifest generation by introducing a References tracker struct, and wires it through Knative and Kubernetes deployer paths to ensure referenced-resource validation runs correctly.
Changes:
- Introduce
k8s.References+NewReferences()and migrate env/volume processing to tracker methods. - Update deploy flows to validate referenced resources using the tracker rather than separate caller-allocated sets.
- Update regression tests to verify the tracker is populated as expected.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| pkg/knative/deployer_test.go | Updates regression test to use the new k8s.References tracker. |
| pkg/knative/deployer.go | Switches Knative deployer resource tracking/validation to k8s.References. |
| pkg/k8s/deployer_test.go | Updates Kubernetes deployer tests to pass References tracker into generation. |
| pkg/k8s/deployer.go | Adds References type, converts env/volume processing to methods, and updates validation API. |
Comments suppressed due to low confidence (2)
pkg/k8s/deployer.go:654
- The env var is appended before checking
err. While the function returns on error (so the slice isn't used), this ordering is easy to misread and can cause accidental use of a nil/invalidValueFromif the control flow changes later. Prefer checkingerrbefore appending toenvVars.
valueFrom, err := t.createEnvVarSource(slices)
envVars = append(envVars, corev1.EnvVar{Name: *env.Name, ValueFrom: valueFrom})
if err != nil {
return nil, nil, err
}
pkg/k8s/deployer.go:518
- On
Forbidden, the message does not include which secret was being accessed (nor the namespace), which makes troubleshooting harder when multiple secrets are referenced. Consider including the secret name (and namespace) in the forbidden branch as well.
if errors.IsForbidden(err) {
errMsg += " Ensure that the service account has the necessary permissions to access the secret.\n"
} else {
errMsg += fmt.Sprintf(" referenced Secret \"%s\" is not present in namespace \"%s\"\n", s, namespace)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // referenced by refs — Secrets, ConfigMaps, or PersistentVolumeClaims — are | ||
| // absent from the given namespace, or if the optional ServiceAccount or | ||
| // imagePullSecret do not exist there. | ||
| func CheckResourcesArePresent(ctx context.Context, namespace string, refs References, referencedServiceAccount, imagePullSecret string) error { |
| if !t.ConfigMaps.Has(sourceName) { | ||
| t.ConfigMaps.Insert(sourceName) | ||
| } |
| if !t.Secrets.Has(sourceName) { | ||
| t.Secrets.Insert(sourceName) | ||
| } |
| if !t.ConfigMaps.Has(sourceName) { | ||
| t.ConfigMaps.Insert(sourceName) | ||
| } |
| if !t.Secrets.Has(sourceName) { | ||
| t.Secrets.Insert(sourceName) | ||
| } |
| if !t.Secrets.Has(*vol.Secret) { | ||
| t.Secrets.Insert(*vol.Secret) | ||
| } |
| if !t.ConfigMaps.Has(*vol.ConfigMap) { | ||
| t.ConfigMaps.Insert(*vol.ConfigMap) | ||
| } |
| if !t.PVCs.Has(*vol.PersistentVolumeClaim.ClaimName) { | ||
| t.PVCs.Insert(*vol.PersistentVolumeClaim.ClaimName) | ||
| } |
lkingland
added
ok-to-test
…ructure - Replaced individual sets for referenced secrets, config maps, and PVCs with a Tracker that encapsulates these references. - Updated the generateDeployment and CheckResourcesArePresent functions to utilize the Tracker, ensuring consistent resource validation. - Modified related tests to verify that the Tracker correctly accumulates resource references during deployment generation. This change enhances the clarity and maintainability of the code by consolidating resource tracking logic.
…ssVolumes functions - Introduced ensureInit method in Tracker to initialize nil sets for Secrets, ConfigMaps, and PVCs, preventing panics on nil-map Insert. - Updated ProcessEnvs and ProcessVolumes functions to call the new Tracker methods, ensuring backward compatibility while encouraging the use of the Tracker directly. - Improved error messages in knative deployer for resource validation failures, enhancing clarity in error reporting. This change improves the robustness of resource tracking and prepares for future refactoring by phasing out deprecated functions.
- Replaced the Tracker structure with References, simplifying the initialization and usage of resource sets for secrets, config maps, and PVCs. - Updated the generateDeployment and service generation functions to utilize the new References type, ensuring consistent resource validation. - Modified tests in both k8s and knative deployers to reflect the changes in the resource tracking mechanism. This refactor enhances code clarity and prepares for future improvements by streamlining resource management.
… 120s to improve reliability during deployment.
Ankitsinghsisodya
force-pushed
the
refactor/issue-3678-consolidate-reference-tracking
branch
from
1f83755 to
5b734f4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3830 +/- ##
=======================================
Coverage 53.44% 53.45%
=======================================
Files 200 200
Lines 23450 23447 -3
=======================================
Hits 12533 12533
+ Misses 9662 9661 -1
+ Partials 1255 1253 -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:
|
Fixes #3678
What
Fixes the three code smells called out in #3678 by introducing a
Trackerstruct (shape b from the issue) that owns the three reference sets and accumulates them via receiver methods.Changes
pkg/k8s/deployer.goReferencesstruct holdingSecrets,ConfigMaps,PVCsassets.Set[string]valuesTrackerstruct embeddingReferencesandNewTracker()constructorProcessEnvs,ProcessVolumes,createEnvFromSource,createEnvVarSourcefrom free functions with*sets.Set[string]out-parameters to*TrackermethodsgenerateDeploymentto accept*Trackerinstead of three pointer parametersCheckResourcesArePresentto acceptReferencesby valuepkg/knative/deployer.gogenerateNewServicesignature to accept*k8s.Trackersetsimportpkg/k8s/deployer_test.go/pkg/knative/deployer_test.gosets.New[string]()+ pointer passing withNewTracker()in all affected testsWhy shape (b) — Tracker receiver
Behavior
No behavior changes. This is a pure shape refactor.
Test plan
go test ./pkg/k8s/ ./pkg/knative/— both passmake check— 0 lint issuesmake test— all pre-existing unit tests pass