fix(k8s): use stable StatefulSet DNS for CubeProxy registry heartbeat

[try-agaaain]

Summary

The CubeProxy registry heartbeat (proxy_registry.lua, published from an
ngx.timer handler) connects to Redis via resty.redis → ngx.socket.tcp.
proxy.yaml pinned that target to a concrete IP captured at render or startup
time:

• chart template: lookup "v1" "Service"/"Endpoints" baked the redis
  ClusterIP or headless Endpoints IP into the env value;
• entrypoint: getent ahostsv4 resolved the hostname to an IP before nginx
  started.

Both pin a stale address once the redis Pod restarts (new Pod IP) while the
cube-proxy Pod keeps running. The heartbeat then silently fails, CLM stops
discovering the proxy, and auto_resume gate state is never pushed →
504 Gateway Timeout.

Fix

Pass the Redis hostname through to CUBE_PROXY_REGISTRY_REDIS_HOST and let
nginx resolve it per connect. The chart already renders resolver at http
scope (start.sh → conf/includes/resolver.inc), so every Lua cosocket path —
including ngx.timer handlers — resolves through it. The entrypoint no longer
pre-resolves the host to an IP (getent block removed).

For the builtin redis, the host is the headless Service DNS name
(<redis>.<ns>.svc.<domain>), which always resolves to the live Pod IP and
self-heals after a redis restart — the same mechanism
cube-lifecycle-manager already relies on.

Additional hardening

• checksum/entrypoint annotation on the cube-proxy Deployment hashes the
  entrypoint ConfigMap body so a helm upgrade that only changes the
  entrypoint rolls the pod — otherwise the old script keeps running until the
  pod is manually deleted.
• cubeProxy.resolver.valid default 30s → 5s: DNS answers are cached for
  valid seconds, so heartbeat recovery takes roughly valid + one heartbeat
  interval after a redis restart. 30s exceeded lifecycleManager.heartbeatTTL
  (15s), leaving the proxy judged dead for ~20s in that window.
• Render guard test test-proxy-registry-host.sh: asserts the builtin
  host renders as the Service DNS name, external host / IP literals pass
  through, sentinel mode leaves the host empty, and checksum/entrypoint
  tracks entrypoint content. cube-proxy-entrypoint.sh added to the CI
  sh -n syntax check.

Compatibility notes

• External redis.host / cubeProxy.redis.host overrides must be an FQDN or
  IP literal: the nginx resolver does not apply /etc/resolv.conf search
  domains (documented in README and values.yaml comments).
• Sentinel mode is unchanged (registry host stays empty; the master is
  resolved via SENTINEL).

[cubesandboxbot]

This moves the IP capture from helm-render time to proxy-container start time — it does not survive a Redis Pod IP change that happens after the proxy has started.

cube-proxy-entrypoint.sh:100-110 resolves this DNS name once at container startup (getent ahostsv4 ... | NR==1) and exports the concrete IP into CUBE_PROXY_REGISTRY_REDIS_HOST. Nothing re-resolves afterwards, and proxy_registry.lua runs off ngx.timer.every against that pinned IP, swallowing errors. So if the Redis Pod restarts (new Pod IP) while the cube-proxy Pod keeps running — the exact scenario in the PR description — the proxy still connects to the stale IP and the heartbeat silently fails until the proxy Pod is itself restarted. As written, the PR's test-plan step 1 ("roll the redis StatefulSet, confirm the proxy registry heartbeat survives the Pod IP change") would not pass without also rolling the proxy.

The stable DNS name is still a real improvement (proxy restarts become self-healing instead of requiring a fresh helm upgrade), but the codebase already has the mechanism for a true fix: nginx.conf:32-33 puts resolver at http scope "so every Lua cosocket path inherits it", so resty.redis:connect(host, ...) from the ngx.timer handler can resolve the hostname per connect and pick up a new IP within the resolver's valid= TTL (default 30s). Consider keeping the hostname as the registry target (letting the entrypoint pre-resolution remain only a best-effort optimization, or removing it) so the heartbeat self-heals on the next tick instead of pinning an IP for the container's lifetime.

[cubesandboxbot]

