Skip to content

fix(controller): degrade scope not whole target on RBAC-forbidden namespace#28

Merged
konih merged 2 commits into
mainfrom
feat/target-scope-degrade
Jun 30, 2026
Merged

fix(controller): degrade scope not whole target on RBAC-forbidden namespace#28
konih merged 2 commits into
mainfrom
feat/target-scope-degrade

Conversation

@konih

@konih konih commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • KollectTarget reconcile previously flipped fully Degraded (removing Ready/Synced) whenever HasForbiddenScope was true, even though collection from allowed namespaces continued — violating GUIDELINES.md §1's ErrForbidden rule ("degrade scope, never hard-fail the whole reconcile").
  • Extracted reconcileTargetReady's engine-derived signals into a pure applyTargetReadyState helper so the previously-untested forbidden-scope branch is directly unit-testable without standing up RBAC-restricted envtest fixtures.
  • On a forbidden scope: Ready stays True, Degraded stays absent, Synced carries a new ScopeForbidden reason + message, and a Warning event is recorded. AccessCheckFailed (a different error class — SAR API failure) and sink-unreachable both keep the prior full-degrade behavior.

Test plan

  • go build ./...
  • task verify (no codegen drift)
  • task lint / task arch-lint (0 issues)
  • task test (full unit + envtest, race on)
  • task coverage (79.0%, floor 72%)
  • task scrub (clean)
  • task changelog:verify
  • 3 new unit tests confirmed red (compile failure) before the implementation commit, green after

konih added 2 commits June 30, 2026 21:33
…amespace

Per GUIDELINES.md §1 (ErrForbidden), a forbidden namespace within a
target's scope must degrade that scope, not hard-fail the whole
reconcile. KollectTarget previously flipped fully Degraded (removing
Ready/Synced) whenever HasForbiddenScope was true, even though
collection from allowed namespaces continued.

Extract reconcileTargetReady's engine-derived signals into a pure
applyTargetReadyState helper so the scope-degrade branch is directly
unit-testable without standing up RBAC-restricted envtest fixtures.
Ready now stays true on a forbidden scope; Synced carries the new
ScopeForbidden reason/message and a Warning event is recorded.
AccessCheckFailed (SAR API failure, a different error class) keeps
hard-failing as before.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/controller/kollecttarget_controller.go 90.47% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@konih

konih commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Review summary (gate matrix + analysis)

Gate Result Notes
go build ./...
task verify no codegen drift
task lint / arch-lint golangci-lint 0 issues
task test full unit + envtest, race on
task coverage 79.0% (floor 72%)
task scrub clean
task changelog:verify

TDD: 3 new unit tests (TestApplyTargetReadyState_*) confirmed red (compile failure on applyTargetReadyState/reasonScopeForbidden not existing) before the implementation commit, green after.

Functional correctness: HasForbiddenScope previously routed into setDegraded unconditionally, removing Ready/Synced for the whole target even though collection from allowed namespaces continued — violating GUIDELINES.md §1's ErrForbidden rule. Now Ready stays true, Synced carries ScopeForbidden, a Warning event is recorded, and the genuinely-different AccessCheckFailed/sink-unreachable paths are unchanged (still hard-degrade, by design — different error classes).

One P3 observation, not blocking: if both HasForbiddenScope and HasAccessCheckFailure are true simultaneously, a ScopeForbidden Warning event is now emitted before falling through to the full AccessCheckFailed degrade (previously impossible since the old code short-circuited on the first true branch). Harmless — more observability, not less correct.

No P0/P1 findings. I can't self-approve this (would defeat the review requirement) — flagging here for your review/approval on GitHub.

@konih
konih merged commit 0cd7e8d into main Jun 30, 2026
25 checks passed
@konih
konih deleted the feat/target-scope-degrade branch June 30, 2026 21:15
konih added a commit that referenced this pull request Jun 30, 2026
PR squash-merges (#28-#32) added (#NN) reference suffixes to commit
subjects that git-cliff renders into changelog entries; the committed
CHANGELOG.md from local generation predated those merges and drifted,
failing the Preflight changelog:verify gate on main.
konih added a commit that referenced this pull request Jul 10, 2026
…espace (#28)

* 🐛 fix(controller): degrade scope not whole target on RBAC-forbidden namespace

Per GUIDELINES.md §1 (ErrForbidden), a forbidden namespace within a
target's scope must degrade that scope, not hard-fail the whole
reconcile. KollectTarget previously flipped fully Degraded (removing
Ready/Synced) whenever HasForbiddenScope was true, even though
collection from allowed namespaces continued.

Extract reconcileTargetReady's engine-derived signals into a pure
applyTargetReadyState helper so the scope-degrade branch is directly
unit-testable without standing up RBAC-restricted envtest fixtures.
Ready now stays true on a forbidden scope; Synced carries the new
ScopeForbidden reason/message and a Warning event is recorded.
AccessCheckFailed (SAR API failure, a different error class) keeps
hard-failing as before.

* 📄 docs(changelog): sync scope-degrade fix entry
konih added a commit that referenced this pull request Jul 10, 2026
PR squash-merges (#28-#32) added (#NN) reference suffixes to commit
subjects that git-cliff renders into changelog entries; the committed
CHANGELOG.md from local generation predated those merges and drifted,
failing the Preflight changelog:verify gate on main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants