Fix CloudSecretManagerBackend regression with explicit project_id (issue #61217)#61654
Fix CloudSecretManagerBackend regression with explicit project_id (issue #61217)#61654dv-gorasiya wants to merge 7 commits intoapache:mainfrom
Conversation
…sue apache#61217) Previously, would fail when Application Default Credentials (ADC) had no default project, because raised an AirflowException before the explicit project_id could be applied. Changes: 1. returns empty string ("") instead of raising when ADC yields None project_id. 2. now validates that a project_id is available (either from ADC or explicit parameter) and raises a clear error. Backward compatibility: - Callers that relied on the exception still get one (now from the backend). - Callers that pass an explicit project_id now work correctly. - No change to the public API of .
shahar1
left a comment
There was a problem hiding this comment.
- There aren't seem to be new unit tests as the description proclaims.
- For the next time - before using generative AI, please read the guidelines Gen-AI assisted contribtutions - it should be declared explicitly in the PR's description (see the template when creating a new PR).
|
Thanks for the review, @shahar1. I've addressed both points:
The changes are ready for another look. |
- Add test for get_credentials_and_project_id returning empty string when ADC yields None project_id - Add validation tests for CloudSecretManagerBackend project_id determination - Addresses review feedback about missing tests and AI disclosure Fixes: apache#61217
Static checks still fail, could you please fix? |
shahar1
left a comment
There was a problem hiding this comment.
Overall looks good, but I'm a bit concerned about the impact on existing operators.
Could you please demonstrate how it fixes the regression by use-casing minimal examples with few different GCP operators (code+screenshots), including before and after the changes.
Thank you!
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py
Outdated
Show resolved
Hide resolved
providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.py
Show resolved
Hide resolved
|
Thanks @shahar1! Yeah sure, Scenario: ADC has no default project, but explicit project_id is providedThis is the core regression from #61217. 1. CloudSecretManagerBackend (secrets backend) Codefrom unittest import mock
with mock.patch("google.auth.default", return_value=(mock.MagicMock(), None)):
from airflow.providers.google.cloud.secrets.secret_manager import CloudSecretManagerBackend
# BEFORE: raises AirflowException from _get_credentials_using_adc()
# before __init__ can apply project_id - broken
# AFTER: initializes successfully, explicit project_id is honored
backend = CloudSecretManagerBackend(project_id="my-project")
print(backend.project_id) # "my-project"2. BigQueryHook (operator-level usage) Hooks and operators that call Codefrom unittest import mock
from airflow.providers.google.cloud.utils.credentials_provider import get_credentials_and_project_id
with mock.patch("google.auth.default", return_value=(mock.MagicMock(), None)):
creds, project_id = get_credentials_and_project_id()
# BEFORE: AirflowException raised here, never reaches the operator
# AFTER: returns (credentials, "") - empty string, not an exception
effective_project_id = "my-explicit-project" or project_id
print(effective_project_id) # "my-explicit-project"3. GCSHook / DataprocHook / any hook using BaseGoogleHook All Google hooks ultimately go through
Scenario: ADC has no default project and NO explicit project_idCodefrom unittest import mock
with mock.patch("google.auth.default", return_value=(mock.MagicMock(), None)):
from airflow.providers.google.cloud.secrets.secret_manager import CloudSecretManagerBackend
backend = CloudSecretManagerBackend()
# ValueError: Project ID could not be determined. Please provide
# 'project_id' in backend configuration or ensure your credentials
# include a default project.The user still gets a clear, actionable error. Scenario: ADC has a default project (normal/happy path)Codefrom unittest import mock
with mock.patch("google.auth.default", return_value=(mock.MagicMock(), "adc-project")):
from airflow.providers.google.cloud.secrets.secret_manager import CloudSecretManagerBackend
backend = CloudSecretManagerBackend()
print(backend.project_id) # "adc-project" - unchanged behaviorTested locally by running pytest on the two affected test files:
|
Sorry for the delay in response, currently on low availability - it will take a while until I'll be able to fully review it again properly. |
0e2d63d to
b022d57
Compare
|
Hey @shahar1, finally got around to this, set up a real GCP project and ran actual DAGs against it. Setup:
BEFORE fix (regression on
|
Great, thanks! As I'm still on low availability, it will take me a long while to go over it properly. @VladaZakharova @MaksYermak - I'll be happy if you could take a look as well. |
|
Thanks @shahar1, completely understand, no rush at all. The PR is in good shape from my side. @VladaZakharova @MaksYermak: happy to answer any questions. The core fix is in |
|
This pull request has had no activity from the author for over 4 weeks. We are converting it to draft to keep the review queue manageable. @dv-gorasiya, please mark this PR as ready for review when you are ready to continue working on it. Thank you for your contribution! |
|
Hello Hello @potiuk , It is ready for review awaiting reviwer, there isn't any action item open with me at the moment. Thanks. |






Summary
Fixes #61217: CloudSecretManagerBackend with explicit
project_idfails when Application Default Credentials (ADC) have no default project.Root Cause
_get_credentials_using_adc()raises anAirflowExceptionwhengoogle.auth.default()returnsNoneproject_id. This occurs beforeCloudSecretManagerBackend.__init__can apply the explicitproject_idparameter, causing the backend to fail even when a valid project ID is provided.Changes
credentials_provider.py–_get_credentials_using_adc()now returns an empty string ("") instead of raising when ADC yieldsNoneproject_id.secret_manager.py– Added validation in__init__that raisesAirflowExceptionif neither ADC nor the explicitproject_idparameter provides a project ID.blackandisortto both modified files.Backward Compatibility
CloudSecretManagerBackend.__init__with a clearer message.project_idnow work correctly – the explicit parameter is honored.get_credentials_and_project_id()still returnstuple[Credentials, str](empty string is a validstr).Testing
CloudSecretManagerBackendandcredentials_providerpass because they mockgoogle.auth.defaultto return a valid project ID.test_credentials_provider.py:test_get_credentials_and_project_id_with_default_auth_no_project_idverifies thatget_credentials_and_project_id()returns an empty string when ADC yieldsNoneproject_id.test_secret_manager.pyto verify thatCloudSecretManagerBackendraisesAirflowExceptionwhen project ID cannot be determined (ADC yieldsNoneand no explicitproject_idis provided).CloudSecretManagerBackend(project_id="my-project")now works when ADC lacks a default project.Impact on Other Callers
Other components that call
get_credentials_and_project_id()without an explicitkey_secret_project_idwill receive an empty string instead of anAirflowException. If those components do not validate the project ID, they may propagate the empty string to downstream Google APIs, which will produce a different error (e.g., "Invalid project"). This is acceptable because:project_idbeing ignored) is fixed.key_secret_project_id(or similar).Checklist
Gen-AI assisted contributions
This contribution was assisted by generative AI. The PR description was generated with AI assistance. The missing unit tests have been added after review to ensure correctness and compliance with project guidelines.
Related Issues