Conversation
* feat(rbac): add zero-allocation RBAC core engine Implements Phase 1 of the RBAC design (issue #16): command ID catalog with bulk categories, dynamic zero-allocation Bitset, role whitelist rules, rule parser, SessionContext hot-path check, and an atomically hot-swappable PolicyStore. IsAllowed benchmarks at 0 allocs/op, ~2.8 ns/op. Signed-off-by: Maximilian Hagen <git@saxy.dev> fix(pr): resolve comments chore(rbac): added test to ensure cmdList is always up2date Signed-off-by: Maximilian Hagen <git@saxy.dev> --------- Signed-off-by: Maximilian Hagen <git@saxy.dev>
* feat(rbac): wire RBAC into RESP and binary servers Add the ROLE command family and per-user bcrypt auth over both protocols, replacing --require-pass when an RBAC policy is loaded: - internal/rbac: Role/User policy store, rule parser (+cmd, +@cat, ~prefix), atomic hot-swap holder, and zero-alloc session gating - RESP: AUTH <user> <pass>, ROLE CREATE/SETUSER/DELUSER/DELETE/LIST/GETUSER, -NOAUTH / -NOPERM replies; nopass default-user semantics - binary: OpRoleCreate/SetUser/DelUser/Delete/List/GetUser wire codecs, MsgAuth username+password frame, ERR NOT_AUTHORIZED gating on both plaintext and TLS loops - server: --rbac-config / TELLSTONE_RBAC_CONFIG, SIGHUP hot-reload - client: AuthUser + Role* methods in the network and public client packages - example: cmd/example/role drives the full flow over the binary protocol Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded opt-in RBAC with YAML/JSON policies, password authentication, role and namespace permissions, binary and RESP management commands, atomic policy reloads, metrics, client APIs, logging support, tests, and documentation. ChangesRBAC implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
internal/rbac/bench_test.go (1)
11-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth benchmarks measure their own setup allocations.
ParseRoleandNewSessionContextallocate before the loop, and the timer and allocation counter are already running. Each benchmark claims to proveIsAllowedreports 0 allocs/op, so the setup cost must be excluded.
internal/rbac/bench_test.go#L11-L24: addb.ResetTimer()afterb.ReportAllocs()inBenchmarkIsAllowedAllowed.internal/rbac/bench_test.go#L26-L39: addb.ResetTimer()afterb.ReportAllocs()inBenchmarkIsAllowedDeniedByPrefix.🤖 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 `@internal/rbac/bench_test.go` around lines 11 - 24, Exclude benchmark setup from timing and allocation measurements by adding b.ResetTimer() immediately after b.ReportAllocs() in BenchmarkIsAllowedAllowed (internal/rbac/bench_test.go:11-24) and BenchmarkIsAllowedDeniedByPrefix (internal/rbac/bench_test.go:26-39).internal/rbac/rbac.go (1)
31-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a value receiver for the read-only
Has.
Hasdoes not modify the bitset, but the pointer receiver forces callers to hold an addressableBitset. A value receiver reads the same slice header without a copy of the backing array and works on any expression, including map values and function results.Bitsetis exported, so this affects external callers too.♻️ Proposed receiver change
-func (b *Bitset) Has(id uint16) bool { +func (b Bitset) Has(id uint16) bool { word := id / 64 - if int(word) >= len(*b) { + if int(word) >= len(b) { return false } - return (*b)[word]&(uint64(1)<<(id%64)) != 0 + return b[word]&(uint64(1)<<(id%64)) != 0 }
Clearcan also take a value receiver, because it never grows the slice. Keep the pointer receiver onSet.🤖 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 `@internal/rbac/rbac.go` around lines 31 - 46, Change the receiver of the read-only Bitset.Has method to a value receiver so it can be called on non-addressable expressions while preserving the same backing-array behavior. Also change Bitset.Clear to a value receiver because it only mutates existing slice contents; keep Bitset.Set’s pointer receiver unchanged.internal/network/role.go (1)
103-116: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
DecodeRoleGetUserResponseaccepts trailing bytes.
DecodeRoleArgsandDecodeRoleListResponseboth requirepos == len(payload). This decoder only checks the minimum length, so a payload with extra bytes after thehaspassbyte decodes as valid. Align the strictness to reject unexpected trailing data.♻️ Proposed strict length check
n := int(binary.BigEndian.Uint16(payload[:2])) - if 2+n+1 > len(payload) { + if 2+n+1 != len(payload) { return RoleUser{}, false }🤖 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 `@internal/network/role.go` around lines 103 - 116, Update DecodeRoleGetUserResponse to require the decoded response to consume the entire payload, rejecting any payload where 2+n+1 does not equal len(payload). Preserve the existing minimum-length and field decoding behavior while rejecting trailing bytes.config/config.go (1)
284-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider warning when both
--require-passand--rbac-configare set.
network.NewServerandresp.NewServerskiprequirePasswhen a policy store exists, so--require-passis silently ignored in RBAC mode. The flag help text states the supersede rule, but the runtime gives no signal. A startup log warning would make the effective authentication mode explicit.🤖 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 `@config/config.go` around lines 284 - 292, Update startup configuration handling around the rbacConfig and require-pass options to emit a warning when both are set, clearly stating that RBAC authentication supersedes require-pass. Ensure the warning is logged once during startup while preserving the existing authentication behavior and flag semantics.internal/network/role_test.go (1)
183-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
golangci-lintreportsnilerrerrors in this handler.The linter flags lines 184, 197, 200, and 216: the code returns a
nilerror after a non-nil error. The pattern is intentional here, because the handler encodes the failure into aMsgErrorframe. Ifnilerrruns as an error in CI, this file fails the lint stage. Add a targeted//nolint:nilerrcomment with a short reason on each return, or excludenilerrfor_test.gofiles in thegolangci-lintconfiguration.🤖 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 `@internal/network/role_test.go` around lines 183 - 216, Add targeted nolint:nilerr annotations with brief reasons to the intentional error-encoded returns in the handler cases for OpRoleCreate, OpRoleSetUser, OpRoleDelUser, and OpRoleDelete. Keep the existing MsgError responses and nil error values unchanged; do not broadly disable nilerr for test files.Source: Linters/SAST tools
internal/rbac/metrics.go (1)
46-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePer-role counters restart at zero after a policy reload.
RoleCommandCountsreads counters from the currently loaded policy. A SIGHUP reload or aROLE CREATEmutation builds newRolevalues, sotellstone_rbac_commands_totaldrops back to zero for every role. Prometheus treats that as a counter reset, andincrease()over the reload window loses the pre-reload commands. Consider carrying the previous count forward when a role of the same name is rebuilt, or document the reset behavior next to the metric.🤖 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 `@internal/rbac/metrics.go` around lines 46 - 57, The per-role command metric resets when policy reloads rebuild Role values. Update the policy replacement and ROLE CREATE paths to preserve each existing role’s Commands count for roles with matching names, ensuring RoleCommandCounts continues from the prior value; alternatively, explicitly document the reset semantics alongside the metric if continuity cannot be supported.
🤖 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 `@internal/metrics/metrics.go`:
- Around line 213-215: Update internal/metrics/metrics.go lines 213-215 so
tellstone_rbac_commands_total emits its HELP and TYPE metadata once before the
per-role loop, while the loop writes only labeled samples. Update
internal/metrics/metrics_test.go lines 67-82 to assert that the TYPE declaration
occurs exactly once, retaining the existing substring checks.
- Around line 203-206: Update the metrics startup path around
NewAggregateCollector in server/server.go to guard s.policy before passing it as
the RBAC metrics interface. When RBAC is disabled and s.policy is nil, pass a
genuinely nil interface or omit the collector so RoleCommandCounts cannot
execute against a nil store; preserve the existing collector behavior when a
policy is configured.
In `@internal/network/server.go`:
- Around line 537-540: Update the unknown-user branch in the authentication flow
around UserFor so it performs the same bcrypt work as existing-user failures:
dispatch the authentication job using the established dummyAuthHash, allowing
the worker’s comparison to fail normally instead of returning immediately.
Preserve the existing auth failure response while equalizing timing for unknown
and known usernames.
In `@internal/rbac/manager_test.go`:
- Around line 44-63: Update TestStoreDelUserAndDeleteRole to explicitly verify
alice’s fallback after DeleteRole("r") using PolicyStore.Default; preserve the
current nil default and assert the resulting denied or nil resolution unless a
permissive fallback is intentionally required. Keep the existing role-removal
and DelUser assertions unchanged.
In `@internal/rbac/manager.go`:
- Around line 76-93: Update DeleteRole to clear the cloned policy’s Default
pointer when it refers to the role identified by name before removing that role
from p.Roles. Preserve the existing deletion and error behavior, allowing
RoleFor to fail closed when no default role remains.
- Around line 45-60: Update SetUser so the role-existence validation runs after
both the existing-policy and empty-store branches initialize p. Reject unknown
roleName values uniformly before assigning the user and storing the policy,
while preserving cloning and fresh-store initialization behavior.
In `@internal/rbac/policy.go`:
- Around line 83-97: Serialize policy reload publication in reloadRBAC with
Store.mu, ensuring the fresh snapshot is based on the current active policy
while holding the same lock used by CreateRole, SetUser, DelUser, and
DeleteRole. Do not call the locking Store mutation methods while holding the
mutex; publish the completed reload atomically under the lock so SIGHUP cannot
overwrite an in-flight ROLE update.
In `@README.md`:
- Around line 177-179: Update the README supported-command statement to include
COMMAND and INFO alongside the existing commands, matching the
registered-command documentation and preserving the existing note about
unknown-command behavior.
In `@server/server.go`:
- Around line 376-391: Update the request handling branches for network.OpSet,
empty keys, storage failures, and the invalid-opcode case so client-visible
response frames are returned with a nil handler error, allowing both traffic
paths to write them to the client. Preserve non-nil errors only for failures
that should close the connection, and change the invalid-opcode response to
network.ResponseInvalidOpCode.
---
Nitpick comments:
In `@config/config.go`:
- Around line 284-292: Update startup configuration handling around the
rbacConfig and require-pass options to emit a warning when both are set, clearly
stating that RBAC authentication supersedes require-pass. Ensure the warning is
logged once during startup while preserving the existing authentication behavior
and flag semantics.
In `@internal/network/role_test.go`:
- Around line 183-216: Add targeted nolint:nilerr annotations with brief reasons
to the intentional error-encoded returns in the handler cases for OpRoleCreate,
OpRoleSetUser, OpRoleDelUser, and OpRoleDelete. Keep the existing MsgError
responses and nil error values unchanged; do not broadly disable nilerr for test
files.
In `@internal/network/role.go`:
- Around line 103-116: Update DecodeRoleGetUserResponse to require the decoded
response to consume the entire payload, rejecting any payload where 2+n+1 does
not equal len(payload). Preserve the existing minimum-length and field decoding
behavior while rejecting trailing bytes.
In `@internal/rbac/bench_test.go`:
- Around line 11-24: Exclude benchmark setup from timing and allocation
measurements by adding b.ResetTimer() immediately after b.ReportAllocs() in
BenchmarkIsAllowedAllowed (internal/rbac/bench_test.go:11-24) and
BenchmarkIsAllowedDeniedByPrefix (internal/rbac/bench_test.go:26-39).
In `@internal/rbac/metrics.go`:
- Around line 46-57: The per-role command metric resets when policy reloads
rebuild Role values. Update the policy replacement and ROLE CREATE paths to
preserve each existing role’s Commands count for roles with matching names,
ensuring RoleCommandCounts continues from the prior value; alternatively,
explicitly document the reset semantics alongside the metric if continuity
cannot be supported.
In `@internal/rbac/rbac.go`:
- Around line 31-46: Change the receiver of the read-only Bitset.Has method to a
value receiver so it can be called on non-addressable expressions while
preserving the same backing-array behavior. Also change Bitset.Clear to a value
receiver because it only mutates existing slice contents; keep Bitset.Set’s
pointer receiver unchanged.
🪄 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: c18a1f23-bd18-47ff-b822-babf6b97ec44
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (47)
ARCHITECTURE.mdREADME.mdROADMAP.mdclient/client_role.gocmd/example/role/main.goconfig/config.goconfig/config_test.gogo.modinternal/metrics/metrics.gointernal/metrics/metrics_test.gointernal/network/README.mdinternal/network/benchmark_tls_test.gointernal/network/client.gointernal/network/client_role.gointernal/network/protocol.gointernal/network/protocol_test.gointernal/network/role.gointernal/network/role_test.gointernal/network/server.gointernal/network/server_test.gointernal/rbac/bench_test.gointernal/rbac/cmd.gointernal/rbac/cmd_test.gointernal/rbac/config.gointernal/rbac/config_test.gointernal/rbac/manager.gointernal/rbac/manager_test.gointernal/rbac/metrics.gointernal/rbac/parser.gointernal/rbac/parser_test.gointernal/rbac/policy.gointernal/rbac/policy_test.gointernal/rbac/rbac.gointernal/rbac/rbac_test.gointernal/rbac/role.gointernal/rbac/role_test.gointernal/rbac/session.gointernal/rbac/session_test.gointernal/rbac/user.gointernal/rbac/user_test.gointernal/resp/protocol.gointernal/resp/role.gointernal/resp/role_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/shard/runner.goserver/server.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
101-103: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up shards and persistence state after RBAC initialization fails.
Runstarts shard goroutines ininitShards, then returns directly wheninitRBACfails. Add startup cleanup for that path: stop each shard started byinitShardsand, if persistence was enabled, close the opened WAL files viaStore.CloseShardor via the shard cleanup path.🤖 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 `@server/server.go` around lines 101 - 103, Update Run’s initRBAC failure path to clean up startup resources before returning: stop every shard created by initShards and close persistence WALs when enabled, using Store.CloseShard or the existing shard cleanup path. Preserve the current wrapped RBAC error while ensuring cleanup also occurs if initialization fails.
🤖 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 `@ideas.md`:
- Around line 1-2: Update the beginning of ideas.md by removing the leading
blank line and changing the “Metrics (verified against source)” heading from H3
to H1, making it the file’s first content.
- Line 12: Update the stale source reference in the production metrics
explanation to point to the current collector wiring symbols, preferably
startMetricsServer, NewAggregateCollector, or
AggregateCollector.WritePrometheus, and use server/server.go:325 only if a line
reference is necessary; preserve the existing statement about shard-only output.
---
Outside diff comments:
In `@server/server.go`:
- Around line 101-103: Update Run’s initRBAC failure path to clean up startup
resources before returning: stop every shard created by initShards and close
persistence WALs when enabled, using Store.CloseShard or the existing shard
cleanup path. Preserve the current wrapped RBAC error while ensuring cleanup
also occurs if initialization fails.
🪄 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: 0a4995a0-23c0-4f81-b3b9-4443e696922c
📒 Files selected for processing (14)
README.mdideas.mdinternal/app/tellstone/startup.gointernal/metrics/metrics.gointernal/metrics/metrics_test.gointernal/network/role.gointernal/network/server.gointernal/rbac/bench_test.gointernal/rbac/manager.gointernal/rbac/manager_test.gointernal/rbac/metrics.gointernal/rbac/policy.gointernal/rbac/rbac.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/rbac/metrics.go
- README.md
- internal/network/role.go
- internal/metrics/metrics.go
- internal/rbac/rbac.go
- internal/metrics/metrics_test.go
- internal/network/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
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 (1)
internal/resp/starttls_test.go (1)
117-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not treat a read timeout as a successful close.
The read deadline prevents the test from hanging, but a timeout returns
n == 0with a non-nil error. The current condition accepts that result. A server that leaves the connection open therefore passes this test. Rejectnet.Errorvalues whereTimeout()is true, or require a peer-close error.Proposed assertion fix
var b [1]byte - if n, err := conn.Read(b[:]); err == nil || n != 0 { + n, err := conn.Read(b[:]) + if err == nil || n != 0 { t.Fatalf("pipelined STARTTLS should close without a reply: n=%d err=%v data=%q", n, err, b[:n]) } + if timeoutErr, ok := err.(net.Error); ok && timeoutErr.Timeout() { + t.Fatalf("pipelined STARTTLS did not close before the read deadline: %v", err) + }🤖 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 `@internal/resp/starttls_test.go` around lines 117 - 123, Update the pipelined STARTTLS read assertion around conn.Read so a timeout is treated as a test failure rather than a successful close. Reject errors implementing net.Error with Timeout() true, while preserving acceptance only for a genuine peer-close error with n == 0.
🧹 Nitpick comments (1)
ARCHITECTURE.md (1)
155-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the RBAC metrics contract.
This section documents authorization behavior but not the observability added by this PR. Add the metric names
tellstone_rbac_auth_failures_total,tellstone_rbac_denied_commands_total, andtellstone_rbac_commands_total{role="..."}. Document therolelabel and the opt-in conditions for exposing these series through/metrics.🤖 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 `@ARCHITECTURE.md` around lines 155 - 198, Add an observability subsection to the RBAC documentation covering the metrics tellstone_rbac_auth_failures_total, tellstone_rbac_denied_commands_total, and tellstone_rbac_commands_total with its role label. Specify that these series are exposed through /metrics only when RBAC is enabled via the configured --rbac-config or TSD_RBAC_CONFIG policy, and document the opt-in conditions clearly.
🤖 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 `@internal/resp/starttls_test.go`:
- Around line 117-123: Update the pipelined STARTTLS read assertion around
conn.Read so a timeout is treated as a test failure rather than a successful
close. Reject errors implementing net.Error with Timeout() true, while
preserving acceptance only for a genuine peer-close error with n == 0.
---
Nitpick comments:
In `@ARCHITECTURE.md`:
- Around line 155-198: Add an observability subsection to the RBAC documentation
covering the metrics tellstone_rbac_auth_failures_total,
tellstone_rbac_denied_commands_total, and tellstone_rbac_commands_total with its
role label. Specify that these series are exposed through /metrics only when
RBAC is enabled via the configured --rbac-config or TSD_RBAC_CONFIG policy, and
document the opt-in conditions clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6755fd7f-36d1-4611-a6ec-deaae3ded190
📒 Files selected for processing (10)
ARCHITECTURE.mdREADME.mdROADMAP.mdconfig/config.goconfig/config_test.gointernal/resp/role_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/resp/starttls_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (8)
- ROADMAP.md
- internal/resp/server_test.go
- README.md
- internal/resp/role_test.go
- config/config.go
- config/config_test.go
- internal/resp/server.go
- server/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/network/client.go (1)
46-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse context-aware dialing instead of
net.DialTimeout.
net.DialTimeouthides thecontext.Background()choice and cannot be cancelled by the caller. Replace it with(*net.Dialer).DialContext; passingtimeout-bounded context satisfies the deadline while adding cancellation support. This change coversDial()throughDialWithLogger.🤖 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 `@internal/network/client.go` around lines 46 - 59, Update DialWithLogger to use a net.Dialer and DialContext with a timeout-bounded context instead of net.DialTimeout, preserving the existing connection setup, logging, and error propagation so callers of Dial also gain cancellation support.Source: Linters/SAST tools
🤖 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 `@internal/rbac/README.md`:
- Line 143: Update the authorization hot path description to hyphenate
“single-bit test,” leaving the rest of the sentence unchanged.
- Around line 193-206: Update the fenced package-contents code block in the
README by adding the text language tag to its opening fence, while leaving the
listed contents unchanged.
---
Nitpick comments:
In `@internal/network/client.go`:
- Around line 46-59: Update DialWithLogger to use a net.Dialer and DialContext
with a timeout-bounded context instead of net.DialTimeout, preserving the
existing connection setup, logging, and error propagation so callers of Dial
also gain cancellation support.
🪄 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: 1dd115b5-4e37-48ae-8364-c4e88dc1c0aa
📒 Files selected for processing (15)
README.mdclient/client.gocmd/example/client/main.gocmd/example/role/main.gocmd/example/role/policy.yamlcmd/example/tls/main.gocmd/tellstone/main.gointernal/log/log.gointernal/log/noop.gointernal/network/client.gointernal/rbac/README.mdlogger/log.gologger/logger.gologger/logger_test.goserver/server.go
💤 Files with no reviewable changes (1)
- internal/log/noop.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- server/server.go
1 participant
Description
Add RBAC observability to the
/metricsendpoint: failed-auth and per-role command counters for both the binary and RESP servers, completing Phase 4 of the RBAC work.Component: Metrics / RBAC
Type of Change:
Related Issue
Closes #16
Technical Deep Dive & Context
atomic.Uint64perRole(internal/rbac/metrics.go) — anAdd(1)that supersedesatomic.AddInt64and avoids the CAS-loop on the benchmarkedIsAllowedpath.SessionContext.CountCommandand the deny counters add no locks, no allocations, and nointerface{}boxing on the request path (verified 0 allocs/op below).Store.RoleCommandCounts()walks the role map under the already-shared store mutex and snapshots counter values;StoreexposesAuthFailures()/DeniedCommands()via the same atomic pattern. No new synchronization introduced.--rbac-configis absent the collector renders notellstone_rbac_*lines and the hot path is untouched.metrics.NewAggregateCollectornow takes anRBACMetricsinterface;server/server.gopassess.policy. Rendering adds three families:tellstone_rbac_auth_failures_total— failedAUTHattempts (both protocols)tellstone_rbac_denied_commands_total—-NOPERM/NOT_AUTHORIZEDdenialstellstone_rbac_commands_total{role="..."}— per-role command counts, label-escaped via a newescapeLabelValue(roles are user-defined strings)handleDecryptedFrames) and plaintext (onTrafficPlaintext) loops, and in the RESP GET/SET/DEL/ROLE dispatch. Auth failures counted inauthFailedfor the binaryAUTHhandshake and RESPAUTHcommand.Performance & Benchmarks (If Applicable)
Workload: single-thread microbenchmarks (RBAC gate + RESP dispatch), Go 1.x
-benchmemNo measurable change — the change adds a single atomic increment on already-instrumented paths. Hot path remains allocation-free.
How Has This Been Tested?
go test ./...— all packages pass.go test -race ./internal/rbac/ ./internal/resp/ ./internal/network/ ./internal/metrics/— pass.go vet ./...— clean.internal/metrics:TestAggregateCollectorRBACMetrics,TestEscapeLabelValueinternal/resp:TestRESPServer_RBACMetricsinternal/network:TestServerRBACMetrics--rbac-config+--enable-metrics):tellstone_rbac_*counters 0.SETunder areadonlyrole + wrong-passwordAUTH:auth_failures_total 1,denied_commands_total 1,commands_total{role="readonly"} 1.SET+GET:commands_total{role="admin"} 2, readonly unchanged at 1.grep -cE '^tellstone_rbac' /metricsreturns 0 (opt-in preserved).Checklist
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
New Features
Documentation
Bug Fixes
ERR.