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

Explicit taskHub parameter for stored procedures #162

Open
michaelplavnik opened this issue Apr 5, 2023 · 4 comments
Open

Explicit taskHub parameter for stored procedures #162

michaelplavnik opened this issue Apr 5, 2023 · 4 comments
Labels
db-schema This issue is related to or will impact the database schema, likely requiring a DB schema update. enhancement New feature or request help wanted Extra attention is needed

Comments

@michaelplavnik
Copy link

Please consider changing approach from using implicit dt.CurrentTaskHub() to explicit parameter in all procedures.

Rational. Flexibility in support database sharing/isolation approaches and simplicity.

For example, from documentation: "This is often valuable when your organization has many small apps but prefers to manage only a single backend database. When multitenancy is enabled, different apps connect to a shared database using different database login credentials and each app will only have access to its own data." Explicit assumption of different login is not flexible, and might be not reflecting reality for majority of financial organizations that actually want to share the database schema.

@cgillum
Copy link
Member

cgillum commented Apr 5, 2023

@michaelplavnik can you share more about the setup that you want to enable? There are several ways to share a database and I want to understand if there is a mechanism to enable your scenario that doesn't require explicit task hub parameters.

I anticipate it will be difficult for us to continue support the higher security model if we add explicit task hub parameters.

@cgillum cgillum added db-schema This issue is related to or will impact the database schema, likely requiring a DB schema update. Needs: Author Feedback Awaiting feedback from the issue author. and removed Needs: Triage 🔍 labels Apr 5, 2023
@michaelplavnik
Copy link
Author

The following setup: hub process is running under AD account, SQL server trusted authentication, shared connection pool between task hub and domain specific logic. Multiple physical hosts, each running single hub process. Hosts are partitioned based on task hub name.

In regards to your concern. It seems to revolve around @taskHubMode = 1. This can still be enforced even with explicit task hub name (e.g. instead of deducing task hub name, assert USER_NAME() = taskHubName). Other mode then becomes application level responsibility mode.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 Issue needs attention from maintainers and removed Needs: Author Feedback Awaiting feedback from the issue author. labels Apr 5, 2023
@cgillum
Copy link
Member

cgillum commented Apr 5, 2023

I see. I think that strategy might work and that the feature could be generally useful - thank you.

While useful, it may be difficult for our team to prioritize this feature at this time without input from more users. That said, many of the features in this project are community contributions, and we'd be happy to accept a PR for this enhancement as well is anyone wants to contribute it (in which case we can publish it immediately).

@cgillum cgillum added enhancement New feature or request help wanted Extra attention is needed and removed Needs: Triage 🔍 Needs: Attention 👋 Issue needs attention from maintainers labels Apr 5, 2023
@vigouredelaruse
Copy link

vigouredelaruse commented Apr 6, 2023

i support this line of discussion because operating durable functions with the sql provider drives one into conflict with unforable combinations of at minimum

  • 1:1 map of sql-login-name<=> task hub separation. in a dev environment with frequent code changes this is a confounding factor for retiring in-flight orchestrations that encourages bloat of 'abandoned' schemas and databases with inflight orchestrations
  • tight coupling of schema names and kubernetes func deploy configuration of scalar metric as per the config below. this is a cross-cutting concern that depends on the keda autoscaler configuration
  - metadata:
      connectionStringFromEnv: FunctionStorage_SQLSERVER
      query: SELECT dt.GetScaleRecommendation(10, 1)
      targetValue: '1'
    type: mssql

to expand on scale recommendations for a minute the issue seems be solvable by parameterizing the query clause in the above exhibit to abstract away the schema name placeholder as per
image

but that's not a simple matter of search and replace here https://github.com/microsoft/durabletask-mssql/blob/main/src/DurableTask.SqlServer/Scripts/logic.sql

instead the achievable use case i seem to be reaching for is an extension of earlier points in this msg with at minimum

  • environment independent platform support for retiring inflight orchestrations

i believe the rationale posited by the original poster is in line with what i've said here. apologies if not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
db-schema This issue is related to or will impact the database schema, likely requiring a DB schema update. enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants