Skip to content

Apply the ambiguous-id refusal to get_config in the Key Vault backend - #70899

Merged
Miretpl merged 1 commit into
apache:mainfrom
potiuk:guard-get-config-in-the-key-vault-backend
Aug 1, 2026
Merged

Apply the ambiguous-id refusal to get_config in the Key Vault backend#70899
Miretpl merged 1 commit into
apache:mainfrom
potiuk:guard-get-config-in-the-key-vault-backend

Conversation

@potiuk

@potiuk potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member

Follow-up to #70876.

The ambiguous-id check used to live in _get_secret, which all three getters
share. Moving it up into get_conn_value and get_variable left get_config
reaching _get_secret unguarded, so _names_a_team_namespace's claim that such
an id is refused for every lookup stopped being true the moment it was written —
two getters and the docstring disagreed with the third.

Not a cross-team read. get_config never receives a team_name, so no team
namespace is reachable through it. The defect is the inconsistency and the false
docstring, not an exposure.

@shahar1 found this while reviewing the Amazon sibling (#70878), where it is
already fixed. Azure had the same shape and had merged by then, so this is the
other half.

_names_a_team_namespace stays an instance method here, unlike Amazon's,
because it builds the candidate path with self.build_path and self.sep.

Test plan

  • test_refusing_an_ambiguous_id_is_logged extended to get_config; verified
    it fails without the guard
  • Assertion reworked off the rendered sentence — Airflow logs through
    structlog, which renders format args into msg before the stdlib record
    exists, so record.args is empty and there is no structured payload to
    assert on; it now asserts level, logger and the refused id
  • test_key_vault.py — 20 passed
  • ruff check / ruff format clean
Was generative AI tooling used to co-author this PR?
  • Yes — Claude Opus 5 (1M context)

Generated-by: Claude Opus 5 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions

@potiuk potiuk added this to the Airflow 3.3.1 milestone Aug 1, 2026
@potiuk potiuk added the backport-to-v3-3-test Backport to v3-3-test label Aug 1, 2026
@potiuk
potiuk requested a review from vincbeck August 1, 2026 12:36
@potiuk
potiuk requested a review from o-nikolas as a code owner August 1, 2026 13:04
@potiuk

potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

CI fixed, and the fix reaches beyond this PR — the Amazon tests already on main were failing the same way, so this now repairs those too.

The failures were Compat 2.11.1:P3.10 and Compat 3.0.6:P3.10, all three backends, assert 0 == 1. Cause: the assertions read record.msg directly, and how a record carries its payload depends on the Airflow version.

# main — structlog renders the args before the stdlib record exists
LogRecord(msg="Connection id 'prod--x' contains '--'", args=())
    "prod--x" in r.msg          -> True

# compat targets — plain stdlib logging keeps the template
LogRecord(msg="%s id %r contains %r", args=("Connection", "prod--x", "--"))
    "prod--x" in r.msg          -> False        <- the failure
    "prod--x" in r.getMessage() -> True

getMessage() renders in both shapes, so the assertion is now version-portable. It still targets the refused id rather than the wording, which was the point of moving off the rendered sentence in the first place.

This is my own regression: I switched those assertions from getMessage() to msg in #70878 while addressing @shahar1's review, verified only against main, and merged. Compat runs on provider PRs, so it surfaced on the next provider PR rather than on the one that introduced it.

@potiuk
potiuk force-pushed the guard-get-config-in-the-key-vault-backend branch from 47506d7 to ce3c021 Compare August 1, 2026 13:09
@potiuk

potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Rewritten to Azure only — the repair to main moved to #70905.

I had folded the Amazon test fix in here because it was the same one-line cause, but that conflated two things: this PR is a follow-up guard for Key Vault, whereas main being red on the compat jobs is a regression that should land on its own and not wait behind a review of unrelated code.

This branch is now:

providers/microsoft/azure/src/.../secrets/key_vault.py       +11/-3
providers/microsoft/azure/tests/.../secrets/test_key_vault.py +16/-3

The getMessage() change remains in the Azure test here, since that test is added by this PR and would otherwise land already broken for the compat jobs.

Follow-up to apache#70876. The check used to live in _get_secret, which all three
getters share. Moving it up into get_conn_value and get_variable left get_config
reaching _get_secret unguarded, so the helper's claim that such an id is refused
for every lookup stopped being true. Not a cross-team read -- get_config never
receives a team_name -- but two getters and the docstring disagreed with the
third.

The same gap was found in review of the Amazon sibling (apache#70878) and fixed there;
this is the Azure half, which had already merged by then.

_names_a_team_namespace stays an instance method here, unlike Amazon's: it builds
the candidate path with self.build_path and self.sep.

The refusal-logging assertion uses getMessage() rather than msg, since how a
record carries its payload differs between the Airflow version this runs on and
the ones the provider compat tests use.
@potiuk
potiuk force-pushed the guard-get-config-in-the-key-vault-backend branch from ce3c021 to d41d9da Compare August 1, 2026 14:40
@Miretpl
Miretpl merged commit cfe3f0b into apache:main Aug 1, 2026
83 checks passed
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Backport failed to create: v3-3-test. View the failure log Run details

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
v3-3-test Commit Link

You can attempt to backport this manually by running:

cherry_picker cfe3f0b v3-3-test

This should apply the commit to the v3-3-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

If you don't have cherry-picker installed, see the installation guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants