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



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,32 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
       ControllerFilePathProvider provider = ControllerFilePathProvider.getInstance();
       String tempFileName = TMP_DIR_PREFIX + System.nanoTime();
       tempDecryptedFile = new File(provider.getFileUploadTempDir(), tempFileName);
+      tempEncryptedFile = new File(provider.getFileUploadTempDir(), tempFileName + ENCRYPTED_SUFFIX);
       tempSegmentDir = new File(provider.getUntarredFileTempDir(), tempFileName);
 
-      // Set default crypter to the noop crypter when no crypter header is sent
-      // In this case, the noop crypter will not do any operations, so the encrypted and decrypted file will have the same
-      // file path.
-      if (crypterClassName == null) {
-        crypterClassName = NoOpPinotCrypter.class.getSimpleName();
-        tempEncryptedFile = new File(provider.getFileUploadTempDir(), tempFileName);
-      } else {
-        tempEncryptedFile = new File(provider.getFileUploadTempDir(), tempFileName + ENCRYPTED_SUFFIX);
-      }
-
-      // TODO: Change when metadata upload added
-      String metadataProviderClass = DefaultMetadataExtractor.class.getName();
+      boolean uploadedSegmentIsEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);
 
-      SegmentMetadata segmentMetadata;
+      File dstFile = uploadedSegmentIsEncrypted ? tempEncryptedFile : tempDecryptedFile;
       FileUploadDownloadClient.FileUploadType uploadType = getUploadType(uploadTypeStr);
       switch (uploadType) {
         case URI:
-          segmentMetadata =
-              getMetadataForURI(crypterClassName, downloadUri, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
-                  metadataProviderClass);
+          getSegmentFileFromURI(downloadUri, dstFile);
           break;
         case SEGMENT:
-          getFileFromMultipart(multiPart, tempEncryptedFile);
-          segmentMetadata = getSegmentMetadata(crypterClassName, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
-              metadataProviderClass);
+          getSegmentFileFromMultipart(multiPart, dstFile);
           break;
         default:
           throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
       }
 
+      if (uploadedSegmentIsEncrypted) {
+        decryptFile(crypterClassNameInHeader, tempEncryptedFile, tempDecryptedFile);
+      }
+
+      // TODO: Change when metadata upload added

Review comment:
       Can you create an issue for this and reference the issue number? It is not clear what needs to be done here. I am guessing that the change is that if metadata is added, then we don't need to extract metadata?

##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java
##########
@@ -80,6 +80,14 @@ public String getActualColumnName(String tableName, String columnName) {
     return columnName;
   }
 
+  public TableConfig getTableConfig(String tableName) {
+    return _tableConfigChangeListener._tableConfigMap.get(canonicalize(tableName));
+  }
+
+  private String canonicalize(String name) {

Review comment:
       Nit: Are you planning to add additional canonicalization logic? If not, we are substituting one method for another, right? 

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +300,28 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
     return value;
   }
 
+  private Pair<Boolean, String> encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+      boolean uploadedSegmentIsEncrypted) {
+
+    // get crypter class name from table config
+    String crypterClassName = _pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+

Review comment:
       How about returning right here if segment does not need encryption? The logic is easier to follow then.

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -290,6 +300,28 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
     return value;
   }
 
+  private Pair<Boolean, String> encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
+      boolean uploadedSegmentIsEncrypted) {
+
+    // get crypter class name from table config
+    String crypterClassName = _pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+
+    boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassName);
+    if (segmentNeedsEncryption && uploadedSegmentIsEncrypted) {
+      throw new ControllerApplicationException(LOGGER, String.format(

Review comment:
       why is this an exception? Should this just not be a case where encryption is not needed? Why are we explicitly preventing the client from encrypting the segment over the wire?
   I would suggest the following:
   (1) Check the crypter class name. If they are the same in the header and tableconfig, then accept the segment.
   (2) If they are not same, we have a few choices: Accept the segment after bumping a metric (and a warn log), or reject the segment with an exception. The problem with rejecting the segment is if the crypter classes are some extension of one another, and actually work fine, then we may be unnecessarily rejecting it. 

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -303,46 +335,37 @@ private String getZkDownloadURIForSegmentUpload(String rawTableName, String segm
     }
   }
 
-  private SegmentMetadata getMetadataForURI(String crypterClassHeader, String currentSegmentLocationURI,
-      File tempEncryptedFile, File tempDecryptedFile, File tempSegmentDir, String metadataProviderClass)
+  private void getSegmentFileFromURI(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());
+    SegmentFetcherFactory.fetchSegmentToLocal(currentSegmentLocationURI, destFile);
   }
 
-  private SegmentMetadata getSegmentMetadata(String crypterClassHeader, File tempEncryptedFile, File tempDecryptedFile,
-      File tempSegmentDir, String metadataProviderClass)
+  private SegmentMetadata getSegmentMetadata(File tempDecryptedFile, File tempSegmentDir, String metadataProviderClass)
       throws Exception {
-
-    decryptFile(crypterClassHeader, tempEncryptedFile, tempDecryptedFile);
-
     // Call metadata provider to extract metadata with file object uri
     return MetadataExtractorFactory.create(metadataProviderClass).extractMetadata(tempDecryptedFile, tempSegmentDir);
   }
 
-  private void completeZkOperations(boolean enableParallelPushProtection, HttpHeaders headers, File tempEncryptedFile,
+  private void completeZkOperations(boolean enableParallelPushProtection, HttpHeaders headers, File uploadedSegmentFile,
       String rawTableName, SegmentMetadata segmentMetadata, String segmentName, String zkDownloadURI,
-      boolean moveSegmentToFinalLocation)
+      boolean moveSegmentToFinalLocation, String crypter)
       throws Exception {
     URI finalSegmentLocationURI = URIUtils
         .getUri(ControllerFilePathProvider.getInstance().getDataDirURI().toString(), rawTableName,
             URIUtils.encode(segmentName));
     ZKOperator zkOperator = new ZKOperator(_pinotHelixResourceManager, _controllerConf, _controllerMetrics);
-    zkOperator.completeSegmentOperations(rawTableName, segmentMetadata, finalSegmentLocationURI, tempEncryptedFile,
-        enableParallelPushProtection, headers, zkDownloadURI, moveSegmentToFinalLocation);
+    zkOperator.completeSegmentOperations(rawTableName, segmentMetadata, finalSegmentLocationURI, uploadedSegmentFile,
+        enableParallelPushProtection, headers, zkDownloadURI, moveSegmentToFinalLocation, crypter);
   }
 
-  private void decryptFile(String crypterClassHeader, File tempEncryptedFile, File tempDecryptedFile) {
-    PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassHeader);
-    LOGGER.info("Using crypter class {}", pinotCrypter.getClass().getName());
+  private void decryptFile(String crypterClassName, File tempEncryptedFile, File tempDecryptedFile) {
+    PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassName);
+    LOGGER.info("Using crypter class {} for decryption.", pinotCrypter.getClass().getName());

Review comment:
       please add the file names to this log, thanks




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