Addressed feedback

Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed · Feb 19, 2024 · 3493565 · 3493565
1 parent 0cdbdad
commit 3493565
Show file tree

Hide file tree

Showing 11 changed files with 39 additions and 18 deletions.
diff --git a/...-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/GeoIPProcessor.java b/...-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/GeoIPProcessor.java
@@ -33,7 +33,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 /**
@@ -69,8 +68,10 @@ public GeoIPProcessor(final PluginMetrics pluginMetrics,
                         final GeoIpConfigSupplier geoIpConfigSupplier,
                         final ExpressionEvaluator expressionEvaluator) {
     super(pluginMetrics);
-    this.geoIPProcessorService = geoIpConfigSupplier.getGeoIPProcessorService();
-    Objects.requireNonNull(geoIPProcessorService, "geoip_service configuration is required when using geoip processor.");
+    if (geoIpConfigSupplier.getGeoIPProcessorService().isEmpty()) {
+      throw new RuntimeException("geoip_service configuration is required when using geoip processor.");
+    }
+    this.geoIPProcessorService = geoIpConfigSupplier.getGeoIPProcessorService().get();
     this.geoIPProcessorConfig = geoIPProcessorConfig;
     this.tagsOnFailure = geoIPProcessorConfig.getTagsOnFailure();
     this.expressionEvaluator = expressionEvaluator;

diff --git a/...ava/org/opensearch/dataprepper/plugins/processor/databaseenrich/GeoIP2DatabaseReader.java b/...ava/org/opensearch/dataprepper/plugins/processor/databaseenrich/GeoIP2DatabaseReader.java
@@ -156,7 +156,7 @@ public void retain() {
     public void close() {
         final int count = closeCount.decrementAndGet();
         if (count == 0) {
-            LOG.info("Closing old geoip database readers");
+            LOG.debug("Closing old geoip database readers");
             closeReader();
         }
     }

diff --git a/...a/org/opensearch/dataprepper/plugins/processor/databaseenrich/GeoLite2DatabaseReader.java b/...a/org/opensearch/dataprepper/plugins/processor/databaseenrich/GeoLite2DatabaseReader.java
@@ -187,7 +187,7 @@ public void retain() {
     public void close() {
         final int count = closeCount.decrementAndGet();
         if (count == 0) {
-            LOG.info("Closing old geoip database readers");
+            LOG.debug("Closing old geoip database readers");
             closeReaders();
         }
     }

diff --git a/...va/org/opensearch/dataprepper/plugins/processor/extension/DefaultGeoIpConfigSupplier.java b/...va/org/opensearch/dataprepper/plugins/processor/extension/DefaultGeoIpConfigSupplier.java
@@ -7,6 +7,7 @@
 
 import org.opensearch.dataprepper.plugins.processor.extension.databasedownload.GeoIPDatabaseManager;
 
+import java.util.Optional;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 public class DefaultGeoIpConfigSupplier implements GeoIpConfigSupplier {
@@ -24,10 +25,10 @@ public DefaultGeoIpConfigSupplier(final GeoIpServiceConfig geoIpServiceConfig,
     }
 
     @Override
-    public GeoIPProcessorService getGeoIPProcessorService() {
+    public Optional<GeoIPProcessorService> getGeoIPProcessorService() {
         if (geoIpServiceConfig != null)
-            return new GeoIPProcessorService(geoIpServiceConfig, geoIPDatabaseManager, readLock);
+            return Optional.of(new GeoIPProcessorService(geoIpServiceConfig, geoIPDatabaseManager, readLock));
         else
-            return null;
+            return Optional.empty();
     }
 }
diff --git a/...main/java/org/opensearch/dataprepper/plugins/processor/extension/GeoIpConfigSupplier.java b/...main/java/org/opensearch/dataprepper/plugins/processor/extension/GeoIpConfigSupplier.java
@@ -5,6 +5,8 @@
 
 package org.opensearch.dataprepper.plugins.processor.extension;
 
+import java.util.Optional;
+
 /**
  * Interface for supplying {@link GeoIPProcessorService} to {@link GeoIpConfigExtension}
  *
@@ -16,5 +18,5 @@ public interface GeoIpConfigSupplier {
      *
      * @since 2.7
      */
-    GeoIPProcessorService getGeoIPProcessorService();
+    Optional<GeoIPProcessorService> getGeoIPProcessorService();
 }
diff --git a/...r/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java b/...r/src/main/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfig.java
@@ -69,7 +69,7 @@ public boolean isAwsAuthenticationOptionsRequired() {
     }
 
     @AssertTrue(message = "database_paths should be https endpoint if using URL and if insecure is set to false")
