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

[FEATURE REQ] Add IConfiguration integration for TokenProvider when using AddAzureAppConfiguration() #43434

Closed
julealgon opened this issue Apr 15, 2024 · 6 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Extensions ASP.NET Core extensions issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@julealgon
Copy link

Library name

Microsoft.Extensions.Azure

Please describe the feature.

I'd like to propose a way to leverage the AzureComponentFactory in contexts where we don't yet have a IServiceProvider in place but we do have an IConfiguration: more specifically, when configuring Azure AppConfiguration.

AzureComponentFactory is an abstract class today, and it's only implementation, called AzureComponentFactoryImpl, is internal and thus not accessible from the outside.

Essentially, this is what I'd like to have:

var builder = WebApplication.CreateBuilder(args);
builder.Configuration.AddAzureAppConfiguration(new Uri("https://our-global-appconfiguration.azconfig.io"));

However, there are a couple of problems there:

  1. There is no overload for AddAzureAppConfiguration that take a Uri: only ones that take string for the connectionString approach. Yes, I know that is handled by Microsoft.Extensions.Configuration.AzureAppConfiguration and is thus not part of this repository
  2. One has to manually provide a tokenCredential value, so even if 1 was in place, it would still not work. We have to do this now:
var builder = WebApplication.CreateBuilder(args);
builder.AddAzureAppConfiguration(
    c => c.Connect(
        endpoint: new Uri("https://our-global-appconfiguration.azconfig.io"),
        credential: new DefaultAzureCredential()));

This is "mostly fine" (even though I think its excessively verbose and the credential argument should just default to DefaultAzureCredential anyways...), but the main issue starts when we have to deal with apps that are not hosted in Azure.

We are in the process of integrating Azure AppConfiguration in a large number of projects, some hosted in Azure, some on-premise services, and even some that are hosted in AWS.

Over this past few days, I've been looking into what the cleanest/safest/most manageable way would be to integrate these various services. For anything directly hosted in Azure, the answer has been pretty clearly to leverage Managed Identity as much as possible, which led us to a solution like the above.

However, the problem starts with the on-premise services, where we are going with dedicated "service principals" created in Entra ID for the applications.

Currently, our code looks exactly the same for those services:

var builder = WebApplication.CreateBuilder(args);
builder.AddAzureAppConfiguration(
    c => c.Connect(
        endpoint: new Uri("https://our-global-appconfiguration.azconfig.io"),
        credential: new DefaultAzureCredential()));

But now we realized we have a problem: we want to persist some of the non-sensitive values in local configuration, such as appSettings.json. But doing so, breaks the implementation as all 3 of the following are expected to be present as environment variables only:

  • AZURE_TENANT_ID
  • AZURE_CLIENT_ID
  • AZURE_CLIENT_SECRET

If we move the tenant ID and the client ID (which are not secrets) to some other configuration source, DefaultAzureCredential cannot read them anymore (as it cannot rely on EnvironmentCredential to fetch all 3 values). There is zero integration with IConfiguration at this level. Suddenly, this becomes a huge mess:

builder.Configuration.AddAzureAppConfiguration(c =>
    c.Connect(
        endpoint: new Uri("https://our-global-appconfiguration.azconfig.io"),
        credential: new ClientSecretCredential(
            tenantId: builder.Configuration["AZURE_TENANT_ID"],
            clientId: builder.Configuration["AZURE_CLIENT_ID"],
            clientSecret: builder.Configuration["AZURE_CLIENT_SECRET"])));

