Fix src and dest namespace handling in clone table operation #5334
Conversation
There was a problem hiding this comment.
Aside from the comment I made about the exception handling and related comments, this change looks good... but may not go far enough.
A good test of one of the problems with this code that this PR should fix, is to try to clone a table from a namespace, n1, where the user has no permissions, into a table in namespace, n2, where they have create table permission and table read permission. Such a test should fail prior to these fixes, and should pass after these fixes.
In addition to these changes (after my comments are addressed), I think more needs to be done in this PR to fully address the underlying issues. Specifically, the fate operation CloneTable currently reserves the srcNamespaceId, but I think it needs to reserve the destination namespaceId instead. I'm not sure it needs to reserve the source namespace at all. In one of the subsequent fate steps, it also recomputes the destination namespaceId, and then clobbers it, and then tries to reserve the destination namespace if it's different than the source namespace. It doesn't need to recompute the destination namespace if that was already done, and it may not need to make a second reservation, either.
| } catch (TableNotFoundException e) { | ||
| // shouldn't happen, but possible once cloning between namespaces is supported | ||
| throw new ThriftTableOperationException(srcTableId.canonical(), null, tableOp, | ||
| TableOperationExceptionType.NOTFOUND, ""); |
There was a problem hiding this comment.
This shouldn't be added to the existing try block, because it's specific to the new line to look up the source namespaceId. It's not applicable to the retrieval of the namespaceId for the destination table name.
Also, the comment in here isn't applicable to this exception. The reason this shouldn't happen is because the source tableId should exist, and it did at some point prior to this call, when we looked up the tableId from the name that was passed in to the client request to clone. So, the proper comment should be something like // could happen if the table was deleted while processing this request
(Also, not related to this PR, but the comment about the NamespaceNotFoundException saying "shouldn't happen" doesn't make sense because cloning between namespaces is already supported.)
It also occurs to me that we need to make sure that you can't clone into the built-in accumulo namespace. There might be a check for that elsewhere... I'm not sure. But this should be tested.
This test fails because the code calls flush on the src table in namespace n1 where the user has no permissions. EDIT: I have the test working now. I misunderstood one of your statements. |
The |
There was a problem hiding this comment.
Looks good overall. I'm not 100% certain about whether the source namespace must also be reserved (I think no, but not sure), and I made some suggestions to add to the test coverage. The fact that there's a flush involved, and that some permissions required on the source table for that, mitigate the security risks of this a bit, but it's still a bit concerning that one might be able to clone with only alter-table or write permission on the source and without read permission on the source.
| long val = Utils.reserveNamespace(environment, cloneInfo.getNamespaceId(), tid, false, true, | ||
| TableOperation.CLONE); | ||
| val += Utils.reserveTable(environment, cloneInfo.srcTableId, tid, false, true, | ||
| val += Utils.reserveTable(environment, cloneInfo.getSrcTableId(), tid, false, true, |
There was a problem hiding this comment.
I think this is right, but I don't know fate well enough. I'd like to hear from @keith-turner about whether this is sufficient for the clone operation:
- reserve destination namespace (READ lock)
- reserve source table (READ lock)
I don't think we need to reserve the source namespace as well (I think reserving the source table is sufficient), but I'm not 100% certain about that.
| @@ -37,20 +35,12 @@ class CloneZookeeper extends ManagerRepo { | |||
| public CloneZookeeper(CloneInfo cloneInfo, ClientContext context) | |||
| throws NamespaceNotFoundException { | |||
| this.cloneInfo = cloneInfo; | |||
| this.cloneInfo.namespaceId = Namespaces.getNamespaceId(context, | |||
There was a problem hiding this comment.
This is going to change the serialization, and could cause problems with a 2.1.4 upgrade if there are any ongoing clone operations. That should be noted in the release notes for 2.1.4.
There was a problem hiding this comment.
How so? namespaceId is resolved in FateServiceHandler, passed to CloneTable and set in CloneInfo there.
There was a problem hiding this comment.
Oh, are you saying that this may cause pre-2.1.4 Clone fate operations to fail, and that I should still check and set this if it's null in CloneInfo?
There was a problem hiding this comment.
I'm not sure exactly what to expect when the serialization of the fate operation's classes are changed, but if a clonetable operation was serialized with 2.1.3, and then interrupted, and then resumed after upgrading to 2.1.4, the deserialization in order to resume the operation will probably fail in some way. I'm not sure if you need to do anything to check it in the code... maybe just warn users that they should make sure there's no outstanding clone operations before doing an upgrade.
There was a problem hiding this comment.
I made a change in 3cfb06c to handle the case of pre-2.1.4 fate transactions with a note to remove that handling in the 3.1 merge.
| expectedCurrStates); | ||
| } | ||
| Utils.unreserveNamespace(environment, cloneInfo.getNamespaceId(), tid, false); | ||
| Utils.unreserveTable(environment, cloneInfo.getSrcTableId(), tid, false); | ||
| Utils.unreserveTable(environment, cloneInfo.getTableId(), tid, true); |
There was a problem hiding this comment.
It probably doesn't matter, but I'd suggest releasing this write lock first. It's the most restrictive, and the most recently reserved, so it'd be good to roll back first.
There was a problem hiding this comment.
Move the table write lock release to the front in 3cfb06c.
| client.tableOperations().create(tableName); | ||
| client.namespaceOperations().create("new"); | ||
| client.tableOperations().clone(tableName, "new." + tableName, CloneConfiguration.empty()); |
There was a problem hiding this comment.
This tests cloning from the default namespace to a user-created namespace. I think this test can be expanded by adding:
- test additional clone succeeds cloning into a second user-defined namespace, i.e. from new.tableName to new2.tableName
- insert a few entries and ensure successfully cloned table(s) contains same data
- test clone fails when target namespace contains a table of the same name already
- test clone fails when cloning to self (same namespace, same table name)
There was a problem hiding this comment.
Modified CloneTestIT with these changes in 3cfb06c.
| client.securityOperations().grantTablePermission(newUserName, srcTableName, | ||
| TablePermission.READ); |
There was a problem hiding this comment.
This is the expected success case. However, prior to this fix, I believe if this was set on the destination namespace, but NOT on the source table, then the operation would still succeed. The test should check that case, to make sure that it succeeds before this fix (to verify the bug), and fails after this fix.
There was a problem hiding this comment.
Added suggested test in 3cfb06c. I will see if it succeeds in the 2.1 branch.
There was a problem hiding this comment.
I copied this test to the 2.1 branch and is succeeds as is with no modifications. Based on what you are saying above, this test should have failed because the clone operation should have succeeded instead of throwing an exception.
Closes #5324