imbajin commented on code in PR #3119:
URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3661390626
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -20,23 +20,74 @@ set -euo pipefail
DOCKER_FOLDER="./docker"
INIT_FLAG_FILE="init_complete"
GRAPH_CONF="./conf/graphs/hugegraph.properties"
+REST_SERVER_CONF="./conf/rest-server.properties"
mkdir -p "${DOCKER_FOLDER}"
log() { echo "[hugegraph-server-entrypoint] $*"; }
+# Sets a property to exactly one canonical `key=value` line. Existing
+# definitions are matched on any separator a properties file allows (`=`, `:`
+# or whitespace) and collapsed into that single line, because leaving a second
+# definition behind would make the parser expose the key as a list and a scalar
+# read of it would then fail. Comment lines are left alone. Matching is
literal,
+# so no regex escaping of the key or value is needed.
set_prop() {
local key="$1" val="$2" file="$3"
- local esc_key esc_val
+ SET_PROP_KEY="$key" SET_PROP_VAL="$val" awk '
+ BEGIN { key = ENVIRON["SET_PROP_KEY"]; val = ENVIRON["SET_PROP_VAL"] }
+ {
+ line = $0
+ probe = line
+ sub(/^[[:space:]]+/, "", probe)
+ if (index(probe, key) == 1) {
+ rest = substr(probe, length(key) + 1)
+ if (rest ~ /^[[:space:]]*[=:]/ || rest ~ /^[[:space:]]+/) {
+ if (!done) { print key "=" val; done = 1 }
+ next
+ }
+ }
+ print line
+ }
+ END { if (!done) print key "=" val }
+ ' "${file}" > "${file}.tmp" && mv "${file}.tmp" "${file}"
Review Comment:
โ ๏ธ Replacing the config with `mv` breaks the mounted-config path this change
supports and also discards file metadata. A single-file Docker/Kubernetes bind
mount cannot normally be replaced by rename (`Device or resource busy`), so
`set -e` aborts startup; for an ordinary `0600` file, the temporary file is
created under the process umask and can become `0644`, exposing the
`auth.admin_pa` written below. Please preserve the existing inode and
protection metadata while replacing the contents, and add a writable bind-mount
test plus a mode-preservation test.
##########
hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh:
##########
@@ -0,0 +1,386 @@
+#!/bin/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.
+#
+# Smoke tests for docker-entrypoint.sh init-store lifecycle.
+#
+# The entrypoint is run against a throwaway install tree whose ./bin scripts
are
+# stubs recording their own invocation, so the tests assert on which scripts
ran
+# and on the resulting config, without needing a JVM, a backend or Docker.
+#
+# Usage: hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh
+
+set -uo pipefail
+
+SELF_DIR="$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+ENTRYPOINT="${SELF_DIR}/../docker-entrypoint.sh"
+
+PASS=0
+FAIL=0
+SKIP=0
+
+# docker-entrypoint.sh rewrites an existing property with GNU `sed -ri`, which
Review Comment:
๐งน This guard is now stale: `set_prop()` was rewritten with `awk`, but the
test still claims it requires GNU `sed -ri` and skips the override and
false-to-true cases on macOS. The local exact-head run consequently skipped
both newly relevant cases. Please remove the obsolete GNU-sed guard so these
portable tests run on every supported development platform.
##########
hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/cmd/InitStore.java:
##########
@@ -69,11 +69,57 @@ public static void main(String[] args) throws Exception {
String restConf = args[0];
- RegisterUtil.registerBackends();
- RegisterUtil.registerPlugins();
+ /*
+ * Only the server options are needed to read the gate below. Backend
+ * and plugin registration is deferred to the enabled path:
+ * registerPlugins() invokes every discovered plugin's register() and
+ * propagates their failures, which must not happen on a path that is
+ * documented to be a no-op.
+ */
RegisterUtil.registerServer();
HugeConfig restServerConfig = new HugeConfig(restConf);
+
+ /*
+ * Distributed deployments (PD/HStore) let the storage side own the
+ * metadata, so there is nothing for init-store to do. The option
+ * defaults to true, keeping standalone/tarball installs on the full
+ * init path.
+ *
+ * The loop below already skips hstore backends, so what this gate
+ * additionally avoids is scanning the graphs directory (which must
+ * otherwise exist), and, when auth is configured, opening the auth
+ * graph store in initAdminUserIfNeeded(). On Kubernetes that ran on
+ * every Server pod restart, since the entrypoint's init flag file does
+ * not survive one.
+ *
+ * NOTE: skipping also means the built-in admin account is not created
+ * here. The only other code path that creates it is
+ * GraphManager.initAdminUserIfNeeded(), reached from loadMetaFromPD(),
+ * which runs only when 'usePD' is true. Enabling auth with this option
+ * false and 'usePD' false therefore yields a server that enforces
+ * authentication with no account to authenticate against.
+ */
+ if (!restServerConfig.get(ServerOptions.INIT_STORE_ENABLED)) {
+ LOG.warn("Skipping init-store: '{}' is false in '{}'. Local " +
+ "backend and admin initialization are not performed.",
+ ServerOptions.INIT_STORE_ENABLED.name(), restConf);
+ if (!restServerConfig.get(ServerOptions.AUTHENTICATOR).isEmpty() &&
+ !restServerConfig.get(ServerOptions.USE_PD)) {
+ LOG.warn("'{}' is set but '{}' is false: no component will " +
+ "create the built-in admin account. Set '{}' to true,
" +
+ "or leave '{}' enabled so it can create the account.",
+ ServerOptions.AUTHENTICATOR.name(),
+ ServerOptions.USE_PD.name(),
+ ServerOptions.USE_PD.name(),
+ ServerOptions.INIT_STORE_ENABLED.name());
+ }
+ return;
Review Comment:
โผ๏ธ This invalid non-Docker configuration still exits successfully. With
`init_store.enabled=false`, a non-empty authenticator, and `usePD=false`, the
code itself establishes that no component will create the built-in admin, but
`init-store.sh` only receives this warning and returns zero. Tarball, init-job,
or other callers can therefore continue with an auth-enabled server that nobody
can access. Please fail this combination with a non-zero result, matching the
Docker entrypoint's rejection, and add a direct CLI regression that verifies
the failure status.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -54,19 +105,89 @@ migrate_env "PD_PEERS" "HG_SERVER_PD_PEERS"
# โโ Map env โ properties file โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ
[[ -n "${HG_SERVER_BACKEND:-}" ]] && set_prop "backend"
"${HG_SERVER_BACKEND}" "${GRAPH_CONF}"
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] && set_prop "pd.peers"
"${HG_SERVER_PD_PEERS}" "${GRAPH_CONF}"
+if [[ -n "${HG_SERVER_INIT_STORE_ENABLED:-}" ]]; then
+ # Canonicalize before writing, so the property file only ever holds `true`
+ # or `false` and cannot be read differently by the shell and the server
+ if ! HG_SERVER_INIT_STORE_ENABLED=$(to_bool
"${HG_SERVER_INIT_STORE_ENABLED}"); then
+ log "ERROR: HG_SERVER_INIT_STORE_ENABLED must be a boolean, got
'${HG_SERVER_INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+ set_prop "init_store.enabled" "${HG_SERVER_INIT_STORE_ENABLED}"
"${REST_SERVER_CONF}"
+fi
# โโ Build wait-storage env โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ
WAIT_ENV=()
[[ -n "${HG_SERVER_BACKEND:-}" ]] &&
WAIT_ENV+=("hugegraph.backend=${HG_SERVER_BACKEND}")
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] &&
WAIT_ENV+=("hugegraph.pd.peers=${HG_SERVER_PD_PEERS}")
-# โโ Init store (once) โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ
-if [[ ! -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ]]; then
+wait_storage() {
if (( ${#WAIT_ENV[@]} > 0 )); then
env "${WAIT_ENV[@]}" ./bin/wait-storage.sh
else
./bin/wait-storage.sh
fi
+}
+
+# โโ Init store (once) โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโ
+# With `init_store.enabled=false` (distributed PD/HStore) init-store is a
no-op:
+# storage owns the metadata and the admin account is created on server startup
+# from `auth.admin_pa`. A requested PASSWORD is therefore written to that
+# property rather than piped into init-store.sh, where it would be read and
+# discarded without creating the account.
+#
+# The value is read back from the config file rather than from the env var, so
+# that a rest-server.properties mounted with the property already set behaves
+# the same as `HG_SERVER_INIT_STORE_ENABLED` (the env mapping above has already
+# been applied, so env still wins).
+INIT_STORE_ENABLED=$(get_prop "init_store.enabled" "${REST_SERVER_CONF}")
+if [[ -n "${INIT_STORE_ENABLED}" ]]; then
+ if ! INIT_STORE_ENABLED=$(to_bool "${INIT_STORE_ENABLED}"); then
+ log "ERROR: init_store.enabled in ${REST_SERVER_CONF} must be a
boolean," \
+ "got '${INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+fi
+if [[ "${INIT_STORE_ENABLED:-true}" == "false" ]]; then
+ log "init-store disabled, skipping local backend/admin init"
+
+ # With init-store skipped, nothing creates the built-in admin account
+ # unless the server takes the PD metadata path, which it only does when
+ # `usePD=true`. Enabling auth without that combination starts a server
+ # that enforces authentication while no account exists, so refuse it here
+ # rather than fail every request later.
+ AUTH_REQUESTED=""
+ [[ -n "${PASSWORD:-}" ]] && AUTH_REQUESTED=1
+ [[ -n "$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")" ]] &&
AUTH_REQUESTED=1
+ if [[ -n "${AUTH_REQUESTED}" ]]; then
+ USE_PD=$(to_bool "$(get_prop "usePD" "${REST_SERVER_CONF}")"
2>/dev/null || echo "false")
+ if [[ "${USE_PD}" != "true" ]]; then
+ log "ERROR: auth is enabled and init_store.enabled=false, but
usePD is not true."
+ log "ERROR: With init-store skipped the admin account is only
created on the PD"
+ log "ERROR: metadata path, so this combination would start a
server that nobody"
+ log "ERROR: can authenticate against."
+ log "ERROR: Set usePD=true in ${REST_SERVER_CONF}, or leave
init-store enabled"
+ log "ERROR: so that it can create the admin account locally."
+ exit 1
+ fi
+ fi
+
+ # Still wait: the server needs the storage side reachable at startup even
+ # though nothing is initialized here
+ wait_storage
+
+ if [[ -n "${PASSWORD:-}" ]]; then
+ log "enabling auth mode, admin password applied via auth.admin_pa"
+ ./bin/enable-auth.sh
Review Comment:
โ ๏ธ `PASSWORD` is not safe with an already-auth-enabled mounted config on
this skip path. `enable-auth.sh` blindly appends `auth.authenticator` and
`auth.graph_store` on its first run, so an accepted `init_store.enabled=false`
+ `usePD=true` config that already contains those scalar keys becomes
duplicated; `HugeConfig` then exposes a list where callers expect a `String`
and server startup fails. Please make auth enablement idempotent or
canonicalize these keys, and add a mounted-auth + `PASSWORD` smoke test that
loads the resulting file through `HugeConfig`.
--
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]
