feat: add Azure China/German support #1836
Conversation
andyzhangx
force-pushed
the
azure-china-support
branch
from
868c043 to
76c392b
Compare
|
cc @skriss for code review |
andyzhangx
changed the title
Signed-off-by: andyzhangx <[email protected]>
andyzhangx
force-pushed
the
azure-china-support
branch
from
76c392b to
be93f4d
Compare
andyzhangx
changed the title
There was a problem hiding this comment.
Handful of comments @andyzhangx. The main question to resolve is whether we want to specify this as an env var (i.e. in the secret), or as a config item in the BackupStorageLocation and VolumeSnapshotLocation. Interested in others' thoughts on this.
| } | ||
|
|
||
| func getRequiredValues(getValue func(string) string, keys ...string) (map[string]string, error) { | ||
| missing := []string{} | ||
| results := map[string]string{} | ||
|
|
||
| for _, key := range keys { | ||
| if val := getValue(key); val == "" { | ||
| if val := getValue(key); val == "" && key != cloudNameEnvVar { |
There was a problem hiding this comment.
Let's remove this special case, since the cloud name is not a required value. see next comment.
| // 1. we need AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_SUBSCRIPTION_ID | ||
| envVars, err := getRequiredValues(os.Getenv, tenantIDEnvVar, clientIDEnvVar, clientSecretEnvVar, subscriptionIDEnvVar) | ||
| // 1. we need AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_SUBSCRIPTION_ID, AZURE_CLOUD_NAME | ||
| envVars, err := getRequiredValues(os.Getenv, tenantIDEnvVar, clientIDEnvVar, clientSecretEnvVar, subscriptionIDEnvVar, cloudNameEnvVar) |
There was a problem hiding this comment.
Regardless of whether we go with the env var or the config key, I would have separate code to try to get its value since it's not a required item. See previous comment as well.
| // 1. we need AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_SUBSCRIPTION_ID, AZURE_RESOURCE_GROUP | ||
| envVars, err := getRequiredValues(os.Getenv, tenantIDEnvVar, clientIDEnvVar, clientSecretEnvVar, subscriptionIDEnvVar, resourceGroupEnvVar) | ||
| // 1. we need AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_SUBSCRIPTION_ID, AZURE_RESOURCE_GROUP, AZURE_CLOUD_NAME | ||
| envVars, err := getRequiredValues(os.Getenv, tenantIDEnvVar, clientIDEnvVar, clientSecretEnvVar, subscriptionIDEnvVar, resourceGroupEnvVar, cloudNameEnvVar) |
There was a problem hiding this comment.
similar to for object store, I would fetch the cloud name outside of the getRequiredValues call, since it's not a required value.
|
thanks guys for the review. |
@andyzhangx that'd be great! For now, we can tag you on any PRs that come in for the Azure code. We're also working on separating the in-tree cloud provider plugins out into their own repos, so once we have a Here's our current backlog of Azure-related items: https://github.com/heptio/velero/issues?q=is%3Aopen+is%3Aissue+label%3ACloud%2FAzure |
Sure, I could take the code review for any Azure related PR. |
andyzhangx
changed the title
|
@andyzhangx I'd stil like to move the code to get the AZURE_CLOUD_NAME value out of the "required values" code path since it's not a required value - see three comments from original review. |
|
@andyzhangx just checking in to see if you needed any more input on the requested code changes? |
|
@andyzhangx checking in again to see if you'd be able to make the requested revisions; if not I can take this over and give you credit for commits made. |
hi @skriss I am a little busy these two weeks, it's great if you could take over, thanks. |
|
I've updated this in #1938, closing this one out. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a new env var(
AZURE_CLOUD_NAME) in azure environment config, available values could be:AzurePublicCloud,AzureUSGovernmentCloud,AzureChinaCloud,AzureGermanCloudIf env var(
AZURE_CLOUD_NAME) is not set,AzurePublicCloudwould be the default, so it's compatible.Which issue(s) this PR fixes:
Fixes #287
Special notes for your reviewer:
Release note: