Miretpl commented on code in PR #72206:
URL: https://github.com/apache/airflow/pull/72206#discussion_r4098384942
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -341,6 +348,9 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- if $keda }}
+ {{- include "metadata_db_environment" . | indent 10 }}
+ {{- end }}
+ {{- if $keda }}
Review Comment:
```suggestion
{{- if $keda }}
{{- include "metadata_db_environment" . | indent 10 }}
```
We don't need to duplicate the same `if` statement.
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -254,7 +257,11 @@ spec:
envFrom: {{- include "custom_airflow_environment_from" . | default
"\n []" | indent 10 }}
env:
{{- include "custom_airflow_environment" . | indent 10 }}
+ {{- /* This init container queries the migration state, so it
needs the database
+ regardless of KEDA. It exits before the worker starts and
does not share
+ its environment with the container that runs task code. */}}
Review Comment:
```suggestion
{{- /* This init container queries the migration state, so it
needs the database regardless of KEDA. */}}
```
The deleted part is just how an init container works. Should be obvious to
anyone using Kubernetes.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
Review Comment:
```suggestion
def test_metadata_db_env_absent_in_workers_by_default(self):
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
+
+ KEDA is the exception: its ScaledObject reads the connection from an
env var on
+ this pod spec, so the variable has to stay when KEDA is doing the
scaling.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in jmespath.search("spec.template.spec.containers[*]",
docs[0]):
+ names = set(jmespath.search("env[*].name", container) or [])
+ assert not (names & METADATA_DB_VARS), f"metadata DB env in
{container['name']}"
Review Comment:
Could you simplify it along with the other changes? E.g. one thing from the
couple: you can take the env names from the first JMESPath search.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
+
+ KEDA is the exception: its ScaledObject reads the connection from an
env var on
+ this pod spec, so the variable has to stay when KEDA is doing the
scaling.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in jmespath.search("spec.template.spec.containers[*]",
docs[0]):
+ names = set(jmespath.search("env[*].name", container) or [])
+ assert not (names & METADATA_DB_VARS), f"metadata DB env in
{container['name']}"
+
+ def test_worker_migration_init_container_keeps_metadata_db(self):
+ """The worker's migration-wait init container queries the DB, so it
still needs it.
Review Comment:
```suggestion
"""The wait-for-migration init container queries the DB.
```
And I think that the rest of the comment is not needed. It is just how init
containers works.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
+
+ KEDA is the exception: its ScaledObject reads the connection from an
env var on
+ this pod spec, so the variable has to stay when KEDA is doing the
scaling.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in jmespath.search("spec.template.spec.containers[*]",
docs[0]):
+ names = set(jmespath.search("env[*].name", container) or [])
+ assert not (names & METADATA_DB_VARS), f"metadata DB env in
{container['name']}"
+
+ def test_worker_migration_init_container_keeps_metadata_db(self):
+ """The worker's migration-wait init container queries the DB, so it
still needs it.
+
+ It runs to completion before the worker starts and shares no
environment with the
+ container that executes task code, so keeping the credentials here
costs nothing.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ names = jmespath.search(
+
"spec.template.spec.initContainers[?name=='wait-for-airflow-migrations'].env[].name",
+ docs[0],
+ )
+ assert "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN" in names
Review Comment:
```suggestion
assert "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN" in jmespath.search(
"spec.template.spec.initContainers[?name=='wait-for-airflow-migrations'].env[].name",
docs[0],
)
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
+
+ KEDA is the exception: its ScaledObject reads the connection from an
env var on
+ this pod spec, so the variable has to stay when KEDA is doing the
scaling.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in jmespath.search("spec.template.spec.containers[*]",
docs[0]):
+ names = set(jmespath.search("env[*].name", container) or [])
+ assert not (names & METADATA_DB_VARS), f"metadata DB env in
{container['name']}"
+
+ def test_worker_migration_init_container_keeps_metadata_db(self):
+ """The worker's migration-wait init container queries the DB, so it
still needs it.
+
+ It runs to completion before the worker starts and shares no
environment with the
+ container that executes task code, so keeping the credentials here
costs nothing.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ names = jmespath.search(
+
"spec.template.spec.initContainers[?name=='wait-for-airflow-migrations'].env[].name",
+ docs[0],
+ )
+ assert "AIRFLOW__DATABASE__SQL_ALCHEMY_CONN" in names
+
+ def test_metadata_db_env_kept_where_the_component_uses_it(self):
Review Comment:
```suggestion
def test_metadata_db_env_in_all_required_components(self):
```
##########
chart/templates/_helpers.yaml:
##########
@@ -152,6 +138,26 @@ If release name contains chart name it will be used as a
full name.
{{- end }}
{{- end }}
+{{/* Metadata database credentials, only needed by components that talk to the
+ metadata DB. Components that merely execute tasks do not: the task SDK
reaches
+ the Execution API instead, so they have no reason to hold these. */}}
Review Comment:
```suggestion
{{/* Metadata database credentials, only needed by components that talk to
the metadata DB. */}}
```
The removed comment is true for current released versions of Airflow, but it
might not be for the future, with plans for more stuff moving away from direct
DB access. I would prefer to avoid potential confusion in the comment and just
mention that it is required by components that require direct access to the
Airflow Metadata Database.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -348,10 +350,14 @@ def test_should_disable_some_variables(self):
expected_vars = [
"AIRFLOW_HOME",
"AIRFLOW__CORE__FERNET_KEY",
- "AIRFLOW_CONN_AIRFLOW_DB",
"AIRFLOW__CELERY__BROKER_URL",
+ "AIRFLOW_CONN_AIRFLOW_DB",
+ ]
+ # Workers do not receive the metadata DB connection: they execute
tasks and reach
+ # the Execution API, so they have no reason to hold the database
credentials.
Review Comment:
```suggestion
# Workers do not receive the metadata DB connection; they execute
tasks via the API Server.
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
+
+ KEDA is the exception: its ScaledObject reads the connection from an
env var on
+ this pod spec, so the variable has to stay when KEDA is doing the
scaling.
+ """
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in jmespath.search("spec.template.spec.containers[*]",
docs[0]):
+ names = set(jmespath.search("env[*].name", container) or [])
+ assert not (names & METADATA_DB_VARS), f"metadata DB env in
{container['name']}"
+
+ def test_worker_migration_init_container_keeps_metadata_db(self):
Review Comment:
```suggestion
def test_worker_migration_init_container_has_metadata_db_env(self):
```
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,45 @@ def test_jwt_secret_can_be_disabled(self):
)
assert "AIRFLOW__API_AUTH__JWT_SECRET" not in env_names, f"Wrong
vars in {component}"
+ def test_metadata_db_env_absent_from_workers_by_default(self):
+ """Celery workers execute Dag-author code and must not hold DB
credentials.
Review Comment:
```suggestion
"""Celery workers accessing the DB via the API Server.
```
--
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]
