tenant: introduce systemtenant #863
Conversation
There was a problem hiding this comment.
I added this to try make us a bit more robust in selectRepoSet #864
I think you should rename EqualsID to communicate at the call sites that it is about allowing the reading of tenant id's data.
I tried my hardest to think of another way we can get at this repository data without introducing a way of bypassing tenant checks. I failed and think this is the most robust way we have.
Some tasks, such as loading shards, require priviledged access on startup. Here I introduce a systemtenant we can use for these things. This is motivated by bug where the symbol sidebar in multi-tenant node wouldn't work because ranked shards were not loaded correctly which in turn caused `SelectRepoSet` to return 0 shards always. Test plan: - added unit test - manual testing: symbol sidebar works now
stefanhengl
force-pushed
the
sh/mt/systemtenant
branch
from
6bbab33
to
934161c
Compare
|
Another idea to avoid misuse is maybe not exporting the ctx, which can accidentally leak around, but maybe only add one function to servicetenant for now which looks like this |
I like that one EDIT: Spoke too early. I am running into circular imports again. I will go with UnsafeCtx for now. |
| @@ -1064,7 +1065,10 @@ func (s *shardedSearcher) getLoaded() loaded { | |||
|
|
|||
| func mkRankedShard(s zoekt.Searcher) *rankedShard { | |||
| q := query.Const{Value: true} | |||
| result, err := s.List(context.Background(), &q, nil) | |||
| // We need to use UnsafeCtx here, otherwise we cannot return a proper | |||
There was a problem hiding this comment.
Would there be a way to bypass List and load the repo metadata directly? So this just obviously becomes a low-level, privileged operation. This would introducing systemtenant just for this single privileged call. Or maybe it's not possible to refactor things to support this 🤔
I'm not an expert on this code and totally lack context, so just ignore me if this not helpful :)
There was a problem hiding this comment.
My initial thoughts was around this, but I couldn't think of a nice way. EG we want to take into account tombstones/etc and anything else we may add to List in the future. So using List feels like the cleanest approach.
Some tasks, such as loading shards, require priviledged access on startup. Here I introduce
systemtenantwhich we can use for these kinds of things.This is motivated by a bug where the symbol sidebar in multi-tenant node wouldn't work because ranked shards were not loaded correctly which in turn caused
SelectRepoSetto return 0 shards always.Test plan: