-
Notifications
You must be signed in to change notification settings - Fork 739
Azure PostgreSQL client #8230
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
Azure PostgreSQL client #8230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an Azure PostgreSQL client and related extensions that simplify registering Npgsql data sources with Azure-specific configurations, health checks, tracing, and metrics. Key changes include the addition of helper methods in NpgsqlDataSourceHelper.cs, new extension methods in AspireAzureNpgsqlExtensions.cs and AspirePostgreSqlNpgsqlExtensions.cs, and updates to supporting configuration and documentation files.
Reviewed Changes
Copilot reviewed 18 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Components/Aspire.Npgsql/NpgsqlDataSourceHelper.cs | Introduces a generic helper method to register Npgsql data sources with health check, telemetry, and metrics support. |
| playground/AzurePostgres/AzurePostgres.AppHost/Program.cs | Demonstrates a playground setup for Azure PostgreSQL flexible server usage. |
| playground/AzurePostgres/AzurePostgres.Api/Program.cs | Adds an example API consuming the Azure Npgsql data source. |
| src/Components/Aspire.Azure.Npgsql/AzureNpgsqlSettings.cs | Adds Azure-specific settings by extending NpgsqlSettings with a TokenCredential property. |
| src/Components/Aspire.Azure.Npgsql/README.md | Updates documentation with details and usage examples for the Azure PostgreSQL client. |
| src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs | Introduces extension methods for adding Azure Npgsql data sources including username resolution from token claims. |
| src/Components/Aspire.Npgsql/AspirePostgreSqlNpgsqlExtensions.cs | Refactors the existing Npgsql extension to leverage the shared helper and adjusts the health check prefix. |
| src/Components/Aspire.Npgsql/NpgsqlSettings.cs | Changes NpgsqlSettings from a sealed class to an open class, allowing further subclassing if needed. |
Files not reviewed (11)
- Aspire.sln: Language not supported
- playground/AzurePostgres/AzurePostgres.Api/AzurePostgres.Api.csproj: Language not supported
- playground/AzurePostgres/AzurePostgres.Api/Properties/launchSettings.json: Language not supported
- playground/AzurePostgres/AzurePostgres.Api/appsettings.Development.json: Language not supported
- playground/AzurePostgres/AzurePostgres.Api/appsettings.json: Language not supported
- playground/AzurePostgres/AzurePostgres.AppHost/AzurePostgres.AppHost.csproj: Language not supported
- playground/AzurePostgres/AzurePostgres.AppHost/Properties/launchSettings.json: Language not supported
- playground/AzurePostgres/AzurePostgres.AppHost/appsettings.Development.json: Language not supported
- playground/AzurePostgres/AzurePostgres.AppHost/appsettings.json: Language not supported
- src/Components/Aspire.Azure.Npgsql/Aspire.Azure.Npgsql.csproj: Language not supported
- src/Components/Aspire.Azure.Npgsql/ConfigurationSchema.json: Language not supported
Comments suppressed due to low confidence (1)
src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs:89
- [nitpick] Consider enhancing the error message by indicating which token claims were expected (e.g. 'upn', 'preferred_username', or 'unique_name') to aid troubleshooting.
throw new InvalidOperationException("Could not determine username from token claims");
|
cc @roji |
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see a nice out-of-the-box experience here :) Though I'm sad that this is an Aspire thing - other non-Aspire users have the same problem and we should fix the problem for everyone. But anyway.
Implementation-wise, see note about caching the token. Also, how does this integrate with e.g. the Aspire EF component? Does the EF component simply resolve the NpgsqlDataSource that this new project registers there (with the Azure auth logic)?
|
|
||
| if (string.IsNullOrEmpty(dataSourceBuilder.ConnectionStringBuilder.Password)) | ||
| { | ||
| // The token is not cached since it it refreshed for each new physical connection, or when it has expired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the ideal thing here to not re-retrieve for every physical open, i.e. cache the token and only retrieve it again once we're getting close to expiry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should rely on the connection pool to cache the token indirectly. Or do you mean that this lambda is invoked on each logical connection (ado.net one)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the password provider lambda indeed only gets invokved when a new physical (unpooled) connection needs to be created (that's when authentication occurs). So perf-wise, caching is indeed not super critical here as it doesn't affect pooled connections in any way.
However, it's still a good idea to reduce the time it takes to establish new physical connections - this can affect startup responsiveness and make things slower than they have to be. As an example, PostgreSQL recently added a new SSL protocol flow to knock out an additional network roundtrip from physical connection setup, so if we can avoid adding a needless one, we should...
Any specific reason to not cache here? It seems pretty trivial...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what its worth, generally token sources are already pretty heavily cache underneath already. A reason not to cache would be rapid invalidation upstream which needs to be accounted for - if we hit auth issues we'd need to be sure to purge any cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @NickCraver 👋
That's a pure Azure question - can you confirm that azure Credentials indeed cache? If they do - and it's documented behavior we can rely on - then I agree we should not be caching here.
A reason not to cache would be rapid invalidation upstream which needs to be accounted for - if we hit auth issues we'd need to be sure to purge any cache.
This could maybe be dealt with via automatic retrying of the open; in any case, I haven't heard of authentication tokens being invalidated before their published expiry time... I get the scenario in principle, but is this something that actually happens?
I'm asking since reducing physical connection time is also an important goal for good application latency - some applications scale down their connection pool in times of less load, and when they ramp up again it could be important that not every physical connection needlessly fetch new tokens while they already have a perfectly good one in hand...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're talking MSI, then yes - documented here: https://learn.microsoft.com/en-us/entra/identity/managed-identities-azure-resources/managed-identities-faq#are-managed-identities-tokens-cached ...and I can confirm this directly from several interactions too. There is quite heavy caching at several layers. However, TokenCredential could be anything so depending on scope that could not be true in some other use case.
As for invalidation, there are 2 aspects: authentication and authorization. The latter RBAC could be revoked at any time effectively invalidating the credential. I'm less familiar if tokens themselves are invalidated but given the latter, I'm not sure the former effectively matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - though the wording in that FAQ seems to suggest that the backend services are the one caching, meaning that maybe we'd still do the network roundtrip for no good reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji It's cached even at the local wire server level (described as backend there) - can grab time and walk through how the infra works if helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see :) Thanks @NickCraver, no need to dive into the details. If you're saying that in most typical scenarios this gets cached and there's no need for us to add caching on top at this layer, that's more than good enough for me!
src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs
Outdated
Show resolved
Hide resolved
playground/AzurePostgres/AzurePostgres.Api/AzurePostgres.Api.csproj
Outdated
Show resolved
Hide resolved
|
We should update the docs to point to this new library. See https://learn.microsoft.com/dotnet/aspire/database/azure-postgresql-integration?tabs=dotnet-cli#add-azure-authenticated-npgsql-client for the old docs. |
src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eric Erhardt <[email protected]>
src/Components/Aspire.Npgsql/AspirePostgreSqlNpgsqlExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs
Outdated
Show resolved
Hide resolved
eerhardt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work here!

Description
Adding Aspire.Azure.Npgsql client
componentintegration that can use aTokenCredentialfor authenticating to an Azure PostgreSQL database.Part of #6669
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):