Jackie-Jiang commented on code in PR #14844:
URL: https://github.com/apache/pinot/pull/14844#discussion_r1951251595
##########
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);
+
Review Comment:
(nit) Remove the empty empty lines. Same for other places
##########
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);
Review Comment:
Let's clean up the unrelated changes. The metric should be emitted from
controller for table config, and broker for query
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -110,11 +178,64 @@ public Object evaluate(Object[] values) {
for (int i = 0; i < _numArguments; i++) {
_binding.setVariable(_arguments.get(i), values[i]);
}
- return _script.run();
+ try {
+ return _script.run();
+ } catch (SecurityException ex) {
+ LOGGER.error("An insecure Groovy script execution caught: {}",
_expression);
+ incrementSecurityViolationCounter();
+ throw ex;
+ }
}
@Override
public String toString() {
return _expression;
}
+
+ public static GroovyStaticAnalyzerConfig getConfig() {
+ synchronized (GroovyFunctionEvaluator.class) {
+ return _config;
+ }
+ }
+
+ private static void incrementSecurityViolationCounter() {
+ synchronized (GroovyFunctionEvaluator.class) {
+ if (_metrics != null) {
+ _metrics.addMeteredGlobalValue(_securityViolationCounter, 1);
+ }
+ }
+ }
+
+ public static void setMetrics(AbstractMetrics metrics, AbstractMetrics.Meter
securityViolationCounter) {
Review Comment:
Let's manage metric on the caller side
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java:
##########
@@ -96,6 +160,10 @@ public Object evaluate(GenericRow genericRow) {
}
try {
return _script.run();
+ } catch (SecurityException ex) {
+ LOGGER.error("An insecure Groovy script execution caught: {}",
_expression);
+ incrementSecurityViolationCounter();
+ throw ex;
Review Comment:
When we throw the exception out, no need to log error because caller is
responsible for handling it. Same for other places
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
