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

Adding Azure sovereign cloud support #7

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

gvashishtha
Copy link

@gvashishtha gvashishtha commented Aug 1, 2023

What changed

Stow's current implementation of Azure Storage support uses NewBasicClient which defaults to a core.windows.net storage URL. This PR switches to NewClient which supports an arbitrary user-defined base URL. If the user does not pass in a base_url, we default to using core.windows.net (public cloud), thereby avoiding any breaking changes.

Why this change is necessary

Sovereign clouds like US Government, China, and Germany have different base URLs. Without this PR, users will be unable to helm install flyte when using Azure Storage in sovereign clouds. Container creation fails with a 403 forbidden error.

@gvashishtha gvashishtha changed the title adding sovereign cloud support Adding Azure sovereign cloud support Aug 1, 2023
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <gvashishtha@anduril.com>
Signed-off-by: Gopal K. Vashishtha <15335863+gvashishtha@users.noreply.github.com>
@kumare3
Copy link

kumare3 commented Aug 2, 2023

I do not think a blocker but can we add test? So that these new confit values do not get accidentally deleted

https://github.com/flyteorg/stow/blob/master/azure/stow_test.go


stow.Dial requires both a string value of the particular Stow Location Kind ("azure") and a stow.Config instance. The stow.Config instance requires two entries with the specific key value attributes:

- a key of azure.ConfigAccount with a value of the Azure Resource Name
- a key of azure.ConfigKey with a value of the Azure Access Key
- an optional key of azure.ConfigBaseUrl to specify which base URL you would like to use. Defaults to core.windows.net (public Azure)
- an optional key of azure.ConfigAPIVersion to specify which Storage API version you would like to use. Defaults to 2018-03-28
Copy link

Choose a reason for hiding this comment

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

Thank you

@gvashishtha
Copy link
Author

@kumare3 happy to add the test, but I may need a bit more context because I wasn't sure what the test was actually doing. I see that it reads some connection parameters from environment variables. Do you have some kind of mock Azure account that it is connecting to? When does the test run? It doesn't look like you have continuous integration set up for this repository.

@kumare3
Copy link

kumare3 commented Aug 2, 2023

@kumare3 happy to add the test, but I may need a bit more context because I wasn't sure what the test was actually doing. I see that it reads some connection parameters from environment variables. Do you have some kind of mock Azure account that it is connecting to? When does the test run? It doesn't look like you have continuous integration set up for this repository.

Tbh we do not run tests against azure today. There are community folks using azure, but I do not have visibility. I also agree I dont understand how it knocks network. Let me +1

@kumare3 kumare3 merged commit 6bd7f9a into flyteorg:master Aug 2, 2023
2 checks passed
@davidmirror-ops
Copy link

Thank you @gvashishtha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants