Add segment encryption on Controller based on table config #5617

sajjad-moradi · 2020-06-25T00:26:53Z

Description

Currently if one wants to have their segments encrypted on Pinot cluster, they need to encrypt the segments before upload and then on the upload http request, CRYPTER header with crypter class name as the value needs to be set. In this PR, encryption is added on Controller side by introducing an optional config crypterClassName int table config. In UploadSegment path of Controller's REST API, this config is retrieved and if its value is not null, it triggers segment encryption using the specified crypter.
Note that to have less pressure on ZK for retrieving table config on each segment upload, an existing cache for table configs, TableCache in HelixResourceManager is being used.

Testing Done

Ran Controller, Server, and Broker locally with the new changes and verified the desired behavior in different scenarios for uploading segments:

CRYPTER header set to NoOpPinotCrypter
crypterClassName config set to NoOpPinotCrypter
Both 1 and 2
Neither

Also in each scenario, checked the value of crypter property of the uploaded segment ZK metadata.

Jackie-Jiang · 2020-06-25T01:32:57Z

I would recommend putting this under the SegmentsValidationAndRetentionConfig instead of creating another top level config.

mcvsubbu

@npawar should the new crypter class name go into ingestion config (because this is related to ingestion)? Or, as @Jackie-Jiang recommends, in the segment validation and retention?

mcvsubbu · 2020-06-25T15:59:09Z

pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java

+    return _tableConfigChangeListener._tableConfigMap.get(canonicalize(tableName));
+  }
+
+  private String canonicalize(String name) {


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

If latter, probably not worth creating another layer of method call.

Using canonicalize explicitly states the reason why we're making the those strings lower case which IMO makes the code more maintainable, but since you both think otherwise, I'm reverting it.

mcvsubbu · 2020-06-25T16:02:15Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+        decryptFile(crypterClassNameInHeader, tempEncryptedFile, tempDecryptedFile);
+      }
+
+      // TODO: Change when metadata upload added


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?

I'm not sure why TODO is added there. I looked at it git history and it's added on this PR: 3155. If you (or other ppl) can confirm the intention there, i'll open up an issue for it.

I went through some history. I believe it is trying to refer to the case when the client (segment pusher) does the following:
(1) Upload segment to deepstore
(2) upload metadata to controller so that it can update ZK.

We don't support this pattern. When a URI is uploaded, we actually download the segment and extract metadata instead.

A lot of code needs to change when we support metata-only upload, so let us just get rid of this TODO. thanks

FYI, we will need the support for metadata push pretty soon. But that is somewhat unrelated here, so just an FYI at this point in time.

mcvsubbu · 2020-06-25T16:09:29Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+
+    boolean segmentNeedsEncryption = !Strings.isNullOrEmpty(crypterClassName);
+    if (segmentNeedsEncryption && uploadedSegmentIsEncrypted) {
+      throw new ControllerApplicationException(LOGGER, String.format(


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.

Makes sense. Refactored per your suggestion.

I went with rejecting the segment option. If we encounter users having same crypter with different names, it'll be a small refactor to allow that change.

mcvsubbu · 2020-06-25T16:10:36Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+
+    // get crypter class name from table config
+    String crypterClassName = _pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+


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

IMO, its better to always go through the same path and just make the crypter no-op

mcvsubbu · 2020-06-25T16:11:44Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

-    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());


please add the file names to this log, thanks

npawar · 2020-06-25T17:54:59Z

@npawar should the new crypter class name go into ingestion config (because this is related to ingestion)? Or, as @Jackie-Jiang recommends, in the segment validation and retention?

I think SegmentSConfig is a better place than ingestionConfig. This is more like a segment property than a "how to ingest" property.
It definitely shouldn't be a top level property

mayankshriv · 2020-06-25T17:27:58Z

pinot-common/src/main/java/org/apache/pinot/common/utils/helix/TableCache.java

+    return _tableConfigChangeListener._tableConfigMap.get(canonicalize(tableName));
+  }
+
+  private String canonicalize(String name) {


If latter, probably not worth creating another layer of method call.

mayankshriv · 2020-06-25T17:33:02Z

...ava/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)


A get method returning void seems odd to me. Probably s/get/download?

Makes sense 👍 Refactored.

mayankshriv · 2020-06-25T17:36:49Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

@@ -251,6 +247,20 @@ private SuccessResponse uploadSegment(@Nullable String tableName, FormDataMultiP
          _controllerMetrics, _leadControllerManager.isLeaderForTable(offlineTableName))
          .validateOfflineSegment(offlineTableName, segmentMetadata, tempSegmentDir);

+      Pair<Boolean, String> encryptionInfo =
+          encryptSegmentIfNeeded(offlineTableName, tempDecryptedFile, tempEncryptedFile, uploadedSegmentIsEncrypted);


crypterClassName should be an input to this method, not a return value? This method should just return the boolean.

kishoreg · 2020-06-26T14:53:01Z

I would recommend putting this under the SegmentsValidationAndRetentionConfig instead of creating another top-level config.

can we rename 'SegmentsValidationAndRetentionConfig' to segmentConfig?

sajjad-moradi · 2020-06-30T01:03:38Z

I would recommend putting this under the SegmentsValidationAndRetentionConfig instead of creating another top-level config.

can we rename 'SegmentsValidationAndRetentionConfig' to segmentConfig?

I tried to do that, but it added a lot of noise to the PR, so I decided to leave the renaming to another PR. I'll create a separate PR for it after this one goes in.

mcvsubbu

The logic in uploadSegment is quite complicated, so needs a unit test i believe, with all the different combinations tested for correctness.

It will be awesome if you can re-factor the logic into a method that can then be tested independently for each combo (inbound segment encrypted, with same/different class, table config has/has not the encryption, whatever).

And add a unit test to test all these combinations.

mcvsubbu · 2020-06-30T16:49:35Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

-
-      // TODO: Change when metadata upload added
-      String metadataProviderClass = DefaultMetadataExtractor.class.getName();
+      boolean uploadedSegmentIsEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);


