feat(main): Add support for psycopg (Issue #214)

[UzielSilva]

See #214
Changelog:

• Add proxy for connections that can only be made through an unix socket, to support the TLS connection
• Add support for psycopg, using the proxy server
• Add unit and integration tests
• Update docs

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[hessjcg]

Thank you for the contribution! We are excited to add this to the code. I will give this careful review and get back in touch soon with some additional comments.

[hessjcg]

Nice work so far. This is going to be a very popular enhancement.

You may want to look at a similar local unix proxy implementation in the node connector.

There are a few things I'd like you to add to this PR. This will get it closer to production-worthy code:

1. The socket path should be a configuration parameter local_socket_path on connector.connect() instead of being hard-coded. This will allow an application to open many connectors to different database instances.
2. When the listener opens the unix socket, it should set the file permissions to be readable only by the user.
3. Handle exceptions from creating and binding the unix socket, log an appropriate error, and raise a LocalProxyStartupError. Define this new exception in google/cloud/sql/connector/exceptions.py
4. Consider using asyncio instead of a thread to copy data to and from the proxy.
5. When the connector is closed, the local_communication loop must be stopped and the socket file removed.

Again, I'm grateful that you put in the time and wrote this feature. Thank you.

[enocom]

cc @rhatgadkar-goog who owns the AlloyDB Python Connector and will be interested to see this.

Big +1 on what @hessjcg. This will be a huge win for people wanting to use psycopg and Cloud SQL (or AlloyDB).

[UzielSilva]

All your comments were addressed on my last commit, I even improve the integration test to use SQLAlchemy like other tests.
I only have this issue when I ran the integration test:

Task was destroyed but it is pending!
task: <Task pending name='Task-12' coro=<local_communication() done, defined at /home/uziel/Enterprises/TSA/cloud-sql-python-connector/google/cloud/sql/connector/proxy.py:72> wait_for=<Future pending cb=[BaseSelectorEventLoop._sock_read_done(20, handle=<Handle BaseS...0>, 10485760)>)(), Task.task_wakeup()]>>

Not sure how to handle or if it is expected, this is caused because the local_communication is still expecting your input when you close the connection. Not sure if there is a more friendly way to stop it, but oh well.
Let me know if this is good enough for production.

[UzielSilva]

FYI: The last issue I mentioned is only a warning, so, actually, it is not affecting the feature.

[rhatgadkar-goog]

All your comments were addressed on my last commit, I even improve the integration test to use SQLAlchemy like other tests. I only have this issue when I ran the integration test:

Task was destroyed but it is pending!
task: <Task pending name='Task-12' coro=<local_communication() done, defined at /home/uziel/Enterprises/TSA/cloud-sql-python-connector/google/cloud/sql/connector/proxy.py:72> wait_for=<Future pending cb=[BaseSelectorEventLoop._sock_read_done(20, handle=<Handle BaseS...0>, 10485760)>)(), Task.task_wakeup()]>>

Not sure how to handle or if it is expected, this is caused because the local_communication is still expecting your input when you close the connection. Not sure if there is a more friendly way to stop it, but oh well. Let me know if this is good enough for production.

Thank you for this PR of adding support for psycopg! We really appreciate this change.

I took a look at this StackOverflow post to help understand what might be happening here: https://stackoverflow.com/questions/71542947/how-can-i-fix-task-was-destroyed-but-it-is-pending.

I think you're getting the error, because when the connector closes, you're not cancelling the local proxy server task and waiting for it to shutdown. So maybe you can try making the following changes:

• Modify start_local_proxy() function to return the created asyncio task and save this as a member variable in the Connector object.
• And then in the Connector class's close_async() function, you can cancel the proxy server's asyncio task and gracefully wait for it to cancel. RefreshAheadCache's close() function might be a good example for how to do this.

[UzielSilva]

@rhatgadkar-goog It worked!, now the warning is gone and the server is being stopped gracefully. Thanks!

So, I think the PR is ready for a review, let me know if you want me to do some other modifications. :)

[kgala2]

/gcbrun

[UzielSilva]

Also fixed a unit test, test coverage is restored to 100%

[kgala2]

/gcbrun

[UzielSilva]

Trying to add tests for the proxy server, to increase code coverage.

[UzielSilva]

Tests were added, code coverage is now at 94%

[kgala2]

/gcbrun

[UzielSilva]

Huh, do I need to also keep support for python 3.9? let me check then the fixes I need to do for it. :)

[UzielSilva]

Added support for Python 3.9, only needed to add some exceptions to handle when I close the server

[kgala2]

/gcbrun

[UzielSilva]

Still not able to merge it, it says changes requested

[enocom]

Only maintainers can merge.

@kgala2 will have to merge.

Great work on this @UzielSilva 👏.

[kgala2]

Hi @UzielSilva,

The overall PR looks good, I am checking for corner cases that might not be covered, I'll comment on this PR if I find anything and have psyopg support available as soon as possible.
cc: @hessjcg

Thanks