hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Liu (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-6392) Wire crypto streams for encrypted files in DFSClient
Date Tue, 03 Jun 2014 04:01:02 GMT

    [ https://issues.apache.org/jira/browse/HDFS-6392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14016195#comment-14016195
] 

Yi Liu commented on HDFS-6392:
------------------------------

Thanks Charles for the patch. Generally looks very good.  I have some comments after your
update (Sorry for that some comments were not put together with my previous comment, not finished
review at that time.)

*1.*
{quote} I also removed the code in HdfsDataInputStream#getWrappedStream().
and {code} 
-    public DFSDataInputStream(DFSInputStream in) throws IOException {
+    public DFSDataInputStream(InputStream in) throws IOException {
{code}
{quote}
*Maybe there is one issue, please look following logic:*

In the patch, HdfsDataInputStream(InputStream in) will be created by passing {{CryptoInputStream}}
in crypto case, but in {{HdfsDataInputStream}}, there are lots of casting {{((DFSInputStream)
getWrappedStream())}}, so it will throw casting exception since {{CryptoInputStream}} not
instanceof {{DFSInputStream}}.
 
BTW, I suggest we don’t modify the input stream from {{DFSInputStream}} to a more general
type for HdfsDataInputStream constructor, since the original code may have some assumption
based on  the type.

Maybe you could add test for {{getCurrentDatanode, getCurrentBlock, getAllBlocks  and so on…}}.
My suggest is we can define some class like {{CryptoDFSInputStream}} to extend DFSInputStream
and wrap CryptoInputStream, like we do for {{CryptoFSDataInputStream}} .


*2.* Same consideration for {{HdfsDataoutputStream}}, I suggest we don’t modify the output
stream to a more general type for constructor, since the original code may have some assumption
based on  the type.


*3.* in FSDataOutputStream, unnecessary import.


*4.* in {{DFSClient}}, the {{public DFSDataInputStream(DFSInputStream in) throws IOException}}
is deprecated, so why not use {{HdfsDataInputStream}} directly in {code}public HdfsDataInputStream
open(Path f, int bufferSize) {code}, then it's more clear.


*5.* It seems we should add {{byte\[\]}} after {{@return}}, otherwise there will be javadoc
warning when testing patch.
{quote}
+  /**
+   * Get the encryption key for this stream.
+   * @return the key
+   */
+  public synchronized byte\[\] getKey() \{
+    return key;
+  \}
+
+  /**
+   * Get the encryption initialization vector (IV) for this stream.
+   * @return the initialization vector (IV).
+   */
+  public synchronized byte\[\] getIv() \{
+    return iv;
+  \}
+
{quote}


*6.* In {{TestHDFSEncryptionStream}}, can you add more javadoc/comments? 
It seems we have not used the key and do encryption/decryption?  

Furthermore, can you change the name to {{TestHDFSEncryption}}, are you intended to use this
test suite to cover end-to-end test for HDFS encryption?

We have very detailed crypto streams test in HDFS-6405 and the {{TestHdfsCryptoStreams}} 
extends from {{CryptoStreamsTestBase}} and I think the test in that JIRA is necessary and
different from the one in this patch.




>  Wire crypto streams for encrypted files in DFSClient
> -----------------------------------------------------
>
>                 Key: HDFS-6392
>                 URL: https://issues.apache.org/jira/browse/HDFS-6392
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: namenode, security
>            Reporter: Alejandro Abdelnur
>            Assignee: Charles Lamb
>         Attachments: HDFS-6392.1.patch, HDFS-6392.2.patch, HDFS-6392.3.patch
>
>
> When the DFS client gets a key material and IV for a file being opened/created, it should
wrap the stream with a crypto stream initialized with the key material and IV.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Mime
View raw message