Fix Kubernetes node probe portability

[backwind1233]

Summary

• strip carriage returns from rendered CubeEgress readiness and liveness probe commands
• treat any HTTP response from CubeProxy as transport reachability in the Helm health test

Why

A Windows checkout can inject CRLF into the included Bash probe and make set -e fail with an invalid option. CubeProxy also validly returns HTTP 400 for / without a sandbox Host header, so curl --fail incorrectly reports a reachable proxy as unhealthy.

Testing

• helm lint deploy/kubernetes/chart with non-default test credentials
• helm template renders without carriage returns
• all eight Helm tests passed on AKS Kubernetes v1.35.6

Assisted-by: GitHub Copilot:GPT-5.4

[cubesandboxbot]

Review: Fix Kubernetes node probe portability (#1262)

AI-generated review — the findings below are machine-produced and have not been reviewed by a human maintainer.

Summary

This PR fixes two Kubernetes chart portability issues:

1. CRLF line endings — adds a repo-root .gitattributes (* text=auto, *.yaml/*.tpl forced to LF) to prevent Windows checkouts from injecting CRLF, and defensively strips carriage returns from the rendered cube.egressNetProbeCommand via replace "\r" "" before quoting (both readiness and liveness probes).
2. CubeProxy health-test false negatives — drops curl --fail in the Helm tests so any HTTP response (including the expected HTTP 400 for / without a sandbox Host header) counts as transport reachability, and turns the previously always-passing ClusterIP check into a real hard failure on unreachable.

What I verified in the base tree

• cube.egressNetProbeCommand is the only multi-line define-based shell command in the chart (_helpers.tpl:658), and both render sites (readiness + liveness probes in node-daemonset.yaml) get the replace filter. The filter order include → replace → quote is correct.
• replace "\r" "" is the canonical Helm CRLF fix: Go template string literals are unquoted (strconv.Unquote), so "\r" is a genuine carriage return, and replace is available via Sprig. This is a no-op on an already-LF checkout, so no regression risk.
• The health-test rationale is credible: CubeProxy's own kubelet probes hit an admin-token endpoint, while / without a sandbox Host header validly returns 4xx, which curl -f would wrongly treat as failure.
• The old first health check never actually failed the test on a downed proxy — it only echoed a warning — so the change to a hard exit 1 on transport failure is a real improvement.
• CI (.github/workflows/kubernetes-chart-check.yml) lints and renders the chart on PRs touching deploy/kubernetes/chart/**, so these changes are exercised on merge. Note CI runs on Linux with LF checkouts, so it would not catch a CRLF regression — the .gitattributes/replace pair is the right belt-and-suspenders approach.

Findings

1. Low/Medium — health test now accepts 5xx as healthy (tests/node-health.yaml): removing --fail means a reachable-but-broken CubeProxy (HTTP 500) passes the test, where previously -f made it a hard failure. This is a deliberate "transport reachability" scope choice and not blocking, but consider asserting status < 500 to keep catching genuine server errors. (Inline comment on the changed line.)

2. Low — CRLF guard covers only the probe command (node-daemonset.yaml): the replace "\r" "" treatment is applied to the egress probe only; the chart's other multi-line scripts (tests/node-health.yaml | blocks, node-bootstrap-daemonset.yaml, the - | command in node-daemonset.yaml) are equally CRLF-sensitive. .gitattributes fixes fresh checkouts, but existing polluted Windows working trees need git add --renormalize . + re-checkout to pick it up. Non-blocking. (Inline comment on the changed line.)

Positives

• .gitattributes targets the root cause (checkout-time normalization) rather than only patching symptoms.
• replace "\r" "" is correctly placed and ordered.
• The curl change fixes a genuine false-negative bug (a healthy proxy failing helm test due to an expected 4xx), and the first check is now a meaningful failure rather than an always-passing soft-fail.

Conclusion

The changes are correct and well-targeted; both findings are non-blocking. Recommend confirming the 5xx-acceptance trade-off (finding 1) is intended, and optionally applying the CRLF guard more broadly (finding 2). No blocking issues found.

[fslongjin]

ping~

[cubesandboxbot]

Removing --fail makes curl exit 0 for any HTTP response, so a CubeProxy that is reachable but broken (e.g. returning HTTP 500 instead of the expected 400 for / without a sandbox Host header) now passes this test — previously -f turned a 5xx into a hard failure through the || … exit 1 chain. If the goal is only to tolerate the expected 4xx, consider asserting the status code so genuine server errors still fail:

status="$(curl --connect-timeout 5 --max-time 15 -sS -o /dev/null -w '%{http_code}' \
  "http://<proxy-service>/" || echo 000)"
test "${status}" -lt 500 || { echo "cube-proxy Service unhealthy (HTTP ${status})"; exit 1; }

keeping the HTTPS attempt as the transport fallback. (The || echo 000 guard matters because the script runs under sh -e.) Not blocking — flagging the trade-off to confirm it's intended.

[cubesandboxbot]

The replace "\r" "" guard correctly fixes this probe command (canonical Helm CRLF idiom, correctly ordered before quote). One completeness note: this is the only render site that got the guard, but the chart's other multi-line scripts — the | blocks in tests/node-health.yaml, node-bootstrap-daemonset.yaml, and the - | command at node-daemonset.yaml:81 — are equally CRLF-sensitive in a Windows working tree. .gitattributes only protects fresh checkouts; an already-polluted tree stays CRLF until re-normalized (git add --renormalize . then re-checkout). Consider applying the same guard at the other multi-line command renders, or documenting the renormalization step. Non-blocking.