-
Notifications
You must be signed in to change notification settings - Fork 209
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
Remove Old JWT Tables #6302
base: main
Are you sure you want to change the base?
Remove Old JWT Tables #6302
Conversation
@maxtropets please review. @sidmore because this change requires user-action, can you add an entry in the CHANGELOG to describe it? There seems to be an OpenAPI change (not immediately clear why), so please run the schema test locally and diff the schemas. If the change is expected, updating the OpenAPI version in the governance frontend file, rebuilding, re-running the test and checking in the schema is the way to go. If the change is unexpected, let's discuss. |
tests/infra/consortium.py
Outdated
@@ -875,3 +875,10 @@ def check_for_service(self, remote_node, status, recovery_count=None): | |||
assert ( | |||
recovery_count is None or current_recovery_count == recovery_count | |||
), f"Current recovery count {current_recovery_count} is not expected {recovery_count}" | |||
|
|||
def remove_old_jwt_tables(self, remote_node, issuer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're not actually calling this anywhere. We should add a call to this in the lts_compatibility
test (or one of the tests in recovery.py
that recovers from an old ledger), to fully confirm that we can have a service where these tables are present before the call, removed after the call, and not repopulated if we do a jwt refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We could also try make a simple new test like that
- populate old tables
- try out proposal
- make sure old tables are gone
You can look up tests in jwt_test.py
and custom_authorization.py
We also probably shall prepare some template for operators (not sure where to put it though, probably @achamayou could elaborate on our vision here), The goal is to have an ultimate step-by-step guide for how to upgrade, probably introduce some scripts to automate some work. I'm not really sure what's the average operator expertise level is. |
Co-authored-by: Eddy Ashton <ashton.eddy@gmail.com>
@achamayou i have updated the schema for gov |
} | ||
}, | ||
function (args) { | ||
// Clear the JWT public signing key table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following comment seem redundant to me
tests/infra/consortium.py
Outdated
@@ -875,3 +875,10 @@ def check_for_service(self, remote_node, status, recovery_count=None): | |||
assert ( | |||
recovery_count is None or current_recovery_count == recovery_count | |||
), f"Current recovery count {current_recovery_count} is not expected {recovery_count}" | |||
|
|||
def remove_old_jwt_tables(self, remote_node, issuer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. We could also try make a simple new test like that
- populate old tables
- try out proposal
- make sure old tables are gone
You can look up tests in jwt_test.py
and custom_authorization.py
@@ -528,6 +528,93 @@ def test_share_resilience(network, args, from_snapshot=False): | |||
recovered_network.service_load.set_network(recovered_network) | |||
return recovered_network | |||
|
|||
@reqs.description("Remove JWT Tables") | |||
def test_recovered_ledger_remove_jwt_tables( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the test. Could we, however:
- remove unnecessary parts except the required initialisation stuff
- probably move the common part (with
test_recover_service_from_files
) to a separate function
As is, it's a copy-pasted test_recover_service_from_files
with added JWT logic at the end with redundant (to the JWT logic) recovery checks, which doesn't make a lot of sense.
@@ -110,6 +110,7 @@ | |||
# recovery: | |||
recovery.test_recover_service, | |||
recovery.test_recover_service_aborted, | |||
recovery.test_recovered_ledger_remove_jwt_tables, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried this? It seems like it can't work, as the new test's parameters are not compatible with this test suite.
To make it work, I assume you should've added that test to the recovery.py
, along with the test you copied the initialisation from.
function (args) { | ||
// Clear the JWT public signing key table | ||
ccf.kv["public:ccf.gov.jwt.public_signing_key"].clear(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a trigger snapshot action
|
||
# Submit Proposal to remove old JWT Tables | ||
network.consortium.remove_old_jwt_tables(primary) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stop the network here to trigger a flush to disk
#6222
Remove