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

Enable External Durable Client Managed Identity Support #2856

Merged
merged 8 commits into from
Jun 27, 2024

Conversation

nytian
Copy link
Contributor

@nytian nytian commented Jun 18, 2024

As titled.

When external durable client is used, it will use the StandardConnectionInfoProvider instead of WebJobsConnectionInfoProvider. This PR updates the Resolve() at StandardConnectionInfoProvider so that it can read the configuration required for identity based authentication.

Also, when using an external durable client within a Functions app (but not within a Durable Functions app), AzureWebJobsStorage configuration is required by Functions Host. To achieve this, the resolution process includes logic to first check the connection name with the AzureWebJobs prefix and then verify the standard connection name.

End-to-end test have been made with asp net core app and functions app at samples dir.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

@nytian nytian requested a review from bachuv June 18, 2024 23:03
@nytian nytian requested a review from davidmrdavid June 18, 2024 23:31
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions

Comment on lines +47 to +48
string prefixedConnectionStringName = "AzureWebJobs" + name;
IConfigurationSection section = this.configuration?.GetSection(prefixedConnectionStringName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about this. Some questions:
(1) Are there other connection strings with prefix "AzureWebJobs" that aren't "AzureWebJobsStorage"?
(2) Why is it that we only apply this logic to connections with the "AzureWebJobs" prefix?
(3) How do we know the "name" parameter doesn't already include the "AzureWebJobs" prefix :) ?

Thanks!

Copy link
Contributor Author

@nytian nytian Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidmrdavid That's a good question, it's a little confusing here. This is a special case that is quite hard to explain and let me know if anything is unclear to you.

This is a scenario that specially for using a durable client in Functions App. When the Functions app started, the Functions Host will look for AzureWebJobsStorage (side note: this is the default value, the connection name could be others, eg: MyCustomeStorage, but functions host will alwasy look for one with AzureWebJobs prefix, in this case it will be AzureWebJobsMyCustomeStorage) especially . If there is no such a value, the host will fail. So we have to check the connection name with AzureWebJobs first so that the host can start successfully. Once the Host successfully started, then the method creating the external durable client will be called. In here, it will only look for connection name as xxx especially with no prefix.


_client = clientFactory.CreateClient(new DurableClientOptions
{
    ConnectionName = "xxx",
});

So to answer the second question, this is the only scenario that we need a prefix check, only at Functions the configuration always starts with AzureWebJobs so we don't need to check other prefix.

For (3), if it already includes, then the prefix name will be like AzureWebJobsAzureWebJobsStorage. And thus the section it look for will be null ( checking bythe method IfExists() ), and thus the Resolve will look for the name itself with no prefix.

Copy link
Contributor Author

@nytian nytian Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our WebJobsConnectionInfoProvider which is for the internal durable client also use this strategy. You can check it here . And it said that this logic is basically from the function host which is here .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment including the information in this thread? I'm mostly looking for a comment that explains why we added the code in the Resolve method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added the comments inside Resolve method. Let me know if it isn't clear to you :)

@nytian nytian requested a review from davidmrdavid June 25, 2024 21:31
Comment on lines +47 to +48
string prefixedConnectionStringName = "AzureWebJobs" + name;
IConfigurationSection section = this.configuration?.GetSection(prefixedConnectionStringName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment including the information in this thread? I'm mostly looking for a comment that explains why we added the code in the Resolve method.

@nytian nytian requested a review from bachuv June 26, 2024 22:57
Copy link
Collaborator

@bachuv bachuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's make sure that the tests pass before merging.

@nytian nytian merged commit 829777a into dev Jun 27, 2024
22 of 23 checks passed
@nytian nytian deleted the nytian/external-client-support branch June 27, 2024 02:27
bachuv pushed a commit that referenced this pull request Jul 9, 2024
* Update StandardConnectionInfoProvider.cs

* Update StandardConnectionInfoProvider.cs

* Update StandardConnectionInfoProvider.cs

* Update StandardConnectionInfoProvider.cs

* add description about the resolve method implementation

* remove whitespace

* update by comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants