Miretpl commented on code in PR #60677:
URL: https://github.com/apache/airflow/pull/60677#discussion_r2838336511
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -24,6 +24,8 @@
{{- $securityContext := include "airflowPodSecurityContext" (list
.Values.workers.kubernetes .Values.workers .Values) }}
{{- $containerSecurityContextKerberosSidecar := include
"containerSecurityContext" (list .Values.workers.kerberosSidecar .Values) }}
{{- $containerLifecycleHooksKerberosSidecar := or
.Values.workers.kerberosSidecar.containerLifecycleHooks
.Values.containerLifecycleHooks }}
+{{- $containerSecurityContextKerberosInitContainer :=
.Values.workers.kubernetes.kerberosInitContainer.securityContexts.container |
default .Values.workers.kerberosInitContainer.securityContexts.container }}
+{{- $containerLifecycleHooksKerberosInitContainer :=
.Values.workers.kubernetes.kerberosInitContainer.containerLifecycleHooks |
default .Values.workers.kerberosInitContainer.containerLifecycleHooks }}
Review Comment:
```suggestion
{{- $containerLifecycleHooksKerberosInitContainer := or
.Values.workers.kubernetes.kerberosInitContainer.containerLifecycleHooks
.Values.workers.kerberosInitContainer.containerLifecycleHooks
.Values.containerLifecycleHooks }}
```
for fallback logic consistency with the rest of the chart + adding
`.Values.containerLifecycleHooks` for same fallback as for `worker-kerberos`
container
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -1050,6 +1050,120 @@ def test_kerberos_init_container_resources(self,
workers_values):
},
}
+ @pytest.mark.parametrize(
+ ("workers_values", "expected"),
+ [
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ }
+ },
+ {"runAsUser": 1000},
+ ),
+ (
+ {
+ "celery": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ }
+ },
+ {"runAsUser": 2000},
+ ),
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ },
+ "celery": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ },
+ },
+ {"runAsUser": 2000},
+ ),
+ ],
+ )
+ def test_kerberos_init_container_security_context(self, workers_values,
expected):
Review Comment:
It is not intuitive, but all securityContext-related test cases are in
`security/test_security_context.py` file (despite pod-template tests). Could we
move this test case there?
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -24,6 +24,8 @@
{{- $securityContext := include "airflowPodSecurityContext" (list
.Values.workers.kubernetes .Values.workers .Values) }}
{{- $containerSecurityContextKerberosSidecar := include
"containerSecurityContext" (list .Values.workers.kerberosSidecar .Values) }}
{{- $containerLifecycleHooksKerberosSidecar := or
.Values.workers.kerberosSidecar.containerLifecycleHooks
.Values.containerLifecycleHooks }}
+{{- $containerSecurityContextKerberosInitContainer :=
.Values.workers.kubernetes.kerberosInitContainer.securityContexts.container |
default .Values.workers.kerberosInitContainer.securityContexts.container }}
Review Comment:
```suggestion
{{- $containerSecurityContextKerberosInitContainer := include
"containerSecurityContext" (list
.Values.workers.kubernetes.kerberosInitContainer
.Values.workers.kerberosInitContainer .Values) }}
```
In general, we use functions for securityContext evaluation, so change for
keeping chart consistency + `.Values` as the end for having same fallback as
for `worker-kerberos` container, which is the same as this init container
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -48,6 +48,8 @@
{{- $containerSecurityContextWaitForMigrations := include
"containerSecurityContext" (list .Values.workers.waitForMigrations .Values) }}
{{- $containerSecurityContextLogGroomerSidecar := include
"containerSecurityContext" (list .Values.workers.logGroomerSidecar .Values) }}
{{- $containerSecurityContextKerberosSidecar := include
"containerSecurityContext" (list .Values.workers.kerberosSidecar .Values) }}
+{{- $containerSecurityContextKerberosInitContainer := include
"containerSecurityContext" (list .Values.workers.kerberosInitContainer .Values)
}}
+{{- $containerLifecycleHooksKerberosInitContainer :=
.Values.workers.kerberosInitContainer.containerLifecycleHooks }}
Review Comment:
```suggestion
{{- $containerLifecycleHooksKerberosInitContainer :=
.Values.workers.kerberosInitContainer.containerLifecycleHooks
.Values.containerLifecycleHooks }}
```
for the same fallback logic as it for `worker-kerberos` container
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1384,3 +1384,115 @@ def test_kerberos_init_container_resources(self,
workers_values):
"memory": "4Mi",
},
}
+
+ @pytest.mark.parametrize(
+ ("workers_values", "expected"),
+ [
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ }
+ },
+ {"runAsUser": 1000},
+ ),
+ (
+ {
+ "kubernetes": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ }
+ },
+ {"runAsUser": 2000},
+ ),
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ },
+ "kubernetes": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ },
+ },
+ {"runAsUser": 2000},
+ ),
+ ],
+ )
+ def test_kerberos_init_container_security_context(self, workers_values,
expected):
Review Comment:
We could set `expected` to always be the same value, which would simplify
our test case
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1384,3 +1384,115 @@ def test_kerberos_init_container_resources(self,
workers_values):
"memory": "4Mi",
},
}
+
+ @pytest.mark.parametrize(
+ ("workers_values", "expected"),
+ [
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ }
+ },
+ {"runAsUser": 1000},
+ ),
+ (
+ {
+ "kubernetes": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ }
+ },
+ {"runAsUser": 2000},
+ ),
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser": 1000}},
+ },
+ "kubernetes": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "securityContexts": {"container": {"runAsUser":
2000}},
+ }
+ },
+ },
+ {"runAsUser": 2000},
+ ),
+ ],
+ )
+ def test_kerberos_init_container_security_context(self, workers_values,
expected):
+ docs = render_chart(
+ values={
+ "workers": workers_values,
+ },
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ assert (
+ jmespath.search("spec.initContainers[?name=='kerberos-init'] |
[0].securityContext", docs[0])
+ == expected
+ )
+
+ @pytest.mark.parametrize(
+ ("workers_values", "expected"),
+ [
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "containerLifecycleHooks": {"postStart": {"exec":
{"command": ["echo", "base"]}}},
+ }
+ },
+ {"postStart": {"exec": {"command": ["echo", "base"]}}},
+ ),
+ (
+ {
+ "kubernetes": {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "containerLifecycleHooks": {
+ "postStart": {"exec": {"command": ["echo",
"kubernetes"]}}
+ },
+ }
+ }
+ },
+ {"postStart": {"exec": {"command": ["echo", "kubernetes"]}}},
+ ),
+ (
+ {
+ "kerberosInitContainer": {
+ "enabled": True,
+ "containerLifecycleHooks": {"postStart": {"exec":
{"command": ["echo", "base"]}}},
Review Comment:
```suggestion
"containerLifecycleHooks": {"preStop": {"exec":
{"command": ["echo", "base"]}}},
```
`preStop` because we want to be sure that the overwrite and fallback logic
works fully correct (there will be no `preStop` and `postStart` defined at the
same time looking at the new test conf)
--
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]
