fix bug - #3120
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe change adds explicit gating for controller and policy-engine ChangesAdmin endpoint controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DockerEntrypoint
participant Envoy
participant HealthCheck
participant AdminServer
DockerEntrypoint->>Envoy: Inject admin listener when enabled
HealthCheck->>Envoy: Use /ready or TCP fallback
AdminServer-->>AdminServer: Gate /config_dump by enabled setting
DockerEntrypoint->>Envoy: Request graceful drain when admin is enabled
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
gateway/gateway-runtime/policy-engine/internal/admin/server_test.go (1)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd disabled-state coverage for
/config_dump.The changed test verifies only the enabled route. Add a separate test that leaves
ConfigDump.Enabledfalse and asserts that/config_dumpreturns HTTP 404. This protects the fail-closed default.🤖 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 `@gateway/gateway-runtime/policy-engine/internal/admin/server_test.go` at line 79, Add a separate test function that verifies the disabled-state behavior of the /config_dump endpoint. In this new test, set ConfigDump.Enabled to false in the configuration, then make a request to /config_dump and assert that it returns HTTP 404. This ensures the endpoint properly enforces the fail-closed default when the feature is disabled.
🤖 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 `@gateway/gateway-runtime/docker-entrypoint.sh`:
- Around line 184-193: Enforce loopback-only Router admin binding before
generating the admin configuration in the ROUTER_ADMIN_ENABLED branch of
gateway/gateway-runtime/docker-entrypoint.sh and the corresponding branch at
gateway/gateway-runtime/docker-entrypoint-debug.sh; reject non-loopback
ROUTER_ADMIN_HOST values. Update the debug entrypoint’s loopback documentation
to match the enforced behavior. In kubernetes/helm/gateway-helm-chart/README.md,
do not describe the admin interface as loopback-only unless both entrypoints
enforce that boundary.
In `@gateway/gateway-runtime/health-check.sh`:
- Around line 32-46: Remove the HTTP listener TCP fallback from the
ROUTER_ADMIN_ENABLED=false branch of health-check.sh, since it cannot establish
Envoy readiness. Use a genuine Envoy readiness check instead, or separate the
probe behavior so TCP is used only for liveness and readiness validates loaded
routes, clusters, listeners, and ext_proc configuration.
In
`@kubernetes/gateway-operator/internal/controller/resources/api-platform-gateway-k8s-manifests.yaml`:
- Around line 37-40: Update the Deployment readiness and liveness probes near
the existing port 9901 `/ready` checks to target a listener that is enabled and
reachable, such as the runtime health endpoint or a TCP probe on the HTTP
listener. Do not rely on the disabled or loopback-only Envoy admin listener
unless the Deployment explicitly enables and locally exposes it.
---
Nitpick comments:
In `@gateway/gateway-runtime/policy-engine/internal/admin/server_test.go`:
- Line 79: Add a separate test function that verifies the disabled-state
behavior of the /config_dump endpoint. In this new test, set ConfigDump.Enabled
to false in the configuration, then make a request to /config_dump and assert
that it returns HTTP 404. This ensures the endpoint properly enforces the
fail-closed default when the feature is disabled.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34116d44-b762-4c78-be7c-afb6645f9a24
📒 Files selected for processing (29)
gateway/configs/config-template.tomlgateway/configs/config.tomlgateway/distribution/docker-compose.yamlgateway/docker-compose-perf.yamlgateway/docker-compose.debug.yamlgateway/docker-compose.yamlgateway/gateway-controller/pkg/adminserver/server.gogateway/gateway-controller/pkg/adminserver/server_test.gogateway/gateway-controller/pkg/config/config.gogateway/gateway-runtime/docker-entrypoint-debug.shgateway/gateway-runtime/docker-entrypoint.shgateway/gateway-runtime/health-check.shgateway/gateway-runtime/policy-engine/internal/admin/server.gogateway/gateway-runtime/policy-engine/internal/admin/server_test.gogateway/gateway-runtime/policy-engine/internal/config/config.gogateway/gateway-runtime/router/config/envoy-bootstrap.yamlgateway/it/docker-compose.test.postgres.yamlgateway/it/docker-compose.test.sqlserver.yamlgateway/it/docker-compose.test.vhosts-multi.yamlgateway/it/docker-compose.test.vhosts-single.yamlgateway/it/docker-compose.test.yamlgateway/it/test-config.tomlgateway/it/test-config.vhosts-multi.tomlgateway/it/test-config.vhosts-single.tomlkubernetes/gateway-operator/internal/controller/resources/api-platform-gateway-k8s-manifests.yamlkubernetes/helm/gateway-helm-chart/README.mdkubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yamlkubernetes/helm/gateway-helm-chart/templates/gateway/gateway-runtime/deployment.yamlkubernetes/helm/gateway-helm-chart/values.yaml
| # The admin interface has no entry in the static bootstrap (envoy-bootstrap.yaml) at all, so | ||
| # it only exists when explicitly opted into here — bound to ROUTER_ADMIN_HOST (loopback by | ||
| # default), never 0.0.0.0, so it is unreachable outside the pod's network namespace regardless. | ||
| if [ "${ROUTER_ADMIN_ENABLED}" = "true" ]; then | ||
| CONFIG_OVERRIDE="${CONFIG_OVERRIDE} | ||
| admin: | ||
| address: | ||
| socket_address: | ||
| address: ${ROUTER_ADMIN_HOST} | ||
| port_value: ${ROUTER_ADMIN_PORT} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^gateway/gateway-runtime/docker-entrypoint(.*\.sh)?$|^kubernetes/helm/gateway-helm-chart/README.md$|^gateway/it/docker-compose.*\.ya?ml$)' || true
echo
echo "== entrypoint relevant sections =="
for f in gateway/gateway-runtime/docker-entrypoint.sh gateway/gateway-runtime/docker-entrypoint-debug.sh; do
if [ -f "$f" ]; then
echo "--- $f defaults/usages ---"
rg -n 'ROUTER_ADMIN_(ENABLED|HOST|PORT)|routerAdminEnabled|admin_server|admin:' "$f" -C 3 || true
echo
fi
done
echo
echo "== compose relevant admin exposure =="
for f in gateway/it/docker-compose.test.yaml gateway/it/docker-compose.test.vhosts-single.yaml; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n 'routerAdmin|9901|admin|host|ports:' "$f" -C 4 || true
fi
done
echo
echo "== helm values/schema references =="
fd -i 'values.*\.ya?ml|schema\.json|README\.md' kubernetes/helm/gateway-helm-chart gateway -d 3 | xargs -r rg -n 'routerAdminEnabled|routerAdminHost|admin_server|9901|config_dump' -C 3 || true
echo
echo "== shell-like validation impact probe =="
python3 - <<'PY'
from pathlib import Path
for path in [
Path("gateway/gateway-runtime/docker-entrypoint.sh"),
Path("gateway/gateway-runtime/docker-entrypoint-debug.sh"),
]:
text = path.read_text()
snippet = text[text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"):text.find("fi", text.find("if [ \"${ROUTER_ADMIN_HOST}\"", text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];")) if "if [ \"${ROUTER_ADMIN_HOST}\"" in text[text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"):] else -1)+1] if "if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ]; else " in text[text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"):] else text[text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"):text.find("\n\ncat", text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"))]]
print(f"{path}:")
print(text[text.find("if [ \"${ROUTER_ADMIN_ENABLED}\" = \"true\" ];"):].split("\n\n",1)[0][:800])
PYRepository: wso2/api-platform
Length of output: 35046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== admin host source/defaults =="
rg -n 'ROUTER_ADMIN_(ENABLED|HOST|PORT)|gateway\.gatewayRuntime\.deployment\.env\.routerAdmin|deployment\.env:' gateway kubernetes -C 4 || true
echo
echo "== compose explicit non-localhost exposure =="
python3 - <<'PY'
from pathlib import Path
for path in Path("gateway/it").glob("docker-compose*.yaml"):
text = path.read_text()
lines = text.splitlines()
for i,l in enumerate(lines, 1):
if "9901:9901" in l:
print(path, i, l.strip())
for i,l in enumerate(lines, 1):
if "ROUTER_ADMIN_HOST" in l:
print(path, i, l.strip())
PYRepository: wso2/api-platform
Length of output: 31399
Security Misconfiguration (CWE-16)
Reachability: Internal
Reachability path
● Entry
gateway/gateway-runtime/docker-entrypoint-debug.sh:220
shutdown
│
▼
● Sink
gateway/gateway-runtime/docker-entrypoint.sh
Enforce or accurately document the Router admin bind boundary.
ROUTER_ADMIN_HOST is documented as loopback-only but is accepted as-is when ROUTER_ADMIN_ENABLED=true; a non-loopback value makes the enabled Envoy admin listener reachable over any network path. Reject non-loopback values before writing admin.address in both gateway/gateway-runtime/docker-entrypoint.sh and gateway/gateway-runtime/docker-entrypoint-debug.sh; update gateway/gateway-runtime/docker-entrypoint-debug.sh’s loopback claim accordingly. Do not document Envoy admin in kubernetes/helm/gateway-helm-chart/README.md as loopback-only unless the entrypoints enforce that boundary.
📍 Affects 3 files
gateway/gateway-runtime/docker-entrypoint.sh#L184-L193(this comment)gateway/gateway-runtime/docker-entrypoint-debug.sh#L145-L153kubernetes/helm/gateway-helm-chart/README.md#L168-L168
🤖 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 `@gateway/gateway-runtime/docker-entrypoint.sh` around lines 184 - 193, Enforce
loopback-only Router admin binding before generating the admin configuration in
the ROUTER_ADMIN_ENABLED branch of gateway/gateway-runtime/docker-entrypoint.sh
and the corresponding branch at
gateway/gateway-runtime/docker-entrypoint-debug.sh; reject non-loopback
ROUTER_ADMIN_HOST values. Update the debug entrypoint’s loopback documentation
to match the enforced behavior. In kubernetes/helm/gateway-helm-chart/README.md,
do not describe the admin interface as loopback-only unless both entrypoints
enforce that boundary.
| # Check Router (Envoy) readiness. | ||
| # The admin interface (/ready) is disabled by default (see docker-entrypoint.sh / | ||
| # ROUTER_ADMIN_ENABLED) — fall back to a raw TCP check against the main listener, | ||
| # which confirms Envoy is up and accepting connections. | ||
| if [ "${ROUTER_ADMIN_ENABLED}" = "true" ]; then | ||
| ROUTER_STATUS=$(curl -s -o /dev/null -w '%{http_code}' "http://localhost:${ROUTER_ADMIN_PORT}/ready") | ||
| if [ "$ROUTER_STATUS" != "200" ]; then | ||
| echo "Router not ready (HTTP ${ROUTER_STATUS})" | ||
| exit 1 | ||
| fi | ||
| else | ||
| if ! (exec 3<>"/dev/tcp/127.0.0.1/${ROUTER_HTTP_PORT}") 2>/dev/null; then | ||
| echo "Router not accepting connections on port ${ROUTER_HTTP_PORT}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file header and relevant lines =="
sed -n '1,90p' gateway/gateway-runtime/health-check.sh 2>/dev/null || true
echo
echo "== references to health-check/router ports/admin =="
rg -n "health-check|ROUTER_ADMIN_ENABLED|ROUTER_ADMIN_PORT|ROUTER_HTTP_PORT|readiness|liveness|/dev/tcp|/ready|curl" gateway -S || true
echo
echo "== files mentioning envoy readiness/admin routes =="
rg -n "ready|admin|readiness|Envoy" -S gateway/gateway-runtime gateway 2>/dev/null | head -200 || trueRepository: wso2/api-platform
Length of output: 46070
Do not use the HTTP listener connection as a readiness probe.
health-check.sh is used by Kubernetes readiness probes, but the ROUTER_ADMIN_ENABLED=false path returns healthy after a TCP connection to 127.0.0.1:${ROUTER_HTTP_PORT}. A listening port does not prove Envoy has loaded usable routes, clusters, listeners, or the ext_proc policy engine. Use a real readiness check for readiness, or split liveness/readiness probes so this TCP fallback is not used as readiness.
🤖 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 `@gateway/gateway-runtime/health-check.sh` around lines 32 - 46, Remove the
HTTP listener TCP fallback from the ROUTER_ADMIN_ENABLED=false branch of
health-check.sh, since it cannot establish Envoy readiness. Use a genuine Envoy
readiness check instead, or separate the probe behavior so TCP is used only for
liveness and readiness validates loaded routes, clusters, listeners, and
ext_proc configuration.
| # Envoy admin (9901) is intentionally NOT published here — it's disabled by | ||
| # default in the image (ROUTER_ADMIN_ENABLED=false) and, even when enabled, | ||
| # binds loopback-only; publishing it on a Service would defeat that. See | ||
| # kubernetes/helm/gateway-helm-chart's expose.routerAdmin (default false). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Change the probes to use a listener that remains enabled.
The runtime defaults ROUTER_ADMIN_ENABLED to false, but this Deployment still probes /ready on port 9901 at Lines 268-289. No listener exists, so the pod can fail readiness and liveness checks. If administration is enabled later, the default 127.0.0.1 bind is still unreachable from a kubelet probe sent to the pod IP. Use the runtime health check or a TCP probe on the HTTP listener, or explicitly configure a local probe and enable the admin listener.
🤖 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
`@kubernetes/gateway-operator/internal/controller/resources/api-platform-gateway-k8s-manifests.yaml`
around lines 37 - 40, Update the Deployment readiness and liveness probes near
the existing port 9901 `/ready` checks to target a listener that is enabled and
reachable, such as the runtime health endpoint or a TCP probe on the HTTP
listener. Do not rely on the disabled or loopback-only Envoy admin listener
unless the Deployment explicitly enables and locally exposes it.
067f59f to
6f36a61
Compare
1 participant
fix bug