From da7fd4660ff672de8496479a2c4ed7f0058d94b9 Mon Sep 17 00:00:00 2001 From: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Mon, 10 Nov 2025 19:03:26 +0100 Subject: [PATCH] Fix `GeoIpDownloaderIT.testInvalidTimestamp` (#137663) The GeoIP expiration logic assumes that the md5 hashes of the databases change over time, as it deletes all database chunks from the .geoip_databases index. If the hashes do not change, no new database chunks are indexed and so the ingest nodes fail to download the databases as there are no chunks to download. In this test suite, we don't have a way to change the database files served by the fixture, and thus change their md5 hashes. Since we assume that in a real-world scenario the database files change over time, we are ok with skipping this part of the test. See https://github.com/elastic/elasticsearch/issues/128284#issuecomment-3496286781 for a detailed investigation. Closes #128284 --- .../ingest/geoip/GeoIpDownloaderIT.java | 69 +++++++++++++++---- muted-tests.yml | 3 - 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java index 5423e6b2a917f..ed4822af1de91 100644 --- a/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java +++ b/modules/ingest-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java @@ -16,18 +16,24 @@ import org.elasticsearch.action.ingest.SimulatePipelineRequest; import org.elasticsearch.action.ingest.SimulatePipelineResponse; import org.elasticsearch.client.internal.Client; +import org.elasticsearch.cluster.metadata.ProjectMetadata; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.IOUtils; import org.elasticsearch.core.SuppressForbidden; import org.elasticsearch.core.TimeValue; import org.elasticsearch.env.Environment; +import org.elasticsearch.index.IndexMode; +import org.elasticsearch.index.IndexSettingProvider; import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.MatchQueryBuilder; import org.elasticsearch.index.query.RangeQueryBuilder; +import org.elasticsearch.indices.IndicesRequestCache; import org.elasticsearch.ingest.AbstractProcessor; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.Processor; @@ -51,6 +57,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.time.Instant; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -69,6 +76,7 @@ import static org.elasticsearch.ingest.IngestPipelineTestUtils.jsonSimulatePipelineRequest; import static org.elasticsearch.ingest.geoip.GeoIpTestUtils.copyDefaultDatabases; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoSearchHits; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.anEmptyMap; @@ -78,7 +86,6 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasItem; -import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -93,7 +100,8 @@ protected Collection> nodePlugins() { ReindexPlugin.class, IngestGeoIpPlugin.class, GeoIpProcessorNonIngestNodeIT.IngestGeoIpSettingsPlugin.class, - NonGeoProcessorsPlugin.class + NonGeoProcessorsPlugin.class, + GeoIpIndexSettingProviderPlugin.class ); } @@ -186,6 +194,11 @@ public void testInvalidTimestamp() throws Exception { } } }); + // We wait for the deletion of the database chunks in the .geoip_databases index. Since the search request cache is disabled by + // GeoIpIndexSettingProvider, we can be sure that all nodes are unable to fetch the deleted chunks from the cache. + logger.info("---> waiting for database chunks to be deleted"); + assertBusy(() -> assertNoSearchHits(prepareSearch(GeoIpDownloader.DATABASES_INDEX).setRequestCache(false))); + logger.info("---> database chunks deleted"); putGeoIpPipeline(); assertBusy(() -> { SimulateDocumentBaseResult result = simulatePipeline(); @@ -204,19 +217,12 @@ public void testInvalidTimestamp() throws Exception { assertFalse(result.getIngestDocument().hasField("ip-asn")); assertFalse(result.getIngestDocument().hasField("ip-country")); }); - updateClusterSettings(Settings.builder().putNull("ingest.geoip.database_validity")); - assertBusy(() -> { - for (Path geoIpTmpDir : geoIpTmpDirs) { - try (Stream files = Files.list(geoIpTmpDir)) { - Set names = files.map(f -> f.getFileName().toString()).collect(Collectors.toSet()); - assertThat( - names, - hasItems("GeoLite2-ASN.mmdb", "GeoLite2-City.mmdb", "GeoLite2-Country.mmdb", "MyCustomGeoLite2-City.mmdb") - ); - } - } - }); - awaitAllNodesDownloadedDatabases(); + // Ideally, we would reset the validity setting to its default value (30d) here and verify that the databases are downloaded again. + // However, this expiration logic assumes that the md5 hashes of the databases change over time, as it deletes all database chunks + // from the .geoip_databases index. If the hashes do not change, no new database chunks are indexed and so the ingest nodes fail + // to download the databases as there are no chunks to download. + // In this test suite, we don't have a way to change the database files served by the fixture, and thus change their md5 hashes. + // Since we assume that in a real world scenario the database files change over time, we are ok with skipping this part of the test. } @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/92888") @@ -855,4 +861,37 @@ public String getType() { return procMap; } } + + /** + * A simple plugin that provides the {@link GeoIpIndexSettingProvider}. + */ + public static final class GeoIpIndexSettingProviderPlugin extends Plugin { + @Override + public Collection getAdditionalIndexSettingProviders(IndexSettingProvider.Parameters parameters) { + return List.of(new GeoIpIndexSettingProvider()); + } + } + + /** + * An index setting provider that disables the request cache for the `.geoip_databases` index. + * Since `.geoip_databases` is a system index, we can't configure this setting using the API or index templates. + */ + public static final class GeoIpIndexSettingProvider implements IndexSettingProvider { + @Override + public void provideAdditionalSettings( + String indexName, + String dataStreamName, + IndexMode templateIndexMode, + ProjectMetadata projectMetadata, + Instant resolvedAt, + Settings indexTemplateAndCreateRequestSettings, + List combinedTemplateMappings, + IndexVersion indexVersion, + Settings.Builder additionalSettings + ) { + if (GeoIpDownloader.GEOIP_DOWNLOADER.equals(indexName)) { + additionalSettings.put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), false); + } + } + } } diff --git a/muted-tests.yml b/muted-tests.yml index 4571d1267cd65..a208fd9ba0bf9 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -233,9 +233,6 @@ tests: - class: org.elasticsearch.packaging.test.EnrollmentProcessTests method: test20DockerAutoFormCluster issue: https://github.com/elastic/elasticsearch/issues/128113 -- class: org.elasticsearch.ingest.geoip.GeoIpDownloaderCliIT - method: testInvalidTimestamp - issue: https://github.com/elastic/elasticsearch/issues/128284 - class: org.elasticsearch.packaging.test.TemporaryDirectoryConfigTests method: test21AcceptsCustomPathInDocker issue: https://github.com/elastic/elasticsearch/issues/128114