Chore: Refactor CLI common functions - #722
Conversation
Signed-off-by: Ed Snible <snible@us.ibm.com>
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesShared runtime integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CommandMain
participant RuntimeUtil
participant HTTPServer
participant ReverseProxyServer
CommandMain->>RuntimeUtil: StartHTTPServer
RuntimeUtil->>HTTPServer: Bind listener and serve
CommandMain->>RuntimeUtil: StartReverseProxyServer
RuntimeUtil->>ReverseProxyServer: Listen with TLS and mTLS handling
ReverseProxyServer-->>CommandMain: Return server handle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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 (2)
authbridge/authlib/runtime/runtime.go (2)
39-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a unit test for the LOG_LEVEL switch in
InitLogging.The level-mapping logic (debug/warn/error/default) is pure and easy to test in isolation, without needing to exercise the goroutines or network binding elsewhere in this file.
🤖 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 `@authbridge/authlib/runtime/runtime.go` around lines 39 - 51, Add unit coverage for the LOG_LEVEL mapping in InitLogging, testing debug, warn, error, and an unset or unknown value mapping to the expected slog levels. Isolate the switch logic from unrelated runtime behavior and restore any environment or global logging state between cases.
72-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd read/write/idle timeouts to the server helpers to bound slow-client connections.
StartHealthServer(Line 93) callshttp.ListenAndServedirectly with no*http.Serverand no timeouts at all.StartHTTPServer(Lines 117-121) andStartReverseProxyServer(Lines 144-148) setReadHeaderTimeoutbut notReadTimeout,WriteTimeout, orIdleTimeout. A slow or malicious client can hold a connection open past the header phase and exhaust server resources.🔧 Proposed fix to bound connection lifetimes
func StartHealthServer(inboundH, outboundH *pipeline.Holder) { go func() { mux := http.NewServeMux() mux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }) mux.HandleFunc("/readyz", func(w http.ResponseWriter, r *http.Request) { if name := inboundH.NotReadyPlugin(); name != "" { http.Error(w, "inbound plugin not ready: "+name, http.StatusServiceUnavailable) return } if name := outboundH.NotReadyPlugin(); name != "" { http.Error(w, "outbound plugin not ready: "+name, http.StatusServiceUnavailable) return } w.WriteHeader(http.StatusOK) }) + srv := &http.Server{ + Addr: ":9091", + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 15 * time.Second, + WriteTimeout: 15 * time.Second, + IdleTimeout: 60 * time.Second, + } slog.Info("health server listening", "addr", ":9091") - if err := http.ListenAndServe(":9091", mux); err != nil { + if err := srv.ListenAndServe(); err != nil { slog.Warn("health server failed", "error", err) } }() }Apply the same
ReadTimeout/WriteTimeout/IdleTimeoutfields to thehttp.Serverliterals inStartHTTPServerandStartReverseProxyServer.Also applies to: 116-133, 143-160
🤖 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 `@authbridge/authlib/runtime/runtime.go` around lines 72 - 97, Update StartHealthServer, StartHTTPServer, and StartReverseProxyServer to use http.Server configurations with bounded ReadTimeout, WriteTimeout, and IdleTimeout values, retaining the existing ReadHeaderTimeout settings. Replace the direct health-server ListenAndServe call with the configured server’s method while preserving current handlers and logging.
🤖 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 `@authbridge/authlib/runtime/runtime.go`:
- Around line 37-51: Update the InitLogging function signature in
authbridge/authlib/runtime/runtime.go (lines 37-51) to accept a binaryName
string parameter and attach it to the default logger using With("binary",
binaryName) on the slog.New call. Simultaneously update all three call sites:
authbridge/cmd/authbridge-cpex/main.go (lines 63-68) to call
runtime.InitLogging("authbridge-cpex"), authbridge/cmd/authbridge-envoy/main.go
(lines 61-66) to call runtime.InitLogging("authbridge-envoy"), and
authbridge/cmd/authbridge-proxy/main.go (lines 136-137) to call
runtime.InitLogging("authbridge-proxy"), ensuring each binary passes its own
name to satisfy the logging guideline.
---
Nitpick comments:
In `@authbridge/authlib/runtime/runtime.go`:
- Around line 39-51: Add unit coverage for the LOG_LEVEL mapping in InitLogging,
testing debug, warn, error, and an unset or unknown value mapping to the
expected slog levels. Isolate the switch logic from unrelated runtime behavior
and restore any environment or global logging state between cases.
- Around line 72-97: Update StartHealthServer, StartHTTPServer, and
StartReverseProxyServer to use http.Server configurations with bounded
ReadTimeout, WriteTimeout, and IdleTimeout values, retaining the existing
ReadHeaderTimeout settings. Replace the direct health-server ListenAndServe call
with the configured server’s method while preserving current handlers and
logging.
🪄 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: 399053dd-ab5a-48d6-9fd2-ddef4b62aeaa
📒 Files selected for processing (4)
authbridge/authlib/runtime/runtime.goauthbridge/cmd/authbridge-cpex/main.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-proxy/main.go
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Clean mechanical extraction — I diffed each of the six moved helpers against its original and they are byte-identical, so there is no behavioral drift here. Net -120 lines across three main.go files is a good trade.
All comments below are about the new shared package's design, not the move itself. Nothing blocking.
Note on CI: the red CodeQL Analysis (python) mark is a cancelled job (this PR touches only Go), not a real failure. Everything else is green.
Areas reviewed: Go (authlib + cmd), commit/PR conventions, security, tests
Commits: 1, signed-off ✓
Assisted-By: Claude Code
| "time" | ||
|
|
||
| "github.com/rossoctl/cortex/authbridge/authlib/config" | ||
| "github.com/rossoctl/cortex/authbridge/authlib/listener/reverseproxy" |
There was a problem hiding this comment.
suggestion — The package doc (line 8) says this package is "intentionally listener-agnostic", but this import is the reverse-proxy listener. The claim and the code disagree.
The practical cost lands on authbridge-envoy: it uses only InitLogging, StartSignalToggle, StartStatServer, LogLevel, and StartHealthServer — none of the reverse-proxy helpers — and it does not import reverseproxy itself. By importing runtime it now transitively links listener/reverseproxy, listener/httpx, listener/internal/tlssniff, listener/internal/sseframe, and authlib/tls for nothing. That matters a bit more than usual given the authbridge-lite trimming comment in authbridge-proxy/main.go.
Two ways out. Either drop the "listener-agnostic" sentence and accept the coupling, or keep the claim true with a small local interface:
// Listenable is the subset of a listener server that StartReverseProxyServer
// needs, so this package stays free of concrete listener imports.
type Listenable interface {
Handler() http.Handler
Listen(addr string) (net.Listener, error)
MTLSEnabled() bool
}*reverseproxy.Server satisfies that structurally, so both call sites pass rpSrv unchanged and the import goes away.
| } | ||
| listener, err := net.Listen("tcp", addr) | ||
| if err != nil { | ||
| log.Fatalf("%s listen: %v", name, err) |
There was a problem hiding this comment.
suggestion — log.Fatalf was fine when this lived in package main; it reads differently now that it is in authlib. Three consequences:
- A bind failure
os.Exit(1)s whatever process imports the package, with no chance for the caller to fall back or clean up.authlibotherwise behaves like a library. - It makes these helpers effectively untestable — a test that exercises the bind-failure path takes the test binary down with it. Plausibly part of why there is no
runtime_test.goyet. log.Fatalfgoes through the stdliblogpackage, bypassing the slog handlerInitLoggingjust installed. The fatal line lands outside the structured log stream operators are scraping.
Since the signature is already changing in this PR, this is the cheap moment to return (*http.Server, error) from StartHTTPServer/StartReverseProxyServer and let each main keep its own log.Fatalf. Same applies to StartStatServer at line 107. Fine to defer to a follow-up if you would rather keep this PR purely mechanical.
(Minor, same region: err != http.ErrServerClosed would be more robust as errors.Is(err, http.ErrServerClosed) now that it appears three times in one file.)
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
| slog.Info("health server listening", "addr", ":9091") | ||
| if err := http.ListenAndServe(":9091", mux); err != nil { |
There was a problem hiding this comment.
suggestion — :9091 is hardcoded here and in the log line above, while the sibling StartStatServer reads cfg.Stats.StatsAddress (config.go:489, defaulting to :9093 at config.go:523). The asymmetry was invisible when this was an inline goroutine in each main; it is more conspicuous as a shared helper, and it means the helper cannot be bound to an ephemeral port in a test or run twice in one process.
Taking addr string and having the three call sites pass ":9091" keeps behavior identical and matches the style of the other two Start* helpers. A Health.Address config field with a :9091 default would be the fuller fix, but that is arguably out of scope for a chore.
|
|
||
| // InitLogging sets the process log level from the LOG_LEVEL env var (debug / | ||
| // warn / error, default info) and installs a slog text handler on stderr. | ||
| func InitLogging() { |
There was a problem hiding this comment.
suggestion — No runtime_test.go. Every other package under authbridge/authlib/ has at least one (pipeline 14, spiffe 5, tlsbridge 5, and ≥1 for the remaining twelve), so this is the one gap in an otherwise consistent convention — and it is newly-shared code, where a regression now hits all three binaries at once.
The two cheapest wins need no refactor:
InitLogging: table test overLOG_LEVELvalues (debug/warn/error/unset/garbage/mixed-case) assertingLogLevel().StartSignalToggle: raiseSIGUSR1and assert the DEBUG↔INFO flip, including the case where the starting level iswarn(today the first signal jumps straight to DEBUG — worth pinning that down as intended).
StartHTTPServer becomes testable against ":0" once the log.Fatalf point above is addressed.
| // This package is intentionally listener-agnostic: it imports no gRPC and no | ||
| // cgo, so authlib does not gain the envoy (go-control-plane / grpc) or cpex | ||
| // (libcpex_ffi) dependency by hosting it. | ||
| package runtime |
There was a problem hiding this comment.
nit — runtime shadows the stdlib package of the same name. Nothing breaks today (I checked: none of the three main.go files use stdlib runtime), but any future runtime.NumCPU(), runtime.GC(), or pprof-adjacent code in these files will need an import alias, and the ambiguity costs a beat on every read of runtime.StartHealthServer.
Renaming a brand-new package is nearly free right now and gets expensive once other repos import it. Something like procruntime, bootstrap, or serve would keep the intent.
Signed-off-by: Ed Snible <snible@us.ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
authbridge/authlib/runtimeutil/runtimeutil.go (1)
70-95: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReturn health-listener bind errors to the caller.
StartHealthServerstarts the goroutine beforehttp.ListenAndServebindsaddr. If the address is unavailable, the command continues without health endpoints, and the current log message appears before the bind succeeds. Bind withnet.Listenbefore starting the goroutine, and return the bind error so callers can fail startup consistently withStartStatServer.🤖 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 `@authbridge/authlib/runtimeutil/runtimeutil.go` around lines 70 - 95, Update StartHealthServer to bind addr with net.Listen before launching the serving goroutine, returning the listener’s error to the caller when binding fails. Pass the successfully created listener to the HTTP server inside the goroutine and log that the server is listening only after binding succeeds, matching StartStatServer’s startup 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.
Inline comments:
In `@authbridge/authlib/runtimeutil/runtimeutil.go`:
- Around line 121-125: Set a nonzero ReadTimeout alongside ReadHeaderTimeout in
both StartHTTPServer and StartReverseProxyServer, using the same bounded
body-read timeout for each listener while preserving their existing server
configuration.
---
Outside diff comments:
In `@authbridge/authlib/runtimeutil/runtimeutil.go`:
- Around line 70-95: Update StartHealthServer to bind addr with net.Listen
before launching the serving goroutine, returning the listener’s error to the
caller when binding fails. Pass the successfully created listener to the HTTP
server inside the goroutine and log that the server is listening only after
binding succeeds, matching StartStatServer’s startup behavior.
🪄 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: 28623217-c12b-401f-a073-408dd9243dd8
📒 Files selected for processing (6)
authbridge/authlib/observe/statserver.goauthbridge/authlib/runtimeutil/runtimeutil.goauthbridge/authlib/runtimeutil/runtimeutil_test.goauthbridge/cmd/authbridge-cpex/main.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-proxy/main.go
🚧 Files skipped from review as they are similar to previous changes (3)
- authbridge/cmd/authbridge-envoy/main.go
- authbridge/cmd/authbridge-proxy/main.go
- authbridge/cmd/authbridge-cpex/main.go
| func StartHTTPServer(name string, handler http.Handler, addr string) (*http.Server, error) { | ||
| srv := &http.Server{ | ||
| Addr: addr, | ||
| Handler: handler, | ||
| ReadHeaderTimeout: 10 * time.Second, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file existence and line count =="
wc -l authbridge/authlib/runtimeutil/runtimeutil.go
echo "== relevant source =="
cat -n authbridge/authlib/runtimeutil/runtimeutil.go | sed -n '1,220p'
echo "== searches for runtimeutil exports/usages =="
rg -n "StartHTTPServer|StartReverseProxyServer|ReadHeaderTimeout|ReadTimeout|WriteTimeout" authbridge -S
echo "== reverse proxy server source references in authbridge =="
fd -a 'server\.go$|proxy' authbridge/authlib authbridge | sed 's#^\./##' | head -200
rg -n "type Server|func \(.*\) Handler|ReadHeaderTimeout|ReadTimeout|WriteTimeout|body" authbridge/authlib/authbridge -S | head -300Repository: rossoctl/cortex
Length of output: 11201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reverseproxy source =="
cat -n authbridge/authlib/listener/reverseproxy/server.go | sed -n '1,260p'
echo "== forwardproxy source snippets =="
cat -n authbridge/authlib/listener/forwardproxy/server.go | sed -n '1,260p'
echo "== config definitions for listener section =="
rg -n "type Listener|ReverseProxyAddr|ForwardProxyAddr|MaxUpload|Upload|Buffer|size" authbridge/authlib/authlib authbridge/authlib -S | head -200
for f in $(fd -a 'config.*\.go$' authbridge/authlib | head -20); do
echo "--- $f"
cat -n "$f" | sed -n '1,220p'
done
echo "== request body read uses in listener packages =="
rg -n "\.Body|\.Request|ReadAll|io\.Copy|MaxMemory|WriteTimeout|ReadTimeout" authbridge/authlib/listener authbridge/authlib -S | head -300
echo "== static verifier: http.Server defaults and timeout fields in runtimeutil helper constructors =="
python3 - <<'PY'
from pathlib import Path
p = Path("authbridge/authlib/runtimeutil/runtimeutil.go")
text = p.read_text()
for start, fn in [
("StartHTTPServer", "func StartHTTPServer"),
("StartReverseProxyServer", "func StartReverseProxyServer"),
]:
idx = text.index(fn)
# until the next top-level func
nxt = text.find("\nfunc ", idx + 1)
if nxt == -1:
nxt = len(text)
body = text[idx:nxt]
print(f"== {start} ==")
print("has ReadHeaderTimeout:", "ReadHeaderTimeout:" in body)
print("has ReadTimeout:", "ReadTimeout:" in body)
print("has WriteTimeout:", "WriteTimeout:" in body)
PY
echo "== Go http.Server documentation in local tool if available =="
if command -v go >/dev/null 2>&1; then
go doc net/http.Server 2>/dev/null | sed -n '1,120p' || true
else
echo "go not available"
fiRepository: rossoctl/cortex
Length of output: 37495
Add bounded request timeouts to both listener helpers.
ReadHeaderTimeout only covers headers. Clients can still stall body reads for the lifetime of the connection because ReadTimeout is unset. Set a nonzero body timeout on both StartHTTPServer and StartReverseProxyServer; this helper is used by both proxy listeners.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 121-125: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: addr,
Handler: handler,
ReadHeaderTimeout: 10 * time.Second,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
📍 Affects 1 file
authbridge/authlib/runtimeutil/runtimeutil.go#L121-L125(this comment)authbridge/authlib/runtimeutil/runtimeutil.go#L148-L152
🤖 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 `@authbridge/authlib/runtimeutil/runtimeutil.go` around lines 121 - 125, Set a
nonzero ReadTimeout alongside ReadHeaderTimeout in both StartHTTPServer and
StartReverseProxyServer, using the same bounded body-read timeout for each
listener while preserving their existing server configuration.
Source: Linters/SAST tools
Signed-off-by: Ed Snible <snible@us.ibm.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
Created using
with Opus 4.8 and
Summary by CodeRabbit
New Features
LOG_LEVEL.Bug Fixes
Tests