|
[
Permlink
| « Hide
]
Pete Wyckoff added a comment - 07/Sep/08 11:41 PM
This is the combination of 3 previous jiras but since all really related, I am combining them and supplying 1 patch.
NOTE: this includes https://issues.apache.org/jira/browse/HADOOP-3963
 which is waiting to be committed.
Otherwise, this patch is basically complete - all tests have been added and pass. One caveat is setting/getting access times. Since there's no way right now to enable this in the libhdfs testing framework, there's really no way to have a unit test for it. Pete,
Looks good so far.
Hi Craig,
Yes, I agree that's a good idea - probably should be enforced in the filesystem java api? we should get Nicholas to comment on why they haven't done this; now that you bring this up, I'm surprised it doesn't raise an exception - I ran the unit tests as me and not root. pete My understanding is that Hadoop has no real authentication in it - it just assumes that what UGI gives is correct. This is possibly the first scenario where we're bending UGI to give something different. That being the case, we should provide the security.
I think any security checks in Hadoop will depend how Hadoop implements authentication and security in due course. In the meantime, it's probably good that we enforce some security in the mean time. My understanding from http://en.allexperts.com/q/Unix-Linux-OS-1064/real-effective-user-id.htm -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12389646/HADOOP-4104.txt against trunk revision 692700. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3200/testReport/ This message is automatically generated. -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12389646/HADOOP-4104.txt against trunk revision 692700. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3201/testReport/ This message is automatically generated. in that case, shouldn't the dfsclient enforce it too?
RE: -1 unit tests – Craig and I added a bunch of them to hdfs_test.c just hudson doesn't know! My opinion would be to avoid adding security and access checks that are specific to libhdfs only. Does libhdfs work on non-unix platforms? If so, then we should avoid using calls like geteuid(). It will keep the library very-hadoop-centric rather than being unix-centric.
+1 Hadoop doesn't have authentication. The ugi and permissions implementation is aimed at preventing mistakes rather than providing any real security. I don't see why libhdfs should take it upon itself to do more. Any future authentication work in Hadoop can be leveraged by libhdfs. This is the patch without the hadoop-4113 diff since 4113 has been committed.
submitting final patch for QA
Sameer and Dhrubas have a good point. I think we should then leave the patch as is. Craig - what do you think? The only thing I removed from the patch were the 4113 changes since they are already committed. Otherwise, I haven't changed anything from the patch you looked at.
should make one other comment since 1/2 of this code is from Craig himself in hadoop-3264. That part of the code I have reviewed before I inserted it and it looks good and I think may have also been looked at by Nicolas.
– pete Yes, I agree. Hadoop doesnt have a authentication system, so we shouldn't force one on it in this patch.
+1 on @ Sameer - I agree that libhdfs can leverage any additional authentication functionality. I should just make clear that a requirement of any authentication change should be "impersonation" by a process running with superuser privileges, as provided hdfsConnectAsUser(). -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12389775/HADOOP-4104.2.txt against trunk revision 693587. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3223/testReport/ This message is automatically generated. RE: -1 Tests - the patch includes a whole bunch of new tests for all the new functionality - its' in hdfs_test.c so hudson doesn't see it.
noticed a bug. This is not handling conversion to milliseconds in hdfsUtimes. Its params are (should be) in seconds and the hdfs API is in milliseconds so need to convert.
I also fixed the tests to properly check chmod and permissions which it wasn't doing as hdfs only does those things on files, not directories.
had a bug in handling of times in writes were expecting milliseconds when the API really calls for seconds since a tTime is a time_t which is typically seconds.
Some comments on
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12389865/HADOOP-4104.3.txt against trunk revision 693705. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3237/testReport/ This message is automatically generated. cancel to address nicholas' comments.
UserGroupInformation doesn't have a constructor?? How do I construct it without already having the login information in a Configuration object?
You are right. UserGroupInformation is abstract, so you must create a subclass object. However, the subclass object can be referred as UserGroupInformation to minimize the reference of the subclass. e.g. UserGroupInformation ugi = new UnixUserGroupInformation(...).
The problem is though that UGI does not support saveToConf, which is what I really want to get at and even that in UUGI, takes a UUGI.
Is there another way to connect as a specific user given that I know the user and groups? It can be done by UGI.setCurrentUGI(...).
But that isn't associated with a specific filesystem connection, so what are the semantics if you have multiple connections? This patch fixes the bug of setting user to root and group to users when null. I also added some #ifdef-ed out code for using the setCurrentUGI method in UuserGroupInformation but not clear on the semantics wrt multiple connections and also doesn't work.
This patch passes ant -Dlibhdfs=1 test-libhdfs but will wait and see if there's an easy way to get away from the UnixUserGroupInformation object before submitting it as a patch. I am going to submit as is and once we figure out the UUGI versus UGI since we have a week before 0.19. Can do a quick fix later when UUGI goes away or sometime later.
> But that isn't associated with a specific filesystem connection, so what are the semantics if you have multiple connections?
In this case, it seems better using UUGI and conf. The patch looks good to me but, again, I don't know much about libhdfs. +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12389977/HADOOP-4104.4.txt against trunk revision 695690. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3266/testReport/ This message is automatically generated. Craig, can you peruse the final patch and give a thumbs up?
+1. I've read through, all code looks good. Good to see API docs in libhdfs.h were updated also
C Just committed this. Thanks Pete!
Committed revision 696741. Resolving issue now that this has been committed.
Integrated in Hadoop-trunk #611 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/611/
) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
New Feature
Closed
Major
add time, permission and user attribute support to libhdfs
