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

feat: enable authentication #174

Merged
merged 5 commits into from
Jun 13, 2022

Conversation

olavloite
Copy link
Collaborator

Enables authentication on PGAdapter so the client can supply the credentials for a specific connection, instead of setting a fixed set of credentials on PGAdapter at startup. This also enables connecting to different Google Cloud projects and instances. The database property in the connection parameters may be a simple database id (e.g. my-database) or a fully qualified database name (e.g. projects/my-project/instances/my-instance/databases/my-database).

The credentials must be one of the following:

  1. The private key string of a service account. This string must include the -----BEGIN PRIVATE KEY----- and -----END PRIVATE KEY----- headers. The username must be the email address of the service account.
  2. The JSON contents of a Google credentials file. This could be a service account file or a user account file. The credentials file is sent in the password message. The username is ignored.

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #174 (3fd7251) into postgresql-dialect (e3016d1) will increase coverage by 0.67%.
The diff coverage is 81.63%.

@@                   Coverage Diff                    @@
##             postgresql-dialect     #174      +/-   ##
========================================================
+ Coverage                 78.87%   79.55%   +0.67%     
- Complexity                 1011     1043      +32     
========================================================
  Files                        89       91       +2     
  Lines                      3494     3560      +66     
  Branches                    406      422      +16     
========================================================
+ Hits                       2756     2832      +76     
+ Misses                      578      566      -12     
- Partials                    160      162       +2     
Flag Coverage Δ
all_tests 79.55% <81.63%> (+0.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ud/spanner/connection/ConnectionOptionsHelper.java 50.00% <50.00%> (ø)
...panner/pgadapter/wireprotocol/PasswordMessage.java 58.97% <58.62%> (+58.97%) ⬆️
...utput/AuthenticationCleartextPasswordResponse.java 71.42% <71.42%> (ø)
...ud/spanner/pgadapter/metadata/OptionsMetadata.java 84.93% <92.10%> (+4.20%) ⬆️
...gle/cloud/spanner/pgadapter/ConnectionHandler.java 76.52% <100.00%> (+0.08%) ⬆️
...ud/spanner/pgadapter/wireoutput/ErrorResponse.java 94.04% <100.00%> (+0.14%) ⬆️
...spanner/pgadapter/wireprotocol/StartupMessage.java 85.36% <100.00%> (+19.57%) ⬆️
... and 1 more

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 e3016d1...3fd7251. Read the comment docs.

@olavloite olavloite marked this pull request as ready for review June 10, 2022 14:35
Copy link

@aseering aseering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guangylegend I wonder if this change to PGAdapter would be sufficient to enable running multiple Spangres tests against multiple databases (in the same local instance) using a single PGAdapter, like we just talked about earlier today?

Anyway -- I am by no means a qualified Java reviewer, but overall this change seems reasonable.

@olavloite
Copy link
Collaborator Author

olavloite commented Jun 12, 2022

@guangylegend I wonder if this change to PGAdapter would be sufficient to enable running multiple Spangres tests against multiple databases (in the same local instance) using a single PGAdapter, like we just talked about earlier today?

You actually don't need this change for that, as that is already supported. If you start PGAdapter without a -d (database) command line argument, PGAdapter will connect to the database that is given in the connection request. Note that this does mean that you must include a database in the connection request when you start for example psql.

This change will however also add support for connecting to different databases on different instances or even in different projects, as it will add support for fully-qualified database names in the connection request.

@olavloite olavloite merged commit 2e34c84 into postgresql-dialect Jun 13, 2022
@olavloite olavloite deleted the credentials-project-instance-optional branch June 13, 2022 13:57
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.

None yet

2 participants