Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clear cache of the result of GCECredentials.on_gce? when GCECredentials.on_gce? returns false. #137

Merged
merged 1 commit into from
Jun 5, 2018
Merged

clear cache of the result of GCECredentials.on_gce? when GCECredentials.on_gce? returns false. #137

merged 1 commit into from
Jun 5, 2018

Conversation

serihiro
Copy link
Contributor

@serihiro serihiro commented Mar 14, 2018

What will this PR change ?

  • Modify to clear the cache of the result of GCECredentials.on_gce? if GCECredentials.on_gce? returns false.
  • This change resolves the problem which GCECredentials.on_gce? keeps returning false even though GCECredentials::COMPUTE_CHECK_URI is active, as I explained in the following.

Why is this change needed?

  • GCECredentials.on_gce? caches the result as a Class instance variable. So, once GCECredentials.on_gce? is called, this method keeps returning a same value in a same ruby process.
  • But, GCECredentials.on_gce? returns false occasionally even though COMPUTE_CHECK_URI is active.
  • So, if GCECredentials.on_gce? returns false in the first time call, GCECredentials.on_gce? keeps returning false within a same ruby process.
    • I faced this problem when I developed an ETL tool which accesses to BigQuery via google-cloud-ruby and uses GCE instance auth.
  • The following is my just experiment.
    • By calling Google::Auth::GCECredentials.on_gce? without caching in 30 times at GCE, I got
      false once in 30 times.

Note: Google::Auth::GCECredentials._unmemoized_on_gce? is a method which is defined by memoist implicitly, and is executed without caching.

irb(main):055:0> 30.times {pp Google::Auth::GCECredentials._unmemoized_on_gce? }
true
true
true
true
true
true
true
true
true
true
true
true
true
true
false
true
true
true
true
true
true
true
true
true
true
true
true
true
true
true
  • To avoid this problem, I think Google:: Auth#get_application_default should clear the cache of the result of GCECredentials.on_gce? when GCECredentials.on_gce? returns false.

when GCECredentials.on_gce? returns false.

Note:
GCECredentials.on_gce? returns false occasionally
even if the endpoint is active.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 14, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 98.508% when pulling 6d6bcd1 on serihiro:clear-cache-when-on_gce-returns-false into 18611aa on google:master.

@dazuma
Copy link
Member

dazuma commented Jun 5, 2018

This is a good catch: there definitely is the possibility of false negatives since this call depends on a network connection, and we shouldn't memoize those. Ideally we'd still want to memoize any "emphatic" negatives, but I can't think of a common case that would generate those. So LGTM.

@dazuma dazuma merged commit edaa784 into googleapis:master Jun 5, 2018
@serihiro serihiro deleted the clear-cache-when-on_gce-returns-false branch June 5, 2018 22:39
@serihiro
Copy link
Contributor Author

serihiro commented Jun 5, 2018

Thanks for reviewing and merging! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants