refactor: move SSL/TLS context creation to ConnectionInfo #1079
Conversation
| # check that refresh is valid | ||
| if not await _is_valid(refresh_task): | ||
| raise RefreshNotValidError( | ||
| f"['{self._instance_connection_string}']: Invalid refresh operation. Certficate appears to be expired." | ||
| ) |
There was a problem hiding this comment.
we do this in AlloyDB: https://github.com/GoogleCloudPlatform/alloydb-python-connector/blob/bb5d107b801e713b4e94c8dc644e6950717704cb/google/cloud/alloydb/connector/instance.py#L196-L199
This check fixed a test that was passing due to bad legacy test infra.
| class BadRefresh(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class MockMetadata(ConnectionInfo): | ||
| """Mock class for ConnectionInfo""" | ||
|
|
||
| def __init__( | ||
| self, expiration: datetime.datetime, ip_addrs: Dict = {"PRIMARY": "0.0.0.0"} | ||
| ) -> None: | ||
| self.expiration = expiration | ||
| self.ip_addrs = ip_addrs | ||
|
|
||
|
|
||
| async def instance_metadata_success(*args: Any, **kwargs: Any) -> MockMetadata: | ||
| return MockMetadata( | ||
| datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(minutes=10) | ||
| ) | ||
|
|
||
|
|
||
| async def instance_metadata_expired(*args: Any, **kwargs: Any) -> MockMetadata: | ||
| return MockMetadata( | ||
| datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta(minutes=10) | ||
| ) | ||
|
|
||
|
|
||
| async def instance_metadata_error(*args: Any, **kwargs: Any) -> None: | ||
| raise BadRefresh("something went wrong...") | ||
|
|
||
|
|
There was a problem hiding this comment.
this legacy mock was hiding a bad test, removed it and now our tests properly rely on our internals.
| return await connector(ip_address, instance_data.context, **kwargs) | ||
| return await connector( | ||
| ip_address, | ||
| instance_data.create_ssl_context(enable_iam_auth), |
There was a problem hiding this comment.
i guess the downside of this is now we are creating an SSL/TLS context per connection vs in the past where we were creating it once per ConnectionInfo class...
There was a problem hiding this comment.
Is there a significant cost associated with creating an SSL context per connection? What would a Python developer expect?
| return await connector(ip_address, instance_data.context, **kwargs) | ||
| return await connector( | ||
| ip_address, | ||
| instance_data.create_ssl_context(enable_iam_auth), |
There was a problem hiding this comment.
Is there a significant cost associated with creating an SSL context per connection? What would a Python developer expect?
| @@ -79,33 +81,31 @@ def _from_str(cls, ip_type_str: str) -> IPTypes: | |||
| return cls(ip_type_str.upper()) | |||
|
|
|||
|
|
|||
| @dataclass | |||
|
Related to #805 |
This refactor moves all SSL/TLS configuration into its own method
create_ssl_context()on theConnectionInfoclass.This way
ConnectionInfo()prepares all the info required to connect and thencreate_ssl_contextwill use the info to establish the SSL/TLS connection when called fromConnector.connectat the time of connection.This refactor also improves the testing infra to be more in line with AlloyDB Python Connector.
Based on Go: GoogleCloudPlatform/cloud-sql-go-connector#761