Miretpl commented on code in PR #72206:
URL: https://github.com/apache/airflow/pull/72206#discussion_r3916546075
##########
chart/templates/_helpers.yaml:
##########
@@ -70,14 +75,14 @@ If release name contains chart name it will be used as a
full name.
name: {{ template "fernet_key_secret" . }}
key: fernet-key
{{- end }}
- {{- if
.Values.enableBuiltInSecretEnvVars.AIRFLOW__DATABASE__SQL_ALCHEMY_CONN }}
+ {{- if and .IncludeMetadataDb
.Values.enableBuiltInSecretEnvVars.AIRFLOW__DATABASE__SQL_ALCHEMY_CONN }}
Review Comment:
I would extract the db-related variables to the dedicated helper function
similar to `jwt_environment_environments` instead of increasing complexity in
standard one.
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -351,7 +351,11 @@ def test_should_disable_some_variables(self):
"AIRFLOW_CONN_AIRFLOW_DB",
"AIRFLOW__CELERY__BROKER_URL",
]
- expected_vars_in_worker = ["DUMB_INIT_SETSID"] + expected_vars
+ # 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.
+ expected_vars_in_worker = ["DUMB_INIT_SETSID"] + [
+ var for var in expected_vars if var != "AIRFLOW_CONN_AIRFLOW_DB"
Review Comment:
There is missing condition for ` if
`AIRFLOW__DATABASE__SQL_ALCHEMY_CONN` variant
##########
chart/tests/helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -455,6 +463,46 @@ 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.
+ """
+ metadata_db_vars = {"AIRFLOW__DATABASE__SQL_ALCHEMY_CONN",
"AIRFLOW_CONN_AIRFLOW_DB"}
+ docs =
render_chart(show_only=["templates/workers/worker-deployment.yaml"])
+ for container in docs[0]["spec"]["template"]["spec"]["containers"]:
Review Comment:
Could we use jmspath for having consistency across test cases?
--
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]
