feat(CubeProxy): make admin port configurable and detect port conflicts - #955
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| for port in "${CUBE_PROXY_HTTP_PORT}" "${CUBE_PROXY_HTTPS_PORT}" "${CUBE_PROXY_ADMIN_PORT}"; do | ||
| if command_output_contains_fixed_string "LISTEN" ss -lnt "( sport = :${port} )"; then | ||
| die "port ${port} is already in use; cube-proxy uses host networking and requires it to be free" | ||
| die "port ${port} is already in use; cube-proxy uses host networking and requires it to be free (override the admin port with CUBE_PROXY_ADMIN_PORT if 8082 is taken)" |
There was a problem hiding this comment.
The die message unconditionally advises "override the admin port with CUBE_PROXY_ADMIN_PORT" even when the conflicting port is HTTP or HTTPS. It also hard-codes 8082 instead of referencing $CUBE_PROXY_ADMIN_PORT, so if the admin port was customized to e.g. 9090 and that specific port is the one in use, the error points to the wrong default.
Consider:
admin_advice=""
if [[ "${port}" == "${CUBE_PROXY_ADMIN_PORT}" ]]; then
admin_advice=" (override the admin port with CUBE_PROXY_ADMIN_PORT)"
fi
die "port ${port} is already in use; cube-proxy uses host networking and requires it to be free${admin_advice}"
| ) | ||
| # Pods run with per-pod networking, so the admin server binds 0.0.0.0 and the | ||
| # port is parametrized independently for consistency with the systemd path. | ||
| cubeproxy_nginx_conf_rendered = replace( |
There was a problem hiding this comment.
__CUBE_PROXY_ADMIN_PORT__ is substituted in a separate replace() call (lines 120-124) outside the main nested replace() chain (lines 103-117). This is inconsistent with how all other tokens are handled: a future developer adding or removing a template token must know to check both paths. Consider including __CUBE_PROXY_ADMIN_PORT__ in the main chain with tostring() inline (e.g. tostring(var.cube_proxy_admin_port)) so the rendering is fully self-contained.
| source "${SCRIPT_DIR}/common.sh" | ||
|
|
||
| postcheck_port="${CUBE_PROXY_HTTP_PORT:-80}" | ||
| admin_port="${CUBE_PROXY_ADMIN_PORT:-8082}" |
There was a problem hiding this comment.
The admin port defaults to 8082 here, but the systemd unit may not propagate the CUBE_PROXY_ADMIN_PORT env var. If the admin port was customized but the env var isn't forwarded to this postcheck, the script probes port 8082 (the wrong port). If some other service happens to listen on 8082, this produces a false-positive success. Consider reading from a systemd environment file or noting this dependency upstream where the unit file is defined.
Review:
|
|
When I ran install.sh, I encounter a problem of silent failure: |
67229ba to
9bc711a
Compare
|
@mon3stera Fixed in 9bc711a. install.sh ran a bare systemctl enable --now, so when cube-proxy failed on the 8082 conflict, set -e aborted before quickcheck and systemd swallowed the child's logs. Now it dumps status + journal for failed cube-sandbox-* units and dies with an actionable hint. |
|
The CubeProxy preflight itself works. However, I found that the new installer diagnostic path is not triggered with the current target dependencies.
The installer then proceeds to quickcheck, while the actionable port-conflict message remains only in the service journal. Other issues:
|
9bc711a to
9262837
Compare
Thanks, fixed in 9262837:
Please test it when you have a chance. |
9262837 to
d1c26cc
Compare
d1c26cc to
f447f75
Compare
f447f75 to
7dbb6d8
Compare
|
@mon3stera There is a requested change from your previous review. I would like to get an explicit approve from you on this PR. Thanks. |
|
Overall, the changes look good. Please address the remaining suggestions left by the bot |
7dbb6d8 to
f48d5e5
Compare
f48d5e5 to
eff0b8d
Compare
| # free on the host before we start the unit. The failure mode when they are not | ||
| # is nginx aborting inside the container with a cryptic "address already in use" | ||
| # that crash-loops and -- because the control target only Wants= its members -- | ||
| # never surfaces as a non-zero exit, leaving quickcheck to burn the whole |
There was a problem hiding this comment.
No port validation before ss conflict check
check_cube_proxy_port_conflict_preflight uses CUBE_PROXY_ADMIN_PORT (and the HTTP/HTTPS ports) directly in an ss invocation without validating they are valid TCP port numbers. If an operator sets CUBE_PROXY_ADMIN_PORT=abc (a typo), the ss error is swallowed by 2>/dev/null || true, the grep finds no LISTEN on the empty result, and the preflight passes silently.
The invalid value is eventually caught by _validate_host_port in up-cube-proxy.sh, but only after start_systemd_target has already been called, wasting the install attempt.
Suggestion: Add a _validate_host_port call (or inline equivalent) for the three ports before the ss loop, matching the validation already present in up-cube-proxy.sh. This could be achieved by sourcing the shared validation function, or by duplicating the simple [[ "${port}" =~ ^[0-9]+$ ]] && (( port >= 1 && port <= 65535 )) check.
|
@yingjun8 @mon3stera Thanks. Will take this in our next release. |
| "__CUBE_PROXY_ADMIN_LISTEN__", | ||
| "0.0.0.0" | ||
| ) | ||
| # Pods run with per-pod networking, so the admin server binds 0.0.0.0 and the |
There was a problem hiding this comment.
@tinklone Hi, could you please take a look and see if this change is correct?
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Scope review
The core change (CUBE_PROXY_ADMIN_PORT threaded through templates / IDs / URLs / TF, plus admin in the service-level conflict/readiness checks) looks necessary and focused.
However, roughly ~2/3 of the diff is a broader installer/quickcheck diagnostics rewrite that is only loosely related to making the admin port configurable. Suggest splitting into:
- This PR: admin port config + service/postcheck conflict detection
- Follow-up: installer UX /
check_unit_activefail-fast rewrite
If kept together, please at least address the P1 dead-path issue on dump_failed_cube_units, and call out the installer UX work explicitly in the PR summary.
| # `enable --now` returns non-zero as soon as a required child unit fails to | ||
| # start. Without this guard `set -e` would abort the installer right here -- | ||
| # before the quickcheck below ever runs -- leaving no diagnostics on screen. | ||
| if ! systemctl enable --now "${target}"; then |
There was a problem hiding this comment.
[P1] dump_failed_cube_units likely never runs on the #945 failure path
This diagnostics dump is gated on if ! systemctl enable --now "${target}". But this PR (and the new quickcheck.sh comments) already document that cube-sandbox-control.target only Wants= its members — so a failed child unit (including cube-proxy) typically still leaves enable --now returning 0.
That means the actual #945 path — "8082 taken → cube-proxy crash-loops → installer appears to succeed / stalls silently" — almost never enters this branch. The helper covers a Requires=/hard-dependency failure mode, not the Wants= silent-failure mode this PR claims to fix.
Please either:
- Drop this from the PR and move it to a separate installer-UX change; or
- Run the scan after a successful
enable --now, looking for non-activecube-sandbox-*.serviceunits (so Wants= failures are actually covered).
As written, ~45 lines of new code don't help close #945 but do add maintenance surface.
| cube-sandbox-control.target \ | ||
| cube-sandbox-compute.target >/dev/null 2>&1 || true | ||
| systemctl enable --now "${target}" | ||
| # `enable --now` returns non-zero as soon as a required child unit fails to |
There was a problem hiding this comment.
[P1] Internal contradiction with the Wants= narrative
This comment says enable --now returns non-zero as soon as a required child fails.
But lines 258–260 in this file, and quickcheck.sh lines 135–137, emphasize that the control target only uses Wants=, so a member failure does not make enable --now fail.
Please reconcile: if Wants= is the real model, this guard does not cover #945; if some path really fails enable --now, cite the concrete dependency graph. As written, future readers will wrongly assume "installer dump covers port conflicts".
| # deployment legitimately holds these ports, so probing before the stop would | ||
| # always false-positive. After the stop our own services have released the | ||
| # ports and a remaining listener means a genuine external conflict. | ||
| check_cube_proxy_port_conflict_preflight() { |
There was a problem hiding this comment.
[P2] Duplicate of the up-cube-proxy.sh conflict check — not required for the feature loop
up-cube-proxy.sh already checks HTTP/HTTPS/admin conflicts and (per the comments here) remains the source of truth for systemctl restart cube-proxy. This copies the same logic again and explicitly admits a TOCTOU window.
For closing #945, service-level detection + postcheck probing the admin port is sufficient. An install-time preflight only buys "fail before starting unrelated units" UX — it is not part of making the admin port configurable.
Suggest splitting to a follow-up, or at least sharing one implementation so the two paths cannot drift (e.g. install still lacks the same port-format validation as up-cube-proxy.sh).
| # was legitimately still coming up. | ||
| # We therefore fetch every category in a single `systemctl show` call and branch | ||
| # exactly once per iteration on the snapshot, so the verdict is consistent. | ||
| check_unit_active() { |
There was a problem hiding this comment.
[P2] Related, but scope-creep — consider a separate PR
This grew from "don't burn the readiness budget on a already-failed unit" into a full systemd state machine (Load/Active/Sub/Result/Job/NRestarts + journal dumps + three new tests). That is a general fix for silent failures under Wants=, not "add CUBE_PROXY_ADMIN_PORT".
Helpful for #945, but the volume (~+120 implementation, ~+110 tests) already dwarfs the core feature. Prefer a dedicated PR (e.g. fail-fast systemd readiness). Keep this PR to admin-port wiring + service/postcheck probing.
If it must stay: please say so explicitly in the title/summary so reviewers don't evaluate this as a small config knob.
| # pre-check only catches an *external* listener, not this self-conflict, so an | ||
| # equal pair would otherwise crash-loop the container with a cryptic | ||
| # "address already in use" from nginx. | ||
| if [[ "${CUBE_PROXY_HTTP_PORT}" == "${CUBE_PROXY_HTTPS_PORT}" ]]; then |
There was a problem hiding this comment.
[P3] HTTP≠HTTPS check is orthogonal to admin-port configurability
Validating admin ≠ HTTP/HTTPS belongs with this feature. Requiring HTTP ≠ HTTPS is sensible hardening, but it is unrelated to making the admin port configurable (likely review/bot follow-on).
Non-blocking — just further amplifies the "rewriting the whole port subsystem" feel. Fine as a follow-up if we want a tighter PR.
| : | ||
| } | ||
|
|
||
| # cube-proxy runs with network_mode: host, so its HTTP/HTTPS/admin ports must be |
There was a problem hiding this comment.
[P3] Comment essay is too long for an install script
~20 lines here re-litigate TOCTOU, Wants=, and why this must run after stop — better suited to the PR description / a short design note than inline in install.sh.
Same pattern shows up in quickcheck.sh / up-cube-proxy.sh in this PR. Please compress to ~3–5 lines: what it does, why it must run after stop, and that the service-level check remains source of truth.
| ) | ||
| # Pods run with per-pod networking, so the admin server binds 0.0.0.0 and the | ||
| # port is parametrized independently for consistency with the systemd path. | ||
| cubeproxy_nginx_conf_rendered = replace( |
There was a problem hiding this comment.
[P3] Admin-port replace() is inconsistent with the main token chain
__CUBE_PROXY_ADMIN_PORT__ is substituted in a separate replace() while other tokens live in the nested chain. Functionally fine, but easy to miss when adding/removing tokens later.
Consider folding it into the main chain with tostring(var.cube_proxy_admin_port) for consistency. Non-blocking.
…nflicts CubeProxy runs with host networking and hard-coded its admin server to 8082, so a pre-existing listener on that port crash-looped the container while the installer stalled at cube-sandbox-control.target without surfacing the bind() failure. Add CUBE_PROXY_ADMIN_PORT (default 8082, backward-compatible) threaded through up-cube-proxy.sh, the nginx template transforms, and the Terraform/K8s deployment. Also add the admin port to the pre-start conflict check, the post-start readiness loop, and the systemd post-start check so a conflict fails early with a clear message. Closes TencentCloud#945 Signed-off-by: yingjun <yingjun.yc@antgroup.com>
Drop the installer port preflight and dump_failed_cube_units dead path, and replace the systemd state machine with a small failed-unit fail-fast. Admin-port configurability and cube-proxy conflict/readiness checks stay. Signed-off-by: jinlong <jinlong@tencent.com>
eff0b8d to
0e8cb41
Compare
| "cube.app+3-key.pem" | ||
| "__CUBE_PROXY_ADMIN_LISTEN__", | ||
| "0.0.0.0" | ||
| ), |
There was a problem hiding this comment.
Split replace silently drops the port override for old-format templates. The previous code replaced the combined literal __CUBE_PROXY_ADMIN_LISTEN__:8082; this splits it into two independent replaces. If the file consumed by file() still uses the old placeholder format (i.e. a cubeproxy-nginx.conf generated by a pre-PR release, when terraform apply is run directly and prepare_cubeproxy_nginx_conf's regeneration is bypassed), the __CUBE_PROXY_ADMIN_LISTEN__ replace renders 0.0.0.0:8082 and the __CUBE_PROXY_ADMIN_PORT__ replace finds nothing to substitute — so nginx binds 8082 regardless of var.cube_proxy_admin_port, while container_port and CUBE_PROXY_ADMIN_URL/CUBE_PROXY_RESUME_URL advertise the new port. That mismatch breaks cube-lifecycle-manager's discovery of the proxy admin endpoint. create.sh regenerates stale files in the normal flow, but the HCL itself has no guard. Consider also replacing the legacy combined pattern (e.g. __CUBE_PROXY_ADMIN_LISTEN__:8082 → 0.0.0.0:<port>) so a stale file cannot silently ignore the variable.
| active|reloading) | ||
| return 0 | ||
| ;; | ||
| failed) |
There was a problem hiding this comment.
Fast-failing on ActiveState=failed can trip on a transient restart. Every unit probed by check_unit_active is Restart=on-failure with RestartSec=2s (see deploy/one-click/systemd/*.service). When such a unit crashes once during cold start — e.g. a dependency race — it passes through failed before systemd schedules the auto-restart; the old wait_until + systemctl is-active loop would keep polling until the restart succeeded, but this new code calls die on the first failed observation. For the target scenario (a permanently crash-looping cube-proxy whose bind conflict trips the start limit) this is exactly right, but it narrows the tolerance for a one-off failure inside the 120s readiness window. Worth confirming a unit stays failed across at least two probe intervals before dying, or scoping the fast-fail to the cube-proxy unit only.
| default = 8082 | ||
|
|
||
| validation { | ||
| condition = var.cube_proxy_admin_port > 0 && var.cube_proxy_admin_port < 65536 |
There was a problem hiding this comment.
Validation permits non-integer ports. 8082.5 satisfies > 0 && < 65536 and then fails late — at the Kubernetes API for container_port, or in nginx parsing the rendered listen. The adjacent cube_proxy_heartbeat_interval_ms validation enforces an integer with floor(...) == ...; apply the same here (e.g. floor(var.cube_proxy_admin_port) == var.cube_proxy_admin_port). The shell-path _validate_host_port in up-cube-proxy.sh also relies on bare (( port >= 1 && port <= 65535 )), which parses leading-zero values as octal and can wrap huge digit strings, unlike the codebase's validate_host_port (10#${port}); forcing base-10 there too would keep the two paths consistent.
6 participants
Summary
CubeProxy runs with host networking and its
/admin/*server was hard-coded to port8082. When another process on the node already holds that port, nginx aborts at startup withbind() to <node-ip>:8082 failed (98: Address already in use)and the container crash-loops — while the one-click installer stalls atcube-sandbox-control.targetwithout ever surfacing the real cause (#945).This PR fixes both halves of the problem:
CUBE_PROXY_ADMIN_PORT(default8082, fully backward-compatible), threaded through the entire stack — systemd one-click path, nginx templating, and the Terraform/TKE deployment.Root cause
Two independent defects:
CUBE_PROXY_ADMIN_LISTEN) but left the port8082hard-coded — in the nginx template transforms, in the derivedCUBE_PROXY_ID/CUBE_PROXY_ADMIN_URL, and in the Terraform deployment — unlike HTTP/HTTPS/gRPC, which already haveCUBE_PROXY_HTTP_PORT/CUBE_PROXY_HTTPS_PORT/CUBE_PROXY_GRPC_PORT.Changes
1. Admin port configurable — systemd one-click path
up-cube-proxy.sh: newCUBE_PROXY_ADMIN_PORT(default 8082); validates it is a TCP port in 1–65535 and distinct from the HTTP/HTTPS/gRPC ports; derivesCUBE_PROXY_ID/CUBE_PROXY_ADMIN_URLfrom it; renders the new__CUBE_PROXY_ADMIN_PORT__nginx template placeholder.env.example: documents the new variable.2. Fail-fast conflict & readiness detection
up-cube-proxy.shpre-start check now covers the admin port, and the error names the specific override variable for whichever port collided (e.g. port 80 occupied → suggestsCUBE_PROXY_HTTP_PORT, not the admin port).docker logs cube-proxyfor thebind()failure.cube-proxy-postcheck.sh(systemdExecStartPost) probes the admin port as well, so a bind failure marks the unit failed with a clear message.quickcheck.sh:check_unit_activenow dies immediately if the unit is already infailedstate (withsystemctl statusoutput) instead of polling until the 120s readiness budget expires — relevant becausecontrol.targetonlyWants=its children, so a failed child never failsenable --now.3. nginx template placeholder
build-release-bundle.sh,terraform/tencentcloud/create.sh,tests/test_package_layout.sh: the sed transforms now emit__CUBE_PROXY_ADMIN_LISTEN__:__CUBE_PROXY_ADMIN_PORT__(previously:8082), and the placeholder-validation loops require the new token.create.shregenerates a stale template that lacks it.4. Terraform / TKE
variables.tf: newcube_proxy_admin_portvariable (number, validated 1–65535, default 8082).create.sh/destroy.sh:TENCENTCLOUD_CUBE_PROXY_ADMIN_PORT→TF_VAR_cube_proxy_admin_port, persisted to.envandresolved.auto.tfvars.json(destroy keeps parity).tke-addons.tf: the admin-portreplace()is folded into the main token chain (tostring(var.cube_proxy_admin_port));containerPortand theCUBE_PROXY_ADMIN_URL/CUBE_PROXY_RESUME_URLenv values follow the variable.terraform/tencentcloud/env.example: documents the new variable.5. Docs
docs/guide/lifecycle.md+docs/zh/guide/lifecycle.md: note that the admin port defaults to 8082 and can be overridden withCUBE_PROXY_ADMIN_PORTwhen the port is taken (EN/ZH in sync).6. Tests
test_runtime_file_safety.sh: the postcheck harness now stubs the admin port (thessstub appends across the three probes); new casetest_quickcheck_check_unit_active_dies_fast_on_failed_unitverifying the failed-unit fail-fast (dies immediately, printssystemctl status, never sleeps).test_package_layout.sh: covers the new template placeholder (and picks up the gRPC placeholder it was missing).Scope note
Earlier revisions of this PR also added an installer-level port preflight and a full systemd state machine in
quickcheck.sh. Per review feedback these were removed as out of scope / redundant with the service-level checks above; general fail-fast behavior for the installer underWants=units will be handled in a follow-up. What remains here is scoped to the admin port: configurability plus service-level conflict/readiness detection.Testing
deploy/one-click/tests/test_runtime_file_safety.sh— OKdeploy/one-click/tests/test_package_layout.sh— OKCloses #945