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.

Sign up for GitHub

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
Merged

Enable External Durable Client Managed Identity Support #2856

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bcae802
Update StandardConnectionInfoProvider.cs
nytian Jun 18, 2024
aeff250
Update StandardConnectionInfoProvider.cs
nytian Jun 18, 2024
cbda2b6
Update StandardConnectionInfoProvider.cs
nytian Jun 18, 2024
9d4384f
Update StandardConnectionInfoProvider.cs
nytian Jun 19, 2024
e386417
add description about the resolve method implementation
nytian Jun 26, 2024
0c6ed16
remove whitespace
nytian Jun 26, 2024
064b5dd
update by comment
nytian Jun 26, 2024
0716f93
update by comment
nytian Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions src/WebJobs.Extensions.DurableTask/StandardConnectionInfoProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License. See LICENSE in the project root for license information.

using System;
using System.Linq;
using Microsoft.Extensions.Configuration;

namespace Microsoft.Azure.WebJobs.Extensions.DurableTask
Expand All @@ -23,10 +24,40 @@ public StandardConnectionInfoProvider(IConfiguration configuration)
this.configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
}

// This implementation is a clone of `IConfigurationSection.Exists` found here https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationExtensions.cs#L78
// Functions host v1 (.net462 framework) doesn't support this method so we implement a substitute one here.
private bool IfExists(IConfigurationSection section)
{
if (section == null)
{
return false;
}

if (section.Value == null)
{
return section.GetChildren().Any();
}

return true;
}

/// <inheritdoc />
public IConfigurationSection Resolve(string name)
{
return this.configuration.GetSection(name);
// This implementation is a replication of the WEbJobsConnectionInfo Provider used for the internal durable client.
// The original code can be found at:
// https://github.com/Azure/azure-functions-durable-extension/blob/dev/src/WebJobs.Extensions.DurableTask/WebJobsConnectionInfoProvider.cs#L37.
// We need to first check the configuration section with the AzureWebJobs prefix, as this is the default name within the Functions app.
bachuv marked this conversation as resolved.
Show resolved Hide resolved
string prefixedConnectionStringName = "AzureWebJobs" + name;
IConfigurationSection section = this.configuration?.GetSection(prefixedConnectionStringName);
Comment on lines +51 to +52
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
Collaborator Author

@nytian nytian Jun 19, 2024 •
edited
Loading

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
Collaborator Author

@nytian nytian Jun 19, 2024 •
edited
Loading

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
Collaborator 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 :)


if (!this.IfExists(section))
{
// If the section doesn't exist, then look for the configuration section without the prefix, since there is no prefix outside the WebJobs app.
section = this.configuration?.GetSection(name);
}

return section;
}
}
}
}
Loading