[API Proposal] Expose SqlConnection.IsAzureDb

grant-d commented 6 years ago

Provisioning and/or runtime logic may depend on whether a specified SqlClient DataSource is traditional MSSQL or AzureDb. The aim of this proposal is to expose the existing function for public consumption.

Rationale and Usage

This can be done in custom code by emulating the same logic as the ADP class, however that approach is fragile should (say) Azure open another endpoint (as happened with database.chinacloudapi.cn and secure.windows.net).

Here's an example callsite where we'd harden a user-supplied connection string (app.config) for SqlAzure. Today we do this via the emulation described above.


SqlConnectionStringBuilder sqlCsb = …; // Load from app.config
if (sqlCsb.IsAzureDb)
{
    sqlCsb.TrustServerCertificate = false;
    sqlCsb.Encrypt = true;
    … // Different security, retry policy, timeouts, etc
}

Proposed API

// Existing: src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.SqlClient.cs
namespace System.Data.Common
{
    internal static partial class ADP
    {
        // Code compares DataSource against a set of (internally) well-known endpoints
        internal static bool IsAzureSqlServerEndpoint(string dataSource) { … }
    }
}

// Suggested public api
public sealed class SqlConnectionStringBuilder
{
    public bool IsAzureDb => ADP.IsAzureSqlServerEndpoint(this.DataSource);
}

// and/or
public sealed class SqlConnection
{
    public bool IsAzureDb => ADP.IsAzureSqlServerEndpoint(this.DataSource);
}

Details

This would appear to be low-risk, since the functionality already exists and is leveraged by SqlClient.
The internal method may need to be hardened against nulls, casing, etc

Questions

Not sure what the best name for this property would be; IsAzureSql, IsAzure, IsAzureDb, IsAzureDataSource, IsAzureEndpoint
Not sure whether to expose this property on one or both classes
Not sure whether it should be a concrete property on the class(es) or rather extension method(s)

grant-d commented 6 years ago

Also see this msdn article where they advise how Azure/non-Azure affects pooling: https://docs.microsoft.com/en-us/dotnet/framework/whats-new/index#v462 (see the Auto section)

karelz commented 6 years ago

cc @divega

Clockwork-Muse commented 6 years ago

sqlCsb.TrustServerCertificate = false;
sqlCsb.Encrypt = true;

In an ideal world shouldn't those always be set like that?

grant-d commented 6 years ago

Any news on getting this through review. I'd like to PR it

karelz commented 6 years ago

First, area owners have to look at it. They may be swamped (SqlClient historically is). After re-reading your proposal, personally, I am not sure it is that critical, or desirable to have such API. (but I am not expert, so if there is anough interest from area owners/experts or community experts, feel free to discard my comment)

grant-d commented 5 years ago

Thanks @karelz. I am a fairly new contributor so I do not have a strong grasp of how features are ushered through the process. Agree that it is non-critical but since it is already used internally, its utility as a public api is worth consideration. Being able to ask a db 'are you in an Azure Paas context' may be useful more generally than my own supportive use case. Lift & shift comes to mind.

karelz commented 5 years ago

I am a fairly new contributor so I do not have a strong grasp of how features are ushered through the process.

Understood. Just trying to set expectations :) ... the API process is a bit slow by design (think weeks at minimum). Some areas have much longer backlogs :( (think months/years) and therefore have to prioritize heavily based on usefulness compared to other APIs/features in the area. You can see the API process details here: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

since it is already used internally, its utility as a public api is worth consideration.

It is worth consideration, it does not mean it has to be prioritized though.

may be useful more generally than my own supportive use case. Lift & shift comes to mind.

I'll leave it to area experts to comment on that. If there is good scenario, it would make sense ...

divega commented 5 years ago

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

David-Engel commented 5 years ago

We can't guarantee that it always returns the correct value. And it could change over time. Right now those functions are used in places that continue to work if the method returns the wrong result. If you want to code a function into your app that does the same thing, SqlClient currently looks for the following endsWith in the datasource:

.database.chinacloudapi.cn .database.windows.net .database.cloudapi.de .database.usgovcloudapi.net

cheenamalhotra commented 3 years ago

As explained in @David-Engel 's above comment and also given Azure's variety of servers, this API design is not feasible from driver layer.

David-Engel commented 3 years ago

I just want to reiterate the existing function's limitations. If it were to be made into a public API, we would want to ensure that it always returned the correct answer. Right now, it uses a brittle method of simply parsing the data source name, which may change in the future.

It's okay to do that where it is used internally because it's not used for critical functionality and if it's wrong, things still work, just a little differently. I would consider the setting of encrypt/trustservercertificate values to be security-critical and I would not want anyone depending on the current internal IsAzureDb implementation for those values.

grant-d commented 3 years ago

Thanks @David-Engel, that is a good argument why it's not a safe bet.

dotnet / SqlClient