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: DB migration fails with AWS RDS MySQL 5.7 #770

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

peterhaochen47
Copy link
Member

@peterhaochen47 peterhaochen47 commented 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: fix: Orphaned encrypted_value when Credentials are deleted #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.85 as of now), which contains the "DROP TRIGGER" step). For these users, we will publish docs on how to fix it (manually drop the triggers).

[#187774971]

- 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 peterhaochen47 marked this pull request as ready for review August 6, 2024 20:05
@hsinn0
Copy link
Contributor

hsinn0 commented Aug 6, 2024

Not much to review for actual code change, so that is OK.
Question:

will publish docs on how to fix it (manually drop the triggers)

How/where are we going to publish it?

@hsinn0 hsinn0 self-requested a review August 6, 2024 20:43
@peterhaochen47
Copy link
Member Author

peterhaochen47 commented Aug 6, 2024

@hsinn0
I was thinking just in the release note, something like:

If you are upgrading to this version directly from CredHub 2.12.67 and if you are using MySQL with CredHub, you will need to manually run the following commands in your database before the upgrade :

DROP TRIGGER IF EXISTS tr_credential_version_pre_del;
DROP TRIGGER IF EXISTS tr_credential_version_post_del;
DROP TRIGGER IF EXISTS tr_credential_pre_del;

I think release note will be good enough given how narrow this IF case is.

@peterhaochen47 peterhaochen47 merged commit 83fbb21 into main Aug 6, 2024
2 checks passed
@hsinn0 hsinn0 deleted the pr/main/fix-drop-trigger-migration-rds-mysql5 branch August 9, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants