[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5617: Add segment encryption on Controller based on table config

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

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



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +342,38 @@ private String getZkDownloadURIForSegmentUpload(String rawTableName, String segm
     }
   }
 
-  private SegmentMetadata getMetadataForURI(String crypterClassHeader, String currentSegmentLocationURI,
-      File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir, String metadataProviderClass)
+  private void downloadSegmentFileFromURI(String currentSegmentLocationURI, File destFile)
       throws Exception {
-    SegmentMetadata segmentMetadata;
     if (currentSegmentLocationURI == null || currentSegmentLocationURI.isEmpty()) {
       throw new ControllerApplicationException(LOGGER, "Failed to get downloadURI, needed for URI upload",
           Response.Status.BAD_REQUEST);
     }
-    LOGGER.info("Downloading segment from {} to {}", currentSegmentLocationURI, tempEncryptedFile.getAbsolutePath());
-    SegmentFetcherFactory.fetchSegmentToLocal(currentSegmentLocationURI, tempEncryptedFile);
-    segmentMetadata = getSegmentMetadata(crypterClassHeader, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
-        metadataProviderClass);
-    return segmentMetadata;
+    LOGGER.info("Downloading segment from {} to {}", currentSegmentLocationURI, destFile.getAbsolutePath());

Review comment:
       I am assuming that the URI tells us segment and table name here, otherwise, it is useful to add table/segment name i this log

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +296,39 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
     return value;
   }
 
+  Pair<String, File> encryptSegmentIfNeeded(File tempDecryptedFile, File tempEncryptedFile,
+      boolean isUploadedSegmentEncrypted, String crypterUsedInUploadedSegment, String crypterClassNameInTableConfig,
+      String segmentName, String tableName) {
+
+    boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassNameInTableConfig);
+
+    // form the output
+    File finalSegmentFile =
+        (isUploadedSegmentEncrypted || segmentNeedsEncryption) ? tempEncryptedFile : tempDecryptedFile;
+    String crypterClassName = Strings.isNullOrEmpty(crypterClassNameInTableConfig) ? crypterUsedInUploadedSegment
+        : crypterClassNameInTableConfig;
+    ImmutablePair<String, File> out = ImmutablePair.of(crypterClassName, finalSegmentFile);
+
+    if (!segmentNeedsEncryption) {
+      return out;
+    }
+
+    if (isUploadedSegmentEncrypted && !crypterClassNameInTableConfig.equals(crypterUsedInUploadedSegment)) {
+      throw new ControllerApplicationException(LOGGER, String
+          .format("Uploaded segment is encrypted with '%s' while table config requires '%s' as crypter "
+                  + "(segment name = '%s', table name = '%s').", crypterUsedInUploadedSegment,
+              crypterClassNameInTableConfig, segmentName, tableName), Response.Status.INTERNAL_SERVER_ERROR);
+    }
+
+    // encrypt segment
+    PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassNameInTableConfig);
+    LOGGER.info("Using crypter class {} for encrypting {} to {}.", crypterClassNameInTableConfig, tempDecryptedFile,

Review comment:
       please add table name and segment name here. In production, we will see multiple parallel pushes going on, and it is useful to determine which segment/table we are talking about.

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -98,7 +102,7 @@ public synchronized void refresh() {
             try {
               TableConfig tableConfig = TableConfigUtils.fromZNRecord(znRecord);
               String tableNameWithType = tableConfig.getTableName();
-              _tableConfigMap.put(tableNameWithType, tableConfig);
+              _tableConfigMap.put(tableNameWithType.toLowerCase(), tableConfig);

Review comment:
       Not sure why we lower case here. Can you elaborate?




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