amoghrajesh opened a new pull request, #71756:
URL: https://github.com/apache/airflow/pull/71756
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull
request.
Write a good git commit message following this guide:
https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the
documentation.
For user-facing UI changes, please attach before/after screenshots (or a
short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for
a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we
sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR,
please
change below checkbox to `[X]` followed by the name of the tool, uncomment
the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
Follow-up to https://github.com/apache/airflow/pull/71701 but for generic
secrets backends, which removed the two token validation calls the HashiCorp
provider made per secret. This removes the repeated authentication underneath
them.
## The problem
`_get_connection` and `_get_variable` call `ensure_secrets_backend_loaded()`
inside the
lookup itself. That reaches `sdk/configuration.py` and ends at a bare
`secrets_backend_cls(**backend_kwargs)` in the shared config parser, so every
lookup gets a new backend, a new client, and a cold `cached_property`. For
Vault that means a fresh AppRole login per secret.
Measured against a real Vault: two variables fetched in one process produced
**two** AppRole logins.
The SDK function is inspired was copied from `airflow.configuration`. One
line differs:
```python
# airflow-core
secrets_backend_list # module global,
built once
# task-sdk, before this change
secrets_backend_list = initialize_secrets_backends() # local, rebuilt
every call
```
The copy turned a reference to a cached global into a fresh call to build
one. Everything downstream follows from that substitution, which is why core
never showed the problem and the SDK always did.
## Why not simply mirror core
Core keeps one module-level list and returns it only when the requested
search path is the
server default. Any other path takes the rebuild branch. The task runner
always asks for `DEFAULT_SECRETS_SEARCH_PATH_WORKERS`, which is not the server
default, so a faithful copy of
core would leave worker lookups rebuilding exactly as they do today.
Caching per search path is what makes both chains cacheable. The SDK also
defers config initialisation deliberately via `__getattr__`, so populating at
import the way core does would regress that.
## Preserved behaviour
Core's `len(secrets_backend_list) == 2` check exists so that a custom
backend configured after
startup is still picked up. That is kept, expressed as `len(backends) ==
len(default_backends)`
so it holds for the worker chain too rather than hardcoding the server
chain's length. When no
custom backend is found nothing is stored and every call rebuilds, exactly
as before.
I traced the old and new implementations across every combination. Chain
contents are identical
in all eight:
| `[workers]` / `[secrets]` | chain | backends | cached |
|---|---|---|---|
| set / unset | server | env, metastore | no |
| set / unset | worker | **metastore**, env, execapi | yes |
| set / set | server | **env**, env, metastore | yes |
| set / set | worker | **metastore**, env, execapi | yes |
| unset / set | server | **env**, env, metastore | yes |
| unset / set | worker | **env**, env, execapi | yes |
| unset / unset | server | env, metastore | no |
| unset / unset | worker | env, execapi | no |
Bold is the custom backend prepended by `initialize_secrets_backends`.
Caching engages only
where one exists for the chain being requested.
## One behaviour difference
The old code always built the server chain first and discarded it, even when
the worker chain
was requested. That discarded construction could raise:
```
[secrets] backend = VaultBackend (misconfigured)
[workers] secrets_backend = MetastoreBackend (fine)
requesting the worker chain:
before: VaultError: The 'token' authentication type requires 'token' or
'token_path'
after: ['MetastoreBackend', 'EnvironmentVariablesBackend',
'ExecutionAPISecretsBackend']
```
A worker with a valid backend no longer fails because of an unrelated
server-side
misconfiguration it never uses. It needs `[workers] secrets_backend` set to
something different
and `[secrets] backend` broken enough to raise on construction; with
`[workers]` unset the
worker falls back to `[secrets] backend` and both versions raise identically.
## Credential expiry across backends
Holding an instance for the life of a process means an expiring credential
has to refresh. All seven external backends were checked:
| Backend | Auth state held | Refreshes? |
|---|---|---|
| amazon (secrets manager, SSM) | boto3 session | yes, botocore credential
resolver |
| google | google-auth credentials | yes |
| microsoft/azure | azure-identity credentials | yes |
| akeyless | own `_cached_token` with `_token_expiry`, TTL 600s | yes,
itself |
| yandex | `yandexcloud.SDK` from OAuth token or SA key | SA key path
refreshes; OAuth tokens are long-lived |
| hashicorp | static hvac token | yes, via the 403 retry added in
#<FIX_A_PR> |
| cncf/kubernetes | `load_incluster_config()` in a `cached_property` |
**no** |
The kubernetes backend reads the projected service account token from disk
once. Bound tokens
expire and kubelet rotates the file, and the Python client does not re-read
it.
That gap is not introduced here. Core has cached backend instances for the
life of the process
on the server path for years, so the same defect already exists in the API
server, scheduler and
Dag processor. This change makes it reachable in a worker whose task
outlives the token. It is
worth fixing separately, with the same shape used for Vault: catch the 401,
drop the cached
client, retry once.
## Improvements
Setup breeze with `VaultBackend` and create 5 variables:
```shell
alias v='docker exec -e VAULT_ADDR=http://127.0.0.1:8200 -e VAULT_TOKEN=root
vault-test vault'
v write auth/approle/role/airflow token_policies=airflow token_type=batch
for i in 1 2 3 4 5; do v kv put -mount=secret variables/var$i
value=value-$i; done
```
Use this dag:
```python
from __future__ import annotations
from datetime import datetime
from airflow.sdk import DAG, Variable, task
with DAG("five_vars", schedule=None, start_date=datetime(2024, 1, 1),
catchup=False):
@task
def read_five():
return [Variable.get(f"var{i}") for i in range(1, 6)]
read_five()
```
### Before the changes
From audit file:
```shell
amoghrajesh.desai …/airflow main $? orbstack ♥ 10:27 docker
exec vault-test cat /vault/logs/audit.log \
| jq -r 'select(.type=="request") | .request.path' | sort | uniq -c
5
auth/approle/login
1 secret/data/variables/var1
1 secret/data/variables/var2
1 secret/data/variables/var3
1 secret/data/variables/var4
1 secret/data/variables/var5
```
### After the changes:
<img width="2560" height="509" alt="image"
src="https://github.com/user-attachments/assets/e52f5b9f-c9c6-4001-8101-738af8f0de72";
/>
From audit file:
```shell
amoghrajesh.desai …/airflow vault-login-cost-reduction-part-2 $?
orbstack ♥ 10:32 docker exec vault-test cat /vault/logs/audit.log \
| jq -r 'select(.type=="request") | .request.path' | sort | uniq -c
1
auth/approle/login
1 secret/data/variables/var1
1 secret/data/variables/var2
1 secret/data/variables/var3
1 secret/data/variables/var4
1 secret/data/variables/var5
```
One Airflow task reading five distinct variables, counted from Vault's own
file audit device.
| Path | main | this PR |
|---|---|---|
| `auth/approle/login` | 5 | **1** |
| `secret/data/variables/var1` … `var5` | 5 | 5 |
| **total** | **10** | **6** |
Cumulative across both changes, same workload:
| | logins | `lookup-self` | reads | total |
|---|---|---|---|---|
| Before either change | 5 | 10 | 5 | 20 |
| #71701 | 5 | 0 | 5 | 10 |
| This PR | 1 | 0 | 5 | 6 |
Six is the floor for five distinct secrets: one authentication plus one read
each. Reads do not
drop because the secrets backend interface fetches one key at a time and
there is no bulk read.
---
* Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information. Note: commit author/co-author name and email in commits
become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License
Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment:
`{pr_number}.significant.rst`, in
[airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
You can add this file in a follow-up commit after the PR is created so you
know the PR number.
--
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]
