HADOOP-17536. ABFS: Supporting customer provided encryption key #2707

bilaharith · 2021-02-19T05:02:22Z

The data for a particular customer needs to be encrypted on account level. At server side the APIs will start accepting the encryption key as part of request headers. The data will be encrypted/decrypted with the given key at the server.

Since the ABFS FileSystem APIs are implementations for Hadoop FileSystem APIs there is no direct way with which customer can pass the key to ABFS driver. In this case driver should have the following capabilities so that it can accept and pass the encryption key as one of the request headers.

There should be a way to configure the encryption key for different accounts.
If there is a key specified for a particular account, the same needs to be sent along with the request headers.
Config changes

They key for an account can be specified in the core-site as follows.

fs.azure.account.client-provided-encryption-key.{account name}.dfs.core.windows.net

hadoop-yetus · 2021-02-19T06:19:55Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	2m 8s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚		0m 0s	test4tests	The patch appears to include 2 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	34m 3s		trunk passed
+1 💚	compile	0m 39s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	compile	0m 32s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	checkstyle	0m 29s		trunk passed
+1 💚	mvnsite	0m 37s		trunk passed
+1 💚	shadedclient	14m 29s		branch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 29s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 29s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+0 🆗	spotbugs	0m 59s		Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	0m 57s		trunk passed
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 29s		the patch passed
+1 💚	compile	0m 29s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javac	0m 29s		the patch passed
+1 💚	compile	0m 24s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	javac	0m 24s		the patch passed
-0 ⚠️	checkstyle	0m 17s	/diff-checkstyle-hadoop-tools_hadoop-azure.txt	hadoop-tools/hadoop-azure: The patch generated 8 new + 5 unchanged - 0 fixed = 13 total (was 5)
+1 💚	mvnsite	0m 28s		the patch passed
+1 💚	whitespace	0m 0s		The patch has no whitespace issues.
+1 💚	shadedclient	12m 40s		patch has no errors when building and testing our client artifacts.
+1 💚	javadoc	0m 26s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 23s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
-1 ❌	findbugs	1m 3s	/new-findbugs-hadoop-tools_hadoop-azure.html	hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
			_ Other Tests _
+1 💚	unit	2m 0s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 32s		The patch does not generate ASF License warnings.
		76m 24s

Reason	Tests
FindBugs	module:hadoop-tools/hadoop-azure
	Found reliance on default encoding in new org.apache.hadoop.fs.azurebfs.services.AbfsClient(URL, SharedKeyCredentials, AbfsConfiguration, AbfsClientContext):in new org.apache.hadoop.fs.azurebfs.services.AbfsClient(URL, SharedKeyCredentials, AbfsConfiguration, AbfsClientContext): String.getBytes() At AbfsClient.java:[line 104]

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/1/artifact/out/Dockerfile
GITHUB PR	#2707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux 402b0297a4da 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `9a298d1`
Default Java	Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/1/testReport/
Max. process+thread count	535 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/1/console
versions	git=2.25.1 maven=3.6.3 findbugs=4.0.6
Powered by	Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran

I can see the need for this.

One thing that it's not yet covered is the need to get this into delegation tokens. Look at what was done for the S3A DT's there: the encryption settings and secret key is picked up on the client and passed into the cluster so users can submit jobs into a shared cluster, without that shared cluster having the encryption key. Is this needed here? If so AbfsDelegationTokenIdentifier will need to be extended.
is it ever the case that different stores will have different keys?

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

bilaharith · 2021-02-26T04:39:20Z

HNS-OAuth

[INFO] Results:
[INFO]
[INFO] Tests run: 93, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 541, Failures: 0, Errors: 0, Skipped: 68
[INFO] Results:
[INFO]
[WARNING] Tests run: 257, Failures: 0, Errors: 0, Skipped: 48

HNS-SharedKey

[INFO] Results:
[INFO]
[INFO] Tests run: 93, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 541, Failures: 0, Errors: 0, Skipped: 26
[INFO] Results:
[INFO]
[WARNING] Tests run: 257, Failures: 0, Errors: 0, Skipped: 40

NonHNS-SharedKey

[INFO] Results:
[INFO]
[INFO] Tests run: 93, Failures: 0, Errors: 0, Skipped: 0
[INFO] Results:
[INFO]
[WARNING] Tests run: 541, Failures: 0, Errors: 0, Skipped: 257
[INFO] Results:
[INFO]
[WARNING] Tests run: 257, Failures: 0, Errors: 0, Skipped: 40

vinaysbadami · 2021-03-10T05:44:13Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

    return requestHeaders;
  }

+  private void addServerSideEncryptionHeaders(


addCustomerProvidedKeyHeaders

vinaysbadami · 2021-03-10T05:47:26Z

...op-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsClientCustomerProvidedKey.java

+  private static final int INT_512 = 512;
+  private static final int INT_50 = 50;
+
+  private final IdentityTransformerInterface identityTransformer;


vinaysbadami · 2021-03-10T05:48:36Z

...op-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsClientCustomerProvidedKey.java

+  @Test
+  public void testAppendWithCPK() throws Exception {
+    boolean isWithCPK = true;
+    final AzureBlobFileSystem fs = getAbfs(isWithCPK);


vinaysbadami · 2021-03-10T05:57:07Z

...op-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsClientCustomerProvidedKey.java

+            .equalsIgnoreCase(headerName)).findFirst();
+    String desc;
+    if (isCPKHeaderExpected) {
+      desc = "CPK hear should be resent";


vinaysbadami · 2021-03-10T05:59:37Z

...op-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsClientCustomerProvidedKey.java

+  }
+
+  @Test
+  public void testAppendWithoutCPK() throws Exception {


we should have test cases for 2 CPK enabled accounts to ensure all is good.

bilaharith · 2021-03-25T19:08:17Z

I can see the need for this.

One thing that it's not yet covered is the need to get this into delegation tokens. Look at what was done for the S3A DT's there: the encryption settings and secret key is picked up on the client and passed into the cluster so users can submit jobs into a shared cluster, without that shared cluster having the encryption key. Is this needed here? If so AbfsDelegationTokenIdentifier will need to be extended.

is it ever the case that different stores will have different keys?

Yes, each account can have it's own key.

hadoop-yetus · 2021-03-25T20:16:25Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 47s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	shelldocs	0m 1s		Shelldocs was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 5 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	32m 42s		trunk passed
+1 💚	compile	0m 46s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	compile	0m 40s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	checkstyle	0m 33s		trunk passed
+1 💚	mvnsite	0m 44s		trunk passed
+1 💚	javadoc	0m 39s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 35s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 6s		trunk passed
+1 💚	shadedclient	14m 3s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	14m 25s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 32s		the patch passed
+1 💚	compile	0m 32s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javac	0m 32s		the patch passed
+1 💚	compile	0m 29s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	javac	0m 29s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 20s	/results-checkstyle-hadoop-tools_hadoop-azure.txt	hadoop-tools/hadoop-azure: The patch generated 3 new + 7 unchanged - 0 fixed = 10 total (was 7)
+1 💚	mvnsite	0m 32s		the patch passed
+1 💚	shellcheck	0m 0s		No new issues.
+1 💚	xml	0m 2s		The patch has no ill-formed XML file.
+1 💚	javadoc	0m 25s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 24s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 0s		the patch passed
+1 💚	shadedclient	14m 2s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	1m 57s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 38s		The patch does not generate ASF License warnings.
		75m 4s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/6/artifact/out/Dockerfile
GITHUB PR	#2707
Optional Tests	dupname asflicense mvnsite unit codespell shellcheck shelldocs compile javac javadoc mvninstall shadedclient spotbugs checkstyle xml
uname	Linux faa57a6d0dfc 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `173bc3c`
Default Java	Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/6/testReport/
Max. process+thread count	546 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/6/console
versions	git=2.25.1 maven=3.6.3 shellcheck=0.7.0 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

vinaysbadami · 2021-03-26T04:42:57Z

hadoop-tools/hadoop-azure/dev-support/testrun-scripts/runtests.sh

 properties=("fs.azure.abfs.account.name" "fs.azure.test.namespace.enabled"
 "fs.azure.account.auth.type")
-values=("{account name}.dfs.core.windows.net" "true" "OAuth")
+values=("abfsitgen2.dfs.core.windows.net" "true" "OAuth")


hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

vinaysbadami · 2021-03-26T09:52:55Z

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

+            .equalsIgnoreCase(headerName)).findFirst();
+    String desc;
+    if (isCPKHeaderExpected) {
+      desc = "CPK header is expected, but the same is absent.";


mention header name

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

hadoop-tools/hadoop-azure/src/test/resources/azure-test.xml

bilaharith

Please review

snvijaya · 2021-04-05T07:36:57Z

...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java

  /** Setting this true will make the driver use it's own RemoteIterator implementation */
  public static final String FS_AZURE_ENABLE_ABFS_LIST_ITERATOR = "fs.azure.enable.abfslistiterator";
+  /** Server side encryption key */
+  public static final String FS_AZURE_CLIENT_PROVIDED_ENCRYPTION_KEY = "fs.azure.client-provided-encryption-key";


dot as a delimiter has been the trend. Any reason to deviate ?

we should not use the dot here. The dot is used to namespace. - is fine else make it one word

snvijaya · 2021-04-05T07:37:56Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

 */
 public class AbfsClient implements Closeable {
  public static final Logger LOG = LoggerFactory.getLogger(AbfsClient.class);
+  private static final String SERVER_SIDE_ENCRYPTION_ALGORITHM = "AES256";


Move these to FileSystemConfigurations.java

snvijaya · 2021-04-05T07:52:51Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java


  public AbfsRestOperation getPathStatus(final String path, final boolean includeProperties) throws AzureBlobFileSystemException {
    final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
+    addCustomerProvidedKeyHeaders(requestHeaders);


When includeProperties is true, REST API GetFileProperties is invoked (which has user metadata in response) and is the case where CPK headers are needed.
REST API GetPathStatus doesnt consume CPK headers. Tomorrow if there is a rejection of headers not consumed, requests will fail.
addCPK for includeProperties=true case.

snvijaya · 2021-04-05T07:53:31Z

...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java

    // dump the headers
    AbfsIoUtils.dumpHeadersToDebugLog("Response Headers",
-        connection.getHeaderFields());
+        responseHeaders);


Why is this change required ?

New method is introduced to print response headers which simply accepts the list of response headers

snvijaya · 2021-04-05T07:54:42Z

...ls/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java


      // Reset URL with configured filesystem
-      final String abfsUrl = this.getFileSystemName() + "@" + this.getAccountName();
+      final String abfsUrl = this.getFileSystemName() + "@" + authorityParts[1];


Why is this change required ?

This was buggy. authorityParts[1] is the actual FS to be loaded, getAccountName returns the FS that is created dynamically.

snvijaya · 2021-04-05T07:55:10Z

...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java


  private String getUserAgentString(AbfsConfiguration config,
-      boolean includeSSLProvider) throws MalformedURLException {
+      boolean includeSSLProvider) throws IOException {


Why is this change required ?

AbfsClient constructor could throw IOException, wiuth the new encoding and hashing logic included

snvijaya · 2021-04-05T07:55:25Z

...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java

      AbfsClient baseAbfsClientInstance,
-      AbfsConfiguration abfsConfig)
-      throws AzureBlobFileSystemException {
+      AbfsConfiguration abfsConfig) throws IOException {


Why is this change required ?

AbfsClient constructor could throw IOException, wiuth the new encoding and hashing logic included

snvijaya · 2021-04-05T09:29:53Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

  public AbfsRestOperation read(final String path, final long position, final byte[] buffer, final int bufferOffset,
                                final int bufferLength, final String eTag, String cachedSasToken) throws AzureBlobFileSystemException {
    final List<AbfsHttpHeader> requestHeaders = createDefaultHeaders();
+    addCustomerProvidedKeyHeaders(requestHeaders);


For the purposes of debugging, add a log line if request fails and the sha256 of the key used in request and the server returned sha256 in response are different.

Server doesn't send back the actual sha of the encryption key with which the data is encrypted if the encryption key sent is wrong

snvijaya

Please check the comments.

snvijaya

+1 - Some minor comments as discussed. Please address.

bilaharith

Hi @steveloughran Could you please take a look

hadoop-yetus · 2021-04-09T03:59:54Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 36s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 3 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	33m 53s		trunk passed
+1 💚	compile	0m 38s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	compile	0m 33s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	checkstyle	0m 28s		trunk passed
+1 💚	mvnsite	0m 40s		trunk passed
+1 💚	javadoc	0m 32s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 29s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 0s		trunk passed
+1 💚	shadedclient	14m 9s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	14m 29s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 29s		the patch passed
+1 💚	compile	0m 30s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javac	0m 30s		the patch passed
+1 💚	compile	0m 26s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	javac	0m 26s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 17s	/results-checkstyle-hadoop-tools_hadoop-azure.txt	hadoop-tools/hadoop-azure: The patch generated 3 new + 5 unchanged - 0 fixed = 8 total (was 5)
+1 💚	mvnsite	0m 29s		the patch passed
+1 💚	javadoc	0m 22s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 21s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 0s		the patch passed
+1 💚	shadedclient	13m 41s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	1m 54s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 33s		The patch does not generate ASF License warnings.
		74m 19s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/13/artifact/out/Dockerfile
GITHUB PR	#2707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell
uname	Linux 4db2c454c8f1 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `09b462b`
Default Java	Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/13/testReport/
Max. process+thread count	668 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/13/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran · 2021-04-12T13:04:33Z

Hi @steveloughran Could you please take a look

I've been on a little vacation. I did have some review which was unsubmitted...now looks out of date. Will review again

steveloughran

Really good test coverage; I do think there is duplication in there of disabling caching and creating new filesystems then invoking an operation to raise an IOE. At the very least, combine the conf.set, filesystem.get and the casting to some createNewABFS(Configuration) method.

If CPK mismatch results in a specific IOE subclass (and it should!) this is what intercept should look for

Avoids any other reason for the call to fail being interpreted as success in the test.
guarantees that subclassed exception will be raised when expected.

I'm afraid we also need some documentation somewhere. This is quite a large new feature and needs coverage. Yes software engineers get the help with the documentation too. This can be beneficial: if you find something is hard to explain, it will be hard to use.

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

steveloughran · 2021-04-12T13:30:02Z

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

+import org.apache.hadoop.fs.permission.AclEntry;
+import org.apache.hadoop.fs.permission.FsAction;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.thirdparty.com.google.common.base.Preconditions;


Now, these do need to go up into the "non org-apache section"; relates to how backporting usually needs to revert these back to the com.google package

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

steveloughran · 2021-04-12T13:39:11Z

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

+    String expectedCPKSha = getCPKSha(fs);
+
+    byte[] fileContent = getRandomBytesArray(FILE_SIZE);
+    Path testFilePath = new Path(testFileName+"1");


nit: spacing

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

bilaharith

Requesting you to review

steveloughran

looks good, some minor test details

+1 pending those changes (or you disagreeing with me about their need)

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

steveloughran · 2021-04-22T18:33:29Z

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

+    conf.set(FS_AZURE_CLIENT_PROVIDED_ENCRYPTION_KEY + "." + accountName,
+        "different-1234567890123456789012");
+    conf.set("fs.abfs.impl.disable.cache", "true");
+    AzureBlobFileSystem fs2 = (AzureBlobFileSystem) FileSystem.get(conf);


again, you can use FileSystem.newInstance()

...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestCustomerProvidedKey.java

...adoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/constants/TestConfigurationKeys.java

bilaharith

Hi Steve, Please review.

hadoop-yetus · 2021-04-25T19:01:34Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	7m 3s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 3 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	35m 11s		trunk passed
+1 💚	compile	0m 38s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	compile	0m 32s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	checkstyle	0m 27s		trunk passed
+1 💚	mvnsite	0m 41s		trunk passed
+1 💚	javadoc	0m 33s		trunk passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 31s		trunk passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 4s		trunk passed
+1 💚	shadedclient	14m 7s		branch has no errors when building and testing our client artifacts.
-0 ⚠️	patch	14m 26s		Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 31s		the patch passed
+1 💚	compile	0m 30s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javac	0m 30s		the patch passed
+1 💚	compile	0m 27s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	javac	0m 27s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 17s		the patch passed
+1 💚	mvnsite	0m 31s		the patch passed
+1 💚	javadoc	0m 23s		the patch passed with JDK Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 23s		the patch passed with JDK Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
+1 💚	spotbugs	1m 6s		the patch passed
+1 💚	shadedclient	13m 44s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	1m 58s		hadoop-azure in the patch passed.
+1 💚	asflicense	0m 30s		The patch does not generate ASF License warnings.
		82m 34s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/17/artifact/out/Dockerfile
GITHUB PR	#2707
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell
uname	Linux 0f36979737b8 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `4e5268f`
Default Java	Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.10+9-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_282-8u282-b08-0ubuntu1~20.04-b08
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/17/testReport/
Max. process+thread count	539 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2707/17/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran · 2021-04-27T10:16:11Z

+1, merged to trunk. Thank you!

Contributed by bilahari t h Change-Id: I86216e755b81e9d14f5e87844d9fd58e8940560c

…he#2707) Contributed by bilahari t h

…on key (apache#2707) Note: Contains one file change to "ITestCustomerProvidedKey.java" from "CDPD-29718. HADOOP-17290. ABFS: Add Identifiers to Client Request Header" required for not breaking the build and was previously skipped. Contributed by bilahari t h Change-Id: I86216e755b81e9d14f5e87844d9fd58e8940560c

Supporting customer provided encryption key

0e5ed8a

steveloughran requested changes Feb 22, 2021

View reviewed changes

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java Outdated Show resolved Hide resolved

bilaharith added 2 commits February 23, 2021 19:37

Merge branch 'trunk' into cpk

05aba4e

findbugs and checkstyle fixes

87335a3

bilaharith added 2 commits March 9, 2021 16:24

Added test cases

9e93938

Fixes in test cases

cc4b587

bilaharith changed the title ~~DRAFT PR: HADOOP-17536. ABFS: Supporting customer provided encryption key~~ HADOOP-17536. ABFS: Supporting customer provided encryption key Mar 10, 2021

vinaysbadami reviewed Mar 10, 2021

View reviewed changes

bilaharith added 3 commits March 12, 2021 09:53

Review comments addressed

fd4c2d0

Adding more test cases

536b5d1

Removed assertions on error messages

173bc3c

Removing runtests.sh from the PR

8844880

vinaysbadami suggested changes Mar 26, 2021

View reviewed changes

bilaharith added 2 commits March 30, 2021 21:43

Addressing review comments

0c04e50

Removing azure-test.xml

1ebf5ae

bilaharith commented Mar 31, 2021

View reviewed changes

Addressing review comments

2cd36d7

vinaysbadami approved these changes Mar 31, 2021

View reviewed changes

Merge branch 'trunk' into cpk

4ce069d

snvijaya reviewed Apr 5, 2021

View reviewed changes

snvijaya suggested changes Apr 5, 2021

View reviewed changes

Addressing review comments

8da250b

snvijaya approved these changes Apr 8, 2021

View reviewed changes

bilaharith added 2 commits April 9, 2021 07:50

Checkstyle fixes

176eb38

Checkstyle fixes

09b462b

bilaharith commented Apr 9, 2021

View reviewed changes

apache deleted a comment from hadoop-yetus Apr 12, 2021

steveloughran requested changes Apr 12, 2021

View reviewed changes

Addressing review comments

53d4900

bilaharith commented Apr 15, 2021

View reviewed changes

steveloughran requested changes Apr 22, 2021

View reviewed changes

bilaharith added 2 commits April 25, 2021 22:48

Addressing review comments

8c94d32

Merge branch 'trunk' into cpk

25f63ce

bilaharith commented Apr 25, 2021

View reviewed changes

Fixing signatures related to the backmerge from trunk

4e5268f

apache deleted a comment from hadoop-yetus Apr 27, 2021

steveloughran merged commit f54e764 into apache:trunk Apr 27, 2021

asfgit pushed a commit that referenced this pull request Apr 27, 2021

HADOOP-17536. ABFS: Supporting customer provided encryption key (#2707)

6649e58

Contributed by bilahari t h Change-Id: I86216e755b81e9d14f5e87844d9fd58e8940560c

kiran-maturi pushed a commit to kiran-maturi/hadoop that referenced this pull request Nov 24, 2021

HADOOP-17536. ABFS: Supporting customer provided encryption key (apac…

870560b

…he#2707) Contributed by bilahari t h

HADOOP-17536. ABFS: Supporting customer provided encryption key #2707

HADOOP-17536. ABFS: Supporting customer provided encryption key #2707

Uh oh!

Conversation

bilaharith commented Feb 19, 2021

Uh oh!

hadoop-yetus commented Feb 19, 2021

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bilaharith commented Feb 26, 2021

HNS-OAuth

HNS-SharedKey

NonHNS-SharedKey

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bilaharith commented Mar 25, 2021

Uh oh!

hadoop-yetus commented Mar 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bilaharith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!