Skip to content

Conversation

@ijrsvt
Copy link

@ijrsvt ijrsvt commented Dec 8, 2021

Brings the logic from googleapis/google-auth-library-python#433 to this vendored package.

@ijrsvt ijrsvt force-pushed the accept-gce-metadata-host branch from c5388f2 to dfccc7e Compare December 8, 2021 22:20
@dilipped
Copy link

dilipped commented Dec 9, 2021

Thanks for writing the fix, but I would hold off from merging this PR for a couple of reasons

  • Looks like Cloud SDK uses GCE_METADATA_ROOT as well, and so does few other libraries as per the issue you have linked above, I would wait for a consensus to reach before making any changes here.
  • This library is now deprecated as mentioned in the README, and hence, only high priority security fixes will be merged in, since there is a work around to use GCE_METADATA_ROOT, we can probably hold this off.

@ijrsvt
Copy link
Author

ijrsvt commented Dec 9, 2021

@dilipped Where is it documented that Cloud SDK uses GCE_METADATA_ROOT. The gcloud CLI, Python & Golang libraries use the GCE_METADATA_HOST Env Var.

@dilipped
Copy link

dilipped commented Dec 9, 2021

Regarding Cloud SDK, I didn't check any documentation, but I've seen it being used in the code (it's not in a public repo so can't share any code). It might be the case that it is used only for some specific case, because looks like google-auth (which is used by Cloud SDK) has started using the new var https://github.com/googleapis/google-auth-library-python/blob/e2b3c98cd8c67b702be1b711c06ee7b9bbedb8ba/google/auth/environment_vars.py#L53.

@ijrsvt
Copy link
Author

ijrsvt commented Dec 9, 2021

@dilipped The reason I created this PR was to use the behaviour of the google-auth library. It seems like gsutil is pretty far away from switching to the google-auth library GoogleCloudPlatform/gsutil#565, so backporting this migration to the new env var seems reasonable.

@dilipped
Copy link

Fair enough. I think we can merge it, just going to leave a few nit comments.

@ijrsvt ijrsvt requested a review from dilipped December 14, 2021 16:23
@ijrsvt
Copy link
Author

ijrsvt commented Dec 15, 2021

@dilipped This is ready for review again :)

@dilipped
Copy link

Looks good. Were you able to run the tests

@ijrsvt
Copy link
Author

ijrsvt commented Dec 16, 2021

@dilipped Yep:

(oauth) ubuntu@ip-172-31-47-243:~/oauth2client$ DJANGO_SETTINGS_MODULE=tests.contrib.django_util.settings pytest -s .
<Output>
=========================================================================================== 519 passed, 72 warnings in 4.20s ============================================================================================

Full output in this gist.

@dilipped
Copy link

Thank you!!

@dilipped dilipped merged commit 30b5394 into gsutil-mirrors:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants