bitflicker64 commented on code in PR #3119:
URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3697914185
##########
hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/cmd/InitStore.java:
##########
@@ -83,6 +103,74 @@ public static void main(String[] args) throws Exception {
}
}
+ /**
+ * 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)) {
Review Comment:
You are right, and I had this backwards. Fixed in 9707feb.
I removed this clause in the rescope on the grounds that nothing in the
container writes `auth.admin_pa`, so it could never be satisfied and would fail
the distributed path once the exit status started propagating. That was the
wrong conclusion from the right fact: unsatisfiable is the correct outcome
here, because the alternative is handing out `admin`/`pa`.
The check now also requires an explicit non-empty `auth.admin_pa`, raised
through the same `unreachableAdmin` path so all four refusals share one type
and message prefix.
You were also right that the accepted-HStore test pinned the unsafe case — I
had deleted the `auth.admin_pa` line from it. It is back, and
`testDisabledInitStoreRejectsDefaultAdminPassword` now covers both the absent
and the explicitly-empty value.
One consequence worth stating: this makes apache/hugegraph#3132 fail at
container start, since its wrapper writes `usePD` and `pd.peers` but not
`auth.admin_pa`. That is the chart's bug rather than this PR's — it currently
ships auth that would come up on the public default — and I have commented
there with what it needs to change.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -74,9 +92,22 @@ if [[ ! -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ]]; then
else
log "init hugegraph with auth mode"
./bin/enable-auth.sh
+ # init-store reads the password from stdin, and a disabled one returns
+ # before it gets there, so say plainly that PASSWORD is being dropped
+ case "${INIT_STORE_ENABLED}" in
+ n | f | no | off | false)
+ log "WARN: PASSWORD is ignored while init-store is disabled;" \
+ "the admin is created on the PD startup path from" \
+ "'auth.admin_pa', which defaults to the public value 'pa'"
;;
+ esac
echo "${PASSWORD}" | ./bin/init-store.sh
fi
- touch "${DOCKER_FOLDER}/${INIT_FLAG_FILE}"
+ # A disabled init-store initialized nothing, so recording it as done would
+ # stop a later re-enable from initializing.
+ case "${INIT_STORE_ENABLED}" in
+ n | f | no | off | false) ;;
+ *) touch "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ;;
Review Comment:
Fixed in 9707feb, and I took the "result" reading rather than re-parsing the
property in shell.
`init-store` now writes the marker itself, and only after it has actually
initialized. The path is handed to it (`HG_SERVER_INIT_COMPLETE_MARKER`, or the
matching system property), so nothing is written when no caller asks for one
and the tarball path is unchanged. The entrypoint just exports the path and no
longer decides anything — the `case` on the environment variable is gone.
That closes the mounted-config case you describe: with
`init_store.enabled=false` in a mounted `rest-server.properties` and the
variable unset, Java returns without initializing and no marker is written, so
a later flip to `true` still initializes.
Regression is in `testGateDecidesWhetherRegistrationRuns`, which asserts the
marker is absent after a disabled run and after a failed enabled run, and that
the enabled run still reaches graph scanning rather than being short-circuited.
It lives in that method because registering backends twice in one JVM trips
`BackendProviderFactory`'s duplicate check — I found that the hard way when it
was a separate test. The one case not covered is a *successful* enabled run
writing the marker, which needs a live backend.
I went this way rather than reading the effective value back in the
entrypoint because a shell properties reader cannot match the grammar
`HugeConfig` uses, which you flagged earlier on this PR. Happy to switch to a
Java-side read if you would rather the entrypoint stayed the owner.
--
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]