Having to read the individual values manually defeats the purpose of using DefaultAzureCredential (you can't even use it). You are then forced into changing the code to create an explicit ClientSecretCredential to pass in the 3 values. Then, if in the near future we want to switch to a cert-based auth, this needs to be changed manually again. And when we move the service from on-premises to inside Azure we have to update it yet again to change it back to either DefaultAzureCredential or the specific ManagedIdentityCredential.

This design seems terrible to me.

I found this library and immediately though we could use the AzureComponentFactory's CreateTokenCredential(IConfiguration) method. This would put us back into "sane" territory:

builder.Configuration.AddAzureAppConfiguration(c =>
    c.Connect(
        endpoint: new Uri("https://our-global-appconfiguration.azconfig.io"),
        credential: new AzureComponentFactory().CreateTokenCredential(builder.Configuration)));

But of course this doesn't work for a number of reasons:

  1. AzureComponentFactory is abstract, and its only implementation, AzureComponentFactoryImpl, is internal
  2. Even if the concrete type wasn't internal, it doesn't have a parameterless ctor (and relies on IServiceProvider which we don't have at this point)
  3. Even if we were able to instantiate it, the values it expects are in a completely different format than the ones from the environment variables.... no AZURE_ prefix, and camelCase instead of snake_case...
    internal static TokenCredential CreateCredential(IConfiguration configuration)
    {
    var credentialType = configuration["credential"];
    var clientId = configuration["clientId"];
    var tenantId = configuration["tenantId"];
    var resourceId = configuration["managedIdentityResourceId"];
    var clientSecret = configuration["clientSecret"];
    var certificate = configuration["clientCertificate"];
    var certificateStoreName = configuration["clientCertificateStoreName"];
    var certificateStoreLocation = configuration["clientCertificateStoreLocation"];
    var additionallyAllowedTenants = configuration["additionallyAllowedTenants"];
    var tokenFilePath = configuration["tokenFilePath"];
    IEnumerable<string> additionallyAllowedTenantsList = null;

At this point was when I decided to create this issue, because this seems extremely convoluted. I understand Microsoft.Extensions.Azure was created more to support clients such as the EventGrid one or direct KeyVault access, but the configuration flow needs some desperate help.

Not only is it super verbose, but it is also extremely brittle and inconsistent: each change of direction requires significant code changes, naming changes, etc.

This will force me to create some crazy custom extensions to workaround some of those limitations such that we can minimize the number of times we have to touch our code once it is deployed and needs Azure auth changes.

At this point I feel like I'm doing something extremely wrong... hopefully that is the case and all of these problems I listed are non-issues with a different approach that I'm not seeing.

Slightly related to:

@github-actions github-actions bot added Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Extensions ASP.NET Core extensions needs-team-attention This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Apr 15, 2024
Copy link

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@jsquire
Copy link
Member

jsquire commented Apr 16, 2024

Hi @julealgon. Thanks for reaching out and we regret that you're experiencing difficulties. I cannot speak for the extensions related to AddAzureAppConfiguration, as you've noted those are provided by the Microsoft.Extensions.Configuration.AzureAppConfiguration and maintained by the Azure SDK team.

Microsoft.Extensions.Azure allows for explicitly creating/setting a credential for clients via the AzureClientBuilder.UseCredential method, which also has an overload that allows you to define a credential factory, which I believe is the TokenProvider that you're asking about. More context and examples can be found in Dependency injection with the Azure SDK for .NET.

It is also possible to define the credential types recommended for production environments directly from configuration, allowing for both a global credential applied to all clients and credentials scoped to a specific service client. This allows credentials to be configured without the use of environment variables. More context and discussion can be found in Create Microsoft Entra credential types using configuration files.

That all said, I cannot speak for what Microsoft.Extensions.Configuration.AzureAppConfiguration may or may not support. If I understand correctly, the pattern that you're describing for creating/configuring the AppConfig provider is entirely defined in that package and may not fully integrate with Microsoft.Extensions.Azure. You may want to consider opening and issue in the repository where Microsoft.Extensions.Configuration.AzureAppConfiguration is maintained for your request.

I'm going to mark this as addressed, as I believe the feature that you're looking for needs to come from the AppConfig package. If I've misunderstood your scenario or request, please feel free to unresolve and continue the discussion.

@jsquire jsquire added issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. and removed needs-team-attention This issue needs attention from Azure service team or SDK team labels Apr 16, 2024
Copy link

Hi @julealgon. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

@julealgon
Copy link
Author

julealgon commented Apr 16, 2024

@jsquire

Hi @julealgon. Thanks for reaching out and we regret that you're experiencing difficulties. I cannot speak for the extensions related to AddAzureAppConfiguration, as you've noted those are provided by the Microsoft.Extensions.Configuration.AzureAppConfiguration and maintained by the Azure SDK team.

Yes. I'll probably open a separate request over there at least for the idea of providing an overload where the tokenProvider is defaulted to DefaultAzureCredential. I know that part is totally out of scope for this repo.

Microsoft.Extensions.Azure allows for explicitly creating/setting a credential for clients via the AzureClientBuilder.UseCredential method, which also has an overload that allows you to define a credential factory, which I believe is the TokenProvider that you're asking about. More context and examples can be found in Dependency injection with the Azure SDK for .NET.

None of that applies to the underlying client used by Azure AppConfiguration's standard integration though, as that client is used much earlier in the "host building" pipeline where not even the DI container is ready yet. My expectation was that, this package being a "helper for when using Microsoft.Extension.* abstractions", that it would also support the hosting aspect of Microsoft.Extensions, which it just doesn't at this point (it focuses only on the container-related aspects).

That all said, I cannot speak for what Microsoft.Extensions.Configuration.AzureAppConfiguration may or may not support. If I understand correctly, the pattern that you're describing for creating/configuring the AppConfig provider is entirely defined in that package and may not fully integrate with Microsoft.Extensions.Azure. You may want to consider opening and issue in the repository where Microsoft.Extensions.Configuration.AzureAppConfiguration is maintained for your request.

I'm going to mark this as addressed, as I believe the feature that you're looking for needs to come from the AppConfig package. If I've misunderstood your scenario or request, please feel free to unresolve and continue the discussion.

I disagree here. The purpose of Microsoft.Extensions.Azure is precisely to provide further helpers and abstractions to deal with Azure clients when leveraging Microsoft.Extensions constructs, like DI, but the entire hosting configuration abstraction is also part of that and has been ignored by this package.

This is why I feel adding such extensions here makes sense for this case. The native AppConfiguration package only concerns itself with the "raw" operations, while this here should add support for hosting, configuration, dependency injection and logging, which are all standard abstractions from Microsoft.Extensions.

Surely it would be possible to at least isolate the TokenCredential creation logic into a more reusable component that doesn't necessarily need DI integration? The core logic has zero dependencies and only takes an IConfiguration as parameter, which I have in the configuration stage. The only reason I can't currently use the factory directly is because extra dependencies it has for other behaviors, so right now it is clearly violating SRP and because of that making it hard or impossible for people to use just one aspect of its functionalities.

Before closing this issue, please reconsider extracting that logic that creates the token provider from config into a separate, public class, that more people can leverage. That would be an extremely simple way of allowing one to tap into the capability during configuration setup without needing you to write a bunch of custom extensions on top of AzureAppConfiguration (although that would probably be the ideal approach moving forward).

@jsquire
Copy link
Member

jsquire commented Apr 16, 2024

@julealgon: I think there's a disconnect as to the intent and purpose of Microsoft.Extensions.Azure. The package is intended to provide the infrastructure to allow clients from the Azure SDK to be integrated with ASP.NET dependency injection and allow DI client creation to integrate with configuration. It is not intended to be a general-purpose utility package nor a helper for using the Microsoft.Extension.* abstractions.

When this package was created, IConfiguration was still tightly coupled to ASP.NET scenarios. In the time since, the .NET team has evolved IConfiguration into a more general abstraction. We are in the process of considering what a general story for IConfiguration support would look like in the Azure SDK, but those are still early efforts. What we do not want to do in the meantime is expand the scope of the Microsoft.Extensions.Azure package and either lock ourselves into a design or introduce potential confusion due to having competing ways to accomplish the same task.

@julealgon
Copy link
Author

@jsquire

@julealgon: I think there's a disconnect as to the intent and purpose of Microsoft.Extensions.Azure. The package is intended to provide the infrastructure to allow clients from the Azure SDK to be integrated with ASP.NET dependency injection and allow DI client creation to integrate with configuration. It is not intended to be a general-purpose utility package nor a helper for using the Microsoft.Extension.* abstractions.

To me, these are basically synonyms at this point though, but fair enough.

When this package was created, IConfiguration was still tightly coupled to ASP.NET scenarios. In the time since, the .NET team has evolved IConfiguration into a more general abstraction. We are in the process of considering what a general story for IConfiguration support would look like in the Azure SDK, but those are still early efforts. What we do not want to do in the meantime is expand the scope of the Microsoft.Extensions.Azure package and either lock ourselves into a design or introduce potential confusion due to having competing ways to accomplish the same task.

I see. Well, if the team is already looking into native IConfiguration integration somewhere else, then I'm good.

For now, I'll try to keep this "as dumb as possible" for us and avoid defining any custom extensions and attempting to integrate with Microsoft.Extensions.Azure for this purpose then. I'll focus on just defining the 3 environment variables as per the requirements on EnvironmentCredential and go from there. It adds some overhead in terms of envvar management for us, but at least the application side is not that impacted.

I'm looking forward to those improvements you mentioned. I'm closing this then.

@julealgon julealgon closed this as not planned Won't fix, can't repro, duplicate, stale Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Extensions ASP.NET Core extensions issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

2 participants