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/06/21 20:43:15 UTC
[GitHub] [airflow] ChrisFraun opened a new pull request, #24588: add: container securityContext not available in podSecurityContext
ChrisFraun opened a new pull request, #24588:
URL: https://github.com/apache/airflow/pull/24588
<!--This PR is only a small change in the helm chart of Airflow.
What: Deployments can have security settings in their manifest on two levels: pod and container. However, there are some capabilities only configurable in one of the respective levels(https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#securitycontext-v1-core). This PR sets a default configuration for container securityContext, which denies privilege escalation and drops all POSIX capabilities. These are and should be standard settings in the context of Kubernetes. It also adds the possibility of running Airflow in an Kubernetes environment without PSP (to be removed in v1.25 https://kubernetes.io/docs/concepts/security/pod-security-policy/), but with OpenPolicyAgent (a or possibly the PSP substitute) with the same capabilities as a restricted PSP instead.
Why: This missing configuration restricts Airflow from being used with the simple upstream helm chart without modifications/unnecessary maintenance. This applies especially the restricted policy use in OPA. The specific setting in this PR is **not** inherited from `podSecurityContext`(pod level) in `securityContext`(container level).
Problem: There is already a `securityContext` in the values.yaml, however, this should be actually be called `podSecurityContext` since it's on pod level. To not break backwards compatibility of Airflow, this PR hardcodes the respective capabilities on container level for statsd, scheduler and webserver.
The other possibility would be to introduce a `containerSecurityContext` in the values.yaml, which is a made up word since it is commonly called `scurityContext`.
Benefit in either case would be a more secure deployment.
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
Could not find any related issue at first sight.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
Test was a simple `helm lint .` on chart level as well as a successful deployment.
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/christophfraundorfer/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/christophfraundorfer/.kube/config
==> Linting .
1 chart(s) linted, 0 chart(s) failed
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r944455439
##########
chart/templates/_helpers.yaml:
##########
@@ -703,14 +703,14 @@ Create the name of the cleanup service account to use
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContext.pod or <node>.securityContext.pod, defaults to global uid and gid.
Review Comment:
You are right, I tried to adapt to your suggestion and renamed also the template.
Btw `airflowSecurityContextIds` was not used anywhere so the renamed version `airflowPodSecurityContextsIds` can probably be deleted.
Hope I got it right this time.
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1157685121
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext or .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
Review Comment:
```suggestion
securityContext: {{ or $containerSecurityContext .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
```
Getting the following error in my test setup. Proposed change should fix it:
```
Error: template: test-airflow/charts/airflow/templates/webserver/webserver-deployment.yaml:166:30: executing "test-airflow/charts/airflow/templates/webserver/webserver-deployment.yaml" at <$containerSecurityContext>: can't give argument to non-function $containerSecurityContext
```
`helm version` output:
```
version.BuildInfo{Version:"v3.11.0", GitCommit:"472c5736ab01133de504a826bd9ee12cbe4e7904", GitTreeState:"clean", GoVersion:"go1.18.10"}
```
This is probably due to what is explained [here](https://github.com/helm/helm/issues/7711#issuecomment-593113347)
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r942427634
##########
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:
actually I moved it to the helper: https://github.com/apache/airflow/blob/85ec1fede2848ae8dc45dca8d0f933c9e400c9f7/chart/templates/_helpers.yaml#L786-L814
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1291023831
Running the tests now (sorry @ChrisFraun for this all taking so long - just returned from Holidays and trying to catch-up - let's see how this one looks like for tests and I will give it another pass).
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1531048979
Feel free to go ahead @mikaeld! π
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1314029889
shouldn't it be there with https://github.com/apache/airflow/pull/24588/files#diff-dc12e5cc8016c85ad9e662af7c8b8cfb49d91965225e94aa12146dc1f371fc86R824?
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r942430314
##########
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:
tried to change the helper 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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1210654262
Hi @jedcunningham thanks a lot for the input! I tried to stick to your comments, but feel free to add some more :)
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r944412466
##########
chart/values.schema.json:
##########
@@ -100,6 +100,39 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition for the scheduler.",
Review Comment:
Ah, yes I missunderstood. I think I should have looked at it properly as well. Tried to change it accordingly now.
##########
chart/values.schema.json:
##########
@@ -100,6 +100,39 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition for the scheduler.",
Review Comment:
Ah, yes I misunderstood. I think I should have looked at it properly as well. Tried to change it accordingly now.
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1516496771
You likely can revert and/or rewrite history to fix this since this is on your fork. If you can't fix the conflicts, we would need to link this PR in the new one to keep the context. @potiuk thoughts on this?
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1180258779
Yep it looks good :). I am not sure if there is an easy way to "warn" if you use the deprecated configuration (I think this is the only change that I would like to see here) - and of course some tests would be useful. @jedcunningham @dstandish - WDYT?
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r943717149
##########
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:
Sorry, should have been more clear. Isn't that the default kubernetes sets if you provide nothing? If so, I'd rather we not set it at all.
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r973172870
##########
chart/templates/_helpers.yaml:
##########
@@ -755,33 +761,68 @@ runAsUser: {{ .uid }}
{{/*
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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +------------------------+ +-----------------+ +------------------------+ +-----------------+ +----------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "localPodSecurityContextsIds" (list . .Values.webserver)
Review Comment:
```suggestion
include "airflowPodSecurityContextsIds" (list . .Values.webserver)
```
##########
chart/templates/_helpers.yaml:
##########
@@ -755,33 +761,68 @@ runAsUser: {{ .uid }}
{{/*
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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +------------------------+ +-----------------+ +------------------------+ +-----------------+ +----------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "localPodSecurityContextsIds" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
*/}}
-{{- define "airflowSecurityContextIds" -}}
+{{- define "airflowPodSecurityContextsIds" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext -}}
-{{ pluck "runAsUser" .securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContext | first | default $.Values.gid }}
+ {{- if .securityContexts.pod -}}
+{{ pluck "runAsUser" .securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ pluck "runAsUser" $.Values.securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContexts.pod | first | default $.Values.gid }}
{{- else if $.Values.securityContext -}}
{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
{{- else -}}
{{ $.Values.uid }}:{{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
+
+{{/*
+Set the default value for container securityContext
+If no value is passed for securityContexts.container or <node>.securityContexts.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.container | -> | securityContexts.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
+ +------------------------+ +-----------------+ +-------------------------+
+
+
+The template can be called like so:
+ include "airflowContainerSecurityContext" (list . .Values.statsd)
+
+Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "airflowContainerSecurityContext" -}}
Review Comment:
I wonder if we should call this `containerSecurityContext` instead, since it's used for more than just Airflow containers?
##########
chart/templates/_helpers.yaml:
##########
@@ -755,33 +761,68 @@ runAsUser: {{ .uid }}
{{/*
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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +------------------------+ +-----------------+ +------------------------+ +-----------------+ +----------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "localPodSecurityContextsIds" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
*/}}
-{{- define "airflowSecurityContextIds" -}}
+{{- define "airflowPodSecurityContextsIds" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext -}}
-{{ pluck "runAsUser" .securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContext | first | default $.Values.gid }}
+ {{- if .securityContexts.pod -}}
+{{ pluck "runAsUser" .securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ pluck "runAsUser" $.Values.securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContexts.pod | first | default $.Values.gid }}
{{- else if $.Values.securityContext -}}
{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
{{- else -}}
{{ $.Values.uid }}:{{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
+
+{{/*
+Set the default value for container securityContext
+If no value is passed for securityContexts.container or <node>.securityContexts.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.container | -> | securityContexts.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
+ +------------------------+ +-----------------+ +-------------------------+
+
+
+The template can be called like so:
+ include "airflowContainerSecurityContext" (list . .Values.statsd)
+
+Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "airflowContainerSecurityContext" -}}
Review Comment:
Wait, this isn't even used? Let's drop it?
##########
chart/templates/_helpers.yaml:
##########
@@ -703,26 +703,30 @@ Create the name of the cleanup service account to use
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <node>.securityContext, defaults to global uid and gid.
- +------------------------+ +-----------------+ +-------------------------+
- | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
- +------------------------+ +-----------------+ +-------------------------+
+ +------------------------+ +-----------------+ +-----------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> | securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+ +------------------------+ +-----------------+ +-----------------+ +-----------------+ +-------------------------+
Review Comment:
```suggestion
+-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
| <node>.securityContexts.pod | -> | <node>.securityContext | -> | securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
```
nit: let's keep these prettty
##########
chart/templates/_helpers.yaml:
##########
@@ -733,20 +737,22 @@ fsGroup: {{ $.Values.gid }}
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node.
+Set the default value for pod securityContext
+If no value is passed for <node>.securityContexts.pod or <node>.securityContext, defaults to UID in the local node.
- +------------------------+ +-------------+
- | <node>.securityContext | > | <node>.uid |
- +------------------------+ +-------------+
+ +------------------------+ +-----------------+ +-------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | > | <node>.uid |
+ +------------------------+ +-----------------+ +-------------+
Review Comment:
```suggestion
+-----------------------------+ +------------------------+ +-------------+
| <node>.securityContexts.pod | -> | <node>.securityContext | -> | <node>.uid |
+-----------------------------+ +------------------------+ +-------------+
```
(I'm not going to do the rest via suggestions, but you get the idea)
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r942430894
##########
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:
I removed it from here and put it in the helper:
{{/*
Set the default value for container securityContext
If no value is passed for securityContext.container or <node>.securityContext.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+------------------------+ +-----------------+ +-------------------------+
| <node>.securityContext.container | -> | securityContext.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
+------------------------+ +-----------------+ +-------------------------+
The template can be called like so:
include "airflowSecurityContextContainer" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
*/}}
{{- define "airflowSecurityContextContainer" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext.container -}}
{{ toYaml .securityContext.container | print }}
{{- else if $.Values.securityContext.container -}}
{{ toYaml $.Values.securityContext.container | print }}
{{- else -}}
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
{{- end -}}
{{- end -}}
{{- end -}}
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1156795262
##########
chart/templates/triggerer/triggerer-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: triggerer
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
Thanks for spotting: I tried to add https://github.com/apache/airflow/pull/24588/files#diff-747f774dc1498138f6e6b9b4ab1fef11473ef9e05ee8ba85e2a3755ebd114ac3R225 to fix it
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511915877
PYTHONPATH is empty, which is exactly what I would expect from creating a new virtualenv in a new pyenv environment. I'm not exactly used to the library setup as I usually work on applications so there might be some implicit detail I'm missing. I can run db migrations using `python -m airflow db init` but `airflow db init` yields the import error.
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1239630890
> Hi, I am still getting this error during the images build: `ERROR: denied: permission_denied: write_package` It does not seem to me that this has something to do with my changes. Any advice?
Rebase please. GitHub seemed to have a rough day.
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1367458636
> @potiuk pretty hard to grok these test failures. I found this though:
>
> ```
> stderr = b'W1206 16:45:48.861465 1052 loader.go:221]
> Config not found: /files/.kube/config\nW1206 16:45:48.879883 1052 lo..." at
> <.securityContexts.pod>: nil pointer evaluating interface {}.pod\n\nUse --debug
> flag to render out invalid YAML\n'
> ```
>
> What is the best way to iterate on this? I think there shouldn't be too much work left here.
Well. First of all there are conflicts - so it needs rebase and fixing them. And if the test are Python helm unit tests, then this is nothing unusual - just regular pytest tests that you can iterate on and the only difference vs. standard airflow unit tests are requirements needed (and helm installed) . Nothing special there.
And as usual (and described for all other parts of our test suite) Helm Unit tests have a separate chapter in our TESTING.rst that describe how they work and how you should run the tests locally https://github.com/apache/airflow/blob/main/TESTING.rst#helm-unit-tests
--
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] malthe commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1367163635
@potiuk pretty hard to grok these test failures. I found this though:
```
stderr = b'W1206 16:45:48.861465 1052 loader.go:221]
Config not found: /files/.kube/config\nW1206 16:45:48.879883 1052 lo..." at
<.securityContexts.pod>: nil pointer evaluating interface {}.pod\n\nUse --debug
flag to render out invalid YAML\n'
```
What is the best way to iterate on this? I think there shouldn't be too much work left 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
[GitHub] [airflow] potiuk commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1509582035
> unfortunately the checks are still failing and I have no means of testing them locally..
What do you mean by no means of testing locally?
All our tests are specifically designed to be possible (and generally easy) to run locally - see https://github.com/apache/airflow/blob/main/TESTING.rst that describe how to run every type of test we have locally.
Do you sse a problem with running some kind of tests? Happy to help if you do. I just need to know what problem you have when following those instructions above.
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1156795789
##########
chart/templates/statsd/statsd-deployment.yaml:
##########
@@ -87,6 +88,9 @@ spec:
- name: statsd
image: {{ template "statsd_image" . }}
imagePullPolicy: {{ .Values.images.statsd.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
+ args:
+ - "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- if .Values.statsd.args }}
args: {{ tpl (toYaml .Values.statsd.args) . | nindent 12 }}
{{- end }}
Review Comment:
thanks for the suggestion!
I added it
--
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 closed pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #24588: add: container securityContext not available in podSecurityContext
URL: https://github.com/apache/airflow/pull/24588
--
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] malthe commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1314014287
@ChrisFraun there seems to be an error running the Helm chart:
```
parse error at (airflow/templates/webserver/webserver-deployment.yaml:65): undefined variable "$containerSecurityContext"
```
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1330464314
then for the above mentioned error it should be here: https://github.com/apache/airflow/pull/24588/files#diff-42f2a460a872212b923b5d7591c552a22816d768478a70d6943103221e08f11fR27, no?
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r973161043
##########
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:
Sorry for the delay, coming back to this now.
I had this backwards - this removes rights. While I agree this is a good sane default, we can't make this change in a minor release without breaking backward compatibility. Given that, I think for now we should keep the default as nothing.
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r975118361
##########
chart/templates/_helpers.yaml:
##########
@@ -755,33 +761,68 @@ runAsUser: {{ .uid }}
{{/*
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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +------------------------+ +-----------------+ +------------------------+ +-----------------+ +----------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "localPodSecurityContextsIds" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
*/}}
-{{- define "airflowSecurityContextIds" -}}
+{{- define "airflowPodSecurityContextsIds" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext -}}
-{{ pluck "runAsUser" .securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContext | first | default $.Values.gid }}
+ {{- if .securityContexts.pod -}}
+{{ pluck "runAsUser" .securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ pluck "runAsUser" $.Values.securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContexts.pod | first | default $.Values.gid }}
{{- else if $.Values.securityContext -}}
{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
{{- else -}}
{{ $.Values.uid }}:{{ $.Values.gid }}
{{- end -}}
{{- end -}}
{{- end -}}
+
+{{/*
+Set the default value for container securityContext
+If no value is passed for securityContexts.container or <node>.securityContexts.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+
+ +------------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.container | -> | securityContexts.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
+ +------------------------+ +-----------------+ +-------------------------+
+
+
+The template can be called like so:
+ include "airflowContainerSecurityContext" (list . .Values.statsd)
+
+Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "airflowContainerSecurityContext" -}}
Review Comment:
I added it to the charts
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1180731598
Deprecation warnings happen in NOTES, e.g: https://github.com/apache/airflow/blob/178af9d24772a8866ac55d25eeb48bed77337031/chart/templates/NOTES.txt#L160-L166
This should be expanded to cover all of the components as well.
I'd also like to see the component specific override come from the components config section, e.g. `scheduler.securityContexts.pod` instead of `securityContexts.pod.scheduler`.
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1454909252
Still errors . I stronly recommend installing `pre-commit` and using it. Then a number of iterations you would have to do would be much smaller .
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r942383960
##########
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:
tried to add some variation
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1161742076
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext or .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
Review Comment:
@ChrisFraun Hey! any chance to look at the proposed changes? Thanks!
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1188808698
Hi, sorry for the late reply!
what was added:
- added deprecation notes
- Modified values.yaml (webserver, scheduler, statsd)
- Modified deployments (webserver, scheduler, statsd)
- Modified values.schema (webserver, scheduler, statsd)
Tested with `helm lint .` and `helm template . --debug` - both compiled.
Is this the way you imagine it?
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r944398631
##########
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:
I think you have to set them yourself
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r985964043
##########
chart/values.schema.json:
##########
@@ -1608,21 +1689,65 @@
}
],
"$ref": "#/definitions/io.k8s.api.core.v1.ResourceRequirements"
+ },
+ "containerSecurityContext": {
+ "description": "Container security context definition. The values in this parameter will be used when `containerSecurityContext` is not defined for specific Containers",
+ "type": "object",
+ "$ref": "#/definitions/io.k8s.api.core.v1.SecurityContext",
+ "default": {
+ "runAsUser": 0
+ },
+ "x-docsSection": "Kubernetes",
+ "examples": [
+ {
+ "allowPrivilegeEscalation": false,
+ "capabilities": {
+ "drop": [
+ "ALL"
+ ]
+ }
+ }
+ ]
}
}
},
- "securityContext": {
Review Comment:
We need to leave these old deprecated ones in the schema also, otherwise helm won't allow you to actually deploy any longer.
Might be worth adding a test or 2 to cover the old ones to make sure they keep working.
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1251514439
Conflict to solve :(
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r975610856
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -247,6 +249,7 @@ spec:
- name: worker-log-groomer
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
Same here, `workers.logGroomerSidecar.securityContexts.container`?
##########
chart/templates/scheduler/scheduler-deployment.yaml:
##########
@@ -134,6 +135,7 @@ spec:
{{ toYaml .Values.scheduler.resources | indent 12 }}
image: {{ template "airflow_image_for_migrations" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
Shouldn't this use `scheduler.waitForMigrations.securityContexts.container`?
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1162358847
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext or .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
Review Comment:
sure! Thanks for the suggest!
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1154515537
##########
chart/templates/statsd/statsd-deployment.yaml:
##########
@@ -87,6 +88,9 @@ spec:
- name: statsd
image: {{ template "statsd_image" . }}
imagePullPolicy: {{ .Values.images.statsd.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
+ args:
+ - "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- if .Values.statsd.args }}
args: {{ tpl (toYaml .Values.statsd.args) . | nindent 12 }}
{{- end }}
Review Comment:
`args` is duplicated
```suggestion
{{- if .Values.statsd.args }}
args: {{ tpl (toYaml .Values.statsd.args) . | nindent 12 }}
{{- else}}
args:
- "--statsd.mapping-config=/etc/statsd-exporter/mappings.yml"
{{- end }}
```
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -171,6 +175,7 @@ spec:
{{- end }}
resources:
{{ toYaml .Values.webserver.resources | indent 12 }}
+ securityContext: {{ or .Values.webserver.securityContexts.container .Values.securityContexts.container }}
Review Comment:
In my local testing, this adds a duplicate `securityContext` key
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r943702505
##########
chart/values.yaml:
##########
@@ -629,12 +634,17 @@ scheduler:
# (when not using LocalExecutor and workers.persistence)
strategy: ~
- # When not set, the values defined in the global securityContext will be used
+ # When not set, the values defined in the global securityContext will be used (deprecated)
securityContext: {}
# runAsUser: 50000
# fsGroup: 0
# runAsGroup: 0
+ # Detailed default security context for scheduler deployments for container and pod level
Review Comment:
```suggestion
# Detailed default security context for scheduler deployments for container and pod level
```
##########
chart/templates/_helpers.yaml:
##########
@@ -703,14 +703,14 @@ Create the name of the cleanup service account to use
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContext.pod or <node>.securityContext.pod, defaults to global uid and gid.
Review Comment:
I think we want this order of precedence:
```
<node>.securityContexts.pod
<node>.securityContext
securityContexts.pod
securityContext (backcompat for deprecated config)
uid/gid
```
So I think this template still needs a little work, right? Also feel free to rename this `airflowPodSecurityContext` to make it more clear.
##########
chart/values.yaml:
##########
@@ -894,12 +904,17 @@ webserver:
# Allow overriding Update Strategy for Webserver
strategy: ~
- # When not set, the values defined in the global securityContext will be used
+ # When not set, the values defined in the global securityContext will be used (deprecated)
securityContext: {}
# runAsUser: 50000
# fsGroup: 0
# runAsGroup: 0
+ # Detailed default security contexts for webserver deployments for container and pod level
Review Comment:
```suggestion
# Detailed default security contexts for webserver deployments for container and pod level
```
##########
chart/values.schema.json:
##########
@@ -100,6 +100,39 @@
}
]
},
+ "securityContexts": {
+ "description": "Security context definition for the scheduler.",
Review Comment:
Sorry, probably could have been more clear with my original comments. This one should still be global, and what you had before was good. I mean the component specific descriptions needs to be updated to reflect they are scheduler/webserver/etc specific.
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r942412758
##########
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:
removed the default to ensure backwards compatibility.
also I could not figure out how to reference it in https://github.com/apache/airflow/blob/1cc1d0a93e7445e0179178d370bfdc6dd8cab250/chart/values.schema.json#L126
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1509578651
unfortunately the checks are still failing and I have no means of testing them locally..
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511825931
I've nuked the virtualenv multiple times and arrive to the same outcome. Here's the latest attempt starting with a fresh Python interpreter.
#### Step 1
```shell
β― pyenv install 3.7.16
Downloading Python-3.7.16.tar.xz...
-> https://www.python.org/ftp/python/3.7.16/Python-3.7.16.tar.xz
Installing Python-3.7.16...
patching file Doc/library/ctypes.rst
patching file Lib/test/test_unicode.py
patching file Modules/_ctypes/_ctypes.c
patching file Modules/_ctypes/callproc.c
patching file Modules/_ctypes/ctypes.h
patching file setup.py
patching file 'Misc/NEWS.d/next/Core and Builtins/2020-06-30-04-44-29.bpo-41100.PJwA6F.rst'
patching file Modules/_decimal/libmpdec/mpdecimal.h
patching file setup.py
Installed Python-3.7.16 to /home/mikaeld/.pyenv/versions/3.7.16
β― pyenv local 3.7.16
β― python -V
Python 3.7.16
β― pip install virtualenv
Collecting virtualenv
Using cached virtualenv-20.21.0-py3-none-any.whl (8.7 MB)
Collecting importlib-metadata>=4.8.3
Using cached importlib_metadata-6.4.1-py3-none-any.whl (22 kB)
Collecting platformdirs<4,>=2.4
Using cached platformdirs-3.2.0-py3-none-any.whl (14 kB)
Collecting filelock<4,>=3.4.1
Using cached filelock-3.11.0-py3-none-any.whl (10.0 kB)
Collecting distlib<1,>=0.3.6
Using cached distlib-0.3.6-py2.py3-none-any.whl (468 kB)
Collecting typing-extensions>=3.6.4
Using cached typing_extensions-4.5.0-py3-none-any.whl (27 kB)
Collecting zipp>=0.5
Using cached zipp-3.15.0-py3-none-any.whl (6.8 kB)
Installing collected packages: distlib, zipp, typing-extensions, filelock, platformdirs, importlib-metadata, virtualenv
Successfully installed distlib-0.3.6 filelock-3.11.0 importlib-metadata-6.4.1 platformdirs-3.2.0 typing-extensions-4.5.0 virtualenv-20.21.0 zipp-3.15.0
WARNING: You are using pip version 22.0.4; however, version 23.1 is available.
You should consider upgrading via the '/home/mikaeld/.pyenv/versions/3.7.16/bin/python3.7 -m pip install --upgrade pip' command.
β― virtualenv .venv
created virtual environment CPython3.7.16.final.0-64 in 74ms
creator CPython3Posix(dest=/home/mikaeld/code/mozilla/airflow/.venv, clear=False, no_vcs_ignore=False, global=False)
seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/home/mikaeld/.local/share/virtualenv)
added seed packages: pip==23.0.1, setuptools==67.6.1, wheel==0.40.0
activators BashActivator,CShellActivator,FishActivator,NushellActivator,PowerShellActivator,PythonActivator
β― source .venv/bin/activate
```
### Step 2
```shell
β― pip install -e ".[devel]" \
--constraint "https://raw.githubusercontent.com/apache/airflow/constraints-main/constraints-source-providers-3.7.txt"
[output omitted]
[notice] A new release of pip is available: 23.0.1 -> 23.1
[notice] To update, run: pip install --upgrade pip
```
### Step 3
```shell
β― airflow db init
Traceback (most recent call last):
File "/home/mikaeld/code/mozilla/airflow/.venv/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/mikaeld/code/mozilla/airflow/airflow/__main__.py", line 48, in main
args.func(args)
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/cli_config.py", line 50, in command
func = import_string(import_path)
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/module_loading.py", line 36, in import_string
module = import_module(module_path)
File "/home/mikaeld/.pyenv/versions/3.7.16/lib/python3.7/importlib/__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
File "<frozen importlib._bootstrap>", line 983, in _find_and_load
File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 728, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/commands/db_command.py", line 30, in <module>
from airflow.utils.db_cleanup import config_dict, drop_archived_tables, export_archived_records, run_cleanup
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/db_cleanup.py", line 37, in <module>
from airflow import AirflowException
ImportError: cannot import name 'AirflowException' from 'airflow' (unknown location)
```
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1515937032
I messed up :(
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511791706
You have broken virtualenv. Nuke it and start from scratch following the instructions. Also in this case your venv can be initialized as with any other Python env (with editable install)
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r975616184
##########
chart/templates/scheduler/scheduler-deployment.yaml:
##########
@@ -134,6 +135,7 @@ spec:
{{ toYaml .Values.scheduler.resources | indent 12 }}
image: {{ template "airflow_image_for_migrations" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
If you'd rather not do it for every single container everywhere in this PR, that's fine with me. It can be expanded in a followup PR.
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1169549440
I'd say that better action would really be to have two separate contexts and maybe simply - rather than adding "containerSecurityContext", introduce `values/securityContexts/pod` and `values/securityContexts/container` as a bit more detailed structure in values, allowing more fine-grained security context configuration (while keeping deprecated default)
Something like:
<img width="547" alt="Screenshot 2022-06-29 at 07 28 03" src="https://user-images.githubusercontent.com/595491/176358546-0bdafb19-14c4-4787-adc5-b3d9d74f8f35.png">
I think that would be much more versatile and rather easy to use configure (more detailed values could be merged into the defaults).
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1336532930
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] potiuk commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511700537
See: https://github.com/apache/airflow/blob/main/TESTING.rst#helm-unit-tests
Those tests are simply executing a "helm" that you see there and you can reproduce what they do by runnig te command with some values generated dynamically - generate the same values (they got generated in /tmp/* file by the test) and convert the command parameter in the shell command if you want to reproduce what they do manually (make sure you run it in the same working directory the orignal command does..
The command will be something like:
"helm template release-name. ..."
based on:
```
'helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpjppcs087', '--kube-version', '1.23.13', '--namespace', 'default', '--show-only', 'templates/scheduler/scheduler-deployment.yaml', '--show-only', 'templates/workers/worker-deployment.yaml', '--show-only', 'templates/webserver/webserver-deployment.yaml
```
You can also run those tests from IDE via remote debugging or SSH debugging (described in CONTRIBUTING docs - including with screenshots - you can even pause breakpoint just before the command to inspect the environment and generated values file.
Or you can just install helm and run the tests locally - they should run if you have `helm` command line installed on your system (likely you need a POSIX system if you try to run it on Windows, it's not guaranteed to work on Windows).
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511703067
I just run the tests in my PyCharm in my virtualenv and other than requiring `helm` to be avialable on command line, it works as usual and I can even debug them step-by-step. You do not need Breeze to run them:
(I am no Linux)
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511776771
I'm also on PyCharm and Linux. Good to know those tests should be able to run locally as I was expecting. My issue is seemingly related to the Airflow database. I'm not sure why these tests require a database though. Here's the output to `./scripts/tools/initialize_virtualenv.py`.
```shell
[...]
Resetting AIRFLOW sqlite database...
Traceback (most recent call last):
File "/home/mikaeld/code/mozilla/airflow/.venv/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/mikaeld/code/mozilla/airflow/airflow/__main__.py", line 48, in main
args.func(args)
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/cli_config.py", line 50, in command
func = import_string(import_path)
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/module_loading.py", line 36, in import_string
module = import_module(module_path)
File "/home/mikaeld/.pyenv/versions/3.7.13/lib/python3.7/importlib/__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
File "<frozen importlib._bootstrap>", line 983, in _find_and_load
File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 728, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/commands/db_command.py", line 30, in <module>
from airflow.utils.db_cleanup import config_dict, drop_archived_tables, export_archived_records, run_cleanup
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/db_cleanup.py", line 37, in <module>
from airflow import AirflowException
ImportError: cannot import name 'AirflowException' from 'airflow' (unknown location)
Resetting AIRFLOW sqlite unit test database...
Traceback (most recent call last):
File "/home/mikaeld/code/mozilla/airflow/.venv/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/mikaeld/code/mozilla/airflow/airflow/__main__.py", line 48, in main
args.func(args)
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/cli_config.py", line 50, in command
func = import_string(import_path)
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/module_loading.py", line 36, in import_string
module = import_module(module_path)
File "/home/mikaeld/.pyenv/versions/3.7.13/lib/python3.7/importlib/__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
File "<frozen importlib._bootstrap>", line 983, in _find_and_load
File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 728, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/home/mikaeld/code/mozilla/airflow/airflow/cli/commands/db_command.py", line 30, in <module>
from airflow.utils.db_cleanup import config_dict, drop_archived_tables, export_archived_records, run_cleanup
File "/home/mikaeld/code/mozilla/airflow/airflow/utils/db_cleanup.py", line 37, in <module>
from airflow import AirflowException
ImportError: cannot import name 'AirflowException' from 'airflow' (unknown location)
Initialization of environment complete! Go ahead and develop Airflow!
```
Same error when running `airflow db init` in the `virtualenv`. Could it be related to the airflow package vs airflow directory?
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1296537230
errors ?
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1191178360
still some static checks are failing,
--
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] anja-istenic commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
anja-istenic commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1257553389
Thanks @ChrisFraun for reconciling our 2 PRs into one (https://github.com/apache/airflow/pull/25985 and this one). These changes look good to me π
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1252137872
Tried to add the changes demanded in [#](https://github.com/apache/airflow/pull/25985)
--
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] malthe commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
malthe commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1330303748
@ChrisFraun that snippet you linked to seems to point to a template definition, not a variable.
Variables are like this:
```yaml
{{- $securityContext := include "airflowSecurityContext" (list . .Values.cleanup) }}
```
--
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 diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
nwalens commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r947804136
##########
chart/templates/_helpers.yaml:
##########
@@ -703,14 +703,14 @@ Create the name of the cleanup service account to use
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContext.pod or <node>.securityContext.pod, defaults to global uid and gid.
Review Comment:
Hi, I think airflowSecurityContextIds is used on workers-deployment to set the uid and gid during a chown. Not sure if has changed.
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r944455439
##########
chart/templates/_helpers.yaml:
##########
@@ -703,14 +703,14 @@ Create the name of the cleanup service account to use
{{- end -}}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContext.pod or <node>.securityContext.pod, defaults to global uid and gid.
Review Comment:
You are right, I tried to adapt to your suggestion and renamed also the template.
Btw `airflowSecurityContextIds` was not used anywhere so the renamed version `localPodSecurityContextsIds` can probably be deleted.
Hope I got it right this time.
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1180166689
is the change how you imagined it @potiuk ?
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1195234355
Hi @potiuk can you maybe point me into a direction on why the checks are not running through? - I don't see the problem so far.
--
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] boring-cyborg[bot] commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1162330391
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1169667220
Updated the change, can you check it?
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r944398631
##########
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:
I think you have to set them yourself, they are not automatically 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] ChrisFraun commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1238989530
Hi, I am still getting this error during the images build: `ERROR: denied: permission_denied: write_package`
It does not seem to me that this has something to do with my changes.
Any advice?
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1367461717
> @potiuk pretty hard to grok these test failures. I found this though:
Also - I am not the author of those Helm Unit tests, but if you have an idea how to improve the output of tests, I think it would be great. Those tests simply (as explained in the docs) are just standard Pytest tests that simply render the charts using helm binary, taking values specified and verify if the rendered chart contains expected values. Possibly the output there can be improved if you think it is difficult to parse - any improvements there are most welcome. The chapter in the docs also explains the general structure of those tests. Maybe also the problem is that the charts have a bug and helm rendering fails there, and maybe that is a good opportunity to handle it better and in a more readable way ? Would love to review an improvement there :D
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r976403883
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -247,6 +249,7 @@ spec:
- name: worker-log-groomer
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
good point. I went through the sidecars and added the helper with the config from the values.yaml
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by GitBox <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1003242671
##########
chart/values.schema.json:
##########
@@ -1608,21 +1689,65 @@
}
],
"$ref": "#/definitions/io.k8s.api.core.v1.ResourceRequirements"
+ },
+ "containerSecurityContext": {
+ "description": "Container security context definition. The values in this parameter will be used when `containerSecurityContext` is not defined for specific Containers",
+ "type": "object",
+ "$ref": "#/definitions/io.k8s.api.core.v1.SecurityContext",
+ "default": {
+ "runAsUser": 0
+ },
+ "x-docsSection": "Kubernetes",
+ "examples": [
+ {
+ "allowPrivilegeEscalation": false,
+ "capabilities": {
+ "drop": [
+ "ALL"
+ ]
+ }
+ }
+ ]
}
}
},
- "securityContext": {
Review Comment:
added some legacy tests and also readded the schema for the deprecated securityContext, lets see what the test say :)
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1171883530
##########
chart/templates/_helpers.yaml:
##########
@@ -171,7 +171,7 @@ If release name contains chart name it will be used as a full name.
- name: {{ .Values.dags.gitSync.containerName }}{{ if .is_init }}-init{{ end }}
image: {{ template "git_sync_image" . }}
imagePullPolicy: {{ .Values.images.gitSync.pullPolicy }}
- securityContext: {{- include "localSecurityContext" .Values.dags.gitSync | nindent 4 }}
+ securityContext: {{ include "localPodSecurityContext" .Values.dags.gitSync | nindent 4 }}
Review Comment:
```suggestion
securityContext: {{ include "containerSecurityContext" (list . .Values.dags.gitSync) | nindent 4 }}
```
Other changes required :point_down:
### values.yaml
```yaml
dags:
# [...]
gitSync:
# [...]
securityContexts:
container: {}
```
and the equivalent schema in
### values.schemas.json
```json
"dags": {
"description": "DAGs settings.",
"type": "object",
"x-docsSection": "Airflow",
"additionalProperties": false,
"properties": {
[...]
"gitSync": {
"description": "Git sync settings.",
"type": "object",
"additionalProperties": false,
"properties": {
[...]
"securityContexts": {
"description": "Security context definition for the git sync sidecar. If not set, the values from global `securityContexts` will be used.",
"type": "object",
"x-docsSection": "Kubernetes",
"properties": {
"container": {
"description": "Container security context definition for the git sync sidecar.",
"type": "object",
"$ref": "#/definitions/io.k8s.api.core.v1.SecurityContext",
"default": {},
"x-docsSection": "Kubernetes",
"examples": [
{
"allowPrivilegeEscalation": false,
"capabilities": {
"drop": [
"ALL"
]
}
}
]
}
}
},
```
##########
chart/templates/_helpers.yaml:
##########
@@ -729,85 +729,125 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <node>.securityContext, defaults to global uid and gid.
- +------------------------+ +-----------------+ +-------------------------+
- | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
- +------------------------+ +-----------------+ +-------------------------+
+ +-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> | securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+ +-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
-Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContext,
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
any extra values set to securityContext or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContext" (list . .Values.webserver)
+ include "airflowPodSecurityContext" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
*/}}
-{{- define "airflowSecurityContext" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContext" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else if $.Values.securityContext }}
- {{- toYaml $.Values.securityContext }}
- {{- else }}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
+ {{- else if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ toYaml $.Values.securityContexts.pod | print }}
+ {{- else if $.Values.securityContext -}}
+{{ toYaml $.Values.securityContext | print }}
+ {{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end }}
{{- end }}
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node.
+Set the default value for pod securityContext
+If no value is passed for <node>.securityContexts.pod or <node>.securityContext, defaults to UID in the local node.
- +------------------------+ +-------------+
- | <node>.securityContext | > | <node>.uid |
- +------------------------+ +-------------+
+ +-----------------------------+ +------------------------+ +-------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> | <node>.uid |
+ +-----------------------------+ +------------------------+ +-------------+
The template can be called like so:
- include "localSecurityContext" .Values.statsd
+ include "localPodSecurityContext" (list . .Values.schedule)
It is important to pass the local variables scope to this template as it is used to determine the local node value for uid.
*/}}
-{{- define "localSecurityContext" -}}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else }}
- {{- printf "runAsUser: %v" .uid }}
- {{- end }}
-{{- end }}
+{{- define "localPodSecurityContext" -}}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
+ {{- else 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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +-----------------------------+ +----------------------------------------------------+ +------------------+ +-------------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "airflowPodSecurityContextsIds" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
*/}}
-{{- define "airflowSecurityContextIds" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContextsIds" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- pluck "runAsUser" .securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContext | first | default $.Values.gid }}
- {{- else if $.Values.securityContext }}
- {{- pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
- {{- else }}
- {{- printf "%s:%s" $.Values.uid $.Values.gid }}
- {{- end }}
- {{- end }}
-{{- end }}
+ {{- if .securityContexts.pod -}}
+{{ pluck "runAsUser" .securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ pluck "runAsUser" $.Values.securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else -}}
+{{ $.Values.uid }}:{{ $.Values.gid }}
+ {{- end -}}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for container securityContext
+If no value is passed for securityContexts.container or <node>.securityContexts.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+
+ +-----------------------------------+ +----------------------------+ +-----------------------------------------------------------+
+ | <node>.securityContexts.container | -> | securityContexts.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
Review Comment:
```suggestion
| <node>.securityContexts.container | -> | securityContexts.containers | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
```
Global container security contexts for containers is `securityContexts.containers` and not `securityContexts.container`
##########
chart/templates/_helpers.yaml:
##########
@@ -729,85 +729,125 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <node>.securityContext, defaults to global uid and gid.
- +------------------------+ +-----------------+ +-------------------------+
- | <node>.securityContext | -> | securityContext | -> | Values.uid + Values.gid |
- +------------------------+ +-----------------+ +-------------------------+
+ +-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> | securityContexts.pod | -> | securityContext | -> | Values.uid + Values.gid |
+ +-----------------------------+ +------------------------+ +----------------------+ +-----------------+ +-------------------------+
-Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContext,
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
any extra values set to securityContext or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContext" (list . .Values.webserver)
+ include "airflowPodSecurityContext" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
*/}}
-{{- define "airflowSecurityContext" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContext" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else if $.Values.securityContext }}
- {{- toYaml $.Values.securityContext }}
- {{- else }}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
+ {{- else if .securityContext -}}
+{{ toYaml .securityContext | print }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ toYaml $.Values.securityContexts.pod | print }}
+ {{- else if $.Values.securityContext -}}
+{{ toYaml $.Values.securityContext | print }}
+ {{- else -}}
runAsUser: {{ $.Values.uid }}
fsGroup: {{ $.Values.gid }}
{{- end }}
{{- end }}
{{- end }}
{{/*
-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to UID in the local node.
+Set the default value for pod securityContext
+If no value is passed for <node>.securityContexts.pod or <node>.securityContext, defaults to UID in the local node.
- +------------------------+ +-------------+
- | <node>.securityContext | > | <node>.uid |
- +------------------------+ +-------------+
+ +-----------------------------+ +------------------------+ +-------------+
+ | <node>.securityContexts.pod | -> | <node>.securityContext | -> | <node>.uid |
+ +-----------------------------+ +------------------------+ +-------------+
The template can be called like so:
- include "localSecurityContext" .Values.statsd
+ include "localPodSecurityContext" (list . .Values.schedule)
It is important to pass the local variables scope to this template as it is used to determine the local node value for uid.
*/}}
-{{- define "localSecurityContext" -}}
- {{- if .securityContext }}
- {{- toYaml .securityContext }}
- {{- else }}
- {{- printf "runAsUser: %v" .uid }}
- {{- end }}
-{{- end }}
+{{- define "localPodSecurityContext" -}}
+ {{- if .securityContexts.pod -}}
+{{ toYaml .securityContexts.pod | print }}
+ {{- else 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.
+If no value is passed for securityContexts.pod or <node>.securityContexts.pod or legacy securityContext and <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 |
- +------------------------+ +-----------------+ +-------------------------+
+ +-----------------------------+ +----------------------------------------------------+ +------------------+ +-------------------------+
+ | <node>.securityContexts.pod | -> | securityContexts.pod | <node>.securityContexts.pod | -> | securityContexts | -> | 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.
+Values are not accumulated meaning that if runAsUser is set to 10 in <node>.securityContexts.pod,
+any extra values set to securityContexts or uid+gid will be ignored.
The template can be called like so:
- include "airflowSecurityContextIds" (list . .Values.workers)
+ include "airflowPodSecurityContextsIds" (list . .Values.webserver)
Where `.` is the global variables scope and `.Values.workers` the local variables scope for the workers template.
*/}}
-{{- define "airflowSecurityContextIds" -}}
- {{- $ := index . 0 }}
+{{- define "airflowPodSecurityContextsIds" -}}
+ {{- $ := index . 0 -}}
{{- with index . 1 }}
- {{- if .securityContext }}
- {{- pluck "runAsUser" .securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContext | first | default $.Values.gid }}
- {{- else if $.Values.securityContext }}
- {{- pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
- {{- else }}
- {{- printf "%s:%s" $.Values.uid $.Values.gid }}
- {{- end }}
- {{- end }}
-{{- end }}
+ {{- if .securityContexts.pod -}}
+{{ pluck "runAsUser" .securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" .securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else if $.Values.securityContexts.pod -}}
+{{ pluck "runAsUser" $.Values.securityContexts.pod | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContexts.pod | first | default $.Values.gid }}
+ {{- else if $.Values.securityContext -}}
+{{ pluck "runAsUser" $.Values.securityContext | first | default $.Values.uid }}:{{ pluck "fsGroup" $.Values.securityContext | first | default $.Values.gid }}
+ {{- else -}}
+{{ $.Values.uid }}:{{ $.Values.gid }}
+ {{- end -}}
+ {{- end -}}
+{{- end -}}
+
+{{/*
+Set the default value for container securityContext
+If no value is passed for securityContexts.container or <node>.securityContexts.container, defaults to deny privileges escallation and dropping all POSIX capabilities.
+
+ +-----------------------------------+ +----------------------------+ +-----------------------------------------------------------+
+ | <node>.securityContexts.container | -> | securityContexts.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]|
+ +-----------------------------------+ +----------------------------+ +-----------------------------------------------------------+
+
+
+The template can be called like so:
+ include "containerSecurityContext" (list . .Values.statsd)
+
+Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "containerSecurityContext" -}}
+ {{- $ := index . 0 -}}
+ {{- with index . 1 }}
+ {{- if .securityContexts.container -}}
+{{ toYaml .securityContexts.container | print }}
+ {{- else if $.Values.securityContexts.container -}}
+{{ toYaml $.Values.securityContexts.container | print }}
Review Comment:
```suggestion
{{- else if $.Values.securityContexts.containers -}}
{{ toYaml $.Values.securityContexts.containers | print }}
```
Same thing here, `securityContexts.containers` and not `securityContexts.container`
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1518592252
Create a new PR and make your changes there again. It happens, and has the opportunity to re-review your own code while applying.
Closing it for now.
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1521860394
@ChrisFraun did you plan on re-creating a PR? I don't mind doing it if you did not plan on re-creating it.
--
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] mikaeld commented on pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511653657
@potiuk I've been trying to figure out some of the test failures. However, I'm not certain how to debug the failing tests. I'm using Breeze because I could not get those tests to work using a local venv. Maybe I'm just not used to these kind of tests and not looking at the right error output; how would you approach debugging something like the following:
```shell
root@7fa702d1f4c4:/opt/airflow# pytest tests/charts/test_airflow_common.py -n auto
============================================================================ test session starts =============================================================================
platform linux -- Python 3.7.16, pytest-7.3.1, pluggy-1.0.0 -- /usr/local/bin/python
cachedir: .pytest_cache
rootdir: /opt/airflow
configfile: pyproject.toml
plugins: timeouts-1.2.1, asyncio-0.21.0, capture-warnings-0.0.4, httpx-0.21.3, time-machine-2.9.0, instafail-0.5.0, rerunfailures-11.1.2, xdist-3.2.1, requests-mock-1.10.0, anyio-3.6.2, cov-4.0.0
asyncio: mode=strict
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s
[gw0] linux Python 3.7.16 cwd: /opt/airflow
[gw1] linux Python 3.7.16 cwd: /opt/airflow
[gw2] linux Python 3.7.16 cwd: /opt/airflow
[gw3] linux Python 3.7.16 cwd: /opt/airflow
[gw4] linux Python 3.7.16 cwd: /opt/airflow
[gw5] linux Python 3.7.16 cwd: /opt/airflow
[gw6] linux Python 3.7.16 cwd: /opt/airflow
[gw7] linux Python 3.7.16 cwd: /opt/airflow
[gw0] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw4] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw1] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw3] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw6] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw2] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw5] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
[gw7] Python 3.7.16 (default, Apr 12 2023, 07:12:00) -- [GCC 10.2.1 20210110]
gw0 [18] / gw1 [18] / gw2 [18] / gw3 [18] / gw4 [18] / gw5 [18] / gw6 [18] / gw7 [18]
scheduling tests via LoadScheduling
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow@user-digest-user-tag-user-digest]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values0-expected_mount0]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_webserver_config_configmap_name_volume_mounts
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow:user-tag-user-tag-None]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow@user-digest-None-user-digest]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values2-expected_mount2]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_disable_some_variables
tests/charts/test_airflow_common.py::TestAirflowCommon::test_global_affinity_tolerations_topology_spread_constraints_and_node_selector
[gw0] [ 5%] FAILED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values0-expected_mount0]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values1-expected_mount1]
[gw4] [ 11%] FAILED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values2-expected_mount2]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values3-expected_mount3]
[gw1] [ 16%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_webserver_config_configmap_name_volume_mounts
tests/charts/test_airflow_common.py::TestAirflowCommon::test_annotations
[gw2] [ 22%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow:user-tag-user-tag-None]
[gw5] [ 27%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow@user-digest-user-tag-user-digest]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow@user-digest-None-user-digest]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_set_correct_helm_hooks_weight
[gw7] [ 33%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_disable_some_variables
[gw6] [ 38%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow@user-digest-None-user-digest]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_have_all_variables
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow@user-digest-user-tag-user-digest]
[gw0] [ 44%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values1-expected_mount1]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_have_all_config_mounts_on_init_containers
[gw4] [ 50%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values3-expected_mount3]
tests/charts/test_airflow_common.py::TestAirflowCommon::test_priority_class_name
[gw3] [ 55%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_global_affinity_tolerations_topology_spread_constraints_and_node_selector
tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow:user-tag-user-tag-None]
[gw7] [ 61%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_have_all_variables
[gw6] [ 66%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow@user-digest-user-tag-user-digest]
[gw5] [ 72%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_set_correct_helm_hooks_weight
[gw2] [ 77%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_default_image[apache/airflow@user-digest-None-user-digest]
[gw0] [ 83%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_have_all_config_mounts_on_init_containers
[gw1] [ 88%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_annotations
[gw3] [ 94%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_should_use_correct_image[apache/airflow:user-tag-user-tag-None]
[gw4] [100%] PASSED tests/charts/test_airflow_common.py::TestAirflowCommon::test_priority_class_name
================================================================================== FAILURES ==================================================================================
_______________________________________________________ TestAirflowCommon.test_dags_mount[dag_values0-expected_mount0] _______________________________________________________
[gw0] linux -- Python 3.7.16 /usr/local/bin/python
self = <tests.charts.test_airflow_common.TestAirflowCommon object at 0x7ff0de8ec590>, dag_values = {'gitSync': {'enabled': True}}
expected_mount = {'mountPath': '/opt/airflow/dags', 'name': 'dags', 'readOnly': True}
@pytest.mark.parametrize(
"dag_values, expected_mount",
[
(
{"gitSync": {"enabled": True}},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": True,
},
),
(
{"persistence": {"enabled": True}},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": False,
},
),
(
{
"gitSync": {"enabled": True},
"persistence": {"enabled": True},
},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": True,
},
),
(
{"persistence": {"enabled": True, "subPath": "test/dags"}},
{
"subPath": "test/dags",
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": False,
},
),
],
)
def test_dags_mount(self, dag_values, expected_mount):
docs = render_chart(
values={
"dags": dag_values,
"airflowVersion": "1.10.15",
}, # airflowVersion is present so webserver gets the mount
show_only=[
"templates/scheduler/scheduler-deployment.yaml",
"templates/workers/worker-deployment.yaml",
> "templates/webserver/webserver-deployment.yaml",
],
)
tests/charts/test_airflow_common.py:84:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/mikaeld/code/mozilla/airflow/tests/charts/helm_template_generator.py:138: in render_chart
???
/usr/local/lib/python3.7/subprocess.py:411: in check_output
**kwargs).stdout
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
input = None, capture_output = False, timeout = None, check = True
popenargs = (['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpjppcs087', ...],)
kwargs = {'cwd': '/opt/airflow/chart', 'stderr': -1, 'stdout': -1}, process = <subprocess.Popen object at 0x7ff0dfc4ef90>, stdout = b''
stderr = b'W0417 15:54:23.300556 17785 loader.go:221] Config not found: /files/.kube/config\nW0417 15:54:23.310400 17785 lo..." at <.securityContexts.pod>: nil pointer evaluating interface {}.pod\n\nUse --debug flag to render out invalid YAML\n'
retcode = 1
def run(*popenargs,
input=None, capture_output=False, timeout=None, check=False, **kwargs):
"""Run command with arguments and return a CompletedProcess instance.
The returned instance will have attributes args, returncode, stdout and
stderr. By default, stdout and stderr are not captured, and those attributes
will be None. Pass stdout=PIPE and/or stderr=PIPE in order to capture them.
If check is True and the exit code was non-zero, it raises a
CalledProcessError. The CalledProcessError object will have the return code
in the returncode attribute, and output & stderr attributes if those streams
were captured.
If timeout is given, and the process takes too long, a TimeoutExpired
exception will be raised.
There is an optional argument "input", allowing you to
pass bytes or a string to the subprocess's stdin. If you use this argument
you may not also use the Popen constructor's "stdin" argument, as
it will be used internally.
By default, all communication is in bytes, and therefore any "input" should
be bytes, and the stdout and stderr will be bytes. If in text mode, any
"input" should be a string, and stdout and stderr will be strings decoded
according to locale encoding, or by "encoding" if set. Text mode is
triggered by setting any of text, encoding, errors or universal_newlines.
The other arguments are the same as for the Popen constructor.
"""
if input is not None:
if kwargs.get('stdin') is not None:
raise ValueError('stdin and input arguments may not both be used.')
kwargs['stdin'] = PIPE
if capture_output:
if kwargs.get('stdout') is not None or kwargs.get('stderr') is not None:
raise ValueError('stdout and stderr arguments may not be used '
'with capture_output.')
kwargs['stdout'] = PIPE
kwargs['stderr'] = PIPE
with Popen(*popenargs, **kwargs) as process:
try:
stdout, stderr = process.communicate(input, timeout=timeout)
except TimeoutExpired as exc:
process.kill()
if _mswindows:
# Windows accumulates the output in a single blocking
# read() call run on child threads, with the timeout
# being done in a join() on those threads. communicate()
# _after_ kill() is required to collect that and add it
# to the exception.
exc.stdout, exc.stderr = process.communicate()
else:
# POSIX _communicate already populated the output so
# far into the TimeoutExpired exception.
process.wait()
raise
except: # Including KeyboardInterrupt, communicate handled that.
process.kill()
# We don't call process.wait() as .__exit__ does that for us.
raise
retcode = process.poll()
if check and retcode:
raise CalledProcessError(retcode, process.args,
> output=stdout, stderr=stderr)
E subprocess.CalledProcessError: Command '['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpjppcs087', '--kube-version', '1.23.13', '--namespace', 'default', '--show-only', 'templates/scheduler/scheduler-deployment.yaml', '--show-only', 'templates/workers/worker-deployment.yaml', '--show-only', 'templates/webserver/webserver-deployment.yaml']' returned non-zero exit status 1.
/usr/local/lib/python3.7/subprocess.py:512: CalledProcessError
_______________________________________________________ TestAirflowCommon.test_dags_mount[dag_values2-expected_mount2] _______________________________________________________
[gw4] linux -- Python 3.7.16 /usr/local/bin/python
self = <tests.charts.test_airflow_common.TestAirflowCommon object at 0x7f8ec5f4ae90>, dag_values = {'gitSync': {'enabled': True}, 'persistence': {'enabled': True}}
expected_mount = {'mountPath': '/opt/airflow/dags', 'name': 'dags', 'readOnly': True}
@pytest.mark.parametrize(
"dag_values, expected_mount",
[
(
{"gitSync": {"enabled": True}},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": True,
},
),
(
{"persistence": {"enabled": True}},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": False,
},
),
(
{
"gitSync": {"enabled": True},
"persistence": {"enabled": True},
},
{
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": True,
},
),
(
{"persistence": {"enabled": True, "subPath": "test/dags"}},
{
"subPath": "test/dags",
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": False,
},
),
],
)
def test_dags_mount(self, dag_values, expected_mount):
docs = render_chart(
values={
"dags": dag_values,
"airflowVersion": "1.10.15",
}, # airflowVersion is present so webserver gets the mount
show_only=[
"templates/scheduler/scheduler-deployment.yaml",
"templates/workers/worker-deployment.yaml",
> "templates/webserver/webserver-deployment.yaml",
],
)
tests/charts/test_airflow_common.py:84:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/mikaeld/code/mozilla/airflow/tests/charts/helm_template_generator.py:138: in render_chart
???
/usr/local/lib/python3.7/subprocess.py:411: in check_output
**kwargs).stdout
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
input = None, capture_output = False, timeout = None, check = True
popenargs = (['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpfd7jte59', ...],)
kwargs = {'cwd': '/opt/airflow/chart', 'stderr': -1, 'stdout': -1}, process = <subprocess.Popen object at 0x7f8ec5be6910>, stdout = b''
stderr = b'W0417 15:54:23.307418 17790 loader.go:221] Config not found: /files/.kube/config\nW0417 15:54:23.316569 17790 lo..." at <.securityContexts.pod>: nil pointer evaluating interface {}.pod\n\nUse --debug flag to render out invalid YAML\n'
retcode = 1
def run(*popenargs,
input=None, capture_output=False, timeout=None, check=False, **kwargs):
"""Run command with arguments and return a CompletedProcess instance.
The returned instance will have attributes args, returncode, stdout and
stderr. By default, stdout and stderr are not captured, and those attributes
will be None. Pass stdout=PIPE and/or stderr=PIPE in order to capture them.
If check is True and the exit code was non-zero, it raises a
CalledProcessError. The CalledProcessError object will have the return code
in the returncode attribute, and output & stderr attributes if those streams
were captured.
If timeout is given, and the process takes too long, a TimeoutExpired
exception will be raised.
There is an optional argument "input", allowing you to
pass bytes or a string to the subprocess's stdin. If you use this argument
you may not also use the Popen constructor's "stdin" argument, as
it will be used internally.
By default, all communication is in bytes, and therefore any "input" should
be bytes, and the stdout and stderr will be bytes. If in text mode, any
"input" should be a string, and stdout and stderr will be strings decoded
according to locale encoding, or by "encoding" if set. Text mode is
triggered by setting any of text, encoding, errors or universal_newlines.
The other arguments are the same as for the Popen constructor.
"""
if input is not None:
if kwargs.get('stdin') is not None:
raise ValueError('stdin and input arguments may not both be used.')
kwargs['stdin'] = PIPE
if capture_output:
if kwargs.get('stdout') is not None or kwargs.get('stderr') is not None:
raise ValueError('stdout and stderr arguments may not be used '
'with capture_output.')
kwargs['stdout'] = PIPE
kwargs['stderr'] = PIPE
with Popen(*popenargs, **kwargs) as process:
try:
stdout, stderr = process.communicate(input, timeout=timeout)
except TimeoutExpired as exc:
process.kill()
if _mswindows:
# Windows accumulates the output in a single blocking
# read() call run on child threads, with the timeout
# being done in a join() on those threads. communicate()
# _after_ kill() is required to collect that and add it
# to the exception.
exc.stdout, exc.stderr = process.communicate()
else:
# POSIX _communicate already populated the output so
# far into the TimeoutExpired exception.
process.wait()
raise
except: # Including KeyboardInterrupt, communicate handled that.
process.kill()
# We don't call process.wait() as .__exit__ does that for us.
raise
retcode = process.poll()
if check and retcode:
raise CalledProcessError(retcode, process.args,
> output=stdout, stderr=stderr)
E subprocess.CalledProcessError: Command '['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpfd7jte59', '--kube-version', '1.23.13', '--namespace', 'default', '--show-only', 'templates/scheduler/scheduler-deployment.yaml', '--show-only', 'templates/workers/worker-deployment.yaml', '--show-only', 'templates/webserver/webserver-deployment.yaml']' returned non-zero exit status 1.
/usr/local/lib/python3.7/subprocess.py:512: CalledProcessError
========================================================================== short test summary info ===========================================================================
FAILED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values0-expected_mount0] - subprocess.CalledProcessError: Command '['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpjppcs087', '--kube-version', '1.23.13', '--namespace', 'default', '--show-only', 'templates/scheduler/scheduler-deployment.yaml', '--show-only', 'templates/workers/worker-deployment.yaml', '--show-only', 'templates/webserver/webserver-deployment.yaml']' returned non-zero exit status 1.
FAILED tests/charts/test_airflow_common.py::TestAirflowCommon::test_dags_mount[dag_values2-expected_mount2] - subprocess.CalledProcessError: Command '['helm', 'template', 'release-name', '/opt/airflow/chart', '--values', '/tmp/tmpfd7jte59', '--kube-version', '1.23.13', '--namespace', 'default', '--show-only', 'templates/scheduler/scheduler-deployment.yaml', '--show-only', 'templates/workers/worker-deployment.yaml', '--show-only', 'templates/webserver/webserver-deployment.yaml']' returned non-zero exit status 1.
======================================================================= 2 failed, 16 passed in 30.49s ========================================================================
/usr/local/lib/python3.7/site-packages/flask_appbuilder/models/sqla/__init__.py:105 MovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings. Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
root@7fa702d1f4c4:/opt/airflow#
```
--
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 #24588: add: container securityContext not available in podSecurityContext
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #24588:
URL: https://github.com/apache/airflow/pull/24588#issuecomment-1511840516
This is a problem of your environment. Probably you have an empty "airflow" package (`airflow` directory with __init__.py) somewhere on your PYTHONPATH that overrides the installed airflow package. Fix your environment.
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1154599386
##########
chart/templates/triggerer/triggerer-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: triggerer
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext | nindent 12 }}
Review Comment:
`triggerer-log-groomer` is missing `securityContexts` implementation
--
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] ChrisFraun commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "ChrisFraun (via GitHub)" <gi...@apache.org>.
ChrisFraun commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1156795574
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -171,6 +175,7 @@ spec:
{{- end }}
resources:
{{ toYaml .Values.webserver.resources | indent 12 }}
+ securityContext: {{ or .Values.webserver.securityContexts.container .Values.securityContexts.container }}
Review Comment:
fused the duplicate keys into one https://github.com/apache/airflow/pull/24588/files#diff-42f2a460a872212b923b5d7591c552a22816d768478a70d6943103221e08f11fR169
--
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] mikaeld commented on a diff in pull request #24588: add: container securityContext not available in podSecurityContext
Posted by "mikaeld (via GitHub)" <gi...@apache.org>.
mikaeld commented on code in PR #24588:
URL: https://github.com/apache/airflow/pull/24588#discussion_r1157685121
##########
chart/templates/webserver/webserver-deployment.yaml:
##########
@@ -160,6 +163,7 @@ spec:
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContext or .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
Review Comment:
```suggestion
securityContext: {{ or $containerSecurityContext .Values.webserver.securityContexts.container .Values.securityContexts.container | nindent 12 }}
```
Getting the following error in my test setup without the proposed change:
```
Error: template: test-airflow/charts/airflow/templates/webserver/webserver-deployment.yaml:166:30: executing "test-airflow/charts/airflow/templates/webserver/webserver-deployment.yaml" at <$containerSecurityContext>: can't give argument to non-function $containerSecurityContext
```
`helm version` output:
```
version.BuildInfo{Version:"v3.11.0", GitCommit:"472c5736ab01133de504a826bd9ee12cbe4e7904", GitTreeState:"clean", GoVersion:"go1.18.10"}
```
This is probably due to what is explained [here](https://github.com/helm/helm/issues/7711#issuecomment-593113347)
--
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