Remove Old JWT Tables #6302
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.
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.
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. |
@achamayou i have updated the schema for gov |
| } | ||
| }, | ||
| function (args) { | ||
| // Clear the JWT public signing key table |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
stop the network here to trigger a flush to disk
|
@sidmore now that |
#6222
Remove