[
https://issues.apache.org/jira/browse/HDFS-6386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14030127#comment-14030127
]
Andrew Wang commented on HDFS-6386:
-----------------------------------
Hi Charles,
Big patch here, thanks for working on this. I didn't give this a full review,
started but figured I'd put up a few high-level comments to chew on first:
* We're holding the FSN lock while potentially doing RPCs to a keyserver. This
might be very slow. The new scheme that Tucu proposed on HDFS-6134 should help
avoid this, since the NN no longer needs to get key material.
* Passing down the KP from NameNode to NNRPCServer to FSNamesystem to
FSDirectory leads to a lot of churn in method signatures. Let's move KP down to
a more appropriate class, like FSNamesystem or FSDirectory. Accessing the KP
directly where needed will also save a lot of passing and churn.
* There are also places where the KP or key/iv are null, which look like places
where we forgot to populate them. This is another reason to try populating in
FSDirectory instead, since it'll be centralized.
* It might be possible to split creation/deletion/listing of an EZ from wiring
through the key/iv everywhere. Would make this easier to review.
Few other things I noticed from a quick pass, again far from a full review:
* Should we be hardcoding the key size of 128? I imagine it depends on the
cipher type, which is something the key server and client would be deciding.
This also doesn't specify bytes or bits either, a javadoc would be good.
* Need to use two stars {{/** */}} for javadoc to kick in on a comment
NameNode:
* Side note, we should have initted the KP before loadNamesystem, since we can
validate the KP before reading the namesystem from disk. Goal should be to fail
fast.
FSNamesystem:
* Don't need to import CRYPTO_XATTR_KEY_ID. UnresolvedPathException also got
pulled in by mismatching Javadoc, should be UnresolvedLinkException.
* Rather than modifying FSN#getXAttrs, you can call the FSDirectory methods
directly when you need an xattr. This avoids the perm checking overhead and so
on too.
* s/allEncryptionZones/encryptionZones
* getFileEncryptionInfo looks up the inode again, when we already have it in
getBlockLocationsUpdateTimes. This is inefficient, so let's try doing this in
FSDirectory.
* startFileInfo, it really looks like getFileInfo should be figuring out the
key and IV
* KeyAndIV could precondition check that neither are null?
* setFileKeyMaterial, keyId.isEmpty() would be more idiomatic.
* getParentEZ is not named well, since it returns the keyID, not the EZ. Not
sure how useful this method is, since it's only used once.
* createEncryptionZone, can we just reuse {{keyId}} and not have {{keyId1}}?
* Not sure why we're dealing with Paths on the NN, since that's a client-side
thing. Let's use strings.
> HDFS Encryption Zones
> ---------------------
>
> Key: HDFS-6386
> URL: https://issues.apache.org/jira/browse/HDFS-6386
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode, security
> Reporter: Alejandro Abdelnur
> Assignee: Charles Lamb
> Fix For: fs-encryption (HADOOP-10150 and HDFS-6134)
>
> Attachments: HDFS-6386.4.patch, HDFS-6386.5.patch, HDFS-6386.6.patch
>
>
> Define the required security xAttributes for directories and files within an
> encryption zone and how they propagate to children. Implement the logic to
> create/delete encryption zones.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
