Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Groovy function by default #8711

Merged
merged 1 commit into from
May 17, 2022
Merged

Conversation

snleee
Copy link
Contributor

@snleee snleee commented May 16, 2022

This is the last step for #7966 to disable groovy function by default.

Release Notes: We are disabling groovy function by default due to the potential security issues. To enable back the functionality, users can choose to use table-level config override or broker/controller config to turn it on globally.

# Enable Groovy globally on Pinot broker
pinot.broker.disable.query.groovy=false

# Enable Groovy globally on Pinot controller
controller.disable.ingestion.groovy=false

# Table override
{
  "tableName": "myTable",
  "tableType": "OFFLINE",
 
  "query" : {
    "disableGroovy": false
  }
}

@snleee snleee added release-notes Referenced by PRs that need attention when compiling the next release notes backward-incompat Referenced by PRs that introduce or fix backward compat issues labels May 16, 2022
@snleee snleee requested review from Jackie-Jiang and npawar May 16, 2022 17:42
@@ -1033,8 +1033,8 @@ private HandlerContext getHandlerContext(@Nullable TableConfig offlineTableConfi
}
}

// Disable Groovy if either offline or realtime table config disables Groovy
boolean disableGroovy = offlineTableDisableGroovyQuery | realtimeTableDisableGroovyQuery;
// Disable Groovy if both offline and realtime table config disable Groovy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we change this? We can only allow groovy when both offline and realtime table has it enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will improve this logic in that case. I was trying to handle offline/ realtime only case.

We need an extra null check for the table configs.

  1. If both offline/realtime exists, allow if both got enabled.
  2. if only one exists, allow if the specific table got enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jackie-Jiang pls review the updated code.

@snleee snleee force-pushed the groovy-default branch 2 times, most recently from d727903 to ceeb249 Compare May 16, 2022 20:15
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2022

Codecov Report

Merging #8711 (1b56986) into master (f90137b) will decrease coverage by 1.20%.
The diff coverage is 69.23%.

❗ Current head 1b56986 differs from pull request most recent head 5a82d27. Consider uploading reports for the commit 5a82d27 to get more accurate results

@@             Coverage Diff              @@
##             master    #8711      +/-   ##
============================================
- Coverage     69.65%   68.44%   -1.21%     
+ Complexity     4571     4555      -16     
============================================
  Files          1720     1725       +5     
  Lines         89835    90105     +270     
  Branches      13333    13416      +83     
============================================
- Hits          62574    61672     -902     
- Misses        22940    24124    +1184     
+ Partials       4321     4309      -12     
Flag Coverage Δ
integration1 26.92% <23.07%> (-0.09%) ⬇️
integration2 ?
unittests1 66.17% <81.35%> (+0.01%) ⬆️
unittests2 14.27% <19.53%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
.../data/manager/offline/OfflineTableDataManager.java 100.00% <ø> (ø)
...ata/manager/realtime/RealtimeTableDataManager.java 45.41% <0.00%> (-22.86%) ⬇️
...inot/core/operator/filter/FilterOperatorUtils.java 86.25% <ø> (ø)
...inot/plugin/inputformat/csv/CSVMessageDecoder.java 0.00% <0.00%> (ø)
...va/org/apache/pinot/spi/utils/CommonConstants.java 21.31% <ø> (ø)
...va/org/apache/pinot/common/utils/SegmentUtils.java 53.84% <16.66%> (-17.59%) ⬇️
.../core/realtime/PinotLLCRealtimeSegmentManager.java 77.41% <20.00%> (-0.55%) ⬇️
.../controller/api/upload/SegmentValidationUtils.java 40.00% <40.00%> (ø)
...ces/PinotSegmentUploadDownloadRestletResource.java 53.75% <63.38%> (-3.52%) ⬇️
...roker/requesthandler/BaseBrokerRequestHandler.java 66.81% <65.21%> (-3.45%) ⬇️
... and 144 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 f90137b...5a82d27. Read the comment docs.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This is the last step for apache#7966 to disable groovy function by default.
@snleee snleee merged commit b672af1 into apache:master May 17, 2022
@snleee snleee deleted the groovy-default branch May 17, 2022 07:00
@abhs50
Copy link
Contributor

abhs50 commented Aug 3, 2022

Hello, Is there an ETA on when this fix will be released in latest version ? Thanks.

@Jackie-Jiang
Copy link
Contributor

cc @atris Do we have an ETA for the 0.11.0 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompat Referenced by PRs that introduce or fix backward compat issues release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants