hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zhe Zhang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-9295) Add a thorough test of the full KMS code path
Date Mon, 26 Oct 2015 18:29:28 GMT

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

Zhe Zhang commented on HDFS-9295:
---------------------------------

Excellent work Daniel! The test is much more comprehensive and systematic than existing ones
on KMS ACLs. The patch LGTM overall. Some comments below:

# Name clash with {{org.apache.hadoop.crypto.key.kms.server.TestKMSACLs}}. Ideally we can
separate out the non-EZ tests into the original {{TestKMSACLs}} (because intuitively that
should be the place to holistically test KMS ACL logics, including whitelist, blacklist, and
key ACL). But I think we can do that separately. So let's just think of a different name here.
# I think the common parts among {{testGoodWithWhitelist}}, {{testGoodWithKeyAcls}}, and {{testGoodWithWhitelistWithoutBlacklist}}
are worth separating out. This is just for better readability and can be done in a separate
JIRA as well.
# Right now we have {{whitelist + blacklist}}, {{no-whitelist (only key ACLs) + blacklist}},
{{whitelist + no-blacklist}}. Does it make sense to complete the spectrum with {{no-whitelist
+ no-blacklist}}? I think that means only {{KEY_ACL}} settings? The above readability suggestion
might make this easier.
# IIUC, the generated configs from {testGoodWithWhitelist}}, {{testGoodWithKeyAcls}}, and
{{testGoodWithWhitelistWithoutBlacklist}} have the same "actual" permissions, so that they
can be verified by the same {{doFullAclTest}}, right? If so it's worth a brief comment.
# I can see that there are 2 dimensions of tests: you first try to test a comprehensive set
of operations against a certain config, and then test a comprehensive set of configs against
a certain operation. I like this methodology a lot. To make the patch smaller and more tractable,
I suggest we separate the 2 dimensions into 2 patches.

Nits:
# Typo: Exeception -- unless this is a British spelling I'm not aware of :)
# {{UserOp#run}} makes it look like a thread. Maybe something like {{runCommand}}, {{runOperation}},
or {{execute}}?

> Add a thorough test of the full KMS code path
> ---------------------------------------------
>
>                 Key: HDFS-9295
>                 URL: https://issues.apache.org/jira/browse/HDFS-9295
>             Project: Hadoop HDFS
>          Issue Type: Test
>          Components: security, test
>    Affects Versions: 2.7.1
>            Reporter: Daniel Templeton
>            Assignee: Daniel Templeton
>            Priority: Critical
>         Attachments: HDFS-9295.001.patch, HDFS-9295.002.patch
>
>
> TestKMS does a good job of testing the ACLs directly, but they are tested out of context.
 Additional tests are needed that test how the ACL impact key creation, EZ creation, file
creation in an EZ, etc.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message