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

Azure secp bulk loading #850

Merged
merged 32 commits into from
Jul 28, 2023
Merged

Conversation

jframe
Copy link
Contributor

@jframe jframe commented Jul 19, 2023

PR Description

Add bulk loading of secp keys to the eth1 mode of Web3Signer.

This adds the following new command line options to web3signer eth1 subcommand:

  • --azure-vault-enabled
  • --azure-vault-auth-mode
  • --azure-vault-name
  • --azure-client-id
  • --azure-tenant-id
  • --azure-client-secret
  • --azure-tags

Changes

  • Add mapKeyProperties to AzureKeyVault to enable processing of Azure keys
  • Created Azure bulk loader to iterate over the keys in the vault and create EthSecpArtifactSigner's
  • Remove ForkJoinPool hack and use an ExecutorService as the with problem blocking ForkJoin pool is also an issue with iterating over the Azure keys
  • Change usage of Azure code to use the AzureKeyVaultFactory instead of the static constructors of the AzureKeyVault so the ExecutorService can be reused and shutdown correctly
  • Add Azure bulk loading paremeters to the Eth1 config

Fixed Issue(s)

fixes #832

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

  • I thought about testing these changes in a realistic/non-local environment.

@jframe jframe added the doc-change-required Indicates an issue or PR that requires doc to be updated label Jul 24, 2023
@jframe jframe marked this pull request as ready for review July 24, 2023 06:43
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test coverage 👍
Just some nits really.

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +55 to +58
return executorServiceCache.updateAndGet(
e ->
Objects.requireNonNullElseGet(
e, () -> Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/musing: could potentially simplify by using a ConcurrentHashMap, but with the downside of a redundant key name. Don't think it's worth it, was just an interesting pattern to review...

return executorServiceCache.computeIfAbsent(
        "key",
        e -> Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()));

Copy link
Contributor

@gfukushima gfukushima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits, otherwise lgtm

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Runners are much more comprehensible now 👍

@jframe jframe enabled auto-merge (squash) July 28, 2023 04:02
@jframe jframe merged commit 09ca080 into Consensys:master Jul 28, 2023
2 checks passed
@jframe jframe deleted the azure_secp_bulk_loading branch July 28, 2023 04:21
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 2, 2023
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.

Eth secp Azure bulk loading
4 participants