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 StorageConnectionString from Azure.Storage.Common public #30893

Closed
danegsta opened this issue Sep 1, 2022 · 13 comments
Closed
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. 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)

Comments

@danegsta
Copy link

danegsta commented Sep 1, 2022

Library name

Azure.Storage.Common

Please describe the feature.

The v11 storage SDK had a CloudStorageAccount class that provides several features our team depends on in the Cloud Services extension for Visual Studio. However, the v12 storage SDK moved the equivalent functionality to an internal StorageConnectionString class, so there's no longer an equivalent way to access the functionality previously provided by CloudStorageAccount.

Our team uses the DevelopmentStorageAccount property from CloudStorageAccount to provide the appropriate connection string for connecting to a local storage emulator (either Azurite or the original Storage Emulator for users who haven't yet migrated). Additionally, we depend on the CloudStorageAccount.TryParse to validate user supplied connection strings to provide UI feedback if a connection string is malformed. While we could potentially do without the immediate feedback from connection string validation provided by TryParse, we don't have a viable alternative to the DevelopmentStorageAccount property for generating a valid local development connection string.

This request seems to be related to issues #11590 and #11329.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 1, 2022
@azure-sdk azure-sdk added Client This issue points to a problem in the data-plane of the library. needs-team-triage Workflow: This issue needs the team to triage. Storage Storage Service (Queues, Blobs, Files) labels Sep 1, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 1, 2022
@jsquire jsquire added 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. and removed needs-team-triage Workflow: This issue needs the team to triage. labels Sep 1, 2022
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Sep 1, 2022
@jsquire jsquire added Service Attention Workflow: This issue is responsible by Azure service team. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Sep 1, 2022
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Sep 1, 2022
@ghost
Copy link

ghost commented Sep 1, 2022

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

Issue Details

Library name

Azure.Storage.Common

Please describe the feature.

The v11 storage SDK had a CloudStorageAccount class that provides several features our team depends on in the Cloud Services extension for Visual Studio. However, the v12 storage SDK moved the equivalent functionality to an internal StorageConnectionString class, so there's no longer an equivalent way to access the functionality previously provided by CloudStorageAccount.

Our team uses the DevelopmentStorageAccount property from CloudStorageAccount to provide the appropriate connection string for connecting to a local storage emulator (either Azurite or the original Storage Emulator for users who haven't yet migrated). Additionally, we depend on the CloudStorageAccount.TryParse to validate user supplied connection strings to provide UI feedback if a connection string is malformed. While we could potentially do without the immediate feedback from connection string validation provided by TryParse, we don't have a viable alternative to the DevelopmentStorageAccount property for generating a valid local development connection string.

This request seems to be related to issues #11590 and #11329.

Author: danegsta
Assignees: -
Labels:

Storage, Service Attention, Client, customer-reported, feature-request

Milestone: -

@jsquire
Copy link
Member

jsquire commented Sep 1, 2022

//fyi: @seanmcc-msft, @jaschrep-msft, @tg-msft

@danegsta
Copy link
Author

Has there been any chance to review this feature request? This has been a blocker for a while in our VS extension from being able to upgrade to the latest storage SDK due to our need to support local emulator connections for customer debug scenarios.

@jaschrep-msft
Copy link
Member

@danegsta please confirm these are the two asks

  1. Ability to get a connection string such that the SDK will authenticate to Azurite (emulator is no longer officially supported as I understand it)
  2. Ability to validate a connection string for correctness

@danegsta
Copy link
Author

Fundamentally it is two different asks; the DevelopmentStorageAccount class would fulfill both needs, but that's ultimately an implementation detail. Arguably #1 is the most important, as I can work around #2 somewhat as it isn't strictly necessary, just nice to have.

As far as the storage emulator, we do still technically support it in our extension for customers who haven't migrated to Azurite yet, but Azurite is the core scenario (if no emulator is running, we'll launch Azurite, but we'll work with the old Storage Emulator if they've started it themselves).

@jaschrep-msft
Copy link
Member

We can discuss these asks amongst the team.

As far as the storage emulator, we do still technically support it in our extension for customers who haven't migrated to Azurite yet, but Azurite is the core scenario

You may support the emulator, but the emulator is no longer a supported product. We do not guarantee support for it.

@danegsta
Copy link
Author

I appreciate it. Don't worry, we know the emulator is out of support and only support it ourselves in the loose sense that we aren't actively blocking customers from using it in their workflow if they chose to; we don't have any expectation of the storage SDK doing any work to ensure compatibility with the emulator.

@jaschrep-msft
Copy link
Member

Hello @danegsta. At this point in time, Azurite documents values for how to point the Storage library towards it successfully.

It notes a few connection string options. UseDevelopmentStorage=true; is not currently supported by v12 SDK. However, the other constant string is perfectly valid, and that's all that's generated by us when we run the values through our existing builder/parser.

So when it comes to the two asks defined earlier:

  1. There is a documented connection string already made, published by the Azurite developers. The ability to obtain it can be done by just storing it in a constant.
  2. Validating said string for correctness is not necessary if you are not constructing said string. If the string is wrong, Azurite documentation is wrong and needs a bug filed against it.

If I'm misunderstanding the needs, please let me know, but I think your solution is to just store the well-known connection string in your extension.

@danegsta
Copy link
Author

Seems like that static config string should work for us in place of DevelopmentStorageAccount. For number 2, we use the parsing to provide some basic validation on user configured connection strings, but from digging through the SDK code it looks like creating a BlobClient instance will ultimately call StorageConnectionString.Parse internally, so I think I can use that instead.

Thanks for following up; I think this unblocks us from migrating to v12.

@danielmarbach
Copy link
Contributor

@danegsta how did you end up solving number 2? It sounds like you ended up try catching around the creation of the BlobClient instance, is my understanding correct?

For the other's what is the rational of not allowing the connection strings to be parsed anymore? I was trying to follow the traces in this issue including the discussion in #11590. As @SeanFeldman pointed out in the discussion there are still quite a few use cases where parsing might be required. For example in our case we get a connection string at runtime which we need to parse to see whether it is valid and only if it is valid we can continue doing parts of the associated logic. In our case we would now need to try to create the corresponding client in a try catch too similar to what @danegsta is hinting at. Or should we just copy and paste the relevant logic from the SDK locally?

@danegsta
Copy link
Author

danegsta commented Oct 3, 2022

@danielmarbach for number 2, I ended up finding that the BlobServiceClient (or really any of the storage clients) internally calls parse if passed a connection string (as well as a couple additional validation checks that parse doesn't run), so yeah if I need to validate a connection string I'm just initializing a client and doing try/catch to report any errors that are thrown. That isn't exactly as clear or straightforward as invoking Parse directly, but I've wrapped the process in a utility method to at least make it obvious why a throwaway client is being initialized.

Since the client constructor invokes Parse on the connection string passed in, it's effectively a roundabout way of calling Parse myself.

@danielmarbach
Copy link
Contributor

Honestly to me that doesn't sound like a good tradeoff. Basically before we had the possibility to parse connection strings in a safe way without paying the price of a runtime exception. Now you need to rely on some implementation detail that might change in the future. All that to avoid making the connection string parsing public

@SeanFeldman
Copy link
Contributor

I wish the Azure SDK team would listen to their customers a bit more. It's okay to revert a decision that was done in the past when new details and information emerges. Especially, when it's coming from the clients, using these SDKs day in and day out, showing the empirical evidence the previous decision had an oversight. No one got fired for admitting an improvement could be made and implementing it to help customers.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2023
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. 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)
Projects
None yet
Development

No branches or pull requests

7 participants
@danielmarbach @jsquire @SeanFeldman @jaschrep-msft @danegsta @azure-sdk and others