fix(docker): make auth bootstrap safe for mounted and upgraded configs - #3192
fix(docker): make auth bootstrap safe for mounted and upgraded configs#3192Adarsh-Me wants to merge 2 commits into
Conversation
The entrypoint's grep/sed property rewriting disagrees with HugeConfig on mounted or upgraded configs: escaped keys, ':'/whitespace separators, line continuations, and duplicate definitions are all read differently, so a mounted config could end up with two logical definitions of one key. Property reading/writing now goes through props.awk, which implements the java.util.Properties grammar (comments, both separators, continuations, backslash escapes, first-definition-wins duplicates) and keeps every untouched line byte-for-byte. Values travel through environment variables instead of command arguments, so a PASSWORD no longer shows up in 'ps' output when a key is rewritten in place. enable-auth.sh appended authentication definitions whenever conf-bak/ was absent, which on a mounted config created duplicate definitions that the properties parser (first definition wins) and the yaml parser (last definition wins) resolved in opposite directions -- Gremlin and REST could land on different authenticators with no error from either. Its appends are now guarded per file, only an absent or still commented-out definition triggers an append, re-runs are idempotent, and the authenticator class is overridable through AUTHENTICATOR_CLASS. The entrypoint aligns both sides before calling it: it copies a yaml authenticator into rest-server.properties, or exports the REST one for the yaml append, and warns without touching anything when the two name genuinely different authenticators. The unit test suite covers escaped keys, continuations, get-mode semantics, and comment-guarded appends; the entrypoint harness now ships props.awk into its sandbox, and both server Dockerfiles COPY it next to the entrypoint. Fixes apache#3133
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: props.awk is careful work and both shell suites this PR touches pass under the image's own mawk, but the new alignment layer does not hold on the mounted configs the PR targets. The yaml authenticator is copied into rest-server.properties without unquoting, a flow-style authentication: block reads as absent so REST silently falls back to the default, enable-auth.sh's guards accept only the key= spelling so duplicates are still appended, and the narrowed gremlin.graph guard no longer converts a CRLF config the old sed did convert. Two doc fixes as well. Evidence: measured at 698b0c3 in ubuntu:22.04 (GNU grep 3.7, GNU sed, mawk 1.3.4), the same toolchain eclipse-temurin:11-jre-jammy ships; test/test-docker-entrypoint.sh and docker-entrypoint-test.sh both exit 0 there; each finding below quotes the config the run produced. All seven workflow runs on this head are action_required and the combined status is pending with zero statuses, so there is no CI evidence for the image builds.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new properties parser still mishandles valid indented keys, so mounted authentication values can remain stale after an environment override. Evidence: reproduced at the exact head with the PR helper; existing current-head comments cover other findings and are not duplicated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3192 +/- ##
============================================
+ Coverage 37.77% 37.80% +0.02%
- Complexity 6560 6567 +7
============================================
Files 800 800
Lines 68960 68960
Branches 9166 9166
============================================
+ Hits 26052 26069 +17
+ Misses 39841 39825 -16
+ Partials 3067 3066 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-ups on the props.awk bootstrap: the yaml authenticator scalar now goes through a snakeyaml-shaped cleanup (inline comments, quotes and padding stripped) instead of only cutting at the first comma or colon; a flow mapping on the authentication line itself is read, and an authentication block without a readable authenticator takes the WARN branch instead of the both-empty default. props.awk strips leading whitespace before the key the way java.util.Properties does, so an indented key is rewritten in place rather than duplicated. enable-auth.sh's append guards now accept the ':', bare-whitespace and backslash-escaped spellings with [[:blank:]] classes (the '[ \t]' bracket matched space, backslash and the letter t), and the gremlin.graph flip embeds the carriage return as a byte because GNU grep reads \r in a pattern as the letter r, which made the anchored guard drop mounted CRLF configs. Test docs name the environment variables and the function count they rely on, and new regression tests cover indented keys, yaml scalar cleanup, flow mappings and the block-without-authenticator WARN.
3 participants
What is changing
Closes #3133 (the parts still open on master
3681148, since #3119 landed the rest).Properties rewriting now implements the Java grammar.
docker-entrypoint.shpreviously rewroterest-server.properties/hugegraph.propertieswithgrep/sed, which disagrees with HugeConfig on mounted or upgraded configs: backslash-escaped keys,:/whitespace separators, line continuations, and duplicate definitions are all parsed differently. The property logic moves to a newprops.awkloaded by the entrypoint, which implements thejava.util.Propertiesline grammar (comments, both separators, continuations, backslash escapes, first-definition-wins duplicates) and rewrites the first definition in place while keeping every untouched line byte-for-byte.PASSWORD no longer appears in
psoutput. The oldsedrewrite interpolated the encoded value into sed's command line, so when a key already existed (mounted or persisted config) the password was visible inps. Values now travel through an environment variable into awk, never through argv.enable-auth.sh appends are per-file guarded. The old script appended authentication definitions whenever
conf-bak/was absent. On a config it did not write, that created duplicate definitions that the properties parser (first definition wins) and snakeyaml (last definition wins) resolved in opposite directions — Gremlin and REST could land on different authenticators with no error from either. Now each append runs only when its file lacks the definition (or still has it commented out), re-runs are idempotent, customgremlin.graphfactories are preserved, and the authenticator class can be overridden viaAUTHENTICATOR_CLASS.The entrypoint aligns both sides before enabling auth. If the yaml declares an authenticator but the properties file does not (or vice versa), the entrypoint propagates it to the other side instead of letting the default
StandardAuthenticatorsplit the pair. When both sides name genuinely different authenticators, it logs a WARN and leaves both untouched instead of silently splitting them.Implementation notes
ConfigToolCLI: it delivers the same parser-agreement contract with a much smaller footprint and no new build artifact. Happy to rework toward the ConfigTool if reviewers prefer that direction.props.awkships in both server images (Dockerfile COPY) and in the test sandbox; CI already runs the unit suite viadocker-build-ci.yml.How was this tested
docker/test/test-docker-entrypoint.shextended with cases for: escaped-key definitions rewritten in place, continuation lines consumed with the key they belong to, get-mode separator/continuation/duplicate semantics, and appends when the key only exists commented out. All pass.docker-entrypoint-test.shfull harness passes end-to-end (secret round-trips incl. backslash/space/trailing-space secrets, enable-auth call counting unchanged).gremlin.graphfactory (preserved).Code Review Handbook
props.awkis the core: block model (comment lines and logical entries), first-definition-wins, raw-value round-trip (get returns the on-disk escaped form so feeding it back into set is byte-exact).docker-entrypoint-test.sh).auth\.admin_pascenario: the old grep could not match it, so the append created a duplicate and HugeConfig silently keptpa.