bitflicker64 commented on code in PR #2945: URL: https://github.com/apache/hugegraph/pull/2945#discussion_r4303528742
########## hugegraph-server/hugegraph-dist/src/assembly/static/conf/rest-server.properties: ########## @@ -12,10 +12,16 @@ batch.max_write_ratio=80 batch.max_write_threads=0 # configuration of arthas +# Legacy aliases: arthas.telnetPort, arthas.httpPort, arthas.disabledCommands. +# The snake_case key takes precedence when both forms are configured. +# Different values for both forms produce a warning when the agent is activated. arthas.telnet_port=8562 arthas.http_port=8561 +# Defaults to local access. External binding (e.g. 0.0.0.0) exposes diagnostic commands; +# enable Arthas native authentication and restrict network access before exposing it. Review Comment: Minor: this asks administrators to enable Arthas native authentication, but neither this file nor docs/server-runtime.md says how to do that for the `PUT /arthas` agent. `ArthasAPI.arthasConfig` passes only the ports, the address and the disabled commands, and the credentials section of the docs covers only `bin/arthas.sh`. A fixture that attaches with the same config map honored `-Darthas.username=...` and `-Darthas.password=...` on the target JVM. Without them, the password Arthas generates for a wildcard bind goes only to the Server stdout and the Arthas log, which an administrator with only REST access may not be able to read. Keeping loopback as the default is right, so this only needs docs and a hint. Please document the supported way, noting that JVM options appear in process listings, and log a warning in `arthasConfig` when `arthas.ip` is not loopback and no Arthas password is set. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/arthas.sh: ########## @@ -0,0 +1,201 @@ +#!/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. +# +# Offline attachment to a running Server or Store using the bundled agent. +set -euo pipefail + +fail() { echo "Error: $*" >&2; exit 1; } +usage() { + echo "Usage: $0 [--pid PID] [--telnet-port PORT] [--target-ip ADDRESS] [--command COMMAND]" + echo " [--username USER] [--password PASSWORD]" + echo "Run as the service OS user; defaults to bin/pid and loopback port 3658." + echo "New agents disable HTTP/MCP; use the existing telnet port for an active agent." + echo "Exiting this helper stops Arthas, including after --command or quit; the JVM keeps running." +} + +BIN=$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd) +TOP=$(cd -P "$BIN/.." && pwd) +pid="" +port=3658 +target_ip=127.0.0.1 +username="" +password="" +command="" +boot_args=() +while [ "$#" -gt 0 ]; do + case "$1" in + --pid|--telnet-port|--target-ip|--command|--username|--password) + [ "$#" -ge 2 ] || fail "Missing value for $1" + case "$1" in + --pid) pid=$2 ;; + --telnet-port) port=$2 ;; + --target-ip) target_ip=$2 ;; + --command) command=$2 ;; + --username) username=$2 ;; + --password) password=$2 ;; + esac + shift 2 ;; + -h|--help) usage; exit 0 ;; + *) usage >&2; fail "Unknown option: $1" ;; + esac +done +# Native batch files contain one command per line. +for credential in "$username" "$password"; do + [[ "$credential" != *$'\n'* && "$credential" != *$'\r'* ]] || fail "Credentials must be single-line values" +done +if [ -z "$pid" ]; then + [ -r "$BIN/pid" ] || fail "No bin/pid; start the service or supply --pid PID" + pid=$(cat "$BIN/pid") +fi +[[ "$pid" =~ ^[1-9][0-9]{0,9}$ ]] || fail "PID must be a positive integer" +[[ "$port" =~ ^[1-9][0-9]{0,4}$ ]] && [ "$port" -le 65535 ] || fail "Invalid telnet port" +[ -t 0 ] || [ -n "$command" ] || fail "Interactive use needs a terminal; use --command COMMAND" +case "$target_ip" in + [Ll][Oo][Cc][Aa][Ll][Hh][Oo][Ss][Tt]) target_ip=127.0.0.1 ;; +esac +JAVA=${JAVA_HOME:+$JAVA_HOME/bin/}java +ARTHAS="$TOP/lib/arthas" +[ -f "$ARTHAS/arthas-boot.jar" ] || fail "Bundled Arthas files are missing from lib/arthas" +# Keep the listener within this diagnosis session. Verify its PID before stopping +# it, since an occupied port must never stop a different application's agent. +state_dir=$(mktemp -d) +client_pid="" +attaching=true +connect_ip=$target_ip +quote_argument() { + local value=${1//\'/\'\\\'\'} + printf "'%s'" "$value" +} +write_batch() { + local file=$1 + local commands=$2 + { + if [ -n "$password" ]; then + printf 'auth --username %s %s\n' "$(quote_argument "${username:-arthas}")" "$(quote_argument "$password")" + fi + # Match arthas-client --command's semicolon-separated command syntax. + printf '%s\n' "${commands//;/$'\n'}" + } > "$file" +} +redact_auth_echo() { + local line + while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + *'$ auth --username '*) printf '%s$ auth [credentials omitted]\n' "${line%%'$ auth --username '*}" ;; + *) printf '%s\n' "$line" ;; + esac + done +} +# The native client's -t option does not bound its final response wait. +bounded_client() { + write_batch "$state_dir/cleanup.commands" "$1" + "$JAVA" -jar "$ARTHAS/arthas-client.jar" -t 5000 -f "$state_dir/cleanup.commands" "$connect_ip" "$port" & + local command_pid=$! + (sleep 10; kill -KILL "$command_pid" 2>/dev/null) >/dev/null 2>&1 & + local timer_pid=$! + local status=0 + wait "$command_pid" || status=$? + kill -KILL "$timer_pid" 2>/dev/null || true + wait "$timer_pid" 2>/dev/null || true + return "$status" +} +stop_agent() { + local session attempt agent_pid + connect_ip=$(cat "$state_dir/connect-ip") + if ! session=$(bounded_client session 2>&1); then + if [[ "$session" == *"Connection refused"* ]]; then + return 0 + fi + echo "Error: could not verify Arthas cleanup at $target_ip:$port; check the agent manually" >&2 + return 1 + fi + if [[ "$session" =~ JAVA_PID[[:space:]]+([0-9]+)([[:space:]]|$) ]]; then + agent_pid=${BASH_REMATCH[1]} + if [ "$agent_pid" != "$pid" ]; then + echo "Error: Arthas port $target_ip:$port belongs to PID $agent_pid, expected $pid; left running" >&2 + return 1 + fi + else + echo "Error: could not verify the target PID for cleanup;" \ + "check Arthas credentials and session at $target_ip:$port; stop the agent manually" >&2 + return 1 + fi + bounded_client stop >/dev/null 2>&1 || true + # Command errors can return zero; verify closure after asynchronous shutdown. + for attempt in 1 2 3; do + if ! session=$(bounded_client session 2>&1) && [[ "$session" == *"Connection refused"* ]]; then + return 0 + fi + sleep 0.2 + done + echo "Error: could not stop Arthas at $target_ip:$port; check session/stop settings or restart the service" >&2 + return 1 +} +cleanup() { + local status=$? + trap '' INT TERM HUP + trap - EXIT + if [ -n "$client_pid" ]; then + # Let an in-flight loadAgent finish before checking/closing its listener. + if [ "$attaching" = false ]; then + kill "$client_pid" 2>/dev/null || true + fi + wait "$client_pid" 2>/dev/null || true + fi + if [ -f "$state_dir/validated" ]; then + stop_agent || { [ "$status" -ne 0 ] || status=1; } + fi + rm -f "$state_dir/validated" "$state_dir/connect-ip" "$state_dir/cleanup.commands" "$state_dir/run.commands" + rmdir "$state_dir" + exit "$status" +} +trap cleanup EXIT +trap 'trap "" INT TERM HUP; exit 130' INT +trap 'trap "" INT TERM HUP; exit 143' TERM +trap 'trap "" INT TERM HUP; exit 129' HUP +echo "Waiting for PID $pid to answer the attach request..." >&2 +( + # Group signals must not end the client before target-side loadAgent finishes. + trap '' INT TERM HUP + exec "$JAVA" --add-modules jdk.attach --class-path "$ARTHAS/arthas-core.jar" \ + "$BIN/ArthasAttach.java" "$pid" "$ARTHAS" "$port" "$TOP" "$state_dir/validated" "$target_ip" \ + "$username" "$password" +) <&0 & +client_pid=$! +wait "$client_pid" +attaching=false +connect_ip=$(cat "$state_dir/connect-ip") Review Comment: Minor: when the Server API has already started an agent on 8562 and `--telnet-port` is left out, the helper fails with a stack trace and the client usage text, and nothing points to the cause. Arthas keeps its existing listener, so the attach succeeds. The client then prints `Connect to telnet server error: 127.0.0.1 3658`, a `ConnectException` trace and about 20 lines of usage, and the helper exits 1. This is the likeliest mistake after API activation. The docs cover it, but the output does not lead there. The fixed default port is fine, so a hint is enough. After the attach, if the requested port does not answer, please print one `Error:` line, for example `no Arthas listener at 127.0.0.1:3658; an existing agent may use another port (API default 8562); retry with --telnet-port`. ########## docs/server-runtime.md: ########## @@ -79,11 +79,120 @@ existing precedence; see [container heap sizing](../docker/README.md#server-heap ### Arthas diagnostics -Server bundles Arthas 4.1.5. Activate the agent with `PUT /arthas` using the Server's -configured authentication. The existing `arthas.ip`, `arthas.httpPort`, -`arthas.telnetPort` and `arthas.disabledCommands` options still configure the agent. -HugeGraph disables the bundled agent's optional MCP endpoint and keeps the existing -activation response and diagnostic HTTP/telnet interfaces. +Server and Store bundle Arthas 4.1.5, managed by the root POM. PD does not embed +an Arthas agent. Diagnostic ports bind to `127.0.0.1` by default. Administrators +can explicitly configure another valid address, including `0.0.0.0` for all IPv4 Review Comment: Minor: `0.0.0.0` is described as all IPv4 interfaces, but on a default Linux JVM the listener is dual-stack and also accepts IPv6. With the `PUT /arthas` config map and `arthas.ip=0.0.0.0` on JDK 17 (net.ipv6.bindv6only=0), Arthas 4.1.5 listened as `*:PORT` in `ss -6 -ltn` and accepted a connection to `::1`. An operator who firewalls only IPv4 leaves the IPv6 side open. Line 121 has the same gap for `::`, which also accepts IPv4. No code change is needed. Please say that both wildcards normally bind IPv4 and IPv6, and that firewall rules should cover both. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/ArthasAttach.java: ########## @@ -0,0 +1,96 @@ +/* + * 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. + */ + +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Properties; +import java.util.regex.Pattern; + +import com.sun.tools.attach.VirtualMachine; +import com.taobao.arthas.core.config.FeatureCodec; + +class ArthasAttach { + public static void main(String[] args) throws Exception { + String ip = args[5]; + InetAddress address = InetAddress.getByName(ip); + String connectIp = address.isAnyLocalAddress() ? + (address.getAddress().length == 4 ? "127.0.0.1" : "::1") : ip; Review Comment: Important: `--target-ip ::` without credentials cannot run commands or clean up, so the helper exits and the agent keeps listening on all interfaces. Arthas 4.1.5 generates a password for `::`, and its local no-auth exception only matches a `127.0.0.1` peer. This line picks `::1` for batch commands and cleanup, which then needs that password. On JDK 17, `bin/arthas.sh --target-ip :: --command version` printed `command not permitted`, then `could not verify the target PID for cleanup`, exit 1, and `*:PORT` stayed open until the JVM stopped. Docs lines 105-106 and 121-122 say this path cleans up. `0.0.0.0` exited 0 and closed the port. Connecting through `127.0.0.1` for both wildcards gave exit 0 and a closed port, and keeps `::` usable without extra flags; docs line 122 would need the same update. Please add this case to test-arthas-cli.sh. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/arthas.sh: ########## @@ -0,0 +1,201 @@ +#!/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. +# +# Offline attachment to a running Server or Store using the bundled agent. +set -euo pipefail + +fail() { echo "Error: $*" >&2; exit 1; } +usage() { + echo "Usage: $0 [--pid PID] [--telnet-port PORT] [--target-ip ADDRESS] [--command COMMAND]" + echo " [--username USER] [--password PASSWORD]" + echo "Run as the service OS user; defaults to bin/pid and loopback port 3658." + echo "New agents disable HTTP/MCP; use the existing telnet port for an active agent." + echo "Exiting this helper stops Arthas, including after --command or quit; the JVM keeps running." +} + +BIN=$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd) +TOP=$(cd -P "$BIN/.." && pwd) +pid="" +port=3658 +target_ip=127.0.0.1 +username="" +password="" +command="" +boot_args=() +while [ "$#" -gt 0 ]; do + case "$1" in + --pid|--telnet-port|--target-ip|--command|--username|--password) + [ "$#" -ge 2 ] || fail "Missing value for $1" + case "$1" in + --pid) pid=$2 ;; + --telnet-port) port=$2 ;; + --target-ip) target_ip=$2 ;; + --command) command=$2 ;; + --username) username=$2 ;; + --password) password=$2 ;; + esac + shift 2 ;; + -h|--help) usage; exit 0 ;; + *) usage >&2; fail "Unknown option: $1" ;; + esac +done +# Native batch files contain one command per line. +for credential in "$username" "$password"; do + [[ "$credential" != *$'\n'* && "$credential" != *$'\r'* ]] || fail "Credentials must be single-line values" +done +if [ -z "$pid" ]; then + [ -r "$BIN/pid" ] || fail "No bin/pid; start the service or supply --pid PID" + pid=$(cat "$BIN/pid") +fi +[[ "$pid" =~ ^[1-9][0-9]{0,9}$ ]] || fail "PID must be a positive integer" +[[ "$port" =~ ^[1-9][0-9]{0,4}$ ]] && [ "$port" -le 65535 ] || fail "Invalid telnet port" +[ -t 0 ] || [ -n "$command" ] || fail "Interactive use needs a terminal; use --command COMMAND" +case "$target_ip" in + [Ll][Oo][Cc][Aa][Ll][Hh][Oo][Ss][Tt]) target_ip=127.0.0.1 ;; +esac +JAVA=${JAVA_HOME:+$JAVA_HOME/bin/}java +ARTHAS="$TOP/lib/arthas" +[ -f "$ARTHAS/arthas-boot.jar" ] || fail "Bundled Arthas files are missing from lib/arthas" +# Keep the listener within this diagnosis session. Verify its PID before stopping +# it, since an occupied port must never stop a different application's agent. +state_dir=$(mktemp -d) +client_pid="" +attaching=true +connect_ip=$target_ip +quote_argument() { + local value=${1//\'/\'\\\'\'} + printf "'%s'" "$value" +} +write_batch() { + local file=$1 + local commands=$2 + { + if [ -n "$password" ]; then + printf 'auth --username %s %s\n' "$(quote_argument "${username:-arthas}")" "$(quote_argument "$password")" + fi + # Match arthas-client --command's semicolon-separated command syntax. + printf '%s\n' "${commands//;/$'\n'}" + } > "$file" +} +redact_auth_echo() { + local line + while IFS= read -r line || [ -n "$line" ]; do + case "$line" in + *'$ auth --username '*) printf '%s$ auth [credentials omitted]\n' "${line%%'$ auth --username '*}" ;; + *) printf '%s\n' "$line" ;; + esac + done +} +# The native client's -t option does not bound its final response wait. +bounded_client() { + write_batch "$state_dir/cleanup.commands" "$1" + "$JAVA" -jar "$ARTHAS/arthas-client.jar" -t 5000 -f "$state_dir/cleanup.commands" "$connect_ip" "$port" & + local command_pid=$! + (sleep 10; kill -KILL "$command_pid" 2>/dev/null) >/dev/null 2>&1 & + local timer_pid=$! + local status=0 + wait "$command_pid" || status=$? + kill -KILL "$timer_pid" 2>/dev/null || true + wait "$timer_pid" 2>/dev/null || true + return "$status" +} +stop_agent() { + local session attempt agent_pid + connect_ip=$(cat "$state_dir/connect-ip") + if ! session=$(bounded_client session 2>&1); then + if [[ "$session" == *"Connection refused"* ]]; then + return 0 + fi + echo "Error: could not verify Arthas cleanup at $target_ip:$port; check the agent manually" >&2 + return 1 + fi + if [[ "$session" =~ JAVA_PID[[:space:]]+([0-9]+)([[:space:]]|$) ]]; then + agent_pid=${BASH_REMATCH[1]} + if [ "$agent_pid" != "$pid" ]; then + echo "Error: Arthas port $target_ip:$port belongs to PID $agent_pid, expected $pid; left running" >&2 + return 1 + fi + else + echo "Error: could not verify the target PID for cleanup;" \ + "check Arthas credentials and session at $target_ip:$port; stop the agent manually" >&2 Review Comment: Minor: the exit messages do not say what happened, both on success and when Arthas rejects the credentials. After a clean exit the helper prints nothing. In interactive mode the last line is then the native client's `Arthas is still running in the background ... execute the 'stop' command`, so operators who know Arthas attach again to stop it. When the credentials are wrong, or missing for an agent that needs them, this line says `check Arthas credentials and session ...; stop the agent manually`. If that agent belongs to another operator's session, stopping it ends their work. The message also prints the bind address, so `::` shows as `:::3658`. Leaving the agent running on failure is the right call. Please print one stderr line when cleanup succeeds, say `credentials rejected; agent left running` when `$session` contains `not permitted`, and print `$connect_ip`, bracketed for IPv6, here and at lines 123, 129 and 145. -- 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]
