Update README and development documentation#64
Merged
BergCyrill merged 2 commits intomainfrom May 8, 2026
Merged
Conversation
…tation and toolchain setup Signed-off-by: Cyrill Berg <cyrill.berg@opendefense.cloud>
📝 WalkthroughWalkthroughThis PR updates project documentation to reflect an architectural shift in dependency resolution: replacing an indexed-cache model with on-demand dependent listing via kcp front-proxy. It also introduces Nix flake–based development setup instructions across README and a new docs/development.md guide. ChangesArchitecture Documentation & Development Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
BergCyrill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/development.md (1)
58-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate webhook file descriptions to match the new on-demand model.
These entries still describe a per-rule dependent cache model, which conflicts with the updated architecture in this PR (metadata registry + on-demand listing at admission time).
Proposed doc patch
- rule_cache_manager.go Per-rule indexed cache lifecycle manager - rule_registry.go Thread-safe registry of rule caches + rule_cache_manager.go Reconciles DependencyRules into in-memory rule metadata + rule_registry.go Thread-safe registry for rule metadata lookups🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development.md` around lines 58 - 60, Update the three file descriptions in docs/development.md to reflect the new metadata-registry + on-demand listing architecture instead of the old per-rule cache model: replace the description for rule_cache_manager.go with something like "Cache lifecycle manager (deprecated) — previously per-rule indexed cache, now unused by on-demand model" or remove it, change rule_registry.go to "Thread-safe metadata registry for rule metadata and lookup used at admission time", and change deletion_validator.go to "Admission webhook handler that queries the metadata registry and performs on-demand listing/validation at admission time"; edit the three entries (rule_cache_manager.go, rule_registry.go, deletion_validator.go) accordingly so the docs match the new design.README.md (1)
221-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRBAC section contradicts the new admission data path.
This paragraph says the webhook watches dependents via VW + permissionClaims, but the README now documents on-demand listing via kcp front-proxy. Please align this section to avoid incorrect RBAC expectations.
Proposed doc patch
-No shard-wide RBAC is needed. The webhook watches dependent resources through the -dep-ctrl APIExport's virtual workspace, authorized by dynamically managed -permissionClaims. Providers accept these claims in their APIBinding. +The webhook does not watch dependent resources via the dep-ctrl virtual workspace. +Instead, on DELETE admission it lists dependents on demand in the consumer workspace +through the kcp front-proxy. Required permissions for those list/get calls are granted +via bootstrap RBAC (as documented in the deployment/development guides), while +`permissionClaims` on dep-ctrl remain for webhook installation in provider workspaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 221 - 223, The README paragraph that claims "The webhook watches dependent resources through the dep-ctrl APIExport's virtual workspace, authorized by dynamically managed permissionClaims" is now incorrect; update the RBAC section to describe the new admission data path: state that the webhook performs on-demand listing via the kcp front-proxy (not via a virtual workspace), explain the RBAC scopes/roles required for the front-proxy/listing path, and remove or replace references to "dep-ctrl APIExport's virtual workspace" and "permissionClaims" so the text matches the implemented on-demand listing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/development.md`:
- Around line 58-60: Update the three file descriptions in docs/development.md
to reflect the new metadata-registry + on-demand listing architecture instead of
the old per-rule cache model: replace the description for rule_cache_manager.go
with something like "Cache lifecycle manager (deprecated) — previously per-rule
indexed cache, now unused by on-demand model" or remove it, change
rule_registry.go to "Thread-safe metadata registry for rule metadata and lookup
used at admission time", and change deletion_validator.go to "Admission webhook
handler that queries the metadata registry and performs on-demand
listing/validation at admission time"; edit the three entries
(rule_cache_manager.go, rule_registry.go, deletion_validator.go) accordingly so
the docs match the new design.
In `@README.md`:
- Around line 221-223: The README paragraph that claims "The webhook watches
dependent resources through the dep-ctrl APIExport's virtual workspace,
authorized by dynamically managed permissionClaims" is now incorrect; update the
RBAC section to describe the new admission data path: state that the webhook
performs on-demand listing via the kcp front-proxy (not via a virtual
workspace), explain the RBAC scopes/roles required for the front-proxy/listing
path, and remove or replace references to "dep-ctrl APIExport's virtual
workspace" and "permissionClaims" so the text matches the implemented on-demand
listing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14d8e57c-0a81-4daf-8d9c-a4a04f5b35a6
📒 Files selected for processing (2)
README.mddocs/development.md
Perseus985
previously approved these changes
May 8, 2026
Contributor
Perseus985
left a comment
Perseus985
left a comment
There was a problem hiding this comment.
Relates to the Old Jira Issue I guess
…ations Signed-off-by: Cyrill Berg <cyrill.berg@opendefense.cloud>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 220-226: The README currently recommends granting the webhook
ServiceAccount `*/* get,list` per-shard under `system:admin`, which is too
broad; update the documentation to mark this wildcard `*/* get,list` binding as
a break-glass fallback only, and replace the default recommendation with a
scoped RBAC approach that limits verbs, API groups, resources, and
namespaces/workspaces to only what the webhook actually needs; mention
`system:admin`, the webhook ServiceAccount, and `BootstrapPolicyAuthorizer` to
explain why the per-shard binding exists, but explicitly advise implementing
least-privilege roles and per-consumer-scoped bindings (or a generated role per
workspace) as the primary recommendation and reserve the `*/* get,list`
per-kcp-shard binding for emergency use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Perseus985
approved these changes
May 8, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Modified README:
Summary by CodeRabbit