imbajin commented on code in PR #3119: URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3719844087
########## hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh: ########## @@ -0,0 +1,43 @@ +#!/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. + +set -euo pipefail + +entrypoint="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/docker-entrypoint.sh" +test_dir="$(mktemp -d)" +trap 'rm -rf "${test_dir}"' EXIT + +eval "$(awk ' + /^set_prop\(\) \{/ { capture = 1 } + capture { print } + capture && /^\}$/ { exit } +' "${entrypoint}")" + +assert_replaced() { + local separator="$1" + local file="${test_dir}/config-${separator// /space}" + + printf 'init_store.enabled%sfalse\n' "${separator}" > "${file}" + set_prop "init_store.enabled" "true" "${file}" Review Comment: 🧹 The added test executes GNU-only `sed -ri` through `set_prop`; on macOS/BSD sed it aborts before any assertion (`sed: ... invalid command code ...`). The previous GNU-sed guard is absent in this head. Use a portable edit command or declare and enforce a Linux-only test contract so the new regression suite is runnable on supported development hosts. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java: ########## @@ -380,10 +387,29 @@ public void initAdminUserIfNeeded(String password) { user.create(new Date()); user.avatar("/image.png"); try { - this.metaManager.createUser(user); + try { + this.metaManager.createUser(user); + } catch (Exception e) { + // Judged by re-reading rather than by matching the message: + // benign only if the admin actually exists, from an earlier + // startup or from a concurrent server that won the race + HugeUser existing; + try { + existing = this.metaManager.findUser(user.name()); + } catch (Exception probe) { + e.addSuppressed(probe); + throw e; + } + if (existing == null) { + throw e; + } + LOG.info("The built-in admin user already exists, " + + "skip creating it"); + } this.metaManager.initDefaultGraphSpace(); } catch (Exception e) { Review Comment: ‼️ This turns every `usePD=true` startup into a fail-fast admin bootstrap. `loadMetaFromPD()` calls `initAdminUserIfNeeded()` unconditionally at line 358, including auth-disabled, remote-auth, and custom-auth configurations that `InitStore.checkAdminBootstrapReachable()` explicitly exempts; before this change the same create/default-graph failures were logged and startup continued. A PD write or permission failure in those deployments now prevents startup for an unrelated built-in admin. Please make fatal propagation conditional on the local built-in-auth/disabled-init case, or separate default graph-space initialization from admin bootstrap, and add caller-level tests for no-auth, remote, and custom authenticators. ########## hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/cmd/InitStore.java: ########## @@ -81,6 +128,150 @@ public static void main(String[] args) throws Exception { } HugeFactory.shutdown(30L, true); } + + recordInitComplete(); + } + + private static String configuredInitCompleteMarker() { + String marker = System.getProperty(INIT_COMPLETE_MARKER, + System.getenv(INIT_COMPLETE_MARKER_ENV)); + return marker == null || marker.isEmpty() ? null : marker; + } + + /** + * The configured marker path, or null when none is configured or the + * file does not exist yet. Consulted only after the disabled-path check, + * so an existing marker can never bypass the fail-closed validation. + */ + private static String presentInitCompleteMarker() { + String marker = configuredInitCompleteMarker(); + if (marker == null) { + return null; + } + Path path = Paths.get(marker); + if (Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) { + return marker; + } + if (Files.exists(path, LinkOption.NOFOLLOW_LINKS)) { + throw invalidInitCompleteMarker(path); + } + return null; + } + + /** + * Only this process knows whether it initialized anything. The Docker + * entrypoint used to decide from its environment variable alone, so a + * mounted config that disabled init-store was still recorded as done and a + * later re-enable skipped for good. Reached only on the enabled path, and + * only after initialization succeeded. + */ + private static void recordInitComplete() throws IOException { + String marker = configuredInitCompleteMarker(); + if (marker == null) { + return; + } + Path path = Paths.get(marker); + Path dir = path.toAbsolutePath().getParent(); + if (dir != null) { + Files.createDirectories(dir); + } + try { + Files.createFile(path); + } catch (FileAlreadyExistsException e) { + // A concurrent container finishing its own successful init has + // already recorded it, which is the same outcome + if (!Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) { + throw invalidInitCompleteMarker(path); + } + } + LOG.info("Recorded init-store completion at '{}'", path); + } + + private static IllegalStateException invalidInitCompleteMarker(Path path) { + return new IllegalStateException(String.format( + "Init-store completion marker '%s' must be a regular file", + path)); + } + + /** + * Skipping leaves the built-in admin to GraphManager.initAdminUserIfNeeded() + * on the PD startup path, which writes it to PD metadata. Only an HStore + * auth graph reads that metadata back, so every other local built-in-auth + * configuration would start a server nobody can log in to. Remote auth and + * custom authenticators keep their identities elsewhere and are exempt. + */ + private static void checkAdminBootstrapReachable(HugeConfig conf, + String restConf) { + if (!requiresLocalBuiltinAdmin(conf)) { + return; + } + if (!conf.get(ServerOptions.USE_PD)) { + throw unreachableAdmin(restConf, ServerOptions.USE_PD.name() + + " is false"); + } + + String name = conf.get(ServerOptions.AUTH_GRAPH_STORE); + String path = ConfigUtil.scanGraphsDir( + conf.get(ServerOptions.GRAPHS)).get(name); + if (path == null) { + throw unreachableAdmin(restConf, "auth graph '" + name + + "' has no local configuration"); + } + String backend = new HugeConfig(path).get(CoreOptions.BACKEND); + if (!"hstore".equals(backend)) { Review Comment: ⚠️ This skip validation compares the backend name case-sensitively, while normal backend selection lowercases it in `BackendProviderFactory` (`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendProviderFactory.java:57`). A valid auth-graph configuration using `backend=HSTORE` is therefore accepted by runtime but rejected here whenever `init_store.enabled=false`. Normalize before comparing and add a mixed-case regression. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -30,10 +31,10 @@ set_prop() { local esc_key esc_val esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g') - esc_val=$(printf '%s' "$val" | sed -e 's/[&|\\]/\\&/g') + esc_val=$(printf '%s' "$val" | sed -e 's/[&|\\~]/\\&/g') - if grep -qE "^[[:space:]]*${esc_key}[[:space:]]*=" "${file}"; then - sed -ri "s|^([[:space:]]*${esc_key}[[:space:]]*=).*|\\1${esc_val}|" "${file}" + if grep -qE "^[[:space:]]*${esc_key}([[:space:]]*[:=]|[[:space:]]+)" "${file}"; then + sed -ri "s~^[[:space:]]*${esc_key}([[:space:]]*[:=]|[[:space:]]+).*~${key}=${esc_val}~" "${file}" Review Comment: ⚠️ The separator-aware replacement still rewrites every matching definition rather than collapsing them. If a mounted file already contains two `init_store.enabled` entries, this produces two `init_store.enabled=<new>` lines; `InitStoreConfigTest.testDuplicateDefinitionFailsToLoad()` documents that Commons Configuration then exposes a list and rejects the scalar Boolean, so this env override can make startup fail. Keep one canonical definition and add coverage for duplicate existing keys, including `:` and whitespace forms. -- 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]
