-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add geoip service extension #3944
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
public class DefaultGeoIpConfigSupplier implements GeoIpConfigSupplier { | ||
private final GeoIpServiceConfig geoIpServiceConfig; | ||
|
||
public DefaultGeoIpConfigSupplier(final GeoIpServiceConfig geoIpServiceConfig) { | ||
this.geoIpServiceConfig = geoIpServiceConfig; | ||
} | ||
|
||
@Override | ||
public GeoIpServiceConfig getGeoIpServiceConfig() { | ||
return this.geoIpServiceConfig; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import org.opensearch.dataprepper.model.annotations.DataPrepperExtensionPlugin; | ||
import org.opensearch.dataprepper.model.annotations.DataPrepperPluginConstructor; | ||
import org.opensearch.dataprepper.model.plugin.ExtensionPlugin; | ||
import org.opensearch.dataprepper.model.plugin.ExtensionPoints; | ||
|
||
@DataPrepperExtensionPlugin(modelType = GeoIpServiceConfig.class, rootKeyJsonPath = "/geoip_service") | ||
public class GeoIpConfigExtension implements ExtensionPlugin { | ||
private final DefaultGeoIpConfigSupplier defaultGeoIpConfigSupplier; | ||
|
||
@DataPrepperPluginConstructor | ||
public GeoIpConfigExtension(final GeoIpServiceConfig geoIpServiceConfig) { | ||
this.defaultGeoIpConfigSupplier = new DefaultGeoIpConfigSupplier(geoIpServiceConfig != null ? geoIpServiceConfig : new GeoIpServiceConfig()); | ||
} | ||
|
||
@Override | ||
public void apply(final ExtensionPoints extensionPoints) { | ||
extensionPoints.addExtensionProvider(new GeoIpConfigProvider(this.defaultGeoIpConfigSupplier)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import org.opensearch.dataprepper.model.plugin.ExtensionProvider; | ||
|
||
import java.util.Optional; | ||
|
||
public class GeoIpConfigProvider implements ExtensionProvider<GeoIpConfigSupplier> { | ||
private final GeoIpConfigSupplier geoIpConfigSupplier; | ||
|
||
public GeoIpConfigProvider(final GeoIpConfigSupplier geoIpConfigSupplier) { | ||
this.geoIpConfigSupplier = geoIpConfigSupplier; | ||
} | ||
|
||
@Override | ||
public Optional<GeoIpConfigSupplier> provideInstance(Context context) { | ||
return Optional.of(this.geoIpConfigSupplier); | ||
} | ||
|
||
@Override | ||
public Class<GeoIpConfigSupplier> supportedClass() { | ||
return GeoIpConfigSupplier.class; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
public interface GeoIpConfigSupplier { | ||
GeoIpServiceConfig getGeoIpServiceConfig(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
import jakarta.validation.Valid; | ||
|
||
public class GeoIpServiceConfig { | ||
private static final MaxMindConfig DEFAULT_MAXMIND_CONFIG = new MaxMindConfig(); | ||
|
||
public GeoIpServiceConfig() { | ||
// This default constructor is used if geoip_service is not configured | ||
} | ||
|
||
@JsonProperty("maxmind") | ||
@Valid | ||
private MaxMindConfig maxMindConfig = DEFAULT_MAXMIND_CONFIG; | ||
|
||
/** | ||
* Gets the configuration for MaxMind. | ||
* | ||
* @return The MaxMind configuration | ||
* @since 2.7 | ||
*/ | ||
public MaxMindConfig getMaxMindConfig() { | ||
return maxMindConfig; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
import jakarta.validation.constraints.AssertTrue; | ||
import jakarta.validation.constraints.Min; | ||
|
||
import java.time.Duration; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class MaxMindConfig { | ||
//TODO: Add validations to database paths | ||
//TODO: Make default path to be a public CDN endpoint | ||
private static final List<String> DEFAULT_DATABASE_PATHS = new ArrayList<>(); | ||
private static final Duration DEFAULT_DATABASE_REFRESH_INTERVAL = Duration.ofDays(7); | ||
private static final int DEFAULT_CACHE_SIZE = 4096; | ||
|
||
@JsonProperty("database_paths") | ||
private List<String> databasePaths = DEFAULT_DATABASE_PATHS; | ||
@JsonProperty("database_refresh_interval") | ||
private Duration databaseRefreshInterval = DEFAULT_DATABASE_REFRESH_INTERVAL; | ||
@JsonProperty("cache_size") | ||
@Min(1) | ||
//TODO: Add a Max limit on cache size | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any thoughts on the maximum cache size? How big is each entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have any right now, but it's something to think about as it can abused if max limit is not added |
||
private int cacheSize = DEFAULT_CACHE_SIZE; | ||
//TODO: Add a destination path to store database files | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not following this comment, are you intended for storing files in database_paths into different paths? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, right now it's stored in a temp folder. But it should be configured |
||
|
||
public MaxMindConfig() { | ||
// This default constructor is used if maxmind is not configured | ||
} | ||
|
||
@AssertTrue(message = "database_refresh_interval should be between 1 and 30 days.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can replace this with the following annotations on
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this, I looked if there is any annotations for duration but could not find it. |
||
public boolean isValidDatabaseRefreshInterval() { | ||
return databaseRefreshInterval.toDays() >= 1 && databaseRefreshInterval.toDays() <= 30; | ||
} | ||
|
||
/** | ||
* Gets the MaxMind database paths | ||
* | ||
* @return The MaxMind database paths | ||
* @since 2.7 | ||
*/ | ||
public List<String> getDatabasePaths() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have multiple paths? Is this to include the country, ASN, etc.? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, for multiple databases city, country, ASN. I will add an annotation so it can have max size of 3. |
||
return databasePaths; | ||
} | ||
|
||
/** | ||
* Gets the database refresh duration. This loads the database from the paths into memory again in case if there are any updates. | ||
* | ||
* @return The refresh duration | ||
* @since 2.7 | ||
*/ | ||
public Duration getDatabaseRefreshInterval() { | ||
return databaseRefreshInterval; | ||
} | ||
|
||
/** | ||
* Gets the cache size used in CHM cache for MaxMind DatabaseReader. | ||
* | ||
* @return The cache size | ||
* @since 2.7 | ||
*/ | ||
public int getCacheSize() { | ||
return cacheSize; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.mockito.Mock; | ||
|
||
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
|
||
class DefaultGeoIpConfigSupplierTest { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose this test needs to be extended in later PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I will be adding tests to this |
||
@Mock | ||
private GeoIpServiceConfig geoIpServiceConfig; | ||
|
||
private DefaultGeoIpConfigSupplier createObjectUnderTest() { | ||
return new DefaultGeoIpConfigSupplier(geoIpServiceConfig); | ||
} | ||
|
||
@Test | ||
void testGetGeoIpServiceConfig() { | ||
assertThat(createObjectUnderTest().getGeoIpServiceConfig(), equalTo(geoIpServiceConfig)); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* PDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package org.opensearch.dataprepper.plugins.processor.extension; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
import org.mockito.ArgumentCaptor; | ||
import org.mockito.Mock; | ||
import org.mockito.junit.jupiter.MockitoExtension; | ||
import org.opensearch.dataprepper.model.plugin.ExtensionPoints; | ||
import org.opensearch.dataprepper.model.plugin.ExtensionProvider; | ||
|
||
import java.time.Duration; | ||
|
||
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.hamcrest.CoreMatchers.instanceOf; | ||
import static org.hamcrest.CoreMatchers.notNullValue; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.mockito.Mockito.verify; | ||
|
||
@ExtendWith(MockitoExtension.class) | ||
class GeoIpConfigExtensionTest { | ||
@Mock | ||
private ExtensionPoints extensionPoints; | ||
|
||
@Mock | ||
private GeoIpServiceConfig geoIpServiceConfig; | ||
|
||
@Mock | ||
private ExtensionProvider.Context context; | ||
|
||
private GeoIpConfigExtension createObjectUnderTest() { | ||
return new GeoIpConfigExtension(geoIpServiceConfig); | ||
} | ||
|
||
@Test | ||
void apply_should_addExtensionProvider() { | ||
createObjectUnderTest().apply(extensionPoints); | ||
final ArgumentCaptor<ExtensionProvider> extensionProviderArgumentCaptor = | ||
ArgumentCaptor.forClass(ExtensionProvider.class); | ||
|
||
verify(extensionPoints).addExtensionProvider(extensionProviderArgumentCaptor.capture()); | ||
|
||
final ExtensionProvider actualExtensionProvider = extensionProviderArgumentCaptor.getValue(); | ||
|
||
assertThat(actualExtensionProvider, instanceOf(GeoIpConfigProvider.class)); | ||
} | ||
|
||
@Test | ||
void apply_should_addExtensionProvider_and_supplier_should_use_default_config_if_not_configured() { | ||
final GeoIpConfigExtension geoIpConfigExtension = new GeoIpConfigExtension(null); | ||
|
||
geoIpConfigExtension.apply(extensionPoints); | ||
final ArgumentCaptor<ExtensionProvider> extensionProviderArgumentCaptor = | ||
ArgumentCaptor.forClass(ExtensionProvider.class); | ||
|
||
verify(extensionPoints).addExtensionProvider(extensionProviderArgumentCaptor.capture()); | ||
|
||
final ExtensionProvider actualExtensionProvider = extensionProviderArgumentCaptor.getValue(); | ||
|
||
assertThat(actualExtensionProvider, instanceOf(GeoIpConfigProvider.class)); | ||
|
||
final GeoIpConfigSupplier geoIpConfigSupplier = (GeoIpConfigSupplier) actualExtensionProvider.provideInstance(context).get(); | ||
|
||
final GeoIpServiceConfig geoIpServiceConfig = geoIpConfigSupplier.getGeoIpServiceConfig(); | ||
assertThat(geoIpServiceConfig, notNullValue()); | ||
|
||
final MaxMindConfig maxMindConfig = geoIpServiceConfig.getMaxMindConfig(); | ||
assertThat(maxMindConfig, notNullValue()); | ||
assertThat(maxMindConfig.getCacheSize(), equalTo(4096)); | ||
assertThat(maxMindConfig.getDatabasePaths().size(), equalTo(0)); | ||
assertThat(maxMindConfig.getDatabaseRefreshInterval(), equalTo(Duration.ofDays(7))); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
And then call:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theGeoIpConfigSupplier
interface and it will create the service using config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.