docs: ADMIN_PORT config and security hardening#69
Conversation
…hitecture - config-reference.md: add ADMIN_PORT row to Server table - howto/production.md: add dedicated admin port section as preferred security approach, NetworkPolicy example, update public-facing checklist - architecture.md: update port binding description, built-in endpoints diagram, add tip about ADMIN_PORT - howto/Debugging.md: note pprof served on admin port when configured - Index.md: add tip linking to security hardening
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR documents an optional Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Updates ColdBrew documentation to introduce ADMIN_PORT as a production security-hardening mechanism by isolating admin endpoints onto a dedicated listener, and aligns multiple docs pages to reference this new deployment model.
Changes:
- Add
ADMIN_PORTto the configuration reference and link it to security hardening guidance. - Expand the production how-to with a recommended dedicated admin port approach and a Kubernetes NetworkPolicy example.
- Update architecture/homepage/debugging docs to mention the admin port and adjust port-binding guidance/diagrams.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Index.md | Adds a homepage tip pointing readers to ADMIN_PORT for security isolation. |
| howto/production.md | Adds recommended dedicated admin port guidance and a NetworkPolicy example; updates public-facing service hardening guidance. |
| howto/Debugging.md | Notes that pprof moves to the admin port when ADMIN_PORT is configured. |
| config-reference.md | Documents the new ADMIN_PORT environment variable in the Server config table. |
| architecture.md | Updates port binding principle, adds an ADMIN_PORT tip, and adjusts the built-in endpoints diagram. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
architecture.md (1)
177-182:⚠️ Potential issue | 🟠 MajorEndpoint mapping is inconsistent: health checks should not be implied as movable to
ADMIN_PORT.This line currently implies all listed built-ins (including
/healthcheckand/readycheck) can move toADMIN_PORT, which conflicts with the rest of this PR’s admin-endpoint definition.Suggested fix
- │ Built-in Endpoints (movable to ADMIN_PORT): │ - │ /metrics - Prometheus │ - │ /healthcheck - Liveness probe │ - │ /readycheck - Readiness probe │ - │ /debug/pprof/ - Go profiling │ - │ /swagger/ - OpenAPI docs │ + │ Built-in Endpoints: │ + │ /healthcheck - Liveness probe (HTTP port) │ + │ /readycheck - Readiness probe (HTTP port) │ + │ /metrics - Prometheus (movable to ADMIN_PORT) │ + │ /debug/pprof/ - Go profiling (movable to ADMIN_PORT)│ + │ /swagger/ - OpenAPI docs (movable to ADMIN_PORT)│🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@architecture.md` around lines 177 - 182, The documentation line grouping built-in endpoints with "movable to ADMIN_PORT" is incorrect for health probes; update the doc so only admin-safe endpoints (e.g., /metrics, /debug/pprof/, /swagger/) are marked as movable to ADMIN_PORT and explicitly state that /healthcheck and /readycheck must remain on the main application port (not ADMIN_PORT). Edit the listed block to separate or annotate the two categories (movable admin endpoints vs. mandatory main-port probes) and reference the exact endpoint names (/metrics, /debug/pprof/, /swagger/, /healthcheck, /readycheck, ADMIN_PORT) to make the intended mapping unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@howto/Debugging.md`:
- Line 18: Update the profiling command examples to make clear that the pprof
endpoint path (/debug/pprof/) may be served on the configured ADMIN_PORT instead
of the default 9091: add a short note near the profiling commands referencing
ADMIN_PORT and the DisableDebug option (DisableDebug=false enables the endpoint)
and replace or annotate any hardcoded 9091 examples to say "9091 (or ADMIN_PORT
if configured)"; reference the /debug/pprof/ endpoint and
ADMIN_PORT/DisableDebug names so reviewers can find and update the examples
consistently.
In `@howto/production.md`:
- Around line 463-466: The line for "Port 9091 (HTTP): gRPC-gateway only — safe
to expose externally" is too absolute; update the wording for Port 9091 to
soften exposure guidance by saying it may be exposed externally only with proper
access controls (e.g., network policies, reverse proxy, or path whitelisting)
and explicitly recommend whitelisting only the required API paths/endpoints
rather than opening the entire port; ensure the new text replaces the phrase
"safe to expose externally" and references Port 9091 and the gRPC-gateway so
readers know which service the guidance applies to.
---
Outside diff comments:
In `@architecture.md`:
- Around line 177-182: The documentation line grouping built-in endpoints with
"movable to ADMIN_PORT" is incorrect for health probes; update the doc so only
admin-safe endpoints (e.g., /metrics, /debug/pprof/, /swagger/) are marked as
movable to ADMIN_PORT and explicitly state that /healthcheck and /readycheck
must remain on the main application port (not ADMIN_PORT). Edit the listed block
to separate or annotate the two categories (movable admin endpoints vs.
mandatory main-port probes) and reference the exact endpoint names (/metrics,
/debug/pprof/, /swagger/, /healthcheck, /readycheck, ADMIN_PORT) to make the
intended mapping unambiguous.
🪄 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
Run ID: f6758455-4bcc-48c4-99f8-28c0de53c0ee
📒 Files selected for processing (5)
Index.mdarchitecture.mdconfig-reference.mdhowto/Debugging.mdhowto/production.md
- Architecture diagram: split health endpoints from admin endpoints, only pprof/metrics/swagger move to ADMIN_PORT - Architecture tip: clarify health/readiness stay on :9091 - Production howto: soften "safe to expose" to "expose with path allowlisting", clarify health endpoints stay on HTTP port - NetworkPolicy: use kubernetes.io/metadata.name label (standard) - Debugging howto: add ADMIN_PORT variant to pprof example commands - Metrics scraping note: mention ADMIN_PORT
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
howto/production.md (1)
486-490:⚠️ Potential issue | 🟠 MajorAvoid absolute “no app-level auth needed” wording.
“Network isolation handles it” is too strong for a security guide. Please soften this to “can reduce need for app-level auth” and keep defense-in-depth framing.
Suggested wording tweak
This approach is better than disabling endpoints entirely because: - Prometheus can still scrape `/metrics` on the admin port - Operations can still access pprof for production debugging -- No application-level auth needed — network isolation handles it +- Reduced application-level auth surface for admin endpoints — enforce isolation with NetworkPolicy and other defense-in-depth controls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@howto/production.md` around lines 486 - 490, Update the phrasing that currently states "No application-level auth needed — network isolation handles it" to a softened security guidance: replace it with something like "Can reduce the need for application-level auth — network isolation can help, but maintain defense-in-depth and consider app-level controls where appropriate" so the doc now says network isolation can reduce the need for app auth but does not absolve application-level protections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@howto/production.md`:
- Around line 467-484: The NetworkPolicy named restrict-admin-port currently
selects pods (podSelector.matchLabels.app: my-service) and only allows ingress
from the monitoring namespace to port 9092, which will deny other app traffic;
update the doc to either (a) add a clear note above the YAML stating that this
policy will block all non-9092 ingress unless companion rules are present and
list example allowed app ports (e.g., 9090/9091), or (b) modify the example
NetworkPolicy spec to include additional ingress rules that explicitly allow
normal app ports (9090/9091) from the appropriate sources while keeping the
monitoring-only 9092 rule; reference the NetworkPolicy resource (kind:
NetworkPolicy, metadata.name: restrict-admin-port) in the text so readers know
which YAML needs companion rules.
---
Duplicate comments:
In `@howto/production.md`:
- Around line 486-490: Update the phrasing that currently states "No
application-level auth needed — network isolation handles it" to a softened
security guidance: replace it with something like "Can reduce the need for
application-level auth — network isolation can help, but maintain
defense-in-depth and consider app-level controls where appropriate" so the doc
now says network isolation can reduce the need for app auth but does not absolve
application-level protections.
🪄 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
Run ID: 5ccf1b68-8511-4b66-8126-e1b16f08990f
📒 Files selected for processing (3)
architecture.mdhowto/Debugging.mdhowto/production.md
🚧 Files skipped from review as they are similar to previous changes (2)
- howto/Debugging.md
- architecture.md
Add explicit ingress rules for ports 9090/9091 alongside the admin-only 9092 rule so the example doesn't accidentally block app traffic. Add explanatory comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The note said "you need separate rules" but the policy already includes app port rules. Updated to describe what the policy actually does.
Summary
ADMIN_PORTto config reference (Server section)Depends on go-coldbrew/core#78 (admin port feature).
Test plan
Summary by CodeRabbit
ADMIN_PORTdoc and callout recommending a dedicated admin port for metrics, profiling, and Swagger./metricsand/debug/pprof/are served whenADMIN_PORTis enabled vs. disabled.