Miretpl commented on code in PR #70238:
URL: https://github.com/apache/airflow/pull/70238#discussion_r3669080286
##########
chart/templates/_helpers.yaml:
##########
@@ -890,15 +896,18 @@ If no value is passed for
<node>.securityContexts.container, defaults to UID in
+-----------------------------------+ +------------+
The template can be called like so:
- include "localContainerSecurityContext" .Values.statsd
+ include "localContainerSecurityContext" (list .Values.dags.gitSync .Values)
Review Comment:
Could you make it unified with other invocations of securityContext-related
helper methods? In one place, we have `list .Values .Values.scheduler` order
and in the other `list .Values.scheduler .Values` order. It would be easier for
the developers and maintainers to have it in one order.
##########
chart/templates/_helpers.yaml:
##########
@@ -914,6 +923,9 @@ The template looks for `runAsUser` and `fsGroup`
specifically, any other paramet
Values are not accumulated meaning that if runAsUser is set to 10 in
<node>.securityContexts.pod,
any extra values set to securityContexts or uid+gid will be ignored.
+If securityContexts.disableDefaults is true and no explicit runAsUser/fsGroup
is set anywhere,
+this template renders an empty string, since there is no meaningful id to
chown to.
Review Comment:
```suggestion
this template renders an empty string, since there is no id to chown to.
```
##########
chart/newsfragments/70238.bugfix.rst:
##########
@@ -0,0 +1 @@
+Added ``securityContexts.disableDefaults`` to the Helm chart. When set, the
chart no longer fills in default ``runAsUser``/``fsGroup`` (pod) or
``runAsUser`` (container) values, letting platforms such as OpenShift assign
them automatically via SCC instead of the chart hard-coding them.
Review Comment:
```suggestion
Added ``securityContexts.disableDefaults`` (default ``false``) to the Helm
chart. When set, the chart no longer fills in the default
``runAsUser``/``fsGroup`` (pod) or ``runAsUser`` (container) values, allowing
platforms such as OpenShift to assign them automatically via SCC instead of the
chart hard-coding them.
Value ``false`` for ``securityContexts.disableDefaults`` is deprecated and
will be ``true`` by default in the next major release. Please adjust your
configuration accordingly.
```
and I would change the type from `bugfix` to `significant` and deprecate the
value `false` for this parameter (with additional change in NOTES and values
files with the `deprecated` information).
##########
chart/values.schema.json:
##########
@@ -126,6 +126,12 @@
"allowPrivilegeEscalation": false
}
]
+ },
+ "disableDefaults": {
+ "description": "If true, the chart will not set any
default `runAsUser`/`fsGroup` (pod) or `runAsUser` (container) values when
`securityContexts.pod` / `securityContexts.containers` (or the equivalent
per-component overrides) are left empty. Useful on platforms such as OpenShift,
where the cluster assigns these values automatically via SCC and rejects
pods/containers that hard-code them.",
Review Comment:
```suggestion
"description": "If ``true``, the chart will not set any
default ``securityContext`` values when ``securityContexts.pod`` /
``securityContexts.containers`` (or the equivalent per-component overrides) are
left empty.",
```
+ deprecated message. Field description should be rather constraints to the
only required information.
##########
chart/values.yaml:
##########
@@ -47,6 +47,12 @@ gid: 0
securityContexts:
pod: {}
containers: {}
+ # If true, the chart will not set any default `runAsUser`/`fsGroup` (pod) or
`runAsUser`
+ # (container) values when securityContexts.pod / securityContexts.containers
(or the
+ # equivalent per-component overrides) are left empty. Useful on platforms
such as
+ # OpenShift, where the cluster assigns these values automatically via SCC
and rejects
+ # pods/containers that hard-code them.
Review Comment:
Same comment as in schema file.
##########
chart/tests/helm_tests/security/test_security_context.py:
##########
@@ -422,6 +422,120 @@ def test_main_pod_setting(self):
for doc in docs[1:]:
assert ctx_value ==
jmespath.search("spec.template.spec.securityContext", doc)
+ def test_disable_defaults_pod_and_container(self):
Review Comment:
Could you add tests for:
1. pod template file
2. overrides when component level option is set
?
--
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]
