sourcegraph: multi-tenant Zoekt #859
Conversation
| // Skip documents that don't belong to the tenant | ||
| if !tenant.EqualsID(ctx, repoMetadata.TenantID) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Having the check at this level means shard merging should work out of the box. We can move it higher if we see it's a bottleneck.
There was a problem hiding this comment.
great, this is how I imagined how this would work originally. At the core loop we check, but then add some other logic in higher up places for efficiency reasons.
There was a problem hiding this comment.
Copying in my comment from the previous PR (#858). What's your new thinking on this?
For other tenant-aware services, we'd tried to design it so there's a "choke point" that clearly enforces tenancy. Often this is single interface and file guarded by //🚨 SECURITY: comments. Right now, we are only enforcing tenancy on Search, but not on List. And there are other places where we access index data, for example to get metadata or ngram stats.
I wonder if we could push this down further to where index data is loaded/ read. Like reader.readTOC and reader.readIndexData? That would help establish a single place on the read path where tenancy is enforced, as close to the data access as possible.
| } | ||
| t, err := tenanttype.FromContext(ctx) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Not returning an error because we pprof log in FromContext already. Nice side-effect: No error here means we can avoid running watchdog with a tenant and we don't need http middleware either.
There was a problem hiding this comment.
agreed, this logic makes sense for zoekt. nice.
| // Skip documents that don't belong to the tenant | ||
| if !tenant.EqualsID(ctx, repoMetadata.TenantID) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
great, this is how I imagined how this would work originally. At the core loop we check, but then add some other logic in higher up places for efficiency reasons.
| } | ||
| t, err := tenanttype.FromContext(ctx) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
agreed, this logic makes sense for zoekt. nice.
- prefix flag instead of bool - padded ids - include tenant in id - add tenant tests
|
Fuzz test failed on commit b93cb3c. To troubleshoot locally, use the GitHub CLI to download the seed corpus with |
stefanhengl
changed the title
This updates webserver and sourcegraph-indexserver to support multi-tenancy.
The change is behind an ENV feature-flag.
Key changes:
TenantIdAssumptions:
Test plan:
This is how the new Sourcegraph naming scheme looks like on disk:
