From 5fdf1919ec3501609cbcb2a1895d63fa87197642 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Feb 2025 12:02:50 -0800 Subject: [PATCH] More flexible settings pattern (#123746) This commit reworks the settings globs to be more useable. Primarily it expands the values so that the settings may be lists, iterating over each value. Additionally it simplifies the function to also allow non-glob settings so that this single method may be used to lookup all values for a given setting pattern, whether it contains a glob or not. --- .../bootstrap/EntitlementBootstrap.java | 11 +++----- .../EntitlementInitialization.java | 3 +-- .../runtime/policy/PathLookup.java | 3 +-- .../policy/entitlements/FilesEntitlement.java | 8 +----- .../runtime/policy/FileAccessTreeTests.java | 3 +-- .../runtime/policy/PolicyManagerTests.java | 3 +-- .../entitlements/FilesEntitlementTests.java | 3 +-- .../bootstrap/Elasticsearch.java | 3 +-- .../common/settings/Settings.java | 26 +++++++++++-------- .../common/settings/SettingsTests.java | 17 ++++++++++-- 10 files changed, 40 insertions(+), 40 deletions(-) diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java index 85ee115be91ad..e82ec923618f0 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/bootstrap/EntitlementBootstrap.java @@ -35,8 +35,7 @@ public class EntitlementBootstrap { public record BootstrapArgs( Map pluginPolicies, Function, String> pluginResolver, - Function settingResolver, - Function> settingGlobResolver, + Function> settingResolver, Path[] dataDirs, Path[] sharedRepoDirs, Path configDir, @@ -51,7 +50,6 @@ public record BootstrapArgs( requireNonNull(pluginPolicies); requireNonNull(pluginResolver); requireNonNull(settingResolver); - requireNonNull(settingGlobResolver); requireNonNull(dataDirs); if (dataDirs.length == 0) { throw new IllegalArgumentException("must provide at least one data directory"); @@ -78,8 +76,7 @@ public static BootstrapArgs bootstrapArgs() { * * @param pluginPolicies a map holding policies for plugins (and modules), by plugin (or module) name. * @param pluginResolver a functor to map a Java Class to the plugin it belongs to (the plugin name). - * @param settingResolver a functor to resolve the value of an Elasticsearch setting. - * @param settingGlobResolver a functor to resolve a glob expression for one or more Elasticsearch settings. + * @param settingResolver a functor to resolve a setting name pattern for one or more Elasticsearch settings. * @param dataDirs data directories for Elasticsearch * @param sharedRepoDirs shared repository directories for Elasticsearch * @param configDir the config directory for Elasticsearch @@ -93,8 +90,7 @@ public static BootstrapArgs bootstrapArgs() { public static void bootstrap( Map pluginPolicies, Function, String> pluginResolver, - Function settingResolver, - Function> settingGlobResolver, + Function> settingResolver, Path[] dataDirs, Path[] sharedRepoDirs, Path configDir, @@ -113,7 +109,6 @@ public static void bootstrap( pluginPolicies, pluginResolver, settingResolver, - settingGlobResolver, dataDirs, sharedRepoDirs, configDir, diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java index dc839a5259821..8ada792983811 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java @@ -144,8 +144,7 @@ private static PolicyManager createPolicyManager() { bootstrapArgs.dataDirs(), bootstrapArgs.sharedRepoDirs(), bootstrapArgs.tempDir(), - bootstrapArgs.settingResolver(), - bootstrapArgs.settingGlobResolver() + bootstrapArgs.settingResolver() ); List serverScopes = new ArrayList<>(); diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PathLookup.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PathLookup.java index df99eecf0f8cf..5cbe6108e009c 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PathLookup.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PathLookup.java @@ -19,6 +19,5 @@ public record PathLookup( Path[] dataDirs, Path[] sharedRepoDirs, Path tempDir, - Function settingResolver, - Function> settingGlobResolver + Function> settingResolver ) {} diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java index f52015572c9b5..94af46dceeb5a 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java @@ -232,13 +232,7 @@ public PathSettingFileData withExclusive(boolean exclusive) { @Override public Stream resolveRelativePaths(PathLookup pathLookup) { - Stream result; - if (setting.contains("*")) { - result = pathLookup.settingGlobResolver().apply(setting); - } else { - String path = pathLookup.settingResolver().apply(setting); - result = path == null ? Stream.of() : Stream.of(path); - } + Stream result = pathLookup.settingResolver().apply(setting); if (ignoreUrl) { result = result.filter(s -> s.toLowerCase(Locale.ROOT).startsWith("https://") == false); } diff --git a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java index 106a7db84e087..6815c1c0b8141 100644 --- a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java +++ b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java @@ -50,8 +50,7 @@ private static Path path(String s) { new Path[] { Path.of("/data1"), Path.of("/data2") }, new Path[] { Path.of("/shared1"), Path.of("/shared2") }, Path.of("/tmp"), - setting -> settings.get(setting), - glob -> settings.getGlobValues(glob) + pattern -> settings.getValues(pattern) ); public void testEmpty() { diff --git a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java index 7f37168a3a7f8..047a5fe4ed59c 100644 --- a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java +++ b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java @@ -71,8 +71,7 @@ public static void beforeClass() { new Path[] { TEST_BASE_DIR.resolve("/data1/"), TEST_BASE_DIR.resolve("/data2") }, new Path[] { TEST_BASE_DIR.resolve("/shared1"), TEST_BASE_DIR.resolve("/shared2") }, TEST_BASE_DIR.resolve("/temp"), - Settings.EMPTY::get, - Settings.EMPTY::getGlobValues + Settings.EMPTY::getValues ); } catch (Exception e) { throw new IllegalStateException(e); diff --git a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java index 31d66e22afd43..e051cc2479a73 100644 --- a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java +++ b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlementTests.java @@ -48,8 +48,7 @@ public static void setupRoot() { new Path[] { Path.of("/data1"), Path.of("/data2") }, new Path[] { Path.of("/shared1"), Path.of("/shared2") }, Path.of("/tmp"), - setting -> settings.get(setting), - glob -> settings.getGlobValues(glob) + pattern -> settings.getValues(pattern) ); public void testEmptyBuild() { diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index 295197900e020..18d1cafdfc369 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -242,8 +242,7 @@ private static void initPhase2(Bootstrap bootstrap) throws IOException { EntitlementBootstrap.bootstrap( pluginPolicies, pluginsResolver::resolveClassToPluginName, - nodeEnv.settings()::get, - nodeEnv.settings()::getGlobValues, + nodeEnv.settings()::getValues, nodeEnv.dataDirs(), nodeEnv.repoDirs(), nodeEnv.configDir(), diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index 04f3d942f34b2..784fb5ebc63d3 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -291,21 +291,25 @@ public String get(String setting, String defaultValue) { } /** - * Returns the values for settings that match the given glob pattern. - * A single glob is supported. + * Returns the values for the given settings pattern. * - * @param settingGlob setting name containing a glob - * @return zero or more values for any settings in this settings object that match the glob pattern + * Either a concrete setting name, or a pattern containing a single glob is supported. + * + * @param settingPattern name of a setting or a setting name pattern containing a glob + * @return zero or more values for any settings in this settings object that match the given pattern */ - public Stream getGlobValues(String settingGlob) { - int globIndex = settingGlob.indexOf(".*."); + public Stream getValues(String settingPattern) { + int globIndex = settingPattern.indexOf(".*."); + Stream settingNames; if (globIndex == -1) { - throw new IllegalArgumentException("Pattern [" + settingGlob + "] does not contain a glob [*]"); + settingNames = Stream.of(settingPattern); + } else { + String prefix = settingPattern.substring(0, globIndex + 1); + String suffix = settingPattern.substring(globIndex + 2); + Settings subSettings = getByPrefix(prefix); + settingNames = subSettings.names().stream().map(k -> prefix + k + suffix); } - String prefix = settingGlob.substring(0, globIndex + 1); - String suffix = settingGlob.substring(globIndex + 2); - Settings subSettings = getByPrefix(prefix); - return subSettings.names().stream().map(k -> k + suffix).map(subSettings::get).filter(Objects::nonNull); + return settingNames.map(this::getAsList).flatMap(List::stream).filter(Objects::nonNull); } /** diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index 21e3541011368..d608136fa564e 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -44,6 +44,7 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.is; @@ -711,9 +712,21 @@ public void testProcessSetting() throws IOException { } public void testGlobValues() throws IOException { - Settings test = Settings.builder().put("foo.x.bar", "1").put("foo.y.bar", "2").build(); - var values = test.getGlobValues("foo.*.bar").toList(); + Settings test = Settings.builder().put("foo.x.bar", "1").build(); + + // no values + assertThat(test.getValues("foo.*.baz").toList(), empty()); + assertThat(test.getValues("fuz.*.bar").toList(), empty()); + + var values = test.getValues("foo.*.bar").toList(); + assertThat(values, containsInAnyOrder("1")); + + test = Settings.builder().put("foo.x.bar", "1").put("foo.y.bar", "2").build(); + values = test.getValues("foo.*.bar").toList(); assertThat(values, containsInAnyOrder("1", "2")); + + values = test.getValues("foo.x.bar").toList(); + assertThat(values, contains("1")); } }