fix: Orphaned encrypted_value when Credentials are deleted #728
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
- Fixed #231 in JPA layer instead of using database triggers. - Used `LAZY` fetch for the JPA one-to-many mapping from `credential` to `crdential_version` because `EAGER` causes memory and perfromance issue when many credentials are loaded, e.g. `DefaultCertificatesHandler.handleGetAllRequest()`. The issue with `EAGER` fetch was reproducible in `DefaultCertificatesHandlerIntegrationTest`. - Made `DefaultCertificatesHandlerIntegrationTest` transactional so as not to commit test data. This not only makes sure no dirty test data is left but also makes the test runs a little faster. - Changed the `credetial_version.type` of test data in `DefaultCertificatesHandlerIntegrationTest` to a valid value 'cert' as previous invalid type value caused data vdalidation failure by Hibernate. - Changed properties of `CredentialVersionData` entity to be non-final, i.e. added Kotlin `open` keyword to them, so the lazy loading can work properly. This takes care of following exception for example: ``` WARN org.hibernate.tuple.entity.PojoEntityTuplizer - HHH000305: Could not create proxy factory for:org.cloudfoundry.credhub.entity.CredentialVersionData org.hibernate.HibernateException: Getter methods of lazy classes cannot be final: org.cloudfoundry.credhub.entity.CredentialVersionData#getUuid ``` [#187367323]
|
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story. The labels on this github issue will be updated when the story is started. |
bruce-ricard
approved these changes
Apr 17, 2024
peterhaochen47
added a commit
that referenced
this pull request
Jul 30, 2024
- issue: the "DROP TRIGGER" step of our MySQL DB migration fails when used with AWS RDS MySQL 5.7 due to: ``` (conn=127) You do not have the SUPER privilege and binary logging is enabled ``` - We decided that the best approach is to just remove this migration step, even though modifying past migrations is not recommended by flyway (but in the past, in our context removing migration steps have not created any problems). - Note: This removal comes with a small problem where the triggers in question are not removed (because we do want these triggers to be removed, since they are not compatible with our latest code in terms of DB cleanup, see: #728). However, this problematic scenario should be quite narrow (only for users who have successfully installed CredHub 2.12.67, which adds the triggers, and have never upgraded to any versions between 2.12.69 and the latest current version (2.12.84), which contains the "DROP TRIGGER" step). For these users, we will publish docs on how to fix it (manually drop the triggers). [#187774971]
peterhaochen47
added a commit
that referenced
this pull request
Aug 6, 2024
- issue: the "DROP TRIGGER" step of our MySQL DB migration fails when used with AWS RDS MySQL 5.7 due to: ``` (conn=127) You do not have the SUPER privilege and binary logging is enabled ``` - We decided that the best approach is to just remove this migration step, even though modifying past migrations is not recommended by flyway (but in the past, in our context removing migration steps have not created any problems). - Note: This removal comes with a small problem where the triggers in question are not removed (because we do want these triggers to be removed, since they are not compatible with our latest code in terms of DB cleanup, see: #728). However, this problematic scenario should be quite narrow (only for users who have successfully installed CredHub 2.12.67, which adds the triggers, and have never upgraded to any versions between 2.12.69 and the latest current version (2.12.84), which contains the "DROP TRIGGER" step). For these users, we will publish docs on how to fix it (manually drop the triggers). [#187774971]
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.
encrypted_valuetable #231 in JPA layer instead of using database triggers.LAZYfetch for the JPA one-to-many mapping fromcredentialtocrdential_versionbecauseEAGERcauses memory and perfromance issue when many credentials are loaded, e.g.DefaultCertificatesHandler.handleGetAllRequest(). The issue withEAGERfetch was reproducible inDefaultCertificatesHandlerIntegrationTest.DefaultCertificatesHandlerIntegrationTesttransactional so as not to commit test data. This not only makes sure no dirty test data is left but also makes the test runs a little faster.credetial_version.typeof test data inDefaultCertificatesHandlerIntegrationTestto a valid value 'cert' as previous invalid type value caused data vdalidation failure by Hibernate.CredentialVersionDataentity to be non-final, i.e. added Kotlinopenkeyword to them, so the lazy loading can work properly. This takes care of following exception for example:[#187367323]