IR-207: Get endpoints for Azure Stack Cloud #710

dmage · 2021-08-19T09:42:44Z

When the registry operator is installed on Azure Stack Hub, it should be able to use its endpoints.

As Azure Stack Hub does not support the latest storage API, we should downgrade github.com/Azure/azure-storage-blob-go (which uses 2018-11-09) and use Microsoft.Storage 2019-06-01.

dmage · 2021-08-19T11:12:45Z

pkg/storage/azure/azure.go

+	kind := storage.StorageV2
+	params := &storage.AccountPropertiesCreateParameters{
+		AllowBlobPublicAccess: to.BoolPtr(false),
+		MinimumTLSVersion:     storage.TLS12,
+	}
+
+	if strings.EqualFold(cloudName, "AZURESTACKCLOUD") {
+		// It seems Azure Stack Hub does not support new API.
+		kind = storage.Storage
+		params = &storage.AccountPropertiesCreateParameters{}
+	}


@patrickdillon does it make sense?

Well, I'm not sure. It seems like ASH supports 2019-06-01, and therefore should support AllowBlobPublicAccess and MinimumTLSVersion. (I don't know about storage.StorageV2 vs storage.Storage. )

Are you basing the removal of these two fields on the fact that the hybrid profile does not have these fields or have you tested and get an error when those fields are included? I would expect that if this code works, you should be able to include those two fields with ASH.

Hopefully this comment is not too unclear.

I tried and it didn't work. It returned

storage.AccountsClient#Create: Failure sending request: StatusCode=0 -- Original Error: Code="InvalidParameter" Message="Parameters are not valid in the request" Target="" Details=[{"code":"AccountPropertyCannotBeUpdated","message":"Property storageAccount.properties.allowBlobPublicAccess that cannot be updated for the storage account was specified in the request.","target":"storageAccount.properties.allowBlobPublicAccess"},{"code":"AccountPropertyCannotBeUpdated","message":"Property storageAccount.properties.minimumTlsVersion that cannot be updated for the storage account was specified in the request.","target":"storageAccount.properties.minimumTlsVersion"}]

Ok. I think this explains why the hybrid profile uses 2017-10-01, which doesn't have these fields.

Yeah, with your explanation, this code makes sense to me.

dmage · 2021-08-19T11:13:51Z

test platfrom flake
/retest

dmage · 2021-08-19T13:47:34Z

/retest

patrickdillon · 2021-08-19T14:03:10Z

@jsafrane could you check this to see if this would be affected by openshift/installer#5138 when we remove credentials from the cloud config?

patrickdillon · 2021-08-19T14:05:44Z

pkg/storage/azure/azure.go

@@ -292,7 +301,7 @@ func (d *driver) storageAccountsClient(cfg *Azure, environment autorestazure.Env
 		storageAccountsClient.Authorizer = d.authorizer
 	} else {
 		clientCredentialsConfig := auth.NewClientCredentialsConfig(cfg.ClientID, cfg.ClientSecret, cfg.TenantID)


@jsafrane to my untrained eye it looks like this is reading the creds from the config which would be a problem with openshift/installer#5138. can you confirm?

these creds are from cloud-credential-operator, the operator requests them via https://github.com/openshift/cluster-image-registry-operator/blob/master/manifests/01-registry-credentials-request-azure.yaml

Perfect. Thank you.

dmage · 2021-08-19T17:07:05Z

/retest

dmage · 2021-08-20T15:37:00Z

/assign @ricardomaraschini @xiuwang @bmcelvee @sferich888

Please review this PR and tag appropriately, we really want to have it in 4.9.

bmcelvee · 2021-08-20T19:56:22Z

/label docs-approved

xiuwang · 2021-08-23T03:47:45Z

/label qe-approved

Test this pr an ASH, no issue found.

ricardomaraschini · 2021-08-23T13:19:34Z

manifests/07-operator-ibm-cloud-managed.yaml

@@ -37,6 +37,8 @@ spec:
          value: docker.io/openshift/origin-docker-registry:latest
        - name: IMAGE_PRUNER
          value: quay.io/openshift/origin-cli:v4.0
+        - name: AZURE_ENVIRONMENT_FILEPATH
+          value: /tmp/azurestackcloud.json


Is this some kinda default location for this? I could not see where this file is used, IIUC the endpoints are copied into it but I could not find where it is used.

azure sdk uses AZURE_ENVIRONMENT_FILEPATH to find its config

Oh yes, of course. Shame on me.

ricardomaraschini

/lgtm

openshift-ci · 2021-08-23T13:33:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, ricardomaraschini

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~~ [dmage,ricardomaraschini]

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

openshift-bot · 2021-08-23T20:49:47Z

/retest-required