You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/04/16 06:34:59 UTC

[GitHub] [incubator-pinot] chenboat opened a new pull request #5260: Add access control for Pinot server segment download api.

chenboat opened a new pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260
 
 
   Add access control for Pinot server segment download api. Borrow most of the classes from the controller access control api.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r410497720
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -159,9 +160,11 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     LOGGER.info("Using class: {} as the AccessControlFactory", accessControlFactoryClass);
     final AccessControlFactory accessControlFactory;
     try {
-      accessControlFactory = (AccessControlFactory) Class.forName(accessControlFactoryClass).newInstance();
+      accessControlFactory = PluginManager.get().createInstance(accessControlFactoryClass);
     } catch (Exception e) {
-      throw new RuntimeException("Caught exception while creating new AccessControlFactory instance", e);
+      throw new RuntimeException(
+          "Caught exception while creating new AccessControlFactory instance in class " + this.getClass()
 
 Review comment:
   Got it. good point.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r409765830
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -153,8 +154,18 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     updateInstanceConfigIfNeeded(host, port);
 
     // Start restlet server for admin API endpoint
+    String accessControlFactoryClass =
+        _serverConf.getString(ACCESS_CONTROL_FACTORY_CLASS, DEFAULT_ACCESS_CONTROL_FACTORY_CLASS);
+    LOGGER.info("Use class: {} as the AccessControlFactory", accessControlFactoryClass);
+    final AccessControlFactory accessControlFactory;
+    try {
+      accessControlFactory = (AccessControlFactory) Class.forName(accessControlFactoryClass).newInstance();
 
 Review comment:
   Please use `PluginManager.createInstance()` 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on issue #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#issuecomment-614475656
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=h1) Report
   > Merging [#5260](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1f1baf85fb318caa48b3b207e032946ddd5de11c&el=desc) will **decrease** coverage by `8.93%`.
   > The diff coverage is `61.38%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5260/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #5260      +/-   ##
   ============================================
   - Coverage     65.90%   56.96%   -8.94%     
   ============================================
     Files          1052     1068      +16     
     Lines         54170    54336     +166     
     Branches       8078     8078              
   ============================================
   - Hits          35702    30955    -4747     
   - Misses        15819    20968    +5149     
   + Partials       2649     2413     -236     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <ø> (-76.67%)` | `0.00 <0.00> (ø)` | |
   | [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-23.41%)` | `0.00 <0.00> (ø)` | |
   | [.../BrokerResourceOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclJlc291cmNlT25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `41.86% <ø> (-13.96%)` | `0.00 <0.00> (ø)` | |
   | [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `70.70% <ø> (-1.28%)` | `0.00 <0.00> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `20.37% <0.00%> (-59.26%)` | `0.00 <0.00> (ø)` | |
   | [...thandler/SingleConnectionBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvU2luZ2xlQ29ubmVjdGlvbkJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `12.19% <ø> (-80.49%)` | `0.00 <0.00> (ø)` | |
   | [...ting/instanceselector/InstanceSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3JGYWN0b3J5LmphdmE=) | `71.42% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...er/routing/segmentpruner/SegmentPrunerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1NlZ21lbnRQcnVuZXJGYWN0b3J5LmphdmE=) | `83.33% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...outing/segmentselector/SegmentSelectorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50c2VsZWN0b3IvU2VnbWVudFNlbGVjdG9yRmFjdG9yeS5qYXZh) | `60.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...oker/routing/timeboundary/TimeBoundaryManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy90aW1lYm91bmRhcnkvVGltZUJvdW5kYXJ5TWFuYWdlci5qYXZh) | `87.50% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | ... and [560 more](https://codecov.io/gh/apache/incubator-pinot/pull/5260/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=footer). Last update [b20ace0...5d7aa45](https://codecov.io/gh/apache/incubator-pinot/pull/5260?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r409766078
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -153,8 +154,18 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     updateInstanceConfigIfNeeded(host, port);
 
     // Start restlet server for admin API endpoint
+    String accessControlFactoryClass =
+        _serverConf.getString(ACCESS_CONTROL_FACTORY_CLASS, DEFAULT_ACCESS_CONTROL_FACTORY_CLASS);
+    LOGGER.info("Use class: {} as the AccessControlFactory", accessControlFactoryClass);
+    final AccessControlFactory accessControlFactory;
+    try {
+      accessControlFactory = (AccessControlFactory) Class.forName(accessControlFactoryClass).newInstance();
+    } catch (Exception e) {
+      throw new RuntimeException("Caught exception while creating new AccessControlFactory instance", e);
 
 Review comment:
   Include class name in the exception string

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r410486118
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -159,9 +160,11 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     LOGGER.info("Using class: {} as the AccessControlFactory", accessControlFactoryClass);
     final AccessControlFactory accessControlFactory;
     try {
-      accessControlFactory = (AccessControlFactory) Class.forName(accessControlFactoryClass).newInstance();
+      accessControlFactory = PluginManager.get().createInstance(accessControlFactoryClass);
     } catch (Exception e) {
-      throw new RuntimeException("Caught exception while creating new AccessControlFactory instance", e);
+      throw new RuntimeException(
+          "Caught exception while creating new AccessControlFactory instance in class " + this.getClass()
 
 Review comment:
   The class name you want to add to the exception is not `this.getClass()`. Instead, it should be the value of `accessControlFactory`. The idea is that the exception message should reflect the remedial action as close as possible (e.g. in this case, it could be a config typo). It may even be useful to put it in quotes, so as to spot extra spaces (e.g. `"... in class ' + accessControlFactoryClass + "'", e)`)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r409759983
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -153,8 +154,18 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     updateInstanceConfigIfNeeded(host, port);
 
     // Start restlet server for admin API endpoint
+    String accessControlFactoryClass =
+        _serverConf.getString(ACCESS_CONTROL_FACTORY_CLASS, DEFAULT_ACCESS_CONTROL_FACTORY_CLASS);
+    LOGGER.info("Use class: {} as the AccessControlFactory", accessControlFactoryClass);
 
 Review comment:
   ```suggestion
       LOGGER.info("Using class: {} as the AccessControlFactory", accessControlFactoryClass);
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5260: Add access control for Pinot server segment download api.

Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5260: Add access control for Pinot server segment download api.
URL: https://github.com/apache/incubator-pinot/pull/5260#discussion_r410482442
 
 

 ##########
 File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
 ##########
 @@ -153,8 +154,18 @@ public HelixServerStarter(String helixClusterName, String zkAddress, Configurati
     updateInstanceConfigIfNeeded(host, port);
 
     // Start restlet server for admin API endpoint
+    String accessControlFactoryClass =
+        _serverConf.getString(ACCESS_CONTROL_FACTORY_CLASS, DEFAULT_ACCESS_CONTROL_FACTORY_CLASS);
+    LOGGER.info("Use class: {} as the AccessControlFactory", accessControlFactoryClass);
+    final AccessControlFactory accessControlFactory;
+    try {
+      accessControlFactory = (AccessControlFactory) Class.forName(accessControlFactoryClass).newInstance();
+    } catch (Exception e) {
+      throw new RuntimeException("Caught exception while creating new AccessControlFactory instance", e);
 
 Review comment:
   done.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org