kaxil commented on code in PR #60108:
URL: https://github.com/apache/airflow/pull/60108#discussion_r2957016171
##########
airflow-core/src/airflow/api_fastapi/auth/tokens.py:
##########
@@ -447,15 +447,33 @@ def signing_arg(self) -> AllowedPrivateKeys | str:
assert self._secret_key
return self._secret_key
Review Comment:
`generate_workload_token` reads `conf.getint("execution_api",
"jwt_workload_token_expiration_time")` on every call. Since this is called
per-task during scheduling, consider reading the value once at generator
construction time (like `valid_for` is).
Also, ashb's earlier comment on the prior PR (#62582) suggested this method
may be unnecessary — callers could use `generate(extras={"sub": sub, "scope":
"workload"}, valid_for=workload_expiry)` directly, keeping the generator
generic.
##########
airflow-core/src/airflow/api_fastapi/execution_api/app.py:
##########
@@ -332,10 +336,23 @@ async def always_allow(request: Request):
)
return TIToken(id=ti_id, claims={"scope": "execution"})
+ # Override _container (the svcs service locator behind
DepContainer).
+ # The default _container reads request.app.state.svcs_registry, but
+ # Cadwyn's versioned sub-apps don't inherit the main app's state,
+ # so lookups raise ServiceNotFoundError. This registry provides
Review Comment:
`_jwt_generator()` calls
`get_signing_args(make_secret_key_if_needed=False)`, which raises `ValueError`
when no signing key is configured. In the `InProcessExecutionAPI` context (used
by `dag.test()`), users typically won't have JWT signing keys set up.
Since `always_allow` already bypasses auth checks, consider registering a
stub/mock `JWTGenerator` here that returns a dummy token, instead of wiring up
the real factory that requires signing key configuration.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -2069,6 +2069,15 @@ execution_api:
type: integer
example: ~
default: "600"
+ jwt_workload_token_expiration_time:
+ description: |
+ Seconds until workload JWT tokens expire. These long-lived tokens are
sent
+ with task workloads to executors and can only call the /run endpoint.
Review Comment:
`version_added` is `~` (null). Should be set to `3.2.0` (or whichever
version this ships in).
##########
airflow-core/tests/unit/api_fastapi/execution_api/conftest.py:
##########
@@ -53,6 +57,11 @@ async def mock_jwt_bearer(request: Request):
exec_app.dependency_overrides[_jwt_bearer] = mock_jwt_bearer
with TestClient(app, headers={"Authorization": "Bearer fake"}) as client:
+ mock_generator = MagicMock(spec=JWTGenerator)
+ mock_generator.generate.return_value = "mock-execution-token"
+ mock_generator.generate_workload_token.return_value =
"mock-workload-token"
+ lifespan.registry.register_value(JWTGenerator, mock_generator)
Review Comment:
`lifespan.registry.register_value(JWTGenerator, mock_generator)` mutates the
module-level svcs registry, which persists across tests. The fixture cleans up
`dependency_overrides` (line 67) but not this registry mutation. Other tests
that depend on `JWTGenerator` from the registry could be affected by test
ordering.
Consider adding a teardown step (after `yield`) to deregister/reset the
`JWTGenerator` entry in the registry.
--
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]
