microsoft / durabletask-mssql

Microsoft SQL storage provider for Durable Functions and the Durable Task Framework
MIT License
87 stars 32 forks source link

Feature Request - Custom Schema Name #105

Closed moldovangeorge closed 2 years ago

moldovangeorge commented 2 years ago

Currently, the multi-tenancy feature of this provider allows multiple services that share the same DB to use the same DT schema via TaskHub work isolation. In some scenarios, using the same schema across multiple services that are independently deployed will not work in the long term due to security and reliability concerns. We want to contribute to the provider by adding custom schema name support in a backward-compatible way but want to agree that this will be accepted by the DTF owners before starting any work on this. For us, isolating each micro-service in their own DB in order to isolate the DTF work is not an option, and this contribution would highly improve our current DTF setup.

Tagging @cgillum as a DTFx owner.

cgillum commented 2 years ago

This is a feature I considered but avoided due to potential complexity and wanting to get community feedback first. Would you be willing to submit a design proposal for this?

If we can make the problem statement clear and come up with a clean solution that doesn’t make the provider harder to use, then we can absolutely accept it as a contribution.

moldovangeorge commented 2 years ago

Great, will come up with a proposal next week and will start from there. đź‘Ť

moldovangeorge commented 2 years ago

I took a look at the code, and it doesn't look like a design change is needed, but more like a very detailed mechanical refactoring. The "dt" hardcoding appears only in the following places :

The plan I propose is :

  1. Add a new property in the SqlOrchestrationServiceSettings class called SchemaName. -> this will default to "dt" to not disrupt existing functionality
  2. In the Sql Scripts from here, Infer the schema name from a variable, and set that variable to the SchemaName from the app tier before executing any script (what is the cleanest way of setting that variable is to be established at implementation time. e.g via some placeHolder that is overwritten by the app-tier prior to execution, or through some environment variable that is set at connection time).
  3. Modify the SqlOrchestrationService class such that : a. It uses the SchemaName from SqlOrchestrationServiceSettings instead of the hardcoded "dt" for all DB related operations (calling stored procedures, etc). b. When using xxxParameter methods from the SqlTypes classes, it will also pass the schemaName, enabling us to remove the dt hardcoding from those files
  4. Modify the SqlDbManager such that : a. It uses the SchemaName from SqlOrchestrationServiceSettings instead of the hardcoded "dt" b.Prior to executing any SQL Script that sets the schemaName var in the script (this is dependent on how we are actually going to manage that script variable)

Everything else should work the same. After this change is made, accessing the schemaName through the SqlOrchestrationServiceSettings should become a pattern and "dt" hardcoding should never be used. (edited)

moldovangeorge commented 2 years ago

Hei @cgillum, @AndreiRR24 worked on implementing my proposal and has everything ready on a feature branch to submit a PR. Can you give him write access to the repository so he can push the feature branch and open a PR? We can start discussing the approach and the implementation once the PR is open.

cgillum commented 2 years ago

I assume the feature branch is on a fork of this repo? If so, are you not able to open the PR from the fork? This is the normal process for submitting PRs and shouldn’t require write access.

mercer commented 2 years ago

Yes, I can confirm the flow with the fork works https://github.com/microsoft/durabletask-mssql/pull/109

cgillum commented 2 years ago

Closing as completed. Thank you so much for this valuable contribution! Note that I made some minor changes, which you can find here: https://github.com/microsoft/durabletask-mssql/pull/122. This new feature will be included in the v1.1.0 release and will be available for both DTFx users and Azure Functions users.