From e8fd22fb36ee9ee616d21439eaa7764dca396061 Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Mon, 22 Jul 2024 13:33:58 -0500 Subject: [PATCH] Avoid calling real maxmind endpoint from EnterpriseGeoIpDownloader #111120 (#111121) If the geoip_use_service system property is set to true, then the EnterpriseGeoIpHttpFixture is disabled, so EnterpriseGeoIpDownloader incorrectly calls maxmind.com without credentials, and fails. This change makes it so that the maxmind server is never called from tests. Closes https://github.com/elastic/elasticsearch/issues/111002 --- .../geoip/EnterpriseGeoIpDownloaderIT.java | 18 +++----- muted-tests.yml | 3 -- .../geoip/EnterpriseGeoIpHttpFixture.java | 42 ++++++++----------- 3 files changed, 24 insertions(+), 39 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/EnterpriseGeoIpDownloaderIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/EnterpriseGeoIpDownloaderIT.java index 2d068373717d8..cc757c413713d 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/EnterpriseGeoIpDownloaderIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/EnterpriseGeoIpDownloaderIT.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CollectionUtils; -import org.elasticsearch.core.Booleans; import org.elasticsearch.core.TimeValue; import org.elasticsearch.ingest.EnterpriseGeoIpTask; import org.elasticsearch.ingest.geoip.direct.DatabaseConfiguration; @@ -54,13 +53,12 @@ public class EnterpriseGeoIpDownloaderIT extends ESIntegTestCase { private static final String DATABASE_TYPE = "GeoIP2-City"; - private static final boolean useFixture = Booleans.parseBoolean(System.getProperty("geoip_use_service", "false")) == false; @ClassRule - public static final EnterpriseGeoIpHttpFixture fixture = new EnterpriseGeoIpHttpFixture(useFixture, DATABASE_TYPE); + public static final EnterpriseGeoIpHttpFixture fixture = new EnterpriseGeoIpHttpFixture(DATABASE_TYPE); protected String getEndpoint() { - return useFixture ? fixture.getAddress() : null; + return fixture.getAddress(); } @Override @@ -71,11 +69,9 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) { builder.setSecureSettings(secureSettings) .put(super.nodeSettings(nodeOrdinal, otherSettings)) .put(GeoIpDownloaderTaskExecutor.ENABLED_SETTING.getKey(), true); - if (getEndpoint() != null) { - // note: this is using the enterprise fixture for the regular downloader, too, as - // a slightly hacky way of making the regular downloader not actually download any files - builder.put(GeoIpDownloader.ENDPOINT_SETTING.getKey(), getEndpoint()); - } + // note: this is using the enterprise fixture for the regular downloader, too, as + // a slightly hacky way of making the regular downloader not actually download any files + builder.put(GeoIpDownloader.ENDPOINT_SETTING.getKey(), getEndpoint()); return builder.build(); } @@ -94,9 +90,7 @@ public void testEnterpriseDownloaderTask() throws Exception { * was updated with information from the database. * Note that the "enterprise database" is actually just a geolite database being loaded by the GeoIpHttpFixture. */ - if (getEndpoint() != null) { - EnterpriseGeoIpDownloader.DEFAULT_MAXMIND_ENDPOINT = getEndpoint(); - } + EnterpriseGeoIpDownloader.DEFAULT_MAXMIND_ENDPOINT = getEndpoint(); final String pipelineName = "enterprise_geoip_pipeline"; final String indexName = "enterprise_geoip_test_index"; final String sourceField = "ip"; diff --git a/muted-tests.yml b/muted-tests.yml index 92b06402f13c8..b8aeaaa442b37 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -103,9 +103,6 @@ tests: - class: org.elasticsearch.xpack.esql.spatial.SpatialPushDownGeoPointIT method: testPushedDownQueriesSingleValue issue: https://github.com/elastic/elasticsearch/issues/111084 -- class: org.elasticsearch.ingest.geoip.EnterpriseGeoIpDownloaderIT - method: testEnterpriseDownloaderTask - issue: https://github.com/elastic/elasticsearch/issues/111002 - class: org.elasticsearch.xpack.esql.spatial.SpatialPushDownCartesianPointIT method: testPushedDownQueriesSingleValue issue: https://github.com/elastic/elasticsearch/issues/110982 diff --git a/test/fixtures/geoip-fixture/src/main/java/fixture/geoip/EnterpriseGeoIpHttpFixture.java b/test/fixtures/geoip-fixture/src/main/java/fixture/geoip/EnterpriseGeoIpHttpFixture.java index 9a5205f66d1f4..5932890dd8459 100644 --- a/test/fixtures/geoip-fixture/src/main/java/fixture/geoip/EnterpriseGeoIpHttpFixture.java +++ b/test/fixtures/geoip-fixture/src/main/java/fixture/geoip/EnterpriseGeoIpHttpFixture.java @@ -32,7 +32,6 @@ public class EnterpriseGeoIpHttpFixture extends ExternalResource { private final Path source; - private final boolean enabled; private final String[] databaseTypes; private HttpServer server; @@ -40,8 +39,7 @@ public class EnterpriseGeoIpHttpFixture extends ExternalResource { * The values in databaseTypes must be in DatabaseConfiguration.MAXMIND_NAMES, and must be one of the databases copied in the * copyFiles method of thisi class. */ - public EnterpriseGeoIpHttpFixture(boolean enabled, String... databaseTypes) { - this.enabled = enabled; + public EnterpriseGeoIpHttpFixture(String... databaseTypes) { this.databaseTypes = databaseTypes; try { this.source = Files.createTempDirectory("source"); @@ -56,28 +54,26 @@ public String getAddress() { @Override protected void before() throws Throwable { - if (enabled) { - copyFiles(); - this.server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + copyFiles(); + this.server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); - // for expediency reasons, it is handy to have this test fixture be able to serve the dual purpose of actually stubbing - // out the download protocol for downloading files from maxmind (see the looped context creation after this stanza), as - // we as to serve an empty response for the geoip.elastic.co service here - this.server.createContext("/", exchange -> { - String response = "[]"; // an empty json array - exchange.sendResponseHeaders(200, response.length()); - try (OutputStream os = exchange.getResponseBody()) { - os.write(response.getBytes(StandardCharsets.UTF_8)); - } - }); - - // register the file types for the download fixture - for (String databaseType : databaseTypes) { - createContextForEnterpriseDatabase(databaseType); + // for expediency reasons, it is handy to have this test fixture be able to serve the dual purpose of actually stubbing + // out the download protocol for downloading files from maxmind (see the looped context creation after this stanza), as + // we as to serve an empty response for the geoip.elastic.co service here + this.server.createContext("/", exchange -> { + String response = "[]"; // an empty json array + exchange.sendResponseHeaders(200, response.length()); + try (OutputStream os = exchange.getResponseBody()) { + os.write(response.getBytes(StandardCharsets.UTF_8)); } + }); - server.start(); + // register the file types for the download fixture + for (String databaseType : databaseTypes) { + createContextForEnterpriseDatabase(databaseType); } + + server.start(); } private void createContextForEnterpriseDatabase(String databaseType) { @@ -108,9 +104,7 @@ private void createContextForEnterpriseDatabase(String databaseType) { @Override protected void after() { - if (enabled) { - server.stop(0); - } + server.stop(0); } private void copyFiles() throws Exception {