support for other Azure cloud environments #3634

staebler · 2020-05-20T19:00:45Z

Add the azure.cloudName field to the installconfig. This field directs which Azure cloud environment is used for the cluster.

The status.platformStatus.azure.cloudName field is set in the infrastructure.config.openshift.io resource to match the cloud name configured in the installconfig.

The cloud field is set in the cloud-provider-config ConfigMap to match the cloud name configured in the installconfig.

Use the cloud name specified in the installconfig to have the installer, destroyer, and terraform connect to the correct Azure cloud environment.

See openshift/enhancements#321

https://issues.redhat.com/browse/CORS-1442

staebler · 2020-05-20T19:02:33Z

/hold

This is currently vendoring an openshift/api fork until openshift/api#650 merges.

staebler · 2020-05-20T21:32:53Z

/test e2e-azure

patrickdillon

I had a chance to read through this and it looks really good. Left a few suggestions in case they are helpful.

patrickdillon · 2020-05-21T19:13:32Z

pkg/asset/installconfig/azure/azure.go

@@ -105,8 +108,8 @@ func getRegions() (map[string]string, error) {
 	return allLocations, nil
 }

-func getResourceCapableRegions() ([]string, error) {
-	client, err := NewClient(context.TODO())
+func getResourceCapableRegions(cloudName azure.CloudEnvironment) ([]string, error) {


It might make more sense for these (getRegions and getResourceCapableRegions) to live in the azure client API. In general I think anything that uses a client should follow this pattern, but I'm not very familiar with the Azure code.

Moving it to the API interface would allow mocking and testing these functions. You didn't create these functions so just a suggestion.

I do not agree that the functions make more sense as part of the client API. The functions rely on making calls to the client API and formatting the results in ways specific to what is needed by the Azure Platform. I do, however, take your point that it would be good to be able to mock and test the functions. So, in that vein, I will change the functions to take a client as a parameter rather than creating a client inside the function.

pkg/types/azure/platform.go

patrickdillon · 2020-05-21T19:17:11Z

pkg/asset/manifests/azure/cloudproviderconfig.go

@@ -23,7 +26,7 @@ func (params CloudProviderConfig) JSON() (string, error) {
 	resourceGroupName := params.ResourcePrefix + "-rg"
 	config := config{
 		authConfig: authConfig{
-			Cloud:                       "AzurePublicCloud",
+			Cloud:                       string(params.CloudName),


Hm I dislike casting or type conversion in general but I don't think there's anyway around this.

I could add a function like the following.

// Environment returns name that Azure uses for the cloud environment. // See https://github.com/Azure/go-autorest/blob/ec5f4903f77ed9927ac95b19ab8e44ada64c1356/autorest/azure/environments.go#L13 func (e CloudEnvironment) Name() string { return string(e) }

That would push the casting to a location closer to where the enum is defined. I don't know of any precedence for this approach, though.

Suggested change

Cloud: string(params.CloudName),

Cloud: params.CloudName.Name(),

staebler · 2020-06-02T15:05:10Z

/hold

This is currently vendoring an openshift/api fork until openshift/api#650 merges.

/hold cancel

The PR has been updated to vendor the latest from openshift/api.

The initial code review feedback has been addressed.

/cc @jhixson74

jhixson74 · 2020-06-02T20:11:26Z

/approve
/lgtm

jhixson74 · 2020-06-02T20:11:38Z

/test e2e-azure

pkg/asset/cluster/tfvars.go

abhinavdahiya · 2020-06-03T15:23:28Z

/test e2e-azure

Add the `azure.cloudName` field to the installconfig. This field directs which Azure cloud environment is used for the cluster. The `status.platformStatus.azure.cloudName` field is set in the infrastructure.config.openshift.io resource to match the cloud name configured in the installconfig. The `cloud` field is set in the cloud-provider-config ConfigMap to match the cloud name configured in the installconfig. See openshift/enhancements#321 https://issues.redhat.com/browse/CORS-1442

staebler · 2020-06-03T16:01:24Z

020e32c...0af108c

Moved code for determining the Azure environment for terraform into pkg/tfvars/azure.
Squashed commits.

Use the cloud name specified in the installconfig to have the installer, destroyer, and terraform connect to the correct Azure cloud environment. https://issues.redhat.com/browse/CORS-1442

Bump to the latest github.com/openshift/api version in order to pick up changes to the infrastructure.config.openshift.io type for supporting other Azure cloud environments. Changes from openshift/api#650. https://issues.redhat.com/browse/CORS-1442

staebler · 2020-06-03T16:48:38Z

/test e2e-azure

abhinavdahiya · 2020-06-04T14:23:23Z

/approve

openshift-ci-robot · 2020-06-04T14:23:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jhixson74

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jhixson74 · 2020-06-08T21:38:45Z

/lgtm

openshift-bot · 2020-06-08T22:51:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

When support for other Azure cloud environments was added [1], there were some necessary changes that were omitted. * Azure clients that are created need to use the base URI appropriate for the cloud environment. * Azure authorizers need to use the ActiveDirectory endpoint appropriate for the cloud environment. * The environment needs to be specified for the terraform azureprivatedns provider. [1] openshift#3634

openshift-ci-robot requested review from jcpowermac and patrickdillon May 20, 2020 19:01

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020

staebler force-pushed the azure_other_cloud_env branch 3 times, most recently from f5e3051 to 790b4cb Compare May 20, 2020 19:39

patrickdillon reviewed May 21, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2020

staebler force-pushed the azure_other_cloud_env branch from 790b4cb to 8b10d04 Compare June 2, 2020 15:02

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2020

openshift-ci-robot requested a review from jhixson74 June 2, 2020 15:05

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2020

openshift-ci-robot assigned jhixson74 Jun 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2020

jhixson74 approved these changes Jun 2, 2020

View reviewed changes

abhinavdahiya reviewed Jun 3, 2020

View reviewed changes

pkg/asset/cluster/tfvars.go Outdated Show resolved Hide resolved

staebler force-pushed the azure_other_cloud_env branch from 020e32c to 0af108c Compare June 3, 2020 16:00

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2020

staebler added 2 commits June 3, 2020 12:47

support for installer connecting to other Azure cloud environments

8f798c8

Use the cloud name specified in the installconfig to have the installer, destroyer, and terraform connect to the correct Azure cloud environment. https://issues.redhat.com/browse/CORS-1442

staebler force-pushed the azure_other_cloud_env branch from 0af108c to 833be8c Compare June 3, 2020 16:48

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

openshift-merge-robot merged commit e513bbb into openshift:master Jun 8, 2020

abhinavdahiya mentioned this pull request Jun 9, 2020

azure: add OutboundType for controlling egress #3324

Merged

abhinavdahiya mentioned this pull request Jul 17, 2020

WiP: Support custom Azure environment installation #3529

Closed

staebler mentioned this pull request Jul 22, 2020

use correct base URI for other azure cloud environments #3946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for other Azure cloud environments #3634

support for other Azure cloud environments #3634

staebler commented May 20, 2020

staebler commented May 20, 2020

staebler commented May 20, 2020

patrickdillon left a comment

patrickdillon May 21, 2020

staebler Jun 2, 2020

patrickdillon May 21, 2020

staebler Jun 2, 2020

staebler commented Jun 2, 2020 •

edited

jhixson74 commented Jun 2, 2020

jhixson74 commented Jun 2, 2020

abhinavdahiya commented Jun 3, 2020

staebler commented Jun 3, 2020

staebler commented Jun 3, 2020

abhinavdahiya commented Jun 4, 2020

openshift-ci-robot commented Jun 4, 2020

jhixson74 commented Jun 8, 2020

openshift-bot commented Jun 8, 2020

	Cloud: string(params.CloudName),
	Cloud: params.CloudName.Name(),

support for other Azure cloud environments #3634

support for other Azure cloud environments #3634

Conversation

staebler commented May 20, 2020

staebler commented May 20, 2020

staebler commented May 20, 2020

patrickdillon left a comment

Choose a reason for hiding this comment

patrickdillon May 21, 2020

Choose a reason for hiding this comment

staebler Jun 2, 2020

Choose a reason for hiding this comment

patrickdillon May 21, 2020

Choose a reason for hiding this comment

staebler Jun 2, 2020

Choose a reason for hiding this comment

staebler commented Jun 2, 2020 • edited

jhixson74 commented Jun 2, 2020

jhixson74 commented Jun 2, 2020

abhinavdahiya commented Jun 3, 2020

staebler commented Jun 3, 2020

staebler commented Jun 3, 2020

abhinavdahiya commented Jun 4, 2020

openshift-ci-robot commented Jun 4, 2020

jhixson74 commented Jun 8, 2020

openshift-bot commented Jun 8, 2020

staebler commented Jun 2, 2020 •

edited