You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(lineage-webhook): keep an explicit tenantresource false - #4920
Lets a chart narrow the lineage webhook's internal.cozystack.io/tenantresource verdict without being able to widen it. The webhook computes the verdict from the ApplicationDefinition's secrets selectors on first admission and copied it over whatever label the chart rendered, so a Secret the definition still selected was projected through core.cozystack.io/tenantsecrets whatever the chart said. A "false" rendered on a Secret the webhook admits for the first time (CREATE) is now kept; a rendered "true" is still replaced by the computed verdict. A re-admission (UPDATE with managed-by-cozystack dropped, which the cacert controller does on a selector change) still gets the definition's verdict, because the "false" it carries is the one this webhook wrote earlier.
This is rollout step 2 of the accepted user secrets API proposal: during a kind's conversion the converted chart renders the platform-only <release>-credentials Secret with tenantresource: "false" while the old definition may still select the name, and the label is what keeps that Secret off the projection in the window. It ships on its own because nothing else depends on it and the conversions do.
Behaviour otherwise unchanged: an object with no rendered label gets the definition's verdict as before, and the re-admission short-circuit on managed-by-cozystack is untouched.
Covered by a handler test: on CREATE, a rendered false kept where the selector matches, a rendered true overwritten where it does not, and no rendered label taking the verdict; on a re-admission UPDATE, a grant where the selector now matches and a revoke where it no longer does.
Screenshots
No UI change.
Downstream repositories
Walked the trigger map against the diff: two files under internal/lineagecontrollerwebhook, no CRD, no package, no chart, no values, no hack/ change. The ApplicationDefinition's selector semantics gain one exception, but the website's ApplicationDefinition page does not describe the projection verdict at that depth, so there is nothing there to update.
No downstream repository is affected by this change
fix(lineage-webhook): a Secret created with `internal.cozystack.io/tenantresource: "false"` keeps that label even where its ApplicationDefinition selects the name, so a chart can take a Secret off the tenantsecrets projection; a rendered `"true"` is still overwritten by the definition's verdict.
Summary by CodeRabbit
Bug Fixes
On resource creation, an existing false tenant-resource label is preserved even when the configured selector matches. Otherwise, the label reflects the selector’s verdict. On updates, the label is set according to the selector.
Tests
Added coverage for tenant-resource label behavior on creation and updates, including existing and missing labels.
Reviewing files that changed from the base of the PR and between da8c1ea and 730fd5e.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
The webhook now receives the admission operation when computing the tenant-resource label. On CREATE, it preserves an existing "false" label; otherwise, it uses the resource selector result. Tests cover label patches and admission responses for CREATE and UPDATE requests.
Changes: Tenant-resource label behavior
Layer / File(s)
Summary
Compute and verify the label internal/lineagecontrollerwebhook/webhook.go, internal/lineagecontrollerwebhook/webhook_tenantresource_test.go
computeLabels receives the admission operation. On CREATE, it preserves an existing "false" label; otherwise, the selector result determines the label. Table-driven tests check label patches and allowed admission responses for CREATE and UPDATE requests.
cozystack/cozystack#4921: Adds the credentials API and account Secret lifecycle, which relates to this PR’s admission-label rule for credential Secret projection.
Merge Risk: Minimal · previous review da8c1
No merge-blocking issue was established.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title "fix(lineage-webhook): keep an explicit tenantresource false" matches the main change. The PR keeps a rendered tenantresource "false" label on CREATE when the ApplicationDefinition selects t…
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check
Explanation
The title "fix(lineage-webhook): keep an explicit tenantresource false" matches the main change. The PR keeps a rendered tenantresource "false" label on CREATE when the ApplicationDefinition selects the Secret. The title names the component, the fix type, and the behavior. It does not mention the UPDATE handling, but that is a secondary detail.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Finishing Touches
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
REQUEST CHANGES. A "false" that the webhook set itself on an earlier admission is now read as the chart's narrowing, so the cacert controller can no longer re-grant a projection it revoked.
Blockers
B1: re-admission freezes the old verdict (medium)
internal/controller/cacert/reconciler.go:875-907 re-runs admission after a selector change by dropping only managed-by-cozystack and keeping the other labels, tenantresource included. It relies on the webhook overwriting the old value (the comment at reconciler.go:135-148 says so). With this PR the old "false" looks like the chart's, selected is forced to false, the webhook puts managed-by-cozystack back, and the next reconcile sees the same digest and writes nothing. Revoke still works, grant doesn't, and the controller stays Ready.
It shows up when a definition starts selecting a Secret that already exists (definition and chart can arrive in any order) or on a manual revoke and grant cycle.
I reproduced it with a webhook test: UPDATE, old object with managed-by-cozystack=true, new object without it, tenantresource=false, definition selector that matches. PR code returns "false", main returns "true". Revoke gives "false" on both.
Fix: keep "false" only on CREATE by passing req.Operation into computeLabels. The Secret created while the definition still selects the name is a CREATE anyway. The other way is to drop tenantresource in the cacert controller at reconciler.go:906 and update the two comments.
The tests can't see it. All rows in webhook_tenantresource_test.go are Operation: Create, and TestReconcile_SelectorChange stops before the webhook runs. The package already covers UPDATE in webhook_test.go:154-276, so please add a row with the labels above and UPDATE, expecting "true".
The reason will be displayed to describe this comment to others. Learn more.
Operation isn't checked here. On the cacert re-admission (UPDATE, managed-by-cozystack dropped) this branch sees the webhook's own old "false" and keeps it, see B1. Passing req.Operation into computeLabels and keeping "false" only for CREATE covers the case from the PR description and restores the re-grant.
The reason will be displayed to describe this comment to others. Learn more.
APPROVE. The blocker from my earlier review is fixed and the tests pin it.
The webhook keeps a rendered "false" only on CREATE now (webhook.go:204). The cacert re-admission (UPDATE, managed-by-cozystack dropped) gets the definition's verdict again, so it can re-grant a projection it revoked.
I ran the package tests on the new head and they pass. On a copy I dropped the op == admissionv1.Create condition and the new "a re-admission grants where the definition now selects the name" row goes red. Dropping the keep-false branch altogether turns the CREATE row red. Both halves of the change are covered.
I also looked at what the narrower rule costs. A plain UPDATE, a helm upgrade of the release for example, doesn't reach computeLabels when the object already has all the lineage labels (webhook.go:112), so a "false" the chart set at creation survives the next upgrade. I read that in the code, I didn't try it on a cluster.
The webhook computes the tenantresource verdict from the ApplicationDefinition selectors on first admission and copies it over whatever label the chart rendered, so a chart cannot take a Secret the definition still selects off the tenantsecrets projection. The credentials conversion in design-proposals/user-secrets-api needs exactly that during its migration window: the converted chart renders the platform-only credentials Secret with tenantresource false while the old definition still selects the name.
A rendered false is now kept; a rendered true is still replaced by the computed verdict, so the object can only narrow what the definition grants, never widen it.
Assisted-by: LLM
Signed-off-by: Timofei Larkin <[email protected]>
Comments carry the mechanism, not the reason the change was made; the
reason is in the design doc and the PR.
Assisted-by: LLM
Signed-off-by: Timofei Larkin <[email protected]>
The cacert controller re-runs admission after a selector change by
dropping managed-by-cozystack and keeping the other labels, so the
UPDATE carries the "false" this webhook wrote on an earlier admission.
Reading that as the chart's narrowing froze the old verdict: a
projection, once revoked, could not be granted again. The chart's
narrowing only matters on CREATE, since an existing Secret already
carries managed-by-cozystack and skips the webhook.
Assisted-by: LLM
Signed-off-by: Timofei Larkin <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
area/uncategorizedPR auto-labeler could not map title scope to a known area/*; please reviewkind/bugCategorizes issue or PR as related to a bugsize/LThis PR changes 100-499 lines, ignoring generated files
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
area/uncategorized
myasnikovdaniil
left a comment
What this PR does
Lets a chart narrow the lineage webhook's
internal.cozystack.io/tenantresourceverdict without being able to widen it. The webhook computes the verdict from the ApplicationDefinition'ssecretsselectors on first admission and copied it over whatever label the chart rendered, so a Secret the definition still selected was projected throughcore.cozystack.io/tenantsecretswhatever the chart said. A"false"rendered on a Secret the webhook admits for the first time (CREATE) is now kept; a rendered"true"is still replaced by the computed verdict. A re-admission (UPDATE withmanaged-by-cozystackdropped, which the cacert controller does on a selector change) still gets the definition's verdict, because the"false"it carries is the one this webhook wrote earlier.This is rollout step 2 of the accepted user secrets API proposal: during a kind's conversion the converted chart renders the platform-only
<release>-credentialsSecret withtenantresource: "false"while the old definition may still select the name, and the label is what keeps that Secret off the projection in the window. It ships on its own because nothing else depends on it and the conversions do.Behaviour otherwise unchanged: an object with no rendered label gets the definition's verdict as before, and the re-admission short-circuit on
managed-by-cozystackis untouched.Covered by a handler test: on CREATE, a rendered
falsekept where the selector matches, a renderedtrueoverwritten where it does not, and no rendered label taking the verdict; on a re-admission UPDATE, a grant where the selector now matches and a revoke where it no longer does.Screenshots
No UI change.
Downstream repositories
Walked the trigger map against the diff: two files under
internal/lineagecontrollerwebhook, no CRD, no package, no chart, no values, nohack/change. The ApplicationDefinition's selector semantics gain one exception, but the website's ApplicationDefinition page does not describe the projection verdict at that depth, so there is nothing there to update.Release note
Summary by CodeRabbit
falsetenant-resource label is preserved even when the configured selector matches. Otherwise, the label reflects the selector’s verdict. On updates, the label is set according to the selector.