Change GeoIP downloader policy after 30 days of no updates #74099

probakowski · 2021-06-14T18:34:49Z

This PR changes the way GeoIpDownloader and GeoIpProcessor handle situation when we are unable to update databases for 30 days. In that case:

GeoIpDownloader will delete all chunks from .geoip_databases index
DatabaseRegistry will delete all files on ingest nodes
GeoIpProcessor will tag document with tags: ["_geoip_expired_database"] field (same way as in Logstash)

This change also fixes bug with that breaks DatabaseRegistry and when it tires to download databases after updating timestamp only (GeoIpDownloader checks if there are new databases and updates timestamp because local databases are up to date)

elasticmachine · 2021-06-14T18:34:52Z

Pinging @elastic/es-core-features (Team:Core/Features)

probakowski · 2021-06-14T18:43:46Z

@elasticmachine update branch

martijnvg

Thanks @probakowski! I left a few questions.

modules/ingest-geoip/build.gradle

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpProcessor.java

martijnvg · 2021-06-16T09:18:59Z

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java

@@ -63,13 +67,16 @@
        TimeValue.timeValueDays(3), TimeValue.timeValueDays(1), Property.Dynamic, Property.NodeScope);
    public static final Setting<String> ENDPOINT_SETTING = Setting.simpleString("ingest.geoip.downloader.endpoint",
        "https://geoip.elastic.co/v1/database", Property.NodeScope);
