search

gardener / machine-controller-manager-provider-azure

This repository is the out of tree implementation of the machine driver for Azure cloud provider
Apache License 2.0
8 stars 27 forks source link

Make cloud instance that is connected to configurable #148

Closed AndreasBurger closed 5 months ago

AndreasBurger commented 5 months ago

What this PR does / why we need it: With this PR the cloud instance to connect to (e.g. Azure Gov Cloud, Azure Public Cloud) can be configured as part of the provider spec.

Which issue(s) this PR fixes: This PR is a requirement to fulfill https://github.com/gardener/gardener-extension-provider-azure/issues/890

Release note:

The cloud instance to connect to can now be configured via the provider spec
kon-angelo commented 5 months ago

cc: @rishabh-11 @unmarshall

kon-angelo commented 5 months ago

/hold I think @AndreasBurger wanted to make an update right ? or is it ready to review

AndreasBurger commented 5 months ago

I've updated the PR to follow the approach we settled on in the provider-extension, so this should be reviewable

rishabh-11 commented 5 months ago

Can you also help me understand why this is needed? If I am understanding this correctly, this PR will reduce the visibility to a particular cloud and not beyond it. Won't this already be done by the Location field (name of the region) in the providerSpec?

unmarshall commented 5 months ago

Can you please raise an issue for this PR? We follow this for all MCM projects. In the issue please detail out the intent of the change explaining why this change is required. Maybe you have got a requirement from a customer or you have already created an extension issue. If yes the please provide relevant customer context or issue link (if created elsewhere) in the issue in mcm-provider-azure.

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

AndreasBurger commented 5 months ago

I've addressed the comments, mostly by removing the CloudConfiguration (for now, we will most probably need it or something like it at some point in the future). For a little bit more detail behind that decision, see https://github.com/gardener/machine-controller-manager-provider-azure/pull/148#discussion_r1636375473.

I have also created an issue in the provider-azure and referenced it in the PR-description.

AndreasBurger commented 5 months ago

I have reverted the latest change and this PR now only uses CloudConfiguration to determine which instance to connect to. The responsibility of providing the correct CloudConfiguration for non-public clouds will then fall to the provider-extensions for the Gardener usecase.