mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
5f9bff9056
* fix(docker): drop curl from the API runtime images curl's only in-container consumer was the readiness loop in start-all.sh; there is no HEALTHCHECK instruction anywhere. It is also the sole reverse-dependency of libcurl4t64, which brings libssh2-1t64, so one line in each install list accounted for nine HIGH findings - all status=affected with no Debian fix published, so `apt-get upgrade` could not clear them and not shipping the package was the only remediation. Trivy 0.74.0 HIGH+CRITICAL, on locally built slim images: api-only 3C / 60H -> 3C / 51H standalone 3C / 60H -> 3C / 51H with exactly the curl, libcurl4t64 and libssh2-1t64 findings removed and nothing new. Replace it with http_probe, which reproduces `curl -sf` WITHOUT -L rather than approximating it. The distinction matters: curl does not follow redirects unless asked, so a 302 is a completed transfer and succeeds regardless of what it points at. urllib.request.urlopen follows it and raises on a 404 behind it, which would report a healthy service that redirects as "not ready". http_probe uses http.client and tests `status < 400` itself, bypassing urllib's redirect handler, and carries userinfo through as Basic auth the way curl does. Verified equivalent to `curl -sf` on 2xx, 3xx-to-good, 3xx-to-bad, 4xx, 5xx, query strings, userinfo auth and connection refused. Exit codes are not reproduced (curl's 22 and 7 become 1); every call site tests zero/non-zero. One deliberate difference, since it is a change and not a translation: curl was called with --connect-timeout, which caps only the connection phase, and the API health loop passed no timeout at all - so a server that accepted a connection and never answered hung the probe forever. The timeout now covers the whole request. There is no wget fallback. BusyBox wget cannot reproduce these semantics (no --max-redirect, so it always follows), and it is not needed: every image that probes anything is Python-based. cp-only, the one image with neither, performs no probe at all and dropped curl in #4197. Missing python3 now fails loudly at startup instead of degrading into a readiness loop that can never succeed. Closes #4198 * refactor(docker): move the readiness probe into hindsight_api.http_probe The first version of this probe was Python embedded in a shell string inside start-all.sh. That was a bad shape for code encoding rules this fiddly: every quote had to survive two levels of escaping, ruff and ty never saw it, and it could only be exercised through the shell. Move it to hindsight_api/http_probe.py, shipped with the code and covered by tests/test_http_probe.py, which pins each case to what `curl -sf` does for the same response. start-all.sh keeps a three-line wrapper that shells out to `python3 -m hindsight_api.http_probe`. hindsight-admin was the obvious home and is the wrong one: it takes 5.1s to start in the built image, against 0.028s for bare stdlib, because it pulls in the CLI and everything behind it. The readiness loop polls once per second, so importing the API to ask whether the API is up would break the loop it drives. This module imports stdlib only; measured 0.035s per probe in the image. `hindsight_api/__init__` is cheap by design and has to stay that way for this to hold - its docstring already says so. The shell test drops to checking the wiring, since the semantics now have a real home, and skips when the package is not importable: test-start-all.sh also runs in CI from a bare checkout with no virtualenv. Reformatting by `ruff format` on first contact is the point - the embedded version could never have received it. * refactor(probe): make the readiness probe its own package, isolated from the API hindsight_api.http_probe was the wrong home. The probe answers "is an API process up?", and living inside the package it probes invited exactly the coupling that would break it: an import of the engine or the config would put API startup cost - and API startup side effects - on a loop that runs once a second. Move it to hindsight_probe, a sibling top-level package in the same distribution. Its dependencies are now explicit by construction: none. It imports the standard library and nothing else. Packaging alone does not enforce that. Both packages install into the same virtualenv, so `import hindsight_api` from the probe would still resolve at runtime. So the rule is a test, not a convention: test_imports_nothing_but_the_standard_library imports the package in a clean subprocess and asserts that nothing outside sys.stdlib_module_names was pulled in. Adding `import hindsight_api` to the probe fails it with the offending name. The audit ignores _sysconfigdata_*, a platform-specific stdlib internal whose name embeds the build triple and so is absent from stdlib_module_names everywhere. Both Dockerfiles now copy the package; the api-builder previously copied only hindsight_api, so the first build without this shipped an image whose probe could not import. That surfaced as require_http_probe_runtime failing at startup with a clear message rather than a readiness loop that could never succeed, which is what that guard is for. Verified in the built Linux image: no non-stdlib imports, hindsight_api never loaded, 0.036s per probe, and the full end-to-end boot still reaches "Hindsight is running" with /health answering 200. * refactor(probe): keep the readiness probe inside hindsight_api Reverts the separate hindsight_probe package. It was justified on a bad measurement: an earlier cold-cache timing suggested `import hindsight_api` cost ~0.12s against ~0.03s for a standalone package. Measured properly, warm, in the built image, they are the same - ~0.03s each - and importing hindsight_api pulls in zero third-party modules. Its PEP 562 lazy-attribute design already does the work the split was meant to do, so the split bought nothing and cost a second top-level package, four pyproject entries and a COPY in each Dockerfile. What was worth keeping is the enforcement, which is orthogonal to where the module lives. test_imports_nothing_heavy imports the probe in a clean subprocess and asserts it pulled in no third-party package and nothing from hindsight_api.engine, .api or .config. Adding `from hindsight_api.engine import memory_engine` to the probe fails it with 43 packages named, numpy, sqlalchemy and asyncpg among them - which is the failure mode the rule exists to prevent. Verified in the built image: no third-party or engine imports, 0.034s per import, probe wiring works, curl absent.
233 lines
7.7 KiB
Bash
Executable File
233 lines
7.7 KiB
Bash
Executable File
#!/bin/bash
|
|
set -euo pipefail
|
|
|
|
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
|
|
|
HINDSIGHT_START_ALL_SOURCE_ONLY=true
|
|
source "$SCRIPT_DIR/start-all.sh"
|
|
unset HINDSIGHT_START_ALL_SOURCE_ONLY
|
|
|
|
TMP_DIR="$(mktemp -d)"
|
|
HTTP_SERVER_PID=""
|
|
|
|
cleanup() {
|
|
if [ -n "$HTTP_SERVER_PID" ]; then
|
|
kill "$HTTP_SERVER_PID" 2>/dev/null || true
|
|
wait "$HTTP_SERVER_PID" 2>/dev/null || true
|
|
fi
|
|
chmod -R u+rwx "$TMP_DIR" 2>/dev/null || true
|
|
rm -rf "$TMP_DIR"
|
|
}
|
|
trap cleanup EXIT
|
|
|
|
assert_contains() {
|
|
local output="$1"
|
|
local expected="$2"
|
|
|
|
if [[ "$output" != *"$expected"* ]]; then
|
|
echo "Expected output to contain: $expected"
|
|
echo "Actual output:"
|
|
echo "$output"
|
|
exit 1
|
|
fi
|
|
}
|
|
|
|
assert_not_contains() {
|
|
local output="$1"
|
|
local unexpected="$2"
|
|
|
|
if [[ "$output" == *"$unexpected"* ]]; then
|
|
echo "Expected output not to contain: $unexpected"
|
|
echo "Actual output:"
|
|
echo "$output"
|
|
exit 1
|
|
fi
|
|
}
|
|
|
|
assert_empty() {
|
|
local output="$1"
|
|
|
|
if [ -n "$output" ]; then
|
|
echo "Expected no output, got:"
|
|
echo "$output"
|
|
exit 1
|
|
fi
|
|
}
|
|
|
|
# =============================================================================
|
|
# http_probe wiring
|
|
#
|
|
# The probe's semantics are covered by pytest, against the module that
|
|
# implements them: hindsight-api-slim/tests/test_http_probe.py. All that is
|
|
# left to check here is that this script delegates to it correctly.
|
|
#
|
|
# Skipped when hindsight_api.http_probe is not importable - this file also runs in CI
|
|
# from a bare checkout with no virtualenv, where only the pg0 helpers below
|
|
# are exercisable.
|
|
# =============================================================================
|
|
if python3 -c "import hindsight_api.http_probe" >/dev/null 2>&1; then
|
|
HTTP_PORT_FILE="$TMP_DIR/http-port"
|
|
|
|
python3 - "$HTTP_PORT_FILE" <<'PY' &
|
|
import http.server
|
|
import pathlib
|
|
import sys
|
|
|
|
|
|
class Handler(http.server.BaseHTTPRequestHandler):
|
|
def do_GET(self):
|
|
self.send_response(204 if self.path == "/ok" else 404)
|
|
self.end_headers()
|
|
|
|
def log_message(self, *_args):
|
|
pass
|
|
|
|
|
|
server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Handler)
|
|
pathlib.Path(sys.argv[1]).write_text(str(server.server_port), encoding="ascii")
|
|
server.serve_forever()
|
|
PY
|
|
HTTP_SERVER_PID=$!
|
|
|
|
for _ in $(seq 1 50); do
|
|
[ -s "$HTTP_PORT_FILE" ] && break
|
|
sleep 0.1
|
|
done
|
|
if [ ! -s "$HTTP_PORT_FILE" ]; then
|
|
echo "HTTP probe test server did not start"
|
|
exit 1
|
|
fi
|
|
HTTP_TEST_URL="http://127.0.0.1:$(cat "$HTTP_PORT_FILE")"
|
|
|
|
if ! http_probe "$HTTP_TEST_URL/ok" 5 >/dev/null 2>&1; then
|
|
echo "http_probe should succeed against a healthy endpoint"
|
|
exit 1
|
|
fi
|
|
if http_probe "$HTTP_TEST_URL/missing" 5 >/dev/null 2>&1; then
|
|
echo "http_probe should fail against a 404"
|
|
exit 1
|
|
fi
|
|
if ! require_http_probe_runtime; then
|
|
echo "require_http_probe_runtime should pass when the module imports"
|
|
exit 1
|
|
fi
|
|
|
|
kill "$HTTP_SERVER_PID" 2>/dev/null || true
|
|
wait "$HTTP_SERVER_PID" 2>/dev/null || true
|
|
HTTP_SERVER_PID=""
|
|
echo "start-all HTTP probe wiring checks passed"
|
|
else
|
|
echo "start-all HTTP probe wiring checks skipped (hindsight_api.http_probe not importable)"
|
|
fi
|
|
|
|
mkdir -p "$TMP_DIR/empty"
|
|
assert_empty "$(check_pg0_data_integrity "$TMP_DIR/empty")"
|
|
|
|
mkdir -p "$TMP_DIR/direct"
|
|
touch "$TMP_DIR/direct/PG_VERSION"
|
|
direct_output="$(check_pg0_data_integrity "$TMP_DIR/direct")"
|
|
assert_contains "$direct_output" "Existing pg0 data directory detected"
|
|
assert_not_contains "$direct_output" "WARNING"
|
|
|
|
mkdir -p "$TMP_DIR/legacy/instance"
|
|
touch "$TMP_DIR/legacy/instance/PG_VERSION"
|
|
legacy_output="$(check_pg0_data_integrity "$TMP_DIR/legacy")"
|
|
assert_contains "$legacy_output" "Existing pg0 data directory detected"
|
|
assert_not_contains "$legacy_output" "WARNING"
|
|
|
|
mkdir -p "$TMP_DIR/nested/instances/hindsight/data"
|
|
touch "$TMP_DIR/nested/instances/hindsight/data/PG_VERSION"
|
|
nested_output="$(check_pg0_data_integrity "$TMP_DIR/nested")"
|
|
assert_contains "$nested_output" "Existing pg0 data directory detected"
|
|
assert_not_contains "$nested_output" "WARNING"
|
|
|
|
mkdir -p "$TMP_DIR/nonempty/instances/hindsight"
|
|
touch "$TMP_DIR/nonempty/instances/hindsight/instance.json"
|
|
nonempty_output="$(check_pg0_data_integrity "$TMP_DIR/nonempty")"
|
|
assert_contains "$nonempty_output" "WARNING: pg0 data directory exists"
|
|
|
|
echo "start-all pg0 integrity checks passed"
|
|
|
|
# =============================================================================
|
|
# resolve_api_startup_wait_seconds (#3733)
|
|
# =============================================================================
|
|
assert_equals() {
|
|
local actual="$1"
|
|
local expected="$2"
|
|
|
|
if [ "$actual" != "$expected" ]; then
|
|
echo "Expected: $expected"
|
|
echo "Actual: $actual"
|
|
exit 1
|
|
fi
|
|
}
|
|
|
|
# Neither knob set: the wrapper default.
|
|
assert_equals "$(HINDSIGHT_API_STARTUP_WAIT_SECONDS= HINDSIGHT_API_MODEL_INIT_TIMEOUT= resolve_api_startup_wait_seconds)" "300"
|
|
|
|
# The documented knob raised: the wrapper waits at least that long, so raising
|
|
# it actually takes effect instead of being cut short at the default.
|
|
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200 resolve_api_startup_wait_seconds)" "7230"
|
|
|
|
# Floats are accepted — the API parses its cap as one.
|
|
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200.0 resolve_api_startup_wait_seconds)" "7230"
|
|
|
|
# A shorter cap never shortens the wrapper wait.
|
|
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=60 resolve_api_startup_wait_seconds)" "300"
|
|
|
|
# Garbage falls back to the default rather than breaking startup.
|
|
assert_equals "$(HINDSIGHT_API_MODEL_INIT_TIMEOUT=abc resolve_api_startup_wait_seconds)" "300"
|
|
|
|
# An explicit wrapper setting always wins.
|
|
assert_equals "$(HINDSIGHT_API_STARTUP_WAIT_SECONDS=45 HINDSIGHT_API_MODEL_INIT_TIMEOUT=7200 resolve_api_startup_wait_seconds)" "45"
|
|
|
|
echo "start-all API startup wait checks passed"
|
|
|
|
# =============================================================================
|
|
# check_pg0_writable (#1483)
|
|
# These rely on filesystem permissions, which root bypasses; skip under root.
|
|
# =============================================================================
|
|
if [ "$(id -u)" != "0" ]; then
|
|
# Writable directory: returns 0, prints nothing, leaves no artifact behind.
|
|
mkdir -p "$TMP_DIR/writable"
|
|
writable_output="$(check_pg0_writable "$TMP_DIR/writable")"
|
|
assert_empty "$writable_output"
|
|
if [ -e "$TMP_DIR/writable/.hindsight-write-test" ]; then
|
|
echo "check_pg0_writable left its write-test file behind"
|
|
exit 1
|
|
fi
|
|
|
|
# Non-writable directory: returns 1 with actionable guidance.
|
|
mkdir -p "$TMP_DIR/readonly"
|
|
chmod 000 "$TMP_DIR/readonly"
|
|
set +e
|
|
readonly_output="$(check_pg0_writable "$TMP_DIR/readonly" 2>&1)"
|
|
readonly_rc=$?
|
|
set -e
|
|
chmod 755 "$TMP_DIR/readonly"
|
|
if [ "$readonly_rc" -eq 0 ]; then
|
|
echo "check_pg0_writable should fail on a non-writable directory"
|
|
exit 1
|
|
fi
|
|
assert_contains "$readonly_output" "not writable"
|
|
assert_contains "$readonly_output" "hindsight-data:/home/hindsight/.pg0"
|
|
assert_contains "$readonly_output" "--user"
|
|
|
|
# External database configured: skip the check regardless of dir perms.
|
|
mkdir -p "$TMP_DIR/extdb"
|
|
chmod 000 "$TMP_DIR/extdb"
|
|
set +e
|
|
HINDSIGHT_API_DATABASE_URL="postgres://x" check_pg0_writable "$TMP_DIR/extdb" >/dev/null 2>&1
|
|
extdb_rc=$?
|
|
set -e
|
|
chmod 755 "$TMP_DIR/extdb"
|
|
if [ "$extdb_rc" -ne 0 ]; then
|
|
echo "check_pg0_writable should skip when an external database is configured"
|
|
exit 1
|
|
fi
|
|
|
|
echo "start-all pg0 writability checks passed"
|
|
else
|
|
echo "⚠️ Running as root; skipping pg0 writability checks (permissions are bypassed)."
|
|
fi
|