feat(rbac)!: least-privilege RBAC, a strict chart values schema, and informers that stop on 403#223
Merged
Conversation
ServesWatchable asks what the API server SERVES. It is not what this ServiceAccount may READ. An ordinary apiserver serves apiregistration.k8s.io whether or not the operator's ClusterRole names apiservices, so a least-privilege install sails past the "is it served?" gate, starts the informer, and takes a 403 on the reflector's first LIST — which client-go then retries forever. That is the same benign, endlessly repeated noise the unserved-resource gate exists to remove, and exactly how a real error gets missed. Both trigger informers now install a watch error handler. A Forbidden stops that informer and logs once; every other error stays with the reflector's own backoff, because a 500 or a reset connection is transient and tearing the informer down would turn a blip into a lost trigger. The reflector routes LIST errors through the same handler, so the denial is caught where it actually happens. A 403 is authoritative and will not resolve by retrying, but it CAN be granted later, so the resource is un-started rather than blacklisted: the next successful catalog refresh re-arms it, no restart. That required giving each trigger its own informer and context. The shared dynamicSharedInformerFactory records an informer as started forever, so a resource stopped for any reason could never be re-armed through it. These two resources are conveniences — they only make the catalog notice a new CRD or aggregated API sooner than its periodic tick would. Failing closed and quiet is the honest answer to "you may not read this". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
config/rbac/role.yaml asked for `apiGroups: ["*"], resources: ["*"], verbs: [get,list,watch]`
and, beside it, `secrets: [get,list,watch,create,update,patch]`. The wildcard is honest about
the general case — a WatchRule may name any type, including one installed later — but RBAC is
ADDITIVE, so while it sat in the manager role no chart value could take back the cluster-wide
Secret read it implied. A reverser asked to mirror two CRDs on a management cluster held read
access to every credential in it.
It does not need them. The manager has held no Secret informer since Secret values stopped
being retained in the control plane: Cache.DisableFor excludes them, so every read is a direct
get of a Secret a GitProvider or GitTarget names by name. Nothing calls list, watch or patch on
a Secret. The marker was never updated to match the binary, so the shipped role still asked for
what the code had stopped using.
Three changes, none of which alter what a default install can do:
- The wildcard moves to its own ClusterRole, gitops-reverser-watch-any-resource, created by
default (chart: rbac.watchAnyResource, kustomize: watch-any-resource-role.yaml) and
droppable whole. It is deliberately not a kubebuilder marker: controller-gen would fold it
straight back into the role it must stay out of.
- The manager role's secrets rule narrows to get;create;update — what the binary calls.
- customresourcedefinitions and apiservices become explicit rules. They were reachable only
through the wildcard, and the API-resource catalog and its trigger informers read both, so
a narrowed install would have 403'd on them.
Set rbac.watchAnyResource=false, grant get/list/watch on the types your WatchRules name, and
the reverser runs with zero cluster-wide Secret access. Pinned by chart-render tests that
assert no rendered ClusterRole grants Secret list/watch once the wildcard is off, and that the
generated manager role never carries the wildcard itself. Documented in docs/rbac.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Teardown closes the watch's result channel AND cancels the context, so both arms of the streaming select are ready at once — and Go picks among ready select cases at random. Roughly half of all clean shutdowns therefore returned "target watch result channel closed" instead of nil. Nothing retried on it, but every teardown could log a failure that had not happened, and a log that cries wolf on every shutdown is a log nobody reads. The context is the authority: the channel closing while ctx is done means we closed it. A channel that closes while the context is still live is still a real error — the watch died under us and the caller must reopen — so that path is unchanged, and pinned by its own test. Both streaming loops (the replay session and the live stream) shared the bug and now share targetWatchClosedErr. Found while stress-testing the API-surface trigger informers: this surfaced as an intermittent failure of TestTargetWatchReplayAndStream_FallsBackWhenReplayWatchIsForbidden under -race, which reproduces on main and is not caused by that change. `task test` does not pass -race, so CI never saw it. The regression test repeats 200 times because a coin flip proves nothing once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change separates manager and watch RBAC, adds configurable Helm watch permissions, updates forbidden informer lifecycle handling, fixes shutdown watch errors, adds chart schema and HTTP/2 wiring, clarifies Redis attribution requirements, and normalizes zero-value Redis key prefixes. ChangesRBAC permission separation
Forbidden trigger informer lifecycle
Target watch shutdown handling
Chart and runtime configuration
Sequence Diagram(s)sequenceDiagram
participant CatalogRefresh
participant TriggerInformer
participant KubernetesAPI
CatalogRefresh->>TriggerInformer: Start informer for discovered GVR
TriggerInformer->>KubernetesAPI: LIST and WATCH
KubernetesAPI-->>TriggerInformer: Return 403 Forbidden
TriggerInformer->>CatalogRefresh: Stop and record GVR
CatalogRefresh->>TriggerInformer: Re-arm after later refresh
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/main_chart_rbac_test.go (1)
40-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: skip gracefully when the
helmbinary is absent.
exec.Command("helm", ...)fails with a "helm template failed" message when helm isn't installed, obscuring the real cause for local runs. ALookPathguard makes the dependency explicit.♻️ Optional guard
+ if _, err := exec.LookPath("helm"); err != nil { + t.Skip("helm binary not found in PATH") + } out, err := exec.Command("helm", args...).CombinedOutput() require.NoErrorf(t, err, "helm template failed: %s", out)🤖 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 `@cmd/main_chart_rbac_test.go` around lines 40 - 41, In the Helm invocation within the test, add an exec.LookPath("helm") check before calling exec.Command and skip the test gracefully with a clear message when the binary is unavailable; retain the existing command execution and assertion when Helm is found.
🤖 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 `@docs/UPGRADING.md`:
- Around line 12-26: Clarify the RBAC documentation’s “Nothing is taken away”
statement to acknowledge that default installs no longer grant Secret patch
permission, since the replacement watch-any-resource role only grants get, list,
and watch. Update the wording around the manager role and watchAnyResource
behavior to distinguish unchanged operator functionality from changed effective
permissions, or retain patch in the replacement role if the operator requires
it.
---
Nitpick comments:
In `@cmd/main_chart_rbac_test.go`:
- Around line 40-41: In the Helm invocation within the test, add an
exec.LookPath("helm") check before calling exec.Command and skip the test
gracefully with a clear message when the binary is unavailable; retain the
existing command execution and assertion when Helm is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e5ebf98-6302-4872-bd33-6f41fff031cd
📒 Files selected for processing (19)
.coverage-baselinecharts/gitops-reverser/templates/rbac.yamlcharts/gitops-reverser/values.yamlcmd/main_chart_rbac_test.goconfig/rbac/kustomization.yamlconfig/rbac/role.yamlconfig/rbac/watch-any-resource-role.yamlconfig/rbac/watch-any-resource-rolebinding.yamldocs/README.mddocs/UPGRADING.mddocs/rbac.mdinternal/controller/gitprovider_controller.gointernal/controller/gittarget_controller.gointernal/watch/api_surface_triggers_forbidden_test.gointernal/watch/api_surface_triggers_test.gointernal/watch/manager.gointernal/watch/manager_catalog.gointernal/watch/target_watch.gointernal/watch/target_watch_shutdown_test.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…e code does Review polish from the merged #221, plus two things it turned up. --redis-addr's help said an empty value was "Incompatible with --author-attribution=true or --admission-webhook". Only the first half is true. validateAuditConfig requires Redis for attribution and returns nil otherwise, and validateAdmissionWebhookConfig says so in a comment: the admission webhook stays enabled without Redis and no-ops command-author capture, so CommitRequests commit as the configured committer. The chart README already documented the real behavior; the flag help was the drift. Pinned now by a flag-combination test, which also caught that --admission-webhook-cert-path has no default and is required whenever the webhook is on -- a TLS requirement, not a Redis one. watchCursorKey built its key from the raw s.keyPrefix while every other key family resolves it at use. Only NewRedisStore constructs a RedisStore today, so nothing was mis-keyed, but the point of resolveKeyPrefix is that a store built any other way still writes under the default root rather than into an unprefixed keyspace indistinguishable from another tool's keys. Made consistent before the key contract freezes, with a zero-value-store test that fails without it. f8's closing line linked ../multi-tenant/README.md, which does not exist here -- those design docs live on the multi-tenant branch. Dropped the reference rather than pointing at a file this repo does not have. docs/README.md indexed docs/serious-bug/, deleted in 7bd1f32 and never unindexed: same bug, removed too. Also: the chart README gains queue.redis.keyPrefix, queue.redis.db, rbac.create and rbac.watchAnyResource rows; docs/configuration.md explains the prefix after the Redis example; and the chart-args contract test gains attribution-enabled cases, so the audit flag block the default path never renders is parsed by the real flag parser too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The opt-out boolean told a user the wildcard was bad without helping them leave it: they
still had to hand-write a ClusterRole, bind it to the right ServiceAccount, and know which
rules the manager role already carries. Most would keep the wildcard.
rbac:
watchTypes:
mode: any # cluster-wide read; the reverser can read every Secret
selected: [] # mode: selected -> read exactly these types, nothing else
The chart renders the ClusterRole and binding for both modes, so least privilege is a value
change rather than a piece of RBAC to maintain and keep in sync with the ServiceAccount.
`any` renders exactly the wildcard rule the boolean did, so the default install is unchanged.
Verbs are never the user's to choose. A watched type is only ever read, so the chart fixes
get/list/watch and refuses a `verbs:` key rather than let someone grant `create` to something
that will never write. It also refuses an unknown mode, and `selected` with an empty list --
which would leave the operator authorized to watch nothing, a silent failure that looks
exactly like a broken WatchRule.
The values comment now names the consequence of `any` instead of describing the mechanism:
the reverser can read every Secret in the cluster, including credentials that have nothing to
do with it. The commented example follows configmaps and deployments, which is what someone
reading it is most likely to want.
Renames the kustomize pair to gitops-reverser-watch-any to match the chart's -watch-<mode>.
Both never shipped -- rbac.watchAnyResource was introduced earlier on this branch -- so this
replaces it outright rather than deprecating it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mplate guards
The five `fail` calls guarding rbac.watchTypes were a hand-rolled type system. helm has one:
values.schema.json, enforced on template, lint, install and upgrade, and carried inside the
packaged chart so it protects a real `helm install` and not just a render from this repo.
Every rule the guards expressed is now a schema rule, and the errors name the offending path
rather than a sentence someone wrote once:
at '/rbac/watchTypes/mode': value must be one of 'any', 'selected'
at '/rbac/watchTypes/selected': minItems: got 0, want 1
at '/rbac/watchTypes/selected/0': missing property 'apiGroups'
at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed
`additionalProperties: false` on rbac buys one the guards could not: an unknown key is an
error rather than a silent no-op. `rbac.watchAnyResoure` -- or a leftover `watchAnyResource`
from earlier on this branch -- now fails the install instead of quietly leaving the wildcard
in place, which is precisely how someone believes they narrowed access and has not.
The schema is deliberately partial: it constrains rbac, whose shape is load-bearing, and
leaves the rest of values.yaml free-form until a value needs a rule. Everything else still
validates, because unspecified properties are permitted at the top level.
Trade-off, stated: `helm template --skip-schema-validation` bypasses all of it. The template
then renders a ClusterRole with no rules, so the reverser is authorized to watch nothing --
it fails closed, not open. That is the right way round, and the tests cover it by asserting
the schema errors rather than the template's.
The one remaining `fail` stays: it reports a missing generated config/role.yaml, which is a
file, not a value, and no values schema can see it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends the rbac-only schema to every value. Objects the chart owns are closed
(additionalProperties: false), so a typo fails the install instead of silently doing nothing:
`--set replicaCounts=2` or `--set servers.metric.port=8080` now name the offending path rather
than rendering the defaults and leaving the operator to wonder. Kubernetes pass-through objects
-- securityContext, affinity, resources, volumes, tolerations -- stay open, because their shape
belongs to Kubernetes and not to this chart. `global` stays open too: a parent chart may set it.
Where the binary already had a contract, the schema states it:
logging.level enum debug|info|error|panic -- `warn` is NOT a zap level. The chart
renders --zap-log-level=warn happily and the pod crash-loops on it.
logging.stacktraceLevel enum info|error|panic (no debug)
logging.encoder enum json|console
queue.redis.keyPrefix pattern + maxLength, the same character class ValidateKeyPrefix enforces.
A quote in it is what CrashLoopBackOff'd the manager on this branch.
servers.admission.timeoutSeconds 1..30, which is what the apiserver accepts
auditService.nodePort 30000..32767
durations Go duration pattern (ttl, grace, timeouts, cert lifetimes)
Two suspicious things fell out of diffing what values.yaml declares against what the templates
actually read:
1. servers.enableHTTP2 was DEAD. No template referenced it, so setting it changed nothing, while
its comment promised a mitigation for the HTTP/2 Rapid-Reset CVE class. The effective behavior
was safe by accident -- the binary's --enable-http2 defaults to false -- but a value that reads
like a security control and does nothing is worse than no value. Wired to the flag, rendered
only when true, so the default install is byte-identical. Pinned by a chart-args case that
fails against the unwired template.
2. env, volumes and volumeMounts are read by the Deployment but were never declared. They were
invisible to anyone reading values.yaml, and a closed top-level schema would have broken them.
Declared, documented, and covered.
Verified against every real invocation: chart defaults, `helm lint`, the e2e `helm upgrade
--install` flags, `task dist-install`, quickstart, attribution, and rbac selected mode.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ale future plans
Three doc problems, one of them a correctness bug in a security claim.
1. "zero cluster-wide Secret access" was FALSE. `selected` mode removes the wildcard, so the
reverser can no longer list or watch Secrets -- but the manager ClusterRole still grants
secrets get/create/update, cluster-scoped, because a GitProvider may name a Secret in any
namespace. A selected install cannot DISCOVER a Secret; it can still read one whose name it
knows. The tests always said this ("cannot enumerate Secrets", "does not grant Secret
list/watch"); four prose sites said something stronger. They now say what the tests say, and
docs/rbac.md carries a verb-by-verb table of what stays. Scoping get/create/update to the
referenced namespaces needs a namespaced Role the chart cannot render without the manifests --
named as remaining work rather than quietly implied.
2. docs/security-model.md contradicted this branch. It still said the Secret narrowing was "not
yet an RBAC change" and that the default install grants secrets get;list;watch. Rewritten
around the two ClusterRoles, with the parts of the (now deleted) retention plan that are
product truth rather than plan: no Secret informer, cache DisableFor, direct GET by name, the
5-minute steady reconcile as the rotation path, and public-recipient-only SOPS. Two mermaid
diagrams, both parsed with the real mermaid parser rather than eyeballed.
3. The future/ plans were describing a world this branch deleted. secret-value-retention-plan.md
was implemented; scoped-rbac-least-privilege-plan.md and secret-handling-roadmap.md both still
opened with "Track B: not started" and documented the wildcard-in-manager-role that no longer
exists. All three removed. What survives is two short docs: the age-recipient metadata idea
(with the annotation-forgery rule that makes it dangerous to get wrong), and the three genuinely
open least-privilege items -- never mirror Secrets, permission-aware followability, and the
namespaced-Role/RBAC-generator work. Every inbound link repointed; no link left dangling.
Also: root README lists docs/rbac.md beside security-model.md, and the aggregated-apiserver design
note no longer claims config/rbac/role.yaml holds the wildcard -- it moved to watch-any-role.yaml.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/rbac.md`:
- Line 57: Update the error-output code fence in docs/rbac.md to specify the
text language identifier by changing the opening fence to ```text, satisfying
MD040.
- Around line 77-97: Qualify the selected-mode Secret-enumeration claims in the
RBAC documentation: explain that selecting core `secrets` grants the
corresponding `get/list/watch` permissions through the watch role, so the
guarantee only applies when `secrets` is not selected. Update the “What selected
does not remove” table and surrounding statements to reflect this conditional
behavior, or instead enforce rejection of `secrets` in the chart schema if
enumeration must never be permitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dd179d8-e9d1-4588-a78a-fafdd66d1a82
📒 Files selected for processing (30)
README.mdcharts/gitops-reverser/README.mdcharts/gitops-reverser/templates/deployment.yamlcharts/gitops-reverser/templates/rbac.yamlcharts/gitops-reverser/values.schema.jsoncharts/gitops-reverser/values.yamlcmd/main.gocmd/main_chart_args_test.gocmd/main_chart_rbac_test.gocmd/main_redis_flags_test.goconfig/rbac/kustomization.yamlconfig/rbac/watch-any-role.yamlconfig/rbac/watch-any-rolebinding.yamldocs/README.mddocs/UPGRADING.mddocs/configuration.mddocs/design/e2e-aggregated-apiserver-test-design.mddocs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.mddocs/design/manifest/gitpathaccepted-projection-race-and-external-drift.mddocs/design/reconcile-triggering.mddocs/future/least-privilege-remaining-work.mddocs/future/publish-age-recipients-as-metadata.mddocs/future/scoped-rbac-least-privilege-plan.mddocs/future/secret-handling-roadmap.mddocs/future/secret-value-retention-plan.mddocs/rbac.mddocs/security-model.mdinternal/queue/key_prefix_test.gointernal/queue/redis_store.gointernal/watch/manager.go
💤 Files with no reviewable changes (3)
- docs/future/scoped-rbac-least-privilege-plan.md
- docs/future/secret-handling-roadmap.md
- docs/future/secret-value-retention-plan.md
✅ Files skipped from review due to trivial changes (10)
- README.md
- docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md
- docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md
- cmd/main.go
- docs/configuration.md
- docs/design/reconcile-triggering.md
- docs/README.md
- docs/design/e2e-aggregated-apiserver-test-design.md
- charts/gitops-reverser/README.md
- docs/future/least-privilege-remaining-work.md
🚧 Files skipped from review as they are similar to previous changes (2)
- config/rbac/kustomization.yaml
- internal/watch/manager.go
sunib
changed the title
This was referenced Jul 10, 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
1 participant
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.
Ships RBAC that matches what the operator actually does at runtime, together with the bug that
narrowing the role would otherwise expose, a chart values schema that makes the new knob safe to use,
and three defects found along the way.
Read
docs/rbac.mdanddocs/UPGRADING.mdbefore upgrading.A default install keeps the same effective permissions. What changes is that the parts can now be
separated, and that a mis-set value fails the install instead of the pod.
1.
feat(rbac)!the wildcard cluster read becomes a mode, not a factconfig/rbac/role.yamlgrantedapiGroups: ["*"], resources: ["*"], verbs: [get,list,watch]and,beside it,
secrets: [get,list,watch,create,update,patch].RBAC is additive. While the wildcard sat in the manager role, no chart value could take back the
cluster-wide Secret read it implied, so deleting the Secret
list/watchverbs on their own wouldhave changed nothing. A reverser asked to mirror two CRDs on a management cluster held read access to
every credential in it.
It never needed them. The manager holds no Secret informer (
Cache.DisableFor), so every read isa direct
getof a Secret aGitProvider/GitTargetnames. Nothing callslist,watchorpatchon a Secret. The marker was simply never updated to match the binary.
<release>-watch-any, its own ClusterRole (mode: any, the default)mode: selectedrenders<release>-watch-selectedfrom your listsecretsget,list,watch,create,update,patchget,create,updatecustomresourcedefinitions,apiservicesThe chart renders the ClusterRole and its binding for both modes, so least privilege is a values
change rather than RBAC to maintain and keep in sync with the ServiceAccount.
What
selecteddoes and does not buyWith
mode: selectedthe reverser cannot enumerate Secrets: nolist, nowatch, anywhere. Itcan still
geta Secret whose name and namespace it already knows, andcreate/updatethe ones itgenerates, because the manager role's Secret verbs stay cluster-scoped. Enumeration is what turns a
mirroring operator into a credential harvester, so this is a real reduction, but it is not zero Secret
access and the docs no longer claim otherwise. Scoping those verbs per namespace needs a namespaced
Rolethe chart cannot render without the manifests; tracked indocs/future/least-privilege-remaining-work.md.2.
fix(watch)stop an API-surface trigger informer RBAC deniesServesWatchableasks what the API server serves. That is not what this ServiceAccount mayread. An ordinary apiserver serves
apiregistration.k8s.iowhether or not your ClusterRole namesapiservices, so a least-privilege install sails past the "is it served?" gate, starts the informer,and takes a
403on the reflector's first LIST, which client-go then retries forever. That is thesame benign, endlessly repeated noise the unserved-resource gate exists to remove.
Forbiddenstops that one informer and logs once. The reflector routes LIST errors through thesame handler, so the denial is caught where it happens.
and tearing the informer down would turn a blip into a lost trigger.
A 403 is authoritative but can be granted later, so the resource is un-started rather than
blacklisted: the next catalog refresh re-arms it, no restart. That needed one informer and context per
trigger, because a shared
dynamicSharedInformerFactoryrecords an informer as started forever.Built red-test-first: four tests against a fake dynamic client that 403s
apiservices, failing onbehaviour before the fix, covering stop, log-once, re-arm-on-grant, and not stopping on a 500.
This is why the two halves ship together. Making the role narrowable is what makes the 403 path
reachable. It will arrive from a second direction too: the
spec.sourceClusterwork on #220 reaches aremote cluster through a user-supplied kubeconfig, and nothing guarantees that identity may list
apiservices.3.
feat(chart)!values.schema.jsonThe five
failguards protectingrbac.watchTypeswere a hand-rolled type system. Helm has one, itis enforced on
template,lint,installandupgrade, and it travels inside the packagedchart, so it protects a real
helm installand not just a render from this repo.Objects the chart owns are closed (
additionalProperties: false). Kubernetes pass-through objects(
securityContext,affinity,resources,volumes,tolerations) andglobalstay open. Errorsname the offending path:
Where the binary already had a contract, the schema states it:
logging.level(warnis not a zaplevel; the chart renders
--zap-log-level=warntoday and the pod crash-loops on it),queue.redis.keyPrefix(the character classValidateKeyPrefixenforces),servers.admission.timeoutSeconds1 to 30,auditService.nodePort30000 to 32767, Go durations.Verified against every real invocation: chart defaults,
helm lint, the e2ehelm upgrade --installflags,
task dist-install, quickstart, attribution, andrbacselected mode.4. Three defects found on the way
fix(watch): a watch closed by shutdown is not a watch error. Teardown closes the resultchannel and cancels the context, so both arms of the streaming
selectare ready, and Go picksamong ready cases at random. Roughly half of all clean shutdowns returned
target watch result channel closedinstead ofnil. Reproduces onmainunder-race. The regression test repeats200 times, because a coin flip proves nothing once.
servers.enableHTTP2was dead. No template read it. Setting it changed nothing, while itscomment promised an HTTP/2 Rapid-Reset mitigation. Now wired to
--enable-http2, rendered only whentrue, so a default install is byte-identical.
env,volumes,volumeMountswere undeclared. Read by the Deployment, absent fromvalues.yaml. A closed schema would have silently broken them. Found by diffing used-vs-declaredbefore writing the schema.
Also:
--redis-addr's help text claimed an empty value was incompatible with--admission-webhook,which the code contradicts (the webhook runs Redis-free, no-op'ing command-author capture), and
watchCursorKeybuilt keys from a raw prefix while every other key family resolved it.5.
docsstop overclaiming, prune the stale planssecurity-model.mdstill said the Secret narrowing was "not yet an RBAC change". It is rewrittenaround the two ClusterRoles, with two mermaid diagrams (parsed with the real mermaid parser, not
eyeballed). Four sites claimed "zero cluster-wide Secret access", which is false and which the tests
already contradicted. They now say what the tests say.
future/secret-value-retention-plan.md(implemented),future/scoped-rbac-least-privilege-plan.mdand
future/secret-handling-roadmap.md(both still opening with "Track B: not started", describing awildcard-in-manager-role that no longer exists) are removed. That is 685 lines deleted, 227 added,
replaced by two short docs: the age-recipient metadata idea, and the three genuinely open
least-privilege items. Every inbound link is repointed.
Validation
task lint,task test(coverage 75.5%), andtask test-e2e(55 passed, 0 failed) pass locally, plustask test-e2e-quickstart-helmspecifically, because ahelm templateunit test is not the same ashelm upgrade --installaccepting the values through the new schema. The manager comes up1/1 Running, 0 restarts, with--redis-key-prefix=gitops-reverserand no--enable-http2.Every fix has a test verified to fail without the fix, including the chart-render assertions (by
folding the wildcard back into the manager role) and the schema (by reintroducing each bad value).
🤖 Generated with Claude Code
BREAKING CHANGE: The manager ClusterRole no longer contains
apiGroups: ["*"], resources: ["*"]. Thetypes a WatchRule may read now come from a separate ClusterRole rendered from the new
rbac.watchTypes(Helm) orconfig/rbac/watch-any-role.yaml(kustomize). The defaultrbac.watchTypes.mode: anyreproduces the old wildcard exactly, so a default install keeps the sameeffective permissions and needs no action. Anyone who bound their own ServiceAccount to
<release>-manager-rolealone, relying on the wildcard it used to carry, must also bind<release>-watch-anyor switch tomode: selected.BREAKING CHANGE: The manager ClusterRole's
secretsrule narrowed fromget,list,watch,create,update,patchtoget,create,update. The operator has held no Secret informersince Secret values stopped being retained in the control plane, and never used
list,watchorpatch. A hand-written role set that assumed the broad grant will now find it absent.BREAKING CHANGE: The chart ships a
values.schema.jsonthat Helm enforces ontemplate,lint,installandupgrade. Values files that previously installed may now be rejected. An unknown ormisspelled key under an object the chart owns is an error rather than a silent no-op (
replicaCounts,servers.metric, a leftoverrbac.watchAnyResource), and values the binary would have rejected atstartup are now rejected at install time:
logging.level: warnis not a zap level;logging.stacktraceLevel: debug;logging.encoder: text; aqueue.redis.keyPrefixoutside[A-Za-z0-9._:-];servers.admission.timeoutSecondsoutside 1 to 30;auditService.nodePortoutside30000 to 32767; a malformed Go duration. Fix the value, or bypass with
--skip-schema-validation.BREAKING CHANGE:
servers.enableHTTP2now reaches the binary. It was a dead value, read by notemplate, so setting it to
truepreviously did nothing. An install that set it expecting a no-op willnow actually serve HTTP/2 on the TLS servers, losing the HTTP/2 Rapid-Reset mitigation its own comment
promised. The default (
false) is unchanged and renders no flag.