Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.2.0
    • Component/s: c client, java client
    • Labels:
      None

      Description

      It would be nice to be able to root ZooKeeper handles at specific points in the namespace, so that applications that use ZooKeeper can work in their own rooted subtree.

      For example, if ops decides that application X can use the subtree /apps/X and application Y can use the subtree /apps/Y, X can to a chroot to /apps/X and then all its path references can be rooted at /apps/X. Thus when X creates the path "/myid", it will actually be creating the path "/apps/X/myid".

      There are two ways we can expose this mechanism: 1) We can simply add a chroot(String path) API, or 2) we can integrate into a service identifier scheme for example zk://server1:2181,server2:2181/my/root. I like the second form personally.

      1. ZOOKEEPER-237.patch
        88 kB
        Mahadev konar
      2. ZOOKEEPER-237.patch
        87 kB
        Mahadev konar
      3. ZOOKEEPER-237.patch
        67 kB
        Patrick Hunt
      4. ZOOKEEPER-237.patch
        67 kB
        Patrick Hunt

        Issue Links

          Activity

          Hide
          Patrick Hunt added a comment -

          I like the idea of being able to bind an application to a particular subtree. However this binding should be opaque to the client - the client should not be specifying (or circumventing) the root. The operator should map a credential to a root (ala aws s3)

          Show
          Patrick Hunt added a comment - I like the idea of being able to bind an application to a particular subtree. However this binding should be opaque to the client - the client should not be specifying (or circumventing) the root. The operator should map a credential to a root (ala aws s3)
          Hide
          Benjamin Reed added a comment -

          If you bind a credential to a root, you end up tying yourself to a particular authorization scheme and id.

          If ACLs are used, you would not be able to circumvent the root. And by using an identifier not tied to a credential, you can support multiple credentials for a given root. For example, you may have /mySpace1, and you have some clients that are in a trusted cluster that "authenticate" with ip addresses and others that use stronger credentials. You may also have the converse problem where a given credential may have access to /mySpace1 and /yourSpace2, so the root would be ambiguous.

          Show
          Benjamin Reed added a comment - If you bind a credential to a root, you end up tying yourself to a particular authorization scheme and id. If ACLs are used, you would not be able to circumvent the root. And by using an identifier not tied to a credential, you can support multiple credentials for a given root. For example, you may have /mySpace1, and you have some clients that are in a trusted cluster that "authenticate" with ip addresses and others that use stronger credentials. You may also have the converse problem where a given credential may have access to /mySpace1 and /yourSpace2, so the root would be ambiguous.
          Hide
          Patrick Hunt added a comment -

          @ben:

          credential: how about binding the id then?
          acl use: I was thinking that some users might use this instead of acls - similar to the way aws does

          So you are saying then that this is purely a user space feature. Clients can set this for their own convenience, but it has no aspirations towards being a true namespace?

          In which case an ops team might assign a client a particular subtree "/apps/app1", they would configure the ACLs such that the "app1" client would only have access to /apps/app1. The client would then have the ability to "chroot" to /apps/app1 (hopefully using the second form you list above) and therefore skip the need to prefix all paths with /apps/app1. That sounds fine.

          Another benefit is that if the client has to move to a diff part of the heirarchy, this could be done by changing the connection string (chroot) alone. Nice.

          Show
          Patrick Hunt added a comment - @ben: credential: how about binding the id then? acl use: I was thinking that some users might use this instead of acls - similar to the way aws does So you are saying then that this is purely a user space feature. Clients can set this for their own convenience, but it has no aspirations towards being a true namespace? In which case an ops team might assign a client a particular subtree "/apps/app1", they would configure the ACLs such that the "app1" client would only have access to /apps/app1. The client would then have the ability to "chroot" to /apps/app1 (hopefully using the second form you list above) and therefore skip the need to prefix all paths with /apps/app1. That sounds fine. Another benefit is that if the client has to move to a diff part of the heirarchy, this could be done by changing the connection string (chroot) alone. Nice.
          Hide
          Benjamin Reed added a comment -

          could the chroot be done entirely in the client library? it's basically just prepending a prefix to all the requests and stripping the prefix off on watches.

          Show
          Benjamin Reed added a comment - could the chroot be done entirely in the client library? it's basically just prepending a prefix to all the requests and stripping the prefix off on watches.
          Hide
          Patrick Hunt added a comment -

          There are a couple of pros for server:

          1) implement once rather than once for each client java/c

          2) it has the additional benefit of being a security feature, rather than just a user convenience.

          would be interesting to look at the complexity of implementation client vs server.

          Show
          Patrick Hunt added a comment - There are a couple of pros for server: 1) implement once rather than once for each client java/c 2) it has the additional benefit of being a security feature, rather than just a user convenience. would be interesting to look at the complexity of implementation client vs server.
          Hide
          Patrick Hunt added a comment -

          The current patch implements the java client side of chroot. After reviewing the code more closely it became clear
          that implementing this on the server is much harder/errorprone/inefficient than implementing the core functionality
          on the client. This means we don't get enforcement with this JIRA, however ZOOKEEPER-424 was created to
          address that feature enhancement.

          As it currently stands the client can set a chroot during client (ZooKeeper) object creation which will provide
          similar functionality to unix chroot command. See the updated API and forrest docs for details.

          I added 3 tests, two of which re-use existing test functionality. I'm validating both the synchronous as well
          as async operations.

          Note: I made delete("/") have the same semantics w/ or w/o chroot being used. I thought this best, although
          not strictly necessary.

          Note: /zookeeper is only available if chroot is not used (we don't map it into the chroot for example). I don't think
          we would want to do that anyway. but wanted to not it here for review.

          When updating the client code I introduced the concept of clientpath and serverpath to make the distinction more
          clear.

          Note that we validate both the chroot path as well as the clientPath, we don't validate the serverpath, assuming
          that it's valid. Also this ensures that clientpaths start, for example, with / character

          Note: watchregistration objects resulted in a very nice way of mapping client/server paths in watches that don't require
          us to do any prefix elimination.

          In general I was pretty happy with the (small) number of changes required to implement this and also the decent
          test coverage we currently have.

          Mahadev should take this next and implement the c client changes, hopefully they will be similar.

          Show
          Patrick Hunt added a comment - The current patch implements the java client side of chroot. After reviewing the code more closely it became clear that implementing this on the server is much harder/errorprone/inefficient than implementing the core functionality on the client. This means we don't get enforcement with this JIRA, however ZOOKEEPER-424 was created to address that feature enhancement. As it currently stands the client can set a chroot during client (ZooKeeper) object creation which will provide similar functionality to unix chroot command. See the updated API and forrest docs for details. I added 3 tests, two of which re-use existing test functionality. I'm validating both the synchronous as well as async operations. Note: I made delete("/") have the same semantics w/ or w/o chroot being used. I thought this best, although not strictly necessary. Note: /zookeeper is only available if chroot is not used (we don't map it into the chroot for example). I don't think we would want to do that anyway. but wanted to not it here for review. When updating the client code I introduced the concept of clientpath and serverpath to make the distinction more clear. Note that we validate both the chroot path as well as the clientPath, we don't validate the serverpath, assuming that it's valid. Also this ensures that clientpaths start, for example, with / character Note: watchregistration objects resulted in a very nice way of mapping client/server paths in watches that don't require us to do any prefix elimination. In general I was pretty happy with the (small) number of changes required to implement this and also the decent test coverage we currently have. Mahadev should take this next and implement the c client changes, hopefully they will be similar.
          Hide
          Mahadev konar added a comment -

          pat, this patch does not apply any more, can you update the patch to merge with the trunk?

          Show
          Mahadev konar added a comment - pat, this patch does not apply any more, can you update the patch to merge with the trunk?
          Hide
          Patrick Hunt added a comment -

          merged into latest trunk

          Show
          Patrick Hunt added a comment - merged into latest trunk
          Hide
          Patrick Hunt added a comment -

          oops, cancel, waiting for c code from Mahadev

          Show
          Patrick Hunt added a comment - oops, cancel, waiting for c code from Mahadev
          Hide
          Mahadev konar added a comment -

          This patch implements the c and includes the java part as well. It has unit tests for watcher testings/and all the apis testing with chroot enabled.

          Show
          Mahadev konar added a comment - This patch implements the c and includes the java part as well. It has unit tests for watcher testings/and all the apis testing with chroot enabled.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12411450/ZOOKEEPER-237.patch
          against trunk revision 787789.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 23 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 release audit. The applied patch does not increase the total number of release audit 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/Zookeeper-Patch-vesta.apache.org/129/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/129/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/129/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12411450/ZOOKEEPER-237.patch against trunk revision 787789. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 23 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 release audit. The applied patch does not increase the total number of release audit 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/Zookeeper-Patch-vesta.apache.org/129/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/129/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/129/console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          looks good. two comments:

          • feel free to ignore this one: when you setup hostname and chroot, i think the code is simpler if you hostname = strdup(host) and then poke a null into hostname to strip off the chroot
          • we need to make sure we have total coverage for the testcases. you are missing a couple of the synchronous calls and you need to add the asynchronous calls. (i know it is tedious)
          Show
          Benjamin Reed added a comment - looks good. two comments: feel free to ignore this one: when you setup hostname and chroot, i think the code is simpler if you hostname = strdup(host) and then poke a null into hostname to strip off the chroot we need to make sure we have total coverage for the testcases. you are missing a couple of the synchronous calls and you need to add the asynchronous calls. (i know it is tedious)
          Hide
          Mahadev konar added a comment -

          good points ben

          • added tests for zoo_wget_children and added a test for async create just for checking purposes.
          Show
          Mahadev konar added a comment - good points ben added tests for zoo_wget_children and added a test for async create just for checking purposes.
          Hide
          Mahadev konar added a comment -

          attached the wrong pathc,

          Show
          Mahadev konar added a comment - attached the wrong pathc,
          Hide
          Benjamin Reed added a comment -

          +1 good to go! nice job.

          Show
          Benjamin Reed added a comment - +1 good to go! nice job.
          Hide
          Mahadev konar added a comment -

          I just committed this. thanks pat.

          Show
          Mahadev konar added a comment - I just committed this. thanks pat.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #357 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/357/)
          . Add a Chroot request (phunt and mahadev)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #357 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/357/ ) . Add a Chroot request (phunt and mahadev)

            People

            • Assignee:
              Mahadev konar
              Reporter:
              Benjamin Reed
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development