bitflicker64 commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r3928440554
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,58 @@ set_prop() {
get_prop_encoded() {
local key="$1" file="$2"
- local esc_key
- esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g')
- sed -nE \
-
"s~^[[:space:]]*${esc_key}([[:space:]]*[:=][[:space:]]*|[[:space:]]+)(.*)$~\\2~p"
\
- "${file}" | head -n 1
+ PROPS_MODE=get PROPS_KEY="${key}" PROPS_FILE="${file}" \
+ awk -f "${PROPS_AWK}" /dev/null
+}
+
+# First uncommented `authenticator:` inside the gremlin-server.yaml
+# authentication block. snakeyaml resolves duplicate top-level keys to the
+# last one, but a mounted file carrying two authentication blocks is
+# pathological; report the first and let the mismatch WARN handle it.
+get_yaml_authenticator() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || return 0
+ awk '
+ /^[ \t]*#/ { next }
+ /^[ \t]*authentication[ \t]*:/ { inblk = 1; next }
+ inblk && /^[ \t]+authenticator[ \t]*:/ {
+ line = $0
+ sub(/^[ \t]*authenticator[ \t]*:[ \t]*/, "", line)
+ sub(/[,:].*$/, "", line)
Review Comment:
⚠️ The yaml scalar goes into rest-server.properties as-is, so quotes and
inline comments become part of the class name.
`sub(/[,:].*$/, "", line)` strips a trailing comma and nothing else, and
line 123 hands the result straight to `set_prop_encoded`. snakeyaml strips
quotes, comments and padding; `java.util.Properties` strips none of them.
Measured at 698b0c3 on `ubuntu:22.04`:
```
yaml authenticator: "com.example.MyAuth" ->
auth.authenticator="com.example.MyAuth"
yaml authenticator: com.example.MyAuth␣␣␣ ->
auth.authenticator=com.example.MyAuth␣␣␣
yaml authenticator: com.example.MyAuth # custom ->
auth.authenticator=com.example.MyAuth # custom
```
Gremlin resolves `com.example.MyAuth` in all three rows. REST gets a
different string in all three, which is the split `align_auth_config` exists to
prevent, and a quoted scalar is ordinary yaml.
Requested change: before returning, strip an inline `#` comment, then
surrounding single or double quotes, then trailing blanks.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -41,16 +41,34 @@ if [ ! -d "$BAK_CONF" ]; then
cp "${CONF}/${GREMLIN_SERVER_CONF}"
"${BAK_CONF}/${GREMLIN_SERVER_CONF}.bak"
cp "${CONF}/${REST_SERVER_CONF}" "${BAK_CONF}/${REST_SERVER_CONF}.bak"
cp "${CONF}/graphs/${GRAPH_CONF}" "${BAK_CONF}/${GRAPH_CONF}.bak"
+fi
+
+# The appends below are guarded per file and match only an absent or still
+# commented-out definition, so they are no-ops on any config that already
+# carries authentication (e.g. a mounted one, or a re-run of this script).
+# Appending unconditionally used to create duplicate definitions that the
+# properties parser (first definition wins) and the yaml parser (last wins)
+# resolved in opposite directions, leaving Gremlin and REST on different
+# authenticators.
+AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}"
+if ! grep -Eq '^[ \t]*authentication[ \t]*:' "${CONF}/${GREMLIN_SERVER_CONF}";
then
sed -i -e '$a\authentication: {' \
- -e '$a\ authenticator:
org.apache.hugegraph.auth.StandardAuthenticator,' \
+ -e "\$a\\ authenticator: ${AUTHENTICATOR_CLASS}," \
-e '$a\ authenticationHandler:
org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler,' \
-e '$a\ config: {tokens: conf/rest-server.properties}' \
-e '$a\}' ${CONF}/${GREMLIN_SERVER_CONF}
+fi
- sed -i -e
'$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' \
- -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF}
+if ! grep -Eq '^[ \t]*auth\.authenticator[ \t]*='
"${CONF}/${REST_SERVER_CONF}"; then
Review Comment:
⚠️ These guards accept only the `key=` spelling, so the forms `props.awk`
was added for still get a duplicate appended.
`^[ \t]*auth\.authenticator[ \t]*=` matches none of `auth.authenticator :
X`, `auth.authenticator X`, `auth\.authenticator=X`, and `java.util.Properties`
reads all three as a definition of `auth.authenticator`. Line 68 has the same
shape. Note also that `[ \t]` in a GNU grep `-E` bracket is the set {space,
backslash, `t`}, not a tab: under GNU grep 3.7 a tab-indented
`auth.authenticator=X` fails this guard too.
Docker path at 698b0c3, mounted config using `:`, after `align_auth_config
&& ./bin/enable-auth.sh`:
```
auth.authenticator : com.example.MyAuth
auth.authenticator=com.example.MyAuth
```
Two definitions of one key, which the unit test you add at
`docker/test/test-docker-entrypoint.sh:76-79` names as the thing to avoid. Run
standalone, with `AUTHENTICATOR_CLASS` unset, the same input gives those two
lines with different values (`: com.example.MyAuth`, then
`=org.apache.hugegraph.auth.StandardAuthenticator`) plus a yaml block on the
default.
This is an incomplete fix rather than a regression, since the old code
appended unconditionally. Requested change: use `[[:blank:]]`, accept `[:=]`
and the bare-whitespace separator, and cover the `\`-escaped key. Shipping
`props.awk` under `bin/` and querying it here would keep one grammar in one
place.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,58 @@ set_prop() {
get_prop_encoded() {
local key="$1" file="$2"
- local esc_key
- esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g')
- sed -nE \
-
"s~^[[:space:]]*${esc_key}([[:space:]]*[:=][[:space:]]*|[[:space:]]+)(.*)$~\\2~p"
\
- "${file}" | head -n 1
+ PROPS_MODE=get PROPS_KEY="${key}" PROPS_FILE="${file}" \
+ awk -f "${PROPS_AWK}" /dev/null
+}
+
+# First uncommented `authenticator:` inside the gremlin-server.yaml
+# authentication block. snakeyaml resolves duplicate top-level keys to the
+# last one, but a mounted file carrying two authentication blocks is
+# pathological; report the first and let the mismatch WARN handle it.
+get_yaml_authenticator() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || return 0
+ awk '
+ /^[ \t]*#/ { next }
+ /^[ \t]*authentication[ \t]*:/ { inblk = 1; next }
Review Comment:
⚠️ This `next` discards the rest of the `authentication:` line, so a
single-line flow mapping reads as no authenticator and the alignment quietly
falls back to the default.
Legal yaml that a hand-written or mounted file can carry:
```yaml
authentication: {authenticator: com.example.MyAuth, authenticationHandler:
org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler, config: {tokens:
conf/rest-server.properties}}
```
At 698b0c3 on `ubuntu:22.04`, `get_yaml_authenticator` prints nothing, so
`align_auth_config` takes the both-empty branch and exports
`StandardAuthenticator`. After `./bin/enable-auth.sh`:
```
rest: auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator
yaml: authentication: {authenticator: com.example.MyAuth, ...}
```
Gremlin on the mounted class, REST on the default, no WARN. On a first run
(pre-PR the whole block was gated on `conf-bak` not existing) the old script
appended a second `authentication:` block, so both sides came out on the
default under the last-wins rule that your comment on line 106 cites.
Requested change: match `authenticator:` on the `authentication:` line as
well, and when an `authentication` block is present but no authenticator can be
read, take the WARN branch rather than the both-empty one.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -41,16 +41,34 @@ if [ ! -d "$BAK_CONF" ]; then
cp "${CONF}/${GREMLIN_SERVER_CONF}"
"${BAK_CONF}/${GREMLIN_SERVER_CONF}.bak"
cp "${CONF}/${REST_SERVER_CONF}" "${BAK_CONF}/${REST_SERVER_CONF}.bak"
cp "${CONF}/graphs/${GRAPH_CONF}" "${BAK_CONF}/${GRAPH_CONF}.bak"
+fi
+
+# The appends below are guarded per file and match only an absent or still
+# commented-out definition, so they are no-ops on any config that already
+# carries authentication (e.g. a mounted one, or a re-run of this script).
+# Appending unconditionally used to create duplicate definitions that the
+# properties parser (first definition wins) and the yaml parser (last wins)
+# resolved in opposite directions, leaving Gremlin and REST on different
+# authenticators.
+AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}"
+if ! grep -Eq '^[ \t]*authentication[ \t]*:' "${CONF}/${GREMLIN_SERVER_CONF}";
then
sed -i -e '$a\authentication: {' \
- -e '$a\ authenticator:
org.apache.hugegraph.auth.StandardAuthenticator,' \
+ -e "\$a\\ authenticator: ${AUTHENTICATOR_CLASS}," \
-e '$a\ authenticationHandler:
org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler,' \
-e '$a\ config: {tokens: conf/rest-server.properties}' \
-e '$a\}' ${CONF}/${GREMLIN_SERVER_CONF}
+fi
- sed -i -e
'$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' \
- -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF}
+if ! grep -Eq '^[ \t]*auth\.authenticator[ \t]*='
"${CONF}/${REST_SERVER_CONF}"; then
+ sed -i -e "\$a\\auth.authenticator=${AUTHENTICATOR_CLASS}"
${CONF}/${REST_SERVER_CONF}
+fi
+
+if ! grep -Eq '^[ \t]*auth\.graph_store[ \t]*=' "${CONF}/${REST_SERVER_CONF}";
then
+ sed -i -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF}
+fi
- sed -i
's/gremlin.graph=org.apache.hugegraph.HugeFactory/gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy/g'
${CONF}/graphs/${GRAPH_CONF}
+if grep -Eq '^gremlin\.graph[ \t]*=org\.apache\.hugegraph\.HugeFactory[ \t]*$'
"${CONF}/graphs/${GRAPH_CONF}"; then
Review Comment:
⚠️ Anchoring the flip to `[ \t]*$` drops a CRLF config that the old
unanchored `sed` did convert.
Measured at 698b0c3 on `ubuntu:22.04` with
`conf/graphs/hugegraph.properties` saved with CRLF line endings:
```
$ grep -Eq '^gremlin\.graph[ \t]*=org\.apache\.hugegraph\.HugeFactory[
\t]*$' hugegraph.properties; echo $?
1
$ sed
's/gremlin.graph=org.apache.hugegraph.HugeFactory/gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy/g'
hugegraph.properties # pre-PR
gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy
```
End to end the graph keeps `gremlin.graph=org.apache.hugegraph.HugeFactory`
while rest-server.properties gains `auth.authenticator` and `auth.graph_store`
and the yaml gains an authentication block, so the factory is never wrapped for
auth although both servers now believe auth is on. A CRLF mounted config is
squarely the case this PR is about. A tab before `=` fails for the same reason
as line 64.
Requested change: use `[[:blank:]]` and allow an optional carriage return,
for example
`^gremlin\.graph[[:blank:]]*=org\.apache\.hugegraph\.HugeFactory[[:blank:]]*\r?$`,
with the `sed` pattern widened to match.
##########
hugegraph-server/hugegraph-dist/docker/props.awk:
##########
@@ -0,0 +1,227 @@
+# 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.
+#
+# props.awk — read and rewrite Java ".properties" files with the grammar
+# HugeConfig (commons-configuration over JDK Properties) applies, so the
+# entrypoint and the server agree on what a mounted file means. grep/sed
+# rewrites do not: they see `\`-escaped keys, `:` separators, continuation
+# lines and duplicate definitions differently, which is how a mounted
+# config ends up with two definitions of one key.
+#
+# One invocation, selected with the `mode` environment variable:
+#
+# mode=get key=K file=F
+# print the value of K's first logical definition
+# mode=set key=K file=F
+# replace K's first definition in place, drop every other
+# definition of K, append one when the file has none. The new
+# value arrives pre-encoded in PROP_VALUE_ENCODED (an environment
Review Comment:
🧹 The documented invocation does not work: the names are `PROPS_MODE`,
`PROPS_KEY`, `PROPS_FILE` and `PROPS_VALUE_ENCODED`.
`BEGIN` at lines 214-223 reads `ENVIRON["PROPS_MODE"]`,
`ENVIRON["PROPS_KEY"]`, `ENVIRON["PROPS_FILE"]` and
`ENVIRON["PROPS_VALUE_ENCODED"]`, so following this header verbatim gives
`props.awk: PROPS_FILE and PROPS_KEY must be set`.
```suggestion
# One invocation, selected with the `PROPS_MODE` environment variable:
#
# PROPS_MODE=get PROPS_KEY=K PROPS_FILE=F
# print the value of K's first logical definition
# PROPS_MODE=set PROPS_KEY=K PROPS_FILE=F
# replace K's first definition in place, drop every other
# definition of K, append one when the file has none. The new
# value arrives pre-encoded in PROPS_VALUE_ENCODED (an environment
```
##########
hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh:
##########
@@ -23,10 +23,16 @@ entrypoint="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." &&
pwd)/docker-entrypoint.s
test_dir="$(mktemp -d)"
trap 'rm -rf "${test_dir}"' EXIT
+# Eval the property helpers plus the PROPS_AWK location block they depend
+# on. The entrypoint's top-level code hard-exits when props.awk is
+# missing, so it cannot be sourced directly; anchor to the marker comment
Review Comment:
🧹 This comment describes an anchor the code does not use. Line 30 recomputes
`PROPS_AWK` independently, and the extraction on lines 32-36 still starts at
`encode_prop_value` and stops on a count of closing braces. No marker comment
is involved.
The count is also newly load-bearing and unexplained: `== 4` means "through
`get_prop_encoded`", so a helper added anywhere between `encode_prop_value` and
`get_prop_encoded` would cut the eval short and the suite would then fail with
a confusing `get_prop_encoded: command not found`.
Requested change: drop the marker-comment sentence and say what the number
selects, for example "stop after the fourth top-level function,
`get_prop_encoded`".
--
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]
