Adding controller config for disabling Groovy in ingestionConfig #8169
Conversation
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
timsants
force-pushed
the
groovy_config_pt2
branch
from
cb5693b to
b4a3fc9
Compare
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Let's add a method isGroovyExpression() instead of passing disableGroovy to the getter
There was a problem hiding this comment.
So are you saying that we first call isGroovyExpression(String transformExpression, boolean disableGroovy) and pass the return value to the getter? i.e.
public static FunctionEvaluator getExpressionEvaluator(String transformExpression, boolean isGroovyExpression).
There was a problem hiding this comment.
Basically adding a method public static boolean isGroovyExpression(String transformExpression). The table config validator can call this method, and throw exception if it returns true
There was a problem hiding this comment.
Updated. Let me know if this was what you were thinking.
Codecov Report
@@ Coverage Diff @@
## master #8169 +/- ##
=======================================
Coverage 30.67% 30.68%
=======================================
Files 1612 1612
Lines 83962 83970 +8
Branches 12602 12604 +2
=======================================
+ Hits 25754 25764 +10
+ Misses 55919 55917 -2
Partials 2289 2289
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
timsants
force-pushed
the
groovy_config_pt2
branch
from
01ba3ca to
38e0d00
Compare
Jackie-Jiang
added
the
release-notes
3 participants
Description
This change adds a broker config for disabling Groovy transform and filter functions in the table ingestion config as it is a security risk. See Github issue #7966. By default, Groovy is allowed for backwards compatibility to not break existing use cases which currently use Groovy.
This PR is a follow up to #8159 which adds the broker config for disabling groovy in Pinot queries.
Testing
Added unit tests and tested config with quick-start config override.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Introduced new config for disabling Groovy in ingestionConfig:
controller.disable.ingestion.groovy. If not defined, defaults tofalse.