+    public static final Setting<TimeValue> DATABASE_VALIDITY = Setting.timeSetting("ingest.geoip.database.validity",


I see that this constant is currently unused. The setting it self is used in testing, and I assume that this setting's only purpose is for testing? If so then we need to make sure that this setting can't be set in production, because otherwise the 30 days check can be circumvented. Is this the case?

I removed the constant as it was leftover, but similar setting (ingest.geoip.database_validity) is used by tests as you said. It's registered only by test module (similarly to ingest.geoip.database_path) so it shouldn't be available in production

martijnvg · 2021-06-16T10:07:33Z

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpDownloader.java

-        state = state.put(name, new Metadata(System.currentTimeMillis(), old.getFirstChunk(), old.getLastChunk(), old.getMd5()));
+        long timestamp = System.currentTimeMillis();
+        UpdateByQueryRequest request = new UpdateByQueryRequest(DATABASES_INDEX).setRefresh(true);
+        request.setScript(new Script("ctx._source.timestamp=" + timestamp + "L"));


Could we record the timestamp elsewhere instead of adding a field to the chunk documents and updating all these documents? Maybe store it in the cluster state as mapping metadata or custom data in IndexMetadata? Under the hood this results in reindexing all the chunk documents, which would be nice if this could be avoided.

Yeah, I've added additional field in GeoIpTaskState.Metadata to use in validity test instead of updating timestamp and it does simplify code greatly. Thanks for suggestion!

That is great! 👍

modules/ingest-geoip/src/test/resources/plugin-security.policy

martijnvg

Looks great! I left a few more comments.

Also I think we should add a section to the documentation that describes when a database can become invalid and what the behaviour is of the geoip processor in that case. Doing this in a followup pr is fine with me too.

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpProcessor.java

martijnvg · 2021-06-17T08:38:48Z

...est-geoip/src/internalClusterTest/java/org/elasticsearch/ingest/geoip/GeoIpDownloaderIT.java

+            client().admin().cluster()
+                .prepareUpdateSettings()
+                .setPersistentSettings(Settings.builder()
+                    .put("ingest.geoip.database_validity", TimeValue.timeValueMillis(1))


Can this test be adjust (or add another version of this test), so that we first test that ingestion works as expected and then we change ingest.geoip.database_validity to 1ms and expect that _geoip_expired_database tag is added during ingestion?

Additionally can we also verify that each node doesn't have any local database files any more after the databases have become invalid?

Good point about verifying that ingestion works prior invalidation, I've added it.
Check for no local database files is there already, around line 129

modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorTests.java

modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/DatabaseRegistry.java

probakowski · 2021-06-17T19:49:45Z

Thanks for the review @martijnvg

Also I think we should add a section to the documentation that describes when a database can become invalid and what the behaviour is of the geoip processor in that case. Doing this in a followup pr is fine with me too.

I think we will need to add it in PR that will enable geoip downloader again - only then database can get invalid

martijnvg

LGTM

…4099) This PR changes the way GeoIpDownloader and GeoIpProcessor handle situation when we are unable to update databases for 30 days. In that case: GeoIpDownloader will delete all chunks from .geoip_databases index DatabaseRegistry will delete all files on ingest nodes GeoIpProcessor will tag document with tags: ["_geoip_expired_database"] field (same way as in Logstash) This change also fixes bug with that breaks DatabaseRegistry and when it tires to download databases after updating timestamp only (GeoIpDownloader checks if there are new databases and updates timestamp because local databases are up to date)

) (#74296) * Change GeoIP downloader policy after 30 days of no updates (#74099) This PR changes the way GeoIpDownloader and GeoIpProcessor handle situation when we are unable to update databases for 30 days. In that case: GeoIpDownloader will delete all chunks from .geoip_databases index DatabaseRegistry will delete all files on ingest nodes GeoIpProcessor will tag document with tags: ["_geoip_expired_database"] field (same way as in Logstash) This change also fixes bug with that breaks DatabaseRegistry and when it tires to download databases after updating timestamp only (GeoIpDownloader checks if there are new databases and updates timestamp because local databases are up to date) * fix compilation

probakowski added 8 commits June 10, 2021 23:14

geoip rework

5ead2e4

Merge branch 'master' into geoip_rework

8d26a44

fix

939eb21

Merge remote-tracking branch 'origin/master' into geoip_rework

b07daa7

imports

84fc408

Remove unneeded method

8d1afe9

imports

63b895f

termination

0ddc6a4

probakowski added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v8.0.0 v7.14.0 labels Jun 14, 2021

probakowski requested a review from martijnvg June 14, 2021 18:34

elasticmachine added the Team:Data Management Meta label for data/management team label Jun 14, 2021

Merge branch 'master' into geoip_rework

424390f

martijnvg reviewed Jun 16, 2021

View reviewed changes

probakowski added 2 commits June 16, 2021 23:22

rework

c8ab28a

rework

5a449db

probakowski force-pushed the geoip_rework branch from 10dcb7d to 5a449db Compare June 16, 2021 21:23

probakowski added 2 commits June 16, 2021 23:26

review

afef431

Merge remote-tracking branch 'origin/master' into geoip_rework

2b03239

probakowski requested a review from martijnvg June 16, 2021 23:30

martijnvg reviewed Jun 17, 2021

View reviewed changes

probakowski added 6 commits June 17, 2021 17:34

tests

b4787e4

cleanup

92f8d1f

cleanup

af474eb

cleanup

ffbee24

cleanup

418252b

check -> assert

8125153

probakowski requested a review from martijnvg June 17, 2021 19:48

martijnvg approved these changes Jun 18, 2021

View reviewed changes

probakowski merged commit 331a44b into elastic:master Jun 18, 2021

probakowski deleted the geoip_rework branch June 18, 2021 11:51

probakowski mentioned this pull request Jun 18, 2021

[7.x] Change GeoIP downloader policy after 30 days of no updates (#74099) #74296

Merged

probakowski added the backport pending label Jun 18, 2021

probakowski removed the backport pending label Jun 18, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

probakowski added the >enhancement label Jul 30, 2021

Change GeoIP downloader policy after 30 days of no updates #74099

Change GeoIP downloader policy after 30 days of no updates #74099

Uh oh!

Conversation

probakowski commented Jun 14, 2021

Uh oh!

elasticmachine commented Jun 14, 2021

Uh oh!

probakowski commented Jun 14, 2021

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

probakowski Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

probakowski commented Jun 17, 2021

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

probakowski Jun 17, 2021 •

edited

Loading