[GitHub] [airflow] kosteev commented on a diff in pull request #23773: Update credentials when using ADC in Compute Engine

[GitBox <gi...@apache.org>]

kosteev commented on code in PR #23773:
URL: https://github.com/apache/airflow/pull/23773#discussion_r877096966


##########
airflow/providers/google/cloud/utils/credentials_provider.py:
##########
@@ -248,6 +250,16 @@ def get_credentials_and_project(self) -> Tuple[google.auth.credentials.Credentia
 
             project_id = _get_project_id_from_service_account_email(self.target_principal)
 
+        if isinstance(credentials, compute_engine.Credentials):
+            try:
+                credentials.refresh(_http_client.Request())
+            except RefreshError as msg:
+                """
+                If the Compute Engine metadata service can't be reached in this case the instance has not
+                credentials.
+                """
+                self._log_debug(msg)

Review Comment:
   This looks like a best-effort approach.
   
   I we were able to retrieve credentials then good, otherwise also good but credentials returned by method will be empty.
   It means that the contract of the get_credentials_and_project method is that sometimes it returns you credentials sometimes not, which is in general not a good idea IMHO. At least we need to outline it in the docstring.
   Or should we just fail and raise exception with proper message?, especially if it is 4** error which is client error, not backend.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org