Skip to content

Commit

Permalink
CLI option for disabling auto-registration of external plugins (#7470)
Browse files Browse the repository at this point in the history
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
  • Loading branch information
Gabriel-Trintinalia authored Aug 16, 2024
1 parent 5133595 commit d7041d4
Show file tree
Hide file tree
Showing 9 changed files with 269 additions and 76 deletions.
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

0 comments on commit d7041d4

Please sign in to comment.