bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3961653828
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -93,33 +116,74 @@ 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.
+ #
+ # A 401 is remembered rather than returned at once, so one
+ # refusing peer no longer ends the wait before the rest of
+ # PD_REST_LIST is tried. That case is real: during a rolling
+ # secret rotation, or against a pre-1.8 PD that answers 200 to
+ # any password, a Server used to die even though the next peer
+ # would have accepted it. Returning 2 only when no peer produced
+ # an Up store keeps the fail-fast for a fleet-wide wrong secret,
+ # which still aborts on the first pass instead of retrying 300s.
check_any_pd_stores() {
+ refused=
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):\" >&2
+ log ' PD_AUTH_PASSWORD must match PD
auth.secret-key' >&2
+ refused=1
+ continue
+ fi
+ if printf '%s' \"\$body\" | grep -qi
'\"state\"[[:space:]]*:[[:space:]]*\"Up\"'; then
echo \"\$peer\"
return 0
fi
done
+ [ -z \"\$refused\" ] || return 2
Review Comment:
Fixed in `cf50aa520`, taking the counting change as specified.
`check_any_pd_stores` now keeps `peers` and `refused` counters and only
returns 2 when
`[ "$peers" -gt 0 ] && [ "$refused" -eq "$peers" ]`, so a lone stale peer no
longer ends the wait. The `peers -gt 0` guard keeps an empty `PD_REST_LIST`
from satisfying `0 -eq 0`. The `:134-141` comment and the test comment at
`:266-268` now describe counting rather than remembering.
Reproduced your case first: with the new `one-401-pending` scenario against
the unfixed script, only one pass ran (two curl calls, not four) and it exited
1 with the refusal message. After the change the same case retries and the
suite is 10/10, with `401 from every peer aborts without retry` still asserting
a single pass.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-pd-shipped-config.sh:
##########
@@ -0,0 +1,74 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Every PD configuration that ships in an archive or in the jar must carry the
+# same REST hardening: no wildcard actuator exposure (that path is anonymous),
+# an auth.secret-key that is present and empty, and no copy of the secret that
+# earlier revisions published. A fix applied to one variant and not the others
+# is what this catches, so the list below covers the PD distribution, the
+# service jar, and the template the cluster test writes onto each PD node.
+
+set -euo pipefail
+
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../../../.." && pwd)"
+PUBLISHED_SECRET='FXQXbJtbCLxODc6tGci732pkH1cyf8Qg'
+# The allowlist these files must carry, spelled out. An exact comparison rather
+# than "no wildcard": a missing include:, a reordered or duplicated entry, and
+# an extra endpoint are all changes to what this port serves anonymously, and
+# each of them used to pass.
+EXPECTED_EXPOSURE='health,metrics,prometheus'
+FAIL=0
+
+check() {
+ local file="$1" rel="${1#"${ROOT}/"}" before="$FAIL"
+ [[ -f "$file" ]] || { echo " FAIL ${rel}: missing"; FAIL=1; return; }
+
+ # The actuator exposure specifically: a config that grows an unrelated
+ # include: above this block must not satisfy the check by accident.
+ local exposure
+ exposure=$(awk '/^[[:space:]]*exposure:/ {found = 1; next}
+ found && /^[[:space:]]*include:/ {
+ sub(/^[[:space:]]*include:[[:space:]]*/, "")
+ sub(/[[:space:]]+$/, "")
+ print; exit
+ }' "$file")
+ # YAML quoting is the file's business, not this contract's
+ exposure=${exposure#\"}; exposure=${exposure%\"}
+ exposure=${exposure#\'}; exposure=${exposure%\'}
+ if [[ "$exposure" != "${EXPECTED_EXPOSURE}" ]]; then
+ echo " FAIL ${rel}: actuator exposure must be exactly" \
+ "'${EXPECTED_EXPOSURE}', got '${exposure}'"; FAIL=1
+ fi
+ if ! grep -qE '^[[:space:]]*secret-key:[[:space:]]*$' "$file"; then
+ echo " FAIL ${rel}: auth.secret-key must be present and empty"; FAIL=1
+ fi
+ if grep -q "${PUBLISHED_SECRET}" "$file"; then
+ echo " FAIL ${rel}: contains the published secret"; FAIL=1
+ fi
+ # Only when nothing above raised FAIL, or the file contradicts its own
report
+ if [[ "$FAIL" == "$before" ]]; then
Review Comment:
Fixed in `cf50aa520`. `before` is gone, replaced by a per-file `local
bad=0`, with `bad=1` beside each of the three `FAIL=1` assignments and `if [[
"$bad" -eq 0 ]]; then ... fi` on the `ok` line.
Kept the `if` form for the reason you gave: as the function's last command a
bare `[[ ... ]] &&` returns 1 on a failing file and `set -euo pipefail` would
abort the loop.
Reproduced the defect first with a driver built from the head copy of
`check()` over two bad files, which printed the four FAIL lines and then `ok
a/f2.yml`. After the change the same two files print only their failures, and
the exit status is still 1. The missing-file branch keeps `FAIL=1` alone, since
it returns before the `ok` line.
##########
docker/README.md:
##########
@@ -60,12 +68,60 @@ behind an HTTPS reverse proxy and trusted network controls.
first authenticated startup. Changing `.env` does not rotate an existing
administrator password; use the HugeGraph user API for credential changes.
-For the verification commands below, set the password in your current shell:
+For the verification commands below, load `.env` into your current shell and
+set the password:
```bash
+set -a; . ./.env; set +a
ADMIN_PASSWORD='the-same-password-used-in-.env'
```
+Compose reads `.env` on its own; the line above is so that the `curl` command
and
+the Hubble helper on this page can use `${HG_PD_AUTH_SECRET_KEY}` too.
+
+The PD REST API (port 8620, HStore topologies only) has its own credential:
+requests other than health probes need HTTP Basic auth with an internal
+service name (for example `hg`) and the PD secret as the password. PD ships
+no default secret, so `HG_PD_AUTH_SECRET_KEY` is required and the HStore
+Compose files refuse to start without it. The `.env` command above generates
+one. To list registered stores:
+
+```bash
+curl -u "hg:${HG_PD_AUTH_SECRET_KEY}" http://localhost:8620/v1/stores
+```
+
+Three consumers read this credential, and all three have to agree or startup
+fails:
+
+- PD itself, through `HG_PD_AUTH_SECRET_KEY`.
+- The Server, whose `bin/wait-storage.sh` polls `/v1/stores` before the
+ Server starts. Both Compose files pass `PD_AUTH_PASSWORD` to it from the
+ same variable, so setting `HG_PD_AUTH_SECRET_KEY` in `.env` covers it. If
+ the Server sends the wrong secret `wait-storage.sh` aborts on the first
+ 401 rather than waiting out `WAIT_STORAGE_TIMEOUT_S`, and the container
+ exits with `ERROR: storage wait aborted, see the message above` after
+ logging `ERROR: PD at <peer> refused the credential (401)`.
+- Hubble, through `operations.pd.password` in
+ `conf/hubble/hstore.local.properties` (Minimal HStore) or
+ `conf/hubble/hstore-ha.local.properties` (HA). Compose mounts those files
+ read-only and does not template them, and the Hubble image has no
+ entrypoint that reads the environment, so they are generated from the
+ tracked `*.properties.example` files by `set-hubble-pd-password.sh`. The
+ `.env` recipe above already runs it. To regenerate after loading `.env`:
+
+```bash
+./set-hubble-pd-password.sh hstore # or hstore-ha
+```
+
+Run it before `docker compose up`: if the file is missing, Docker creates an
Review Comment:
Fixed in `cf50aa520`, both halves.
The README now says the Compose files pin the mount with `create_host_path:
false`, so a missing generated file makes `docker compose up` refuse to start.
The two other copies of the old wording are corrected as well, at
`test-compose.sh:325-327` and `set-hubble-pd-password.sh:28-29`;
`test-compose.sh:155-158` already read correctly and was left alone.
The printable-ASCII requirement is now in the guarantee list, naming the
ISO-8859-1 versus UTF-8 mismatch and the silent permanent 401, since that is
the least diagnosable of the three.
While there I applied the README guidance from #3187: the touched block runs
to about 120 characters instead of wrapping at 75, and the guarantee list is
folded into a `<details>` block so the urgent part stays two lines. Nothing
outside that block was reflowed, so the diff is a single hunk.
One caveat: I could not watch Compose actually refuse, since the Docker
daemon is not up here. `test-compose.sh render` passes, and as a negative
control, removing the `create_host_path` block makes render fail with `the
hstore.local.properties bind does not disable create_host_path`, so the pin is
enforced rather than merely present.
--
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]
