search

CloudVE / cloudbridge

A consistent interface to multiple IaaS clouds; in Python.
https://cloudbridge.cloudve.org
MIT License
113 stars 51 forks source link

Azure - add AZURE_NETWORK_RESOURCE_GROUP to pick vnet from another ResourceGroup #321

Closed patchkez closed 4 months ago

patchkez commented 5 months ago

This PR adds new option for defining extra ResourceGroup from which vnet can be picked.

Closes #320

nuwang commented 5 months ago

@patchkez Thanks for this PR. The maintainers will take it up for discussion on Tuesday and will be able to provide some feedback then. An immediate comment which sprang to mind is whether the partitioning of resource groups is somewhat subjective, or whether having a separate network resource group is standard practice. In the former case, we should perhaps look into making this more generic.

Please also look into the linting failures in the PR.

patchkez commented 5 months ago

Lint error has been fixed. I think such scenario is common for enterprise/bigger companies, where every team want's to have it's resources in own resource group. Such segregation allows automatically apply/inherit tags to resources and use tags to manage and allocate costs across the different groups. https://learn.microsoft.com/en-us/azure/cost-management-billing/costs/enable-tag-inheritance

Then when such Azure cloud account is connected to internal company network, the configuration (peer links, virtual network, net security group, address space) can be deployed in separate resource group and access can be given to other resource groups to obtain IPs from pool of internal network.

almahmoud commented 5 months ago

Hey @patchkez ! First of all, thank you for the PR! We love to see people using and contributing back to the repo!!

First, a small mostly aesthetic change: could you rename NETWORK to NETWORKING in this PR, to be in line with the naming of the service? I believe this will be better as it will affect networking resources (floating IPs, subnet, etc...) not just networks/VPCs.

Beyond that, we discussed this change today, and given that it is Azure-specific and backwards compatible, we believe it is okay to be done for networking only, but have a suggestion for a possible way to make it better long-term.

The idea is to allow for specifying various resource groups per service type, rather than a specific value only for networking service. A similar implementation was done for Zones in Openstack, where the need arose for one to be able to specify different zone name per service (eg: different compute vs networking zone name). You can find this implementation here: https://github.com/CloudVE/cloudbridge/blob/main/cloudbridge/providers/openstack/provider.py#L308

Concretely, the idea would be to add an optional variable for each type of service (networking, security, compute, storage), so you'd then be able to provide the configuration option or environment variable AZURE_NETWORKING_RESOURCE_GROUP, AZURE_COMPUTE_RESOURCE_GROUP, etc... and default to the general RG if not specified. I believe our zone implementation also allowed for a dictionary (json dict) to be passed in the configuration, in addition to the individual env variables. However, that is purely aesthetic for readability, so even implementing it as separate named config options/variables for all services would be greatly appreciated!

If that makes sense and you're willing to add the more general extension for other services, that'd be awesome, and will likely become a useful feature that other users might benefit from more generally!

We however understand time can be hard to find, so while we would encourage that to be added, we don't want to block this PR until that can be done, since this one can exist in the meantime as it's backwards compatible.

We do want the name change to happen, unless you can argue why it makes more sense to remain NETWORK over NETWORKING, but can go ahead with testing and merging it after that, and you can complement it with another PR if/when you find the time for a more general implementation for all services.

patchkez commented 4 months ago

@almahmoud Thanks for the feedback. I have no problem with renaming NETWORK->NETWORKING. Just pushed new commit with this change. Not sure when I will have more time for this, but I could try to implement similar pattern as you showed in Openstack/provider.py for other Azure services. I also noticed the GH tests are failing, also for other providers, not sure what is wrong there...

almahmoud commented 4 months ago

No worries, the integration tests are likely failing because we don't make tokens available to PR runs. @nuwang could you plz do a final review and merge?