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 2022/08/09 18:29:12 UTC
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r941662272
##########
chart/values.schema.json:
##########
@@ -1907,6 +1935,34 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container",
Review Comment:
```suggestion
"description": "Security context definition for the scheduler.",
```
##########
chart/templates/NOTES.txt:
##########
@@ -176,6 +176,14 @@ DEPRECATION WARNING:
{{- if not (or .Values.webserverSecretKey .Values.webserverSecretKeySecretName) }}
+{{- if .Values.securityContext }}
+
+ DEPRECATION WARNING:
+ `securityContext` has been renamed to `securityContexts`, to be enabled on container and pod level.
Review Comment:
We also need the corresponding update to the `airflowSecurityContext` helper to use the new config also.
##########
chart/values.yaml:
##########
@@ -35,12 +35,21 @@ revisionHistoryLimit: ~
uid: 50000
gid: 0
-# Default security context for airflow
+# Default security context for airflow (deprecated, use below in the future)
securityContext: {}
# runAsUser: 50000
# fsGroup: 0
# runAsGroup: 0
+# Detailed default security context for airflow deployments
+securityContexts:
+ pod: {}
+ containers:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop:
+ - ALL
Review Comment:
This is the default, yeah? Not sure we need to expand it here.
##########
chart/values.schema.json:
##########
@@ -100,6 +100,34 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container",
Review Comment:
```suggestion
"description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container.",
```
Missing some punctuation.
##########
tests/charts/test_scheduler.py:
##########
@@ -304,6 +304,38 @@ def test_logs_persistence_changes_volume(self, log_persistence_values, expected_
assert {"name": "logs", **expected_volume} in jmespath.search("spec.template.spec.volumes", docs[0])
+ def test_scheduler_securityContexts_are_configurable(self):
+ docs = render_chart(
+ values={
+ "scheduler": {
+ "securityContexts": {
+ "pod": {
+ "fsGroup": 1000,
+ 'runAsGroup': 1000,
+ 'runAsNonRoot': "true",
+ 'runAsUser': 1000,
+ },
+ "container": {
+ "allowPrivilegeEscalation": "false",
+ 'readOnlyRootFilesystem': "true",
+ },
+ }
+ },
+ },
+ show_only=["templates/scheduler/scheduler-deployment.yaml"],
+ )
+ assert "false" == jmespath.search(
+ "spec.template.spec.containers[0].securityContext.allowPrivilegeEscalation", docs[0]
+ )
+ assert "true" == jmespath.search(
+ "spec.template.spec.containers[0].securityContext.readOnlyRootFilesystem", docs[0]
+ )
Review Comment:
```suggestion
assert {"allowPrivilegeEscalation": False, "readOnlyRootFilesystem": True} == jmespath.search(
"spec.template.spec.containers[0].securityContext", docs[0]
)
```
Might be cleaner to check the whole thing in 1 go (probably need some formatting, but you get the idea).
##########
chart/values.schema.json:
##########
@@ -1907,6 +1935,34 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "properties": {
+ "pod": {
+ "description": "Pod security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods",
+ "type": "object",
+ "$ref": "#/definitions/io.k8s.api.core.v1.PodSecurityContext",
+ "default": {},
+ "x-docsSection": "Kubernetes",
+ "examples": [
+ {
+ "runAsUser": 50000,
+ "runAsGroup": 0,
+ "fsGroup": 0
+ }
+ ]
+ },
+ "container": {
+ "description": "Container security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific containers",
Review Comment:
```suggestion
"description": "Container security context definition for the scheduler.",
```
##########
chart/values.schema.json:
##########
@@ -1907,6 +1935,34 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "properties": {
+ "pod": {
+ "description": "Pod security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods",
Review Comment:
```suggestion
"description": "Pod security context definition for the scheduler.",
```
##########
chart/values.schema.json:
##########
@@ -100,6 +100,34 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods/Container",
+ "type": "object",
+ "x-docsSection": "Kubernetes",
+ "properties": {
+ "pod": {
+ "description": "Pod security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific Pods",
+ "type": "object",
+ "$ref": "#/definitions/io.k8s.api.core.v1.PodSecurityContext",
+ "default": {},
+ "x-docsSection": "Kubernetes",
+ "examples": [
+ {
+ "runAsUser": 50000,
+ "runAsGroup": 0,
+ "fsGroup": 0
+ }
+ ]
+ },
+ "container": {
+ "description": "Container security context definition. The values in this parameter will be used when `securityContexts` is not defined for specific containers",
+ "type": "object",
+ "$ref": "#/definitions/io.k8s.api.core.v1.SecurityContext",
+ "default": {},
Review Comment:
There is a default though?
##########
tests/charts/test_scheduler.py:
##########
@@ -304,6 +304,38 @@ def test_logs_persistence_changes_volume(self, log_persistence_values, expected_
assert {"name": "logs", **expected_volume} in jmespath.search("spec.template.spec.volumes", docs[0])
+ def test_scheduler_securityContexts_are_configurable(self):
+ docs = render_chart(
+ values={
+ "scheduler": {
+ "securityContexts": {
+ "pod": {
+ "fsGroup": 1000,
+ 'runAsGroup': 1000,
Review Comment:
```suggestion
'runAsGroup': 2000,
```
Might be nice to use different ints here.
--
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