-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3313 Cache AWS Credentials Where Possible #982
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
Conversation
test/auth_aws/test_auth_aws.py
Outdated
with MongoClient(self.uri) as client: | ||
client.get_database().test.find_one() | ||
pool = get_pool(client) | ||
pool.reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the get_pool/reset logic?
test/auth_aws/test_auth_aws.py
Outdated
pool.reset() | ||
with mock.patch.dict(os.environ, {}, clear=True): | ||
with MongoClient(self.uri) as client: | ||
client.get_database().test.find_one() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test supposed to fail without caching? If so it would be good to make that explicit by asserting that this fails initially:
# Auth fails without env variables.
with mock.patch.dict(os.environ, {}, clear=True):
with MongoClient(self.uri) as client:
with self.assertRaises(OperationFailure):
client.get_database().test.find_one()
# Auth succeeds and caches credential.
with MongoClient(self.uri) as client:
client.get_database().test.find_one()
# Auth succeeds with cached credential.
with mock.patch.dict(os.environ, {}, clear=True):
with MongoClient(self.uri) as client:
client.get_database().test.find_one()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be possible to test. We don't cache when the creds environment variables are given directly, which is why those tests are failing. We have no way of knowing if cached creds were used on EC2 without using requests-mock
which seems like too much of invasion into implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We at least need to test what we can. For example, the cache reset on auth failure logic. We can use failCommand to trigger a server side auth failure.
Would it be possible to use the connectionStatus command to determine which credential was authed?
>>> client.admin.command('connectionStatus')
{'authInfo': {'authenticatedUsers': [...]
Probably not I suppose, since I imagine different temp creds will map to the same database level user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a test that "poisons" the cache with invalid creds. The first auth attempt should fail and clear the cache, the next attempt should generate a new cred and succeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be updated to use the new set_use_cached_credentials api now: mongodb/pymongo-auth-aws#11
Done |
This is ready for review. The remaining failure is tracked by https://jira.mongodb.org/browse/PYTHON-3468 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM but could you add a changelog entry too?
Done |
No description provided.