feat: BCE-11514 - scope web3signer key sync to one cluster and prune stale keystores - #38
Open
kenrick-g wants to merge 3 commits into
Open
feat: BCE-11514 - scope web3signer key sync to one cluster and prune stale keystores#38kenrick-g wants to merge 3 commits into
kenrick-g wants to merge 3 commits into
Conversation
sync-web3signer-keys is about to be pointed at a keystore table that holds several clusters' keys, so it needs to read only its own cluster's rows. fetch_keys previously ran SELECT * and mapped columns by ordinal. That is correct for the five-column agent-managed table but not for a six-column table, where positions 3 and 4 hold different data. It now names the three columns this command actually uses, which is correct against either shape, and returns a narrower record type. Naming all five columns would not work, because the six-column table has no validator_index or fee_recipient. An optional client_cluster_id predicate restricts the read, bound as a parameter. Selection fails closed: if the table carries a client_cluster_id column and no cluster was named, the command refuses to run unless --all-clusters is passed explicitly. Tables without that column, including the existing default, keep working with no flags. Detection resolves the table through search_path so a same-named table in another schema cannot answer for it, and aborts rather than falling through to an unfiltered read. An empty or whitespace cluster id is rejected instead of disabling the filter. Keystores left by a previous, larger key set are now removed. web3signer loads every YAML in the output directory, so without this a run returning fewer keys keeps serving the surplus. Files are written first and pruned after, so the directory is never empty. Tests: 62 pass, up from 52, including real-PostgreSQL coverage of schema resolution behind SYNC_KEYS_TEST_DSN. Verified end to end against PostgreSQL 16 with the real CLI and genuine AES-EAX ciphertext.
CI runs pre-commit with --all-files. These are the formatter and mypy fixes for the files this branch touches: ruff-format normalisation, and client_cluster_id typed Optional[str] rather than an implicit optional, which also needed the import that its annotation refers to.
Review found the prune was scoped too broadly and the zero-row case unsafe. The prune globbed *.yaml and deleted every basename outside the generated set, so an unrelated file in the output directory would have been removed. It now only considers files matching key_<n>.yaml, which is what this command generates; anything else is left alone. The keep-set is built once rather than per file. A zero-row result previously wrote nothing and then pruned everything, leaving the directory empty and reporting success. For a signer that is already serving, that means silently ceasing to sign. It now fails without touching the directory, since zero rows almost always means a wrong cluster id or an unpopulated table. Adds CLI-level tests through the click entrypoint, which is where these safeguards live: mutually exclusive flags, omitted cluster on scoped versus legacy tables, the all-clusters override, empty id, stale keystore removal, unrelated-file preservation, and the zero-row refusal. The preservation and zero-row tests both fail against the previous implementation. Also rejects a schema-qualified table_name rather than half-supporting it: to_regclass would resolve "schema.table" while the queries quote table_name as a single identifier, so detection could succeed and the fetch then fail with UndefinedTable. And types the test DSN as str so mypy is clean.
This file contains hidden or 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
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.
sync-web3signer-keysis about to be pointed at a keystore table that holds several clusters' keys, so it needs to read only the rows belonging to its own cluster.Column selection
fetch_keysranSELECT *and mapped columns by ordinal to(public_key, private_key, nonce, validator_index, fee_recipient). That is exact for the five-column agent-managed table, but a six-column table has different data in positions 3 and 4 — it does not fail, it mislabels.It now names the three columns this command actually uses and returns a narrower
Web3SignerKeyRecord. Three rather than five is deliberate: a six-column keystore table has novalidator_indexorfee_recipient, so naming all five errors withUndefinedColumn. Three works against either shape. No caller reads the two dropped fields.sync-dbandsync-validator-keysare untouched.Cluster selection, failing closed
--client-cluster-idrestricts the read, bound as a parameter.client_cluster_idcolumn and no cluster was named, the command refuses to run unless--all-clustersis passed deliberately. Tables without that column — including the current default — keep working with no flags, so existing deployments are unaffected.search_pathviato_regclass, so a same-named table in another schema cannot answer for it, and it aborts rather than falling through to an unfiltered read if the table cannot be resolved. This applies to unqualified table names; a schema-qualifiedtable_nameis now rejected outright, becauseto_regclasswould resolve it while the queries quote it as a single identifier.--client-cluster-idis rejected instead of silently disabling the filter.Keystore reconciliation
The command wrote
key_{index}.yamlbut never removed leftovers, and web3signer loads every YAML in the output directory. A run returning fewer keys than the previous one therefore kept serving the surplus — so cluster selection on its own would not have changed what an already-running signer serves.key_<n>.yaml, which is what this command generates. Anything else in the directory is left alone.The write-then-prune ordering is safe because
fetch-keysruns as an init container, where a failure prevents startup rather than mutating a live signer's directory. If it is ever moved to a sidecar or given a periodic reconcile, it should use staging plus an atomic handoff instead.Testing
72 tests pass, up from 52.
tests/test_sync_web3signer_keys.py, 10) exercise the safeguards through the real click entrypoint, since that is where they live: mutually exclusive flags, omitted cluster on scoped versus legacy tables, the--all-clustersoverride, empty id, stale keystore removal, unrelated-file preservation, and the zero-row refusal. The preservation and zero-row tests both fail against the previous implementation.tests/test_database_schema_resolution.pyruns against real PostgreSQL behindSYNC_KEYS_TEST_DSN(skipped otherwise) and asserts the answer inverts when schema order onsearch_pathflips — a case mocks cannot catch.fetch_keystests were updated where they encoded the old behaviour: the cursor fixture went from a five-tuple to a three-tuple, andfee_recipientexpectations became field-absence expectations. Those are deliberate changes, not just patch-target moves.59 passed / 3 skippedwithout a database,72 passedwith one.Verified end to end against PostgreSQL 16 with the real CLI and genuine AES-EAX ciphertext: the filtered run emits exactly the expected keys, an unfiltered run over the same data emits the superset, a wrong decryption key emits nothing, and a directory left over from a larger run is pruned to the correct set.
CI note
Lint & Formatfails on pre-existing debt unrelated to this branch:ruff formatwants to reformattests/test_sync_validator_keys.py, andmypyreports three errors insync_keys/encoder.py. Both are untouched here and both fail onmaintoday. Every file this branch touches is ruff-clean and mypy-clean. Suggest a separatechore:PR for that debt rather than mixing it in here.(Supersedes #37, which was committed from a temporary working tree whose untouched files had been removed by the OS tmp reaper, so
git add -Astaged unintended deletions.)