one-click: make the CubeProxy admin port configurable - #1256
one-click: make the CubeProxy admin port configurable#1256Arpit-Ahuja-293 wants to merge 3 commits into
Conversation
CubeProxy's /admin/* listener was pinned to 8082. cube-proxy runs with host networking, so anything else holding that port made nginx exit with "bind() ... (98: Address in use)" and the whole cube-sandbox-control target failed to come up, with no indication of the cause. Add CUBE_PROXY_ADMIN_PORT, defaulting to 8082 so existing deployments are unaffected, and thread it through the nginx template, the CLM discovery URLs (CUBE_PROXY_ID / CUBE_PROXY_ADMIN_URL) and the pre-start port check. The pre-start check previously covered only the HTTP/HTTPS/gRPC ports and the systemd postcheck only probed HTTP, so a failed admin bind could leave the service looking healthy while lifecycle operations were dead. Both now include the admin port. Refs TencentCloud#945 Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Arpit Ahuja <iamarpitahuja@gmail.com>
Follow up on the one-click change so the Tencent Cloud deployer does not reintroduce a hardcoded 8082. Adds a cube_proxy_admin_port variable (default 8082) and uses it for the rendered nginx ConfigMap, the container port and the cube-lifecycle-manager discovery env vars. Refs TencentCloud#945 Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Arpit Ahuja <iamarpitahuja@gmail.com>
Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Arpit Ahuja <iamarpitahuja@gmail.com>
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| -e 's|^\(\s*set \$host_proxy_port \)8081;|\1__CUBE_PROXY_HTTP_PORT__;|' \ | ||
| -e 's|^\(\s*set \$host_proxy_port \)8080;|\1__CUBE_PROXY_HTTPS_PORT__;|' \ | ||
| -e 's|^\(\s*listen \)127\.0\.0\.1:8082;|\1__CUBE_PROXY_ADMIN_LISTEN__:8082;|' \ | ||
| -e 's|^\(\s*listen \)127\.0\.0\.1:8082;|\1__CUBE_PROXY_ADMIN_LISTEN__:__CUBE_PROXY_ADMIN_PORT__;|' \ |
There was a problem hiding this comment.
Stale cubeproxy-nginx.conf regeneration check won't pick up the new token (upgrade path) — prepare_cubeproxy_nginx_conf (create.sh:616/627) only forces regeneration when __CUBE_PROXY_GRPC_PORT__ is missing. On upgrade, an existing cubeproxy-nginx.conf generated by a pre-PR deploy contains listen __CUBE_PROXY_ADMIN_LISTEN__:8082; and is kept as-is. The new replace("__CUBE_PROXY_ADMIN_LISTEN__:__CUBE_PROXY_ADMIN_PORT__", ...) in tke-addons.tf then never matches, so the deployed nginx.conf keeps the literal __CUBE_PROXY_ADMIN_LISTEN__ placeholder — nginx fails to resolve it at startup and cube-proxy CrashLoops (fresh deploys are fine; this is upgrade-only). Please require __CUBE_PROXY_ADMIN_PORT__ in both staleness checks so stale templates are regenerated.
| } | ||
| } | ||
|
|
||
| variable "cube_proxy_admin_port" { |
There was a problem hiding this comment.
New variable is not plumbed through create.sh / terraform env.example — unlike cube_proxy_replicas (create.sh:908) and cube_proxy_heartbeat_interval_ms (create.sh:915), there is no export TF_VAR_cube_proxy_admin_port="${TENCENTCLOUD_CUBE_PROXY_ADMIN_PORT:-8082}", no TENCENTCLOUD_CUBE_PROXY_ADMIN_PORT entry in the terraform env.example, and no persisted-env / --argjson round-trip (create.sh ~4233 / ~4447). So setting TENCENTCLOUD_CUBE_PROXY_ADMIN_PORT through the standard one-click env path is silently ignored and the variable always stays 8082 (only a manual TF_VAR_cube_proxy_admin_port export or terraform.tfvars would take effect). Since the PR's goal is Terraform coverage for this port, please wire it through the same four sites as the other cube_proxy_* variables, or drop it from this PR.
| wait_for_tcp_port "${postcheck_grpc_port}" "${postcheck_retries}" "${postcheck_delay}" || die "cube-proxy gRPC tcp port not ready: ${postcheck_grpc_port}" | ||
| # The admin listener is what cube-lifecycle-manager drives for pause/resume. | ||
| # Without this check nginx can fail to bind it while the public ports come up | ||
| # fine, leaving the service "healthy" but lifecycle operations broken. |
There was a problem hiding this comment.
This probe breaks the existing postcheck behavioral tests — run_cube_proxy_postcheck_case in tests/test_runtime_file_safety.sh stubs ss to report only the HTTP/GRPC ports, so every test_cube_proxy_postcheck_* case will now time out on the admin port and die, aborting the suite (set -euo pipefail). It's currently masked by that suite's pre-existing webui After= failure (test_runtime_file_safety.sh:171), but once that's fixed, the new probe breaks all of them. Please extend the ss stub (and the case env) to also report the admin port, e.g. SS_ADMIN_PORT.
| # host before we attempt to start the container; otherwise the failure mode is | ||
| # a cryptic "address already in use" from nginx inside the container. | ||
| for port in "${CUBE_PROXY_HTTP_PORT}" "${CUBE_PROXY_HTTPS_PORT}" "${CUBE_PROXY_GRPC_PORT}"; do | ||
| for port in "${CUBE_PROXY_HTTP_PORT}" "${CUBE_PROXY_HTTPS_PORT}" "${CUBE_PROXY_GRPC_PORT}" "${CUBE_PROXY_ADMIN_PORT}"; do |
There was a problem hiding this comment.
Preflight conflict check is over-broad for the admin listener — the admin server binds to CUBE_PROXY_ADMIN_LISTEN (the node IP), not 0.0.0.0, but ss -lnt "( sport = :8082 )" matches any address. A loopback-only listener on 8082 (which wouldn't conflict with a node-IP bind, e.g. a local agent on 127.0.0.1:8082) now aborts the install where it previously succeeded. Conservative direction (no false negatives), so non-blocking — but consider scoping the check to the actual listen address or noting in a comment that it is address-agnostic.
Review: one-click — make the CubeProxy admin port configurable (#1256)AI-generated review — no human maintainer sign-off. VerdictRequest changes. The one-click (docker) path is clean and well-tested: the default What's good
Findings (in severity order)1. Medium — Terraform upgrade path leaves a literal placeholder in the deployed nginx.conf
2. Medium —
|
|
#955 Thank you for your contribution, Does this meet your requirements? It has already been scheduled for the next release. |
4 participants
Problem
CubeProxy's
/admin/*listener is pinned to8082. cube-proxy runs with host networking, so if anything else on the node already holds that port, nginx exits with:cube-sandbox-control.targetthen fails to come up, andinstall.shgives no indication of why. The public HTTP/HTTPS/gRPC ports have all been configurable for a while; the admin port was the gap.Two checks made this worse rather than catching it:
up-cube-proxy.shcoveredCUBE_PROXY_HTTP_PORT/HTTPS/GRPC, but not the admin port.cube-proxy-postcheck.shonly probed HTTP and gRPC, so a failed admin bind left the unit looking healthy while every lifecycle operation (pause/resume via cube-lifecycle-manager) was dead.Fix
Adds
CUBE_PROXY_ADMIN_PORT, defaulting to8082so existing deployments are unaffected, and threads it through everything that referenced the port:up-cube-proxy.sh__CUBE_PROXY_ADMIN_PORT__nginx placeholder;CUBE_PROXY_ID/CUBE_PROXY_ADMIN_URLup-cube-proxy.shpreflightcube-proxy-postcheck.shbuild-release-bundle.shcreate.sh/tke-addons.tf/variables.tfcube_proxy_admin_portvariable across 5 sitesenv.example,README.md,README_zh.mdThe Kubernetes chart already supported
CUBE_PROXY_ADMIN_PORT(chart/files/cube-proxy/cube-proxy-entrypoint.sh), so this brings the one-click and Terraform paths in line with naming that was already settled rather than inventing any.Worth a maintainer's eye
The Terraform
cube_proxy_admin_portvariable is the only piece here that adds new public surface rather than plumbing an existing value. The issue asked for Terraform coverage, so it is included — happy to drop it into a follow-up if you would rather keep this PR to the one-click path.Tests
Written test-first in
deploy/one-click/tests/test_package_layout.sh(already run in CI byterraform-validate.yml). 8 assertions covering the default, the discovery URLs, the preflight loop, the postcheck,env.exampleand the bundle placeholder.Plus a round-trip test that goes beyond static grepping: it renders the generated template with a deliberately non-default port and asserts the result is
listen 10.0.0.7:9082;, with no127.0.0.1:8082binding left behind. Verified it actually bites by temporarily changingCubeProxy/nginx.confto listen on8085— three assertions failed — then reverting.Verification
test_package_layout.sh→package layout tests OKdeploy/one-click/tests/suite → 9/10 passterraform fmt -check -recursive→ cleanterraform validate→Success! The configuration is valid.bash -nclean on every modified scriptThe one failing suite file is
test_runtime_file_safety.sh, which fails identically on unmodifiedmaster(an unrelatedcube-sandbox-webui.serviceAfter=expectation) and is not run by CI. Untouched here.Closes #945