Add geoip service extension #3944

asifsmohammed · 2024-01-10T23:04:24Z

Description

Adds geoip_service extension which is used to configure maxmind
Adds geoip plugin to build

This PR does not remove existing config, I will make a separate PR to remove old config. It might break a lot of code.

Issues Resolved

Resolves #3941

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dlvenable

I have one initial comment. I'll try to look deeper a little later.

dlvenable · 2024-01-11T16:59:40Z

...oip-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/GeoIPProcessor.java

    super(pluginSetting);
    this.geoIPProcessorConfig = geoCodingProcessorConfig;
    this.tempPath = System.getProperty("java.io.tmpdir")+ File.separator + TEMP_PATH_FOLDER;
    geoIPProcessorService = new GeoIPProcessorService(geoCodingProcessorConfig,tempPath);
    tagsOnSourceNotFoundFailure = geoCodingProcessorConfig.getTagsOnSourceNotFoundFailure();
    this.geoIpProcessingMatchCounter = pluginMetrics.counter(GEO_IP_PROCESSING_MATCH);
    this.geoIpProcessingMismatchCounter = pluginMetrics.counter(GEO_IP_PROCESSING_MISMATCH);
+    // TODO: use this config and clean up MaxMind service config from pipeline.yaml
+    geoIpServiceConfig = geoIpConfigSupplier.getGeoIpServiceConfig();


I think that this code should want to get a GeoIpService. That is, we can have one underlying object for handling GeoIP requests.

By doing that, we'd be able to share the underlying database and structures across multiple instances of GeoIPProcessor. This can reduce the memory load in Data Prepper.

So, I think we'd want to pass into the constructor:

final GeoIpServiceProvider geoIpServiceProvider

And then call:

geoIpService = geoIpServiceProvider.getService();

Yeah, I like the suggestion. This way only one GeoIP service is created and updated once per node. I will implement in a different PR, I will expose a getGeoIpService in the GeoIpConfigSupplier interface and it will create the service using config.

I'm ok with creating in a new PR. But, if you go that route, I think we will want to remove the code that exposes the config outside of the extension. I think it will be unnecessary.

dlvenable

This looks good. Thanks for adding this extension! I have a few small comments.

By the way, the tests look good. They are a good size and very readable.

dlvenable · 2024-01-11T21:54:18Z

...ssor/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java

+        // This default constructor is used if maxmind is not configured
+    }
+
+    @AssertTrue(message = "database_refresh_interval should be between 1 and 30 days.")


You can replace this with the following annotations on getDatabaseRefreshInterval().

@DurationMin(days = 0) @DurationMax(days = 30)

Thanks for this, I looked if there is any annotations for duration but could not find it.

dlvenable · 2024-01-11T21:58:40Z

...ssor/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java