The premise "(ngx.timer cosockets have no nginx resolver)" contradicts the chart's own nginx config. nginx.conf:32-33 configures resolver at http scope with the explicit comment "Keep resolver configuration at http scope so every Lua cosocket path inherits it" (rendered by start.sh into resolver.inc). ngx.socket.tcp:connect() from a ngx.timer handler resolves hostnames through that http-scope resolver — the reason init_worker_phase.lua defers into ngx.timer.at(0) is cosocket creation in init_worker, not DNS resolution in timers.

That matters because this comment is what motivates the entrypoint's startup-time getent pin, which is exactly what stops the heartbeat from recovering when the Redis Pod IP changes while the proxy stays up (see the comment on the printf line below).

[cubesandboxbot]

Review: fix(k8s): use stable StatefulSet DNS for CubeProxy registry heartbeat

AI-generated review — not a human approval.

Overview

The PR fixes a real production bug. proxy_registry.lua publishes the CubeProxy registry heartbeat from an ngx.timer.every handler, connecting to Redis via resty.redis → ngx.socket.tcp. Both prior mechanisms for pinning the target address — a lookup "v1" "Endpoints" IP in the chart template, then a getent ahostsv4 resolution at container startup — cached a stale address across a redis Pod restart while the cube-proxy Pod kept running. The heartbeat silently stopped, CLM never discovered the proxy, and the auto_resume gate state was never pushed → 504 Gateway Timeout.

The fix passes the Redis hostname and lets nginx resolve it per connect, relying on the http-scope resolver already rendered into conf/includes/resolver.inc by CubeProxy/start.sh. I verified the supporting pieces on the base tree:

• The builtin redis Service is headless (clusterIP: None) with serviceName on a single-replica StatefulSet (templates/redis.yaml), so <release>-cube-redis.<ns>.svc.<domain> resolves to the live Pod IP and updates on restart — the exact name cube-lifecycle-manager already uses (templates/lifecycle-manager.yaml line 40).
• CubeProxy/nginx.conf includes resolver.inc at http scope, and start.sh renders it (prepare_resolver_include → render_resolver_include) before exec'ing nginx. Cosockets created from timer handlers resolve hostnames through the http-level resolver — standard OpenResty behavior, and the same infra the data-plane Redis path already relied on. The removed comment ("ngx.timer has no nginx resolver") was the workaround's original rationale; the http-scope resolver infra already existed on master.
• Removing the lookup() call also makes helm template output deterministic (lookup is a no-op without a live cluster), which the new test correctly exploits.

Net assessment: the approach is sound and a genuine improvement. Findings below are mostly documentation/behavioral-compatibility nits, plus one live-verification recommendation.

Findings

1. Stale default in the new template comment (minor, definite)

templates/proxy.yaml line 26: the new comment says cubeProxy.resolver.valid "(default 30s)" and then recommends lowering it to 5s — but the PR simultaneously changes the default to 5s in values.yaml and the README. The comment contradicts the change it documents. [Inline comment posted]

2. Compatibility regression for short-name external Redis (needs a release note)

Removing the getent ahostsv4 pre-resolution changes behavior for external Redis configured with a short name (e.g. redis): previously the glibc resolver applied /etc/resolv.conf search domains and produced a working heartbeat; now the name goes to nginx's resolver, which does not apply search domains, so it fails to resolve and the heartbeat silently stops — the same 504 failure mode the PR fixes. This is documented for new deployments, but existing installs using a short name will regress on helm upgrade without a values change. Recommend an explicit upgrade/release note. [Inline comment posted on the entrypoint change]

3. lifecycleManager.redis.host values comment misattributes the mechanism (minor)

The added comment in the lifecycleManager.redis section cites "nginx resolver applies no search domains." CLM connects via go-redis → Go's system resolver, which does apply search domains; the nginx limitation applies only to CubeProxy (which shares the same host value). The cross-reference to redis.host is also below this block, not above. The FQDN requirement itself is fine — just the rationale is misplaced. [Inline comment posted]

4. Core premise needs live verification (recommendation, not blocking)

