Skip to content

Commit

Permalink
[7.x] Enable GeoIP downloader by default (elastic#71505) (elastic#71733)
Browse files Browse the repository at this point in the history
* Enable GeoIP downloader by default (elastic#71505)

This change enables GeoIP downloader by default.
It removes feature flag but adds flag that is used by tests to disable it again (as we don't want to hammer GeoIP database service with every test cluster we spin up).

Relates to elastic#68920

* fix compilation

* spotless

* packaging tests

* disableGeoIpDownloader

* fix packaging
  • Loading branch information
probakowski authored Apr 15, 2021
1 parent f192b69 commit f178e33
Show file tree
Hide file tree
Showing 16 changed files with 62 additions and 43 deletions.
9 changes: 9 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.gradle.util.DistributionLocator
import org.gradle.util.GradleVersion
import static org.elasticsearch.gradle.util.GradleUtils.maybeConfigure
import org.gradle.plugins.ide.eclipse.model.ProjectDependency
import org.elasticsearch.gradle.testclusters.TestClustersPlugin

plugins {
id 'lifecycle-base'
Expand Down Expand Up @@ -511,3 +512,11 @@ subprojects {
}
}
}

subprojects { Project subproj ->
plugins.withType(TestClustersPlugin).whenPluginAdded {
testClusters.all {
systemProperty "geoip.downloader.enabled.default", "false"
}
}
}
2 changes: 0 additions & 2 deletions client/rest-high-level/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ tasks.named("check").configure {
testClusters.all {
testDistribution = 'DEFAULT'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
systemProperty 'es.geoip_v2_feature_flag_enabled', 'true'
setting 'geoip.downloader.enabled', 'false'
setting 'reindex.remote.whitelist', '[ "[::1]:*", "127.0.0.1:*" ]'
setting 'xpack.license.self_generated.type', 'trial'
setting 'xpack.security.enabled', 'true'
Expand Down
2 changes: 2 additions & 0 deletions distribution/docker/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ services:
- cluster.routing.allocation.disk.watermark.high=1b
- cluster.routing.allocation.disk.watermark.flood_stage=1b
- node.store.allow_mmap=false
- geoip.downloader.enabled=false
- xpack.security.enabled=true
- xpack.security.transport.ssl.enabled=true
- xpack.security.http.ssl.enabled=true
Expand Down Expand Up @@ -59,6 +60,7 @@ services:
- cluster.routing.allocation.disk.watermark.high=1b
- cluster.routing.allocation.disk.watermark.flood_stage=1b
- node.store.allow_mmap=false
- geoip.downloader.enabled=false
- xpack.security.enabled=true
- xpack.security.transport.ssl.enabled=true
- xpack.security.http.ssl.enabled=true
Expand Down
6 changes: 0 additions & 6 deletions modules/ingest-geoip/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,11 @@ if (useFixture) {
}

tasks.named("internalClusterTest").configure {
systemProperty "es.geoip_v2_feature_flag_enabled", "true"
if (useFixture) {
nonInputProperties.systemProperty "geoip_endpoint", "${-> fixtureAddress()}"
}
}

testClusters.all {
systemProperty "es.geoip_v2_feature_flag_enabled", "true"
setting "geoip.downloader.enabled", "false"
}

tasks.register("copyDefaultGeoIp2DatabaseFiles", Copy) {
from { zipTree(configurations.testCompileClasspath.files.find { it.name.contains('geolite2-databases') }) }
into "${project.buildDir}/ingest-geoip"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ public class GeoIpDownloader extends AllocatedPersistentTask {

private static final Logger logger = LogManager.getLogger(GeoIpDownloader.class);

public static final boolean GEOIP_V2_FEATURE_FLAG_ENABLED = "true".equals(System.getProperty("es.geoip_v2_feature_flag_enabled"));

public static final Setting<TimeValue> POLL_INTERVAL_SETTING = Setting.timeSetting("geoip.downloader.poll.interval",
TimeValue.timeValueDays(3), TimeValue.timeValueDays(1), Property.Dynamic, Property.NodeScope);
public static final Setting<String> ENDPOINT_SETTING = Setting.simpleString("geoip.downloader.endpoint",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@
import java.util.concurrent.atomic.AtomicReference;

import static org.elasticsearch.ingest.geoip.GeoIpDownloader.GEOIP_DOWNLOADER;
import static org.elasticsearch.ingest.geoip.GeoIpDownloader.GEOIP_V2_FEATURE_FLAG_ENABLED;

/**
* Persistent task executor that is responsible for starting {@link GeoIpDownloader} after task is allocated by master node.
* Also bootstraps GeoIP download task on clean cluster and handles changes to the 'geoip.downloader.enabled' setting
*/
public final class GeoIpDownloaderTaskExecutor extends PersistentTasksExecutor<GeoIpTaskParams> implements ClusterStateListener {

public static final Setting<Boolean> ENABLED_SETTING = Setting.boolSetting("geoip.downloader.enabled", GEOIP_V2_FEATURE_FLAG_ENABLED,
private static final boolean ENABLED_DEFAULT = "false".equals(System.getProperty("geoip.downloader.enabled.default")) == false;
public static final Setting<Boolean> ENABLED_SETTING = Setting.boolSetting("geoip.downloader.enabled", ENABLED_DEFAULT,
Setting.Property.Dynamic, Setting.Property.NodeScope);

private static final Logger logger = LogManager.getLogger(GeoIpDownloader.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
import java.io.Closeable;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -67,7 +66,6 @@
import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
import static org.elasticsearch.ingest.geoip.GeoIpDownloader.DATABASES_INDEX;
import static org.elasticsearch.ingest.geoip.GeoIpDownloader.GEOIP_DOWNLOADER;
import static org.elasticsearch.ingest.geoip.GeoIpDownloader.GEOIP_V2_FEATURE_FLAG_ENABLED;

public class IngestGeoIpPlugin extends Plugin implements IngestPlugin, SystemIndexPlugin, Closeable, PersistentTaskPlugin, ActionPlugin {
public static final Setting<Long> CACHE_SIZE =
Expand All @@ -81,13 +79,8 @@ public class IngestGeoIpPlugin extends Plugin implements IngestPlugin, SystemInd

@Override
public List<Setting<?>> getSettings() {
List<Setting<?>> settings = new ArrayList<>(Arrays.asList(CACHE_SIZE,
GeoIpDownloader.ENDPOINT_SETTING,
GeoIpDownloader.POLL_INTERVAL_SETTING));
if (GEOIP_V2_FEATURE_FLAG_ENABLED) {
settings.add(GeoIpDownloaderTaskExecutor.ENABLED_SETTING);
}
return settings;
return Arrays.asList(CACHE_SIZE, GeoIpDownloader.ENDPOINT_SETTING, GeoIpDownloader.POLL_INTERVAL_SETTING,
GeoIpDownloaderTaskExecutor.ENABLED_SETTING);
}

@Override
Expand Down Expand Up @@ -119,11 +112,9 @@ public Collection<Object> createComponents(Client client,
} catch (IOException e) {
throw new UncheckedIOException(e);
}
if (GEOIP_V2_FEATURE_FLAG_ENABLED) {
geoIpDownloaderTaskExecutor = new GeoIpDownloaderTaskExecutor(client, new HttpClient(), clusterService, threadPool);
return Arrays.asList(databaseRegistry.get(), geoIpDownloaderTaskExecutor);
}
return Collections.singletonList(databaseRegistry.get());

geoIpDownloaderTaskExecutor = new GeoIpDownloaderTaskExecutor(client, new HttpClient(), clusterService, threadPool);
return Arrays.asList(databaseRegistry.get(), geoIpDownloaderTaskExecutor);
}

@Override
Expand All @@ -135,31 +126,21 @@ public void close() throws IOException {
public List<PersistentTasksExecutor<?>> getPersistentTasksExecutor(ClusterService clusterService, ThreadPool threadPool,
Client client, SettingsModule settingsModule,
IndexNameExpressionResolver expressionResolver) {
if (GEOIP_V2_FEATURE_FLAG_ENABLED) {
return Collections.singletonList(geoIpDownloaderTaskExecutor);
} else {
return Collections.emptyList();
}
return Collections.singletonList(geoIpDownloaderTaskExecutor);
}

@Override
public List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
if (GEOIP_V2_FEATURE_FLAG_ENABLED) {
return Collections.singletonList(new ActionHandler<>(GeoIpDownloaderStatsAction.INSTANCE,
GeoIpDownloaderStatsTransportAction.class));
}
return Collections.emptyList();
return Collections.singletonList(new ActionHandler<>(GeoIpDownloaderStatsAction.INSTANCE,
GeoIpDownloaderStatsTransportAction.class));
}

@Override
public List<RestHandler> getRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings,
IndexScopedSettings indexScopedSettings, SettingsFilter settingsFilter,
IndexNameExpressionResolver indexNameExpressionResolver,
Supplier<DiscoveryNodes> nodesInCluster) {
if (GEOIP_V2_FEATURE_FLAG_ENABLED) {
return Collections.singletonList(new RestGeoIpDownloaderStatsAction());
}
return Collections.emptyList();
return Collections.singletonList(new RestGeoIpDownloaderStatsAction());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.elasticsearch.packaging.util.FileUtils.append;
import static org.elasticsearch.packaging.util.FileUtils.mv;
import static org.elasticsearch.packaging.util.FileUtils.rm;
import static org.elasticsearch.packaging.util.ServerUtils.disableGeoIpDownloader;
import static org.elasticsearch.packaging.util.ServerUtils.makeRequest;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
Expand All @@ -51,6 +52,7 @@ public static void filterDistros() {
public void test10Install() throws Exception {
installation = installArchive(sh, distribution());
verifyArchiveInstallation(installation, distribution());
disableGeoIpDownloader(installation);
}

public void test20PluginsListWithNoPlugins() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public static void filterDistros() {

@Before
public void setupTest() throws IOException {
installation = runContainer(distribution());
installation = runContainer(distribution(), builder().envVars(Collections.singletonMap("geoip.downloader.enabled", "false")));
tempDir = createTempDir(DockerTests.class.getSimpleName());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -49,6 +50,7 @@
import static org.elasticsearch.packaging.util.Packages.assertRemoved;
import static org.elasticsearch.packaging.util.Packages.installPackage;
import static org.elasticsearch.packaging.util.Packages.verifyPackageInstallation;
import static org.elasticsearch.packaging.util.ServerUtils.disableGeoIpDownloader;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
Expand All @@ -71,6 +73,8 @@ public void test10InstallArchiveDistribution() throws Exception {
installation = installArchive(sh, distribution);
verifyArchiveInstallation(installation, distribution());

disableGeoIpDownloader(installation);

final Installation.Executables bin = installation.executables();
Shell.Result r = sh.runIgnoreExitCode(bin.keystoreTool.toString() + " has-passwd");
assertFalse("has-passwd should fail", r.isSuccess());
Expand All @@ -85,6 +89,7 @@ public void test11InstallPackageDistribution() throws Exception {
installation = installPackage(sh, distribution);
assertInstalled(distribution);
verifyPackageInstallation(installation, distribution, sh);
disableGeoIpDownloader(installation);

final Installation.Executables bin = installation.executables();
Shell.Result r = sh.runIgnoreExitCode(bin.keystoreTool.toString() + " has-passwd");
Expand All @@ -98,7 +103,10 @@ public void test11InstallPackageDistribution() throws Exception {
public void test12InstallDockerDistribution() throws Exception {
assumeTrue(distribution().isDocker());

installation = Docker.runContainer(distribution());
installation = Docker.runContainer(
distribution(),
builder().envVars(Collections.singletonMap("geoip.downloader.enabled", "false"))
);

try {
waitForPathToExist(installation.config("elasticsearch.keystore"));
Expand Down Expand Up @@ -300,7 +308,9 @@ public void test60DockerEnvironmentVariablePassword() throws Exception {

// restart ES with password and mounted keystore
Map<Path, Path> volumes = singletonMap(localKeystoreFile, dockerKeystore);
Map<String, String> envVars = singletonMap("KEYSTORE_PASSWORD", password);
Map<String, String> envVars = new HashMap<>();
envVars.put("KEYSTORE_PASSWORD", password);
envVars.put("geoip.downloader.enabled", "false");
runContainer(distribution(), builder().volumes(volumes).envVars(envVars));
waitForElasticsearch(installation);
ServerUtils.runElasticsearchTests();
Expand Down Expand Up @@ -333,6 +343,7 @@ public void test61DockerEnvironmentVariablePasswordFromFile() throws Exception {

Map<String, String> envVars = new HashMap<>();
envVars.put("KEYSTORE_PASSWORD_FILE", "/run/secrets/" + passwordFilename);
envVars.put("geoip.downloader.enabled", "false");

runContainer(distribution(), builder().volumes(volumes).envVars(envVars));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import static org.elasticsearch.packaging.util.Packages.verifyPackageInstallation;
import static org.elasticsearch.packaging.util.Platforms.getOsRelease;
import static org.elasticsearch.packaging.util.Platforms.isSystemd;
import static org.elasticsearch.packaging.util.ServerUtils.disableGeoIpDownloader;
import static org.elasticsearch.packaging.util.ServerUtils.makeRequest;
import static org.elasticsearch.packaging.util.ServerUtils.runElasticsearchTests;
import static org.hamcrest.CoreMatchers.equalTo;
Expand All @@ -62,6 +63,7 @@ public void test10InstallPackage() throws Exception {
installation = installPackage(sh, distribution());
assertInstalled(distribution());
verifyPackageInstallation(installation, distribution(), sh);
disableGeoIpDownloader(installation);
}

public void test20PluginsCommandWhenNoPlugins() {
Expand Down Expand Up @@ -217,6 +219,7 @@ public void test60Reinstall() throws Exception {
install();
assertInstalled(distribution());
verifyPackageInstallation(installation, distribution(), sh);
disableGeoIpDownloader(installation);

remove(distribution());
assertRemoved(distribution());
Expand All @@ -226,6 +229,7 @@ public void test70RestartServer() throws Exception {
try {
install();
assertInstalled(distribution());
disableGeoIpDownloader(installation);

startElasticsearch();
restartElasticsearch(sh, installation);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static org.elasticsearch.packaging.util.Archives.verifyArchiveInstallation;
import static org.elasticsearch.packaging.util.FileUtils.append;
import static org.elasticsearch.packaging.util.FileUtils.mv;
import static org.elasticsearch.packaging.util.ServerUtils.disableGeoIpDownloader;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;

Expand Down Expand Up @@ -93,6 +94,7 @@ public void test10InstallArchive() throws Exception {
installation = installArchive(sh, distribution());
verifyArchiveInstallation(installation, distribution());
serviceScript = installation.bin("elasticsearch-service.bat").toString();
disableGeoIpDownloader(installation);
}

public void test11InstallServiceExeMissing() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,14 @@
import java.security.KeyStore;
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

import static java.nio.file.StandardOpenOption.APPEND;
import static java.nio.file.StandardOpenOption.CREATE;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;

Expand Down Expand Up @@ -248,4 +252,14 @@ public static String makeRequest(Request request, String username, String passwo

return body;
}

public static void disableGeoIpDownloader(Installation installation) throws IOException {
List<String> yaml = Collections.singletonList("geoip.downloader.enabled: false");
Path yml = installation.config("elasticsearch.yml");
try (Stream<String> lines = Files.readAllLines(yml).stream()) {
if (lines.noneMatch(s -> s.startsWith("geoip.downloader.enabled"))) {
Files.write(installation.config("elasticsearch.yml"), yaml, CREATE, APPEND);
}
}
}
}
2 changes: 2 additions & 0 deletions qa/remote-clusters/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ services:
- cluster.routing.allocation.disk.watermark.high=1b
- cluster.routing.allocation.disk.watermark.flood_stage=1b
- node.store.allow_mmap=false
- geoip.downloader.enabled=false
- xpack.security.enabled=true
- xpack.security.transport.ssl.enabled=true
- xpack.security.http.ssl.enabled=true
Expand Down Expand Up @@ -69,6 +70,7 @@ services:
- cluster.routing.allocation.disk.watermark.high=1b
- cluster.routing.allocation.disk.watermark.flood_stage=1b
- node.store.allow_mmap=false
- geoip.downloader.enabled=false
- xpack.security.enabled=true
- xpack.security.transport.ssl.enabled=true
- xpack.security.http.ssl.enabled=true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ public class Constants {
"cluster:monitor/data_frame/stats/get",
"cluster:monitor/eql/async/status",
"cluster:monitor/health",
"cluster:monitor/ingest/geoip/stats",
"cluster:monitor/main",
"cluster:monitor/nodes/hot_threads",
"cluster:monitor/nodes/info",
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/idp-fixture/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ services:
- cluster.routing.allocation.disk.watermark.high=1b
- cluster.routing.allocation.disk.watermark.flood_stage=1b
- node.store.allow_mmap=false
- geoip.downloader.enabled=false
- xpack.license.self_generated.type=trial
- xpack.security.enabled=true
- xpack.security.http.ssl.enabled=true
Expand Down

0 comments on commit f178e33

Please sign in to comment.