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] Make ConnectionString class public #11590

Closed
vladkol opened this issue Apr 24, 2020 · 29 comments
Closed

[FEATURE REQ] Make ConnectionString class public #11590

vladkol opened this issue Apr 24, 2020 · 29 comments
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. feature-request This issue requires a new behavior in the product in order be resolved. issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. Service Attention Workflow: This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files)
Milestone

Comments

@vladkol
Copy link

vladkol commented Apr 24, 2020

Azure.Core

Make ConnectionString class public so developers can leverage it broadly, as "Key=Value;" format is widely adopted in Azure.

For example, Storage SDK used to have CloudStorageAccount.Parse. It doesn't exist in v12, and it's Storage-specific.
Now, in order to use Azure.Storage.StorageSharedKeyCredential with a connection string (together with BlobSasBuilder, for example) , they have to extract AccountKey value from the connection string themselves.

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Apr 24, 2020
@jsquire jsquire added Azure.Core Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved. needs-team-triage Workflow: This issue needs the team to triage. labels Apr 25, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Apr 25, 2020
@jsquire jsquire removed the question The issue doesn't require a change to the product in order to be resolved. Most issues start as that label Apr 25, 2020
@pakrym
Copy link
Contributor

pakrym commented Apr 25, 2020

I'm not sure I understand the use case for storage.

For SAS Url there is a ctor that doesn't take a credential https://docs.microsoft.com/en-us/dotnet/api/azure.storage.blobs.blobclient.-ctor?view=azure-dotnet#Azure_Storage_Blobs_BlobClient__ctor_System_Uri_Azure_Storage_Blobs_BlobClientOptions_ but I think I'm missing something because SAS is not a connection string.

In general our goal was to free customers from having to parse connection strings and expose ways to consume entire connection and parse it internally.

Can you please provide more details on when you have a connection string but have to manually parse it to create a valid client?

@vladkol
Copy link
Author

vladkol commented Apr 25, 2020

BlobClient itself is not as issue. The problem I faced is when I want to create an SAS-token myself, StorageSharedKeyCredential requires AccountName and AccountKey separately.
2 more examples on top of my head are:

  • Configuring Blob Storage on IoT Edge (they want those values separately again)
  • Creating a custom SAS for Service Bus
    In both cases I need to deal with keys, so if initially my service was configured with a connection string, I have to parse it myself.

@AlexGhiondea AlexGhiondea added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-team-triage Workflow: This issue needs the team to triage. labels May 4, 2020
@pakrym pakrym added this to the Backlog milestone May 5, 2020
@AlexGhiondea AlexGhiondea removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label May 7, 2020
@Nisden
Copy link

Nisden commented May 19, 2020

We have the same issue. We want to return an URL, that allows a web browser to download the file directly from Azure Blob Storage. However we only store the ConnectionString in our configuration.

My current workaround looks like this

    public string DownloadUrl(BlobContainerClient client, string blobFileName, string downloadFileName, string azureStorageConnectionString)
    {
        // HACK: Resolve internal StorageConnectionString to parse storage connection string
        var storageConnectionStringType = Type.GetType("Azure.Storage.StorageConnectionString, Azure.Storage.Common");

        var storageConnectionString = storageConnectionStringType
            .GetMethod("Parse", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static)
            .Invoke(null, new object[] { azureStorageConnectionString });

        var storageCredentials = (StorageSharedKeyCredential)storageConnectionStringType
            .GetProperty("Credentials", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public)
            .GetValue(storageConnectionString);

        // Resolve the details about our blob
        var blobClient = client.GetBlobClient(blobFileName);
        BlobSasBuilder blobSasBuilder = new BlobSasBuilder()
        {
            BlobContainerName = blobClient.BlobContainerName,
            BlobName = blobClient.Name,
            Protocol = storageCredentials.AccountName == "devstoreaccount1" ? SasProtocol.HttpsAndHttp : SasProtocol.Https,
            CacheControl = "no-store",
            ExpiresOn = DateTimeOffset.UtcNow.AddMinutes(10)
        };
        blobSasBuilder.SetPermissions(BlobSasPermissions.Read); // Only allow downloading the file

        // Ensure browser downloads the file instead of loads it in the current page, and if required change filename
        var contentDispositionHeader = new System.Net.Http.Headers.ContentDispositionHeaderValue("attachment");
        if (downloadFileName != null)
        {
            contentDispositionHeader.FileName = downloadFileName;
        }
        blobSasBuilder.ContentDisposition = contentDispositionHeader.ToString();

        // Return the full url including sasToken
        string sasToken = blobSasBuilder.ToSasQueryParameters(storageCredentials).ToString();
        return blobClient.Uri.AbsoluteUri + "?" + sasToken;
    }

@pakrym
Copy link
Contributor

pakrym commented May 19, 2020

