Skip to content

Handle undecryptable Variable values gracefully in Stable REST API#65452

Merged
pierrejeambrun merged 5 commits into
apache:mainfrom
shan-zeeshan786:get_variable_failure
Jun 1, 2026
Merged

Handle undecryptable Variable values gracefully in Stable REST API#65452
pierrejeambrun merged 5 commits into
apache:mainfrom
shan-zeeshan786:get_variable_failure

Conversation

@shan-zeeshan786

Copy link
Copy Markdown
Contributor

Fix Variables API returning 500 for undecryptable values

What is the problem?
When a Fernet key is rotated or misconfigured, previously stored encrypted Variable values may become undecryptable.

Calling:
GET /api/v1/variables/{key}
result in a 500 Internal Server Error with a schema validation message

While calling list varibales
GET /api/v2/variables
returns the variables successfully, but with values as null when decryption fails.

List api output:
{
"total_entries": 1,
"variables": [
{
"description": "Testing variable use",
"key": "Testing",
"value": null
}
]
}

GET variable Response:
{
"detail": "None is not of type 'string'\n\nFailed validating 'type' in schema['allOf'][1]['properties']['value']:\n{'type': 'string'}\n\nOn instance['value']:\n None",
"status": 500,
"title": "Response body does not conform to specification",
"type": "https://airflow.apache.org/docs/apache-airflow/3.2.0/stable-rest-api-ref.html#section/Errors/Unknown"
}

This occurs because the decrypted value becomes None, which violates the OpenAPI schema expecting a string and inconsistent and confusing API behavior.

Expected output:
{
"description": "Testing variable use",
"key": "Testing",
"value": null
}

Why is this change needed?

  • A 500 error is misleading since this is a predictable scenario (Fernet mismatch)
  • The API response violates schema validation
  • Leads to inconsistent and confusing API behavior
  • Impacts API consumers relying on stable responses

What is changed?
The value field in the Variables API response schema has been updated to handle undecryptable values safely:

  • Before:
    val: str = Field(alias="value")
  • After
    val: str | None = Field(alias="value", default=None)

Why this change?

  • Previously, the field strictly required a string
  • When Fernet decryption failed, the value became None
  • This caused schema validation errors → 500 Internal Server Error

Related issue
open: #65391

@boring-cyborg

boring-cyborg Bot commented Apr 18, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@potiuk

potiuk commented Apr 19, 2026

Copy link
Copy Markdown
Member

This is far too little for a PR. This one needs to:

a) explain in the code (not only in PR) we are doing it in the code
b) add unit tests covering the functionality
c) explain changed behaviour and discuss compatibility
d) possibly introduce cadwyn migrations
c) update the user documentation as needed.

potiuk
potiuk previously requested changes Apr 19, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a lot more.

@shan-zeeshan786 shan-zeeshan786 requested a review from potiuk April 20, 2026 18:31
@shan-zeeshan786

Copy link
Copy Markdown
Contributor Author

Behavior change

Before: GET /variables/{key} returned HTTP 500 with an OpenAPI validation error when the stored value could not be decrypted.

After: The same endpoint returns HTTP 200 with "value": null, matching what the list endpoint already did.

Compatibility

  • This is a server-side behavior fix, not a new feature.
  • Existing consumers that assumed value is always a string should tolerate null (list endpoint already returned null in the same situation).
  • No request-schema change. Creating/updating variables still requires a non-null value.
  • No DB migration needed.

@potiuk potiuk added ready for maintainer review Set after triaging when all criteria pass. and removed ready for maintainer review Set after triaging when all criteria pass. labels Apr 22, 2026
@potiuk potiuk marked this pull request as draft April 23, 2026 01:01
@potiuk

potiuk commented Apr 23, 2026

Copy link
Copy Markdown
Member

@shan-zeeshan786 Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.

  • Failing CI: Kubernetes tests / K8S System:CeleryExecutor-3.10-v1.30.13-false, Special tests / Min SQLAlchemy test: core / DB-core:MinSQLAlchemy-Postgres:14:3.10:API...CLI, Special tests / Latest SQLAlchemy test: core / DB-core:LatestSQLAlchemy-Postgres:14:3.10:API...CLI (+6 more). Please investigate and fix.

See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush.


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@shan-zeeshan786 shan-zeeshan786 changed the base branch from main to v3-2-test April 29, 2026 14:41
@shan-zeeshan786 shan-zeeshan786 force-pushed the get_variable_failure branch 2 times, most recently from dc9033c to a04ad3d Compare April 29, 2026 17:01
shan-zeeshan786 pushed a commit to shan-zeeshan786/airflow that referenced this pull request Apr 30, 2026
…t mock, regenerate openapi spec + UI/airflowctl datamodels
shan-zeeshan786 pushed a commit to shan-zeeshan786/airflow that referenced this pull request May 1, 2026
…rate openapi/airflowctl datamodels, single-line newsfragment, real fernet decrypt mock
@shan-zeeshan786 shan-zeeshan786 marked this pull request as ready for review May 1, 2026 13:45
@potiuk