Suggested change

boolean uploadedSegmentIsEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);

boolean isUploadedSegmentEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);

mcvsubbu · 2020-06-30T17:00:35Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+        decryptFile(crypterClassNameInHeader, tempEncryptedFile, tempDecryptedFile);
+      }
+
+      // TODO: Change when metadata upload added


I went through some history. I believe it is trying to refer to the case when the client (segment pusher) does the following:
(1) Upload segment to deepstore
(2) upload metadata to controller so that it can update ZK.

We don't support this pattern. When a URI is uploaded, we actually download the segment and extract metadata instead.

A lot of code needs to change when we support metata-only upload, so let us just get rid of this TODO. thanks

mcvsubbu · 2020-06-30T17:03:38Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+
+    if (uploadedSegmentIsEncrypted) {
+      if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {
+        throw new ControllerApplicationException(LOGGER, String


Add segment name and table name to the exception string

mcvsubbu · 2020-06-30T17:05:25Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+    }
+
+    if (uploadedSegmentIsEncrypted) {
+      if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {


Suggested change

if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {

if (!crypterClassNameInTableConfig.equals(crypterUsedInUploadedSegment)) {

mcvsubbu · 2020-06-30T17:07:27Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

@@ -290,6 +297,34 @@ private String extractHttpHeader(HttpHeaders headers, String name) {
    return value;
  }

+  private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,


Suggested change

private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,

private boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,

Refactored.

mcvsubbu · 2020-07-01T00:35:56Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+
+    // encrypt segment
+    PinotCrypter pinotCrypter = PinotCrypterFactory.create(crypterClassNameInTableConfig);
+    LOGGER.info("Using crypter class {} for encrypting {} to {}.", crypterClassNameInTableConfig, tempDecryptedFile,


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.

mcvsubbu · 2020-07-01T00:37:34Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

-    segmentMetadata = getSegmentMetadata(crypterClassHeader, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
-        metadataProviderClass);
-    return segmentMetadata;
+    LOGGER.info("Downloading segment from {} to {}", currentSegmentLocationURI, destFile.getAbsolutePath());


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

I think the same way, but I added the table name to the logs anyways. For the segment, it is not available here. Segment needs to be downloaded first and then its name can be retrieved from the segment metadata.

mcvsubbu · 2020-07-01T00:38:59Z

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


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

Reverted. I was following the pattern with column and table name in this class.

mcvsubbu

Please fix the log message

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

Co-authored-by: Subbu Subramaniam <mcvsubbu@users.noreply.github.com>

kishoreg · 2020-07-02T01:23:23Z

is Crypter part of the pinot-spi? Should we treat this similar to other plugins?

kishoreg · 2020-07-02T01:25:48Z

...ava/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;


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

If you're talking about the difference between fully qualified name vs just the name of the crypter, then how can we differentiate two crypters which have the same name but different packages?

kishoreg · 2020-07-02T01:26:48Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

-          segmentMetadata =
-              getMetadataForURI(crypterClassName, downloadUri, tempEncryptedFile, tempDecryptedFile, tempSegmentDir,
-                  metadataProviderClass);
+          downloadSegmentFileFromURI(downloadUri, dstFile, tableName);


why are we download the segment?

Downloading logic was there before. I just renamed the functions.
Basically if the download uri, instead of the segment data, is provided, controllers download the segment from the given uri.

kishoreg · 2020-07-02T01:28:25Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

          break;
        default:
          throw new UnsupportedOperationException("Unsupported upload type: " + uploadType);
      }

+      if (uploadedSegmentIsEncrypted) {


better to always go through the crypter with noopcrypter

If we do that, the reader needs to know the assumption that there's always a default noopcrypter which gets used when there's no encryption involved. IMO using the proposed approach, it's more explicit and reads better - if the segment is encrypted, then decryption needs to happen.

kishoreg · 2020-07-02T01:29:28Z

...ava/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 =


is this making a ZK call?

No, it uses an existing TableCache which gets updated upon changes.

kishoreg · 2020-07-02T01:30:34Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+
+    // get crypter class name from table config
+    String crypterClassName = _pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);
+


IMO, its better to always go through the same path and just make the crypter no-op

kishoreg · 2020-07-02T01:33:03Z

I would recommend putting this under the SegmentsValidationAndRetentionConfig instead of creating another top level config.

Why is this the right place for this config. IMO, SegmentsValidationAndRetentionConfig should be broken into multiple sections.

Jackie-Jiang · 2020-07-02T18:51:00Z

I would recommend putting this under the SegmentsValidationAndRetentionConfig instead of creating another top level config.

Why is this the right place for this config. IMO, SegmentsValidationAndRetentionConfig should be broken into multiple sections.

That's why I added a TODO in this config: TODO: Consider break this config into multiple configs
At least we can rename it to SegmentsConfig.
But I don't think this is in the scope of this PR.

sajjad-moradi · 2020-07-02T21:39:08Z

is Crypter part of the pinot-spi?

It is part of pinot-spi.

Should we treat this similar to other plugins?

I'm not sure. I'll let more experienced ppl to chime in.

mcvsubbu requested changes Jun 25, 2020

View reviewed changes

mayankshriv reviewed Jun 25, 2020

View reviewed changes

sajjad-moradi force-pushed the feature/encryption.in.controller branch from 2ec6fab to 190f3f3 Compare June 30, 2020 00:56

mcvsubbu reviewed Jun 30, 2020

View reviewed changes

sajjad-moradi added 3 commits June 30, 2020 15:11

Add segment encryption on Controller based on table config

b31f04e

Applied PR's comments

af5eeab

Addressed PR's comments

055e47a

sajjad-moradi force-pushed the feature/encryption.in.controller branch from 190f3f3 to 055e47a Compare June 30, 2020 22:11

Add license header + equality check on crypters

bd8368a

mcvsubbu reviewed Jul 1, 2020

View reviewed changes

Addressed PR's comments

575407a

mcvsubbu approved these changes Jul 1, 2020

View reviewed changes

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java Outdated Show resolved Hide resolved

Fix log message

289ca9e

Co-authored-by: Subbu Subramaniam <mcvsubbu@users.noreply.github.com>

mcvsubbu merged commit 40a3152 into apache:master Jul 1, 2020

kishoreg reviewed Jul 2, 2020

View reviewed changes


		// get crypter class name from table config
		String crypterClassName = _pinotHelixResourceManager.getCrypterClassNameFromTableConfig(offlineTableName);

	boolean uploadedSegmentIsEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);
	boolean isUploadedSegmentEncrypted = !Strings.isNullOrEmpty(crypterClassNameInHeader);

	if (!crypterClassNameInTableConfig.equalsIgnoreCase(crypterUsedInUploadedSegment)) {
	if (!crypterClassNameInTableConfig.equals(crypterUsedInUploadedSegment)) {

	private Boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,
	private boolean encryptSegmentIfNeeded(String offlineTableName, File tempDecryptedFile, File tempEncryptedFile,

Add segment encryption on Controller based on table config #5617

Add segment encryption on Controller based on table config #5617

Conversation

sajjad-moradi commented Jun 25, 2020 • edited

Description

Testing Done

Jackie-Jiang commented Jun 25, 2020

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npawar commented Jun 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishoreg commented Jun 26, 2020

sajjad-moradi commented Jun 30, 2020

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajjad-moradi Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu left a comment

Choose a reason for hiding this comment

kishoreg commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajjad-moradi Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishoreg commented Jul 2, 2020

Jackie-Jiang commented Jul 2, 2020

sajjad-moradi commented Jul 2, 2020

sajjad-moradi commented Jun 25, 2020 •

edited

npawar commented Jun 25, 2020 •

edited

sajjad-moradi Jun 30, 2020 •

edited

sajjad-moradi Jul 2, 2020 •

edited