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

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

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


##########
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:
   I think we cannot assume we have Metadata server around. This code should work fine whether we have the metadada server or not. IMHO this method should do what it says and return credentials when they are set.
   
   Is there a way to actually check if the credentials we got at this place are ACTUALLY empty? As I understand - we can get the credentials in many ways before - from file, secret, keyfile dictionary or even using "google default" authentication - which does not need refreshing (and that's why Breeze and other places where no meta-data server is available will continue to work - because they got credentials using one of those).
   
   The credentials retrieved this way are perfectly valid. Raising error is not good.
   
   But there is one case that actually **should** throw an error here:
   
   1) none of the methods above were configured
   2) default authentication did not provide any authenticatio
   3) refresh did not work
   
   In this case, yeah - we should throw an error, as there is no sense continuing and returnign "empty credentials" in this case does not help anyone (and fail-early is the best approach for that).
   
   So - can we throw an error when none-of the above methods actually returned valid credentials and refresh did not do it either?



-- 
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