You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/12/15 21:12:32 UTC
[GitHub] [airflow] nwalens opened a new pull request #18249: Add support for securityContext per deployment
nwalens opened a new pull request #18249:
URL: https://github.com/apache/airflow/pull/18249
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #17744
related: #18136
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
closes: #17744
related: #18136
This PR replaces the global "uid" and "gid" with a per deployment securityContext setting.
Fields are configured as follows:
```
podSecurity:
default:
securityContext:
runAsUser: 50000
fsGroup: 0
runAsGroup: 0
containerSecurityContext:
runAsUser: 50000
runAsGroup: 0
webserver:
securityContext:
enabled: false
containerSecurityContext:
enabled: false
```
The default will be used in case the individual setting is disabled.
This change allows for more configurability and allows the usage of arbitrary securityContexts as permitted by the official docker images.
The issue #18136 still requires the PR #18147 for Openshift clusters since there is no option to remove securityContexts altogether.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r708827609
##########
File path: chart/values.schema.json
##########
@@ -70,6 +70,332 @@
"default": "2.1.3",
"x-docsSection": "Common"
},
+ "podSecurity": {
+ "description": "Set security contexts for certain containers",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "additionalProperties": false,
+ "properties": {
+ "default": {
+ "description": "Default global security context.",
+ "type": "object",
+ "additionalProperties": false,
+ "properties": {
+ "securityContext": {
+ "description": "Global Pod security context as defined in https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core",
+ "type": "object",
+ "default": "See values.yaml",
Review comment:
```suggestion
"default": {"runAsUser": 50000, "fsGroup": 0, "runAsGroup": 0},
```
I think we should bring the actual defaults into the docs instead.
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -45,6 +45,7 @@ spec:
{{- end }}
containers:
- args: []
+ securityContext: {{- omit .Values.podSecurity.pod_template.containerSecurityContext "enabled" | default (.Values.podSecurity.default.containerSecurityContext) | toYaml | nindent 8 }}
Review comment:
Instead of this `omit` pattern, would this work instead? I think it will and is more intuitive imo.
values.yaml:
```
podSecurity.pod_template.containerSecurityContext: {}
```
and:
```suggestion
securityContext: {{- .Values.podSecurity.pod_template.containerSecurityContext | default (.Values.podSecurity.default.containerSecurityContext) | toYaml | nindent 8 }}
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-927656408
@potiuk the code was rebased. I'm still testing it after applying the changes @jedcunningham has requested and I am having some issues with several tests.
I'm currently checking each test and identifying the failures.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740672226
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
+ fsGroup: 0
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
Review comment:
where is it getting 1001 from since it wasn't provided in config? i guess these are not configurable?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
50,000?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
i tried this with the given config and found that runas and fs are only output for `spec.template.spec.securityContext` but not the others
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
re
`podSecurity.securityContext`
it does not seem you have actually added a `podSecurity` node?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
ok so i would just make sure that the examples make sense / consistent. they are there to help users understand what happens given certain inputs, and if the example output is inconsistent with the behavior, or if it contains output that is not affected by or related to the particular input, then it can cause confusion. so in this case, the container sec context doesn't seem like it should be here, but please correct me if i'm missing something.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995458013
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995599940
> I don't _think_ you have a test that covers when you have a partial global security setting (say) UID, but a local security setting only GID
>
> Could you add that please?
@ashb - On an second thought, I'm not sure we would test with that. This PR does not allow for using UID and GID + securityContext + <node>.securityContext.
If you set <node>.securityContext, the values set here will be used and all other values from UID, GID and securityContext ignored. They don't add on top of each other.
The order they currently follow is <node>.securityContext > securityContext > (UID & GID).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995599940
> I don't _think_ you have a test that covers when you have a partial global security setting (say) UID, but a local security setting only GID
>
> Could you add that please?
@ashb - On a second thought, I'm not sure we should test that. This PR does not allow for using UID and GID + securityContext + <node>.securityContext.
If you set <node>.securityContext, the values set here will be used and all other values from UID, GID and securityContext ignored. They don't add on top of each other.
The order they currently follow is <node>.securityContext > securityContext > (UID & GID).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770883736
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,85 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
+ +------------------------+ +-----------------+ +-------------------------+
+
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContext,
+any extra values set to securityContext or uid+gid will be ignored.
+
+The template can be called like so:
+ include "airflowSecurityContext" (list . .Values.webserver)
+
+Where '.' is the global varriables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "airflowSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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.
+
+ +------------------------+ +-------------+
+ | <node>.securityContext | > | <node>.uid |
+ +------------------------+ +-------------+
+
+The template can be called like so:
+ include "localSecurityContext" .Values.statsd
+
+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 | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+The template looks for `runAsUser` and `fsGroup` specifically, any other parameter will be ignored.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
+ +------------------------+ +-----------------+ +-------------------------+
+
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContext,
+any extra values set to securityContext or uid+gid will be ignored.
+
+The template can be called like so:
+ include "airflowSecurityContextIds" (list . .Values.workers)
+
+Where '.' is the global varriables scope and `.Values.workers` the local variables scope for the workers template.
Review comment:
```suggestion
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,85 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
+ +------------------------+ +-----------------+ +-------------------------+
+
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContext,
+any extra values set to securityContext or uid+gid will be ignored.
+
+The template can be called like so:
+ include "airflowSecurityContext" (list . .Values.webserver)
+
+Where '.' is the global varriables scope and `.Values.webserver` the local variables scope for the webserver template.
Review comment:
```suggestion
Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r717791801
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -56,6 +56,7 @@ spec:
image: {{ template "pod_template_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
name: base
+ securityContext: {{- .Values.podTemplateSecurity.containerSecurityContext | default (.Values.podSecurity.containerSecurityContext) | toYaml | nindent 4 }}
Review comment:
This should use `workers.containerSecurityContext` instead of having a separate param.
##########
File path: chart/values.yaml
##########
@@ -383,6 +393,19 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
Review comment:
```suggestion
securityContext: {}
# runAsUser: 50000
# fsGroup: 0
```
We shouldn't set these for all of the components, otherwise `podSecurity.securityContext` will never actually be used and isn't an easy way to set it across the board.
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -87,9 +88,7 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
restartPolicy: Never
- securityContext:
- runAsUser: {{ .Values.uid }}
- fsGroup: {{ .Values.gid }}
+ securityContext: {{- .Values.podTemplateSecurity.securityContext | default (.Values.podSecurity.securityContext) | toYaml | nindent 4 }}
Review comment:
Same here, should use `workers.securityContext`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r717732834
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -45,6 +45,7 @@ spec:
{{- end }}
containers:
- args: []
+ securityContext: {{- omit .Values.podSecurity.pod_template.containerSecurityContext "enabled" | default (.Values.podSecurity.default.containerSecurityContext) | toYaml | nindent 8 }}
Review comment:
The template was amended to reflect the change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740674914
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770682265
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
I like `airflowSecurityContext`.
I think I like having a separate `localSecurityContext`, as it does grab the uid from a different object too (`$.Values.uid` vs `.uid`).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-996155783
> Should it be extended to the likes of pgBouncer and redis at this point?
Let's leave these for a future PR.
@ash, there is already coverage for the local vs global vs uid/gid precedence, so we should be good on that front.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740674914
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
https://github.com/apache/airflow/pull/19181
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740950411
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
Hey, thanks for the feedback. Those were only examples, it does make sense though that we use the default values I guess.
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
Sorry about that, I did have it in the past and since them after speaking to @jedcunningham I realised that that node was kind of useless. I will update it accordingly.
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
That is correct. Before it was changing the containers as well but after speaking to @jedcunningham we went for changing only the global pod security instead of the securityContext for each container.
The exception is only the gitSync container as it it works as a "add-on" initContainer or sidecar in some deployments.
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Yeah, that is great, was not aware of the possibility!
Will update it accordingly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721587250
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
Make sense except fir gitSync I think. Should I leave gitSync with the option?
That would also mean that for gitSync we would have the RunAsUser.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r717732335
##########
File path: chart/values.schema.json
##########
@@ -70,6 +70,332 @@
"default": "2.1.3",
"x-docsSection": "Common"
},
+ "podSecurity": {
+ "description": "Set security contexts for certain containers",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "additionalProperties": false,
+ "properties": {
+ "default": {
+ "description": "Default global security context.",
+ "type": "object",
+ "additionalProperties": false,
+ "properties": {
+ "securityContext": {
+ "description": "Global Pod security context as defined in https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core",
+ "type": "object",
+ "default": "See values.yaml",
Review comment:
Hi @jedcunningham, the changes were applied as requested.
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -45,6 +45,7 @@ spec:
{{- end }}
containers:
- args: []
+ securityContext: {{- omit .Values.podSecurity.pod_template.containerSecurityContext "enabled" | default (.Values.podSecurity.default.containerSecurityContext) | toYaml | nindent 8 }}
Review comment:
The template was amended to reflect the change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-929370861
Basic tests should be working fine and behaving the same as the current chart.
@jedcunningham should I remove the previous **uid** and **gid** fields from values.yaml and documentation?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770721300
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
yeah i was trying to think of one and struggled also. `serviceSecurityContext` or just `securityContext`
nice thing is it's 100% internal and can be changed later without hurting anyone :) (so local is fine)
but that said, we could do these: `securityContextAirflow` and `securityContextOther`... i kindof like that
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995574714
> any reason you did not touch pgBouncer?
At first the intention was to cover whatever was referencing the global UID and GID parameters, in a later stage it was extended to some components like gitSync and Triggerer due to the integration they have with the main components.
I can extend it to other components as well, I just don't want to make it too big so it breaks the current helmchart behaviour.
What do you think? Should it be extended to the likes of pgBouncer and redis at this point?
@dstandish @jedcunningham
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770669303
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
It ultimately doesn't matter, but as of today, the example chart Helm generates does use (namespaced) camel case.
`"example_chart.serviceAccountName"`
This is probably something worth making consistent eventually.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995228652
any reason you did not touch pg bouncer?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770202985
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
separately, i think a better name might be `airflowSecurityContext`. the difference between the templates `globalSecurityContext` and `localSecurityContext` is not that one is global and one is local. why? because the global one could actually resolve to _either_ "local" or "global" -- it will try local, else it will try global, else it will use top level uid / gid. it seems the real difference is one uses the airflow image and the other does not.
could consider just having one defined template `securityContext` and add a parameter (to the template, i.e. using a 3rd list element) signaling whether it's the airflow image or not? and based on that you could conditionally check the global params or not.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
@jedcunningham we are inconsistent with our naming of defined templates (i.e. with respect to snake case vs camel case) do you have any thoughts about which way is the right way?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r770683086
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,49 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+{{/*
+Set the default value for securityContext
+If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
Review comment:
(I can't think of a better name than `localSecurityContext` - I think it fits pretty well anyways)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740951250
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
Sorry about that, I did have it in the past and since them after speaking to @jedcunningham I realised that that node was kind of useless. I will update it accordingly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-929370861
Basic tests should be working fine and behaving the same as the current chart.
@jedcunningham should I remove the previous **uid** and **gid** fields from values.yaml and documentation? As far as this PR goes, they are being replaced by the **podSecurity** parameter.
Basically, when a configuration is not set for a component, it will default to **podSecurity.securityContext** or **podSecurity.containerSecurityContext**.
securityContext will be set for the whole Pod and containerSecurityContext for each container within the pod.
It does respect the options set for specific addons like git-sync.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740594865
##########
File path: chart/values.schema.json
##########
@@ -1629,6 +1666,18 @@
"description": "Annotations to add to the triggerer pods.",
"type": "object",
"default": {}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
Review comment:
```suggestion
"description": "Triggerer Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
```
Or similar, and for all the others that aren't actually global?
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+
+ def test_check_statsd_uid(self):
+ docs = render_chart(
+ values={"statsd": {"enabled": True, "uid": 3000}},
+ show_only=["templates/statsd/statsd-deployment.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
Review comment:
```suggestion
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
```
Both here and elsewhere.
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Both here and elsewhere.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ pluck "runAsUser" $securityContext | first }}:{{ pluck "fsGroup" $securityContext | first }}"
Review comment:
Is this pluck the only reason we use the `mustFromJson | toYaml` pattern everywhere?
Might be better to simplify the common case and just handle this case slightly differently? Haven't experimented with this at all, just my off the cuff thoughts.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740950411
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
Hey, thanks for the feedback. Those were only examples, it does make sense though that we use the default values I guess.
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
Sorry about that, I did have it in the past and since them after speaking to @jedcunningham I realised that that node was kind of useless. I will update it accordingly.
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
That is correct. Before it was changing the containers as well but after speaking to @jedcunningham we went for changing only the global pod security instead of the securityContext for each container.
The exception is only the gitSync container as it it works as a "add-on" initContainer or sidecar in some deployments.
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Yeah, that is great, was not aware of the possibility!
Will update it accordingly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740955336
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Yeah, that is great, was not aware of the possibility!
Will update it accordingly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740674914
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
https://github.com/apache/airflow/pull/19181
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740672226
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
+ fsGroup: 0
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
Review comment:
where is it getting 1001 from since it wasn't provided in config? i guess these are not configurable?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
50,000?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
i tried this with the given config and found that runas and fs are only output for `spec.template.spec.securityContext` but not the others
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
re
`podSecurity.securityContext`
it does not seem you have actually added a `podSecurity` node?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r742089969
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,189 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[0])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Sorry, my last set of suggestions may not have been clear. For the tests where we do have more than 1 doc (e.g. more than 1 template in `show_only` in this case), we should check them all.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ include "globalSecurityContextIds" (list . .Values.workers) }}"
Review comment:
I'd wondering if we'd be better off doing something like this instead (untested, but you get the idea):
`bash -c 'exec chown -R $(id -u):$(id -g) {{ template "airflow_logs" . }}`
Then `chown` can just use the uid/gid it's running as instead of being configured explicitly.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContextIds" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ pluck "runAsUser" .securityContext | first }}:{{ pluck "fsGroup" .securityContext | first }}
Review comment:
Does this work if only `securityContext.runAsUser` 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-929370861
Basic tests should be working fine and behaving the same as the current chart.
@jedcunningham should I remove the previous **uid** and **gid** fields from values.yaml and documentation?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-929370861
Basic tests should be working fine and behaving the same as the current chart.
@jedcunningham should I remove the previous **uid** and **gid** fields from values.yaml and documentation? As far as this PR goes, they are being replaced by the **podSecurity** parameter.
Basically, when a configuration is not set for a component, it will default to **podSecurity.securityContext** or **podSecurity.containerSecurityContext**.
securityContext will be set for the whole Pod and containerSecurityContext for each container within the pod.
It does respect the options set for specific addons like git-sync.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-922342595
Needs rebase (and some comments application)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-990989741
Can you fix the conflicts one more time please
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-990989741
Can you fix the conflicts one more time please and mention us when you are done?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-994076096
@kaxil rebased as requested.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740953091
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
That is correct. Before it was changing the containers as well but after speaking to @jedcunningham we went for changing only the global pod security instead of the securityContext for each container.
The exception is only the gitSync container as it it works as a "add-on" initContainer or sidecar in some deployments.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-962699544
You need to rebase and resolve the conflicts, I am afraid.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721686719
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Hi @jedcunningham, another thing on the gitSync vs the rest with dict.
When a template is generated, the result is always a string which in turn the rendering of the chart does not seem to understand very well - that is the reason for the toJson and fromJason I have there.
When I pass a dict, I can then serialise and deserialise as follows:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- if .Values.podSecurity.securityContext -}}
{{ .Values.podSecurity.securityContext | toJson }}
{{- else -}}
{{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
{{- $result | toJson }}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
```
{{- $securityContext := or .Values.webserver.securityContext (include "defaultSecurityContext" . | mustFromJson) }}
```
Printing strings works fine for gitSync since it is being done as a template instead of an include.
To make the rest work the same as gitSync, I would have to transfer the logic completely to _helpers, including the value we have in the values.yaml.
When I tried, it became much less readable in my opinion:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext -}}
{{ .securityContext | toYaml }}
{{- else if $.Values.podSecurity.securityContext -}}
{{ $.Values.podSecurity.securityContext | toYaml }}
{{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
``
{{- $securityContext := include "defaultSecurityContext" (list . .Values.webserver) }}
``
I had to pass arguments to the template and then evaluate the proper result and output it in a way that the rendering will be understood when building the template.
Again, I'm not sure if there is an easier solution, at least I could not find one with my limited knowledge in go templating.
Any thoughts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721686719
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Hi @jedcunningham, another thing on the gitSync vs the rest with dict.
When a template is generated, the result is always a string which in turn the rendering of the chart does not seem to understand very well - that is the reason for the toJson and fromJason I have there.
When I pass a dict, I can then serialise and deserialise as follows:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- if .Values.podSecurity.securityContext -}}
{{ .Values.podSecurity.securityContext | toJson }}
{{- else -}}
{{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
{{- $result | toJson }}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
```
{{- $securityContext := or .Values.webserver.securityContext (include "defaultSecurityContext" . | mustFromJson) }}
```
Printing strings works fine for gitSync since it is being done as a template instead of an include.
To make the rest work the same as gitSync, I would have to transfer the logic completely to _helpers, including the value we have in the values.yaml.
When I tried, it became much less readable in my opinion:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext -}}
{{ .securityContext | toYaml }}
{{- else if $.Values.podSecurity.securityContext -}}
{{ $.Values.podSecurity.securityContext | toYaml }}
{{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
```
{{- $securityContext := include "defaultSecurityContext" (list . .Values.webserver) }}
```
I had to pass arguments to the template and then evaluate the proper result and output it in a way that the rendering will be understood when building the template.
Again, I'm not sure if there is an easier solution, at least I could not find one with my limited knowledge in go templating.
Any thoughts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r722090589
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Another thing that came to mind is that the worker deployment requires a **uid** and **gid** to run a chown when fixed permissions is enabled.
To do that, we need to be able to extract the keys from the yaml which was produced by the template like so:
```
- "{{ pluck "runAsUser" $containerSecurityContext | first }}:{{ pluck "runAsGroup" $containerSecurityContext | first }}"
```
I'm not sure we can do that with a plain text result from the template.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r717791801
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -56,6 +56,7 @@ spec:
image: {{ template "pod_template_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
name: base
+ securityContext: {{- .Values.podTemplateSecurity.containerSecurityContext | default (.Values.podSecurity.containerSecurityContext) | toYaml | nindent 4 }}
Review comment:
This should use `workers.containerSecurityContext` instead of having a separate param.
##########
File path: chart/values.yaml
##########
@@ -383,6 +393,19 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
Review comment:
```suggestion
securityContext: {}
# runAsUser: 50000
# fsGroup: 0
```
We shouldn't set these for all of the components, otherwise `podSecurity.securityContext` will never actually be used and isn't an easy way to set it across the board.
##########
File path: chart/files/pod-template-file.kubernetes-helm-yaml
##########
@@ -87,9 +88,7 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
restartPolicy: Never
- securityContext:
- runAsUser: {{ .Values.uid }}
- fsGroup: {{ .Values.gid }}
+ securityContext: {{- .Values.podTemplateSecurity.securityContext | default (.Values.podSecurity.securityContext) | toYaml | nindent 4 }}
Review comment:
Same here, should use `workers.securityContext`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-959879038
@jedcunningham fort he chown, we cannot rely on container user and group as the initContainer has runAsUser hardcoded to 0, since it must run as root to set the permissions, and someone might want to set the runAsGroup, together with fsGroup, with different values which would cause a problem there.
I added a | default to the values though so we make sure we will always have something sane to set there.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r742089969
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,189 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[0])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Sorry, my last set of suggestions may not have been clear. For the tests where we do have more than 1 doc (e.g. more than 1 template in `show_only` in this case), we should check them all.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ include "globalSecurityContextIds" (list . .Values.workers) }}"
Review comment:
I'd wondering if we'd be better off doing something like this instead (untested, but you get the idea):
`bash -c 'exec chown -R $(id -u):$(id -g) {{ template "airflow_logs" . }}`
Then `chown` can just use the uid/gid it's running as instead of being configured explicitly.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContextIds" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ pluck "runAsUser" .securityContext | first }}:{{ pluck "fsGroup" .securityContext | first }}
Review comment:
Does this work if only `securityContext.runAsUser` is set?
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,189 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[0])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Sorry, my last set of suggestions may not have been clear. For the tests where we do have more than 1 doc (e.g. more than 1 template in `show_only` in this case), we should check them all.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ include "globalSecurityContextIds" (list . .Values.workers) }}"
Review comment:
I'd wondering if we'd be better off doing something like this instead (untested, but you get the idea):
`bash -c 'exec chown -R $(id -u):$(id -g) {{ template "airflow_logs" . }}`
Then `chown` can just use the uid/gid it's running as instead of being configured explicitly.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContextIds" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ pluck "runAsUser" .securityContext | first }}:{{ pluck "fsGroup" .securityContext | first }}
Review comment:
Does this work if only `securityContext.runAsUser` 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r741355381
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
ok so i would just make sure that the examples make sense / consistent. they are there to help users understand what happens given certain inputs, and if the example output is inconsistent with the behavior, or if it contains output that is not affected by or related to the particular input, then it can cause confusion. so in this case, the container sec context doesn't seem like it should be here, but please correct me if i'm missing something.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r769827430
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityContext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityContext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityContext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityContext or <node>.securityContext, defaults to UID in the local node
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -616,3 +615,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityContext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens edited a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens edited a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995599940
> I don't _think_ you have a test that covers when you have a partial global security setting (say) UID, but a local security setting only GID
>
> Could you add that please?
@ashb - On a second thought, I'm not sure we would test with that. This PR does not allow for using UID and GID + securityContext + <node>.securityContext.
If you set <node>.securityContext, the values set here will be used and all other values from UID, GID and securityContext ignored. They don't add on top of each other.
The order they currently follow is <node>.securityContext > securityContext > (UID & GID).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r722672775
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Fair enough, makes sense, I should have known it wasn't as simple as it seemed 👍
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721682315
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
Hi @jedcunningham, another thing on the gitSync vs the rest with dict.
When a template is generated, the result is always a string which in turn the rendering of the chart does not seem to understand very well - that is the reason for the toJson and fromJason I have there.
When I pass a dict, I can then serialise and deserialise as follows:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- if .Values.podSecurity.securityContext -}}
{{ .Values.podSecurity.securityContext | toJson }}
{{- else -}}
{{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
{{- $result | toJson }}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
```
{{- $securityContext := or .Values.webserver.securityContext (include "defaultSecurityContext" . | mustFromJson) }}
```
Printing strings works fine for gitSync since it is being done as a template instead of an include.
To make the rest work the same as gitSync, I would have to transfer the logic completely to _helpers, including the value we have in the values.yaml.
When I tried, it became much less readable in my opinion:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext -}}
{{ .securityContext | toYaml }}
{{- else if $.Values.podSecurity.securityContext -}}
{{ $.Values.podSecurity.securityContext | toYaml }}
{{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
``
{{- $securityContext := include "defaultSecurityContext" (list . .Values.webserver) }}
``
I had to pass arguments to the template and then evaluate the proper result and output it in a way that the rendering will be understood when building the template.
Again, I'm not sure if there is an easier solution, at least I could not find one with my limited knowledge in go templating.
Any thoughts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721682315
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
Hi @jedcunningham, another thing on the gitSync vs the rest with dict.
When a template is generated, the result is always a string which in turn the rendering of the chart does not seem to understand very well - that is the reason for the toJson and fromJason I have there.
When I pass a dict, I can then serialise and deserialise as follows:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- if .Values.podSecurity.securityContext -}}
{{ .Values.podSecurity.securityContext | toJson }}
{{- else -}}
{{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
{{- $result | toJson }}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
```
{{- $securityContext := or .Values.webserver.securityContext (include "defaultSecurityContext" . | mustFromJson) }}
```
Printing strings works fine for gitSync since it is being done as a template instead of an include.
To make the rest work the same as gitSync, I would have to transfer the logic completely to _helpers, including the value we have in the values.yaml.
When I tried, it became much less readable in my opinion:
*_helpers.yaml:*
```
{{- define "defaultSecurityContext" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext -}}
{{ .securityContext | toYaml }}
{{- else if $.Values.podSecurity.securityContext -}}
{{ $.Values.podSecurity.securityContext | toYaml }}
{{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
```
*webserver-deployment.yaml:*
``
{{- $securityContext2 := include "defaultSecurityContext" (list . .Values.webserver) }}
``
I had to pass arguments to the template and then evaluate the proper result and output it in a way that the rendering will be understood when building the template.
Again, I'm not sure if there is an easier solution, at least I could not find one with my limited knowledge in go templating.
Any thoughts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740950411
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
Hey, thanks for the feedback. Those were only examples, it does make sense though that we use the default values I guess.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham closed pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #18249:
URL: https://github.com/apache/airflow/pull/18249
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-996941520
Thanks for the contribution @nwalens 🎉! (also thanks for being patient)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham merged pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #18249:
URL: https://github.com/apache/airflow/pull/18249
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r717732335
##########
File path: chart/values.schema.json
##########
@@ -70,6 +70,332 @@
"default": "2.1.3",
"x-docsSection": "Common"
},
+ "podSecurity": {
+ "description": "Set security contexts for certain containers",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "additionalProperties": false,
+ "properties": {
+ "default": {
+ "description": "Default global security context.",
+ "type": "object",
+ "additionalProperties": false,
+ "properties": {
+ "securityContext": {
+ "description": "Global Pod security context as defined in https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core",
+ "type": "object",
+ "default": "See values.yaml",
Review comment:
Hi @jedcunningham, the changes were applied as requested.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r720453456
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
Review comment:
```suggestion
This function is required for backwards compatibility
```
##########
File path: chart/templates/cleanup/cleanup-cronjob.yaml
##########
@@ -22,6 +22,8 @@
{{- $nodeSelector := or .Values.cleanup.nodeSelector .Values.nodeSelector }}
{{- $affinity := or .Values.cleanup.affinity .Values.affinity }}
{{- $tolerations := or .Values.cleanup.tolerations .Values.tolerations }}
+{{- $securityContext := or .Values.cleanup.securityContext (include "defaultSecurityContext" . | mustFromJson ) }}
+{{- $containerSecurityContext := or .Values.cleanup.containerSecurityContext (include "defaultContainerSecurityContext" . | mustFromJson ) }}
Review comment:
```suggestion
{{- $securityContext := or .Values.cleanup.securityContext (include "defaultSecurityContext" . | mustFromJson) }}
{{- $containerSecurityContext := or .Values.cleanup.containerSecurityContext (include "defaultContainerSecurityContext" . | mustFromJson) }}
```
nit
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Can this be consistent with how `gitsync` is handled (I think the non-dict route is more readable)?. That should also work for setting more than 1, no?
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
I don't think we should set these at the container level. We should only set it at the pod level, otherwise choices for `podSecurity.securityContext.runAsUser` or `scheduler.podSecurity.securityContext.runAsUser` will be just be overridden by this default.
Similarly, if we want to set runAsGroup by default, we should set it only in the `defaultSecurityContext` I think.
That probably means we don't need this helper template and the logic for defaulting the container securityContext can be simpler?
Am I overlooking something?
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,234 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+import pytest
+
+from tests.helm_template_generator import render_chart
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+ assert 3000 == jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.containers[0].securityContext.runAsGroup", docs[index])
+
+ def test_check_statsd_uid(self):
+ docs = render_chart(
+ values={
+ "statsd": {"enabled": True, "uid": 3000}
+ },
+ show_only=["templates/statsd/statsd-deployment.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 3000 == jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser", docs[index])
+
+ def test_check_cleanup_job(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "cleanup": {"enabled": True}
+ },
+ show_only=["templates/cleanup/cleanup-cronjob.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.jobTemplate.spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.jobTemplate.spec.template.spec.securityContext.fsGroup", docs[index])
+ assert 3000 == jmespath.search("spec.jobTemplate.spec.template.spec.containers[0].securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.jobTemplate.spec.template.spec.containers[0].securityContext.runAsGroup", docs[index])
+
+ def test_gitsync_sidecar_and_init_container(self):
+ docs = render_chart(
+ values={
+ "dags": {"gitSync": {"enabled": True, "uid": 3000}},
+ "airflowVersion": "1.10.15",
+ },
+ show_only=[
+ "templates/workers/worker-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert "git-sync" in [c["name"] for c in jmespath.search("spec.template.spec.containers", docs[index])]
+ assert "git-sync-init" in [c["name"] for c in jmespath.search("spec.template.spec.initContainers", docs[index])]
+ assert 3000 == jmespath.search("spec.template.spec.initContainers[?name=='git-sync-init'].securityContext.runAsUser | [0]", docs[index])
+ assert 3000 == jmespath.search("spec.template.spec.containers[?name=='git-sync'].securityContext.runAsUser | [0]", docs[index])
+
+class TestSecurityContext:
+ # Test podSecurity setting for Pods and Containers
+ def test_check_default_setting(self):
+ docs = render_chart(
+ values={
+ "podSecurity": {
+ "securityContext": {"runAsUser": 6000, "fsGroup": 60},
+ "containerSecurityContext": {"runAsUser": 4000, "runAsGroup": 40}
+ },
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "statsd": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ "templates/statsd/statsd-deployment.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ print (docs[index])
+ assert 6000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 60 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+ assert 4000 == jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser", docs[index])
+ assert 40 == jmespath.search("spec.template.spec.containers[0].securityContext.runAsGroup", docs[index])
+
+ # Test priority:
+ # <local>.containerSecurityContext > podSecurity.containerSecurityContext > uid + gid
+ # <local>.containerSecurityContext > podSecurity.containerSecurityContext > uid + gid
+ def test_check_local_setting(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "podSecurity": {
+ "securityContext": {"runAsUser": 6000, "fsGroup": 60},
+ "containerSecurityContext": {"runAsUser": 4000, "runAsGroup": 40}
+ },
+ "webserver": {
+ "defaultUser": {"enabled": True},
+ "securityContext": {"runAsUser": 9000, "fsGroup": 90},
+ "containerSecurityContext": {"runAsUser": 8000, "runAsGroup": 80}
Review comment:
So we don't have to define these for all of them, maybe pull these out and do something like this instead:
```
# define this above
component_contexts = {
"securityContext": {"runAsUser": 9000, "fsGroup": 90},
"containerSecurityContext": {"runAsUser": 8000, "runAsGroup": 80}
}
```
Then change this section (and the rest to):
```suggestion
**component_contexts
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r721587250
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
Make sense except for gitSync I think. Should I leave gitSync with the option?
That would also mean that for gitSync we would have the RunAsUser.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-926835942
Hello @nwalens - can you rebase please?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740594865
##########
File path: chart/values.schema.json
##########
@@ -1629,6 +1666,18 @@
"description": "Annotations to add to the triggerer pods.",
"type": "object",
"default": {}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
Review comment:
```suggestion
"description": "Triggerer Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
```
Or similar, and for all the others that aren't actually global?
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+
+ def test_check_statsd_uid(self):
+ docs = render_chart(
+ values={"statsd": {"enabled": True, "uid": 3000}},
+ show_only=["templates/statsd/statsd-deployment.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
Review comment:
```suggestion
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
```
Both here and elsewhere.
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Both here and elsewhere.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ pluck "runAsUser" $securityContext | first }}:{{ pluck "fsGroup" $securityContext | first }}"
Review comment:
Is this pluck the only reason we use the `mustFromJson | toYaml` pattern everywhere?
Might be better to simplify the common case and just handle this case slightly differently? Haven't experimented with this at all, just my off the cuff thoughts.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740674914
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
##########
File path: chart/values.schema.json
##########
@@ -1203,6 +1216,18 @@
]
}
}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
+ "type": "object",
Review comment:
Can you use a more strict schema?
https://github.com/apache/airflow/pull/19181
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740594865
##########
File path: chart/values.schema.json
##########
@@ -1629,6 +1666,18 @@
"description": "Annotations to add to the triggerer pods.",
"type": "object",
"default": {}
+ },
+ "securityContext": {
+ "description": "Global Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
Review comment:
```suggestion
"description": "Triggerer Pod security context as defined in `Pod Security Context <https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#podsecuritycontext-v1-core>`_. If not set, the values from `securityContext` will be used.",
```
Or similar, and for all the others that aren't actually global?
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+
+ def test_check_statsd_uid(self):
+ docs = render_chart(
+ values={"statsd": {"enabled": True, "uid": 3000}},
+ show_only=["templates/statsd/statsd-deployment.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
Review comment:
```suggestion
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
```
Both here and elsewhere.
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,206 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Both here and elsewhere.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ pluck "runAsUser" $securityContext | first }}:{{ pluck "fsGroup" $securityContext | first }}"
Review comment:
Is this pluck the only reason we use the `mustFromJson | toYaml` pattern everywhere?
Might be better to simplify the common case and just handle this case slightly differently? Haven't experimented with this at all, just my off the cuff thoughts.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r740672226
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
+ fsGroup: 0
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
Review comment:
where is it getting 1001 from since it wasn't provided in config? i guess these are not configurable?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+
+And finally if we set ``securityContext`` but not ``workers.securityContext``:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was not defined in ``workers``, the values from securityContext will take priority
+ runAsUser: 5000
Review comment:
50,000?
##########
File path: docs/helm-chart/production-guide.rst
##########
@@ -258,3 +258,140 @@ In order to enable the usage of SCCs, one must set the parameter :ref:`rbac.crea
In this chart, SCCs are bound to the Pods via RoleBindings meaning that the option ``rbac.create`` must also be set to ``true`` in order to fully enable the SCC usage.
For more information about SCCs and what can be achieved with this construct, please refer to `Managing security context constraints <https://docs.openshift.com/container-platform/latest/authentication/managing-security-context-constraints.html#scc-prioritization_configuring-internal-oauth/>`_.
+
+Security Context
+----------------
+
+In Kubernetes a ``securityContext`` can be used to define user ids, group ids and capabilities such as running a container in privileged mode.
+
+When deploying an application to Kubernetes, it is recommended to give the least privilege to containers so as
+to reduce access and protect the host where the container is running.
+
+In the Airflow Helm chart, the ``securityContext`` can be configured in several ways:
+
+ * :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>`_)
+
+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:
+
+.. code-block:: yaml
+
+ workers:
+ securityContext:
+ runAsUser: 5000
+ fsGroup: 0
+
+In the example above, the global Pod ``securityContext`` will be set to ``runAsUser: 5000`` and ``runAsGroup: 0``.
+
+As one can see, the local setting will take precedence over the global setting when defined. The following explains the precedence rule for ``securityContext`` options in this chart:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext:
+ runAsUser: 50000
+ fsGroup: 0
+
+ workers:
+ securityContext:
+ runAsUser: 1001
+ fsGroup: 0
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext: # As the securityContext was defined in ``workers``, its value will take priority
+ runAsUser: 1001
+ fsGroup: 0
+
+If we remove both the ``securityContext`` and ``workers.securityContext`` from the example above, the output will be the following:
+
+.. code-block:: yaml
+
+ uid: 40000
+ gid: 0
+
+ securityContext: {}
+
+ workers:
+ securityContext: {}
+
+This will generate the following worker deployment:
+
+.. code-block:: yaml
+
+ kind: StatefulSet
+ apiVersion: apps/v1
+ metadata:
+ name: airflow-worker
+ spec:
+ serviceName: airflow-worker
+ template:
+ spec:
+ securityContext:
+ runAsUser: 40000 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from uid will be used
+ fsGroup: 0 # As the securityContext was not defined in ``workers`` or ``podSecurity``, the value from gid will be used
+ initContainers:
+ - name: wait-for-airflow-migrations
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
+ ...
+ containers:
+ - name: worker
+ securityContext:
+ runAsUser: 1001
+ runAsGroup: 0
Review comment:
i tried this with the given config and found that runas and fs are only output for `spec.template.spec.securityContext` but not the others
##########
File path: chart/values.yaml
##########
@@ -384,6 +390,12 @@ workers:
maxSurge: "100%"
maxUnavailable: "50%"
+ # When not set, the values defined on podSecurity.securityContext will be used
Review comment:
re
`podSecurity.securityContext`
it does not seem you have actually added a `podSecurity` node?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r742089969
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
Review comment:
```suggestion
If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid
```
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,189 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+
+from tests.helm_template_generator import render_chart
+
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ ],
+ )
+
+ assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", docs[0])
+ assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", docs[0])
Review comment:
```suggestion
for doc in docs:
assert 3000 == jmespath.search("spec.template.spec.securityContext.runAsUser", doc)
assert 30 == jmespath.search("spec.template.spec.securityContext.fsGroup", doc)
```
Sorry, my last set of suggestions may not have been clear. For the tests where we do have more than 1 doc (e.g. more than 1 template in `show_only` in this case), we should check them all.
##########
File path: chart/templates/workers/worker-deployment.yaml
##########
@@ -111,7 +110,7 @@ spec:
command:
- chown
- -R
- - "{{ .Values.uid }}:{{ .Values.gid }}"
+ - "{{ include "globalSecurityContextIds" (list . .Values.workers) }}"
Review comment:
I'd wondering if we'd be better off doing something like this instead (untested, but you get the idea):
`bash -c 'exec chown -R $(id -u):$(id -g) {{ template "airflow_logs" . }}`
Then `chown` can just use the uid/gid it's running as instead of being configured explicitly.
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +609,50 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default value for securityContext
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | 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 secirityCpntext or <node>.securityContext, defaults to UID in the local node
+*/}}
+{{- define "localSecurityContext" -}}
+ {{- if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else -}}
+runAsUser: {{ .uid }}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for workers chown for persistent storage
+If no value is passed for secirityCpntext or <node>.securityContext, defaults to global uid and gid
+*/}}
+{{- define "globalSecurityContextIds" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContext -}}
+{{ pluck "runAsUser" .securityContext | first }}:{{ pluck "fsGroup" .securityContext | first }}
Review comment:
Does this work if only `securityContext.runAsUser` 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj removed a comment on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
mik-laj removed a comment on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-957039017
What do you think about adding a guide about this feature similar to [init/sidecar container guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/using-additional-containers.html), [resource guide](http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/setting-resources-for-containers.html)?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] nwalens commented on pull request #18249: Add support for securityContext per deployment
Posted by GitBox <gi...@apache.org>.
nwalens commented on pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#issuecomment-995599940
> I don't _think_ you have a test that covers when you have a partial global security setting (say) UID, but a local security setting only GID
>
> Could you add that please?
@ashb - definitely, will do it shortly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org