Avoid calling real maxmind endpoint from EnterpriseGeoIpDownloader #1…

…11120 (#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 #111002
elastic · Jul 22, 2024 · e8fd22f · e8fd22f
1 parent 4ed54ce
commit e8fd22f
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 39 deletions.
diff --git a/.../internalClusterTest/java/org/elasticsearch/ingest/geoip/EnterpriseGeoIpDownloaderIT.java b/.../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
@@ -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
@@ -32,16 +32,14 @@
 public class EnterpriseGeoIpHttpFixture extends ExternalResource {
 
     private final Path source;
-    private final boolean enabled;
     private final String[] databaseTypes;
     private HttpServer server;
 
     /*
      * 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 {