jedcunningham commented on a change in pull request #18249:
URL: https://github.com/apache/airflow/pull/18249#discussion_r720453456
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
Review comment:
```suggestion
This function is required for backwards compatibility
```
##########
File path: chart/templates/cleanup/cleanup-cronjob.yaml
##########
@@ -22,6 +22,8 @@
{{- $nodeSelector := or .Values.cleanup.nodeSelector .Values.nodeSelector }}
{{- $affinity := or .Values.cleanup.affinity .Values.affinity }}
{{- $tolerations := or .Values.cleanup.tolerations .Values.tolerations }}
+{{- $securityContext := or .Values.cleanup.securityContext (include
"defaultSecurityContext" . | mustFromJson ) }}
+{{- $containerSecurityContext := or .Values.cleanup.containerSecurityContext
(include "defaultContainerSecurityContext" . | mustFromJson ) }}
Review comment:
```suggestion
{{- $securityContext := or .Values.cleanup.securityContext (include
"defaultSecurityContext" . | mustFromJson) }}
{{- $containerSecurityContext := or .Values.cleanup.containerSecurityContext
(include "defaultContainerSecurityContext" . | mustFromJson) }}
```
nit
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+For gitSync and statsD, we use their respectice uid properties as fallback
+*/}}
+{{- define "gitSyncContainerSecurityContext" -}}
+{{- if .Values.dags.gitSync.containerSecurityContext -}}
+ {{ .Values.dags.gitSync.containerSecurityContext | toYaml }}
+{{- else if .Values.podSecurity.containerSecurityContext -}}
+ {{ .Values.podSecurity.containerSecurityContext | toYaml }}
+{{- else -}}
+runAsUser: {{ .Values.dags.gitSync.uid }}
+{{- end -}}
+{{- end -}}
+
+{{- define "statsdSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.statsd.uid }}
+ {{- $result | toJson }}
Review comment:
Can this be consistent with how `gitsync` is handled (I think the
non-dict route is more readable)?. That should also work for setting more than
1, no?
##########
File path: chart/templates/_helpers.yaml
##########
@@ -610,3 +610,62 @@ Create the name of the cleanup service account to use
{{- end -}}
{{- $kubeVersion -}}
{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultSecurityContext" -}}
+{{- if .Values.podSecurity.securityContext -}}
+ {{ .Values.podSecurity.securityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "fsGroup" .Values.gid }}
+ {{- $result | toJson }}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Set the default podsecurity.securityContext
+If no value is passed, defaults to .Values.uid and .Values.gid
+This function is required fr backwards compatibility
+*/}}
+{{- define "defaultContainerSecurityContext" -}}
+{{- if .Values.podSecurity.containerSecurityContext -}}
+{{ .Values.podSecurity.containerSecurityContext | toJson }}
+{{- else -}}
+ {{- $result := dict "runAsUser" .Values.uid "runAsGroup" .Values.gid }}
Review comment:
I don't think we should set these at the container level. We should only
set it at the pod level, otherwise choices for
`podSecurity.securityContext.runAsUser` or
`scheduler.podSecurity.securityContext.runAsUser` will be just be overridden by
this default.
Similarly, if we want to set runAsGroup by default, we should set it only in
the `defaultSecurityContext` I think.
That probably means we don't need this helper template and the logic for
defaulting the container securityContext can be simpler?
Am I overlooking something?
##########
File path: chart/tests/test_security_context.py
##########
@@ -0,0 +1,234 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import jmespath
+import pytest
+
+from tests.helm_template_generator import render_chart
+
+class TestSCBackwardsCompatibility:
+ def test_check_deployments_and_jobs(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 ==
jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 30 ==
jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+ assert 3000 ==
jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser",
docs[index])
+ assert 30 ==
jmespath.search("spec.template.spec.containers[0].securityContext.runAsGroup",
docs[index])
+
+ def test_check_statsd_uid(self):
+ docs = render_chart(
+ values={
+ "statsd": {"enabled": True, "uid": 3000}
+ },
+ show_only=["templates/statsd/statsd-deployment.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 ==
jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 3000 ==
jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser",
docs[index])
+
+ def test_check_cleanup_job(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "cleanup": {"enabled": True}
+ },
+ show_only=["templates/cleanup/cleanup-cronjob.yaml"],
+ )
+
+ for index in range(len(docs)):
+ assert 3000 ==
jmespath.search("spec.jobTemplate.spec.template.spec.securityContext.runAsUser",
docs[index])
+ assert 30 ==
jmespath.search("spec.jobTemplate.spec.template.spec.securityContext.fsGroup",
docs[index])
+ assert 3000 ==
jmespath.search("spec.jobTemplate.spec.template.spec.containers[0].securityContext.runAsUser",
docs[index])
+ assert 30 ==
jmespath.search("spec.jobTemplate.spec.template.spec.containers[0].securityContext.runAsGroup",
docs[index])
+
+ def test_gitsync_sidecar_and_init_container(self):
+ docs = render_chart(
+ values={
+ "dags": {"gitSync": {"enabled": True, "uid": 3000}},
+ "airflowVersion": "1.10.15",
+ },
+ show_only=[
+ "templates/workers/worker-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ assert "git-sync" in [c["name"] for c in
jmespath.search("spec.template.spec.containers", docs[index])]
+ assert "git-sync-init" in [c["name"] for c in
jmespath.search("spec.template.spec.initContainers", docs[index])]
+ assert 3000 ==
jmespath.search("spec.template.spec.initContainers[?name=='git-sync-init'].securityContext.runAsUser
| [0]", docs[index])
+ assert 3000 ==
jmespath.search("spec.template.spec.containers[?name=='git-sync'].securityContext.runAsUser
| [0]", docs[index])
+
+class TestSecurityContext:
+ # Test podSecurity setting for Pods and Containers
+ def test_check_default_setting(self):
+ docs = render_chart(
+ values={
+ "podSecurity": {
+ "securityContext": {"runAsUser": 6000, "fsGroup": 60},
+ "containerSecurityContext": {"runAsUser": 4000,
"runAsGroup": 40}
+ },
+ "webserver": {"defaultUser": {"enabled": True}},
+ "flower": {"enabled": True},
+ "statsd": {"enabled": True},
+ "airflowVersion": "2.2.0",
+ "executor": "CeleryKubernetesExecutor",
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ "templates/statsd/statsd-deployment.yaml"
+ ],
+ )
+
+ for index in range(len(docs)):
+ print (docs[index])
+ assert 6000 ==
jmespath.search("spec.template.spec.securityContext.runAsUser", docs[index])
+ assert 60 ==
jmespath.search("spec.template.spec.securityContext.fsGroup", docs[index])
+ assert 4000 ==
jmespath.search("spec.template.spec.containers[0].securityContext.runAsUser",
docs[index])
+ assert 40 ==
jmespath.search("spec.template.spec.containers[0].securityContext.runAsGroup",
docs[index])
+
+ # Test priority:
+ # <local>.containerSecurityContext > podSecurity.containerSecurityContext
> uid + gid
+ # <local>.containerSecurityContext > podSecurity.containerSecurityContext
> uid + gid
+ def test_check_local_setting(self):
+ docs = render_chart(
+ values={
+ "uid": 3000,
+ "gid": 30,
+ "podSecurity": {
+ "securityContext": {"runAsUser": 6000, "fsGroup": 60},
+ "containerSecurityContext": {"runAsUser": 4000,
"runAsGroup": 40}
+ },
+ "webserver": {
+ "defaultUser": {"enabled": True},
+ "securityContext": {"runAsUser": 9000, "fsGroup": 90},
+ "containerSecurityContext": {"runAsUser": 8000,
"runAsGroup": 80}
Review comment:
So we don't have to define these for all of them, maybe pull these out
and do something like this instead:
```
# define this above
component_contexts = {
"securityContext": {"runAsUser": 9000, "fsGroup": 90},
"containerSecurityContext": {"runAsUser": 8000, "runAsGroup": 80}
}
```
Then change this section (and the rest to):
```suggestion
**component_contexts
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
