Adding broker level config for disabling Pinot queries with Groovy

[timsants]

Description

This change adds a broker config for disabling Groovy in Pinot queries 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.

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)

• Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

• Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

• Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Introduced new config for disabling Groovy in queries: pinot.broker.disable.query.groovy. If not defined, defaults to false.

[Jackie-Jiang]

LGTM otherwise

[codecov-commenter]

Codecov Report

Merging #8159 (75ef030) into master (6844cb3) will increase coverage by 39.53%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##             master    #8159       +/-   ##
=============================================
+ Coverage     30.67%   70.20%   +39.53%     
- Complexity        0     4302     +4302     
=============================================
  Files          1612     1623       +11     
  Lines         83962    84350      +388     
  Branches      12602    12652       +50     
=============================================
+ Hits          25754    59217    +33463     
+ Misses        55919    21038    -34881     
- Partials       2289     4095     +1806     

Flag	Coverage Δ
integration1	?
integration2	27.68% <2.77%> (+0.12%)	⬆️
unittests1	67.88% <ø> (?)
unittests2	14.19% <66.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...r/transform/function/TransformFunctionFactory.java	82.72% <ø> (+2.72%)	⬆️
...va/org/apache/pinot/spi/utils/CommonConstants.java	23.63% <ø> (+23.63%)	⬆️
...roker/requesthandler/BaseBrokerRequestHandler.java	70.70% <66.66%> (+6.26%)	⬆️
...pinot/minion/exception/TaskCancelledException.java	0.00% <0.00%> (-100.00%)	⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java	0.00% <0.00%> (-100.00%)	⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java	0.00% <0.00%> (-94.74%)	⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java	0.00% <0.00%> (-87.10%)	⬇️
.../transform/function/MapValueTransformFunction.java	0.00% <0.00%> (-85.30%)	⬇️
...ot/common/messages/RoutingTableRebuildMessage.java	0.00% <0.00%> (-81.82%)	⬇️
...data/manager/realtime/DefaultSegmentCommitter.java	0.00% <0.00%> (-80.00%)	⬇️
... and 1167 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6844cb3...75ef030. Read the comment docs.

[amrishlal]

This will scan each and every statement when DISABLE_GRROVY option is enabled. I am wondering if there is a more efficient option like doing a check while invoking groovy function whether it is disabled for SQL statements or not? I don't think there is an issue with groovy function being called during ingestion so that can be ignored.

[Jackie-Jiang]

@amrishlal It is done on a per query basis on the broker side, which should be fine. We already have multiple similar operations on the broker (e.g. fixing column name, override hll etc,), and so far no obvious performance hit, so I wouldn't worry too much about that.