The fix hinges on timer-handler cosockets resolving through the http-scope resolver. The reasoning is sound and the infra exists on master, but the new test only checks template rendering — it cannot catch a runtime resolution failure (the template test would pass even if the timer cosocket couldn't resolve). The unchecked "roll the redis StatefulSet" step in the PR's test plan is the critical verification; the existing CubeProxy/tests/test_start.sh could be extended to exercise the timer→resolver path if feasible.

5. Recovery-window math slightly understates connection pooling (minor)

redis_iresty.lua uses set_keepalive(60000, 1000); pooled cosockets are keyed by hostname:port, so stale connections to the old Pod IP drain one per failing tick, and the pool is shared with the data-plane Redis connections (same host:port). With a single registry connection per tick this matches the "valid + one heartbeat interval" claim, but under burst traffic the drain window can be a few ticks. Still comfortably under heartbeatTTL: 15s with the new valid: 5s + 5s interval (≈10s worst case), so the tuning choice is correct.

Test plan / CI

• The new deploy/kubernetes/chart/scripts/test-proxy-registry-host.sh is consistent with the existing chart guard scripts (same helm template override pattern, python3 already used by sibling tests), and the checksum tamper check meaningfully verifies the new checksum/entrypoint annotation tracks the entrypoint file. Expected host registry-host-cube-redis.default.svc.cluster.local matches cube.fullname for Chart name cube + release registry-host.
• The sh -n addition for the entrypoint in the workflow is fine.
• The checksum/entrypoint annotation is a good addition: the entrypoint is mounted from a ConfigMap without subPath, so a content-only change would otherwise never roll the Pod — exactly the failure mode for external-redis overrides. It also correctly hashes the un-indented file, which is what YAML's | block scalar strips and thus what gets mounted.

Overall

Solid fix that addresses a genuine failure mode, is consistent with existing resolver infrastructure and CLM's headless-DNS usage, and improves render determinism. Please fix the comment inconsistencies (findings 1 and 3) and add a release note for the short-name behavior change (finding 2) before merge.

[cubesandboxbot]

Two notes on this comment block:

1. Attribution is off. global.conf is included at location scope (each location block includes it), not http scope. The resolver an ngx.timer cosocket actually uses comes from the http-scope include .../includes/resolver.inc; in nginx.conf (CubeProxy/nginx.conf:33), rendered by start.sh's prepare_resolver_include. If the http-scope include were ever removed while global.conf stayed, the heartbeat would silently break again — exactly the failure mode this PR fixes. Suggest rewording to reference resolver.inc / the nginx.conf include (the parenthetical is the accurate half).

2. Load-bearing assumption worth an explicit check. The removed block's comment asserted ngx.timer has no nginx resolver; this fix depends on the opposite (http-scope resolver serving timer cosockets). Per lua-nginx-module, timer-handler coroutines are bound to the http-level loc conf, so an http-scope resolver is used by ngx.socket.tcp inside timers — consistent with OpenResty semantics, and start.sh's ensure_hostname_target_has_resolver already anticipated hostname registry targets. But since it contradicts the prior in-repo belief and has no automated coverage, the manual "roll the redis StatefulSet" verification is what actually proves it — please ensure it's run before merge.

[cubesandboxbot]

The pod DNS name is correct for the current chart (builtin redis Service is headless, StatefulSet serviceName and name both equal cube.redisName, and redis.yaml hardcodes replicas: 1), but the -0 ordinal is hardcoded here. If the StatefulSet is ever scaled, the registry would silently pin to replica 0. Consider deriving the ordinal from the StatefulSet, or at least adding a comment noting the single-replica coupling so it stays intentional.

[cubesandboxbot]

This is the load-bearing claim of the whole fix, and it directly contradicts the comment this PR deletes ("proxy_registry.lua publishes from ngx.timer, which has no nginx resolver"). If the new claim is wrong, the failure mode is worse than on the base branch: instead of a stale-but-occasionally-working heartbeat, resty.redis connect on a hostname with no usable resolver would fail and proxy_registry.lua swallows the error — so the heartbeat would never publish. The in-repo evidence supports the new claim (nginx.conf keeps resolver at http scope specifically so every Lua cosocket path inherits it, and start.sh's ensure_hostname_target_has_resolver already guards hostname registry targets), but this is important enough that the unchecked test-plan item (roll the redis StatefulSet, confirm the heartbeat survives) should be run before merge.

[cubesandboxbot]

Behavior change for external registry targets: previously a hostname here was resolved to an IP at startup via getent ahostsv4, which applies /etc/resolv.conf search domains; a bare/short name (e.g. redis-cache) worked. nginx's resolver directive does not apply search domains, so such a target now fails to resolve. The builtin-redis FQDN is unaffected (it's absolute), but worth a sentence in the chart docs/release notes if non-FQDN external redis hosts are in use.

[cubesandboxbot]

Hardcoding the -0 ordinal couples the registry target to StatefulSet internals. Today it's always correct (redis.yaml hardcodes replicas: 1, and the StatefulSet/service names both come from cube.redisName), and the comment documents the limitation. One thing to weigh: for this single-replica builtin, the headless Service DNS name that $redisHost already defaults to (<redis>.<ns>.svc.<domain>) resolves to the same Pod IP and self-heals identically on Pod restart — without the ordinal assumption — so the override is only strictly necessary if the StatefulSet is ever scaled (where it would also break unless the ordinal is derived from the StatefulSet). Consider deriving the ordinal via lookup instead of pinning replica 0.

[try-agaaain]

Addressed all review comments with commit 1b64488 (and the earlier two rounds
2e926c1 / 10055df). Mapping each comment to the final design:

1. "the getent resolution happens once at container startup — it does not survive a redis Pod IP change" (entrypoint cube-proxy-entrypoint.sh)
Removed the getent ahostsv4 pre-resolution entirely. The entrypoint now passes
the Redis hostname through untouched, and proxy_registry.lua's
resty.redis → ngx.socket.tcp resolves it through nginx's http-scope
resolver on every connect. Each timer tick re-resolves, so a redis Pod IP
change self-heals without restarting cube-proxy.

2. "the premise (ngx.timer cosockets have no resolver) contradicts the chart's own nginx config" (proxy.yaml comment)
Correct — this is the load-bearing claim and it is supported by the repo:
nginx.conf:32-33 keeps resolver at http scope via the start.sh-rendered
conf/includes/resolver.inc explicitly so "every Lua cosocket path inherits it",
and init_worker_phase.lua defers the first registry publish into
ngx.timer.at(0) because cosocket creation is illegal in init_worker, not
because DNS resolution in timers is special. The comments in both the entrypoint
and proxy.yaml now state this accurately.

3. "global.conf is included at location scope; attribute the http-scope include correctly" (entrypoint comment)
Fixed — the comment now references nginx.conf / conf/includes/resolver.inc
at http scope, not global.conf.

4. "hardcoded -0 ordinal couples to StatefulSet internals" (proxy.yaml)
Removed the -0 ordinal and the lookup() branches entirely. For the builtin
redis, cube.redisHost already defaults to the headless Service DNS name
(<redis>.<ns>.svc.<domain>), which resolves to the live Pod IP for the
single-replica StatefulSet and self-heals on restart — the same address
cube-lifecycle-manager connects to through the system resolver. No ordinal
assumption, no dead lookup code (the builtin redis Service is always headless,
so the ClusterIP branch could never fire).

5. "bare/short-name external redis targets lose search-domain resolution"
Documented in chart/README.md: redis.host / cubeProxy.redis.host must be
an FQDN or IP literal because nginx resolver does not apply /etc/resolv.conf
search domains. The builtin FQDN is unaffected.

Verified with helm lint and helm template: CUBE_PROXY_REGISTRY_REDIS_HOST
renders to the headless Service DNS name.

[cubesandboxbot]

re-resolved on every connection over-states the self-heal: nginx's resolver caches answers for valid= (values default cubeProxy.resolver.valid: 30s), and resty.redis can reuse a pooled cosocket to the old Pod IP for one tick before it is dropped. So after a redis Pod IP change the proxy heartbeat can stay down for up to ~valid + one heartbeat interval. Since CLM's heartbeatTTL is 15s, CLM can still consider the proxy stale during that window (and auto-resume can still 504 inside it). The fix is a strict improvement over the previous permanent pinning, so this is non-blocking — but consider lowering cubeProxy.resolver.valid (e.g. 5s) for the builtin-redis path, or at least documenting the recovery window rather than implying instant self-heal.

[cubesandboxbot]

The fix is split across this template (env: pinned IP → hostname) and the entrypoint script (stop pre-resolving the host). For builtin redis the env value change rolls the Deployment on helm upgrade, so the new entrypoint is picked up. But for external redis (redis.host set to an FQDN) the rendered Deployment spec is unchanged, and the ...-entrypoint ConfigMap is not checksum'd into the pod template (no checksum/config annotation — only webui.yaml does this in this chart). A plain helm upgrade will then leave the OLD entrypoint running, which still runs the removed getent pre-resolution and pins CUBE_PROXY_REGISTRY_REDIS_HOST to an IP at container start — re-introducing exactly the stale-IP bug this PR removes for FQDN external Redis. Suggest adding a checksum/config annotation for the entrypoint ConfigMap so the entrypoint change also rolls the Deployment, or noting that external-redis deployments need a manual rollout (the PR summary's "plain helm upgrade" claim only holds for the builtin case).

