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

CLI option for disabling auto-registration of external plugins #7470

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
dab923d
Add auto detection disable option
Gabriel-Trintinalia Aug 14, 2024
1762ea5
Add tests and rename flag
Gabriel-Trintinalia Aug 15, 2024
3755376
Rename variables
Gabriel-Trintinalia Aug 15, 2024
de29c56
Rename variables
Gabriel-Trintinalia Aug 15, 2024
88f4607
Rename variables
Gabriel-Trintinalia Aug 15, 2024
c95c5d0
Merge main into branch
Gabriel-Trintinalia Aug 15, 2024
3af21df
Merge branch 'hyperledger:main' into plugin-auto-detection-flag
Gabriel-Trintinalia Aug 15, 2024
afe52af
Merge branch 'main' into plugin-auto-detection-flag
Gabriel-Trintinalia Aug 16, 2024
9ed862e
Merge branch 'main' into plugin-auto-detection-flag
Gabriel-Trintinalia Aug 16, 2024
7ce0840
Rename as PR suggestions
Gabriel-Trintinalia Aug 16, 2024
2978b6d
Merge branch 'plugin-auto-detection-flag' of https://github.com/Gabri…
Gabriel-Trintinalia Aug 16, 2024
e14e98d
Rename tests to match optiona name
Gabriel-Trintinalia Aug 16, 2024
20759a9
Merge branch 'main' into plugin-auto-detection-flag
Gabriel-Trintinalia Aug 16, 2024
c46ecda
Change behaviour to disable external plugins and log an error if both…
Gabriel-Trintinalia Aug 16, 2024
919455e
Merge branch 'plugin-auto-detection-flag' of https://github.com/Gabri…
Gabriel-Trintinalia Aug 16, 2024
0622852
Rename methods
Gabriel-Trintinalia Aug 16, 2024
a0442f8
Change option description
Gabriel-Trintinalia Aug 16, 2024
bffe6e6
Rename variables
Gabriel-Trintinalia Aug 16, 2024
53a08c3
Fix javadoc
Gabriel-Trintinalia Aug 16, 2024
13c3be2
Merge branch 'main' into plugin-auto-detection-flag
Gabriel-Trintinalia Aug 16, 2024
2cab6b8
Remove wrong validation
Gabriel-Trintinalia Aug 16, 2024
a134d66
Merge branch 'plugin-auto-detection-flag' of https://github.com/Gabri…
Gabriel-Trintinalia Aug 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,8 @@ public BesuPluginContextImpl providePluginContext(
besuPluginContext.addService(PermissioningService.class, permissioningService);
besuPluginContext.addService(PrivacyPluginService.class, new PrivacyPluginServiceImpl());

besuPluginContext.registerPlugins(new PluginConfiguration(pluginsPath));
besuPluginContext.registerPlugins(
new PluginConfiguration.Builder().pluginsDir(pluginsPath).build());

// register built-in plugins
new RocksDBPlugin().register(besuPluginContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ void setup() {
@Test
public void verifyEverythingGoesSmoothly() {
assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
contextImpl.registerPlugins(new PluginConfiguration(DEFAULT_PLUGIN_DIRECTORY));
contextImpl.registerPlugins(
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build());
assertThat(contextImpl.getRegisteredPlugins()).isNotEmpty();

final Optional<TestPicoCLIPlugin> testPluginOptional =
Expand All @@ -86,7 +87,8 @@ public void registrationErrorsHandledSmoothly() {
System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER");

assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
contextImpl.registerPlugins(new PluginConfiguration(DEFAULT_PLUGIN_DIRECTORY));
contextImpl.registerPlugins(
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build());
assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class);

contextImpl.beforeExternalServices();
Expand All @@ -104,7 +106,8 @@ public void startErrorsHandledSmoothly() {
System.setProperty("testPicoCLIPlugin.testOption", "FAILSTART");

assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
contextImpl.registerPlugins(new PluginConfiguration(DEFAULT_PLUGIN_DIRECTORY));
contextImpl.registerPlugins(
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build());
assertThat(contextImpl.getRegisteredPlugins())
.extracting("class")
.contains(TestPicoCLIPlugin.class);
Expand All @@ -129,7 +132,8 @@ public void stopErrorsHandledSmoothly() {
System.setProperty("testPicoCLIPlugin.testOption", "FAILSTOP");

assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
contextImpl.registerPlugins(new PluginConfiguration(DEFAULT_PLUGIN_DIRECTORY));
contextImpl.registerPlugins(
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build());
assertThat(contextImpl.getRegisteredPlugins())
.extracting("class")
.contains(TestPicoCLIPlugin.class);
Expand All @@ -151,7 +155,9 @@ public void stopErrorsHandledSmoothly() {
@Test
public void lifecycleExceptions() throws Throwable {
final ThrowableAssert.ThrowingCallable registerPlugins =
() -> contextImpl.registerPlugins(new PluginConfiguration(DEFAULT_PLUGIN_DIRECTORY));
() ->
contextImpl.registerPlugins(
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build());

assertThatExceptionOfType(IllegalStateException.class).isThrownBy(contextImpl::startPlugins);
assertThatExceptionOfType(IllegalStateException.class).isThrownBy(contextImpl::stopPlugins);
Expand All @@ -173,7 +179,8 @@ public void lifecycleExceptions() throws Throwable {

@Test
public void shouldRegisterAllPluginsWhenNoPluginsOption() {
final PluginConfiguration config = createConfigurationForAllPlugins();
final PluginConfiguration config =
PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build();

assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
contextImpl.registerPlugins(config);
Expand Down Expand Up @@ -227,12 +234,33 @@ void shouldThrowExceptionIfExplicitlySpecifiedPluginNotFound() {
assertThat(contextImpl.getRegisteredPlugins()).isEmpty();
}

private PluginConfiguration createConfigurationForAllPlugins() {
return new PluginConfiguration(null, DEFAULT_PLUGIN_DIRECTORY);
@Test
void shouldNotRegisterAnyPluginsIfExternalPluginsDisabled() {
PluginConfiguration config =
PluginConfiguration.builder()
.pluginsDir(DEFAULT_PLUGIN_DIRECTORY)
.externalPluginsEnabled(false)
.build();
contextImpl.registerPlugins(config);
assertThat(contextImpl.getRegisteredPlugins().isEmpty()).isTrue();
}

@Test
void shouldRegisterPluginsIfExternalPluginsEnabled() {
PluginConfiguration config =
PluginConfiguration.builder()
.pluginsDir(DEFAULT_PLUGIN_DIRECTORY)
.externalPluginsEnabled(true)
.build();
contextImpl.registerPlugins(config);
assertThat(contextImpl.getRegisteredPlugins().isEmpty()).isFalse();
}

private PluginConfiguration createConfigurationForSpecificPlugin(final String pluginName) {
return new PluginConfiguration(List.of(new PluginInfo(pluginName)), DEFAULT_PLUGIN_DIRECTORY);
return PluginConfiguration.builder()
.requestedPlugins(List.of(new PluginInfo(pluginName)))
.pluginsDir(DEFAULT_PLUGIN_DIRECTORY)
.build();
}

private Optional<TestPicoCLIPlugin> findTestPlugin(
Expand Down
9 changes: 7 additions & 2 deletions besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,7 @@ public void run() {
runner.startExternalServices();

startPlugins(runner);
validatePluginOptions();
validatePrivacyPluginOptions();
setReleaseMetrics();
preSynchronization();

Expand Down Expand Up @@ -1347,7 +1347,7 @@ private void startPlugins(final Runner runner) {
besuPluginContext.startPlugins();
}

private void validatePluginOptions() {
private void validatePrivacyPluginOptions() {
// plugins do not 'wire up' until start has been called
// consequently you can only do some configuration checks
// after start has been called on plugins
Expand Down Expand Up @@ -1491,6 +1491,7 @@ private void validateOptions() {
validateGraphQlOptions();
validateApiOptions();
validateConsensusSyncCompatibilityOptions();
validatePluginOptions();
p2pTLSConfigOptions.checkP2PTLSOptionsDependencies(logger, commandLine);
}

Expand All @@ -1511,6 +1512,10 @@ private void validateConsensusSyncCompatibilityOptions() {
}
}

private void validatePluginOptions() {
pluginsConfigurationOptions.validate(commandLine);
}

private void validateApiOptions() {
apiConfigurationOptions.validate(commandLine, logger);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ public interface DefaultCommandValues {
/** The constant DEFAULT_PLUGINS_OPTION_NAME. */
String DEFAULT_PLUGINS_OPTION_NAME = "--plugins";

/** The constant DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME. */
String DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME = "--Xplugins-external-enabled";

/**
* Gets default besu data path.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.hyperledger.besu.cli.options.stable;

import static org.hyperledger.besu.cli.DefaultCommandValues.DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME;
import static org.hyperledger.besu.cli.DefaultCommandValues.DEFAULT_PLUGINS_OPTION_NAME;

import org.hyperledger.besu.cli.converter.PluginInfoConverter;
Expand All @@ -28,6 +29,15 @@

/** The Plugins Options options. */
public class PluginsConfigurationOptions implements CLIOptions<PluginConfiguration> {

@CommandLine.Option(
names = {DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME},
description = "Enables external plugins (default: ${DEFAULT-VALUE})",
hidden = true,
defaultValue = "true",
arity = "1")
private Boolean externalPluginsEnabled = true;

@CommandLine.Option(
names = {DEFAULT_PLUGINS_OPTION_NAME},
description = "Comma-separated list of plugin names",
Expand All @@ -42,7 +52,24 @@ public PluginsConfigurationOptions() {}

@Override
public PluginConfiguration toDomainObject() {
return new PluginConfiguration(plugins);
return new PluginConfiguration.Builder()
.externalPluginsEnabled(externalPluginsEnabled)
.requestedPlugins(plugins)
.build();
}

/**
* Validate that there are no inconsistencies in the specified options.
*
* @param commandLine the full commandLine to check all the options specified by the user
*/
public void validate(final CommandLine commandLine) {
String errorMessage =
String.format(
"%s option can only be used when %s is true",
DEFAULT_PLUGINS_OPTION_NAME, DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME);
CommandLineUtils.failIfOptionDoesntMeetRequirement(
commandLine, errorMessage, externalPluginsEnabled, List.of(DEFAULT_PLUGINS_OPTION_NAME));
}

@Override
Expand All @@ -61,6 +88,13 @@ public static PluginConfiguration fromCommandLine(final CommandLine commandLine)
CommandLineUtils.getOptionValueOrDefault(
commandLine, DEFAULT_PLUGINS_OPTION_NAME, new PluginInfoConverter());

return new PluginConfiguration(plugins);
boolean externalPluginsEnabled =
CommandLineUtils.getOptionValueOrDefault(
commandLine, DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME, Boolean::parseBoolean);

return new PluginConfiguration.Builder()
.requestedPlugins(plugins)
.externalPluginsEnabled(externalPluginsEnabled)
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -132,20 +132,25 @@ public void registerPlugins(final PluginConfiguration config) {
"Besu plugins have already been registered. Cannot register additional plugins.");
state = Lifecycle.REGISTERING;

detectedPlugins = detectPlugins(config);
if (!config.getRequestedPlugins().isEmpty()) {
// Register only the plugins that were explicitly requested and validated
requestedPlugins = config.getRequestedPlugins();

// Match and validate the requested plugins against the detected plugins
List<BesuPlugin> registeringPlugins =
matchAndValidateRequestedPlugins(requestedPlugins, detectedPlugins);

registerPlugins(registeringPlugins);
if (config.isExternalPluginsEnabled()) {
detectedPlugins = detectPlugins(config);

if (config.getRequestedPlugins().isEmpty()) {
// If no plugins were specified, register all detected plugins
registerPlugins(detectedPlugins);
} else {
// Register only the plugins that were explicitly requested and validated
requestedPlugins = config.getRequestedPlugins();
// Match and validate the requested plugins against the detected plugins
List<BesuPlugin> registeringPlugins =
matchAndValidateRequestedPlugins(requestedPlugins, detectedPlugins);

registerPlugins(registeringPlugins);
}
} else {
// If no plugins were specified, register all detected plugins
registerPlugins(detectedPlugins);
LOG.debug("External plugins are disabled. Skipping plugins registration.");
}
state = Lifecycle.REGISTERED;
}

private List<BesuPlugin> matchAndValidateRequestedPlugins(
Expand Down Expand Up @@ -182,7 +187,6 @@ private void registerPlugins(final List<BesuPlugin> pluginsToRegister) {
registeredPlugins.add(plugin);
}
}
state = Lifecycle.REGISTERED;
}

private boolean registerPlugin(final BesuPlugin plugin) {
Expand Down
118 changes: 118 additions & 0 deletions besu/src/test/java/org/hyperledger/besu/cli/PluginsOptionsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.cli;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.verify;

import org.hyperledger.besu.ethereum.core.plugins.PluginConfiguration;

import java.util.List;

import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;

public class PluginsOptionsTest extends CommandTestAbstract {

@Captor protected ArgumentCaptor<PluginConfiguration> pluginConfigurationArgumentCaptor;

@Test
public void shouldParsePluginOptionForSinglePlugin() {
parseCommand("--plugins", "pluginA");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());
assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins())
.isEqualTo(List.of("pluginA"));
assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldParsePluginOptionForMultiplePlugins() {
parseCommand("--plugins", "pluginA,pluginB");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());
assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins())
.isEqualTo(List.of("pluginA", "pluginB"));

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldNotUsePluginOptionWhenNoPluginsSpecified() {
parseCommand();
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());
assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins())
.isEqualTo(List.of());
assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldNotParseAnyPluginsWhenPluginOptionIsEmpty() {
parseCommand("--plugins", "");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());
assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins())
.isEqualTo(List.of());
assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldParsePluginsExternalEnabledOptionWhenFalse() {
parseCommand("--Xplugins-external-enabled=false");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());

assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled())
.isEqualTo(false);

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldParsePluginsExternalEnabledOptionWhenTrue() {
parseCommand("--Xplugins-external-enabled=true");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());

assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled())
.isEqualTo(true);

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldEnablePluginsExternalByDefault() {
parseCommand();
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());
assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled())
.isEqualTo(true);

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void shouldFailWhenPluginsIsDisabledAndPluginsExplicitlyRequested() {
parseCommand("--Xplugins-external-enabled=false", "--plugins", "pluginA");
verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture());

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8))
.contains("--plugins option can only be used when --Xplugins-external-enabled is true");
}
}
4 changes: 4 additions & 0 deletions besu/src/test/resources/everything_config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -242,3 +242,7 @@ Xp2p-tls-clienthello-sni=false

#contracts
Xevm-jumpdest-cache-weight-kb=32000

# plugins
Xplugins-external-enabled=true
plugins=["none"]
Loading
Loading