[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6613: Basic Auth for pinot-controller

[GitBox <gi...@apache.org>]

mcvsubbu commented on a change in pull request #6613:
URL: https://github.com/apache/incubator-pinot/pull/6613#discussion_r592873439



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -304,6 +307,7 @@
       public static final String CONFIG_OF_CONTROLLER_HTTPS_ENABLED = "enabled";
       public static final String CONFIG_OF_CONTROLLER_HTTPS_PORT = "controller.port";
       public static final String CONFIG_OF_SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS = "upload.request.timeout.ms";
+      public static final String CONFIG_OF_SEGMENT_UPLOAD_AUTH_TOKEN = "upload.auth.token";

Review comment:
       same doc comment

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -353,6 +357,8 @@
     public static final String PREFIX_OF_CONFIG_OF_SEGMENT_FETCHER_FACTORY = "segment.fetcher";
     public static final String PREFIX_OF_CONFIG_OF_SEGMENT_UPLOADER = "segment.uploader";
     public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "crypter";
+
+    public static final String CONFIG_OF_TASK_AUTH_TOKEN = "task.auth.token";

Review comment:
       same doc comment

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -180,6 +180,16 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
     requestStatistics.setRequestId(requestId);
     requestStatistics.setRequestArrivalTimeMillis(System.currentTimeMillis());
 
+    // first-stage access control to prevent unauthenticated requests from using up resources
+    // secondary table-level check comes later
+    boolean hasAccess = _accessControlFactory.create().hasAccess(requesterIdentity, null);

Review comment:
       Better to introduce a new default method in AccessControl that takes only requesterIdentity as the argument. Default implementation can return true.

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
 
 
 public class SegmentFetcherFactory {
-  private SegmentFetcherFactory() {
-  }
+  private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
 
   static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
   private static final String PROTOCOLS_KEY = "protocols";
+  private static final String AUTH_TOKEN_KEY = "auth.token";
   private static final String ENCODED_SUFFIX = ".enc";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
-  private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
-  private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
-  private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
-  static {
-    PinotConfiguration emptyConfig = new PinotConfiguration();
-    DEFAULT_HTTP_SEGMENT_FETCHER.init(emptyConfig);
-    DEFAULT_PINOT_FS_SEGMENT_FETCHER.init(emptyConfig);
+
+  private final Map<String, SegmentFetcher> _segmentFetcherMap = new HashMap<>();
+  private final SegmentFetcher _httpSegmentFetcher = new HttpSegmentFetcher();
+  private final SegmentFetcher _pinotFSSegmentFetcher = new PinotFSSegmentFetcher();
+
+  private SegmentFetcherFactory() {
+    // left blank
+  }
+
+  public static SegmentFetcherFactory getInstance() {
+    return instance;
   }
 
   /**
    * Initializes the segment fetcher factory. This method should only be called once.
    */
   public static void init(PinotConfiguration config)
       throws Exception {
+    getInstance().initInternal(config);
+  }
+
+  private void initInternal(PinotConfiguration config)
+      throws Exception {
+    _httpSegmentFetcher.init(config); // directly, without sub-namespace

Review comment:
       Am I missing this? Are these added to the map anywhere as default fetchers?

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java
##########
@@ -168,16 +172,6 @@ public static URI getRetrieveSchemaURI(String protocol, String host, int port, S
     return getURI(protocol, host, port, SCHEMA_PATH + "/" + schemaName);
   }
 
-  public static URI getUploadSchemaHttpURI(String host, int port)

Review comment:
       @jackjlli  can you verify if any of these are used in our internal spark/hadoop jobs? 
   @apucher you should flag that this is a backward incompatible change for some cases such as this one above.
   
   Another way to do this may be to move/copy this to spi with new methods, and deprecate the class altogether?

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.api.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.controller.api.access.AccessControl;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessType;
+
+
+@Api(tags = "Auth")
+@Path("/")
+public class PinotControllerAuthResource {
+
+  @Inject
+  private AccessControlFactory _accessControlFactory;
+
+  @Context
+  HttpHeaders _httpHeaders;
+
+  @GET
+  @Path("auth/verify")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Check whether authentication is enabled")
+  @ApiResponses(value = {@ApiResponse(code = 200, message = "Verification result provided"), @ApiResponse(code = 500, message = "Verification error")})
+  public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("tableName") String tableName,
+      @ApiParam(value = "API access type") @QueryParam("accessType") AccessType accessType,
+      @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
+    AccessControl accessControl = _accessControlFactory.create();
+
+    if (StringUtils.isBlank(tableName)) {
+      return accessControl.hasAccess(accessType, _httpHeaders, endpointUrl);
+    }
+
+    return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+  }
+
+  @GET
+  @Path("auth/info")

Review comment:
       what is auth workflow?

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/SegmentFetcherFactory.java
##########
@@ -34,29 +34,40 @@
 
 
 public class SegmentFetcherFactory {
-  private SegmentFetcherFactory() {
-  }
+  private final static SegmentFetcherFactory instance = new SegmentFetcherFactory();
 
   static final String SEGMENT_FETCHER_CLASS_KEY_SUFFIX = ".class";
   private static final String PROTOCOLS_KEY = "protocols";
+  private static final String AUTH_TOKEN_KEY = "auth.token";
   private static final String ENCODED_SUFFIX = ".enc";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(SegmentFetcherFactory.class);
-  private static final Map<String, SegmentFetcher> SEGMENT_FETCHER_MAP = new HashMap<>();
-  private static final SegmentFetcher DEFAULT_HTTP_SEGMENT_FETCHER = new HttpSegmentFetcher();
-  private static final SegmentFetcher DEFAULT_PINOT_FS_SEGMENT_FETCHER = new PinotFSSegmentFetcher();
-
-  static {

Review comment:
       Need to look at the history of this class carefully. The last time we made some changes like this, we had issues where the controller (I think) had an exception, probably during startup. It was when this whole thing was in com.linkedin space, so we don't have the history readily available. 

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -235,6 +235,9 @@
         "pinot.server.instance.realtime.alloc.offheap.direct";
     public static final String PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY = "pinot.server.storage.factory";
     public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "pinot.server.crypter";
+
+    public static final String CONFIG_OF_AUTH_TOKEN = "auth.token";

Review comment:
       What would be a good user doc of this config? You can add that in the comments here. It will then be a matter to translating it to user docs at some point in time.




----------------------------------------------------------------
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



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