bitflicker64 commented on code in PR #3381:
URL: https://github.com/apache/hugegraph/pull/3381#discussion_r4328884103
##########
helm/hugegraph/values.yaml:
##########
@@ -98,9 +98,14 @@ pd:
size: 10Gi
storageClassName: ""
resources: {}
- # Pod-level and container-level securityContext. Empty by default because the
- # published images run as root; set these to satisfy a restricted namespace.
- podSecurityContext: {}
+ # Pod-level and container-level securityContext. The images run as the
+ # non-root UID/GID 10001; fsGroup lets that user write the data volume,
+ # including one created by an older image that ran as root. A restricted
+ # namespace also needs securityContext.runAsNonRoot: true, which is safe
+ # with images from this version only.
+ podSecurityContext:
Review Comment:
Important: The fsGroup default lives only in values.yaml, so the chart's own
upgrade command drops it. The README's "Upgrade and verify" block runs `helm
upgrade hugegraph ./helm/hugegraph -n hugegraph --reuse-values --wait --timeout
15m`, and `--reuse-values` replaces the new chart's defaults with the release's
old values, where `podSecurityContext` is `{}`. The PR says so itself
("`--reuse-values` keeps the old empty value"), and the CI step "legacy
--reuse-values compatibility" exists because of this. After such an upgrade the
PD and Store pods start the new image from `latest` with `pullPolicy: Always`,
without fsGroup, on a PVC the root images wrote. They then fail with
`Permission denied`, and `--wait` times out. This is the case the new default
is meant to cover ("PVCs, including ones written by older root images, stay
writable"). Today only an operator who reads the new paragraph and adds four
`--set` flags avoids it.
Evidence: I copied the chart and used `testdata/values-pre-hardening.yaml`
as its values.yaml, which is what the CI step does to stand in for
--reuse-values. `helm template legacy <copy> --is-upgrade` renders no
`securityContext` on either StatefulSet, while the default render has `fsGroup:
10001`. The chart already puts defaults that must survive value reuse in the
template, for example `{{ .Values.store.waitTimeoutSeconds | default 900 }}` in
store-statefulset.yaml.
Requested change: apply the default in pd-statefulset.yaml and
store-statefulset.yaml as well, so it also applies under --reuse-values and
still respects an explicit value. In my local copy, this renders `fsGroup:
10001` under the legacy values and keeps `--set
pd.podSecurityContext.fsGroup=2000`:
```yaml
{{- $podSC := deepCopy (.Values.pd.podSecurityContext | default dict)
}}
{{- if not (hasKey $podSC "fsGroup") }}
{{- $_ := set $podSC "fsGroup" 10001 }}
{{- $_ := set $podSC "fsGroupChangePolicy" "OnRootMismatch" }}
{{- end }}
securityContext:
{{- toYaml $podSC | nindent 8 }}
```
If you would rather keep it opt-in, at least have NOTES warn on
`.Release.IsUpgrade` when `pd`/`store.podSecurityContext` has no fsGroup, as it
already does for the NodePort entry types, and put the four `--set` flags in
the main upgrade command.
--
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]
