* fix(helm): gate S3 TLS cert args on httpsPort to stop probe failures (#9202) With `global.seaweedfs.enableSecurity=true` and the default `s3.httpsPort=0`, the chart was unconditionally passing `-cert.file` / `-key.file` to the S3 frontend. In `weed/command/s3.go`, when `tlsPrivateKey != ""` and `portHttps == 0`, the server promotes its main `-port` (8333 by default) into an HTTPS listener. The pod's readiness / liveness probes still use `scheme: HTTP`, so every kubelet probe produces http: TLS handshake error from <node-ip>:<port>: client sent an HTTP request to an HTTPS server in the pod log, as reported in #9202. `enableSecurity=true` is supposed to activate security.toml / gRPC mTLS, not silently flip the S3 HTTP port to HTTPS. Move the `seaweedfs.s3.tlsArgs` include inside the `if httpsPort` guard in all three templates that wire up an S3 frontend (standalone S3 deployment, filer with S3 sub-server, all-in-one deployment). The TLS cert args are now emitted only when the user explicitly opts into an HTTPS port; the main `-port` stays HTTP so probes work. Also add a regression test to `.github/workflows/helm_ci.yml` that renders all three templates with and without `httpsPort` and asserts the cert/key/ `-port.https` args are emitted together or not at all. * test(helm): add bash -n parse check to the S3 TLS-gating regression test Addresses gemini-code-assist review comment on #9206 flagging a potential "dangling backslash" shell-syntax risk in the rendered all-in-one command script when httpsPort is set but most S3/SFTP args are defaulted off. In practice bash -n accepts a trailing `\<newline><EOF>` (it's line-continuation to an empty line), so no current rendering is broken. Locking that contract down in CI so a future helper change that leaves a dangling backslash — or any other shell-syntax regression in the rendered command — fails loudly instead of silently shipping broken pods.
This commit is contained in:
Chris Lu
100
.github/workflows/helm_ci.yml
vendored
100
.github/workflows/helm_ci.yml
vendored
@@ -217,6 +217,106 @@ jobs:
|
||||
print("✓ No blank lines in security+S3 command blocks")
|
||||
PYEOF
|
||||
|
||||
echo ""
|
||||
echo "=== Testing security+S3: -cert.file/-key.file gated on httpsPort (issue #9202) ==="
|
||||
# Regression test: when enableSecurity=true but *.httpsPort is 0 (the default),
|
||||
# the chart must NOT emit -cert.file / -key.file to the S3 frontend. Passing
|
||||
# them promotes weed s3's main -port to HTTPS (see weed/command/s3.go), which
|
||||
# makes the HTTP readinessProbe spam "TLS handshake error ... client sent an
|
||||
# HTTP request to an HTTPS server" into the pod log.
|
||||
#
|
||||
# When *.httpsPort > 0, both -port.https and cert/key args MUST be emitted
|
||||
# together so the opt-in HTTPS listener actually has credentials.
|
||||
python3 - "$CHART_DIR" <<'PYEOF'
|
||||
import subprocess, sys, yaml
|
||||
chart = sys.argv[1]
|
||||
|
||||
def render(values):
|
||||
args = ["helm", "template", "test", chart]
|
||||
for k, v in values.items():
|
||||
args += ["--set", f"{k}={v}"]
|
||||
return subprocess.check_output(args, text=True)
|
||||
|
||||
def script_of(manifest, kind_name):
|
||||
for doc in yaml.safe_load_all(manifest):
|
||||
if not doc or doc.get("kind") not in ("Deployment", "StatefulSet"):
|
||||
continue
|
||||
if doc["metadata"]["name"] != kind_name:
|
||||
continue
|
||||
for c in doc["spec"]["template"]["spec"]["containers"]:
|
||||
cmd = c.get("command", [])
|
||||
if len(cmd) >= 3 and cmd[0] == "/bin/sh" and cmd[1] == "-ec":
|
||||
return cmd[2]
|
||||
raise AssertionError(f"no container script for {kind_name}")
|
||||
|
||||
cases = [
|
||||
# (values, workload-name, httpsPort-set?, arg-prefix)
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"s3.enabled": "true"},
|
||||
"test-seaweedfs-s3", False, ""),
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"s3.enabled": "true",
|
||||
"s3.httpsPort": "8443"},
|
||||
"test-seaweedfs-s3", True, ""),
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"filer.s3.enabled": "true"},
|
||||
"test-seaweedfs-filer", False, "s3."),
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"filer.s3.enabled": "true",
|
||||
"filer.s3.httpsPort": "8444"},
|
||||
"test-seaweedfs-filer", True, "s3."),
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"allInOne.enabled": "true",
|
||||
"allInOne.s3.enabled": "true"},
|
||||
"test-seaweedfs-all-in-one", False, "s3."),
|
||||
({"global.seaweedfs.enableSecurity": "true",
|
||||
"allInOne.enabled": "true",
|
||||
"allInOne.s3.enabled": "true",
|
||||
"allInOne.s3.httpsPort": "8445"},
|
||||
"test-seaweedfs-all-in-one", True, "s3."),
|
||||
]
|
||||
|
||||
failed = False
|
||||
for values, name, https_on, prefix in cases:
|
||||
script = script_of(render(values), name)
|
||||
cert_flag = f"-{prefix}cert.file="
|
||||
key_flag = f"-{prefix}key.file="
|
||||
https_flag = f"-{prefix}port.https="
|
||||
has_cert = cert_flag in script
|
||||
has_key = key_flag in script
|
||||
has_https = https_flag in script
|
||||
label = f"{name} (httpsPort {'set' if https_on else 'unset'})"
|
||||
if https_on:
|
||||
if not (has_cert and has_key and has_https):
|
||||
print(f"FAIL: {label}: expected {cert_flag}, {key_flag}, {https_flag} all present "
|
||||
f"(got cert={has_cert} key={has_key} https={has_https})", file=sys.stderr)
|
||||
failed = True
|
||||
else:
|
||||
print(f"✓ {label}: cert/key/https args emitted together")
|
||||
else:
|
||||
if has_cert or has_key or has_https:
|
||||
print(f"FAIL: {label}: expected none of {cert_flag}/{key_flag}/{https_flag}; "
|
||||
f"main S3 -port would silently become HTTPS and break HTTP probes "
|
||||
f"(got cert={has_cert} key={has_key} https={has_https})", file=sys.stderr)
|
||||
failed = True
|
||||
else:
|
||||
print(f"✓ {label}: no TLS args emitted, main -port stays HTTP")
|
||||
|
||||
# bash -n: pin down that the rendered script parses. Guards against
|
||||
# a future helper change that leaves a dangling `\` with nothing
|
||||
# after it (every current caller already exits cleanly because
|
||||
# bash treats trailing `\<newline><EOF>` as line-continuation to
|
||||
# an empty line — but keep the contract explicit).
|
||||
parse = subprocess.run(["bash", "-n"], input=script, text=True,
|
||||
capture_output=True)
|
||||
if parse.returncode != 0:
|
||||
print(f"FAIL: {label}: bash -n rejected rendered script: {parse.stderr.strip()}",
|
||||
file=sys.stderr)
|
||||
failed = True
|
||||
|
||||
sys.exit(1 if failed else 0)
|
||||
PYEOF
|
||||
|
||||
echo "✅ All template rendering tests passed!"
|
||||
|
||||
- name: Create kind cluster
|
||||
|
||||
@@ -247,9 +247,9 @@ spec:
|
||||
{{- $httpsPort := .Values.allInOne.s3.httpsPort | default .Values.s3.httpsPort }}
|
||||
{{- if $httpsPort }}
|
||||
-s3.port.https={{ $httpsPort }} \
|
||||
{{- end }}
|
||||
{{- include "seaweedfs.s3.tlsArgs" (dict "root" . "prefix" "s3.") | nindent 14 }}
|
||||
{{- end }}
|
||||
{{- end }}
|
||||
{{- if or .Values.allInOne.s3.enableAuth .Values.s3.enableAuth .Values.filer.s3.enableAuth }}
|
||||
-s3.config=/etc/sw/s3/seaweedfs_s3_config \
|
||||
{{- end }}
|
||||
|
||||
@@ -200,9 +200,9 @@ spec:
|
||||
{{- if .Values.global.seaweedfs.enableSecurity }}
|
||||
{{- if .Values.filer.s3.httpsPort }}
|
||||
-s3.port.https={{ .Values.filer.s3.httpsPort }} \
|
||||
{{- end }}
|
||||
{{- include "seaweedfs.s3.tlsArgs" (dict "root" . "prefix" "s3.") | nindent 14 }}
|
||||
{{- end }}
|
||||
{{- end }}
|
||||
{{- if .Values.filer.s3.enableAuth }}
|
||||
-s3.config=/etc/sw/seaweedfs_s3_config \
|
||||
{{- end }}
|
||||
|
||||
@@ -127,9 +127,9 @@ spec:
|
||||
{{- if .Values.global.seaweedfs.enableSecurity }}
|
||||
{{- if .Values.s3.httpsPort }}
|
||||
-port.https={{ .Values.s3.httpsPort }} \
|
||||
{{- end }}
|
||||
{{- include "seaweedfs.s3.tlsArgs" (dict "root" . "prefix" "") | nindent 14 }}
|
||||
{{- end }}
|
||||
{{- end }}
|
||||
{{- if .Values.s3.domainName }}
|
||||
-domainName={{ .Values.s3.domainName }} \
|
||||
{{- end }}
|
||||
|
||||
Reference in New Issue
Block a user