+     * @return The MaxMind database paths
+     * @since 2.7
+     */
+    public List<String> getDatabasePaths() {


Why do we have multiple paths? Is this to include the country, ASN, etc.?

Yeah, for multiple databases city, country, ASN. I will add an annotation so it can have max size of 3.

dlvenable · 2024-01-11T21:59:31Z

...test/java/org/opensearch/dataprepper/plugins/processor/extension/GeoIpServiceConfigTest.java

+        assertThat(dataPrepperConfiguration.getPipelineExtensions(), CoreMatchers.notNullValue());
+
+        final Map<String, Object> geoipServiceConfigMap = (Map<String, Object>) dataPrepperConfiguration.getPipelineExtensions().getExtensionMap().get("geoip_service");
+        System.out.println(geoipServiceConfigMap);


Please do not use println.

You can use a logger in testing if you'd really like.

dlvenable · 2024-01-11T22:01:18Z

.../src/test/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfigTest.java

+        assertThat(maxMindConfig.getDatabasePaths().size(), equalTo(3));
+    }
+
+    private void reflectivelySetField(final MaxMindConfig maxMindConfig, final String fieldName, final Object value) throws NoSuchFieldException, IllegalAccessException {


Let's use the common implementation.

data-prepper/data-prepper-test-common/src/main/java/org/opensearch/dataprepper/test/helper/ReflectivelySetField.java

Line 29 in 6648d9f

public static <T> void setField(final Class<T> configurationClass, final Object configurationObject,

dlvenable · 2024-01-11T22:04:03Z

...ssor/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java

+    private Duration databaseRefreshInterval = DEFAULT_DATABASE_REFRESH_INTERVAL;
+    @JsonProperty("cache_size")
+    @Min(1)
+    //TODO:  Add a Max limit on cache size


Do you have any thoughts on the maximum cache size? How big is each entry?

I don't have any right now, but it's something to think about as it can abused if max limit is not added

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dlvenable

Thanks @asifsmohammed . This looks good. Please correct the license header and then I'll be good to approve.

dlvenable · 2024-01-16T16:27:31Z

...a/org/opensearch/dataprepper/plugins/processor/extension/AwsAuthenticationOptionsConfig.java

@@ -1,9 +1,9 @@
 /*
 * Copyright OpenSearch Contributors
- * SPDX-License-Identifier: Apache-2.0
+ *  PDX-License-Identifier: Apache-2.0


This should be SPDX. See: https://github.com/opensearch-project/data-prepper/blob/main/CONTRIBUTING.md#java-gradlegroovy

Maybe this was a replace-all error?

This change was applied to a few files.

It's just a typo in my Intellij copyright setup, fixed it.

Signed-off-by: Asif Sohail Mohammed <[email protected]>

chenqi0805

LGTM. Assuming this missing pieces will be in a separate PR.

chenqi0805 · 2024-01-16T22:57:39Z

...ssor/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java

+    //TODO:  Add a Max limit on cache size
+    private int cacheSize = DEFAULT_CACHE_SIZE;
+
+    //TODO: Add a destination path to store database files


Not following this comment, are you intended for storing files in database_paths into different paths?

Yeah, right now it's stored in a temp folder. But it should be configured

chenqi0805 · 2024-01-16T23:03:09Z

...a/org/opensearch/dataprepper/plugins/processor/extension/DefaultGeoIpConfigSupplierTest.java

+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+class DefaultGeoIpConfigSupplierTest {


I suppose this test needs to be extended in later PR

Yes, I will be adding tests to this

Add geoip service extension

be8469d

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, dlvenable and oeyh as code owners January 10, 2024 23:04

dlvenable requested changes Jan 11, 2024

View reviewed changes

Addressed feedback and updated supplier interface

e6f7964

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dlvenable requested changes Jan 16, 2024

View reviewed changes

Fixed license typo

6cf5de4

Signed-off-by: Asif Sohail Mohammed <[email protected]>

chenqi0805 approved these changes Jan 16, 2024

View reviewed changes

dlvenable approved these changes Jan 17, 2024

View reviewed changes

asifsmohammed merged commit 29b0630 into opensearch-project:main Jan 17, 2024
72 of 74 checks passed

asifsmohammed mentioned this pull request Jan 18, 2024

Update geoip config and use extensions #3975

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add geoip service extension #3944

Add geoip service extension #3944

asifsmohammed commented Jan 10, 2024 •

edited

Loading

dlvenable left a comment

dlvenable Jan 11, 2024

asifsmohammed Jan 11, 2024

dlvenable Jan 11, 2024

dlvenable left a comment

dlvenable Jan 11, 2024

asifsmohammed Jan 12, 2024

dlvenable Jan 11, 2024

asifsmohammed Jan 12, 2024

dlvenable Jan 11, 2024

dlvenable Jan 11, 2024

dlvenable Jan 11, 2024

asifsmohammed Jan 12, 2024

dlvenable left a comment

dlvenable Jan 16, 2024

dlvenable Jan 16, 2024

asifsmohammed Jan 16, 2024

chenqi0805 left a comment

chenqi0805 Jan 16, 2024

asifsmohammed Jan 16, 2024

chenqi0805 Jan 16, 2024

asifsmohammed Jan 16, 2024

Add geoip service extension #3944

Add geoip service extension #3944

Conversation

asifsmohammed commented Jan 10, 2024 • edited Loading

Description

Issues Resolved

Check List

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifsmohammed commented Jan 10, 2024 •

edited

Loading