gcsfs icon indicating copy to clipboard operation
gcsfs copied to clipboard

GCSFileSystem constructor with token=None does not try to get credentials from metadata service

Open dynamix opened this issue 5 years ago • 7 comments

Tried connect methods according to the documentation: https://github.com/dask/gcsfs/blob/2021a89471990c58f3acaf84bd4e277c0d6c2e4d/gcsfs/core.py#L143-L146

However the methods are: https://github.com/dask/gcsfs/blob/2021a89471990c58f3acaf84bd4e277c0d6c2e4d/gcsfs/core.py#L408

The google compute metadata service is missing in the current implementation.

Suggested fix: https://github.com/dask/gcsfs/pull/263

dynamix avatar Jun 03 '20 13:06 dynamix

This came up again and I found the reason why cloud was missing (actually intentionally removed): https://github.com/dask/gcsfs/pull/144.

We are running in various GCE environments (vanilla, GKE and AI platform). What actually caused google_default to fail for us, was not google.auth.default() but the exception raise in _connect_google_default which unfortunately is swallowed silently here. I added some more insightful logging in this PR: https://github.com/dask/gcsfs/pull/294

I configured gcsfs with our Google project id but if you run a customer container under AI platform, google.auth.default() returns a different temporary project id (i.e. odabb7293a2859199-ml) - so the check fails. Not providing the project to gcsfs fixes the issue but I'm not sure if the project id check in _connect_google_default is correct for all possible scenarios.

So it might make sense to remove cloud indeed from the list again and update the documentation.

dynamix avatar Oct 08 '20 11:10 dynamix

Would you like to finish off the logging PR and then update the documentation?

martindurant avatar Oct 22 '20 16:10 martindurant

I believe this is fixed

martindurant avatar May 13 '21 17:05 martindurant

FYI, due to this change, the project mismatch that @dynamix mentioned is no longer caught, so the token="cloud" fallback is never tried, and this causes a failure in current master. Ideally this try-catch would go back to catching all exceptions so the token="cloud" fallback is still tried in this case.

clarkzinzow avatar Dec 15 '21 02:12 clarkzinzow

Oh dear. So this is fixable, right? @clarkzinzow would you mind adding specific exceptions to the list to catch and fallback nicely? Probably catching all exceptions is overkill.

martindurant avatar Dec 15 '21 14:12 martindurant

@martindurant To keep this aggressively scoped down to failures that we know that we wish to swallow, gcsfs could define a CredentialsProjectMismatchError exception type, this could be the exception raised when use of the default credentials hits this error, and the catch could be expanded to cover this case:

class CredentialsProjectMismatchError(Exception):
    pass

except (
        google.auth.exceptions.GoogleAuthError,
        CredentialsProjectMismatchError) as e:
    ...

clarkzinzow avatar Dec 15 '21 18:12 clarkzinzow

That sounds very reasonable

martindurant avatar Dec 15 '21 18:12 martindurant