[try-agaaain]

Follow-up commits 752931f (fix) and 3b55dfa (test) address a review round
across all dimensions. Summary:

Design/perf — resolver.valid vs CLM TTL (validated)
Lowered cubeProxy.resolver.valid default from 30s to 5s. After a redis Pod
IP change, the registry heartbeat recovers within roughly valid + one
heartbeat interval; with 30s that exceeded CLM's heartbeatTTL: 15s, so CLM
would still judge the proxy dead for ~20s after a redis restart — the exact
auto-resume 504 window this fix targets. 5s keeps the worst-case recovery
under ~10s. Documented the coupling in values.yaml and README.

Rollout — checksum/entrypoint (new mechanism)
Added a checksum/entrypoint annotation to the cube-proxy Deployment,
hashing the entrypoint ConfigMap body (same pattern as webui.yaml). Without
it, an external-redis deployment (where the registry env value is unchanged)
would keep running the old entrypoint after helm upgrade — the old
entrypoint still pinned the Redis host to an IP, re-introducing the bug.

Shell syntax in CI
Added cube-proxy-entrypoint.sh to the kubernetes-chart-check sh -n step;
it was edited by this fix but previously unlisted.

Render guard test
New test-proxy-registry-host.sh: asserts the registry env renders to the
headless Service DNS name for builtin redis (never a pinned IP), passes
external redis.host (hostname or IP literal) through untouched, and that
checksum/entrypoint tracks the entrypoint file content.

