Init groovy AST config for server, minion and controller

pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java

@@ -68,7 +68,9 @@ public enum ControllerMeter implements AbstractMetrics.Meter {
   NUMBER_ADHOC_TASKS_SUBMITTED("adhocTasks", false),
   IDEAL_STATE_UPDATE_FAILURE("IdealStateUpdateFailure", false),
   IDEAL_STATE_UPDATE_RETRY("IdealStateUpdateRetry", false),
-  IDEAL_STATE_UPDATE_SUCCESS("IdealStateUpdateSuccess", false);
+  IDEAL_STATE_UPDATE_SUCCESS("IdealStateUpdateSuccess", false),
+  GROOVY_SECURITY_VIOLATIONS("exceptions", true);
+
 
 
   private final String _brokerMeterName;

pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java

@@ -38,7 +38,8 @@ public enum MinionMeter implements AbstractMetrics.Meter {
   SEGMENT_BYTES_UPLOADED("bytes", false),
   RECORDS_PROCESSED_COUNT("rows", false),
   RECORDS_PURGED_COUNT("rows", false),
-  COMPACTED_RECORDS_COUNT("rows", false);
+  COMPACTED_RECORDS_COUNT("rows", false),
+  GROOVY_SECURITY_VIOLATIONS("exceptions", true);
 
   private final String _meterName;
   private final String _unit;

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java

@@ -122,6 +122,7 @@
 import org.apache.pinot.core.segment.processing.lifecycle.PinotSegmentLifecycleEventListenerManager;
 import org.apache.pinot.core.transport.ListenerConfig;
 import org.apache.pinot.core.util.ListenerConfigUtil;
+import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator;
 import org.apache.pinot.segment.local.utils.TableConfigUtils;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.crypt.PinotCrypterFactory;
@@ -382,7 +383,8 @@ public ControllerConf getConfig() {
   }
 
   @Override
-  public void start() {
+  public void start()
+      throws Exception {
     LOGGER.info("Starting Pinot controller in mode: {}. (Version: {})", _controllerMode.name(), PinotVersion.VERSION);
     LOGGER.info("Controller configs: {}", new PinotAppConfigs(getConfig()).toJSONString());
     long startTimeMs = System.currentTimeMillis();
@@ -407,6 +409,12 @@ public void start() {
         break;
     }
 
+    // Initializing Groovy execution security
+    GroovyFunctionEvaluator.configureGroovySecurity(
+        _config.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG));
+    GroovyFunctionEvaluator.setMetrics(_controllerMetrics, ControllerMeter.GROOVY_SECURITY_VIOLATIONS);
+
+
     ServiceStatus.setServiceStatusCallback(_helixParticipantInstanceId,
         new ServiceStatus.MultipleCallbackServiceStatusCallback(_serviceStatusCallbackList));
     _controllerMetrics.addTimedValue(ControllerTimer.STARTUP_SUCCESS_DURATION_MS,

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java

@@ -187,8 +187,8 @@ public GroovyStaticAnalyzerConfig getGroovyStaticAnalysisConfig() throws Excepti
     HelixConfigScope configScope = new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER)
         .forCluster(_pinotHelixResourceManager.getHelixClusterName()).build();
     Map<String, String> configs = helixAdmin.getConfig(configScope,
-        List.of(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG));
-    String json = configs.get(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG);
+        List.of(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG));
+    String json = configs.get(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG);
     if (json != null) {
       return GroovyStaticAnalyzerConfig.fromJson(json);
     } else {
@@ -215,7 +215,7 @@ public SuccessResponse setGroovyStaticAnalysisConfig(String body) throws Excepti
               _pinotHelixResourceManager.getHelixClusterName()).build();
       Map<String, String> properties = new TreeMap<>();
       GroovyStaticAnalyzerConfig groovyConfig = GroovyStaticAnalyzerConfig.fromJson(body);
-      properties.put(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG,
+      properties.put(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG,
           groovyConfig == null ? null : groovyConfig.toJson());
       admin.setConfig(configScope, properties);
       GroovyFunctionEvaluator.setConfig(groovyConfig);

pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java

@@ -26,13 +26,14 @@
 import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator;
 import org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig;
 import org.apache.pinot.spi.data.readers.GenericRow;
-import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import org.testng.collections.Lists;
 
 import static org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig.getDefaultAllowedImports;
 import static org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig.getDefaultAllowedReceivers;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.fail;
 
 
 /**
@@ -61,7 +62,7 @@ public void testLegalGroovyScripts()
       GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script);
       GenericRow row = new GenericRow();
       Object result = groovyFunctionEvaluator.evaluate(row);
-      Assert.assertEquals(2, result);
+      assertEquals(2, result);
     }
   }
 
@@ -92,10 +93,8 @@ public void testIllegalGroovyScripts()
 
     for (String script : scripts) {
       try {
-        GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script);
-        GenericRow row = new GenericRow();
-        groovyFunctionEvaluator.evaluate(row);
-        Assert.fail("Groovy analyzer failed to catch malicious script");
+        new GroovyFunctionEvaluator(script);
+        fail("Groovy analyzer failed to catch malicious script");
       } catch (Exception ignored) {
       }
     }
@@ -121,7 +120,7 @@ public void testUpdatingConfiguration()
         GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script);
         GenericRow row = new GenericRow();
         groovyFunctionEvaluator.evaluate(row);
-        Assert.fail(String.format("Groovy analyzer failed to catch malicious script: %s", script));
+        fail(String.format("Groovy analyzer failed to catch malicious script: %s", script));
       } catch (Exception ignored) {
       }
     }
@@ -132,10 +131,10 @@ public void testGroovyFunctionEvaluation(String transformFunction, List<String>
       Object expectedResult) {
 
     GroovyFunctionEvaluator groovyExpressionEvaluator = new GroovyFunctionEvaluator(transformFunction);
-    Assert.assertEquals(groovyExpressionEvaluator.getArguments(), arguments);
+    assertEquals(groovyExpressionEvaluator.getArguments(), arguments);
 
     Object result = groovyExpressionEvaluator.evaluate(genericRow);
-    Assert.assertEquals(result, expectedResult);
+    assertEquals(result, expectedResult);
   }
 
   @DataProvider(name = "groovyFunctionEvaluationDataProvider")

pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyStaticAnalyzerConfigTest.java

@@ -89,6 +89,7 @@ public void testAllowedStaticImports()
     Assert.assertNull(decodedConfig.getAllowedImports());
     Assert.assertEquals(GroovyStaticAnalyzerConfig.getDefaultAllowedImports(), decodedConfig.getAllowedStaticImports());
     Assert.assertNull(decodedConfig.getDisallowedMethodNames());
+    Assert.assertFalse(decodedConfig.isMethodDefinitionAllowed());
   }
 
   @Test

pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java

@@ -61,6 +61,7 @@
 import org.apache.pinot.minion.executor.PinotTaskExecutorFactory;
 import org.apache.pinot.minion.executor.TaskExecutorFactoryRegistry;
 import org.apache.pinot.minion.taskfactory.TaskFactoryRegistry;
+import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator;
 import org.apache.pinot.spi.crypt.PinotCrypterFactory;
 import org.apache.pinot.spi.env.PinotConfiguration;
 import org.apache.pinot.spi.filesystem.PinotFSFactory;
@@ -272,6 +273,10 @@ public void start()
       minionContext.setSSLContext(sslContext);
     }
 
+    // Initializing Groovy execution security
+    GroovyFunctionEvaluator.configureGroovySecurity(_config.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG));
+    GroovyFunctionEvaluator.setMetrics(minionMetrics, MinionMeter.GROOVY_SECURITY_VIOLATIONS);
+
     // Join the Helix cluster
     LOGGER.info("Joining the Helix cluster");
     _helixManager.getStateMachineEngine().registerStateModelFactory("Task", new TaskStateModelFactory(_helixManager,

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java

@@ -28,8 +28,7 @@
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import org.apache.pinot.common.metrics.ServerMeter;
-import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.common.metrics.AbstractMetrics;
 import org.apache.pinot.spi.data.readers.GenericRow;
 import org.codehaus.groovy.ast.expr.MethodCallExpression;
 import org.codehaus.groovy.control.CompilerConfiguration;
@@ -64,7 +63,8 @@ public class GroovyFunctionEvaluator implements FunctionEvaluator {
   private static final String SCRIPT_GROUP_NAME = "script";
   private static final String ARGUMENTS_SEPARATOR = ",";
   private static GroovyStaticAnalyzerConfig _config;
-  private static ServerMetrics _metrics;
+  private static AbstractMetrics _metrics;
+  private static AbstractMetrics.Meter _securityViolationCounter;
 
   private final List<String> _arguments;
   private final int _numArguments;
@@ -201,17 +201,33 @@ private static GroovyStaticAnalyzerConfig getConfig() {
   private static void incrementSecurityViolationCounter() {
     synchronized (GroovyFunctionEvaluator.class) {
       if (_metrics != null) {
-        _metrics.addMeteredGlobalValue(ServerMeter.GROOVY_SECURITY_VIOLATIONS, 1);
+        _metrics.addMeteredGlobalValue(_securityViolationCounter, 1);
       }
     }
   }
 
-  public static void setServerMetrics(ServerMetrics metrics) {
+  public static void setMetrics(AbstractMetrics metrics, AbstractMetrics.Meter securityViolationCounter) {
     synchronized (GroovyFunctionEvaluator.class) {
       _metrics = metrics;
+      _securityViolationCounter = securityViolationCounter;
     }
   }
 
+  public static void configureGroovySecurity(String groovyASTConfig) throws Exception {
+    try {
+      if (groovyASTConfig != null) {
+        setConfig(GroovyStaticAnalyzerConfig.fromJson(groovyASTConfig));
+      } else {
+        LOGGER.info("No Groovy Security Configuration found, Groovy static analysis is disabled");
+      }
+    } catch (Exception ex) {
+      // If there was an issue reading the security configuration then send an exception to the startup routine.
+      LOGGER.error("Failed to read config from ZK");
+      throw ex;
+    }
+  }
+
+
   /**
    * Initialize or update the configuration for the Groovy Static Analyzer.
    * @param config GroovyStaticAnalyzerConfig instance to be used for static syntax analysis.

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java

@@ -79,7 +79,6 @@
 import org.apache.pinot.core.transport.ListenerConfig;
 import org.apache.pinot.core.util.ListenerConfigUtil;
 import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator;
-import org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig;
 import org.apache.pinot.segment.local.realtime.impl.invertedindex.RealtimeLuceneIndexRefreshManager;
 import org.apache.pinot.segment.local.realtime.impl.invertedindex.RealtimeLuceneTextIndexSearcherPool;
 import org.apache.pinot.segment.local.utils.SegmentPreprocessThrottler;
@@ -673,8 +672,10 @@ public void start()
     _adminApiApplication = createServerAdminApp();
     _adminApiApplication.start(_listenerConfigs);
 
-    // Initializing Groovy execution engine security
-    configureGroovySecurity(serverMetrics);
+    // Initializing Groovy execution security
+    GroovyFunctionEvaluator.configureGroovySecurity(
+        _serverConf.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG));
+    GroovyFunctionEvaluator.setMetrics(serverMetrics, ServerMeter.GROOVY_SECURITY_VIOLATIONS);
 
     // Init QueryRewriterFactory
     LOGGER.info("Initializing QueryRewriterFactory");
@@ -1012,24 +1013,4 @@ private void initSegmentFetcher(PinotConfiguration config)
   protected AdminApiApplication createServerAdminApp() {
     return new AdminApiApplication(_serverInstance, _accessControlFactory, _serverConf);
   }
-
-  private void configureGroovySecurity(ServerMetrics metrics) throws Exception {
-    GroovyStaticAnalyzerConfig config = null;
-    try {
-      String json = _serverConf.getProperty(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG);
-
-      if (json != null) {
-        LOGGER.info("Groovy Security Configuration: {}", json);
-        config = GroovyStaticAnalyzerConfig.fromJson(json);
-        GroovyFunctionEvaluator.setConfig(config);
-        GroovyFunctionEvaluator.setServerMetrics(metrics);
-      } else {
-        LOGGER.info("No Groovy Security Configuration found, Groovy static analysis is disabled.");
-      }
-    } catch (Exception ex) {
-      // If there was an issue reading the security configuration then send an exception to the startup routine.
-      LOGGER.error("Failed to read config from ZK. Loading Default configuration.");
-      throw ex;
-    }
-  }
 }

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

@@ -29,6 +29,8 @@
 
 
 public class CommonConstants {
+  public static final String GROOVY_STATIC_ANALYZER_CONFIG = "pinot.server.groovy.static.analyzer";
+
   private CommonConstants() {
   }
 
@@ -673,7 +675,6 @@ public enum Type {
   }
 
   public static class Server {
-    public static final String GROOVY_STATIC_ANALYZER_CONFIG = "pinot.server.groovy.static.analyzer";
     public static final String INSTANCE_DATA_MANAGER_CONFIG_PREFIX = "pinot.server.instance";
     public static final String QUERY_EXECUTOR_CONFIG_PREFIX = "pinot.server.query.executor";
     public static final String METRICS_CONFIG_PREFIX = "pinot.server.metrics";