fix(azure): sovereign cloud support

[jbpaux]

Description

The old implementation allowed use of alternative Azure Clouds (Azure China, Azure Gov etc.)
With the changes to the new SDK, the other clouds were partially implemented : the authentication to the other clouds was working but the calls to the management endpoints to create/delete records was only targeting public Azure Cloud.
This PR aims to fix this bug.
To test it, an access to an sovereign cloud is required. The requestor who filled the issue has access to one and validated the fix.

Fixes #3927

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Hi @jbpaux. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[johngmyers]

Would it make sense to extract the construction of the arm.ClientOptions into a method?
/ok-to-test

[miwithro]

@johngmyers @njuettner can we get this over the finish line. This is blocking some gov customers.

[mloiseleur]

/lgtm

[mloiseleur]

/retitle fix(azure): sovereign cloud support

[jbpaux]

Would it make sense to extract the construction of the arm.ClientOptions into a method?

It's doable. Maybe the getCloud method can be refactored as getClientOptions but I don't see a lot of added value.

[johngmyers]

The structure of the initialization is weird and complicated. Both providers call getCredentials() which itself calls getCloudConfiguration(). Then in this PR they copy-paste identical code to call getCloudConfiguration() a second time and construct a ClientOptions.

Would it not make sense to have getCredentials() also return a ClientOptions?

[jbpaux]

I agree for the first part, the getCloudConfiguration should be call once, we define the environment once and we shouldn't have to call it again but I don't think getCredentials should also take care of creation of the ClientOptions as it's a bit out of its responsibility.

What do you think ?

[johngmyers]

I think it's okay to expand the responsibility of getCredentials

[jhongturney]

Will this be going into a release soon? We were going from 0.13.4 to 0.13.6 to address a vulnerability, but the issue with sovereign clouds is preventing us.

[jbpaux]

I think it's okay to expand the responsibility of getCredentials

ok, done in latest commit

Will this be going into a release soon? We were going from 0.13.4 to 0.13.6 to address a vulnerability, but the issue with sovereign clouds is preventing us.

it must be merged first in master then in a few month a new release will be out. So in the meantime you'll have to either use master image or a custom made image with this fix included

[mloiseleur]

I'm unsure why you didn't use the client factory, but the code looks good.
/lgtm

[Raffo]

Hey @jbpaux , can you address @mloiseleur's comment. The implementation looks good enough to me, but I'm not too familiar with azure sdk, so I'd love to get that comment answered.

@jhongturney this is on my radar, so it will be merged soon. that said, it's right that there won't be a new released right away. You can consider using an image from the master branch if you need it sooner than released.

[jbpaux]

I didn't use the client factory as the initial implementation didn't use it either. I didn't tought it was worth switching to it for only 2 clients. If it's really wanted I can rerefactor to use it but I think it's good enough :)

[Raffo]

@mloiseleur given that you mention that, what do you think? For me it's okay like this.

[mloiseleur]

Yes, it's okay for me like this too.

[johngmyers]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 [johngmyers]

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

[nbjohnson]

Great to see this fix got merged. When can we expect to see a tagged release version with this included? This fixed a bug that is currently completely breaking externaldns for us

[Raffo]

@nbjohnson we still have to plan a release. I think towards the end of October is realistic. In the meantime you can use one of the images built from master.

[nbjohnson]

@Raffo Just want to check again now that we have reached November what the status of a new tagged release with this fix. We need to deploy with a tagged release with this critical fix. We look forward for a release, Thanks!

[Raffo]

We talked about it and it's planned, I need to get the time to do it, likely next week.