DCO
All six commits now carry Signed-off-by: tsingyue.

The self-heal claim in the README was also reworded to be precise about the
recovery window (valid + one heartbeat interval) rather than implying
instant recovery.

[cubesandboxbot]

Stale default: this PR changes cubeProxy.resolver.valid to 5s in values.yaml and the README, but this new comment still states (default 30s) and then recommends lowering it "e.g. 5s". Please update to (default 5s) (or drop the parenthetical) so the comment matches the change it's documenting.

[cubesandboxbot]

Compatibility note worth surfacing in the release notes: this removes the getent ahostsv4 fallback, which used the glibc resolver and therefore applied /etc/resolv.conf search domains. An existing external-Redis install using a short name for redis.host / cubeProxy.redis.host (e.g. redis) previously got a working heartbeat via getent; after this change the name goes to nginx's resolver, which does not apply search domains, so the heartbeat would silently stop (proxy_registry swallows errors → CLM never discovers the proxy → the same 504 failure mode this PR fixes). The README documents the FQDN requirement for new deploys, but existing deployments won't know they need a values change on helm upgrade.

[cubesandboxbot]

Minor: this FQDN/IP rationale cites nginx's resolver, but this is the lifecycleManager.redis section — CLM connects via go-redis/Go's system resolver, which does apply /etc/resolv.conf search domains. The nginx limitation applies to CubeProxy (which shares the same host value). Also, the referenced redis.host is below this block, not above. Suggest rewording, e.g. "Shared with CubeProxy, whose nginx resolver applies no search domains; use an FQDN or IP literal."

[fslongjin]

Thank you for your contribution—this looks really good. However, your code changes introduce quite a few new comments that seem redundant. Could you remove them?