bitflicker64 commented on code in PR #3253:
URL: https://github.com/apache/hugegraph/pull/3253#discussion_r4217946978
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -34,11 +34,11 @@ fi
BIN=$(abs_path)
TOP="$(cd "$BIN"/../ && pwd)"
-CONF="$TOP/conf"
+CONF="${CONF_OVERRIDE:-$TOP/conf}"
Review Comment:
Important: `-c` only moves rest-server.properties and gremlin-server.yaml.
The stock rest-server.properties has `graphs=./conf/graphs`, and this script
runs `cd "${TOP}"` (line 92) before Java starts. HugeGraphServer, GraphManager
and InitStore read `ServerOptions.GRAPHS` relative to the working directory, so
a conf dir copied to `/etc/hugegraph` and passed with `-c` still loads
`$TOP/conf/graphs/*.properties`, and edits under `/etc/hugegraph/graphs` are
ignored without any message. Please resolve a relative `graphs` value against
`$CONF` when an override is set, or reject a relative `graphs` path with a
clear error when `-c` is used.
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -174,11 +186,11 @@ case "$GC_OPTION" in
-XX:+UnlockDiagnosticVMOptions
-XX:-ZProactive"
;;
*)
- echo "Unrecognized gc option: '$GC_OPTION', default use g1, options
only support 'ZGC' now" >> ${OUTPUT}
+ echo "Unrecognized gc option: '$GC_OPTION', default use g1, options
only support 'ZGC' now" >> "${OUTPUT}"
exit 1
esac
-JVM_OPTIONS="-Dlog4j.configurationFile=${CONF}/log4j2.xml
-Dfastjson.parser.safeMode=true
-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager"
+JVM_OPTIONS="-Dlog4j.configurationFile=${CONF}/log4j2.xml
-Dfastjson.parser.safeMode=true -Dlogging.config=${CONF}/log4j2.xml
-DLOG_PATH=${LOGS} -Dbolt.log.path=${LOGS} -Dlogging.path=${LOGS}
-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager"
Review Comment:
Important: `${CONF}` and `${LOGS}` are placed inside the `JVM_OPTIONS`
string, and the `exec ${JAVA} ... ${JVM_OPTIONS} ${JAVA_OPTIONS}` lines expand
it unquoted, so a path with a space is split into separate arguments. With `-l
'/data/Huge Graph/logs'` the JVM receives `-DLOG_PATH=/data/Huge` followed by
`Graph/logs`, which it treats as the main class, so the service does not start.
This applies to the same lines in start-hugegraph-pd.sh (line 186) and
hugegraph-server.sh (line 158), to `-XX:HeapDumpPath=${LOGS}` and
`-Xlog:gc...file=${LOGS}/gc.log` in JAVA_OPTIONS, and to the unquoted
`${GREMLIN_SERVER_CONF} ${REST_SERVER_CONF}` in hugegraph-server.sh. The PR
description says paths with spaces are handled, and test-cron-quote.sh
exercises such paths. Please build the generated JVM arguments in a bash array
and pass it as `"${JVM_ARGS[@]}"`, or reject override paths that contain
whitespace with a clear error.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/test-cron-quote.sh:
##########
@@ -0,0 +1,292 @@
+#!/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.
+#
+# Local regression tests for the cron serialization helpers in util.sh:
+# shell_quote, reject_unsafe_path, cron_quote,
+# crontab_append, crontab_remove.
+# No cluster, Java, or real crontab needed: `crontab` is replaced by a stub
that
+# reads/writes a temp file, so the invoking user's crontab is not touched.
+# Run: bash test-cron-quote.sh
+
+set -u
Review Comment:
Important: Neither new test script runs in any build. `git grep` finds no
reference to test-canonicalize.sh or test-cron-quote.sh in a workflow, pom or
other script, which is how the failing nocreate case in test-canonicalize.sh
got past CI. Both files also sit in `src/assembly/static/bin`, and
hugegraph-dist's assembly.xml copies `${assembly.static.dir}/bin` with
`<include>*</include>` and mode 755, so every Server release tarball would ship
these two test scripts in its `bin/` next to start-hugegraph.sh.
test-canonicalize.sh also writes a fixed `/tmp/_cz_err` file. Please move the
scripts out of the assembly directory (for example to a `src/test/shell`
folder), run them from a CI workflow, and use a file under the script's own
`mktemp -d` directory for the captured stderr.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/monitor-hugegraph.sh:
##########
@@ -25,35 +25,42 @@ function abs_path() {
echo "$( cd -P "$( dirname "$SOURCE" )" && pwd )"
}
-BIN=`abs_path`
-TOP="$(cd $BIN/../ && pwd)"
+BIN="$(abs_path)"
+TOP="$(cd "$BIN"/../ && pwd)"
-. $BIN/util.sh
+. "$BIN"/util.sh
-SERVER_URL=`read_property "$TOP/conf/rest-server.properties" "restserver.url"`
+CONF="${CONF_OVERRIDE:-$TOP/conf}"
+LOGS="${LOGS_OVERRIDE:-$TOP/logs}"
+
+SERVER_URL=`read_property "$CONF/rest-server.properties" "restserver.url"`
DETECT_URL="$SERVER_URL/versions"
PROC_NAME="HugeGraphServer"
-LOG_DIR="$TOP/logs"
+LOG_DIR="$LOGS"
MONITOR_LOG="$LOG_DIR/monitor.log"
-if [ ! -d $LOG_DIR ]; then
- mkdir $LOG_DIR
+if [ ! -d "$LOG_DIR" ]; then
+ mkdir -p "$LOG_DIR"
fi
function record_monitor_log() {
- echo `date '+%Y-%m-%d %H:%M:%S'`, $1 >> $MONITOR_LOG
+ echo `date '+%Y-%m-%d %H:%M:%S'`, $1 >> "$MONITOR_LOG"
}
function restart_server() {
local stop_old=$1
if [ "$stop_old" == "true" ]; then
- # Don't remove monitor
- $BIN/stop-hugegraph.sh false
+ # Don't remove monitor; stop the pid file the caller used.
+ "$BIN"/stop-hugegraph.sh ${PID_FILE_OVERRIDE:+-i "$PID_FILE_OVERRIDE"}
false
Review Comment:
Important: The comment says this stop keeps the monitor, but
stop-hugegraph.sh only reads options with `getopts "i:m:"`, and getopts stops
at the positional `false`. With `-i /x/pid false` it sets the PID file and
leaves `CLOSE_MONITOR=true`, so this call runs stop-monitor.sh and removes the
cron job that is doing the restart. After one health-check restart the server
is no longer monitored. The bug exists at the base too, but this PR edits this
line and its comment. Please pass `-m false` instead of the bare `false`.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/test-canonicalize.sh:
##########
@@ -0,0 +1,143 @@
+#!/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.
+#
+# Local unit tests for canonicalize_dir / canonicalize_file in util.sh.
+# No cluster, Java, or CM needed. Run: bash test-canonicalize.sh
+
+set -u
+
+UTIL="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/util.sh"
+
+# We only want the helpers; source util.sh in a guarded way. util.sh has no
+# top-level side effects for these functions, so a plain source is fine.
+# shellcheck disable=SC1090
+. "$UTIL"
+
+PASS=0
+FAIL=0
+TMP="$(mktemp -d)"
+trap 'rm -rf "$TMP"' EXIT
+
+ok() { echo "PASS: $1"; PASS=$((PASS+1)); }
+bad() { echo "FAIL: $1"; FAIL=$((FAIL+1)); }
+
+# Run a function in a subshell so its `exit 1` paths don't kill the harness.
+run_capture() { # usage: run_capture FUNC ARG ; sets OUT, RC
+ OUT="$("$@" 2>/tmp/_cz_err)"; RC=$?
+ ERR="$(cat /tmp/_cz_err)"
+}
+
+echo "== canonicalize_dir =="
+
+# 1. empty input -> empty output, rc 0
+run_capture canonicalize_dir ""
+[ $RC -eq 0 ] && [ -z "$OUT" ] && ok "dir: empty input returns empty, rc=0" \
+ || bad "dir: empty input (rc=$RC out='$OUT')"
+
+# 2. absolute existing dir -> same absolute path
+run_capture canonicalize_dir "$TMP"
+[ "$OUT" = "$TMP" ] && ok "dir: absolute existing path echoes itself" \
+ || bad "dir: absolute path (out='$OUT' want='$TMP')"
+
+# 3. relative dir -> absolute, and dir is created
+( cd "$TMP" && run_capture canonicalize_dir "sub/rel" \
+ && [ "$OUT" = "$TMP/sub/rel" ] && [ -d "$TMP/sub/rel" ] ) \
+ && ok "dir: relative path canonicalized + created" \
+ || bad "dir: relative path (out='$OUT')"
+
+# 4. nested non-existent absolute -> created + echoed
+run_capture canonicalize_dir "$TMP/a/b/c"
+[ "$OUT" = "$TMP/a/b/c" ] && [ -d "$TMP/a/b/c" ] \
+ && ok "dir: nested absolute created" || bad "dir: nested (out='$OUT')"
+
+# 5. unresolvable (parent not writable) -> error + exit 1
+RO="$TMP/ro"; mkdir -p "$RO"; chmod 500 "$RO"
+run_capture canonicalize_dir "$RO/child"
+if [ "$(id -u)" -eq 0 ]; then
+ echo "SKIP: dir unwritable test (running as root bypasses perms)"
+else
+ [ $RC -eq 1 ] && echo "$ERR" | grep -q "cannot resolve path" \
+ && ok "dir: unwritable parent -> exit 1 + error msg" \
+ || bad "dir: unwritable parent (rc=$RC err='$ERR')"
+fi
+chmod 700 "$RO"
+
+echo "== canonicalize_file =="
+
+# 6. empty input -> empty, rc 0
+run_capture canonicalize_file ""
+[ $RC -eq 0 ] && [ -z "$OUT" ] && ok "file: empty input returns empty, rc=0" \
+ || bad "file: empty input (rc=$RC out='$OUT')"
+
+# 7. absolute file path -> abs parent + filename, parent created, file NOT
created
+run_capture canonicalize_file "$TMP/pids/server.pid"
+[ "$OUT" = "$TMP/pids/server.pid" ] && [ -d "$TMP/pids" ] && [ ! -e
"$TMP/pids/server.pid" ] \
+ && ok "file: abs path resolves, parent created, file not created" \
+ || bad "file: abs path (out='$OUT')"
+
+# 8. relative file path -> absolute
+( cd "$TMP" && run_capture canonicalize_file "relpids/x.pid" \
+ && [ "$OUT" = "$TMP/relpids/x.pid" ] && [ -d "$TMP/relpids" ] ) \
+ && ok "file: relative path canonicalized" || bad "file: relative
(out='$OUT')"
+
+# 9. unresolvable parent -> error + exit 1
+# Use a NEVER-created subpath under the read-only dir so mkdir -p must fail.
+chmod 500 "$RO"
+run_capture canonicalize_file "$RO/fresh_sub/x.pid"
+if [ "$(id -u)" -eq 0 ]; then
+ echo "SKIP: file unwritable test (running as root)"
+else
+ [ $RC -eq 1 ] && echo "$ERR" | grep -q "cannot resolve path" \
+ && ok "file: unwritable parent -> exit 1 + error msg" \
+ || bad "file: unwritable parent (rc=$RC err='$ERR')"
+fi
+chmod 700 "$RO"
+
+echo "== canonicalize_file nocreate (stop scripts) =="
+
+# 10. existing parent -> absolute path, nothing created
+mkdir -p "$TMP/existing"
+run_capture canonicalize_file "$TMP/existing/s.pid" nocreate
+[ $RC -eq 0 ] && [ "$OUT" = "$TMP/existing/s.pid" ] && [ ! -e
"$TMP/existing/s.pid" ] \
+ && ok "nocreate: existing parent resolves, file not created" \
+ || bad "nocreate: existing parent (rc=$RC out='$OUT')"
+
+# 11. relative path with existing parent -> absolute
+( cd "$TMP/existing" && run_capture canonicalize_file "r.pid" nocreate \
+ && [ "$OUT" = "$TMP/existing/r.pid" ] ) \
+ && ok "nocreate: relative path resolves" || bad "nocreate: relative
(out='$OUT')"
+
+# 12. missing parent -> fails with the same error, and the parent is NOT
created
+run_capture canonicalize_file "$TMP/missing/deeper/s.pid" nocreate
Review Comment:
Important: This test fails against the code in this PR. `canonicalize_file`
in util.sh takes one argument and always runs `mkdir -p "$dir"`; there is no
`nocreate` mode, so the second argument is ignored and the missing parent is
created. Running `bash test-canonicalize.sh` at e272e27 prints `FAIL: nocreate:
missing parent (rc=0 err='' created=yes)` and `PASS=13 FAIL=1`. The PR
description also says the stop scripts call `canonicalize_file ... nocreate`,
but stop-hugegraph.sh, stop-hugegraph-pd.sh and stop-hugegraph-store.sh do not
canonicalize `-i` at all. Please either add the `nocreate` mode to all three
util.sh copies and call it from the three stop scripts, or remove the nocreate
cases (tests 11 to 14) and correct the description, so the suite passes.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -39,30 +39,60 @@ function abs_path() {
BIN=$(abs_path)
TOP="$(cd "$BIN"/../ && pwd)"
-CONF="$TOP/conf"
-LOGS="$TOP/logs"
SCRIPTS="$TOP/scripts"
-PID_FILE="$BIN/pid"
. "$BIN"/util.sh
# Note: keep ':' in the end of the string to indicate the option needs a value
-while getopts "d:g:m:p:s:j:t:y:" arg; do
- case ${arg} in
- d) DAEMON="$OPTARG" ;;
- g) GC_OPTION="$OPTARG" ;;
- m) OPEN_MONITOR="$OPTARG" ;;
- p) PRELOAD="$OPTARG" ;;
- s) OPEN_SECURITY_CHECK="$OPTARG" ;;
- j) USER_OPTION="$OPTARG" ;;
- t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
- # Telemetry is used to collect metrics, traces and logs
- y) OPEN_TELEMETRY="$OPTARG" ;;
- # Note: update usage info when the params changed
- ?) exit_with_usage_help ;;
- esac
+while getopts "c:d:g:i:j:l:m:o:p:s:t:y:" arg; do
+ case ${arg} in
+ c) reject_unsafe_path "$OPTARG" "-c conf dir" crlf || exit 1
+ CONF_OVERRIDE="$OPTARG" ;;
+ d) DAEMON="$OPTARG" ;;
+ g) GC_OPTION="$OPTARG" ;;
+ i) reject_unsafe_path "$OPTARG" "-i pid file" crlf || exit 1
+ PID_FILE_OVERRIDE="$OPTARG" ;;
+ j) USER_OPTION="$OPTARG" ;;
+ l) reject_unsafe_path "$OPTARG" "-l logs dir" crlf || exit 1
+ LOGS_OVERRIDE="$OPTARG" ;;
+ m) OPEN_MONITOR="$OPTARG" ;;
+ o) reject_unsafe_path "$OPTARG" "-o plugins dir" crlf || exit 1
+ PLUGINS_OVERRIDE="$OPTARG" ;;
+ p) PRELOAD="$OPTARG" ;;
+ s) OPEN_SECURITY_CHECK="$OPTARG" ;;
+ t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
+ # Telemetry is used to collect metrics, traces and logs
+ y) OPEN_TELEMETRY="$OPTARG" ;;
+ # Note: update usage info when the params changed
+ ?) exit_with_usage_help ;;
+ esac
done
+# Canonicalize relative path overrides to absolute paths.
+CONF_OVERRIDE="$(canonicalize_dir "$CONF_OVERRIDE")" || exit 1
Review Comment:
Minor: `canonicalize_dir` runs `mkdir -p` on every override, including the
conf dir. A mistyped `-c /etc/hugegrap` creates an empty directory and the
script continues: `read_property` returns empty for the missing
rest-server.properties, and the failure shows up later as a port check or Java
config error instead of a missing conf dir. PD and Store do the same with `-c`.
Please let the conf override require an existing directory (create only the
logs, plugins and PID parent directories) and print a clear error when it does
not exist.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/stop-monitor.sh:
##########
@@ -25,11 +25,15 @@ function abs_path() {
echo "$( cd -P "$( dirname "$SOURCE" )" && pwd )"
}
-BIN=`abs_path`
-TOP="$(cd $BIN/../ && pwd)"
+BIN="$(abs_path)"
+TOP="$(cd "$BIN"/../ && pwd)"
-. $BIN/util.sh
+. "$BIN"/util.sh
-CRONTAB_JOB="$TOP/bin/monitor-hugegraph.sh"
+# Must equal the serialized form start-monitor.sh saved (one quoted shell
word),
+# otherwise grep -F cannot match a path containing an apostrophe or space and
the
+# job could never be removed. shell_quote (not cron_quote) so removal is not
+# blocked by validation.
+CRONTAB_JOB="$(shell_quote "$TOP/bin/monitor-hugegraph.sh")"
Review Comment:
Important: The removal key now always has single quotes around it, but the
previous start-monitor.sh wrote the unquoted form `... &&
$TOP/bin/monitor-hugegraph.sh`. After an in-place upgrade with an existing
monitor job, `grep -F` does not match, `crontab_remove` returns 0,
stop-hugegraph.sh prints that the monitor was closed, and the old cron line can
restart the server within a minute. The new start-monitor.sh also does not
detect the old line and adds a second job. Please match both the quoted and
unquoted forms when removing (and when checking for duplicates in
crontab_append), and add an old-format-to-new-format case to test-cron-quote.sh.
--
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]
