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

Fix MostRecentProvider.encryption_materials() caching bug #102

Merged
merged 8 commits into from
Jan 14, 2019
Merged

Fix MostRecentProvider.encryption_materials() caching bug #102

merged 8 commits into from
Jan 14, 2019

Conversation

mattsb42-aws
Copy link
Member

Description of changes:
A customer encountered a bug where MostRecentProvider was not actually using cached results. Upon investigation, this uncovered this bug where the provider was returning invalid results once the cache was populated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mattsb42-aws mattsb42-aws requested a review from a team January 11, 2019 22:07
@johnwalker
Copy link
Contributor

Working on understanding the fix here. It seems that the original logic was this: check if ttl_action is TtlActions.LIVE, and if so return the given provider from the cache. Also, in this case return a provider instead of encryption materials like in the other cases.

If it failed, set the ttl_action to EXPIRED and pull the provider from the local cache if thats allowed. If it's not allowed, miss the cache.

Is it the case that this fixes the return types of the method to be consistent? And then, was there a problem with the cache returning None for its values?

@mattsb42-aws
Copy link
Member Author

Is it the case that this fixes the return types of the method to be consistent?

Yes. What used to happen was that the return type from encryption_materials() would be different if there was cache miss (EncryptionMaterials) than if there was a cache hit (CryptographicMaterialsProvider).

This test[1] failing was what identified this issue.

[1] https://github.com/aws/aws-dynamodb-encryption-python/pull/102/files#diff-6420d750c8994c892392e8d4caaa35fbR81

Copy link
Contributor

@johnwalker johnwalker left a comment

Choose a reason for hiding this comment

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

Thanks! Good find

@mattsb42-aws mattsb42-aws merged commit c8e4059 into aws:master Jan 14, 2019
@mattsb42-aws mattsb42-aws deleted the metastore branch January 14, 2019 21:01
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.

2 participants