feat: invalidate cache on bad connection info and failed IP lookup #389
Conversation
rhatgadkar-goog
changed the title
tests/unit/test_async_connector.py
Outdated
| connector._client = FakeAlloyDBClient( | ||
| instance=FakeInstance(name="bad-test-instance") | ||
| ) | ||
| cache = RefreshAheadCache(instance_uri, connector._client, connector._keys) |
There was a problem hiding this comment.
can we not just use the fake_client fixture for these tests?
alloydb-python-connector/tests/unit/conftest.py
Lines 42 to 43 in e682102
The connector should just use the cache's client, i don't think we need to override the connector._client
| connector._client = FakeAlloyDBClient( | |
| instance=FakeInstance(name="bad-test-instance") | |
| ) | |
| cache = RefreshAheadCache(instance_uri, connector._client, connector._keys) | |
| cache = RefreshAheadCache(instance_uri, fake_client, connector._keys) |
There was a problem hiding this comment.
From what I'm observing: if I change a member variable of a fixture object, it affects other test cases that are run in the session.
For example, if test_Connector_remove_cached_bad_instance is run right before test_connect in a test session, and if I do something like fake_client.instance.name = "bad-test-instace" in test_Connector_remove_cached_bad_instance, this will also be propagated to test_connect.
So that's why I'm not using the fixture here. Because I was seeing similar behavior when running nox -s unit-3.11.
There was a problem hiding this comment.
@rhatgadkar-goog you do not need to change any of the fake_client.instance attributes... since you do not need the bad-instance to be a FakeInstance it should give a 404 without needing it...
instance_uri = "projects/test-project/locations/test-region/clusters/test-cluster/instances/bad-test-instance"
async with AsyncConnector(credentials=credentials) as connector:
cache = RefreshAheadCache(instance_uri, fake_client, connector._keys)
connector._cache[instance_uri] = cache
with pytest.raises(ClientResponseError):
await connector.connect(instance_uri, "asyncpg")
assert instance_uri not in connector._cacheThere was a problem hiding this comment.
Ok, I did this for test_Connector_remove_cached_bad_instance. I think I still need to initialize a FakeAlloyDBClient for test_Connector_remove_cached_no_ip_type to override the instance's IP address and to not send an actual API request.
| """ | ||
| instance_uri = "projects/test-project/locations/test-region/clusters/test-cluster/instances/test-instance" | ||
| # set instance to only have Public IP | ||
| fake_client = FakeAlloyDBClient() |
There was a problem hiding this comment.
same here and below spots, can we not use the fake_client fixture?
rhatgadkar-goog
changed the title
This change is analogous to CloudSQL's GoogleCloudPlatform/cloud-sql-python-connector#1118.
The
Connectorcaches connection info for future connections and schedules refresh operations. For unrecoverable errors, the cache should be invalidated to stop future bad refreshes. The cache should also be invalidated on failed IP lookups. This PR does these things.