bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3959273112
##########
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:
🧹 This describes the failure mode the same head replaced, and the paragraph
under it is now missing one of the helper's checks.
Both HStore Compose files pin the Hubble mount with `create_host_path:
false` (`docker-compose-hstore.yml:134`,
`docker-compose-3pd-3store-3server.yml:255`), so a missing `*.local.properties`
makes Compose refuse to start rather than mounting an empty directory over
Hubble's config. `test-compose.sh:155-158`, the comment above
`assert_hubble_bind_pinned`, states the new behaviour correctly; a different
comment in the same file, `:325-327`, still carries the old wording, as does
`set-hubble-pd-password.sh:28-29`.
`:118-121` lists what the helper guarantees but not the printable-ASCII
rejection added at this head (`set-hubble-pd-password.sh:57-60`), which is the
check an operator is most likely to trip. Its failure mode is the least
diagnosable of the three: a non-ASCII secret gives a permanent 401 with nothing
logged on either side, because Hubble reads `.properties` as ISO-8859-1 while
PD compares UTF-8 bytes.
Requested change: say that `docker compose up` fails outright when the
generated file is missing, update the two other copies of the old wording, and
add the printable-ASCII requirement to the guarantee list.
##########
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:
🧹 The `ok` line still fires for a failing file once an earlier file has
failed, so the half of my last-round suggestion (`3940228488`) that captured
`before` does not reach the second failure onward.
`before` takes the global `FAIL` at entry (`:37`), which only separates 0
from 1 on the first failing file. After that `FAIL` is already 1, this file's
own failures leave it at 1, and `FAIL == before` holds again.
Reproduced by driving the head copy of `check()` over two files that both
carry `include: "*"` and no `secret-key`:
```
FAIL a/f1.yml: actuator exposure must be exactly
'health,metrics,prometheus', got '*'
FAIL a/f1.yml: auth.secret-key must be present and empty
FAIL a/f2.yml: actuator exposure must be exactly
'health,metrics,prometheus', got '*'
FAIL a/f2.yml: auth.secret-key must be present and empty
ok a/f2.yml
```
Exit status is still 1, so this is output only, but it is the output someone
reads while fixing the drift.
Requested change: track the result per file. `local bad=0` at `:37` in place
of `before`, which then has no reader; `bad=1` beside each `FAIL=1` in this
function; and `if [[ "$bad" -eq 0 ]]; then ... fi` here. Keep the `if` rather
than a bare `[[ ... ]] && echo`: as the function's last command that would
return 1 on a failing file, and `set -euo pipefail` at `:25` would abort the
loop, which is the reason you took the `if` form last time.
##########
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:
⚠️ The remembered 401 still ends the wait when the pass finds no Up store.
This refines what I asked for last round (`3956307914`), which you took in
`ae2a39a5f`.
`:134-136` says one refusing peer no longer ends the wait before the rest of
`PD_REST_LIST` is tried, but `refused` is a single flag: one 401 anywhere in
the pass turns a "nothing Up yet" result into `return 2`, and `:172` makes that
`exit 2`. Neither condition is unusual on its own, and together they are
ordinary during a rolling secret rotation: PD answers `/v1/stores` well before
stores register, so the first pass hits the stale peer, finds the healthy peers
still storeless, and kills the Server instead of retrying.
Reproduced with a mock curl (pd0 returns 401, pd1 returns 200 with one store
in state `Pending`). Both peers were polled, then:
```
[wait-storage] ERROR: PD at pd0:8620 refused the credential (401):
[wait-storage] ERROR: storage wait aborted, see the message above
rc=1
```
`test-wait-storage.sh:269` passes only because its pd1 reports `Up` inside
that same pass.
Requested change: count refusals instead of flagging one, so only a
fleet-wide refusal aborts. In the escaped inner script: `refused=0` and
`peers=0` at `:143`, `peers=\$((peers + 1))` as the first statement of the
loop, `refused=\$((refused + 1))` at `:155`, and here:
```
[ \"\$peers\" -gt 0 ] && [ \"\$refused\" -eq \"\$peers\" ]
&& return 2
```
The `auth-401` case still aborts on pass one under that. `:134-141` and the
test comment at `test-wait-storage.sh:266-268` would need the same correction,
and a case with one refusing peer and no Up store anywhere would pin it.
--
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]
