Token store deleted parent #4193
Conversation
vault/token_store.go
Outdated
| lock.Unlock() | ||
| continue | ||
| } | ||
| // Otherwise, if the entry doesn't exist, or if the parent doesn't exit go |
vault/token_store.go
Outdated
| continue | ||
| } | ||
| // Otherwise, if the entry doesn't exist, or if the parent doesn't exit go | ||
| // on with the delete onthe secondary index |
vault/token_store.go
Outdated
| } | ||
| } | ||
| // Add current children deleted count to the total count | ||
| deletedCountParentList += deletedChildrenCount | ||
| // If we deleted all the children, then add that to our deleted parent entries count |
There was a problem hiding this comment.
Should the parent prefix be cleaned out here if exists == nil? Since ClearView above operates at revocation time but in this situation we won't be revoking the parent.
There was a problem hiding this comment.
Consul will remove the prefix entry if the "directory" is empty so I didn't have an explicit delete in here, but I guess we can't guarantee that this is the behavior for other storage backends so I might be good to explicitly call a delete (though I'd do it inside if originalChildrenCount == deletedChildrenCount {...} which ensures that we delete the prefix iff all child/secondary indexes are removed).
vault/token_store.go
Outdated
| // N.B.: This is a no-op for the consul storage backend since the delete doesn't support | ||
| // recursive deletes, and delete on a path is a no-op. | ||
| if exists == nil { | ||
| err := ts.view.Delete(ctx, parentPrefix+parent) |
There was a problem hiding this comment.
@jefferai @vishalnayak I added the explicit delete in here to ensure that the parent prefix gets deleted if there are no children as per previous discussion in here.
In the case for Consul, it should automatically remove this (and even if it didn't, this would result in a no-op since the underlying ConsulBackend.Delete would have to call DeleteTree to handle tree deletions and deletes on a non-existing key is simply treated as a success). However, other storage backend implementations might error on this since the delete method is usually done against an entry, and not a path/tree (thinking that this might be an issue for Cassandra's delete, for instance). This would result in the tidy operation being treated as a failure, so I wonder if we should a) not do this at all or b) treat this as warning instead of an error.
There was a problem hiding this comment.
@calvn Apologies -- I think this needs to come back out. You're right that we don't know what storage implementations will do, but I believe the expected behavior is that they clean up their own empty prefixes. I remember making this change with file for instance, so if a storage backend isn't doing this then it's for the backend to fix. It was right the first way :-/
There was a problem hiding this comment.
No worries, I'll revert! Glad this discussion happened; I learn something new every day :)
vault/token_store.go
Outdated
| @@ -1169,6 +1169,33 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedId string) (ret er | |||
| } | |||
| } | |||
|
|
|||
| // Mark all children token as orphan by removing | |||
| // their parent index, and clear the parent entry | |||
There was a problem hiding this comment.
Maybe add to this comment explaining that marking orphan is correct since revokeTree will ensure that any are deleted anyways if it is not an explicit call to orphan the child tokens.
| if err != nil { | ||
| return fmt.Errorf("failed to get child token: %v", err) | ||
| } | ||
| lock := locksutil.LockForKey(ts.tokenLocks, entry.ID) |
There was a problem hiding this comment.
If this lock happens to be the same lock that is being used outside of this loop, acquiring this will deadlock. No?
Addresses the following: