You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jedcunningham (via GitHub)" <gi...@apache.org> on 2023/06/22 01:03:45 UTC
[GitHub] [airflow] jedcunningham commented on a diff in pull request #31865: Chart: Do not propagate global security context to statsd and redis
jedcunningham commented on code in PR #31865:
URL: https://github.com/apache/airflow/pull/31865#discussion_r1237863627
##########
tests/charts/security/test_security_context.py:
##########
@@ -210,6 +210,36 @@ def test_gitsync_sidecar_and_init_container(self):
docs[index],
)
+ # Test securityContexts for main containers
+ def test_global_security_context(self):
+ ctx_value_pod = {"runAsUser": 7000}
+ ctx_value_container = {"allowPrivilegeEscalation": False}
+ docs = render_chart(
+ values={"securityContexts": {"containers": ctx_value_container, "pod": ctx_value_pod}},
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-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/triggerer/triggerer-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/redis/redis-statefulset.yaml",
+ ],
+ )
+
+ for index in range(len(docs) - 2):
Review Comment:
Hmm, is the order of the docs deterministic? Might be worth a comment why we treat them differently.
##########
chart/templates/statsd/statsd-deployment.yaml:
##########
@@ -89,7 +88,7 @@ spec:
- name: statsd
image: {{ template "statsd_image" . }}
imagePullPolicy: {{ .Values.images.statsd.pullPolicy }}
- securityContext: {{ $containerSecurityContext | nindent 12 }}
+ securityContext: {{- toYaml .Values.statsd.securityContexts.container | nindent 12 }}
Review Comment:
This breaks backcompat on `stastd.securityContext`.
##########
chart/templates/redis/redis-statefulset.yaml:
##########
@@ -80,7 +79,7 @@ spec:
- name: redis
image: {{ template "redis_image" . }}
imagePullPolicy: {{ .Values.images.redis.pullPolicy }}
- securityContext: {{ $containerSecurityContext | nindent 12 }}
+ securityContext: {{- toYaml .Values.redis.securityContexts.container | nindent 12 }}
Review Comment:
This breaks backcompat on `redis.securityContext`.
##########
chart/values.yaml:
##########
@@ -1629,7 +1629,6 @@ statsd:
uid: 65534
# When not set, `statsd.uid` will be used
- # When not set, the values defined in the global securityContext will be used
Review Comment:
Also need to remove this comment from values.schema.json.
##########
tests/charts/security/test_security_context.py:
##########
@@ -210,6 +210,36 @@ def test_gitsync_sidecar_and_init_container(self):
docs[index],
)
+ # Test securityContexts for main containers
+ def test_global_security_context(self):
+ ctx_value_pod = {"runAsUser": 7000}
+ ctx_value_container = {"allowPrivilegeEscalation": False}
+ docs = render_chart(
+ values={"securityContexts": {"containers": ctx_value_container, "pod": ctx_value_pod}},
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-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/triggerer/triggerer-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/redis/redis-statefulset.yaml",
+ ],
+ )
+
+ for index in range(len(docs) - 2):
+ assert ctx_value_container == jmespath.search(
+ "spec.template.spec.containers[0].securityContext", docs[index]
+ )
+ assert ctx_value_pod == jmespath.search("spec.template.spec.securityContext", docs[index])
+ for index in range(len(docs) - 2, len(docs)):
+ assert ctx_value_container != jmespath.search(
+ "spec.template.spec.containers[0].securityContext", docs[index]
+ )
+ assert ctx_value_pod != jmespath.search("spec.template.spec.securityContext", docs[index])
Review Comment:
Or, maybe provide values for statds and redis and test those are applied properly too?
##########
tests/charts/security/test_security_context.py:
##########
@@ -210,6 +210,36 @@ def test_gitsync_sidecar_and_init_container(self):
docs[index],
)
+ # Test securityContexts for main containers
+ def test_global_security_context(self):
+ ctx_value_pod = {"runAsUser": 7000}
+ ctx_value_container = {"allowPrivilegeEscalation": False}
+ docs = render_chart(
+ values={"securityContexts": {"containers": ctx_value_container, "pod": ctx_value_pod}},
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-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/triggerer/triggerer-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/redis/redis-statefulset.yaml",
+ ],
+ )
+
+ for index in range(len(docs) - 2):
+ assert ctx_value_container == jmespath.search(
+ "spec.template.spec.containers[0].securityContext", docs[index]
+ )
+ assert ctx_value_pod == jmespath.search("spec.template.spec.securityContext", docs[index])
+ for index in range(len(docs) - 2, len(docs)):
+ assert ctx_value_container != jmespath.search(
+ "spec.template.spec.containers[0].securityContext", docs[index]
+ )
+ assert ctx_value_pod != jmespath.search("spec.template.spec.securityContext", docs[index])
Review Comment:
```suggestion
assert {} == jmespath.search(
"spec.template.spec.containers[0].securityContext", docs[index]
)
assert {} == jmespath.search("spec.template.spec.securityContext", docs[index])
```
Probably better to check what it is, vs isn't?
--
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