Skip to content

Comments

Add test for sensitive config masking in airflowctl#60361

Merged
bugraoz93 merged 3 commits intoapache:mainfrom
Subham-KRLX:fix/airflowctl-config-masking-clean
Jan 31, 2026
Merged

Add test for sensitive config masking in airflowctl#60361
bugraoz93 merged 3 commits intoapache:mainfrom
Subham-KRLX:fix/airflowctl-config-masking-clean

Conversation

@Subham-KRLX
Copy link
Contributor

@Subham-KRLX Subham-KRLX commented Jan 10, 2026

Description

Added test to verify sensitive config values are properly masked when using airflowctl.

The Airflow API already masks sensitive values (like fernet_key, sql_alchemy_conn) when returning config data. This test confirms that airflowctl correctly receives and displays those masked values.

Changes

  • Added config masking tests to tests/airflow_ctl/ctl/commands/test_config_command.py (CLI list command)
  • Added masking verification to tests/airflow_ctl/api/test_operations.py (API get operation)
  • Verifies ConfigOperations.list() handles masked data correctly

Related Issue

relates: #59843

AI Usage

Was generative AI tooling used to co-author this PR?
Yes, used an AI coding assistant to help generate test cases and refactor code structure.

Testing

pytest tests/airflow_ctl/ctl/commands/test_config_command.py
pytest tests/airflow_ctl/api/test_operations.py

@Subham-KRLX Subham-KRLX force-pushed the fix/airflowctl-config-masking-clean branch from 3fc9d40 to 0332c5f Compare January 10, 2026 05:19
@Subham-KRLX
Copy link
Contributor Author

@potiuk @bugraoz93 thanks for the review setup.
This PR adds a focused test for airflowctl preserving API-masked sensitive configs (e.g. fernet_key, sql_alchemy_conn).
One static check is failing (likely a minor lint/formatting issue post-force-push). Could you advise what to fix/add? Happy to iterate quickly to get it green.

@potiuk
Copy link
Member

potiuk commented Jan 21, 2026

You can use prek to fix it - just follow basic configuration of development environment / contributing docs to set it up and help you to fix the issues.

It's actually even written in the error message - no need to post questions and wait for someone- better to read what error message says and apply the remediation it suggests.

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Thanks for raising a PR! It would be cleaner if we didn't create a new file, but instead put the test in the file we already have for config command. We generally tend to have a test per Python file, which can serve for multiple purposes under the unit test category

https://github.com/apache/airflow/blob/main/airflow-ctl/tests/airflow_ctl/ctl/commands/test_config_command.py

As Jarek suggested, please use prek to fix. You can check the CI job and use the --all-files param on prek to rerun those for all files

@bugraoz93
Copy link
Contributor

We also have some under here for auto generated ones tests/.../api/test_operations.py. I think we should add test for get and export.

Export will be on specific file in above message test_config_command.py and for get it should be in test_operations.py

@Subham-KRLX
Copy link
Contributor Author

@bugraoz93 @potiuk I have reorganized the tests per your suggestions:

Moved test_config_list_masking_preservation() to test_config_command.py (for CLI list command)
Moved test_get_masked_value() to test_operations.py (for API get operation)
Removed the standalone test file
Fixed all static checks with prek
The PR now follows the "one test per Python file" convention.

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Looks better! Thanks!

Sorry for being double, but could you please add a test to airflow-ctl-test as well on this specific case? It could be on another PR as well, then we can just make this PR relates: #<issue> and next PR closes: #<issue> in the description? Because we are indeed testing if these lines work well with the line of codes but actual integration test can only be proven there. Although I am not sure if we need to put this integration test on both sides. For me, they should exist in both places since they are different distributions and released separately to fully show our tooling follows up on what has been written.
What do you think, @potiuk?

@bugraoz93
Copy link
Contributor

In integration tests, we should consider creating different test cases (different file, we can still use debug mode as initiation) then usual happy path. Splitting the integration test over the test scenario paths could be easier to manage for integration tests.

@Subham-KRLX
Copy link
Contributor Author

@bugraoz93 Thanks for the approval I will create a separate PR for the integration tests in airflow-ctl-tests. Will update this to Relates #59843 and the new PR will Closes #59843.

@bugraoz93
Copy link
Contributor

bugraoz93 commented Jan 29, 2026

@Subham-KRLX Could you please also update the description about AI usage? We have added a template that explains the AI usage on the solution

Was generative AI tooling used to co-author this PR?
Yes, explain which one or No

@Subham-KRLX
Copy link
Contributor Author

@bugraoz93 All necessary changes have been made including the updated AI usage section in the description as requested.

Test verifies that ConfigOperations.list() correctly preserves masked
values (e.g., '< hidden >') from the API response without accidentally
exposing or modifying them during JSON parsing and model validation.

Addresses apache#59843
- Move test_config_sensitive_masking.py tests to existing test files:
  - test_config_list_masking_preservation() added to test_config_command.py for CLI list command
  - test_get_masked_value() added to test_operations.py for API get operation
- Remove standalone test file to maintain one test per Python file convention
- Run prek to fix all static check issues (ruff formatting)
- All tests verify API-masked sensitive values are preserved through the CLI

Closes apache#59843
@bugraoz93 bugraoz93 force-pushed the fix/airflowctl-config-masking-clean branch from 0721b0a to ea0c26e Compare January 31, 2026 17:46
@bugraoz93 bugraoz93 merged commit 9d83ec9 into apache:main Jan 31, 2026
62 checks passed
morelgeorge pushed a commit to morelgeorge/airflow that referenced this pull request Feb 1, 2026
* Add unittest to add a mechanism if adding to airflowctl preserves the API's sensitive config masking
shashbha14 pushed a commit to shashbha14/airflow that referenced this pull request Feb 2, 2026
* Add unittest to add a mechanism if adding to airflowctl preserves the API's sensitive config masking
jason810496 pushed a commit to abhijeets25012-tech/airflow that referenced this pull request Feb 3, 2026
* Add unittest to add a mechanism if adding to airflowctl preserves the API's sensitive config masking
jhgoebbert pushed a commit to jhgoebbert/airflow_Owen-CH-Leung that referenced this pull request Feb 8, 2026
* Add unittest to add a mechanism if adding to airflowctl preserves the API's sensitive config masking
choo121600 pushed a commit to choo121600/airflow that referenced this pull request Feb 22, 2026
* Add unittest to add a mechanism if adding to airflowctl preserves the API's sensitive config masking
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.

3 participants