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/07/02 01:31:48 UTC
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5617: Add segment encryption on Controller based on table config
kishoreg commented on a change in pull request #5617:
URL: https://github.com/apache/incubator-pinot/pull/5617#discussion_r448697821
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -177,13 +179,13 @@ public Response downloadSegment(
private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiPart multiPart,
boolean enableParallelPushProtection, HttpHeaders headers, Request request, boolean moveSegmentToFinalLocation) {
String uploadTypeStr = null;
- String crypterClassName = null;
+ String crypterClassNameInHeader = null;
Review comment:
it's better to pass the crypterName instead of the classname right? Its ok to have the ability to override it with a classname but it weird to have users pass classname in the request
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
Review comment:
why are we download the segment?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -251,6 +246,17 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
_controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
.validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);
+ // Encrypt segment
+ String crypterClassNameInTableConfig =
Review comment:
is this making a ZK call?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -194,38 +196,31 @@ 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);
+ downloadSegmentFileFromURI(downloadUri, dstFile, tableName);
break;
case SEGMENT:
- getFileFromMultipart(multiPart, tempEncryptedFile);
- segmentMetadata = getSegmentMetadata(crypterClassName, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
- metadataProviderClass);
+ createSegmentFileFromMultipart(multiPart, dstFile);
break;
default:
throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
}
+ if (uploadedSegmentIsEncrypted) {
Review comment:
better to always go through the crypter with noopcrypter
##########
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:
IMO, its better to always go through the same path and just make the crypter no-op
----------------------------------------------------------------
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