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

Replace deprecated GoogleCredential with new auth lib #129

Merged
merged 3 commits into from Jul 2, 2019

Conversation

Projects
None yet
4 participants
@hstonec
Copy link
Collaborator

commented Jun 20, 2019

This change is Reviewable

@googlebot googlebot added the cla: yes label Jun 20, 2019

@hstonec hstonec force-pushed the hstonec:replace-google-credential branch from e7e5ff4 to d63b8a7 Jun 21, 2019

@hstonec hstonec requested review from weiminyu and CydeWeys Jun 21, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @hstonec, and @weiminyu)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 114 at r1 (raw file):

  }

  /** Access token supplier that auto refreshes 1 minute before expiry. */

The reordering in this file is making the diff go crazy. Is there some reason all this reordering was necessary?

@weiminyu
Copy link
Collaborator

left a comment

Could you include test evidence for each type of credentials?
Try deploy your build to alpha, the test the following:

  • For default credential, start a datastore backup and check in bigquery
  • For json credential, try syncRegistrarSheetAction
  • For delegated credential, try syncGroupMemberAction
  • For local credential, try login using your locally built nomulus tool.

Reviewed 5 of 24 files at r1.
Reviewable status: 5 of 24 files reviewed, 2 unresolved discussions (waiting on @hstonec and @weiminyu)


core/src/main/java/google/registry/export/sheet/SheetsServiceModule.java, line 34 at r1 (raw file):

      @JsonCredential GoogleCredentials credential, @Config("projectId") String projectId) {
    return new Sheets.Builder(
            Utils.getDefaultTransport(),

Original code uses GoogleNetHttpTransport.newTrustedTransport(), which activates SSL.
Would this change work?

@weiminyu
Copy link
Collaborator

left a comment

For JsonCredential, you also need to check if appengine can connect to StackDriver. If credential is bad, exception will appear in the AppEngine log.

Reviewable status: 5 of 24 files reviewed, 2 unresolved discussions (waiting on @hstonec)

@hstonec hstonec force-pushed the hstonec:replace-google-credential branch from d63b8a7 to 7973372 Jun 26, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 4 of 25 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @hstonec)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 114 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The reordering in this file is making the diff go crazy. Is there some reason all this reordering was necessary?

It seems google-java-format did this. I can try to revert it and applied the change manually without formatting it.

@hstonec
Copy link
Collaborator Author

left a comment

Please see the test result below:

For default credential, start a datastore backup and check in bigquery

  • I just started the export job, will update there once it is done.

For json credential, try syncRegistrarSheetAction

For delegated credential, try syncGroupMemberAction

For local credential, try login using your locally built nomulus tool.

  • Tested with the nomulus.jar built from ./gradlew :core:nomulus, logged out first then logged in, and used the command to deploy a spec11 pipeline to alpha to make sure the login worked
  • https://paste.googleplex.com/6315346849955840

For JsonCredential, you also need to check if appengine can connect to StackDriver. If credential is bad, exception will appear in the AppEngine log.

Reviewable status: 4 of 25 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @hstonec)

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 4 of 25 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


core/src/main/java/google/registry/export/sheet/SheetsServiceModule.java, line 34 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Original code uses GoogleNetHttpTransport.newTrustedTransport(), which activates SSL.
Would this change work?

Tested the SyncRegistrarsSheetAction which uses this service. So I think this should work.

@hstonec hstonec force-pushed the hstonec:replace-google-credential branch from 7973372 to c6d186f Jun 27, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 4 of 23 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 114 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

It seems google-java-format did this. I can try to revert it and applied the change manually without formatting it.

Done.

@hstonec
Copy link
Collaborator Author

left a comment

For default credential, start a datastore backup and check in bigquery

I just started the export job, will update there once it is done.

The datastore backup succeeded https://paste.googleplex.com/5393657202999296 . I executed some simple sql to query against the backup and didn't find any issue.

Reviewable status: 4 of 23 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)

@weiminyu
Copy link
Collaborator

left a comment

Reviewable status: 4 of 23 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)


core/src/main/java/google/registry/bigquery/BigqueryModule.java, line 43 at r2 (raw file):

            Utils.getDefaultTransport(),
            Utils.getDefaultJsonFactory(),
            new HttpCredentialsAdapter(credential))

We should either defined a class that carries all three of them, or inject transport and json factory.

@hstonec hstonec force-pushed the hstonec:replace-google-credential branch from 45aaab4 to 490e01e Jun 28, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 29 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @weiminyu)


core/src/main/java/google/registry/bigquery/BigqueryModule.java, line 43 at r2 (raw file):

Previously, weiminyu (Weimin Yu) wrote…
            Utils.getDefaultTransport(),
            Utils.getDefaultJsonFactory(),
            new HttpCredentialsAdapter(credential))

We should either defined a class that carries all three of them, or inject transport and json factory.

Done.

@weiminyu
Copy link
Collaborator

left a comment

Reviewed 3 of 24 files at r1, 1 of 3 files at r2, 25 of 25 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @weiminyu)

@hstonec hstonec requested a review from CydeWeys Jul 1, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hstonec and @weiminyu)


proxy/src/main/java/google/registry/proxy/ProxyModule.java, line 114 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Done.

In the future, just use Ctrl-Shift-F in IntelliJ after selecting only the lines you want to format. That'll keep diffs cleaner and make reviews easier.

google-java-format really shouldn't be reordering functions anyway. I've never seen that.

@CydeWeys
Copy link
Member

left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @weiminyu)

@hstonec hstonec merged commit 6daf72a into google:master Jul 2, 2019

3 of 4 checks passed

code-review/reviewable 2 discussions left (weiminyu)
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro Kokoro build finished
Details

CydeWeys added a commit to CydeWeys/nomulus that referenced this pull request Jul 19, 2019

Replace deprecated GoogleCredential with new auth lib (google#129)
Replace deprecated GoogleCredential with new lib

This PR also introduced a CredentialsBundle class to carry
HttpTransport and JsonFactory object which are needed by
most of the GCP library to instantiate client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.