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