> On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > core/src/main/java/org/apache/accumulo/core/security/TableNamespacePermission.java, > > line 26 > > <https://reviews.apache.org/r/15166/diff/1/?file=376101#file376101line26> > > > > I think we also need BULK_IMPORT for bulk importing to namespace > > tables, DROP for dropping the entire namespace, and possibly GRANT_TABLE > > (though I can see that being iffy)
I thought about adding BULK_IMPORT, but I didn't add it because I don't think I added bulk importing for namespaces. I made DROP_NAMESPACE in SystemPermission, but I agree that it should also be in TableNamespacePermission (because TablePermission has DROP_TABLE). I was a little confused how permissions were supposed to work in general, so I don't know how well GRANT_TABLE would work as a namespace permission. > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java, > > line 311 > > <https://reviews.apache.org/r/15166/diff/1/?file=376139#file376139line311> > > > > Part of is wondering we want to change the hasTablePermission(blah) to > > hit both the table and the namespace > > Christopher Tubbs wrote: > I'm a big fan of changing this whole pluggable permissions model to one > where we have a simple: > boolean canPerform(User user, Action action); // possibly with an > optional "environment" as an arbitrary property map > > With a few defined Actions, like TableRenameAction(oldName, newName), > etc. and User with a strict interface like "boolean isAuthenticated(); String > getPrincipal();", the pluggable permissions handler becomes an > operations-based implementation of *policy*, rather than a dumb storage for a > difficult to extend set of permissions *tokens*. The default implementation > will still be token based, but the API will be much cleaner, much more > extensible/pluggable, and wouldn't change when we add new features like this. > > Had we done that, this would be trivial to extend in a sensible way. > > John Vines wrote: > It's still trivial now (more trivial than the current implementation), > you're just using this circumstance to gripe about how you want things to be, > we have other tickets for that. That doesn't change things for this release. > > This change simply adds a namespace argument to the hasTablePermission or > _hasTablePermission call that can be propagated throughout with simple > signature changing. It might prove convenient to have hasTablePermission to check both, but the structure I was following was that each security check (all the "canDoSomething" functions) had a separate check for the table and system permissions that might allow it to do that. Because of that, the permission that might allow you do something might be different for the table vs namespace vs system. This might be from me misunderstanding the permissions model, but for example, when you check for canCompact(table), we're checking for ALTER_TABLE in both System and Namespace permissions but then WRITE for Table permissions. If we change it so that hasTablePermission checks for namespace (and maybe even system) permissions, we'll just need to realize what that specific table permission might translate to for the higher level permissions. > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java, > > line 309 > > <https://reviews.apache.org/r/15166/diff/1/?file=376152#file376152line309> > > > > Locking on NamespaceIds for compactions, and many other FATE > > operations, could be a nasty bottleneck. Do we really need to lock on them?! > > Christopher Tubbs wrote: > I'm not sure. Probably not, but could there be problems with moving > tables between namespaces in some cases if not? Or, perhaps preventing that > would be a better way to go. > > John Vines wrote: > I'm sure some operations need to lock namespaces, but not as many as we > do now. Possibly a multi-read/write lock is needed Maybe not, but I just didn't want to introduce any concurrency issues. For most table operations, it does only grab a read-lock on the namespace (so other people just can't change the namespace while you're changing a table). > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > server/master/src/main/java/org/apache/accumulo/master/Master.java, line 953 > > <https://reviews.apache.org/r/15166/diff/1/?file=376147#file376147line953> > > > > I think we should have dedicated thrift returns for these. Or at least > > dedicated thrift error codes Ya, I wasn't sure how to handle that exactly. Looking back at the code, there is a TableNotFoundException that's a Thrift Exception, so we should probably just make a similar one for namespaces. > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java, > > line 431 > > <https://reviews.apache.org/r/15166/diff/1/?file=376130#file376130line431> > > > > This error could cause confusion when you have a table in the default > > namespace with the same name as a namespace (if that's possible?) > > Christopher Tubbs wrote: > I don't think so, not since the "why" string explicitly says "Could not > find table namespace while getting configuration." > > John Vines wrote: > Resorting to parsing strings to determine errors programmatically is > pretty crummy for end users. Also, looking through this code path shows that > TableNamespaceOperationsImpl#getProperties doesn't work right since it looks > for the type NOTFOUND, which this error isn't setting. Perhaps we should make > TableOperation a combination of Table and TableNamespace options in order to > support backwards compatibility while maintaining a solid set of easily > decipherable errors. Well, if you really want to be able to easily find it programmatically, it might be best to just make a new type of exception (like ThriftNamespaceOperationException or something) because all the namespace stuff is new and won't break any backwards compatibility. I was just trying to reuse some existing stuff, especially with the thrift calls because I was brand new to thrift. > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceExistsException.java, > > line 24 > > <https://reviews.apache.org/r/15166/diff/1/?file=376077#file376077line24> > > > > If we make TableNamespaceExistException extend AccumuloExtension we can > > get around a lot of api compatibility issues > > Christopher Tubbs wrote: > That doesn't follow the existing pattern with TableExistsException, and > the Exception does not appear to be thrown in any existing API. To what > API-compatibility issues are you referring? > > John Vines wrote: > There is an issue with TableNamespaceNotFoundException and I think all 3 > of these exceptions should be in the same family, i.e. implemented by similar > means. Like Christopher pointed out, I was just following what Tables did with their exceptions. If you think that should be different, then we might want to change the table exceptions too. - Sean ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15166/#review28029 ----------------------------------------------------------- On Nov. 1, 2013, 2:01 a.m., Christopher Tubbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15166/ > ----------------------------------------------------------- > > (Updated Nov. 1, 2013, 2:01 a.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-802 > https://issues.apache.org/jira/browse/ACCUMULO-802 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-802 Tablespaces (Table Namespaces), work done by Sean Hickey, > https://github.com/Wisellama/accumulo/tree/ACCUMULO-802, rebase'd onto latest > master (including 210 commits) > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/Constants.java > 9db0c405c5b9fbac13fa735c3ffd6433e9831051 > core/src/main/java/org/apache/accumulo/core/client/Connector.java > bbfa55f4b9ad8fc0e5f0c0058e2e0564685d7c85 > > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceExistsException.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceNotEmptyException.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceNotFoundException.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperations.java > 86a3ff271fc2e085c426da3156cfab9cdbb5c36b > > core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperationsImpl.java > 0f0e998f334631cfb342f9a39fdd12e62aa98f13 > > core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperations.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsHelper.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsImpl.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java > 1b652f99e23e2dfa06c7373a6f4c3c044f5cd1a3 > core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java > bd1156912849b13ebad8f6b974fd35d8adf18f1d > > core/src/main/java/org/apache/accumulo/core/client/impl/TableNamespaces.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java > 8bc725a3405a79c20c690bff3f6fdb6f713be523 > > core/src/main/java/org/apache/accumulo/core/client/impl/thrift/ClientService.java > 488e0654720250542145e0d543ad16813f8d8b6d > > core/src/main/java/org/apache/accumulo/core/client/impl/thrift/SecurityErrorCode.java > b706ce87bb1e8a525e585b14b7fb8563c3493c2c > core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java > 5ee144d9eb67c8e79dad870abbd823cac64e111e > core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java > 80ec5133334f9ef466dd7e4077230acfd2c77fb5 > > core/src/main/java/org/apache/accumulo/core/client/mock/MockSecurityOperations.java > 765cda9cac6fb3bc7df9eb8db1c83846960e85c3 > core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java > 3dcab11bd50bf189ccc4cbaf97d3d4ecb9133047 > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTableNamespace.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTableNamespaceOperations.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java > a3c2043b8120e13a190eb173d8342c205700382f > > core/src/main/java/org/apache/accumulo/core/client/security/SecurityErrorCode.java > fb51387fabea6d47854048daad4c8ec14047ab33 > > core/src/main/java/org/apache/accumulo/core/master/thrift/MasterClientService.java > 5b9949a1b6b7997d08de371c9a36c1e1a2b50b1f > core/src/main/java/org/apache/accumulo/core/security/SystemPermission.java > 699396e7e47b858ea8273c969f79dc7b3ebdc577 > > core/src/main/java/org/apache/accumulo/core/security/TableNamespacePermission.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java > 52e1d04998060954b7f09a3b9a6d9fdd898208b0 > core/src/main/java/org/apache/accumulo/core/util/shell/ShellCompletor.java > 85f68ef2464cb784861a587badd4835d842d8b87 > core/src/main/java/org/apache/accumulo/core/util/shell/ShellOptions.java > 31529730935ff78de6c39fea531e20e2ade30a2d > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/CloneNamespaceCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConfigCommand.java > 73d21ef6108282c9dd8dd8bc0f2ce77524acedcf > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConstraintCommand.java > 4280555dfce0856e2e1329412b0051b15ce5a748 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/CreateNamespaceCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/CreateTableCommand.java > 0ad787d67eaa5a90d26e113d7a79f0f3218ce0f4 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/DUCommand.java > 189458a504dce012f3c579ff3b8425f030ad1b54 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteIterCommand.java > d0644828af83f5c2d2ecbc58bd7545d958be31a0 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteNamespaceCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteTableCommand.java > 35c534d791648267539833b922643fc65dd600d4 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/GrantCommand.java > f3e720029714da63a7ec7e2bfb7587443ed7ed8b > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/ListIterCommand.java > e2d4d915c3311205ca4731b449769c37cd92ac82 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/NamespacePermissionsCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/NamespacesCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/OptUtil.java > f0c14d4cfd9ebf5a480ac63bb9dcd41e085b22f7 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/RenameNamespaceCommand.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/RenameTableCommand.java > 057f2b4374b3cc137a20158e70c33dd97c019c87 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/RevokeCommand.java > 676284aa9591d05a946d0bc4290ff88c3eb04eba > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/SetIterCommand.java > 3a7155687994049d2580f7b39e0df1a39d52c385 > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/TableOperation.java > 2bec526af38d328ea86cb21860471d8bed384eac > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/TablesCommand.java > 19b49e9cb4ca8483de55e644700d2a68d14ab42f > > core/src/main/java/org/apache/accumulo/core/util/shell/commands/UserPermissionsCommand.java > 79d78da3b0f6de47f57740ee13678792b61a87c7 > core/src/main/thrift/client.thrift 2f5e9d191a0d80d32e2a88a6e31845f3b93ae289 > core/src/main/thrift/master.thrift 19960c8857295d22ffba728c95501f069ba953c0 > > core/src/test/java/org/apache/accumulo/core/client/mock/MockTableNamespacesTest.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/ServerConstants.java > 9e0ac393deb962799e4e98753db1814dd630d14e > > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java > 3f1aaa29ef023d2f33fa5b35aad2e57df9b6aff4 > > server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfiguration.java > c9fd5a141a437aef6f1a9d8f8c80999f19af6ec8 > > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java > 4c581530b77a88e141837507a32cdc4f89982c56 > > server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfWatcher.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfiguration.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/conf/TableParentConfiguration.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > 0477a443e307e174965140f0fd14c898a0d98727 > > server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java > 3e0a2bf0e06227780adb92967d439212c0316924 > > server/base/src/main/java/org/apache/accumulo/server/security/AuditedSecurityOperation.java > ee337a5d23f04b321a306a306c41ea59e2f731a0 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java > f00159cc21d8a7b8a47efbbc3d1f2bd96f479a03 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/InsecurePermHandler.java > b57abfec483e3e07b85ed41148e11789fc45461c > > server/base/src/main/java/org/apache/accumulo/server/security/handler/PermissionHandler.java > 72c64b5897a4b0bf7de8081a76288fd8fb639021 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java > f2196035d93ce5718b09e9754d130015515da91f > > server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java > 3b9d8b24b85996dfbf734d4910aa6cd0d9247beb > > server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java > 8a74d0b936d8fad3b020a230b6b20536d740288f > > server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java > cdee8fbe448592120a5cc5b6682aedfafca7e7bf > server/master/src/main/java/org/apache/accumulo/master/Master.java > 8a1a9b272bf332258febf46b7535d12f83a1dbd0 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CancelCompactions.java > dd4c2292996d758860191fc054631812adfa34b2 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ChangeTableState.java > 697c15e173b13bc5909862998b8742708bc933c1 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneTable.java > 8d906d602ae1abdd2f4d774c64b3b4cd1174aa72 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneTableNamespace.java > PRE-CREATION > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java > 160fc7e89320250d24e89526188528ff4786cd6d > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateTable.java > df2e0285b10fbdacb9fcacd007b48c1e2891d5d1 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateTableNamespace.java > PRE-CREATION > > server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteTable.java > 427125721cfaa122378bfe74b6c89b96b2f93137 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteTableNamespace.java > PRE-CREATION > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ExportTable.java > 22df3b340bbb9e76ef1fabe16b521654eb7a6bcb > > server/master/src/main/java/org/apache/accumulo/master/tableOps/ImportTable.java > b91da522ff13b2be4de3fe82c47875e74cc1e569 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTable.java > 3069aaf9eea3044d14bf7bb7b81c0270a0997f08 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTableNamespace.java > PRE-CREATION > > server/master/src/main/java/org/apache/accumulo/master/tableOps/TableRangeOp.java > 0ad2196a30a959f5ca5c7bdaf6d46a8628fd957a > server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java > fa14f43100f5a13b16d2d34b95a8466501b93299 > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java > 0ef6c6b867b284e86f8252ebb19913b3c1831362 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/ChangePermissions.java > be2de2a547aecc72eca7e2c980964ef85551aae8 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CheckPermission.java > 5fa9bc49439974071fe12d97b371eb4e6ac3db3f > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CloneTable.java > 65d3aed4faf1f2a360aa0d4df123390206eae0f3 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CloneTableNamespace.java > PRE-CREATION > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Config.java > b47f1a946e904820a0cc83a4b3e078d40f11bb99 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CreateTable.java > 0aa7f7ad0882a3d82bd6ca423a3a8a3dac706fba > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CreateTableNamespace.java > PRE-CREATION > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/DeleteTableNamespace.java > PRE-CREATION > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Merge.java > 8fcfab593071458954939970892a88b8793088b3 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/OfflineTableNamespace.java > PRE-CREATION > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/RenameTable.java > d4a4e956699c2b2a483d6360e201da3eccd29847 > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/RenameTableNamespace.java > PRE-CREATION > > test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Setup.java > 2cf37cecb8223b31b58e1afa4fa08ebf9967a8e1 > > test/src/main/java/org/apache/accumulo/test/randomwalk/security/WalkingSecurity.java > 0ddb75254da156d75bbb0ca1f0a6e63c2443ab1c > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java > 31867f39dd4d9661a00557c3291258b5e2507b30 > test/src/test/java/org/apache/accumulo/test/TableNamespacesIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/functional/PermissionsIT.java > b8e1a4fcfcd1f02aa12926a44046493dbe74fe15 > test/system/randomwalk/conf/modules/Concurrent.xml > 03e5542d6b5b02f09e899301c68cf1d6746285f5 > > Diff: https://reviews.apache.org/r/15166/diff/ > > > Testing > ------- > > > Thanks, > > Christopher Tubbs > >
