bitflicker64 commented on PR #3132: URL: https://github.com/apache/hugegraph/pull/3132#issuecomment-5155533587
Blocking, and it comes from a change in #3119 rather than from this PR's own code. **What changed.** Review of #3119 found that with `init_store.enabled=false`, the built-in authenticator's admin is created on the PD startup path from `auth.admin_pa`, whose default is the public value `pa` — while Docker `PASSWORD` is discarded, because `init-store` reads it from stdin and the disabled path returns before that. `init-store` now fails closed unless an explicit non-empty `auth.admin_pa` is configured. **What breaks here.** This chart sets `HG_SERVER_INIT_STORE_ENABLED=false` and, when `server.auth.enabled` is true, supplies `PASSWORD` from `server.auth.existingSecret`. The wrapper in `server-deployment.yaml` writes `usePD=true` and `pd.peers` into `conf/rest-server.properties`, but nothing writes `auth.admin_pa`. So once #3119 merges, an auth-enabled deployment of this chart will fail at container start with: ``` Refusing to skip init-store: '...' configures the built-in authenticator but no explicit non-empty 'auth.admin_pa' is configured, so the admin would be created with the public default. ``` Before that change it did not fail — it came up with `admin`/`pa`, silently ignoring the Secret. So this is an existing bug in the chart that #3119 makes visible rather than a new one. **What this PR needs to do.** The wrapper already assembles `rest-server.properties` from the environment; it needs to write `auth.admin_pa` from the same Secret it already mounts as `PASSWORD`, alongside the `usePD` and `pd.peers` lines. That also makes this claim in `helm/hugegraph/README.md` true for the first time: > The image entrypoint keeps ownership of `PASSWORD` handling and `auth.admin_pa`. It does not, on this path — worth rewording once the wrapper does it. Two smaller things to check while touching that block: `auth.admin_pa` applies only when the admin account is first created, so it will not rotate the password on an existing PD cluster, and the value lands in a file inside the container, so it should be written with the same care as any other secret material. Merge order is unchanged: this chart still depends on #3119's env mapping and should land after it. -- 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]
