Create a shared utility for generating hash of a sandbox's TemplateRef name#763
Conversation
…name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name
…name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name
✅ Deploy Preview for agent-sandbox canceled.
|
|
Welcome @lauragalbraith! |
|
Hi @lauragalbraith. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/check-cla |
1 similar comment
|
/check-cla |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| "sigs.k8s.io/controller-runtime/pkg/log" | ||
|
|
||
| sandboxcontrollers "sigs.k8s.io/agent-sandbox/controllers" | ||
|
|
There was a problem hiding this comment.
gofmt changed the content here - let me know if there's any more specific action desired here!
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
Pull request overview
Refactors hash generation for sandbox template references into a shared utility in extensions/controllers, replacing direct calls to sandboxcontrollers.NameHash(template.Name) across the extension controllers. This is preparatory work toward issue #703, which will introduce a namespace-aware hash to avoid cross-namespace collisions in the warm-pool/claim queues.
Changes:
- Adds
SandboxTemplateRefHashandHashUsingSandboxTemplateRefNamehelpers inextensions/controllers/utils.go, with a TODO to deprecate the latter once #703 is fixed. - Replaces all in-package call sites (sandboxclaim, sandboxtemplate, sandboxwarmpool controllers) with the new helper.
- Adds a unit test asserting hash length, determinism, and distinctness across inputs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/controllers/utils.go | Introduces shared SandboxTemplateRefHash and legacy HashUsingSandboxTemplateRefName helpers. |
| extensions/controllers/utils_test.go | New unit test covering determinism, length, and uniqueness of the hash. |
| extensions/controllers/sandboxclaim_controller.go | Routes all template-name hashing through the new helper. |
| extensions/controllers/sandboxtemplate_controller.go | Same refactor; drops now-unused sandboxcontrollers import. |
| extensions/controllers/sandboxwarmpool_controller.go | Replaces direct NameHash calls in label construction and stale-check with the helper. |
| } | ||
|
|
||
| // Check that different inputs produce different hashes | ||
| if results["simple template ref name"] == results["a different template ref name"] { |
There was a problem hiding this comment.
would it be better to iterate over all unique pairs in results to ensure there are no collisions among any of the different test inputs?
k8s-ci-robot
added
size/L
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lauragalbraith, vicentefb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…f name (kubernetes-sigs#763) * Create shared utility for generating hash of a sandbox's TemplateRef name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name * Create shared utility for generating hash of a sandbox's TemplateRef name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name * gofmt and fix typo * add extensions/controllers/utils_test.go * test results more robustly
…f name (kubernetes-sigs#763) * Create shared utility for generating hash of a sandbox's TemplateRef name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name * Create shared utility for generating hash of a sandbox's TemplateRef name This is part of the fix for issue kubernetes-sigs#703. The next step will be creating a new utility which uses the namespace in addition to name * gofmt and fix typo * add extensions/controllers/utils_test.go * test results more robustly
What this PR does / why we need it:
Create a shared utility for generating hash of a sandbox's TemplateRef name
The code was modified with the help of https://antigravity.google/.
The next step will be creating a new utility which uses the namespace in addition to name
Which issue(s) this PR is related to:
This is part of the fix for issue #703.
Release Note
NONE