jedcunningham commented on code in PR #31043:
URL: https://github.com/apache/airflow/pull/31043#discussion_r1195929697
##########
chart/templates/_helpers.yaml:
##########
@@ -731,85 +731,148 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults
to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod
or legacy securityContext and <node>.securityContext, defaults to global uid
and gid.
- +------------------------+ +-----------------+
+-------------------------+
- | <node>.securityContext | -> | securityContext | -> | Values.uid +
Values.gid |
- +------------------------+ +-----------------+
+-------------------------+
+ +-----------------------------+ +------------------------+
+----------------------+ +-----------------+
+-------------------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> |
securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+ +-----------------------------+ +------------------------+
+----------------------+ +-----------------+
+-------------------------+
-Values are not accumulated meaning that if runAsUser is set to 10 in
<node>.securityContext,
+Values are not accumulated meaning that if runAsUser is set to 10 in
<node>.securityContexts.pod,
any extra values set to securityContext or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContext" (list . .Values.webserver)
+ include "airflowPodSecurityContext" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.webserver` the local
variables scope for the webserver template.
*/}}
-{{- define "airflowSecurityContext" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContext" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else if $.Values.securityContext }}
- {{- toYaml $.Values.securityContext }}
- {{- else }}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
Review Comment:
```suggestion
{{- toYaml .securityContexts.pod | print }}
```
Can you fix the indentation here?
##########
chart/values.schema.json:
##########
@@ -1998,7 +2221,87 @@
},
"logGroomerSidecar": {
"$ref": "#/definitions/logGroomerConfigType",
- "description": "Configuration for log groomer sidecar"
+ "description": "Configuration for the schedulers log
groomer sidecar.",
+ "type": "object",
+ "properties": {
Review Comment:
We don't need to duplicate this because of the ref. Another accident during
rebase?
##########
chart/templates/_helpers.yaml:
##########
@@ -731,85 +731,148 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults
to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod
or legacy securityContext and <node>.securityContext, defaults to global uid
and gid.
- +------------------------+ +-----------------+
+-------------------------+
- | <node>.securityContext | -> | securityContext | -> | Values.uid +
Values.gid |
- +------------------------+ +-----------------+
+-------------------------+
+ +-----------------------------+ +------------------------+
+----------------------+ +-----------------+
+-------------------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> |
securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+ +-----------------------------+ +------------------------+
+----------------------+ +-----------------+
+-------------------------+
-Values are not accumulated meaning that if runAsUser is set to 10 in
<node>.securityContext,
+Values are not accumulated meaning that if runAsUser is set to 10 in
<node>.securityContexts.pod,
any extra values set to securityContext or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContext" (list . .Values.webserver)
+ include "airflowPodSecurityContext" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.webserver` the local
variables scope for the webserver template.
*/}}
-{{- define "airflowSecurityContext" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContext" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else if $.Values.securityContext }}
- {{- toYaml $.Values.securityContext }}
- {{- else }}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
+ {{- else if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ toYaml $.Values.securityContexts.pod | print }}
+ {{- else if $.Values.securityContext -}}
+{{ toYaml $.Values.securityContext | print }}
+ {{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end }}
{{- end }}
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults
to UID in the local node.
+Set the default value for pod securityContext
+If no value is passed for <node>.securityContexts.pod or
<node>.securityContext, defaults to UID in the local node.
- +------------------------+ +-------------+
- | <node>.securityContext | > | <node>.uid |
- +------------------------+ +-------------+
+ +-----------------------------+ +------------------------+
+-------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> |
<node>.uid |
+ +-----------------------------+ +------------------------+
+-------------+
The template can be called like so:
- include "localSecurityContext" .Values.statsd
+ include "localPodSecurityContext" (list . .Values.schedule)
It is important to pass the local variables scope to this template as it is
used to determine the local node value for uid.
*/}}
-{{- define "localSecurityContext" -}}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else }}
- {{- printf "runAsUser: %v" .uid }}
- {{- end }}
-{{- end }}
+{{- define "localPodSecurityContext" -}}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
Review Comment:
```suggestion
{{- toYaml .securityContexts.pod | print }}
```
Here too. Last one I'll comment on, but hit the rest of these as well?
##########
chart/values.schema.json:
##########
@@ -1624,8 +1705,88 @@
}
},
"logGroomerSidecar": {
- "$ref": "#/definitions/logGroomerConfigType",
Review Comment:
I think this might have been a bad rebase? We want to keep using this def
yeah?
##########
chart/values.schema.json:
##########
@@ -3394,25 +3959,25 @@
"default": []
},
"networkPolicy": {
- "description": "Webserver NetworkPolicy configuration",
+ "description": "Webserver network policy configuration",
Review Comment:
This doesn't really feel related. If you want to make this change, can you
move it to a new PR?
##########
chart/values.yaml:
##########
@@ -35,12 +35,17 @@ revisionHistoryLimit: ~
uid: 50000
gid: 0
-# Default security context for airflow
+# Default security context for airflow (deprecated, use below in the future)
Review Comment:
```suggestion
# Default security context for airflow (deprecated, use `securityContexts`
instead)
```
Just going to comment on this once, but we should probably mention what to
use instead. Also, can you add this to values.schema.json as well, as that's
used for docs and artifacthub.
##########
chart/values.schema.json:
##########
@@ -3995,7 +4622,7 @@
"type": "object",
"properties": {
"create": {
- "description": "Specifies whether a ServiceAccount
should be created.",
+ "description": "Specifies whether a service
account should be created.",
Review Comment:
Same with this one.
##########
docs/helm-chart/production-guide.rst:
##########
@@ -332,6 +332,7 @@ In the Airflow Helm chart, the ``securityContext`` can be
configured in several
* :ref:`uid <parameters:Airflow>` (configures the global uid or RunAsUser)
* :ref:`gid <parameters:Airflow>` (configures the global gid or fsGroup)
* :ref:`securityContext <parameters:Kubernetes>` (same as ``uid`` but allows
for setting all `Pod securityContext options
<https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_)
+ * :ref:`securityContexts <parameters:Kubernetes>` (same as
``securityContext`` with additional security context on the container level
`Container securityContext options
<https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#securitycontext-v1-core>`_)
The same way one can configure the global :ref:`securityContext
<parameters:Kubernetes>`, it is also possible to configure different values for
specific workloads by setting their local ``securityContext`` as follows:
Review Comment:
Since we are deprecating `securityContext`, we need to update this example
to show `securityContexts` instead.
--
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]
