Prometheus metrics setup#125
Conversation
pablin-10
changed the title
alexhulbert
left a comment
alexhulbert
left a comment
There was a problem hiding this comment.
One small change, otherwise lgtm
pablin-10
force-pushed
the
pablo/observability_v2
branch
from
924cb6e to
89fa85b
Compare
pablin-10
force-pushed
the
pablo/observability_v2
branch
2 times, most recently
from
bd99350 to
c0ce099
Compare
pablin-10
force-pushed
the
pablo/observability_v2
branch
from
6b2fc99 to
738717b
Compare
pablin-10
changed the title
| if [ -n "${METRICS_ENDPOINTS:-}" ]; then | ||
| for ip in $METRICS_ENDPOINTS; do | ||
| accept_dst_ip_port $CHAIN_ALWAYS_OUT tcp "$ip" $HTTPS_PORT "Metrics endpoint (Flashbots)" | ||
| done | ||
| fi |
There was a problem hiding this comment.
I know this is important but I have concerns about it from different angles:
- it introduces dynamic IP allowlisting which deviates from the Flashbox L1 images having everything static and part of the measurements for attestation/verification purposes
- should we consider dropping those opened endpoints manually for the searcher's rootless podman container as we do for couple always out endpoints in the init-container.sh? or what is the rational behind leaving that out? what's the impact if the searcher's container could reach those endpoints too beside the guest-os?
- I recall you mentioned those IP endpoints might change, what is the process to update those and refresh the firewall rules at runtime? how invasive it is? does it have potential downtime? is it automated or manually triggered?
There was a problem hiding this comment.
out of curiosity, what is this used/needed for here inside the observability module itself?
| --web.console.templates=/usr/share/prometheus/consoles \ | ||
| --web.console.libraries=/usr/share/prometheus/console_libraries \ | ||
| --web.listen-address=127.0.0.1:9090 | ||
| ExecReload=/bin/kill -HUP $MAINPID |
There was a problem hiding this comment.
why is this needed? doesnt systemd handle this automatically?
| if [ -z "${METRICS_FLASHBOTS_URL:-}" ]; then | ||
| echo "No metrics URL configured, remote_write disabled" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
wouldnt this always trigger an exit 0 here or where is "METRICS_FLASHBOTS_URL" being populated beforehand?
| local key value | ||
| for key in $keys; do | ||
| value=$(echo "$secret_data" | jq -rc --arg k "$key" '.[$k] // ""') | ||
| export "${key}=${value}" |
There was a problem hiding this comment.
I wonder if this is sanitized enough to later call source on. Can we verify this somehow?
Claude (under some pressure) gave an example payload using the fact that the export is not sanitized (using various gadgets from this PR), I think it would work in practice (nothing fancy, bare bones sh)
exploit.sh
There was a problem hiding this comment.
In general, I'd try to avoid doing these stuff manually in bash script.
Could we do templating with minimal render engine like original did in BuilderNet using mustache (examples).
This way, the template would exactly render those values into the corresponding place-holders and avoid potential malicious attacks
pablin-10
force-pushed
the
pablo/observability_v2
branch
from
65c0e7e to
736a61c
Compare
4 participants
Here we add the observability features but in a separate module.
This PR aims to replace these two: