Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support dynamic index definition for multi-tenancy (Prefix, IndexName) #242

Open
houstonhaynes opened this issue Oct 21, 2022 · 8 comments · May be fixed by #245
Open

Support dynamic index definition for multi-tenancy (Prefix, IndexName) #242

houstonhaynes opened this issue Oct 21, 2022 · 8 comments · May be fixed by #245
Labels
enhancement New feature or request

Comments

@houstonhaynes
Copy link

This issue relates to issue 98 so I will try to keep this tight.

We are looking to manage multi-tenancy with discrete indices (or indexes) where there is a customer (company) specific index as well as an "admin" index that allows admins to see all records. So in our case we'll always have N+1 number of indices - one for each customer and the +1 over-arching index that admins can use to see all data.

ACLs are used to "tune" an individual Redis "user" (in our case a customer/company) to only see the records that have a key with their ID present - and similarly with subscription channels.

The Challenge with OM as it stands now is that index definition is held within the decorator of the type which defines the index. That decorator requires constants, so they are "baked" into the program at compile-time. In a domain where only one index is required this is OK, but in a configuration where multiple tenants are managed with discrete indices and ACLs this is a blocker.

In discussion with my working group a "code gen" approach was suggested - BUT - we arrived at a stopping place with that when we considered the business case of adding a customer (and its respective users) to the identity system where a new ACL and customer-specific index would be required while the application was in-flight. To code-gen multiple types from a base at compile time means that you can only generate types for the companies you can know at compile time. So to add a new customer index would ostensibly require re-compilation.

From this I'd assert that the type decorator is not the right location for setting index properties such as "PREFIX" and "IndexName". They should be eligible to be assigned at run-time and not limited to a compile-time construct.

@VagyokC4
Copy link

Hi @houstonhaynes,

We tackled something like this with a simple array property called Owners[] directly on the record. Then we pushed into this array all owners (ids) who have access to the record. Then we query based on ids that come from the security context to ensure you only get back records that you are allowed to see. x=>x.Owners.Contains("some_id")

This allowed us to have a single index / property to control who gets access to what etc. We put an admin id along with tenant id, along with customer id etc. and providing any one of the Owner ids gets back the record.

Maybe something like this works for your use case?

@houstonhaynes
Copy link
Author

houstonhaynes commented Oct 22, 2022

Thanks for this! We considered a similar approach and after experimenting a bit we set it aside as an option on three grounds, 1) security 2) client code management burden and 3) Redis burden.

The latter considerations (to me) translate to: 1) you're depending on an ersatz "WHERE" clause - putting the burden of segmenting records on the app layer to get right every time, and 2) you're asking Redis to reason over the entire corpus for every single query. Both of those are strong negatives for us - assuming I have that assessment right.

My understanding is that using indices to partition data and ACLs for access both "shortens" lookups and gets the app/client out of the business of determining record visibility logic.

I think there are some scenarios where that approach would be the right value-to-burden ratio. Our current approach slides in between your suggestion of query segmentation and the prospect of setting up a Redis instance per company, which in our minds would then shift a large adverse burden onto the admin function - as well as create a range of NOC/SRE activities every time a new customer is on-boarded.

And FWIW our chosen approach works with straight hashsets - and we considered setting OM aside completely as a consequence. It would be a shame but with an application where security posture is primary we're not in a position to pick what's easiest for the tool/engineer to contemplate.

@VagyokC4
Copy link

VagyokC4 commented Oct 24, 2022

Hi @houstonhaynes , I appreciate the detailed response and the point of view. For all the reasons mentioned, I think it would be good to see a solution worked out with Redis-OM, if for no other reason than to meet this specific "security-centric" use case going forward.

What specifically intrigues me is the data partitioning aspect, if I'm understanding correctly, would be the ability to store the data once, and have different "views" of the data via the various indexes operating on the underlying data? This would mean Redis-OM would support multiple indexes per collection along with the ability to search using a specific index. I am wanting to do something similar (#244).

I think once the ability to support multiple indexes exists, then the home stretch for you would be to add the multiple dynamic attributes at runtime via dynamically-adding-attributes, or something to that effect.

@slorello89
Copy link
Member

Thanks for opening this @houstonhaynes and I apologize to the author of #98 for being a bit distracted from this issue. It seems like the main ask here is for a way to customize the exact key-prefix that's generated by redis OM so that you can properly namespace the records that you're putting into redis. It strikes me that the best way to get there from the perspective of the public API is to add 2 things.

  1. Add a prefix argument for the initializers for the RedisCollection (the constructor list and the the RedisConnectionProvider.RedisCollection method)
  2. Add the ability to create/delete indexes directly from the RedisCollection

This would mean that the inserts/queries/index creation would all respect the prefixes from the RedisCollection, which makes sense to me.

Going to be working on this on stream at 2PM EDT if anyone wants to stop by and give their thoughts. Should be a pretty large effort I'd think so probably won't all get done today.

@houstonhaynes
Copy link
Author

This is great - sorry I can't make it - meeting heavy day.

I'm not precisely sure if what you're suggesting covers the bases - as I'm expecting both the name and the prefix to be settable - as the context for those two are what's most coupled. There's also the secondary issue of "first do no harm" for those garden-variety cases where setting the values in the type decorator is totally OK. Just mulling that over gives me a bit of a headache so I can imagine that might be what you mean by it not being a one-day effort.

@slorello89 slorello89 linked a pull request Oct 26, 2022 that will close this issue
@slorello89 slorello89 added the enhancement New feature or request label Oct 27, 2022
@mohammed-ehsan
Copy link

Hi Everyone,
I am the issuer of #98.

An interesting take here from @houstonhaynes, to me both of our problems are an extension to this issue #196 to some degree.

Adding support for Fluent API, something that is similar to EF Core fluent API, would provide a much greater flexibility. It would eliminate the need to implement all of these somewhat ad hoc functionality to support specific use cases, what do you think? I know it would take a tremendous effort to implement, yet I'm still willing to collaborate on my free time.

@houstonhaynes
Copy link
Author

I saw the work here and like it - just not understanding the failure - perhaps pointing back to the need for a default behavior using type decoration?

@HSKC
Copy link

HSKC commented Mar 28, 2023

We are looking for a solution to parallel CI Tests on a Redis instance. This would solve our Problem. We could generate a prefix for every Test instance, create an index with that prefix, run the tests on it and delete it afterwards. The tests would not interfere with each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants