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

Support aliases in environment variables and YAML configuration #825

Merged
merged 7 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- Hashicorp connection properties can now override http protocol to HTTP/1.1 from the default of HTTP/2. [#817](https://github.com/ConsenSys/web3signer/pull/817)

### Bugs fixed
- Support long name aliases in environment variables and YAML configuration [#825](https://github.com/Consensys/web3signer/pull/825)

---
## 23.6.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,15 @@ public String defaultValue(final ArgSpec argSpec) {
final OptionSpec optionSpec = (OptionSpec) argSpec;
final String qualifiedPrefix =
optionSpec.command().qualifiedName("_").replace("-", "_").toUpperCase();
final String key = stripPrefix(optionSpec.longestName()).replace("-", "_").toUpperCase();
final String qualifiedKey = qualifiedPrefix + "_" + key;
return environment.get(qualifiedKey);

for (final String alias : optionSpec.names()) {
final String key = stripPrefix(alias).replace("-", "_").toUpperCase();
final String qualifiedKey = qualifiedPrefix + "_" + key;
final String value = environment.get(qualifiedKey);
if (value != null) {
return value;
}
}
}

return null; // currently not supporting positional parameters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@
package tech.pegasys.web3signer.commandline.valueprovider;

import static java.util.function.Predicate.not;
import static tech.pegasys.web3signer.commandline.valueprovider.PrefixUtil.stripPrefix;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.NoSuchFileException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.TreeSet;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -107,14 +108,14 @@ private void checkUnknownOptions(final Map<String, Object> result) {
// parent command options
final Set<String> mainCommandOptions =
commandLine.getCommandSpec().options().stream()
.map(YamlConfigFileDefaultProvider::buildOptionName)
.flatMap(YamlConfigFileDefaultProvider::buildOptionNames)
.collect(Collectors.toSet());

// subcommands options
final Set<String> subCommandsOptions =
subCommandValues(commandLine)
.flatMap(YamlConfigFileDefaultProvider::allSubCommandOptions)
.map(YamlConfigFileDefaultProvider::buildQualifiedOptionName)
.flatMap(YamlConfigFileDefaultProvider::buildQualifiedOptionNames)
.collect(Collectors.toSet());

picoCliOptionsKeys.addAll(mainCommandOptions);
Expand Down Expand Up @@ -142,15 +143,16 @@ private void checkConfigurationValidity(final boolean isEmpty) {
}

private String getConfigurationValue(final OptionSpec optionSpec) {
final String keyName;
final Stream<String> keyAliases;
if (commandLine.getCommandName().equals(optionSpec.command().name())) {
keyName = buildOptionName(optionSpec);
keyAliases = buildOptionNames(optionSpec);
} else {
// subcommand option
keyName = buildQualifiedOptionName(optionSpec);
keyAliases = buildQualifiedOptionNames(optionSpec);
}

final Object value = result.get(keyName);
final Object value =
keyAliases.map(k -> result.get(k)).filter(Objects::nonNull).findFirst().orElse(null);

if (value == null) {
return null;
Expand All @@ -173,13 +175,13 @@ private static Stream<OptionSpec> allSubCommandOptions(final CommandLine subcomm
subCommandValues(subcommand).flatMap(YamlConfigFileDefaultProvider::allSubCommandOptions));
}

private static String buildQualifiedOptionName(final OptionSpec optionSpec) {
private static Stream<String> buildQualifiedOptionNames(final OptionSpec optionSpec) {
final String cmdPrefix = optionSpec.command().qualifiedName(".");
final String prefixWithoutWeb3Signer = cmdPrefix.replaceFirst(WEB3SIGNER_CMD_PREFIX, "");
return prefixWithoutWeb3Signer + "." + buildOptionName(optionSpec);
return buildOptionNames(optionSpec).map(s -> prefixWithoutWeb3Signer + "." + s);
}

private static String buildOptionName(final OptionSpec optionSpec) {
return stripPrefix(optionSpec.longestName());
private static Stream<String> buildOptionNames(final OptionSpec optionSpec) {
return Arrays.stream(optionSpec.names()).map(PrefixUtil::stripPrefix);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,14 @@ public static String validBaseYamlOptions() {
return "http-listen-port: 6001\n"
+ "http-listen-host: \"localhost\"\n"
+ "key-store-path: \"./keys_yaml\"\n"
+ "metrics-categories: \"HTTP\"\n"
+ "logging: \"INFO\"\n";
}

public static String validBaseYamlAliasOptions() {
return "metrics-category: \"HTTP\"\n" + "l: \"INFO\"\n";
}

public static Map<String, String> validBaseEnvironmentVariableOptions() {
return Map.of(
"WEB3SIGNER_HTTP_LISTEN_PORT",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,19 @@ public class DemoCommand {
@Option(names = "--name", description = "Name")
String name;

@Option(
names = {"--alias", "--aliases"},
description = "Aliases")
String alias;

@Command(name = "country", description = "Country Codes")
static class SubCommand {
@Option(names = "--codes", split = ",")
List<String> countryCodes;

@Option(
names = {"--subalias", "--subaliases"},
description = "Subcommand Alias")
String subalias;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,29 @@ void emptyEnvironmentVariablesResultsNullValues() {
assertThat(subCommand.countryCodes).isNullOrEmpty();
}

@Test
void environmentVariablesAreCreatedForAliases() {

assertAliasCreated("DEMO_ALIAS", "testValue");
assertAliasCreated("DEMO_ALIASES", "testValue2");
assertAliasCreated("DEMO_COUNTRY_SUBALIAS", "testValue3", "country");
assertAliasCreated("DEMO_COUNTRY_SUBALIASES", "testValue4", "country");
}

private void assertAliasCreated(String envKey, String envValue, String... subCommandName) {
final EnvironmentVariableDefaultProvider defaultProvider =
new EnvironmentVariableDefaultProvider(Map.of(envKey, envValue));

commandLine.setDefaultValueProvider(defaultProvider);
commandLine.parseArgs(subCommandName);

if (subCommandName.length > 0) {
assertThat(subCommand.subalias).isEqualTo(envValue);
} else {
assertThat(demoCommand.alias).isEqualTo(envValue);
}
}

private Map<String, String> validEnvMap() {
return Map.of(
"DEMO_X", "10", "DEMO_Y", "20", "DEMO_NAME", "test name", "DEMO_COUNTRY_CODES", "AU,US");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@
import tech.pegasys.web3signer.CmdlineHelpers;
import tech.pegasys.web3signer.commandline.Web3SignerBaseCommand;
import tech.pegasys.web3signer.commandline.valueprovider.DemoCommand.SubCommand;
import tech.pegasys.web3signer.common.Web3SignerMetricCategory;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Set;

import org.apache.logging.log4j.Level;
import org.junit.jupiter.api.Test;
Expand All @@ -51,13 +53,21 @@ void valuesFromConfigFileArePopulated(@TempDir final Path tempDir) throws IOExce

assertThat(web3SignerBaseCommand.getHttpListenPort()).isEqualTo(6001);
assertThat(web3SignerBaseCommand.getKeyConfigPath()).isEqualTo(Path.of("./keys_yaml"));
assertThat(web3SignerBaseCommand.getMetricCategories())
.isEqualTo(Set.of(Web3SignerMetricCategory.HTTP));
assertThat(web3SignerBaseCommand.getLogLevel()).isEqualTo(Level.INFO);
}

@Test
void valuesFromConfigFileWithSubcommandArePopulated(@TempDir final Path tempDir)
throws IOException {
final String subcommandOptions =
String.join(lineSeparator(), "demo.x: 10", "demo.y: 2", "demo.name: \"test name\"");
String.join(
lineSeparator(),
"demo.x: 10",
"demo.y: 2",
"demo.name: \"test name\"",
"demo.aliases: \"test alias\"");
final String config = CmdlineHelpers.validBaseYamlOptions() + subcommandOptions;
final File configFile = Files.writeString(tempDir.resolve("config.yaml"), config).toFile();

Expand All @@ -74,6 +84,26 @@ void valuesFromConfigFileWithSubcommandArePopulated(@TempDir final Path tempDir)
assertThat(subCommand.x).isEqualTo(10);
assertThat(subCommand.y).isEqualTo(2);
assertThat(subCommand.name).isEqualTo("test name");
assertThat(subCommand.alias).isEqualTo("test alias");
}

@Test
void aliasesFromConfigFileArePopulated(@TempDir final Path tempDir) throws IOException {
final String subcommandOptions = String.join(lineSeparator(), "demo.alias: \"test alias\"");
final String config = CmdlineHelpers.validBaseYamlAliasOptions() + subcommandOptions;
final File configFile = Files.writeString(tempDir.resolve("config.yaml"), config).toFile();
final Web3SignerBaseCommand web3SignerBaseCommand = new Web3SignerBaseCommand();
final DemoCommand subCommand = new DemoCommand();
final CommandLine commandLine = new CommandLine(web3SignerBaseCommand);
commandLine.registerConverter(Level.class, Level::valueOf);
commandLine.addSubcommand("demo", subCommand);
commandLine.setDefaultValueProvider(new YamlConfigFileDefaultProvider(commandLine, configFile));
commandLine.parseArgs("demo");

assertThat(web3SignerBaseCommand.getMetricCategories())
.isEqualTo(Set.of(Web3SignerMetricCategory.HTTP));
assertThat(web3SignerBaseCommand.getLogLevel()).isEqualTo(Level.INFO);
assertThat(subCommand.alias).isEqualTo("test alias");
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions gradle/versions.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,9 @@ dependencyManagement {
entry 'besu'
entry ('core') {
exclude group: 'com.github.jnr', name: 'jnr-unixsocket'
exclude group: 'org.bouncycastle', name: 'bcprov-jdk15on'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that since the artefact name itself changed, it's not possible for gradle to automatically resolve to the jdk18 version. We're using a version of web3j that pulls in the old one. I tried upgrading web3j first and it caused other issues so aborted that, but we should revisit.

I believe the utils module is a dependency of core. In an earlier commit I had introduced the utils entry simply to be able to exclude bouncycastle. In this PR I realised I could simplify that by moving the exclude.

}
entry 'crypto'
entry ('utils') {
entry ('crypto') {
exclude group: 'org.bouncycastle', name: 'bcprov-jdk15on'
}
}
Expand Down