Duansg commented on PR #4295:
URL: https://github.com/apache/hertzbeat/pull/4295#issuecomment-5422734906
Did a deep review of this PR (diff-first, independent of the description).
Overall the change is technically solid — I found no blocking technical defect
— but there are two substantive points that I think deserve maintainer
consensus before merge, plus a few minor nits. Details below.
## Verified correct
- `${HERTZBEAT_BIND_ADDRESS:-127.0.0.1}` /
`${HERTZBEAT_OTLP_BIND_ADDRESS:-127.0.0.1}` interpolation is correct (`:-` also
falls back on empty string, which is the right choice over `-`). Compose
auto-loads the sibling `.env` for interpolation, so the `.env.example` → `.env`
flow works as documented.
- The CI script asserts loopback bindings for `postgres` (5432→15432) and
`greptime` (4000–4003→14000–14003) which this diff does not touch — I
cross-checked current `master`: those services already carry the `127.0.0.1:`
prefix, so the assertions match reality rather than assuming it.
- `check-quickstart-compose.sh`: POSIX sh, `set -eu`, `mktemp` + trap
cleanup, `unset` before the default render, and the jq types match `docker
compose config --format json` output (`target` numeric via `--argjson`,
`published` string via `--arg`). The `quickstart-config` job passing on the
real runner confirms this empirically.
- Workflow hygiene is good: `permissions: contents: read`, path filters,
concurrency group, 5-minute timeout. `actions/checkout@v4` is consistent with
the other 7 usages in this repo.
- All new files carry ASF license headers; license check is green.
## Points that need a maintainer decision
**1. Defaulting the quick-start to loopback is a user-facing behavior
change, not pure maintenance.**
After this merges, a user who runs `docker compose up -d` on a remote server
can no longer reach `http://<server-ip>:1157` from their workstation browser —
which is arguably the most common deployment pattern for a monitoring system.
The security motivation is real and well-founded (bundled `admin/hertzbeat`
credentials, and Docker-published ports bypassing ufw/firewalld), so I'm
sympathetic to the change — but it currently ships with only the in-directory
READMEs updated. I'd suggest a release-note / upgrade-doc entry so operators
aren't surprised, and an explicit ack from maintainers that the community
accepts secure-by-default here.
**2. Consistency gap: only 1 of 5 compose variants is scoped — and not the
officially recommended one.**
`home/docs/start/docker-compose-deploy.md` recommends
`hertzbeat-postgresql-victoria-metrics`, which still publishes
`1157/1158/14317` on all interfaces — and even its `15432:5432` PostgreSQL
mapping has no loopback prefix. That same doc also instructs users to "Access
http://ip:1157/";. So the security benefit doesn't reach the documented default
path, and after merge the variants behave differently from each other and from
the website docs. Not necessarily a blocker for this PR, but I'd like to see a
follow-up issue tracked to align the remaining four variants and the website
docs — otherwise this hardening stays an island.
## Minor nits (non-blocking)
- Assertion failures are silent: `jq -e` output goes to `/dev/null`, so when
`set -e` aborts, the CI log doesn't say which binding failed. A small error
message per assertion would help.
- Running the script locally with a `.env` present in the compose directory
pollutes the "default" render (the script `unset`s env vars but can't shield
the `.env` file) and produces a false failure. CI is unaffected (clean
checkout); `--env-file /dev/null` would make it robust locally too.
- The workflow's path filter covers all of `script/docker-compose/**`, but
the script validates only the greptimedb variant — changes to the other
variants trigger the job without being checked.
- `host_ip` only accepts an IP literal; a user following the docs' wording
"address" might supply a hostname and get a Compose error. Perhaps say "IP
address" explicitly in `.env.example`/README.
## Suggested path to merge
Maintainer ack on the loopback-by-default decision → file the follow-up for
the remaining variants + website docs + release note → update branch (currently
behind master) → approve & merge. If the community prefers keeping the old
default, the variable mechanism here is still fully reusable with `0.0.0.0` as
the default.
--
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]