-    public boolean isSecureEndpoint() throws URISyntaxException {
+    public boolean isHttpsEndpointOrInsecure() throws URISyntaxException {
         if (insecure) {
             return true;
         }
@@ -128,6 +128,6 @@ public AwsAuthenticationOptionsConfig getAwsAuthenticationOptionsConfig() {
      * @since 2.7
      */
     public String getDatabaseDestination() {
-        return databaseDestination + File.separator + File.separator + "geoip";
+        return databaseDestination + File.separator + "geoip";
     }
 }
diff --git a/...rch/dataprepper/plugins/processor/extension/databasedownload/ManifestDownloadService.java b/...rch/dataprepper/plugins/processor/extension/databasedownload/ManifestDownloadService.java
@@ -92,7 +92,8 @@ private void downloadZipFile(final String databaseUrl, final String destinationP
         try {
             final URL url = new URL(databaseUrl);
             httpURLConnection = (HttpURLConnection) url.openConnection();
-            httpURLConnection.addRequestProperty("User-Agent", "Custom-User-Agent");
+            // CDN endpoint returns 403 without User Agent.
+            httpURLConnection.addRequestProperty("User-Agent", "Data Prepper");
         } catch (IOException ex) {
             throw new DownloadFailedException("Exception occurred while opening connection due to: " + ex.getMessage());
         }

diff --git a/...cessor/src/test/java/org/opensearch/dataprepper/plugins/processor/GeoIPProcessorTest.java b/...cessor/src/test/java/org/opensearch/dataprepper/plugins/processor/GeoIPProcessorTest.java
@@ -35,6 +35,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static org.hamcrest.CoreMatchers.equalTo;
@@ -104,7 +105,7 @@ class GeoIPProcessorTest {
 
     @BeforeEach
     void setUp() {
-        when(geoIpConfigSupplier.getGeoIPProcessorService()).thenReturn(geoIPProcessorService);
+        when(geoIpConfigSupplier.getGeoIPProcessorService()).thenReturn(Optional.of(geoIPProcessorService));
         lenient().when(geoIPProcessorService.getGeoIPDatabaseReader()).thenReturn(geoIPDatabaseReader);
         lenient().when(pluginMetrics.counter(GEO_IP_EVENTS_PROCESSED)).thenReturn(geoIpEventsProcessed);
         lenient().when(pluginMetrics.counter(GEO_IP_EVENTS_FAILED_LOOKUP)).thenReturn(geoIpEventsFailedLookup);

diff --git a/...rg/opensearch/dataprepper/plugins/processor/extension/DefaultGeoIpConfigSupplierTest.java b/...rg/opensearch/dataprepper/plugins/processor/extension/DefaultGeoIpConfigSupplierTest.java
@@ -12,11 +12,13 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.dataprepper.plugins.processor.extension.databasedownload.GeoIPDatabaseManager;
 
+import java.util.Optional;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mockConstruction;
 
 @ExtendWith(MockitoExtension.class)
@@ -39,11 +41,24 @@ void getGeoIpProcessorService_returns_geoIPProcessorService() {
         try (final MockedConstruction<GeoIPProcessorService> mockedConstruction =
                 mockConstruction(GeoIPProcessorService.class)) {
             final DefaultGeoIpConfigSupplier objectUnderTest = createObjectUnderTest();
-            final GeoIPProcessorService geoIPProcessorService = objectUnderTest.getGeoIPProcessorService();
+            final Optional<GeoIPProcessorService> geoIPProcessorService = objectUnderTest.getGeoIPProcessorService();
 
             assertThat(mockedConstruction.constructed().size(), equalTo(1));
-            assertThat(geoIPProcessorService, instanceOf(GeoIPProcessorService.class));
-            assertThat(geoIPProcessorService, equalTo(mockedConstruction.constructed().get(0)));
+            assertTrue(geoIPProcessorService.isPresent());
+            assertThat(geoIPProcessorService.get(), instanceOf(GeoIPProcessorService.class));
+            assertThat(geoIPProcessorService.get(), equalTo(mockedConstruction.constructed().get(0)));
+        }
+    }
+
+    @Test
+    void getGeoIpProcessorService_returns_empty_if_service_config_is_null() {
+        try (final MockedConstruction<GeoIPProcessorService> mockedConstruction =
+                     mockConstruction(GeoIPProcessorService.class)) {
+            final DefaultGeoIpConfigSupplier objectUnderTest = new DefaultGeoIpConfigSupplier(null, geoIPDatabaseManager, readLock);
+            final Optional<GeoIPProcessorService> geoIPProcessorService = objectUnderTest.getGeoIPProcessorService();
+
+            assertThat(mockedConstruction.constructed().size(), equalTo(0));
+            assertTrue(geoIPProcessorService.isEmpty());
         }
     }
 }
diff --git a/...c/test/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfigTest.java b/...c/test/java/org/opensearch/dataprepper/plugins/processor/extension/MaxMindConfigTest.java
@@ -61,6 +61,6 @@ void testSecureEndpoint(final String databasePath, final boolean insecure, final
         ReflectivelySetField.setField(MaxMindConfig.class, maxMindConfig, "insecure", insecure);
 
         assertThat(maxMindConfig.getDatabasePaths().size(), equalTo(1));
-        assertThat(maxMindConfig.isSecureEndpoint(), equalTo(result));
+        assertThat(maxMindConfig.isHttpsEndpointOrInsecure(), equalTo(result));
     }
 }
diff --git a/settings.gradle b/settings.gradle
@@ -117,7 +117,7 @@ include 'data-prepper-expression'
 include 'data-prepper-plugins:mutate-string-processors'
 include 'data-prepper-plugins:s3-source'
 include 'data-prepper-plugins:s3-sink'
-//include 'data-prepper-plugins:rss-source'
+include 'data-prepper-plugins:rss-source'
 include 'data-prepper-plugins:csv-processor'
 include 'data-prepper-plugins:parse-json-processor'
 include 'data-prepper-plugins:trace-peer-forwarder-processor'