@tg-msft, tell me if I'm wrong but aren't you addressing the later issue with the new SasBuilder design and allowing to create it from the client?

@tg-msft
Copy link
Member

tg-msft commented May 19, 2020

The SAS changes won't help with this problem - you'll still need to pull a credential for signing from somewhere.

I wouldn't expose the entirety of the existing StorageConnectionString class because it's got a larger API surface area than we need here. We should look into something like a StorageConnectionBuilder if it's possible to create a utility analogous to BlobUriBuilder in Azure.Storage.Common. The trickier part will be serializing out a TokenCredential which may force us to only support parsing instead of creation.

This shouldn't be assigned to Azure.Core so I'm changing everything to Storage.

@tg-msft tg-msft added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team Service Attention Workflow: This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files) and removed Azure.Core labels May 19, 2020
@ghost
Copy link

ghost commented May 19, 2020

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

@SeanFeldman
Copy link
Contributor

Having StorageConnectionString functionality hidden forces workarounds that every developer working with a connection string will end up implementing. I have a need to parse a connection string to extract information and rather not use reflection or duplicated the code.

Here's an example of how having this functionality public would save unnecessary issues and save me time:

@tomkerkhove
Copy link
Member

We frequently use these things as well and would be happy to have it public

@pinkfloydx33
Copy link

We appreciated the connection string validation we got with TryParse. But we also have some scenarios were we have abstracted some of the work and need to generally build and/or validate URLs, etc. using information gleaned from the connection string. For example, ensuring that a blob URL is part of the same account/container as another component. At the moment, we are keeping a reference to the old library just for CloudStorageAccount.[Try]Parse, keeping around the account object to use properties off it (since reflection would get ugly there). The CloudStorageAccount also gives you access to the SecondaryUri's allowing you to pass them via BlobClientOptions (I know of no other way to retrieve this value?)

We were trying to use the BlobUriBuilder to construct a dynamic blob URI, only to realize we still needed to pass in some credentials and the only way to get those was to parse them out of the connection string

@okolobaxa
Copy link

I also used CloudStorageAccount.Parse(_connectionString) method for connection string validation and extracting parts and had to copy-paste entire internal StorageConnectionString class for this.

@StannieV
Copy link

StannieV commented Nov 25, 2020

It looks like I also need the StorageConnectionString class to extract the 'StorageAccountName' and 'StorageAccountKey' from the connection string to create a SAS when I follow the documentation.

Off-topic but related because this is about generating a SAS and is the reason why I need the ConnectionString class to be public:
Unfortunately, the following code doesn't work. The 'GetBlobClient()' on the container doesn't invoke the correct constructor overload on BlobClient. In the current implementation '_storageSharedKeyCredential' remains empty.

var connectionString = "***";            
var containerName = "test";

BlobContainerClient container = new BlobContainerClient(connectionString, containerName);
foreach (var blobItem in container.GetBlobs())
{
    BlobClient blobClient = container.GetBlobClient(blobItem.Name);

    if (blobClient.CanGenerateSasUri)
    {
        var expiresOn = DateTimeOffset.Now.Add(TimeSpan.FromHours(5));
        var sas = blobClient.GenerateSasUri(BlobSasPermissions.Read, expiresOn);

        Console.WriteLine($"{blobItem.Name}: {sas}");
    }
}

But if you create the BlobClient with the correct constructor overload (with the connection string) there is no need to parse the connection string with the 'StorageConnectionString' class to create a SAS:

var connectionString = "***";            
var containerName = "test";

BlobContainerClient container = new BlobContainerClient(connectionString, containerName);
foreach (var blobItem in container.GetBlobs())
{
    BlobClient blobClient = new BlobClient(connectionString, containerName, blobItem.Name);

    if (blobClient.CanGenerateSasUri)
    {
        var expiresOn = DateTimeOffset.Now.Add(TimeSpan.FromHours(5));
        var sas = blobClient.GenerateSasUri(BlobSasPermissions.Read, expiresOn);

        Console.WriteLine($"{blobItem.Name}: {sas}");
    }
}

It would be better if the 'GetBlobClient()' method on the container would be able to return a BlobClient were the property of 'CanGenerateSasUri' is true after it's initialized with a connection string. With this solution, you're forced to pass the connection string throughout the application. I prefer to get it from the config/KeyVault in the startup class and only use the BlobContainerClient.

@amnguye
Copy link
Member

amnguye commented Dec 1, 2020

Hi @StannieV . I already created an issue for the off topic GetBlobClient issue you are referring to and am looking to resolve it with a PR I put out yesterday. Please look forward to the next release when the fix comes out. Thanks!

If you have anything to add to the relevant to this thread, feel free to, otherwise please add your concerns to the bug you are referring to the issue I linked, so we can stay organized with the issue at hand.

@StannieV
Copy link

Hi @StannieV . I already created an issue for the off topic GetBlobClient issue you are referring to and am looking to resolve it with a PR I put out yesterday. Please look forward to the next release when the fix comes out. Thanks!

If you have anything to add to the relevant to this thread, feel free to, otherwise please add your concerns to the bug you are referring to the issue I linked, so we can stay organized with the issue at hand.

Just tested it, problem solved in version 12.8.0!

@lundog
Copy link

lundog commented Feb 11, 2021

This (or some sort of parser/builder) would be very helpful as connection string is the most common format for storing connection details in configuration. Having some methods accept connection string while others accept account name + account key forces us to either store redundant info in configuration or roll our own connection string logic.

@amishra-dev
Copy link
Contributor

@kasobol-msft
Kamil, there seems to be enough interest in this. Lets reevaluate this feature request.

@DaleyKD
Copy link

DaleyKD commented May 6, 2021

I very much could utilize this.

I just need it for simplicity:

  1. If the connectionString passed in has an AccountKey, then I'll use the QueueClient ctor that takes a connectionString and queueName.
  2. If the connectionString passed in does NOT have an AccountKey, then I assume Managed Identity and will use the QueueClient ctor that takes the Uri and TokenCredential.

@kasobol-msft
Copy link
Contributor

@DaleyKD could you please elaborate more (best provide code snippet of how do you solve it now vs desired) so that we can understand your scenario better?

If you have some connection string in hand it's going to have either key or sas token. Why would you need to parse it to double check that instead of directly feeding connection string into QueueClient ctor?

@DaleyKD
Copy link

DaleyKD commented May 6, 2021

@kasobol-msft Great question. I don't have a solution right now outside of using reflection.

The main reason for this is local development vs. AKS deployment.

  • When local development, we use Azurite in Docker and the connection string looks like DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;QueueEndpoint=http://azurite:10001/devstoreaccount1;
  • When deploying to our AKS cluster, we will use an actual Azure Storage Queue, and to try and use Managed Identity with it, the connection string would look more like ...

Well, now that I'm typing it out, maybe it's not such a big deal. We'd probably end up passing in (as part of our Helm values) https://{storage-account-name}.queue.core.windows.net and then if that's a Uri, we'll append the queue name to the end.

Ignore me.

@kasobol-msft
Copy link
Contributor

@DaleyKD you can use QueueUriBuilder to append the queue name to base URI.

@SeanFeldman
Copy link
Contributor

@kasobol-msft it's been over a year since the issue was raised and backlogged. It's still needed. What are the plans to address it? Going through reflection is a workaround, and it would be great to see a more permanent solution.

@kasobol-msft
Copy link
Contributor

@SeanFeldman So far, for each ask there was an alternative without exposing SDK internals. Please describe your scenarios.

@SeanFeldman
Copy link
Contributor

@kasobol-msft, in my case I need to generate an account SAS token for a whole container and not specific files. This didn't work in the past but could be resolved. I'll give it a whirl.

@SeanFeldman
Copy link
Contributor

I've taken a look at the original code and think I have sorted it out.
The original code used AccountSasBuilder configured for AccountSasServices.Blobs with AccountSasPermissions. I had to switch to BlobSasBuilder and BlobContainerSasPermissions that would be passed into BlobContainerClient.

@kasobol-msft, I've also looked again at the migration guide and couldn't see anything. This issue would be much simpler if customers could find the necessary information in the upgrade guide.

@kasobol-msft
Copy link
Contributor

Thanks @SeanFeldman for pointing out the migration guide gap, we'll take care of that.

@amishra-dev amishra-dev added the issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. label Oct 26, 2021
@ghost
Copy link

ghost commented Oct 26, 2021

Hi @vladkol. 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.

@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Oct 26, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

Hi @vladkol, since you haven’t asked that we “/unresolve” the issue, we’ll close this out. If you believe further discussion is needed, please add a comment “/unresolve” to reopen the issue.

@ghost ghost closed this as completed Nov 3, 2021
@danegsta
Copy link

danegsta commented Sep 1, 2022

This issue is currently blocking our team from upgrading from the v11 to v12 SDK in the Cloud Services extension for Visual Studio. We depend on the DevelopmentStorageAccount property to connect to Azurite (or the Storage Emulator for users that haven't migrated yet) for local debugging as well as providing UI for creating and editing storage connection strings. With ConnectionString being internal in v12, we would be missing this essential functionality to support our existing features once we upgrade.

@danegsta
Copy link

danegsta commented Sep 1, 2022

/unresolve

@ghost
Copy link

ghost commented Sep 1, 2022

Hi danegsta, only the original author of the issue can ask that it be unresolved. Please open a new issue with your scenario and details if you would like to discuss this topic with the team.

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. feature-request This issue requires a new behavior in the product in order be resolved. issue-addressed Workflow: The Azure SDK team believes it to be addressed and ready to close. Service Attention Workflow: This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests