bitflicker64 commented on code in PR #3119:
URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3662038150
##########
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:
Agreed and fixed in 99d7de87. `InitStore` now throws for that combination,
so `init-store.sh` exits non-zero and tarball or init-job callers see the
failure.
One exemption: `auth.remote_url`. The auth manager is then an RPC client and
`requireInitAdminUser()` already gates on `StandardAuthManager.isLocal()`, so
no local admin is expected and failing there would be a false positive. The
entrypoint guard was missing the same exemption and now has it.
CLI regressions added for both the non-zero failure and the remote-auth
exemption.
##########
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:
Correct on both counts, fixed in 99d7de87. `set_prop` now truncates and
rewrites in place (`cat tmp > file`) instead of renaming, so the inode,
ownership and mode survive and a single-file bind mount is not replaced. The
scratch file is created under `umask 077`, since it holds `auth.admin_pa`.
Tests assert the inode is unchanged, that a 0600 config is still 0600
afterwards, and that no scratch file is left behind.
##########
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:
Confirmed and fixed in 99d7de87. `enable-auth.sh` now runs only when
`auth.authenticator` is not already set, and `auth.authenticator` and
`auth.graph_store` are collapsed to single definitions afterwards.
Same failure mode as the `set_prop` separator issue, reached by a different
route: the duplicated key fails the scalar type check while the config is still
loading, so `new HugeConfig(...)` throws rather than merely reading oddly.
Added a mounted-auth plus `PASSWORD` case asserting each key ends up defined
once.
##########
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:
Right, that guard was stale once `set_prop` moved to awk. Removed in
99d7de87.
Both previously skipped cases now run on macOS, and the local run is 74
assertions across 24 cases with no skips.
--
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]
