Improved and refactored support for multiple Load Balancers (of #111 …

…and #328, in preparation for #644)

Note: There was already a high degree of symmetry (of both functionality and implementation) between the CPI's support for LBs and AGWs. However, previous CPI enhancements (for example: #541 for #111 and #638 for #328) had reduced this symmetry to some degree, and also caused some divergence between the names of some methods and variables (vs their implementations/values), which would make it harder to both implement and review the new functionality for #644, and also harder to maintain the symmetry between the LB and AGW functionality.

These changes include:

- refactoring of existing code/implementation. E.g. Renamed some vars and methods to more accurately reflect their current values/behavior.
- minor configuration API enhancements. E.g. Allow the `load_balancer` config to be configured as an Array, instead of the (previously-undocumented, but already-supported) comma-delimited string of LB names, to support multiple LB configs which differ by more than just the LB name (which now enables the functionality of #111 and #328 to be used more independently within a single config).
- adding multiple new/missing unit tests, which cover existing functionality (previously added by #541 for #111 and #638 for #328) which was previously not covered/tested by the unit test suite.

---

COMMIT HISTORY:

Modified the `VMCloudProps._parse_load_balancer_config` method to allow the `load_balancer` config to be configured as an Array.
Note: When specifying an Array of LBs, each element of the Array should be a String and/or Hash of the same format which was previously (and still is) valid when configuring a single LB as a non-Array.

Refactoring: Minor refactoring in the `VMManager._get_load_balancers` method.

Added additional data type validation to the `VMCloudProps._parse_load_balancer_config` method.

Modified the `VMCloudProps._parse_load_balancer_config` method to return `nil` when the `load_balancer` config is omitted/missing.

Added additional specs for the `VMCloudProps._parse_load_balancer_config` method.

Refactoring: Renamed the `VMCloudProps.load_balancer` attribute.

Refactoring of the `VMCloudProps._parse_load_balancer_config` and `VMManager._get_load_balancers` methods' implementations.

Refactoring: Cleanup of the `VMCloudProps._parse_load_balancer_config` method's implementation.

Refactoring: Minor cleanup of the `VMManager._get_load_balancers` method's implementation.

Renamed a key within the Hash returned by the `AzureClient.parse_network_interface` method.

Fixed the `AzureClient.parse_network_interface` method to correctly return all of the NIC's load_balancers' info (instead of only the first LB's info).

Refactoring: Minor refactoring in the `AzureClient.create_network_interface` method.

Refactoring: Renamed some vars in the `AzureClient.create_network_interface` method.

Refactoring: Renamed a key within the `nic_params` Hash (from "load_balancer" to "load_balancers"), and fixed inaccurate doc comments for that key.

Fixed a comment typo.

Refactoring: Renamed a method and some vars: vm_manager_network.rb.

src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb

@@ -9,7 +9,7 @@ class VMCloudProps
     attr_reader :root_disk, :ephemeral_disk, :caching
     attr_reader :availability_zone
     attr_reader :availability_set
-    attr_reader :load_balancer
+    attr_reader :load_balancers
     attr_reader :application_gateway
     attr_reader :managed_identity
     attr_reader :security_group
@@ -56,7 +56,7 @@ def initialize(vm_properties, global_azure_config)
       @availability_set = _parse_availability_set_config(vm_properties, global_azure_config)
       cloud_error("Only one of 'availability_zone' and 'availability_set' is allowed to be configured for the VM but you have configured both.") if !@availability_zone.nil? && !@availability_set.name.nil?
 
-      @load_balancer = _parse_load_balancer_config(vm_properties, global_azure_config)
+      @load_balancers = _parse_load_balancer_config(vm_properties, global_azure_config)
       @application_gateway = vm_properties['application_gateway']
 
       @managed_identity = global_azure_config.default_managed_identity
@@ -99,21 +99,34 @@ def _default_fault_domain_count(global_azure_config)
       end
     end
 
+    # @return [Array<Bosh::AzureCloud::LoadBalancerConfig>,nil]
     def _parse_load_balancer_config(vm_properties, global_azure_config)
-      if vm_properties[LOAD_BALANCER_KEY].is_a?(Hash)
-        resource_group_name = vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY] || global_azure_config.resource_group_name
-        Bosh::AzureCloud::LoadBalancerConfig.new(
-          resource_group_name,
-          vm_properties[LOAD_BALANCER_KEY][NAME_KEY]
-        )
-      else
-        Bosh::AzureCloud::LoadBalancerConfig.new(
-          global_azure_config.resource_group_name,
-          vm_properties[LOAD_BALANCER_KEY]
-        )
+      load_balancer_config = vm_properties[LOAD_BALANCER_KEY]
+
+      return nil unless load_balancer_config
+
+      cloud_error("Property '#{LOAD_BALANCER_KEY}' must be a String, Hash, or Array.") unless load_balancer_config.is_a?(String) || load_balancer_config.is_a?(Hash) || load_balancer_config.is_a?(Array)
+
+      load_balancer_configs = load_balancer_config.is_a?(Array) ? load_balancer_config : [load_balancer_config]
+      load_balancers = Array(load_balancer_configs).flat_map do |lbc|
+        if lbc.is_a?(Hash)
+          load_balancer_names = lbc[NAME_KEY]
+          resource_group_name = lbc[RESOURCE_GROUP_NAME_KEY]
+        else
+          load_balancer_names = lbc
+          resource_group_name = nil
+        end
+        String(load_balancer_names).split(',').map do |load_balancer_name|
+          Bosh::AzureCloud::LoadBalancerConfig.new(
+            resource_group_name || global_azure_config.resource_group_name,
+            load_balancer_name
+          )
+        end
       end
+      load_balancers.compact
     end
 
+    # @return [Bosh::AzureCloud::AvailabilitySetConfig]
     def _parse_availability_set_config(vm_properties, global_azure_config)
       if vm_properties[AVAILABILITY_SET_KEY].is_a?(Hash)
         platform_update_domain_count = vm_properties[AVAILABILITY_SET_KEY]['platform_update_domain_count'] || _default_update_domain_count(global_azure_config)

src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb

@@ -1417,7 +1417,7 @@ def _get_load_balancer(url)
     # * +:dns_servers                   - Array. DNS servers.
     # * +:network_security_group        - Hash. The network security group which the network interface is bound to.
     # * +:application_security_groups   - Array. The application security groups which the network interface is bound to.
-    # * +:load_balancer                 - Hash. The load balancer which the network interface is bound to.
+    # * +:load_balancers                - Array<Hash>. The load balancers which the network interface is bound to.
     # * +:application_gateway           - Hash. The application gateway which the network interface is bound to.
     #
     # @return [Boolean]
@@ -1461,13 +1461,13 @@ def create_network_interface(resource_group_name, nic_params)
       end
       interface['properties']['ipConfigurations'][0]['properties']['applicationSecurityGroups'] = application_security_groups unless application_security_groups.empty?
 
-      load_balancer = nic_params[:load_balancer]
-      unless load_balancer.nil?
-        backend_pools = load_balancer.collect { |single_load_balancer| {:id => single_load_balancer[:backend_address_pools][0][:id]} }
+      load_balancers = nic_params[:load_balancers]
+      unless load_balancers.nil?
+        backend_pools = load_balancers.map { |load_balancer| {:id => load_balancer[:backend_address_pools][0][:id]} }
         inbound_nat_rules = Array.new
-        load_balancer.each do |single_load_balancer|
-          unless single_load_balancer[:frontend_ip_configurations][0][:inbound_nat_rules].nil?
-            inbound_nat_rules += single_load_balancer[:frontend_ip_configurations][0][:inbound_nat_rules]
+        load_balancers.each do |load_balancer|
+          unless load_balancer[:frontend_ip_configurations][0][:inbound_nat_rules].nil?
+            inbound_nat_rules += load_balancer[:frontend_ip_configurations][0][:inbound_nat_rules]
           end
         end
         interface['properties']['ipConfigurations'][0]['properties']['loadBalancerBackendAddressPools'] = backend_pools
@@ -2051,13 +2051,18 @@ def parse_network_interface(result, recursive: true)
                                     { id: ip_configuration_properties['publicIPAddress']['id'] }
                                   end
         end
-        unless ip_configuration_properties['loadBalancerBackendAddressPools'].nil?
-          if recursive
-            names = _parse_name_from_id(ip_configuration_properties['loadBalancerBackendAddressPools'][0]['id'])
-            interface[:load_balancer] = get_load_balancer_by_name(names[:resource_group_name], names[:resource_name])
-          else
-            interface[:load_balancer] = { id: ip_configuration_properties['loadBalancerBackendAddressPools'][0]['id'] }
+        load_balancer_backend_pools = ip_configuration_properties['loadBalancerBackendAddressPools']
+        unless load_balancer_backend_pools.nil?
+          load_balancers = load_balancer_backend_pools.map do |lb_backend_pool|
+            if recursive
+              names = _parse_name_from_id(lb_backend_pool['id'])
+              load_balancer = get_load_balancer_by_name(names[:resource_group_name], names[:resource_name])
+            else
+              load_balancer = { id: lb_backend_pool['id'] }
+            end
+            load_balancer
           end
+          interface[:load_balancers] = load_balancers
         end
         unless ip_configuration_properties['applicationGatewayBackendAddressPools'].nil?
           if recursive

src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb

@@ -88,18 +88,18 @@ def _get_public_ip(vip_network)
       public_ip
     end
 
-    def _get_load_balancer(vm_props)
-      load_balancer = nil
-      unless vm_props.load_balancer.name.nil?
-        load_balancer_name_split = vm_props.load_balancer.name.split(',')        
-        load_balancer = Array.new
-        load_balancer_name_split.each do |load_balancer_name|
-          single_load_balancer = @azure_client.get_load_balancer_by_name(vm_props.load_balancer.resource_group_name, load_balancer_name)
-          cloud_error("Cannot find the load balancer '#{load_balancer_name}'") if single_load_balancer.nil?
-          load_balancer.push(single_load_balancer)
+    # @return [Array<Hash>]
+    def _get_load_balancers(vm_props)
+      load_balancers = nil
+      load_balancer_configs = vm_props.load_balancers
+      unless load_balancer_configs.nil?
+        load_balancers = load_balancer_configs.map do |load_balancer_config|
+          load_balancer = @azure_client.get_load_balancer_by_name(load_balancer_config.resource_group_name, load_balancer_config.name)
+          cloud_error("Cannot find the load balancer '#{load_balancer_config.name}'") if load_balancer.nil?
+          load_balancer
         end
       end
-      load_balancer
+      load_balancers
     end
 
     def _get_application_gateway(vm_props)
@@ -133,8 +133,8 @@ def _get_or_create_public_ip(resource_group_name, vm_name, location, vm_props, n
 
     def _create_network_interfaces(resource_group_name, vm_name, location, vm_props, network_configurator, primary_nic_tags = AZURE_TAGS)
       # Tasks to prepare before creating NICs:
-      #   * preapre public ip
-      #   * prepare load balancer
+      #   * prepare public ip
+      #   * prepare load balancer(s)
       #   * prepare application gateway
       tasks_preparing = []
 
@@ -144,8 +144,8 @@ def _create_network_interfaces(resource_group_name, vm_name, location, vm_props,
         end
       )
       tasks_preparing.push(
-        task_get_load_balancer = Concurrent::Future.execute do
-          _get_load_balancer(vm_props)
+        task_get_load_balancers = Concurrent::Future.execute do
+          _get_load_balancers(vm_props)
         end
       )
       tasks_preparing.push(
@@ -158,7 +158,7 @@ def _create_network_interfaces(resource_group_name, vm_name, location, vm_props,
       tasks_preparing.map(&:wait)
 
       public_ip = task_get_or_create_public_ip.value!
-      load_balancer = task_get_load_balancer.value!
+      load_balancers = task_get_load_balancers.value!
       application_gateway = task_get_application_gateway.value!
 
       # tasks to create NICs, NICs will be created in different threads
@@ -185,12 +185,12 @@ def _create_network_interfaces(resource_group_name, vm_name, location, vm_props,
         if index.zero?
           nic_params[:public_ip] = public_ip
           nic_params[:tags] = primary_nic_tags
-          nic_params[:load_balancer] = load_balancer
+          nic_params[:load_balancers] = load_balancers
           nic_params[:application_gateway] = application_gateway
         else
           nic_params[:public_ip] = nil
           nic_params[:tags] = AZURE_TAGS
-          nic_params[:load_balancer] = nil
+          nic_params[:load_balancers] = nil
           nic_params[:application_gateway] = nil
         end
         tasks_creating.push(

src/bosh_azure_cpi/spec/unit/azure_client/create_network_interface_spec.rb

@@ -55,7 +55,7 @@
             public_ip: { id: 'fake-public-id' },
             network_security_group: { id: nsg_id },
             application_security_groups: [],
-            load_balancer: nil,
+            load_balancers: nil,
             application_gateway: nil
           }
         end
@@ -130,7 +130,7 @@
             enable_accelerated_networking: false,
             network_security_group: { id: nsg_id },
             application_security_groups: [],
-            load_balancer: nil,
+            load_balancers: nil,
             application_gateway: nil
           }
         end
@@ -208,7 +208,7 @@
             public_ip: { id: 'fake-public-id' },
             network_security_group: nil,
             application_security_groups: [],
-            load_balancer: nil,
+            load_balancers: nil,
             application_gateway: nil
           }
         end
@@ -284,7 +284,7 @@
             public_ip: { id: 'fake-public-id' },
             network_security_group: { id: nsg_id },
             application_security_groups: [],
-            load_balancer: [{
+            load_balancers: [{
               backend_address_pools: [
                 {
                   id: 'fake-id'
@@ -380,7 +380,7 @@
             public_ip: { id: 'fake-public-id' },
             network_security_group: { id: nsg_id },
             application_security_groups: [{ id: 'fake-asg-id-1' }, { id: 'fake-asg-id-2' }],
-            load_balancer: nil,
+            load_balancers: nil,
             application_gateway: nil
           }
         end
@@ -466,7 +466,7 @@
             public_ip: { id: 'fake-public-id' },
             network_security_group: { id: nsg_id },
             application_security_groups: [],
-            load_balancer: nil,
+            load_balancers: nil,
             application_gateway: {
               backend_address_pools: [
                 {

src/bosh_azure_cpi/spec/unit/azure_client/get_operation_spec.rb

@@ -816,7 +816,7 @@
             ip_configuration_id: 'fake-id',
             private_ip: '10.0.0.100',
             private_ip_allocation_method: 'Dynamic',
-            load_balancer: fake_load_balancer
+            load_balancers: [fake_load_balancer]
           }
         end
         it 'should return the network interface with load balancer' do

src/bosh_azure_cpi/spec/unit/azure_client/list_network_interfaces_by_keyword_spec.rb

@@ -167,7 +167,7 @@
           private_ip_allocation_method: 'f0',
           network_security_group: { id: 'i' },
           public_ip: { id: 'j' },
-          load_balancer: { id: 'k' },
+          load_balancers: [{ id: 'k' }],
           application_gateway: { id: 'l' },
           application_security_groups: [{ id: 'asg-id-1' }]
         }