Skip to content

Commit

Permalink
[refactor] add Environment in BootstrapContext (#36573)
Browse files Browse the repository at this point in the history
There are certain BootstrapCheck checks that may need access environment-specific
values. Watcher's EncryptSensitiveDataBootstrapCheck passes in the node's environment
via a constructor to bypass the shortcoming in BootstrapContext. This commit
pulls in the node's environment into BootstrapContext.

Another case is found in #36519, where it is useful to check the state of the
data-path. Since PathUtils.get and Paths.get are forbidden APIs, we rely on
the environment to retrieve references to things like node data paths.

This means that the BootstrapContext will have the same Settings used in the
Environment, which currently differs from the Node's settings.
  • Loading branch information
talevy committed Dec 13, 2018
1 parent d40037c commit cd1bec3
Show file tree
Hide file tree
Showing 24 changed files with 201 additions and 176 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@

import org.apache.logging.log4j.Logger;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.node.NodeValidationException;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.AbstractBootstrapCheckTestCase;
import org.hamcrest.Matcher;
import org.junit.After;
import org.junit.Before;
Expand All @@ -40,7 +39,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

public class EvilBootstrapChecksTests extends ESTestCase {
public class EvilBootstrapChecksTests extends AbstractBootstrapCheckTestCase {

private String esEnforceBootstrapChecks = System.getProperty(ES_ENFORCE_BOOTSTRAP_CHECKS);

Expand All @@ -65,7 +64,7 @@ public void testEnforceBootstrapChecks() throws NodeValidationException {

final NodeValidationException e = expectThrows(
NodeValidationException.class,
() -> BootstrapChecks.check(new BootstrapContext(Settings.EMPTY, null), false, checks, logger));
() -> BootstrapChecks.check(emptyContext, false, checks, logger));
final Matcher<String> allOf =
allOf(containsString("bootstrap checks failed"), containsString("error"));
assertThat(e, hasToString(allOf));
Expand All @@ -77,7 +76,7 @@ public void testNonEnforcedBootstrapChecks() throws NodeValidationException {
setEsEnforceBootstrapChecks(null);
final Logger logger = mock(Logger.class);
// nothing should happen
BootstrapChecks.check(new BootstrapContext(Settings.EMPTY, null), false, emptyList(), logger);
BootstrapChecks.check(emptyContext, false, emptyList(), logger);
verifyNoMoreInteractions(logger);
}

Expand All @@ -87,7 +86,7 @@ public void testInvalidValue() {
final boolean enforceLimits = randomBoolean();
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> BootstrapChecks.check(new BootstrapContext(Settings.EMPTY, null), enforceLimits, emptyList()));
() -> BootstrapChecks.check(emptyContext, enforceLimits, emptyList()));
final Matcher<String> matcher = containsString(
"[es.enforce.bootstrap.checks] must be [true] but was [" + value + "]");
assertThat(e, hasToString(matcher));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ static void check(final BootstrapContext context, final BoundTransportAddress bo
final List<BootstrapCheck> combinedChecks = new ArrayList<>(builtInChecks);
combinedChecks.addAll(additionalChecks);
check( context,
enforceLimits(boundTransportAddress, DiscoveryModule.DISCOVERY_TYPE_SETTING.get(context.settings)),
enforceLimits(boundTransportAddress, DiscoveryModule.DISCOVERY_TYPE_SETTING.get(context.settings())),
Collections.unmodifiableList(combinedChecks));
}

Expand Down Expand Up @@ -302,7 +302,7 @@ static class MlockallCheck implements BootstrapCheck {

@Override
public BootstrapCheckResult check(BootstrapContext context) {
if (BootstrapSettings.MEMORY_LOCK_SETTING.get(context.settings) && !isMemoryLocked()) {
if (BootstrapSettings.MEMORY_LOCK_SETTING.get(context.settings()) && !isMemoryLocked()) {
return BootstrapCheckResult.failure("memory locking requested for elasticsearch process but memory is not locked");
} else {
return BootstrapCheckResult.success();
Expand Down Expand Up @@ -408,7 +408,7 @@ static class MaxMapCountCheck implements BootstrapCheck {
@Override
public BootstrapCheckResult check(final BootstrapContext context) {
// we only enforce the check if mmapfs is an allowed store type
if (IndexModule.NODE_STORE_ALLOW_MMAPFS.get(context.settings)) {
if (IndexModule.NODE_STORE_ALLOW_MMAPFS.get(context.settings())) {
if (getMaxMapCount() != -1 && getMaxMapCount() < LIMIT) {
final String message = String.format(
Locale.ROOT,
Expand Down Expand Up @@ -525,7 +525,7 @@ static class SystemCallFilterCheck implements BootstrapCheck {

@Override
public BootstrapCheckResult check(BootstrapContext context) {
if (BootstrapSettings.SYSTEM_CALL_FILTER_SETTING.get(context.settings) && !isSystemCallFilterInstalled()) {
if (BootstrapSettings.SYSTEM_CALL_FILTER_SETTING.get(context.settings()) && !isSystemCallFilterInstalled()) {
final String message = "system call filters failed to install; " +
"check the logs and fix your configuration or disable system call filters at your own risk";
return BootstrapCheckResult.failure(message);
Expand Down Expand Up @@ -725,10 +725,10 @@ boolean isAllPermissionGranted() {
static class DiscoveryConfiguredCheck implements BootstrapCheck {
@Override
public BootstrapCheckResult check(BootstrapContext context) {
if (DiscoveryModule.ZEN2_DISCOVERY_TYPE.equals(DiscoveryModule.DISCOVERY_TYPE_SETTING.get(context.settings)) == false) {
if (DiscoveryModule.ZEN2_DISCOVERY_TYPE.equals(DiscoveryModule.DISCOVERY_TYPE_SETTING.get(context.settings())) == false) {
return BootstrapCheckResult.success();
}
if (ClusterBootstrapService.discoveryIsConfigured(context.settings)) {
if (ClusterBootstrapService.discoveryIsConfigured(context.settings())) {
return BootstrapCheckResult.success();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,36 @@

import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;

/**
* Context that is passed to every bootstrap check to make decisions on.
*/
public class BootstrapContext {
/**
* The nodes settings
* The node's environment
*/
public final Settings settings;
private final Environment environment;

/**
* The nodes local state metadata loaded on startup
* The node's local state metadata loaded on startup
*/
public final MetaData metaData;
private final MetaData metaData;

public BootstrapContext(Settings settings, MetaData metaData) {
this.settings = settings;
public BootstrapContext(Environment environment, MetaData metaData) {
this.environment = environment;
this.metaData = metaData;
}

public Environment environment() {
return environment;
}

public Settings settings() {
return environment.settings();
}

public MetaData metaData() {
return metaData;
}
}
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/node/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ public Node start() throws NodeValidationException {
onDiskMetadata = MetaData.EMPTY_META_DATA;
}
assert onDiskMetadata != null : "metadata is null but shouldn't"; // this is never null
validateNodeBeforeAcceptingRequests(new BootstrapContext(settings, onDiskMetadata), transportService.boundAddress(), pluginsService
validateNodeBeforeAcceptingRequests(new BootstrapContext(environment, onDiskMetadata), transportService.boundAddress(), pluginsService
.filterPlugins(Plugin
.class)
.stream()
Expand Down
Loading

0 comments on commit cd1bec3

Please sign in to comment.