Repository navigation
fix(registry): run validating admission on TenantSecret writes - #4226
Conversation
The apiserver hands validating admission to a storage as the ValidateObjectFunc it passes into Create, Update and Delete: mutating admission runs in the handler, but validating webhooks and ValidatingAdmissionPolicies only execute when the storage calls that callback. TenantSecret bound all three to _, so every write answered as if no policy existed. The cluster-wide no-delete guardrail and any webhook written for tenantsecrets were inert on this resource, while application and securitygroup honour them. Call the callbacks on all four write paths, including the create-on-update branch, which is a create and so carries createValidation rather than updateValidation. The object handed to admission is the aggregated TenantSecret the caller submitted or the stored one it replaces, not the backing Secret, so a policy sees the resource it was written against. Assisted-by: LLM Signed-off-by: IvanHunters <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughTenantSecret REST create, update, force-create, and delete operations now invoke validation callbacks. Denied operations return the callback error before changing the backing Secret. Tests cover denied and nil callback paths. ChangesTenantSecret admission validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant REST as tenantsecret.REST
participant Validation as validation callback
participant SecretStorage as backing Secret storage
REST->>Validation: validate TenantSecret operation
Validation-->>REST: return validation result
REST->>SecretStorage: persist or delete Secret when validation succeeds
Merge Risk: ⚪ Minimal · up to TenantSecret admission validation is applied before writes and deletions, and denied operations preserve the backing Secret. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. All four write paths call the callback, every one fails closed, and each is pinned by a test that goes red when I delete it.
Business context: validating webhooks and ValidatingAdmissionPolicies matching tenantsecrets never ran, because this storage dropped the ValidateObjectFunc the apiserver hands it. The cluster-wide deletion guardrail was inert on the resource.
What I checked
I took the write paths from the method set, not from the description. Create, Update, Delete, and no DeleteCollection, so a collection delete is not served and needs no callback. Update splits in two, so four paths, and the diff covers all four:
rest.go:261,CreatecallscreateValidation(ctx, obj)rest.go:410, the create-on-update branch callscreateValidation(ctx, newObj). That branch is a create, so this is right; reaching forupdateValidationthere is the usual mistake.rest.go:419,UpdatecallsupdateValidation(ctx, newObj, secretToTenant(cur))rest.go:447,DeletecallsdeleteValidation(ctx, secretToTenant(current))
PATCH needs no separate entry. rest.Patcher is Getter plus Updater, so it lands on Update.
All four branches are if err := ...; err != nil { return ..., err }. An unreachable webhook or a cancelled context denies the write instead of waving it through, which is the case the control exists for.
This callback is also the only enforcement for anything scoped to tenantsecrets. The backing write goes out through the controller-runtime client to the main apiserver, which runs its own chain against the resulting Secret: different resource, different rules, and the aggregated server's identity instead of the caller's. The two chains do not overlap, so this is load-bearing.
Mutation ladder, restoring between each. Deleting rest.go:261 reds only TestCreateRunsValidatingAdmission, :410 only TestUpdateForceCreateRunsValidatingAdmission, :419 only TestUpdateRunsValidatingAdmission, :447 only TestDeleteRunsValidatingAdmission. The "did not write" assertions are not vacuous either: keeping the call but swallowing its error fails at the denial assertion instead of the "never invoked" one, and moving the backing create ahead of the callback fails at rest_admission_test.go:71, the assertion that the Secret stayed absent. So the fixture does persist, and that assertion tells the two cases apart.
I walked the other storages too. application and securitygroup call all three. tap is Creater plus GracefulDeleter only, and calls both callbacks its verb set has. option, tenantnamespace and tenantmodule are read-only. The description is right that this was the last gap.
Non-blocking
-
The rationale for the nil guard is wrong, in the test comment and in the PR description: "the apiserver passes a nil callback when no admission plugin handles the operation". It does not.
AdmissionToValidateObjectFuncandAdmissionToValidateObjectUpdateFuncreturn a closure that returns nil whenadmitis not aValidationInterface,AdmissionToValidateObjectDeleteFuncreturns one that short-circuits on!mutating && !validating, andwithAuthorizationwraps instead of replacing. No handler path hands the storage a nil func. Keep the guard, it matches the sibling storages and covers direct in-process callers like the tests, but the comment should say that instead of describing a case that never happens. -
Pre-existing, and next door to this diff. The backing writes pass options as
&client.CreateOptions{Raw: opts}and the Update and Delete equivalents, and controller-runtime'sAsCreateOptionsdoeso.Raw.DryRun = o.DryRun, overwritingRaw.DryRunwith the empty structured field. A server-side dry-run write loses its marker and persists. Worth its own fix. It matters a bit more after this change, since admission will now seeIsDryRun()on a request the storage commits anyway.
CI
The one red check is a single chainsaw suite failing on status.readyReplicas: Required value: field not found in the input object against opensearch-test-nodes. I don't think it is attributable: this diff is Go-only inside one aggregated storage and cannot reach an OpenSearch StatefulSet, and the same single-suite failure shows up on unrelated PRs that only touch OpenSearch. Not gating on it.
2 participants
What this PR does
The apiserver hands validating admission to a storage as the
ValidateObjectFuncit passes intoCreate,UpdateandDelete. Mutating admission runs in the handler, but validating webhooks andValidatingAdmissionPolicyexecute only when the storage calls that callback, asrest.AdmissionToValidateObjectFuncink8s.io/apiserverbuilds it for exactly that purpose. TenantSecret bound all three callbacks to_, so every write answered as if no policy existed.The practical effect is that the cluster-wide
cozystack-no-delete-guardrailpolicy, and any webhook an operator writes fortenantsecrets, are inert on this resource.applicationandsecuritygroupcall all three callbacks; TenantSecret was the only write-capable storage in the tree that did not.The callbacks are now called on all four write paths, including the create-on-update branch, which is a create and so carries
createValidationrather thanupdateValidation. What goes to admission is the aggregated TenantSecret the caller submitted, or the stored one it replaces, not the backing Secret, so a policy sees the resource it was written against.This is a behaviour change for anyone who has a policy matching
tenantsecretstoday: writes that silently passed will start being evaluated, and may now be denied. That is the point of the fix, but it is worth saying out loud before it ships.Tests
Each write path is covered by a denying admission callback, asserting both that the callback runs and that the backing Secret is left alone when it denies. A nil callback keeps every path working, which is what the apiserver passes when no plugin handles the operation. Removing any one of the four calls, one at a time, turns the suite red.
Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff. It touches one aggregated storage's write paths: no package added, renamed or removed, novalues.schema.json, no version enum, no changed default, nokindorplural, norelease.prefix, no renamed output Secret or Service, no hand-typed CRD, and nothing underhack/.Release note
Summary by CodeRabbit