feat(docker): add attachable Hubble Compose add-on for the 3-node cluster - #3149
feat(docker): add attachable Hubble Compose add-on for the 3-node cluster#3149bitflicker64 wants to merge 15 commits into
Conversation
Sorry, something went wrong.
…ster Add docker-compose-hubble.yml, a Hubble-only add-on that joins the cluster's pre-created external network (HUGEGRAPH_NETWORK, default hugegraph-net) with no depends_on, so attaching, upgrading, or removing Hubble never recreates PD, Store, or Server containers. Attach flow uses an explicit project (-p hugegraph-hubble); the fresh flow brings up cluster plus Hubble in one command with both -f flags. Give the 3-node cluster the Server settings Hubble's PD mode requires: PD registration (HG_SERVER_CLUSTER/USE_PD/REST_URL per replica via a shared env anchor), a required shared auth token secret so tokens validate on every replica, and a required admin password. The Server healthcheck now probes the bound REST URL. Hubble reads the 3x3 topology from hugegraph-hubble-3x3.properties. Document the attach, fresh, and dev-override flows plus migration notes in docker/README.md, update the cluster call sites across the docs, and extend the CI compose contract checks to the cluster file and add-on. Image tags stay on latest until the 1.8.0 release publishes; pin via HUGEGRAPH_VERSION in docker/.env.
…in compose checks Render the combined cluster+Hubble topology with non-default network and version values and assert the overridden network name and all four image tags, so CI fails if any Compose file stops honoring either override. The standalone add-on render keeps asserting the defaults and now strips any runner-level overrides for hermeticity.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #3149 +/- ##
============================================
- Coverage 39.30% 37.74% -1.57%
- Complexity 264 6528 +6264
============================================
Files 771 800 +29
Lines 65915 68821 +2906
Branches 8754 9127 +373
============================================
+ Hits 25910 25977 +67
- Misses 37241 39804 +2563
- Partials 2764 3040 +276 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The Compose add-on has a false-green authentication check and operational failover and attachment gaps that can make a healthy-looking deployment unusable. Evidence: exact-head static review across six independent lanes; docker-entrypoint.sh:93-98 requires a 32-byte token, Hubble LiveOperationsCollector uses one pd.server, and the add-on has no data volume.
Sorry, something went wrong.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The cluster quickstart currently fails after generating its own credentials, and the Hubble add-on mounts the H2 database outside the path used by Hubble; additional deployment and CI gaps can leave a false-green or state-inconsistent setup. Evidence: exact-head static review across six independent lanes plus Docker Compose render and guard checks; live container startup was unavailable.
Sorry, something went wrong.
| hostname: server0 | ||
| environment: | ||
| <<: *server-env | ||
| HG_SERVER_REST_URL: http://server0:8080 |
There was a problem hiding this comment.
⚠️ Each Server registers http://serverN:8080 with PD while the host publishes ports 8080/8081/8082. Evidence: Server registration uses restserver.url, so a PD-aware client outside the Docker network receives server0/server1/server2, names that only resolve inside hugegraph-net. Provide a separate configurable advertised address or make the external client path use addresses it can resolve.
Sorry, something went wrong.
There was a problem hiding this comment.
Agree for PD-aware clients outside Docker DNS. Hubble on hugegraph-net can use serverN names as-is. Configurable advertised addresses feel like a follow-up to external-client addressing, not required for the add-on path. What do you think?
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in c311557 rather than deferred.
Each replica's registered URL is now ${HUGEGRAPH_SERVER0_REST_URL:-http://server0:8080} and siblings, so a PD-aware client outside Docker can be given addresses it can resolve. The defaults are unchanged, so Hubble and anything else on hugegraph-net behave exactly as before.
The README explains when the container-name defaults are wrong and that widening the Server ports is part of that setup.
Sorry, something went wrong.
There was a problem hiding this comment.
Correction to my earlier reply on this thread: the fix I described does not work and has been removed in 9616037.
HG_SERVER_REST_URL becomes restserver.url, which is the address the REST server binds, not only the one registered with PD. Running the cluster with the documented override crash-looped that replica on java.net.BindException: Cannot assign requested address while the two default replicas stayed healthy.
Your original point stands: this needs a separate advertised-address setting in the Server. The README now documents the limitation and points external clients at the published ports instead of PD discovery.
Sorry, something went wrong.
There was a problem hiding this comment.
Leaving this one open deliberately, since it is the only finding I did not actually satisfy.
You asked for a configurable advertised address, or for the external client path to use addresses it can resolve. Neither is possible from the Compose files: HG_SERVER_REST_URL becomes restserver.url, which is also the address the REST server binds, so an externally resolvable value crash-loops the replica. Splitting bind from advertise needs a Server-side setting.
What shipped is the limitation documented, with external clients pointed at the published ports instead of PD discovery. Your call whether that is an acceptable close or whether it should become a Server issue.
Sorry, something went wrong.
Read docker/.env as data instead of sourcing it as shell, and take the quoted value rather than the optional export capture when validating generated credentials. Point Hubble H2 at a file inside the /hubble/db volume, give attach and combined flows the same explicit volume names, and assert PD/auth env on every Server replica plus pd.enabled=true.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Four previously reported deployment blockers remain on this head: incompatible cached images can leave authentication false-green, PD/Store control-plane ports remain exposed without real authentication, PD-registered Server URLs are not resolvable outside Docker DNS, and CI remains render-only. Evidence: exact head 950f645; 21 GitHub checks passed; fresh Docker Compose render, credential guard, topology, and dotenv-parser checks passed locally; six independent review lanes were unavailable because the local review gateway repeatedly failed.
Sorry, something went wrong.
Hubble supports auth.enabled=false, so the cluster file no longer requires PASSWORD. Match Hubble to the anonymous Servers and keep PD registration for discovery.
The 3-node cluster is anonymous. Remove agent-memory edits that still required docker/.env, and describe Hubble auth.enabled=false without citing toolchain fork PRs.
…ach" The 3-node cluster is the quickstart most people copy, so it keeps authentication on by default. Running it anonymously is still supported, but as an explicit opt-in documented alongside the default flow rather than as the default itself. This restores the required admin password and JWT token secret, the credential setup and validation steps in the docker README, and the matching CI assertions. The agent-memory files removed by the follow-up commit stay out of the diff.
The 3-node cluster could report healthy while running unauthenticated. The images default to a floating tag with pull_policy: missing, so an older cached image was never refreshed, and that image ignores PASSWORD and the token secret while still answering /versions. Readiness now proves the image enforces authentication: an unauthenticated graph request must return 401 and an authenticated one 200, and all four images pull by default. An incompatible image no longer becomes healthy, so up -d --wait fails instead of handing back a false green. PD and Store publish REST, gRPC and Raft ports, and neither has real authentication. Together with the well-known external network that exposed an unauthenticated control plane on every host interface. Those ports and the Server REST ports now bind to 127.0.0.1, with HUGEGRAPH_CONTROL_PLANE_HOST and HUGEGRAPH_SERVER_PUBLISH_HOST to widen them deliberately. Each Server registers its own REST URL with PD, so a PD-aware client outside Docker received container names it cannot resolve. The registered URLs are configurable through HUGEGRAPH_SERVER0_REST_URL and its siblings, and the README explains when the defaults are wrong. Compose rendering cannot catch any of this, so CI now starts the cluster, attaches Hubble, and asserts the 401/200 pair on all three replicas, PD registration of all three Servers, that attaching Hubble recreates no cluster container, and that the H2 database survives recreation through the shared volumes. Running without authentication stays possible through an explicit opt-in, docker-compose-3x3.non-auth.yml with a matching Hubble properties file, documented next to the default flow along with a prompt for assistants that keeps the authenticated path as the default.
…t proves Running the smoke test against a real cluster showed two of its assertions were wrong. PD's /v1/cluster returns the PD peers, not the graph servers, so grepping it for registered Server addresses always found nothing; /v1/registry answers 405 to GET and 500 to POST, so there is no simple REST proof of Server registration. Assert what PD does report and what actually matters for a distributed deployment: three PD peers and three Stores in state Up. The persistence check only listed the H2 database file, which passes even if the attach and combined flows use different volumes. Write a marker through the attach flow and read it back through the combined flow after Hubble has been recreated, which is the property the explicit volume names exist for.
…eholders The Port Reference section still said cluster ports bind every host interface, which contradicted both the quickstart section and the compose file after the loopback change. It now describes the loopback default and what widening it actually exposes. The non-auth override documented a placeholder token secret that was long enough to be accepted as a real one. Anyone who copied the two placeholder lines into docker/.env and then started the default stack would have got a cluster that looked authenticated while signing tokens with a key published in this repository. Both placeholders are now obvious non-values and the token is deliberately shorter than the 32 bytes the Server requires, so it aborts startup instead. The README and the override header both say to pass them inline and never store them. Also lists the two new files in the file table and documents HUBBLE_DB_VOLUME and HUBBLE_UPLOAD_VOLUME, which were usable but undocumented.
…irst The override shared one YAML anchor across the three Server services. The plain healthcheck override propagates through the alias, but the !reset tags do not on every Compose version: on 5.1.2 only the anchored service loses PASSWORD and HG_SERVER_AUTH_TOKEN_SECRET, so server1 and server2 keep them and come up authenticated while server0 does not. The result is a cluster that is half authenticated, which is worse than either mode on its own, and readiness still passes on all three because the healthcheck override does propagate. The same file renders correctly on 5.1.4, so this depends on the Compose version rather than failing everywhere, which is how it survived a live run. Spell the three services out instead of aliasing them; the CI render check already asserts all three, so a regression fails there.
|
Pushed 6b6c3a3c. All four blocking findings from the last review are fixed in this PR rather than deferred, and the PR description is updated to match.
The cluster also goes back to being authenticated by default, per your point that this is the file people copy. Running without auth is now an explicit opt-in, docker-compose-3x3.non-auth.yml plus a matching Hubble properties file, documented beside the default flow with a copy-paste prompt for people who hand this to an assistant. Tested on a clean Docker host, from zero images: 9/9 healthy, 401/200 on all three replicas, ports on loopback, three PD peers and three Stores Up, Hubble attached without recreating a single cluster container, H2 state surviving recreation across both flows, and the non-auth path returning 200 with no credentials anywhere in the container environment. Testing caught three bugs that rendering never would have, one of them a half-authenticated cluster where only server0 lost its credentials because !reset does not survive a YAML anchor on every Compose version. That one is the clearest argument for the smoke job, so I would rather keep it than trim it. |
Sorry, something went wrong.
bitflicker64
left a comment
There was a problem hiding this comment.
Reviewed the Compose files, the CI additions, and the docs end to end, and ran the render
checks and several of the README shell blocks locally.
The shape of this is good: an add-on file rather than a forked topology, an external
network so attach never recreates the cluster, and readiness that proves authentication
instead of trusting a tag. The CI render checks are unusually thorough.
I think it needs another pass before merge. Most of what I found is in the seams around
that core: teardown and the non-auth flow leave the operator without a working path, two
documented overrides cannot work as described, and one paragraph promises a safety net
across three files that only one of them has. Details inline.
One thing that has no inline anchor: the Healthcheck Endpoints table further down (Server
row) still says GET /versions -> 200 OK. The cluster Server healthcheck is now the
401/200 pair, so that row needs a separate touch; it sits outside the diff.
Minor, take or leave: the combined render's jq is a 21-clause conjunction that fails with
a bare exit 1 naming no clause, while the same function already has named-error helpers
(assert_guard, assert_props); and the token_fixture length check guards a literal
two lines above it, so it can only fire if someone edits that literal.
Sorry, something went wrong.
Self-review found the advertised-address option cannot work. HG_SERVER_REST_URL becomes restserver.url, which is the address the REST server binds, not only the one registered with PD. A Server given an externally reachable address exits with java.net.BindException and restarts forever, confirmed by running the cluster with the documented override: that replica crash-looped while the two defaults stayed healthy. Separating bind from advertise needs a Server-side setting, so the three overrides are removed and the limitation is documented instead. The Server healthcheck now asserts only that an unauthenticated graph request returns 401. That is what proves the image enforces authentication, and unlike the authenticated half it stays correct after an operator rotates the admin password through the API, which would otherwise leave every replica permanently unhealthy on a working cluster. The shared Hubble volumes are external now. A fixed name is not enough: Compose removes a fixed-name non-external volume on `down -v` from any project that declares the name, so leaving the attach flow destroyed the combined flow's H2 database. The setup block creates them alongside the network, and the H2 path is absolute so it cannot drift outside the mount. Also: the image pull policy is overridable, since `always` with no escape made the cluster unusable offline even with every image cached; the non-auth flow documents the network, volumes and its own teardown, which the guards demand on `down` too; the dotenv reader rejects unquoted values instead of returning an empty password, and its block runs in a subshell so a failure cannot close an interactive shell; CI unsets the operator-facing variables it asserts defaults for, covers the non-auth properties with the same topology contract, and runs the live smoke test only when this contract changes, since it boots published images and should not redden unrelated pull requests.
|
Fixed in 9616037, verified against a real cluster. The advertised-address point is the important one and you were right: HG_SERVER_REST_URL becomes restserver.url, which is the bind address. Running the cluster with the documented override crash-looped that replica on java.net.BindException: Cannot assign requested address while the two defaults stayed healthy. The three variables are gone and the README documents the limitation instead. My earlier reply on that thread was wrong. Also fixed: healthcheck no longer pins to the seeded password (401-only, so password rotation cannot brick readiness), Hubble volumes are external so down -v from either flow cannot destroy shared state, pull policy is overridable for offline use, the non-auth flow documents network/volumes/teardown, the dotenv reader rejects unquoted values and runs in a subshell, and CI unsets the variables it asserts defaults for, covers the non-auth properties, and gates the smoke test to contract changes. |
Sorry, something went wrong.
The version-pinning paragraph opened by naming the cluster, the add-on and the single-node quickstart, then concluded that an image which ignores PASSWORD never reports healthy. Only the 3-node cluster has that check. The single-node files probe /versions, which stays open whether authentication works or not, so a reader who pinned a version exactly as instructed got no protection there and no warning about it. The same paragraph also still described the old healthcheck, which required an authenticated 200 alongside the 401. It asserts only the 401 now.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The documented no-credentials attach flow still requires external Hubble volumes to exist in advance. Evidence: exact-head review of the Compose file and README; the attach path is actionable but approval is withheld because the required independent lanes were unavailable and a latest-head CI job is failing.
Sorry, something went wrong.
Making the volumes external fixed a data-loss bug but broke the flow that needs them most. Compose refuses to start a service whose external volume is absent, and the attach flow exists for the case where someone else owns the cluster, so the reader never ran the setup block that creates them: external volume "hugegraph-hubble-upload-files" not found The section now creates both volumes before attaching, honouring HUBBLE_DB_VOLUME and HUBBLE_UPLOAD_VOLUME when the cluster owner renamed them. The block is inspect-then-create, so it is safe to repeat and never touches an existing volume. Verified against a real cluster: the attach fails without it, and Hubble reaches healthy with it.
|
Pushed d2a12bc1. Commit-by-commit, since this branch has grown a lot since the last review. 0d1e67ba: back to authenticated by default. The two commits before it had made the 3-node cluster anonymous. Reverted on top rather than rewritten, so every thread here keeps its anchor. Anonymous is still supported, as an explicit opt-in rather than the default. c3115579: the four blocking findings. Images pull by default and readiness proves the running image enforces auth, so a cached older image cannot look healthy while serving the graph APIs open. PD, Store and Server ports bind to loopback, widened only through HUGEGRAPH_CONTROL_PLANE_HOST / HUGEGRAPH_SERVER_PUBLISH_HOST. And a live smoke job replaces the render-only checks. 10c6c6b5: corrections the smoke job found in itself. Running it against a real cluster showed two of its own assertions were wrong: /v1/cluster returns PD peers, not graph servers, so the registration check could never pass; and the persistence check passed even when the two flows used different volumes. Now it asserts three PD peers and three Stores Up, and proves shared state by writing a marker through one flow and reading it back through the other. 7ea0efe2: the port reference contradicted the code. It still said cluster ports bind every interface after they had moved to loopback. Also, the documented no-auth placeholder token was long enough to pass validation, so copying those two lines into docker/.env would produce a cluster that looked authenticated while signing tokens with a key printed in this repo. The placeholder is now deliberately too short for the Server to accept. 6b6c3a3c: the no-auth override only disabled auth on one Server. The three services shared a YAML anchor, and !reset does not survive an alias on every Compose version: on 5.1.2 only server0 lost its credentials, so the cluster came up half authenticated, while 5.1.4 reset all three. The healthcheck override propagated either way, which is what hid it. Spelled out per service. 96160375: the seams around the contract. Chiefly: HG_SERVER_REST_URL becomes restserver.url, which is the bind address, so the advertised-address option crash-looped a replica on java.net.BindException; those variables are gone and the limitation is documented. Readiness no longer pins to the seeded admin password, which would have bricked every replica after a password rotation. Hubble's volumes became external, because Compose deletes a fixed-name volume on down -v from any project declaring the name, so leaving one flow destroyed the other's database. Plus the offline pull escape hatch, the no-auth flow's missing network and teardown, a dotenv reader that returned an empty password for malformed values, and a verification block whose exit 1 closed the operator's shell. 05715714: a claim that covered files it did not. The version-pinning paragraph promised an image ignoring PASSWORD never reports healthy across three Compose files. Only the cluster has that check; the single-node files probe /versions, which stays open either way. d2a12bc1: your latest finding. Making the volumes external fixed data loss but broke the flow that needs them most: external volume "hugegraph-hubble-upload-files" not found. The attach section now creates both first, honouring HUBBLE_DB_VOLUME and HUBBLE_UPLOAD_VOLUME, inspect-then-create so it is safe to repeat. Reproduced the failure and confirmed the fix against a running cluster. Everything above was verified by running it, not by reading. On the current head: ten containers healthy, 401 unauthenticated and 200 authenticated on all three replicas, wrong password 401, three PD peers and three Stores Up, the H2 database inside its mount, Hubble attaching without recreating a cluster container, and shared state surviving the teardown that used to destroy it. One note on CI: docker-build (Dockerfile-hstore) is red, and it is a Maven build inside that Dockerfile. This branch changes no Java, no pom and no Dockerfile, and the same job passes on current master, so I believe it is unrelated to this diff. This push re-runs it. |
Sorry, something went wrong.
|
@imbajin one open question before this is mergeable, on the PD-registered Server addresses. The inline thread is marked outdated (the lines it pointed at are gone), so raising it here where it is visible: #3149 (comment) Your ask was a configurable advertised address, or an external client path that resolves. I could not do either from Compose: HG_SERVER_REST_URL becomes restserver.url, which is also the address the REST server binds, so an externally reachable value crash-loops the replica on java.net.BindException: Cannot assign requested address. I confirmed that by running it. Splitting bind from advertise needs a Server-side setting. So, which way do you want it closed? A. Accept it as a documented limitation. External clients use the published Server ports; PD discovery stays correct for anything on hugegraph-net, which is the Hubble case this add-on exists for. B. I open a Server issue for a real advertised-address option and link it here. I lean A for this PR, since B is a Server change that would hold up the add-on. Happy to file B regardless if you want it tracked. |
Sorry, something went wrong.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The documented Hubble no-auth flow does not disable Hubble's own API authentication, and the pull-policy and volume-management instructions need correction. Evidence: exact-head static review of the Hubble configuration and README; Compose configuration rendering passed; latest ordinary CI completed with only the non-blocking codecov/project failure; required independent review lane evidence is incomplete.
Sorry, something went wrong.
| pd.enabled=true | ||
| # Hubble's code default is auth.enabled=true. Without this line the UI would | ||
| # demand a login against Servers that have no users configured. | ||
| auth.enabled=false |
There was a problem hiding this comment.
⚠️ The documented no-auth flow cannot disable Hubble login. Evidence: current apache/hugegraph-toolchain master still registers LoginInterceptor for /api/** and rejects requests without a session token; it has no auth.enabled option or consumer, while this file only adds auth.enabled=false. Starting with this properties file therefore still leaves Hubble's API behind its login, so the advertised no-auth flow cannot operate as documented. Implement a supported Hubble auth-disable setting or remove and document this unsupported Hubble no-auth path.
Sorry, something went wrong.
| | `HUBBLE_DB_VOLUME` | add-on | `hugegraph-hubble-db` | Volume holding Hubble's H2 database; the explicit name is what lets the attach and combined flows share state, so change it only to run a second independent Hubble | | ||
| | `HUBBLE_UPLOAD_VOLUME` | add-on | `hugegraph-hubble-upload-files` | Volume holding Hubble's uploaded files; same naming caveat as `HUBBLE_DB_VOLUME` | | ||
| | `HUGEGRAPH_NETWORK` | cluster, add-on | `hugegraph-net` | Pre-created external Docker network shared by the 3-node cluster and the Hubble add-on; the single-node files use their own project bridge instead | | ||
| | `HUGEGRAPH_PULL_POLICY` | cluster | `always` | Pull policy for the PD, Store, and Server images. The default refreshes a stale cached `latest`; set it to `missing` to run offline or against locally built tags | |
There was a problem hiding this comment.
🧹 missing does not guarantee offline use. Evidence: Docker Compose pulls an image when it is absent under pull_policy: missing, while this table says HUGEGRAPH_PULL_POLICY=missing runs offline; the same guide uses never for local images. Document never for strict offline or local use and reserve missing for pull-if-absent.
Sorry, something went wrong.
| # Hubble's volumes are external for the same reason the network is: both | ||
| # documented flows share them, and an external volume is not destroyed by a | ||
| # `down -v` from either one. | ||
| for vol in "${HUBBLE_DB_VOLUME:-hugegraph-hubble-db}" \ |
There was a problem hiding this comment.
⚠️ The quickstart volume setup ignores custom names stored in docker/.env. Evidence: this shell loop expands HUBBLE_DB_VOLUME and HUBBLE_UPLOAD_VOLUME from the process environment, but Compose later reads those variables from .env; setting either there leaves only the default volume created and combined up fails with an external-volume-not-found error. Export these variables before the block or parse the .env values here.
Sorry, something went wrong.
| # Hubble's volumes are external, so no `down` removes them. Drop them | ||
| # explicitly when you want the H2 database and uploaded files gone, or a | ||
| # later deployment reattaches the old state. | ||
| docker volume rm hugegraph-hubble-db hugegraph-hubble-upload-files |
There was a problem hiding this comment.
🧹 This teardown only removes the default volume names. Evidence: the add-on supports HUBBLE_DB_VOLUME and HUBBLE_UPLOAD_VOLUME, but this command hard-codes the defaults, so a customized deployment retains its H2 and upload data despite the instruction to drop them. Use the same variables with defaults or say custom volumes must be removed separately.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
What this PR does
Adds an attachable Hubble deployment for the 3-node Compose cluster, and gives that cluster the authenticated defaults Hubble needs.
Design
Review findings addressed
All four items previously listed here as follow-ups are now in this PR:
Breaking change
Existing 3-node deployments: create docker/.env with an admin password and a token secret before running any Compose command against an older stack, down included, because the :? guards fire on every subcommand. The first up -d after this change recreates all nine containers (network move; named volumes survive). Graph APIs now require credentials. To keep an unauthenticated cluster, use the non-auth override described in the README.
Validation
Run on a 16 GB Linux host from a clean Docker state (no images, no containers), against latest, which currently resolves to 1.7.0.
One bug found this way was mine. The non-auth override first shared a YAML anchor across the three Server services, and !reset does not survive an anchor reference on every Compose version: on 5.1.2 only server0 lost its credentials, so the cluster came up half authenticated, while 5.1.4 reset all three. The live run passed because it happened to use 5.1.4. The services are now spelled out separately and the render check asserts all three.