feat(cli): add new migrate-secret-key command (#240)
#240
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
tiborsimko
reviewed
Nov 19, 2024
| @@ -612,3 +612,33 @@ def update_workflows_disk_quota() -> None: | |||
| for workflow in workflows: | |||
| store_workflow_disk_quota(workflow) | |||
| timer.count_event() | |||
|
|
|||
|
|
|||
| def change_key_encrypted_columns(old_key): | |||
There was a problem hiding this comment.
praise: I tested both scenarios (with a without the fix) and it works very nicely 👍
There was a problem hiding this comment.
suggestion: One observation about the smoothness of the operation for the site admin. I tested a situtaion when the admin does migrate the Invenio DB, but not the REANA DB, following the change of the secret key. In this case, the user cannot login via the web interface, and sees a nice message saying that something went wrong with the application. That's good. However, when the admin looks into reana-server logs, the admin only sees a Python traceback regrading SQLAlchemy padding error (see below). Could we catch the exception in the reana-server in order to provide a better phrased hint to the admin about what is happening?
$ k logs reana-server-568fcf5d48-kqvx2
...
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy/orm/loading.py", line 725, in _populate_full
dict_[key] = getter(row)
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy/sql/type_api.py", line 1278, in process
return process_value(impl_processor(value), dialect)
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy_utils/types/encrypted/encrypted_type.py", line 479, in process_result_value
value = super().process_result_value(value=value, dialect=dialect)
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy_utils/types/encrypted/encrypted_type.py", line 424, in process_result_value
decrypted_value = self.engine.decrypt(value)
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy_utils/types/encrypted/encrypted_type.py", line 123, in decrypt
decrypted = self.padding_engine.unpad(decrypted)
File "/usr/local/lib/python3.8/dist-packages/sqlalchemy_utils/types/encrypted/padding.py", line 44, in unpad
raise InvalidPaddingError()
sqlalchemy_utils.types.encrypted.padding.InvalidPaddingErrorThere was a problem hiding this comment.
I have pushed a new commit on top of reana-server/towards-release: reanahub/reana-server@d50c508
Alputer
added a commit
to Alputer/reana-db
that referenced
this pull request
Nov 20, 2024
tiborsimko
approved these changes
Nov 20, 2024
tiborsimko
added a commit
to tiborsimko/reana-db
that referenced
this pull request
Dec 4, 2024
chore(maint-0.9): release 0.9.5 (reanahub#219) feat(cli): add new `migrate-secret-key` command (reanahub#240) ci(python): test more Python versions (reanahub#239) ci(actions): pin setuptools 70 (reanahub#239) Note: The merge commit removes the changes related to pinning `setuptools` to version 70, because this was only necessary for the `maint-0.9` branches, as well as other 0.9.4 release-related changes.
tiborsimko
added a commit
to tiborsimko/reana-db
that referenced
this pull request
Dec 4, 2024
chore(maint-0.9): release 0.9.5 (reanahub#219) feat(cli): add new `migrate-secret-key` command (reanahub#240) ci(python): test more Python versions (reanahub#239) ci(actions): pin setuptools 70 (reanahub#239) Note: The merge commit removes the changes related to pinning `setuptools` to version 70, because this was only necessary for the `maint-0.9` branches, as well as other 0.9.4 release-related changes.
tiborsimko
added a commit
to tiborsimko/reana-db
that referenced
this pull request
Dec 4, 2024
chore(maint-0.9): release 0.9.5 (reanahub#219) feat(cli): add new `migrate-secret-key` command (reanahub#240) ci(python): test more Python versions (reanahub#239) ci(actions): pin setuptools 70 (reanahub#239) Note: The merge commit removes the changes related to pinning `setuptools` to version 70, because this was only necessary for the `maint-0.9` branches, as well as other 0.9.4 release-related changes.
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.
Closes reanahub/reana-server#695
How to test:
invenio instance migrate-secret-key --old-key CHANGE_MEkubectl rollout restart deployment reana-serverkubectl rollout restart deployment reana-workflow-controllerinvenio instance migrate-secret-key --old-key <old_key>andreana-db migrate-secret-key --old-key <old_key>(if you didn't set a secret key in (1) and (3), then the old secret key issecret_key)