From f178e33f6bba46c503be87996503f8a24709a6a9 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Thu, 15 Apr 2021 18:45:53 +0200 Subject: [PATCH] [7.x] Enable GeoIP downloader by default (#71505) (#71733) * Enable GeoIP downloader by default (#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 #68920 * fix compilation * spotless * packaging tests * disableGeoIpDownloader * fix packaging --- build.gradle | 9 +++++ client/rest-high-level/build.gradle | 2 - distribution/docker/docker-compose.yml | 2 + modules/ingest-geoip/build.gradle | 6 --- .../ingest/geoip/GeoIpDownloader.java | 2 - .../geoip/GeoIpDownloaderTaskExecutor.java | 4 +- .../ingest/geoip/IngestGeoIpPlugin.java | 37 +++++-------------- .../packaging/test/ArchiveTests.java | 2 + .../packaging/test/DockerTests.java | 2 +- .../test/KeystoreManagementTests.java | 15 +++++++- .../packaging/test/PackageTests.java | 4 ++ .../packaging/test/WindowsServiceTests.java | 2 + .../packaging/util/ServerUtils.java | 14 +++++++ qa/remote-clusters/docker-compose.yml | 2 + .../xpack/security/operator/Constants.java | 1 + x-pack/test/idp-fixture/docker-compose.yml | 1 + 16 files changed, 62 insertions(+), 43 deletions(-) diff --git a/build.gradle b/build.gradle index b426535bfebfa..c16c2e563e58a 100644 --- a/build.gradle +++ b/build.gradle @@ -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' @@ -511,3 +512,11 @@ subprojects { } } } + +subprojects { Project subproj -> + plugins.withType(TestClustersPlugin).whenPluginAdded { + testClusters.all { + systemProperty "geoip.downloader.enabled.default", "false" + } + } +} diff --git a/client/rest-high-level/build.gradle b/client/rest-high-level/build.gradle index e21e66f5de92d..39edc5033aa2c 100644 --- a/client/rest-high-level/build.gradle +++ b/client/rest-high-level/build.gradle @@ -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' diff --git a/distribution/docker/docker-compose.yml b/distribution/docker/docker-compose.yml index 62704cc6510c8..ae4f5ad3e8811 100644 --- a/distribution/docker/docker-compose.yml +++ b/distribution/docker/docker-compose.yml @@ -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 @@ -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 diff --git a/modules/ingest-geoip/build.gradle b/modules/ingest-geoip/build.gradle index 8920f3bc47a3e..eced11cf01253 100644 --- a/modules/ingest-geoip/build.gradle +++ b/modules/ingest-geoip/build.gradle @@ -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" diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java index b42f4c874eec3..73a8e44a3280c 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java @@ -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 POLL_INTERVAL_SETTING = Setting.timeSetting("geoip.downloader.poll.interval", TimeValue.timeValueDays(3), TimeValue.timeValueDays(1), Property.Dynamic, Property.NodeScope); public static final Setting ENDPOINT_SETTING = Setting.simpleString("geoip.downloader.endpoint", diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java index 7a28f6800fc73..ffafa3d7ca3e0 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderTaskExecutor.java @@ -30,7 +30,6 @@ 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. @@ -38,7 +37,8 @@ */ public final class GeoIpDownloaderTaskExecutor extends PersistentTasksExecutor implements ClusterStateListener { - public static final Setting 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 ENABLED_SETTING = Setting.boolSetting("geoip.downloader.enabled", ENABLED_DEFAULT, Setting.Property.Dynamic, Setting.Property.NodeScope); private static final Logger logger = LogManager.getLogger(GeoIpDownloader.class); diff --git a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java index c6374cb920dd3..b1190f1e738d1 100644 --- a/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java +++ b/modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/IngestGeoIpPlugin.java @@ -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; @@ -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 CACHE_SIZE = @@ -81,13 +79,8 @@ public class IngestGeoIpPlugin extends Plugin implements IngestPlugin, SystemInd @Override public List> getSettings() { - List> 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 @@ -119,11 +112,9 @@ public Collection 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 @@ -135,20 +126,13 @@ public void close() throws IOException { public List> 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> 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 @@ -156,10 +140,7 @@ public List getRestHandlers(Settings settings, RestController restC IndexScopedSettings indexScopedSettings, SettingsFilter settingsFilter, IndexNameExpressionResolver indexNameExpressionResolver, Supplier nodesInCluster) { - if (GEOIP_V2_FEATURE_FLAG_ENABLED) { - return Collections.singletonList(new RestGeoIpDownloaderStatsAction()); - } - return Collections.emptyList(); + return Collections.singletonList(new RestGeoIpDownloaderStatsAction()); } @Override diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java index 66094e52b87d7..e74ad51cc628c 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java @@ -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; @@ -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 { diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index f60375fb69adc..cb961abed7805 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -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()); } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java index bba91602a5e9f..c46f7eee1cb45 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/KeystoreManagementTests.java @@ -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; @@ -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; @@ -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()); @@ -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"); @@ -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")); @@ -300,7 +308,9 @@ public void test60DockerEnvironmentVariablePassword() throws Exception { // restart ES with password and mounted keystore Map volumes = singletonMap(localKeystoreFile, dockerKeystore); - Map envVars = singletonMap("KEYSTORE_PASSWORD", password); + Map 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(); @@ -333,6 +343,7 @@ public void test61DockerEnvironmentVariablePasswordFromFile() throws Exception { Map envVars = new HashMap<>(); envVars.put("KEYSTORE_PASSWORD_FILE", "/run/secrets/" + passwordFilename); + envVars.put("geoip.downloader.enabled", "false"); runContainer(distribution(), builder().volumes(volumes).envVars(envVars)); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java index 5c7fddc6e3f4b..2b947917e77fc 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java @@ -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; @@ -62,6 +63,7 @@ public void test10InstallPackage() throws Exception { installation = installPackage(sh, distribution()); assertInstalled(distribution()); verifyPackageInstallation(installation, distribution(), sh); + disableGeoIpDownloader(installation); } public void test20PluginsCommandWhenNoPlugins() { @@ -217,6 +219,7 @@ public void test60Reinstall() throws Exception { install(); assertInstalled(distribution()); verifyPackageInstallation(installation, distribution(), sh); + disableGeoIpDownloader(installation); remove(distribution()); assertRemoved(distribution()); @@ -226,6 +229,7 @@ public void test70RestartServer() throws Exception { try { install(); assertInstalled(distribution()); + disableGeoIpDownloader(installation); startElasticsearch(); restartElasticsearch(sh, installation); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/WindowsServiceTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/WindowsServiceTests.java index 24f6f63f3a522..dcfe0d8943148 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/WindowsServiceTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/WindowsServiceTests.java @@ -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; @@ -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 { diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java index 5139f512c849f..669c5c8c69d92 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java @@ -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; @@ -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 yaml = Collections.singletonList("geoip.downloader.enabled: false"); + Path yml = installation.config("elasticsearch.yml"); + try (Stream lines = Files.readAllLines(yml).stream()) { + if (lines.noneMatch(s -> s.startsWith("geoip.downloader.enabled"))) { + Files.write(installation.config("elasticsearch.yml"), yaml, CREATE, APPEND); + } + } + } } diff --git a/qa/remote-clusters/docker-compose.yml b/qa/remote-clusters/docker-compose.yml index bcbbe9c37d259..ed5dd34417426 100644 --- a/qa/remote-clusters/docker-compose.yml +++ b/qa/remote-clusters/docker-compose.yml @@ -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 @@ -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 diff --git a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java index 27c89ce916dba..26a3a7051859b 100644 --- a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java +++ b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java @@ -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", diff --git a/x-pack/test/idp-fixture/docker-compose.yml b/x-pack/test/idp-fixture/docker-compose.yml index 5c6bc0762217e..193112e2c21bb 100644 --- a/x-pack/test/idp-fixture/docker-compose.yml +++ b/x-pack/test/idp-fixture/docker-compose.yml @@ -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