WIP Better credential workflow #444

Temikus · 2019-03-20T10:32:31Z

This PR centers around:

Reworking credentials workflow to be easier to read and fall back to google application default credentials
Removing deprecated google_client_email parameter everywhere

`google_clien_email` is an auth option that was leftover from PKCS12 key days and is no longer required as it is baked into all json keys and is no longer supplied to any auth method

- Separate different auth streams into private methods - Separated some logic into helper methods - Switch to File.read when loading up JSON key - Do not decode/re-encode JSON keys when loading up auth - Add a warning if we’re falling back on Google Application Default auth

houndci-bot · 2019-03-20T10:32:43Z

lib/fog/google/shared.rb

+        validate_json_credentials(json_key)
+
+        ::Google::Auth::ServiceAccountCredentials.make_creds(
+            :json_key_io => StringIO.new(json_key),


Layout/FirstParameterIndentation: Indent the first parameter one step more than the start of the previous line.

houndci-bot · 2019-03-20T10:32:44Z

lib/fog/google/shared.rb

+      def process_application_default_auth(options)
+        begin
+          return ::Google::Auth.get_application_default(options[:google_api_scope_url])
+        rescue RuntimeError => e


Lint/UselessAssignment: Useless assignment to variable - e.

houndci-bot · 2019-03-20T10:32:44Z

lib/fog/google/shared.rb

+      # @param [Hash]  options - client options hash
+      # @return [Google::Auth::DefaultCredentials] - google auth object
+      def process_application_default_auth(options)
+        begin


Style/RedundantBegin: Redundant begin block detected.

houndci-bot · 2019-03-20T10:32:44Z

lib/fog/google/shared.rb

+
+        if auth.nil?
+          raise Fog::Errors::Error.new(
+              "Failed to configure authentication for Fog client.\n" \


Layout/FirstParameterIndentation: Indent the first parameter one step more than the start of the previous line.

houndci-bot · 2019-03-20T10:32:44Z

lib/fog/google/shared.rb

-          auth = ::Google::Auth.get_application_default(
-            options[:google_api_scope_url]
+          Fog::Logger.warning(
+              "Didn't detect any explicit auth settings, " \


Layout/FirstParameterIndentation: Indent the first parameter one step more than the start of the previous line.

codecov · 2019-03-20T10:39:29Z

Codecov Report

Merging #444 into master will decrease coverage by 0.04%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   85.31%   85.27%   -0.05%     
==========================================
  Files         339      339              
  Lines        5829     5840      +11     
==========================================
+ Hits         4973     4980       +7     
- Misses        856      860       +4

Impacted Files	Coverage Δ
lib/fog/google/sql.rb	`100% <ø> (ø)`	⬆️
lib/fog/google/pubsub.rb	`96.66% <ø> (ø)`	⬆️
lib/fog/storage/google_json.rb	`100% <ø> (ø)`	⬆️
lib/fog/dns/google.rb	`100% <ø> (ø)`	⬆️
lib/fog/google/monitoring.rb	`100% <ø> (ø)`	⬆️
lib/fog/compute/google.rb	`100% <ø> (ø)`	⬆️
lib/fog/google/shared.rb	`57.3% <58.82%> (+0.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bad94d...26e5ebd. Read the comment docs.

hattorious

@Temikus Looks much easier to follow. I noted one configuration edge-case that I could reproduce.

I have a few more documentation nits that I haven't added so feel free to tag me once you're ready for a second set of eyes.

lib/fog/google/shared.rb

application_default -> google_application_default to follow convention

houndci-bot · 2019-03-20T22:50:46Z

lib/fog/google/shared.rb

+          return process_application_default_auth(options)
+        rescue
+          raise Fog::Errors::Error.new(
+              "Fallback auth failed, could not configure authentication for Fog client.\n" \


Layout/FirstParameterIndentation: Indent the first parameter one step more than the start of the previous line.

houndci-bot · 2019-03-20T22:50:47Z

lib/fog/google/shared.rb

+        )
+        begin
+          return process_application_default_auth(options)
+        rescue


Style/RescueStandardError: Avoid rescuing without specifying an error class.

houndci-bot · 2019-03-20T22:50:47Z

lib/fog/google/shared.rb

+      # @return [Google::Auth::DefaultCredentials] - google auth object
+      def process_fallback_auth(options)
+        Fog::Logger.warning(
+            "Didn't detect any client auth settings, " \


Layout/FirstParameterIndentation: Indent the first parameter one step more than the start of the previous line.

Temikus · 2019-03-20T23:00:49Z

@Mavin Reworked a bit according to your comments, now only fallback has a more user-friendly message covering the exceptions underneath but explicit application default auth will not rescue any exceptions.

PTAL and thanks again for the review!

hattorious

Documentation nits

hattorious · 2019-03-21T16:28:30Z

lib/fog/google/shared.rb

      # @option options [Google::Auth|Signet] :google_auth Manually created authorization to use
-      # @option options [String] :google_client_email A @developer.gserviceaccount.com email address to use
      # @option options [String] :google_key_location The location of a pkcs12 key file


nit: I'm not sure how ruby docs work but can this be removed since they have been deprecated for a while?

hattorious · 2019-03-21T21:41:38Z

lib/fog/google/shared.rb

      # @option options [Google::Auth|Signet] :google_auth Manually created authorization to use
-      # @option options [String] :google_client_email A @developer.gserviceaccount.com email address to use
      # @option options [String] :google_key_location The location of a pkcs12 key file
      # @option options [String] :google_key_string The content of the pkcs12 key file


and :google_client

hattorious · 2019-03-21T22:21:50Z

:LGTM: Now merge and release so I can get to work on mitchellh/vagrant-google#194 😁

Temikus · 2019-03-21T22:23:53Z

@Mavin will roll out the RC sometime today.

Thank you so much for your input and review!

Temikus added 3 commits March 20, 2019 15:36

Remove deprecated google_client_email option

03e91b1

`google_clien_email` is an auth option that was leftover from PKCS12 key days and is no longer required as it is baked into all json keys and is no longer supplied to any auth method

Remove leftover test for google_client_email

1249eed

Temikus changed the title ~~Better credential workflow~~ WIP Better credential workflow Mar 20, 2019

houndci-bot reviewed Mar 20, 2019

View reviewed changes

Temikus added the integrate label Mar 20, 2019

Temikus mentioned this pull request Mar 20, 2019

feat: Allow Google Application Default Credentials #442

Merged

hattorious reviewed Mar 20, 2019

View reviewed changes

lib/fog/google/shared.rb Outdated Show resolved Hide resolved

Temikus added 2 commits March 21, 2019 09:48

Put the fallback auth into a separate method

9612f93

Rename cred name

1e11db9

application_default -> google_application_default to follow convention

houndci-bot reviewed Mar 20, 2019

View reviewed changes

Address linter

5ba7505

hattorious approved these changes Mar 21, 2019

View reviewed changes

Remove deprecated options from docs

26e5ebd

Temikus merged commit c6b0533 into fog:master Mar 21, 2019

Temikus deleted the better_credential_workflow branch March 21, 2019 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Better credential workflow #444

WIP Better credential workflow #444

Temikus commented Mar 20, 2019

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading

hattorious left a comment

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

houndci-bot Mar 20, 2019

Temikus commented Mar 20, 2019

hattorious left a comment

hattorious Mar 21, 2019

hattorious Mar 21, 2019

hattorious Mar 21, 2019

hattorious commented Mar 21, 2019

Temikus commented Mar 21, 2019

WIP Better credential workflow #444

WIP Better credential workflow #444

Conversation

Temikus commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2019 • edited Loading

Codecov Report

hattorious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temikus commented Mar 20, 2019

hattorious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hattorious commented Mar 21, 2019

Temikus commented Mar 21, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading