[GitHub] [hadoop] steveloughran commented on a change in pull request #2422: HADOOP-17311. ABFS: Masking SAS signatures from logs

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

steveloughran commented on a change in pull request #2422:
URL: https://github.com/apache/hadoop/pull/2422#discussion_r514443770



##########
File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
##########
@@ -256,7 +256,9 @@ private boolean executeHttpOperation(final int retryCount) throws AzureBlobFileS
       }
     } catch (IOException ex) {
       if (ex instanceof UnknownHostException) {
-        LOG.warn(String.format("Unknown host name: %s. Retrying to resolve the host name...", httpOperation.getUrl().getHost()));
+        LOG.warn(String.format(
+            "Unknown host name: %s. Retrying to resolve the host name...",
+            httpOperation.getHost()));

Review comment:
       While you are there
   * add a catch for UnknownHostException
   * move from String.format to Log.warn("unknown host {}", httpOperation,getHost()
   

##########
File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java
##########
@@ -513,4 +509,45 @@ private void parseListFilesResponse(final InputStream stream) throws IOException
   private boolean isNullInputStream(InputStream stream) {
     return stream == null ? true : false;
   }
+
+  @VisibleForTesting
+  public String getSignatureMaskedUrlStr() {
+    if (this.maskedUrlStr != null) {
+      return this.maskedUrlStr;
+    }
+    final String urlStr = url.toString();

Review comment:
       This is complicated enough it could be pulled out into a static method, and so its handling fully tested in (new) Unit tests, as well as in the ITests.

##########
File path: hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelegationSAS.java
##########
@@ -381,4 +383,39 @@ public void testProperties() throws Exception {
 
     assertArrayEquals(propertyValue, fs.getXAttr(reqPath, propertyName));
   }
+
+  @Test
+  public void testSignatureMask() throws Exception {
+    final AzureBlobFileSystem fs = getFileSystem();
+    String src = "/testABC/test.xt";
+    fs.create(new Path(src));
+    AbfsRestOperation abfsHttpRestOperation = fs.getAbfsClient()
+        .renamePath(src, "/testABC" + "/abc.txt", null);
+    AbfsHttpOperation result = abfsHttpRestOperation.getResult();
+    String url = result.getSignatureMaskedUrlStr();
+    String encodedUrl = result.getSignatureMaskedEncodedUrlStr();
+    Assertions.assertThat(url.substring(url.indexOf("sig=")))
+        .describedAs("Signature query param should be masked")
+        .startsWith("sig=XXXX");
+    Assertions.assertThat(encodedUrl.substring(encodedUrl.indexOf("sig%3D")))
+        .describedAs("Signature query param should be masked")
+        .startsWith("sig%3DXXXX");
+  }
+
+  @Test
+  public void testSignatureMaskOnExceptionMessage() {
+    final AzureBlobFileSystem fs;
+    String msg = null;
+    try {
+      fs = getFileSystem();

Review comment:
       use LambdaTestUtils.intercept(). Not only simpler, it will (correctly) fail if the rest operation didn't actually raise an exception

##########
File path: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java
##########
@@ -36,6 +36,7 @@
 import org.codehaus.jackson.JsonParser;
 import org.codehaus.jackson.JsonToken;
 import org.codehaus.jackson.map.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;

Review comment:
       now all shaded I'm afraid. Making backporting harder already




----------------------------------------------------------------
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: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org