potiuk commented May 1, 2026

Copy link
Copy Markdown
Member

Why is it targetting v3-2-test and not main @shan-zeeshan786 ? Can you explain please. I will convert it to draft until this is explained. Usually we fix things in main and cherry-pick - see contrib guides

@potiuk potiuk marked this pull request as draft May 1, 2026 15:24
@shan-zeeshan786

shan-zeeshan786 commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

In one of my PR @pierrejeambrun suggested to target *-test branch that's why i changed it to test instead of main @potiuk. Here also, lots of CI were failing.
Please check #65466 for that comment.

ss

@shan-zeeshan786 shan-zeeshan786 changed the base branch from v3-2-test to main May 1, 2026 16:28
@shan-zeeshan786 shan-zeeshan786 changed the base branch from main to v3-2-test May 1, 2026 16:29
Md Zeeshan Alam added 4 commits May 1, 2026 22:16
code, add a regression test, and add newsfragment 65452.bugfix.rst
…t mock, regenerate openapi spec + UI/airflowctl datamodels
…rate openapi/airflowctl datamodels, single-line newsfragment, real fernet decrypt mock
@shan-zeeshan786 shan-zeeshan786 force-pushed the get_variable_failure branch from a7148be to bc5ff80 Compare May 1, 2026 16:46
@shan-zeeshan786 shan-zeeshan786 changed the base branch from v3-2-test to main May 1, 2026 16:46
@shan-zeeshan786 shan-zeeshan786 marked this pull request as ready for review May 1, 2026 16:47
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 5, 2026
@pierrejeambrun

pierrejeambrun commented May 15, 2026

Copy link
Copy Markdown
Member

In one of my PR @pierrejeambrun suggested to target *-test branch that's why i changed it to test instead of main @potiuk. Here also, lots of CI were failing.

Please check #65466 for that comment.

ss

That's because you were targeting airflow 2.x, so latest v2-x-test branch is the appropriate one.

Here if you target airflow 3.x it's handled differently. We always merge to main. Then people merging or the release manager will handle backporting to the appropriate minor version branch (v3-2-test) if needed.

@shan-zeeshan786

Copy link
Copy Markdown
Contributor Author

In one of my PR @pierrejeambrun suggested to target *-test branch that's why i changed it to test instead of main @potiuk. Here also, lots of CI were failing.
Please check #65466 for that comment.
ss

That's because you were targeting airflow 2.x, so latest v2-x-test branch is the appropriate one.

Here if you target airflow 3.x it's handled differently. We always merge to main. Then people merging or the release manager will handle backporting to the appropriate minor version branch (v3-2-test) if needed.

Thank you for the clarification, @pierrejeambrun. I had already updated the target branch to main at that time. Could you please review and validate this PR when convenient?

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few suggestion before we can merge.

@potiuk If you mind taking another look and reconsider your 'request for change' it would be great.

Comment thread airflow-core/newsfragments/65452.bugfix.rst Outdated
Comment thread airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py Outdated
Comment thread airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py Outdated
@shan-zeeshan786

Copy link
Copy Markdown
Contributor Author

LGTM, just a few suggestion before we can merge.

@potiuk If you mind taking another look and reconsider your 'request for change' it would be great.

Removed all extra comments, suggested by @pierrejeambrun.

@pierrejeambrun pierrejeambrun merged commit f4cc43d into apache:main Jun 1, 2026
145 checks passed
@boring-cyborg

boring-cyborg Bot commented Jun 1, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-2-test

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-2-test PR Link

pierrejeambrun pushed a commit that referenced this pull request Jun 1, 2026
… REST API (#65452) (#67828)

* Allow null values for Variable value field to handle decryption failures gracefully

* Document rationale in
code, add a regression test, and add newsfragment 65452.bugfix.rst

* Fix CI for #65452: single-line newsfragment, real fernet decrypt mock, regenerate openapi spec + UI/airflowctl datamodels

* Fix CI for #65452: handle nullable Variable.value in UI, regenerate openapi/airflowctl datamodels, single-line newsfragment, real fernet decrypt mock

* Removed all the comments, suggested by the reviewers

---------
(cherry picked from commit f4cc43d)

Co-authored-by: Md Zeeshan alam <93471402+shan-zeeshan786@users.noreply.github.com>
Co-authored-by: Md Zeeshan Alam <zeeshan@Mds-MacBook-Air.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants