strelok89 commented on code in PR #3855:
URL: https://github.com/apache/celeborn/pull/3855#discussion_r4056423680
##########
charts/celeborn/templates/worker/statefulset.yaml:
##########
@@ -103,15 +131,51 @@ spec:
true; do
echo "waiting for master";
sleep 2;
- done && exec /opt/celeborn/sbin/start-worker.sh
+ {{- /*
+ Publish the zone as a metric label, so queries select on it rather
than on pod names.
+ The conf file is loaded with setIfMissing, so a system property wins
over it, and
+ start-worker.sh builds CELEBORN_JAVA_OPTS from
CELEBORN_WORKER_JAVA_OPTS - so this
+ composes with whatever `worker.env` already sets.
+ */}}
+ {{- $zoneOpts := "" }}
+ {{- if and .zone .Values.worker.zoneAwareReplication.metricsLabel }}
+ {{- $extraLabels := printf "zone=%s" .zone.name }}
+ {{- with get .Values.celeborn "celeborn.metrics.extraLabels" }}
+ {{- $extraLabels = printf "%s,%s" . $extraLabels }}
+ {{- end }}
+ {{- $zoneOpts = printf "export
CELEBORN_WORKER_JAVA_OPTS=\"${CELEBORN_WORKER_JAVA_OPTS:-}
-Dceleborn.metrics.extraLabels=%s\" && " $extraLabels }}
Review Comment:
Good catch, fixed in `df4aa1e6a`.
Confirmed the concern: with `celeborn.metrics.extraLabels: env=$(id -u)` the
value landed verbatim inside the double-quoted `/bin/sh` string, so it would
have been evaluated at container start. Not a privilege boundary — anyone who
can set values can already set `worker.env` — but a real correctness bug, since
a `$`, backtick or quote in an otherwise valid label would mangle or break
worker startup.
The value now goes through a `CELEBORN_ZONE_METRIC_LABELS` environment
variable and the command references `$CELEBORN_ZONE_METRIC_LABELS`, since the
shell expands a variable's value without re-parsing it. Regression test asserts
the hostile value reaches the env var and never appears in the command string.
--
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]
