Skip to content

Add instant clusters monitoring#143

Open
brennentsmith-runpod wants to merge 1 commit into
mainfrom
instant-clusters-monitoring
Open

Add instant clusters monitoring#143
brennentsmith-runpod wants to merge 1 commit into
mainfrom
instant-clusters-monitoring

Conversation

@brennentsmith-runpod

Copy link
Copy Markdown

No description provided.

for k, v in resp.getheaders():
if k.lower() in DROP:
continue
self.send_header(k, v)
for k, v in resp.getheaders():
if k.lower() in DROP:
continue
self.send_header(k, v)
# Fresh valid token in the URL -> (re)establish the session cookie.
self.send_header(
"Set-Cookie",
f"{COOKIE}={token}; Path=/; HttpOnly; Secure; SameSite=Lax",
# Fresh valid token in the URL -> (re)establish the session cookie.
self.send_header(
"Set-Cookie",
f"{COOKIE}={token}; Path=/; HttpOnly; Secure; SameSite=Lax",

@daveseddon-runpod daveseddon-runpod left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. I reviewed this by standing up a pinned Nix static-analysis harness (ruff, bandit, shellcheck, hadolint, yamllint, actionlint, JSON validation) and running it against the new files, plus a manual pass on the security-sensitive pieces. The auth model is well thought through and correctly implemented — no blocking issues.

Security model — verified sound ✅

The shared-token proxy in front of Grafana holds up end to end:

  • Grafana binds loopback only (http_addr = 127.0.0.1:3300) and auth.proxy uses whitelist = 127.0.0.1, so the injected X-WEBAUTH-User identity is unspoofable from outside.
  • auth_proxy.py strips any client-supplied X-WEBAUTH-* and injects it only on a successful hmac.compare_digest token match (constant-time).
  • Login form and HTTP basic auth are disabled, and post_start.sh randomizes the built-in admin password — so the proxy is genuinely the only way in.
  • Only EXPOSE 8889 (the proxy) is public; 3300 is never mapped. No bypass path.

Nits (non-blocking)

  1. post_start.sh:71 — shellcheck SC2155. export GF_SECURITY_ADMIN_PASSWORD="$(openssl rand … || …)" masks the subshell's return value. Harmless here (the || fallback covers a missing openssl and the export status isn't checked), but you could split declaration/assignment to keep shellcheck clean.
  2. Dockerfile:33 — hadolint DL3008 (unpinned apt versions). Consistent with the ignore rules the repo already applies to its other Dockerfiles, so fine to leave — flagging only for parity.
  3. cluster.yml — actionlint "unknown runner label" for blacksmith-16vcpu-ubuntu-2404 / blacksmith-4vcpu-ubuntu-2204. False positive — these are the same Blacksmith self-hosted runners already used in base.yml, nvidia.yml, and rocm.yml. If we want CI to stop warning, we could add an .github/actionlint.yaml declaring the custom labels.

Optional robustness notes in auth_proxy.py (low risk for this use case, just noting)

  • conn is only closed on the success path; a non-OSError exception from getresponse()/read() would leak the upstream connection.
  • Chunked request bodies aren't forwarded (transfer-encoding is stripped and only Content-Length bodies are read). Grafana traffic is effectively always Content-Length, so this shouldn't bite in practice.

Clean: ruff (auth_proxy.py), all 6 Grafana dashboard JSONs parse valid, yamllint (only a line-length warning on the workflow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants