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.
There was a problem hiding this comment.
Its good to reserve the source namespace because it prevents it from being deleted or renamed during this operation.
There was a problem hiding this comment.
The source namespace can't be deleted because the source table can't be deleted. I don't think we care about the source namespace being renamed, so long as we only look up the ID once. If a rename happens, we shouldn't care.
There was a problem hiding this comment.
Looked into this a bit more and opened #5389. If #5389 were fixed then it would be good if this code got a read lock on the source namespace. Most other code will get the read lock on a namespace and then lock the table, it would be good to follow that pattern here w/ the soruce namespace planning that #5389 will be fixed.
There was a problem hiding this comment.
Would also be good to eventually get the lock on the dest namespace like the code used to do.
There was a problem hiding this comment.
The source namespace can't be deleted because the source table can't be deleted. I don't think we care about the source namespace being renamed, so long as we only look up the ID once. If a rename happens, we shouldn't care.
Just saw this comment, did not see when I posted the comment about #5389. The model I assumed all fate table operations were following was :
- All table operations get a read lock on the namespace they are operating on
- All destructive namespace operations get a write lock on the namespace.
Destructive namespace ops do not lock each table in the namespace, but the assumption that every table op gets a namespace read lock makes this unnecessary. If per table ops do not lock the namespace and namepsace ops do not lock the tables then nothing prevents them from running concurrently.
There was a problem hiding this comment.
We could try to optimize the locking via transitive assumptions I suppose. Like clone table can assume delete table will get a read lock on the namesapce and a write lock on the table and therefore it does not need to get the namespace lock only the table read lock. None of the fate operations work like this currently. They just get the read lock on the namespace and then the table lock. To me its much easier to reason about if all table ops follow the same protocol.
There was a problem hiding this comment.
If we do want to look into optimizing namespace locking that seems fine. I would advise doing that in this PR though. Making clone table, that is not called frequently, do namespace locking differently than all other fate operations seems confusing. If there is a performance benefit in optimizing, it would be good to do all table operations in the same PR and modify frequently called operations like bulk import and compact to get this benefit.
| @@ -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.
There was a problem hiding this comment.
Seems like that will handle the case I was concerned about, but I was also just generally concerned about any slight change to CloneInfo that might break deserialization of the older version.
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java
Outdated
Show resolved
Hide resolved
test/src/main/java/org/apache/accumulo/test/functional/CloneTestIT.java
Outdated
Show resolved
Hide resolved
| 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.
There was a problem hiding this comment.
I copied the test to 2.1 myself and it failed on the assertThrows... (the clone succeeded with the wrong permissions). I'm not sure why it would succeed for you, but I've tried several times, and am certain that I'm on the 2.1 branch without your fixes, and the test fails.
There was a problem hiding this comment.
I also confirmed it passes in your branch after the fixes.
Closes #5324