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

Enhanced control over plugins registration #6700

Merged

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Mar 8, 2024

PR description

This PR introduces improvements to the plugin registration logic in Besu, providing users with granular control over the registration of plugins. Previously, Besu attempted to register all plugins found in the plugins directory indiscriminately. With this update, users can now specify exactly which plugins should be registered, halting the application startup if any specified plugin does not exist. This feature is beneficial for environments where precise plugin configurations are necessary.

fixes #6714

CLI Options and Configuration Profiles

The new plugin registration logic introduces one CLI options:

--plugins: Specifies a comma-separated list of plugin names to be registered.

Changes

  • Plugins Registration Logic: Users can now specify a list of plugins to be registered via CLI options. This list determines which plugins Besu will attempt to register. Besu validates the presence of each specified plugin in the plugins directory. If any specified plugin is not found, the application will halt, preventing startup with an incomplete set of plugins. Any plugin found in the directory that is not explicitly required will be ignored.

Examples

besu --plugins=essential-plugin,security-plugin

In this scenario, if either "essential-plugin" or "security-plugin" is not found in the specified directory, Besu will halt, signalling a configuration issue that needs resolution. Any other plugins will be ignored.

It can also be defined in TOML configuration profiles.

Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title Enhanced Control Over Plugin Registration Enhanced control over plugin registration Mar 8, 2024
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title Enhanced control over plugin registration Enhanced control over plugins registration Mar 8, 2024
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review March 8, 2024 02:51
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

I think you should get rid of the --strict-plugin-registration-enabled
When I'm specifying --plugins on the CL and one of the plugins is not found I would expect that besu does not start

Signed-off-by: Gabriel-Trintinalia <[email protected]>
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Looks good and it is a nice feature, just some minor suggestions

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

couple of comments. Also if the plugin AT failure turns out to be because of the changes in the PR, would be good to make that easier to find (error handling?)

CHANGELOG.md Outdated Show resolved Hide resolved
Gabriel-Trintinalia and others added 9 commits March 15, 2024 17:20
Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@macfarla
Copy link
Contributor

@Gabriel-Trintinalia I see you have made more changes and the tests are passing now 💪 - can you explain about the execution strategy? How does it work and do we have to do anything differently?

@Gabriel-Trintinalia
Copy link
Contributor Author

@Gabriel-Trintinalia I see you have made more changes and the tests are passing now 💪 - can you explain about the execution strategy? How does it work and do we have to do anything differently?

@macfarla Nothing different. I changed the implementation to make it easier to understand. The tests failed because registerPlugins are executed as the next step after ConfigOptionSearchAndRunHandler. At this stage, we have not registered any options added by plugins, and the parse failed with an unknown option error. The changes that I made to fix that are .setUnmatchedArgumentsAllowed(true) and .setUnmatchedArgumentsAllowed(false) That ensures that while we are parsing the options to create the default value provider it will not fail because of the plugin options. The second one ensures that after the plugin registration is done Besu will fail if we still have an unknown option. (expected behaviour)

I have also simplified the ConfigOptionSearchAndRunHandler. It is used to extend RunLast which at the end implements IExecutionStrategy.

CHANGELOG.md Outdated
@@ -41,6 +41,8 @@
- Expose bad block events via the BesuEvents plugin API [#6848](https://github.com/hyperledger/besu/pull/6848)
- Add RPC errors metric [#6919](https://github.com/hyperledger/besu/pull/6919/)
- Add `rlp decode` subcommand to decode IBFT/QBFT extraData to validator list [#6895](https://github.com/hyperledger/besu/pull/6895)
- Enhanced control over plugins registration [#6700](https://github.com/hyperledger/besu/pull/6700)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't say what kind of control is being added. Maybe something like Allow users to specify which plugins are registered

@Gabriel-Trintinalia Gabriel-Trintinalia enabled auto-merge (squash) April 19, 2024 16:08
@Gabriel-Trintinalia Gabriel-Trintinalia merged commit a1f73d9 into hyperledger:main Apr 19, 2024
42 checks passed
macfarla pushed a commit to macfarla/besu that referenced this pull request Apr 26, 2024
jflo pushed a commit to jflo/besu that referenced this pull request May 28, 2024
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
jflo pushed a commit to jflo/besu that referenced this pull request Jun 10, 2024
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature - Flexible Plugin Registration
5 participants