fix(hugegraph-dist): gate init-store on a dedicated init_store.enabled option#3119
fix(hugegraph-dist): gate init-store on a dedicated init_store.enabled option#3119bitflicker64 wants to merge 5 commits into
Conversation
…g is false Honor the existing ServerOptions flag in InitStore so distributed/HStore can opt out of local backend and admin init. Unset keeps full init for standalone. Docker maps HG_SERVER_LOAD_FROM_LOCAL_CONFIG to rest-server.properties. Fixes apache#3118
dosubot
Bot
added
size:M
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Disabling local graph loading also bypasses the Docker password-based admin bootstrap, so the requested credential is never installed. Evidence: exact-head static trace through docker-entrypoint.sh, InitStore.java, and GraphManager.java; 3/3 focused tests passed; visible checks are green.
| + "backend/admin init; distributed/Helm sets false.", | ||
| ServerOptions.GRAPH_LOAD_FROM_LOCAL_CONFIG.name(), | ||
| restConf); | ||
| return; |
There was a problem hiding this comment.
StandardAuthenticator.initAdminUserIfNeeded(restConf) below. The Docker entrypoint still handles a documented PASSWORD by enabling auth, piping that password to init-store.sh, and then writing docker/init_complete; with HG_SERVER_LOAD_FROM_LOCAL_CONFIG=false, the password is therefore consumed without creating the admin. In PD mode startup instead uses auth.admin_pa (default pa), so it still ignores the requested Docker credential. Please preserve the authentication bootstrap while skipping only local backend initialization, or explicitly reject/map this configuration combination, and add coverage for PASSWORD together with HG_SERVER_LOAD_FROM_LOCAL_CONFIG=false.
There was a problem hiding this comment.
Fixed in 69ac25e.
I took the "map the configuration combination" option rather than preserving the bootstrap, because initAdminUserIfNeeded() calls setup(), which scans the graphs directory and opens the auth graph store locally. That is the work the skip exists to avoid.
So in skip mode the entrypoint writes PASSWORD to auth.admin_pa instead of piping it into init-store.sh, and the server creates the admin from that on startup. It also no longer writes docker/init_complete, since nothing was initialized and a later run with init-store enabled must still be able to perform the real initialization.
PASSWORD together with skip is covered in hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh, which asserts the password reaches auth.admin_pa and never reaches init-store.sh stdin.
Two caveats I documented rather than fixed, both pre-existing properties of auth.admin_pa:
- it only applies when the admin account is first created, so changing
PASSWORDon a later restart silently keeps the old one, without an error; - it leaves the password at rest in
rest-server.properties, whereas the enabled path only passes it over stdin.
Both are noted at the line that causes them and in docker/README.md. Happy to take them on here if you would rather they were fixed than documented.
To be explicit about the limit of the testing: the smoke tests prove the entrypoint wiring, not that the server then creates a working admin from auth.admin_pa under PD. I have not run the 3pd-3store cluster to confirm that end to end.
…d option Replace the graph.load_from_local_config gate with a dedicated init_store.enabled option defaulting to true. That property already means whether GraphManager loads graph definitions from the local directory, and existing configs may materialize its declared false default, so reusing it made an explicit false skip backend and admin initialization for standalone RocksDB/HBase installs. The early return also skipped StandardAuthenticator.initAdminUserIfNeeded, so a Docker PASSWORD was piped into init-store.sh and discarded without creating the admin, leaving the server on the auth.admin_pa default. In skip mode the entrypoint now writes PASSWORD to auth.admin_pa instead, and does not create docker/init_complete since nothing was initialized. The gate is read back from rest-server.properties so a config mounted without the env var behaves the same way. Cover the lifecycle with a main-level test whose graphs directory would fail if the early exit were missed, and entrypoint smoke tests for default init, PASSWORD, skip, skip with PASSWORD, mounted property, and false to true restart. Run those in Docker CI and extend its path filter to the dist docker directory.
dosubot
Bot
added
size:L
bitflicker64
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3119 +/- ##
============================================
- Coverage 37.43% 0.08% -37.36%
+ Complexity 520 22 -498
============================================
Files 808 748 -60
Lines 69581 63271 -6310
Branches 9165 8280 -885
============================================
- Hits 26048 51 -25997
- Misses 40750 63218 +22468
+ Partials 2783 2 -2781 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The skip path still fails admin bootstrap for the shipped HStore configuration and has inconsistent option/password handling; backend and plugin registration also occurs before the gate. Evidence: six independent review lanes, exact-head static traces, an uppercase FALSE regression test failing four assertions, and all visible latest-head checks passing.
| # ── Init store (once) ───────────────────────────────────────────────── | ||
| # With `init_store.enabled=false` (distributed PD/HStore) init-store is a no-op: | ||
| # storage owns the metadata and the admin account is created on server startup | ||
| # from `auth.admin_pa`. A requested PASSWORD is therefore written to that |
There was a problem hiding this comment.
GraphManager calls initAdminUserIfNeeded() only inside usePD=true, but ServerOptions.USE_PD defaults to false and the repository's HStore compose files set only the HStore backend and PD peers. With this skip option plus PASSWORD, InitStore no longer creates the admin and normal startup does not take the PD metadata path, so authentication can start without the requested admin account. Please either make bootstrap work for every advertised HStore skip configuration or reject/avoid this mode unless usePD=true, and add an integration test that authenticates with the supplied password.
There was a problem hiding this comment.
Confirmed, and worse than a mismatch: with usePD false nothing creates the admin at all. StandardAuthenticator.initAdminUserIfNeeded is called only from InitStore, and GraphManager.initAdminUserIfNeeded only from loadMetaFromPD(), which is gated on usePD.
Took the reject option in a62f6ff. The entrypoint refuses skip + auth unless usePD=true and exits with the reason; InitStore logs the same for non-Docker installs. Skip without auth is unchanged. usePD=true is already the pairing in the cluster-test rest-server.properties.template.
The shipped compose files not setting usePD is pre-existing and left alone, since enabling it also moves graph loading to PD metadata.
| # the same as `HG_SERVER_INIT_STORE_ENABLED` (the env mapping above has already | ||
| # been applied, so env still wins). | ||
| INIT_STORE_ENABLED=$(get_prop "init_store.enabled" "${REST_SERVER_CONF}") | ||
| if [[ "${INIT_STORE_ENABLED:-true}" == "false" ]]; then |
There was a problem hiding this comment.
false, while HugeConfig accepts case-insensitive false values (and Java-properties syntax also permits separators other than =). With HG_SERVER_INIT_STORE_ENABLED=FALSE and PASSWORD, the shell pipes the password to init-store.sh and later writes init_complete, but Java parses the option as false and returns before creating the admin. A focused exact-head test reproduced this with four failed assertions. Please canonicalize or validate the value consistently across both layers and cover uppercase and mounted-property variants.
There was a problem hiding this comment.
Confirmed and fixed in a62f6ff. to_bool now mirrors PropertyConverter.toBoolean (BooleanUtils: y/t/on/yes/true, n/f/no/off/false, case-insensitive), canonicalizes before writing so the file only ever holds true or false, and exits non-zero on anything else instead of diverging silently.
get_prop also accepts the : and whitespace separators a properties file allows, not just =. Covered for FALSE, off, no, a non-boolean, and a mounted : separator.
| # created, so changing PASSWORD on a later restart silently keeps the | ||
| # old one. It also leaves the password at rest in rest-server.properties, | ||
| # unlike the enabled path where it only travels over stdin. | ||
| set_prop "auth.admin_pa" "${PASSWORD}" "${REST_SERVER_CONF}" |
There was a problem hiding this comment.
set_prop() escapes for sed, not for Java-properties serialization. A valid password containing a backslash, such as abc\def, is written with a single backslash and is read back by the properties parser as abcdef; the old stdin path preserved it. Please serialize the secret using Java-properties escaping (or avoid storing it in this file) and add a round-trip test containing backslashes and other properties metacharacters.
There was a problem hiding this comment.
Confirmed and fixed in a62f6ff. props_escape doubles backslashes and escapes a leading space before the value reaches set_prop, so abc\def round-trips instead of arriving as abcdef. Test asserts the escaped form is what lands in the file.
| * every Server pod restart, since the entrypoint's init flag file does | ||
| * not survive one. | ||
| */ | ||
| if (!restServerConfig.get(ServerOptions.INIT_STORE_ENABLED)) { |
There was a problem hiding this comment.
registerBackends() and registerPlugins(). Plugin registration invokes every discovered plugin.register() and propagates failures, so disabled mode can still fail or trigger plugin side effects before reaching the documented no-op/exit-0 path. Please register only the server options first, evaluate this flag, and move backend/plugin registration to the enabled path; add coverage proving disabled mode bypasses those calls.
There was a problem hiding this comment.
Confirmed and fixed in a62f6ff. Order is now registerServer(), build the config, evaluate the gate, then registerBackends() and registerPlugins() on the enabled path only.
Verified no default-path change: every key in the shipped rest-server.properties is declared in ServerOptions, which registerServer() covers. Added a test asserting rocksdb.data_path never enters OptionSpace in disabled mode.
… is true With init_store.enabled=false the built-in admin account is not created by init-store, and the only other component that creates it is GraphManager.initAdminUserIfNeeded(), reached from loadMetaFromPD() and so gated on usePD. That option defaults to false and the shipped HStore compose files do not set it, so enabling auth in skip mode would start a server that enforces authentication with no account to authenticate against. The entrypoint now refuses that combination and exits with an explanation, and InitStore logs the same condition for non-Docker installs. Skipping without auth is unaffected. usePD=true is already the documented pairing for distributed deployments in the cluster-test rest-server.properties template. Also align the shell with the server's boolean parsing, which is case-insensitive, so HG_SERVER_INIT_STORE_ENABLED=FALSE no longer means skip to Java and run to the entrypoint; read the property through the separators a properties file allows; escape the password for properties serialization so a backslash survives; and register backends and plugins only on the enabled path, since plugin registration runs every plugin's register() and propagates its failures. Add entrypoint coverage for these plus a test that disabled mode leaves backend options unregistered. Note a pre-existing arthas config key mismatch found while checking the option table.
|
Unrelated to this change, but noticed while checking the The arthas options are declared camelCase ( It is invisible today because each shipped value equals its default. It bites on any change, e.g. |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The mounted-properties override path can create duplicate scalar keys and make init-store or server startup fail. Evidence: six independent review lanes, exact-head static tracing through docker-entrypoint.sh and HugeConfig.java, and all visible latest-head checks passing.
| log "ERROR: HG_SERVER_INIT_STORE_ENABLED must be a boolean, got '${HG_SERVER_INIT_STORE_ENABLED}'" | ||
| exit 1 | ||
| fi | ||
| set_prop "init_store.enabled" "${HG_SERVER_INIT_STORE_ENABLED}" "${REST_SERVER_CONF}" |
There was a problem hiding this comment.
get_prop() accepts the =, :, and whitespace separators, but set_prop() only replaces key=value. With a mounted init_store.enabled:false and HG_SERVER_INIT_STORE_ENABLED=true, this appends a second key instead of overriding the first. Commons PropertiesConfiguration then exposes multiple values for this scalar Boolean, so HugeConfig.get(INIT_STORE_ENABLED) no longer receives a Boolean and init-store/server startup fails; the same issue affects a colon-form auth.admin_pa overridden by PASSWORD. Please make set_prop() recognize the same separators and normalize all existing definitions to one canonical key, then add colon/whitespace env-override tests that load the result through HugeConfig.
There was a problem hiding this comment.
Confirmed and fixed in 3cb4fda. set_prop now matches the same =, : and whitespace separators as get_prop and collapses every existing definition into one canonical key=value line, leaving comments alone. Rewrote it in awk so the key and value are matched and emitted literally, with no regex escaping.
Worth noting it is worse than a stale read: a duplicated key fails the scalar type check while the config is still loading, so new HugeConfig(...) throws Invalid value for key 'init_store.enabled': '[false, true]' and both init-store and server startup fail outright. Pinned that with a test that loads a duplicated key through HugeConfig.
Entrypoint coverage added for a colon-form override, a whitespace-form override, a colon-form auth.admin_pa overridden by PASSWORD, and that commented defaults are not counted as definitions.
get_prop was widened to read the `=`, `:` and whitespace separators a properties file allows, but set_prop still matched only `key=value`. A mounted `init_store.enabled:false` overridden by HG_SERVER_INIT_STORE_ENABLED therefore gained a second definition instead of being replaced, and a key defined twice collects both values into a list that fails the scalar type check while the config is still loading. Both init-store and server startup would fail. The same applied to a colon-form auth.admin_pa overridden by PASSWORD. set_prop now matches the same separators and collapses every existing definition into one canonical `key=value` line, leaving comments alone. It is written in awk, so the key and value are matched and emitted literally and no regex escaping is involved. Cover colon-form and whitespace-form overrides asserting a single remaining definition, a colon-form auth.admin_pa override, and that commented defaults are not treated as definitions. Add a test loading a duplicated key through HugeConfig to pin why the collapse is required.
2 participants
Fixes #3118
Kept in sync with hugegraph#171
Problem
InitStorealways runs full local initialization: scan./conf/graphs, initnon-hstore backends, create the built-in admin account. That is correct for
standalone / tarball users and must remain the default.
In PD / HStore deployments the storage side already owns metadata, so there
is nothing for init-store to do.
Why the Docker guard is not enough
docker-entrypoint.shguards init with a flag file,docker/init_complete.That file lives in the container's writable layer at
/hugegraph-server/docker/init_complete. No compose file mounts a volume overit, and the server services declare no volumes at all.
That guard works for Docker and does not work for Kubernetes:
docker restartSo on Helm the flag file never accumulates: every Server pod restart re-runs
the complete local init against a PD/HStore cluster that already owns its
metadata. Working around that is what pushed the chart toward one-shot init
Jobs and
HG_SERVER_SKIP_INIT-style switches. A file-based guard cannot fixthis, because the problem is precisely that the file does not persist. The
decision has to come from configuration instead.
This is the whole motivation for the change: it is a distributed-deployment
concern, not a Docker one.
Solution
A dedicated option,
init_store.enabled, defaulting totrue:init_store.enabledinrest-server.propertiesinit-storetruefalseA dedicated option rather than reusing
graph.load_from_local_config: thatproperty already means "whether
GraphManagerloads graph definitions from thelocal directory", its declared default is
false, and existing configs maymaterialize that default explicitly. Giving it a second contract would make an
explicit
falsesilently skip backend and admin init for standaloneRocksDB/HBase installs.
Docker maps
HG_SERVER_INIT_STORE_ENABLEDonto the property inrest-server.properties, the fileinit-store.shpasses toInitStore.Auth bootstrap in skip mode
When init-store is skipped,
StandardAuthenticator.initAdminUserIfNeeded()does not run, so a Docker
PASSWORDpiped intoinit-store.shwould be readand discarded, so the server would then come up with the
auth.admin_padefaultof
painstead of the requested credential.The entrypoint therefore writes
PASSWORDtoauth.admin_pain skip modeinstead of piping it to
init-store.sh, so the account the server creates onstartup uses the requested password.
That startup path has a precondition.
GraphManager.initAdminUserIfNeeded()isreached only from
loadMetaFromPD(), which runs only whenusePDis true, andusePDdefaults to false. With init-store skipped andusePDfalse, nocomponent creates the built-in admin at all, so enabling auth would start a
server that enforces authentication against an empty account list. The
entrypoint refuses that combination and exits with an explanation, and
InitStorelogs the same condition for non-Docker installs.usePD=trueis the intended setting for distributed deployments: thecluster-test
rest-server.properties.templatealready pairs it withauth.authenticatorandauth.admin_pa. The shipped compose files not settingit is a pre-existing gap, and is left to a follow-up because enabling it also
switches graph loading from local config to PD metadata.
The entrypoint also does not write
docker/init_completein skip mode: nothingwas initialized, so a later run with init-store enabled must still be able to
perform the real initialization.
Files changed
.../config/ServerOptions.javaINIT_STORE_ENABLEDoption, defaulttrue.../cmd/InitStore.javainit_store.enabled=false.../docker/docker-entrypoint.shPASSWORD→auth.admin_pain skip mode; refuse auth + skip withoutusePD; no init flag when nothing was initialized.../unit/cmd/InitStoreConfigTest.javamain()-level proof of the early exit.../unit/UnitTestSuite.java.../docker/test/test-docker-entrypoint.sh.github/workflows/docker-build-ci.ymlhugegraph-dist/docker/**docker/README.mdTest plan
InitStoreConfigTestpoints the temporaryrest-server.propertiesat agraphsdirectory that does not exist, then callsInitStore.main()withinit_store.enabled=false. A companion test asserts that the same directorydoes make
ConfigUtil.scanGraphsDirfail, so the skip test proves the gate isapplied before any graph or admin work rather than being a tautology.
The entrypoint lifecycle is covered separately, with no JVM, backend or Docker
needed. The entrypoint runs against a throwaway install tree whose
./binscripts are stubs that record their own invocation:
Cases: default init, default +
PASSWORD, skip via env, skip +PASSWORD(asserts the password reaches
auth.admin_paand neverinit-store.shstdin),skip via a mounted property with no env var, env overriding a conflicting
property,
false → truerestart, flag-file suppression of re-init, booleanparsing parity with the server (
FALSE,off,no, and fail-fast onnon-booleans), the
:property separator, a backslash password round trip, andrefusal of auth plus skip without
usePD.Manual smoke (optional):
conf/rest-server.properties(no flag):bin/init-store.sh→ fullinit, same as master.
init_store.enabled=false, run again → WARN and exit 0 without backendinit.
Out of scope
util.sh/check_port/ lsofSKIP_INITremoval (the chart sets the env, andusePD=truewhen it enables auth)usePD=truein the shipped compose files, which also switches graphloading from local config to PD metadata
GraphManagerbehavior and thegraph.load_from_local_configdefaultCompatibility
No breaking change for tarball / bare init-store. The option defaults to
true, so unset config keeps the full init path.Distributed deployments set
init_store.enabled=false(orHG_SERVER_INIT_STORE_ENABLED=false).