Remove JWT secrets from triggerer, worker and dag-processor#63204
Remove JWT secrets from triggerer, worker and dag-processor#63204deepujain wants to merge 3 commits intoapache:mainfrom
Conversation
jscheffl
left a comment
There was a problem hiding this comment.
Thanks for this fix... although I am not really sure if this is needed.
Triggerer, workers are just "clients" in repect to JWT tokens. They consume workload and execute. These components are not issuing new JWT. So if the JWT changes I assume these do not need a re-deployment.
The other way around I'd rather say, these two components should not have to know and should not have access to JWT at all to harden security.
For Dag Processor I am 80% sure only, this is running an internal API Server. In future this should get to be an API client as well. I assume no knowledge aout JWT is needed to process Dags. I assume a rotation is not needed there as well.
Other maintainer opinions welcome... maybe happy to be enlightened.
|
Thanks for the review. Triggerer and workers : They act as clients (consume/execute), they don’t issue JWTs, so they don’t need to be restarted when the JWT secret rotates. And from a security perspective, they ideally shouldn’t have access to the JWT secret at all. Dag processor, Same idea: if it only runs the internal API server (and may become an API client later), it doesn’t need the JWT secret to process DAGs, so no need to restart it on JWT rotation. The PR only added the checksum where the chart already injects the JWT secret (via the shared env when enableBuiltInSecretEnvVars.AIRFLOW__API_AUTH__JWT_SECRET is true), so the intent was “when the secret is rotated, restart these pods so they pick up the new value.” |
|
Okay, yeah then... to correct this can we reverse it and rather say: The JWT should not be populated to Pods where not needed? The signal is that they have no checksum. |
858d456 to
9d0617a
Compare
|
Addressed the review: JWT secret is now only injected into api-server and scheduler (not dag-processor, triggerer, or workers). Removed the checksum/jwt-secret annotation from those three; updated the shared helper to use an IncludeJwtSecret flag and adjusted the helm test expectations. Rebased on main; ran prek (static checks) and helm tests (test_airflow_common, test_annotations, apiserver) Ready for another look once CI is green. |
…ectations Made-with: Cursor
9d0617a to
711687f
Compare
jscheffl
left a comment
There was a problem hiding this comment.
Looks much better and thanks for the rework!
Changed the title of PR to match current state - is this OK in your view?
Some small comments or I mis-understand.
| env: | ||
| {{- include "custom_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" (merge (dict "IncludeJwtSecret" true) .) | indent 10 }} |
There was a problem hiding this comment.
The "wait-for-migrations" does not need the JWT secret in my view.
| env: | ||
| {{- include "custom_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" (merge (dict "IncludeJwtSecret" true) .) | indent 10 }} |
There was a problem hiding this comment.
The "wait-for-migrations" does not need the JWT secret in my view.
…-server, scheduler)
|
Agreed on wait-for-migrations: it only needs DB/config to run migrations, not the JWT secret. I’ve updated the chart so the wait-for-migrations init container uses IncludeJwtSecret: false in both api-server and scheduler |
| env: | ||
| {{- include "custom_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" . | indent 10 }} | ||
| {{- include "standard_airflow_environment" (merge (dict "IncludeJwtSecret" false) .) | indent 10 }} |
There was a problem hiding this comment.
If JWT secret is not needed in all components, maybe we should remove it from standard_airflow_environment?
Summary
Deployments that use the chart-managed
jwt-secret(dag-processor, triggerer, workers) did not have thechecksum/jwt-secretpod template annotation. When the JWT secret value was updated, those pods were not restarted automatically, unlike the api-server and scheduler which already had the annotation. This adds the same annotation (gated bysemverCompare ">=3.0.0"andnot .Values.jwtSecretName) so a change to the jwt-secret triggers a rollout.Change
checksum/jwt-secretannotation when Airflow >= 3.0 and chart manages the JWT secret.Condition matches the pattern used in scheduler and api-server so behavior is consistent.
Fixes
Fixes #62146