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

Remove Old JWT Tables #6302

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

sidmore
Copy link
Contributor

@sidmore sidmore commented Jun 27, 2024

#6222

Remove

  1. References to Legacy tables
  2. Add Remove Old JWT table proposal

@achamayou
Copy link
Member

@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.

@@ -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):
Copy link
Member

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.

Copy link
Contributor

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

@maxtropets
Copy link
Contributor

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.

@sidmore sidmore marked this pull request as ready for review July 2, 2024 16:19
@sidmore sidmore requested a review from a team as a code owner July 2, 2024 16:19
@sidmore
Copy link
Contributor Author

sidmore commented Jul 2, 2024

@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.

@achamayou i have updated the schema for gov

}
},
function (args) {
// Clear the JWT public signing key table
Copy link
Contributor

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

@@ -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):
Copy link
Contributor

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(
Copy link
Contributor

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,
Copy link
Contributor

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();

Copy link
Contributor Author

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)

Copy link
Contributor Author

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

@achamayou
Copy link
Member

@sidmore now that release/5.x is cut, it'd be great to update this and get it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants