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

WIP: sketching up support for other Azure environments #193

Closed

Conversation

jesinity
Copy link

@jesinity jesinity commented Mar 22, 2021

hello, in this WIP I am schetching up the support for other cloud environments (in blob endpoint at least, but it would require all the services as the keyvalut, and others....) as It could be for Azure China, or Azure Germany.

It is just a sketch, if you could guide me through on how you would approach this I am happy to contribute to this task.

Also note that there likely to be a typo ( "parms" instead of "params") in the signature of the methods

@jesinity jesinity changed the title WIP: schetching up support for other Azure environments WIP: sketching up support for other Azure environments Mar 22, 2021
@jesinity jesinity force-pushed the FT_support_other_environments branch 2 times, most recently from 26a8a45 to 1e5107a Compare March 22, 2021 09:24
@MindFlavor
Copy link
Contributor

Thank you for taking care of this! 👍
I know it's still a WIP but I want to point out two things:

  1. You should probably wait for Storage crate reorg and cleanup #194 to be merged before working on this further. While that PR won't change much of the code, there is a lot of renaming/shuffling that will probably confuse git.
  2. You should use once_cell instead of lazy_static! (I wasn't aware of it either until few days ago and @rylev pointed it out to me).

@jesinity
Copy link
Author

hello thanks for your comments! I will wait for #194 it to be completed then.
Do you think it makes sense to create a global "Environment" for all endpoints (keyvault, storage etc ) or do you prefer it to be separated?

@jesinity jesinity force-pushed the FT_support_other_environments branch from 1e5107a to c5c5ad7 Compare March 28, 2021 21:06
@heaths
Copy link
Member

heaths commented Mar 29, 2021

For the official Azure SDKs, we take the whole URL (or full host name if in a connection string, for example) such that we don't need explicit environment support. For consistency with Azure SDK guidelines, we should do that here as well. This way, users just provide the whole URL and just about everything works. Our token credentials, when necessary, may need to know about environments but, in general, the discovery of authorities should be automatic (e.g. see ClientSecretCredential in .NET) or a challenge response should include the right authority and scope in its WWW-Authentication header.

This also allows for onboarding new environments easily since there's no dependency internally, not to mention makes it possible to use against Azure Stack.

@jesinity
Copy link
Author

Right, what you think about :

  • providing the base_url as a parameter in another method ("https://blob.core.windows.net" by default)
  • adding an utility close to what is done in Powershell Azure Module, where you have the cmdlet Get-AzureEnvironment that gives you all the known environments with all supported endpoints?

@heaths
Copy link
Member

heaths commented Mar 31, 2021

In the Azure SDK we actually take the full URI (or connection string with full host name, etc.) in a constructor/builder, such that it's not passed to every single method. The idea is that each method now takes minimal parameters to call against that URI for client so that you're not having to repeat the URI or environment over and over. Having a fixed list of environments also doesn't solve the Azure Stack issue.

}

lazy_static! {
static ref AZURE: AzureEnvironment = AzureEnvironment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn get_sas_token_parms(sas_token: &str) -> Result<Vec<(String, String)>, url::ParseError> {
fn get_sas_token_params_with_env(
sas_token: &str,
environment: &AzureEnvironment,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the general comments, this is inconsistent with Azure SDK guidelines that SDKs be consistent except when idiomatic differences would make more sense, and I don't see taking an environment with fixed URIs is idiomatic. Supplying a full URI is just as easy. Customers could also keep their own structure of common URIs and pass those as &str parameters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the Azure SDK hasn't done much with environments (not in public APIs, anyway - certainly in testing) in support of Azure Stack, in which case the customer is entirely in control of their domain suffixes and endpoints in general.

@ctaggart
Copy link
Contributor

ctaggart commented Jun 1, 2021

What about just pub const &strs , similar to #278?

@heaths
Copy link
Member

heaths commented Jun 1, 2021

For now, we should take in domain-specific strings (or Urls, ideally) to match the other Azure SDKs, but there is work being done - driven by Identity - that will be providing a mechanism like this. While we want to be idiomatic across the SDKs for each language, we also want to be consistent and should follow suit.

I'm going to close this for now - and thank you for all your work - but will try to remember to post back here when more is known. I imagine it will look something like this (/cc @schaabs) and, if you're still interested, we'd love if you wanted to contribute a change using a similar mechanism.

@heaths heaths closed this Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants