bitflicker64 commented on code in PR #3149: URL: https://github.com/apache/hugegraph/pull/3149#discussion_r3789894380
########## docker/docker-compose-3pd-3store-3server.yml: ########## @@ -31,6 +36,8 @@ volumes: # ── Shared service defaults ────────────────────────────────────────── x-pd-common: &pd-common + # Pin a release via HUGEGRAPH_VERSION in docker/.env; unset, the image Review Comment: Agree this is a real gap for the auth-required 3-node quickstart. I’d like to keep #3149 scoped to the Hubble add-on and the minimum cluster contract Hubble needs. Changing Server readiness to prove 401/200 and changing default pull policy feels like follow-up cluster hardening rather than the add-on itself. README already documents the manual 401/200 check for incompatible images. ########## docker/docker-compose-3pd-3store-3server.yml: ########## @@ -17,9 +17,14 @@ name: hugegraph-3x3 +# The cluster network is shared with the Hubble add-on +# (docker-compose-hubble.yml), so it is external and must exist first: +# docker network create hugegraph-net +# Set HUGEGRAPH_NETWORK to use a differently named network. networks: hg-net: - driver: bridge + external: true Review Comment: Agree on the exposure concern. Default loopback binds / ACL+TLS requirements are a cluster-quickstart security redesign; I’d track that as a follow-up so this PR stays reviewable as a Hubble add-on. ########## docker/docker-compose-hubble.yml: ########## @@ -0,0 +1,53 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# Hubble add-on for the distributed cluster defined in +# docker-compose-3pd-3store-3server.yml. Requires the pre-created cluster +# network (docker network create hugegraph-net). See "Hubble for the +# 3-Node Cluster" in docker/README.md for the attach and combined flows. + +networks: + hg-net: + external: true + name: ${HUGEGRAPH_NETWORK:-hugegraph-net} + +volumes: + hg-hubble-db: Review Comment: Agreed — in scope. I’ll give the Hubble DB/upload volumes explicit stable names so attach and combined share the same physical volumes. ########## docker/README.md: ########## @@ -196,11 +296,184 @@ curl http://localhost:8520/v1/health # Check Server (Graph API) curl http://localhost:8080/versions -# List registered stores via PD -curl http://localhost:8620/v1/stores +# Every PD endpoint except /v1/health, /actuator/* and /v1/prom/targets/* +# needs an Authorization header. PD only checks that the Basic-auth user is +# one of its internal service names, so the empty password below is enough +# — and it grants the full PD control plane, writes included, not just these +# reads. That is exactly why the shared network must be treated as a trust +# boundary. Without the header PD answers with an exception body, not data. +pd_auth="Authorization: Basic $(printf 'hubble:' | base64)" + +# List registered stores via PD (expect three, each "state":"Up") +curl -H "${pd_auth}" http://localhost:8620/v1/stores # List partitions -curl http://localhost:8620/v1/partitions +curl -H "${pd_auth}" http://localhost:8620/v1/partitions +``` + +Confirm authentication actually engaged — `/versions` stays open by design, +so it cannot tell you whether auth is on. A graph read without credentials +must be rejected: + +```bash +cd docker +# Expect 401 on all three replicas +for port in 8080 8081 8082; do + curl -s -o /dev/null -w "${port}: %{http_code}\n" \ + "http://localhost:${port}/graphs/hugegraph/schema/vertexlabels"; +done + +# And a signed-in read must succeed. Compose reads docker/.env by itself, but +# your shell does not — load it first. Passing the credential through +# --config keeps it out of argv, where `ps` would expose it to other users. +set -a; . ./.env; set +a Review Comment: Agreed — in scope. I’ll replace `set -a; . ./.env; set +a` with a parse-only read of the required key so the verify block does not execute dotenv as shell. ########## docker/docker-compose-3pd-3store-3server.yml: ########## @@ -187,16 +207,25 @@ services: <<: *server-common container_name: hg-server0 hostname: server0 + environment: + <<: *server-env + HG_SERVER_REST_URL: http://server0:8080 Review 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? ########## .github/workflows/server-ci.yml: ########## @@ -138,6 +140,189 @@ jobs: check_compose docker/docker-compose.yml always always check_compose docker/docker-compose.dev.yml build missing + check_cluster_compose() { + local cluster="docker/docker-compose-3pd-3store-3server.yml" + local addon="docker/docker-compose-hubble.yml" + local rendered + local token_fixture=ci-test-token-secret-32-bytes-long + rendered="$(mktemp)" + if [ "${#token_fixture}" -lt 32 ]; then + echo "CI token fixture must be at least 32 bytes" >&2 + return 1 + fi + + # RETURN only: an EXIT trap would fire after this function's + # `local rendered` has gone out of scope, which `set -u` turns + # into an "unbound variable" error. A hard errexit abort can + # therefore still leak one temp file, which is acceptable on an + # ephemeral runner. + trap 'rm -f "$rendered"' RETURN + + # --env-file /dev/null on every invocation: Compose otherwise reads + # docker/.env automatically, and the quickstart tells operators to + # create one holding exactly the credentials these assertions + # control. Without it the checks pass in CI (which has no .env) but + # report false failures for anyone running them locally after + # following the quickstart, and the add-on render below — which + # deliberately unsets the variables to assert their defaults — would + # read whatever HUGEGRAPH_NETWORK/HUGEGRAPH_VERSION that .env holds. + # Pinning an empty env file makes both renders depend only on what + # each invocation sets explicitly. + + # Both cluster credentials are required and may not be empty. + # Each case asserts the guard fired for the *intended* variable: + # a bare non-zero exit would also be produced by a YAML error, a + # renamed file, or a missing docker binary. + assert_guard() { # assert_guard <blamed-var> <description> + local var="$1" desc="$2" err + if err="$(docker compose --env-file /dev/null -f "$cluster" \ + config -q 2>&1)"; then + echo "$cluster accepted $desc" >&2 + return 1 + fi + case "$err" in + *"$var"*) : ;; + *) echo "$cluster rejected $desc, but not because of $var: $err" >&2 + return 1 ;; + esac + } + + ( unset HUGEGRAPH_ADMIN_PASSWORD + export HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" + assert_guard HUGEGRAPH_ADMIN_PASSWORD "an unset admin password" ) + ( export HUGEGRAPH_ADMIN_PASSWORD= + export HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" + assert_guard HUGEGRAPH_ADMIN_PASSWORD "an empty admin password" ) + ( unset HUGEGRAPH_AUTH_TOKEN_SECRET + export HUGEGRAPH_ADMIN_PASSWORD=ci-test-password + assert_guard HUGEGRAPH_AUTH_TOKEN_SECRET "an unset token secret" ) + ( export HUGEGRAPH_ADMIN_PASSWORD=ci-test-password + export HUGEGRAPH_AUTH_TOKEN_SECRET= + assert_guard HUGEGRAPH_AUTH_TOKEN_SECRET "an empty token secret" ) + # The add-on alone must define Hubble and nothing else, join the + # shared external network with its default name, and need no + # credentials or overrides. + env -u HUGEGRAPH_ADMIN_PASSWORD -u HUGEGRAPH_AUTH_TOKEN_SECRET \ + -u HUGEGRAPH_NETWORK -u HUGEGRAPH_VERSION \ + -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$addon" config --format json > "$rendered" + jq -e ' + (.services | keys) == ["hubble"] and + .networks."hg-net".external == true and + .networks."hg-net".name == "hugegraph-net" and + (.services.hubble.networks | has("hg-net")) and + (.services.hubble | has("depends_on") | not) and + any(.services.hubble.volumes[]; + .target == "/hubble/conf/hugegraph-hubble.properties" and + (.source | endswith("hugegraph-hubble-3x3.properties"))) + and any(.services.hubble.volumes[]; + .source == "hg-hubble-db" and + .target == "/hubble/db") + and any(.services.hubble.volumes[]; + .source == "hg-hubble-upload-files" and + .target == "/hubble/upload-files") + ' "$rendered" >/dev/null + + # The cluster's own default network name must match the add-on's, + # or the attach flow and the cluster land on different networks. + # The combined render below pins an override, so it cannot catch a + # drifting default. + HUGEGRAPH_ADMIN_PASSWORD=ci-test-password \ + HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" \ + env -u HUGEGRAPH_NETWORK -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY \ + -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$cluster" \ + config --format json > "$rendered" + jq -e '.networks."hg-net".name == "hugegraph-net"' \ + "$rendered" >/dev/null + + # The combined render carries the PD-registration and auth + # settings on every server replica and keeps Hubble on loopback. + # Rendered with non-default HUGEGRAPH_NETWORK/HUGEGRAPH_VERSION so + # CI fails if any file stops honoring the overrides (the add-on + # render above covers the defaults). + HUGEGRAPH_ADMIN_PASSWORD=ci-test-password \ + HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" \ + HUGEGRAPH_NETWORK=ci-test-net \ + HUGEGRAPH_VERSION=ci-test-tag \ + env -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$cluster" -f "$addon" \ + config --format json > "$rendered" + jq -e ' + .name == "hugegraph-3x3" and + (.services | keys | length) == 10 and + .networks."hg-net".external == true and + .networks."hg-net".name == "ci-test-net" and + all(.services[]; .networks | has("hg-net")) and + .services.pd0.image == "hugegraph/pd:ci-test-tag" and + .services.store0.image == "hugegraph/store:ci-test-tag" and + .services.server0.image == "hugegraph/server:ci-test-tag" and + .services.hubble.image == "hugegraph/hubble:ci-test-tag" and + .services.server0.environment.HG_SERVER_USE_PD == "true" and Review Comment: Agreed — in scope. This PR introduced the PD/auth Server env for Hubble discovery, so CI should assert the required values on `server0`/`server1`/`server2`, not only `server0`. ########## .github/workflows/server-ci.yml: ########## @@ -138,6 +140,189 @@ jobs: check_compose docker/docker-compose.yml always always check_compose docker/docker-compose.dev.yml build missing + check_cluster_compose() { + local cluster="docker/docker-compose-3pd-3store-3server.yml" + local addon="docker/docker-compose-hubble.yml" + local rendered + local token_fixture=ci-test-token-secret-32-bytes-long + rendered="$(mktemp)" + if [ "${#token_fixture}" -lt 32 ]; then + echo "CI token fixture must be at least 32 bytes" >&2 + return 1 + fi + + # RETURN only: an EXIT trap would fire after this function's + # `local rendered` has gone out of scope, which `set -u` turns + # into an "unbound variable" error. A hard errexit abort can + # therefore still leak one temp file, which is acceptable on an + # ephemeral runner. + trap 'rm -f "$rendered"' RETURN + + # --env-file /dev/null on every invocation: Compose otherwise reads + # docker/.env automatically, and the quickstart tells operators to + # create one holding exactly the credentials these assertions + # control. Without it the checks pass in CI (which has no .env) but + # report false failures for anyone running them locally after + # following the quickstart, and the add-on render below — which + # deliberately unsets the variables to assert their defaults — would + # read whatever HUGEGRAPH_NETWORK/HUGEGRAPH_VERSION that .env holds. + # Pinning an empty env file makes both renders depend only on what + # each invocation sets explicitly. + + # Both cluster credentials are required and may not be empty. + # Each case asserts the guard fired for the *intended* variable: + # a bare non-zero exit would also be produced by a YAML error, a + # renamed file, or a missing docker binary. + assert_guard() { # assert_guard <blamed-var> <description> + local var="$1" desc="$2" err + if err="$(docker compose --env-file /dev/null -f "$cluster" \ + config -q 2>&1)"; then + echo "$cluster accepted $desc" >&2 + return 1 + fi + case "$err" in + *"$var"*) : ;; + *) echo "$cluster rejected $desc, but not because of $var: $err" >&2 + return 1 ;; + esac + } + + ( unset HUGEGRAPH_ADMIN_PASSWORD + export HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" + assert_guard HUGEGRAPH_ADMIN_PASSWORD "an unset admin password" ) + ( export HUGEGRAPH_ADMIN_PASSWORD= + export HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" + assert_guard HUGEGRAPH_ADMIN_PASSWORD "an empty admin password" ) + ( unset HUGEGRAPH_AUTH_TOKEN_SECRET + export HUGEGRAPH_ADMIN_PASSWORD=ci-test-password + assert_guard HUGEGRAPH_AUTH_TOKEN_SECRET "an unset token secret" ) + ( export HUGEGRAPH_ADMIN_PASSWORD=ci-test-password + export HUGEGRAPH_AUTH_TOKEN_SECRET= + assert_guard HUGEGRAPH_AUTH_TOKEN_SECRET "an empty token secret" ) + # The add-on alone must define Hubble and nothing else, join the + # shared external network with its default name, and need no + # credentials or overrides. + env -u HUGEGRAPH_ADMIN_PASSWORD -u HUGEGRAPH_AUTH_TOKEN_SECRET \ + -u HUGEGRAPH_NETWORK -u HUGEGRAPH_VERSION \ + -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$addon" config --format json > "$rendered" + jq -e ' + (.services | keys) == ["hubble"] and + .networks."hg-net".external == true and + .networks."hg-net".name == "hugegraph-net" and + (.services.hubble.networks | has("hg-net")) and + (.services.hubble | has("depends_on") | not) and + any(.services.hubble.volumes[]; + .target == "/hubble/conf/hugegraph-hubble.properties" and + (.source | endswith("hugegraph-hubble-3x3.properties"))) + and any(.services.hubble.volumes[]; + .source == "hg-hubble-db" and + .target == "/hubble/db") + and any(.services.hubble.volumes[]; + .source == "hg-hubble-upload-files" and + .target == "/hubble/upload-files") + ' "$rendered" >/dev/null + + # The cluster's own default network name must match the add-on's, + # or the attach flow and the cluster land on different networks. + # The combined render below pins an override, so it cannot catch a + # drifting default. + HUGEGRAPH_ADMIN_PASSWORD=ci-test-password \ + HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" \ + env -u HUGEGRAPH_NETWORK -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY \ + -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$cluster" \ + config --format json > "$rendered" + jq -e '.networks."hg-net".name == "hugegraph-net"' \ + "$rendered" >/dev/null + + # The combined render carries the PD-registration and auth + # settings on every server replica and keeps Hubble on loopback. + # Rendered with non-default HUGEGRAPH_NETWORK/HUGEGRAPH_VERSION so + # CI fails if any file stops honoring the overrides (the add-on + # render above covers the defaults). + HUGEGRAPH_ADMIN_PASSWORD=ci-test-password \ + HUGEGRAPH_AUTH_TOKEN_SECRET="${token_fixture}" \ + HUGEGRAPH_NETWORK=ci-test-net \ + HUGEGRAPH_VERSION=ci-test-tag \ + env -u HUBBLE_IMAGE -u HUBBLE_PULL_POLICY -u HUBBLE_PUBLISH_HOST \ + docker compose --env-file /dev/null -f "$cluster" -f "$addon" \ + config --format json > "$rendered" + jq -e ' + .name == "hugegraph-3x3" and + (.services | keys | length) == 10 and + .networks."hg-net".external == true and + .networks."hg-net".name == "ci-test-net" and + all(.services[]; .networks | has("hg-net")) and + .services.pd0.image == "hugegraph/pd:ci-test-tag" and + .services.store0.image == "hugegraph/store:ci-test-tag" and + .services.server0.image == "hugegraph/server:ci-test-tag" and + .services.hubble.image == "hugegraph/hubble:ci-test-tag" and + .services.server0.environment.HG_SERVER_USE_PD == "true" and + .services.server0.environment.HG_SERVER_CLUSTER == "hg" and + .services.server0.environment.HG_SERVER_REST_URL == + "http://server0:8080"; and + .services.server1.environment.HG_SERVER_REST_URL == + "http://server1:8080"; and + .services.server1.environment.HG_SERVER_AUTH_TOKEN_SECRET == + "ci-test-token-secret-32-bytes-long" and + .services.server2.environment.HG_SERVER_REST_URL == + "http://server2:8080"; and + .services.server2.environment.HG_SERVER_AUTH_TOKEN_SECRET == + "ci-test-token-secret-32-bytes-long" and + .services.server0.environment.HG_SERVER_INIT_STORE_ENABLED == + "false" and + .services.server0.environment.PASSWORD == + "ci-test-password" and + .services.server0.environment.HG_SERVER_AUTH_TOKEN_SECRET == + "ci-test-token-secret-32-bytes-long" and + (.services.hubble | has("depends_on") | not) and + .services.hubble.pull_policy == "missing" and + (.services.hubble.healthcheck.test[1] | + contains("http://127.0.0.1:8088/about";) and + contains("\"status\":200") and + contains("\"name\":\"hugegraph-hubble\"")) and + any(.services.hubble.ports[]; + .target == 8088 and .published == "8088" and + .host_ip == "127.0.0.1") + ' "$rendered" >/dev/null + + # Hubble's properties file is mounted, not rendered, so Compose + # validation alone cannot catch it drifting from the services it + # describes. Tie the two together: renaming a service, changing a + # container hostname, or moving a REST port must be reflected in + # both places or CI fails here. Values are derived from the + # rendered model (hostnames and ports included) so the assertions + # cannot silently agree with a stale file. + local props="docker/hugegraph-hubble-3x3.properties" + local pd_peers store_targets cluster_name pd_rest + assert_props() { # assert_props <exact-line> <what-it-must-match> + grep -Fqx "$1" "$props" || + { echo "$props: expected line '$1' ($2)" >&2; return 1; } + } + cluster_name="$(jq -r '.services.server0.environment.HG_SERVER_CLUSTER' \ + "$rendered")" + pd_peers="$(jq -r '.services.server0.environment.HG_SERVER_PD_PEERS' \ + "$rendered")" + store_targets="[$(jq -r '[.services | to_entries[] + | select(.key | startswith("store")) + | "http://"; + .value.hostname + ":" + + (.value.environment.HG_STORE_REST_PORT)] + | sort | join(",")' "$rendered")]" + pd_rest="$(jq -r --arg h "$(printf '%s' "${pd_peers}" | cut -d, -f1 | cut -d: -f1)" \ + '.services | to_entries[] + | select(.value.hostname == $h) + | .value.hostname + ":" + .value.environment.HG_PD_REST_PORT' \ + "$rendered")" + assert_props "cluster=${cluster_name}" "matches HG_SERVER_CLUSTER" Review Comment: Correction: that last reply was meant for the advertised-URL thread. For this one — agreed and in scope. I’ll add an explicit `assert_props "pd.enabled=true"` so the Hubble properties check cannot pass with PD mode disabled. -- 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]
