bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3956307914
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -93,19 +116,34 @@ if env | grep '^hugegraph\.' > /dev/null; then
export PD_REST_LIST
log "PD REST peers = $PD_REST_LIST"
+ # Only worth saying where PD is actually polled: topologies without
+ # pd.peers never send this credential anywhere.
+ if [ -z "${PD_AUTH_PASSWORD}" ]; then
+ log "WARN: PD_AUTH_PASSWORD is empty; PD will answer 401 unless
it runs without auth"
+ fi
log "Timeout = ${WAIT_STORAGE_TIMEOUT_S}s"
timeout "${WAIT_STORAGE_TIMEOUT_S}s" bash -c "
log() { echo '[wait-storage] '\"\$1\"; }
+ # curl stays out of the grep pipeline so its status code is
+ # readable: a 401 is a wrong secret, not a storage problem, and
+ # retrying it for 300s only hides that.
check_any_pd_stores() {
for peer in \$(echo \"\$PD_REST_LIST\" | tr ',' ' '); do
- if curl ${PD_AUTH_ARGS} -f -s \
- --connect-timeout ${WAIT_STORAGE_PD_CONNECT_TIMEOUT_S} \
- --max-time ${WAIT_STORAGE_PD_MAX_TIMEOUT_S} \
- http://\${peer}/v1/stores 2>/dev/null | \
- grep -qi '\"state\"[[:space:]]*:[[:space:]]*\"Up\"'; then
+ body=\$(printf 'user = \"%s:%s\"\n' \
+ \"\$PD_AUTH_CURL_USER\" \"\$PD_AUTH_CURL_PASSWORD\"
| \
+ curl -K - -s -w '\n%{http_code}' \
+ --connect-timeout
${WAIT_STORAGE_PD_CONNECT_TIMEOUT_S} \
+ --max-time ${WAIT_STORAGE_PD_MAX_TIMEOUT_S} \
+ \"http://\${peer}/v1/stores\"; 2>/dev/null)
+ code=\${body##*\$'\n'}
+ if [ \"\$code\" = 401 ]; then
+ log \"ERROR: PD at \${peer} refused the credential (401):
PD_AUTH_PASSWORD must match PD's auth.secret-key\" >&2
+ return 2
Review Comment:
`return 2` fires on the first 401, so one refusing peer aborts the wait
before the rest of `PD_REST_LIST` is tried. During a rolling secret rotation,
or with mixed PD versions (a pre-1.8 PD answers 200 to any password), a Server
restart dies even when the next peer would have accepted. An alternative that
keeps the fail-fast: remember the 401, keep looping, and return 2 only when no
peer produced an Up store. A fleet-wide wrong secret still aborts on the first
pass instead of retrying for 300s, and one stale peer stops costing a Server.
The `no retry after 401` assertion in test-wait-storage.sh would then expect
one call per peer instead of one total.
##########
hugegraph-pd/hg-pd-dist/docker/docker-entrypoint.sh:
##########
@@ -26,10 +26,31 @@ require_env() {
fi
}
+# Escape a value for use inside a JSON string: backslash and quote, then every
+# remaining C0 control character as \uXXXX. Dropping only LF, as an earlier
+# version did, left CR and TAB to produce invalid JSON and a container that
+# failed before startup.
json_escape() {
- local s="$1"
- s=${s//\\/\\\\}; s=${s//\"/\\\"}; s=${s//$'\n'/}
- printf "%s" "$s"
+ local s="$1" out="" i c
Review Comment:
`[[ "$c" < $'\x20' ]]` sorts with the locale's collation, and
`${#s}`/`${s:i:1}` count characters or bytes depending on the locale, and the
image this runs in is not in the C locale: `eclipse-temurin:11-jre-jammy` sets
`LC_ALL=en_US.UTF-8`, so the walk runs under UTF-8 collation today. Pinning
`LC_ALL=C` for the function makes it deterministic: iteration is byte-wise, the
comparison is ordinal, and non-ASCII UTF-8 bytes (0x80 and up) pass through
raw, which is still valid JSON. The escape branch then only ever sees
single-byte ASCII, so the pin is the only behavioral change.
```suggestion
local LC_ALL=C
local s="$1" out="" i c
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
