Remove memoization to accept key_provider overridden by with_encryption_context
#51019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is necessary to allow key_provider to be changed dynamically using with_encryption_context. Co-authored-by: Kyle Stevens <kstevens715@gmail.com>
jorgemanrubia
approved these changes
Feb 9, 2024
jhawthorn
added a commit
that referenced
this pull request
Feb 13, 2024
Remove memoization to accept `key_provider` overridden by `with_encryption_context`
|
Backported to |
rosa
added a commit
to rosa/rails
that referenced
this pull request
Mar 13, 2024
The memoization of Scheme#key_provider was removed completely in rails#51019 because it prevented overriding the `key_provider` via `with_encryption_context`. However, this also made our tests for the HEY app about 5 times slower. We traced this down to all attributes where we either provide a key or declare them as deterministic and have to derive a key to instantiate the provider every time we load them. This might happen hundreds of times per test, and ultimately, we call ``` ActiveSupport::KeyGenerator#generate_key ``` and ``` OpenSSL::KDF.pbkdf2_hmac ``` hundreds of times. This adds significant overhead per test. In reality, what's overridden bv `with_encryption_context` is the value used as default provider, that is, `ActiveRecord::Encryption.key_provider`. This is only used in `Scheme#key_provider` if the scheme doesn't already have either a `key_provider` passed directly, or a `key`, or is `deterministic`. The `key_provider` passed directly is already memoized simply by having it stored as is. Let's memoize the other two, in order, so we save all those extra calls to derive the same keys again and again.
4 tasks
rosa
added a commit
to rosa/rails
that referenced
this pull request
Mar 13, 2024
The memoization of Scheme#key_provider was removed completely in rails#51019 because it prevented overriding the `key_provider` via `with_encryption_context`. However, this also made our tests for the HEY app about 5 times slower. We traced this down to all attributes where we either provide a key or declare them as deterministic and have to derive a key to instantiate the provider every time we load them. This might happen hundreds of times per test, and ultimately, we call ``` ActiveSupport::KeyGenerator#generate_key ``` and ``` OpenSSL::KDF.pbkdf2_hmac ``` hundreds of times. This adds significant overhead per test. In reality, what's overridden bv `with_encryption_context` is the value used as default provider, that is, `ActiveRecord::Encryption.key_provider`. This is only used in `Scheme#key_provider` if the scheme doesn't already have either a `key_provider` passed directly, or a `key`, or is `deterministic`. The `key_provider` passed directly is already memoized simply by having it stored as is. Let's memoize the other two, in order, so we save all those extra calls to derive the same keys again and again.
viralpraxis
pushed a commit
to viralpraxis/rails
that referenced
this pull request
Mar 24, 2024
The memoization of Scheme#key_provider was removed completely in rails#51019 because it prevented overriding the `key_provider` via `with_encryption_context`. However, this also made our tests for the HEY app about 5 times slower. We traced this down to all attributes where we either provide a key or declare them as deterministic and have to derive a key to instantiate the provider every time we load them. This might happen hundreds of times per test, and ultimately, we call ``` ActiveSupport::KeyGenerator#generate_key ``` and ``` OpenSSL::KDF.pbkdf2_hmac ``` hundreds of times. This adds significant overhead per test. In reality, what's overridden bv `with_encryption_context` is the value used as default provider, that is, `ActiveRecord::Encryption.key_provider`. This is only used in `Scheme#key_provider` if the scheme doesn't already have either a `key_provider` passed directly, or a `key`, or is `deterministic`. The `key_provider` passed directly is already memoized simply by having it stored as is. Let's memoize the other two, in order, so we save all those extra calls to derive the same keys again and again.
fractaledmind
pushed a commit
to fractaledmind/rails
that referenced
this pull request
May 13, 2024
The memoization of Scheme#key_provider was removed completely in rails#51019 because it prevented overriding the `key_provider` via `with_encryption_context`. However, this also made our tests for the HEY app about 5 times slower. We traced this down to all attributes where we either provide a key or declare them as deterministic and have to derive a key to instantiate the provider every time we load them. This might happen hundreds of times per test, and ultimately, we call ``` ActiveSupport::KeyGenerator#generate_key ``` and ``` OpenSSL::KDF.pbkdf2_hmac ``` hundreds of times. This adds significant overhead per test. In reality, what's overridden bv `with_encryption_context` is the value used as default provider, that is, `ActiveRecord::Encryption.key_provider`. This is only used in `Scheme#key_provider` if the scheme doesn't already have either a `key_provider` passed directly, or a `key`, or is `deterministic`. The `key_provider` passed directly is already memoized simply by having it stored as is. Let's memoize the other two, in order, so we save all those extra calls to derive the same keys again and again.
xjunior
pushed a commit
to xjunior/rails
that referenced
this pull request
Jun 9, 2024
The memoization of Scheme#key_provider was removed completely in rails#51019 because it prevented overriding the `key_provider` via `with_encryption_context`. However, this also made our tests for the HEY app about 5 times slower. We traced this down to all attributes where we either provide a key or declare them as deterministic and have to derive a key to instantiate the provider every time we load them. This might happen hundreds of times per test, and ultimately, we call ``` ActiveSupport::KeyGenerator#generate_key ``` and ``` OpenSSL::KDF.pbkdf2_hmac ``` hundreds of times. This adds significant overhead per test. In reality, what's overridden bv `with_encryption_context` is the value used as default provider, that is, `ActiveRecord::Encryption.key_provider`. This is only used in `Scheme#key_provider` if the scheme doesn't already have either a `key_provider` passed directly, or a `key`, or is `deterministic`. The `key_provider` passed directly is already memoized simply by having it stored as is. Let's memoize the other two, in order, so we save all those extra calls to derive the same keys again and again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Somewhat similar to #48923 (but with a different cause), as of 7.1 we weren't correctly observing
key_providerwhen overridden withwith_encryption_context. I believe this regressed in #44540 (though it feels possible in some cases there would have been a memoization problem beforehand).Reported by @kstevens715. Thank you!
I'll also backport this to 7-1-stable