From 01c16576896b6620cb53ecfa891baa5092dab9df Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Wed, 26 Sep 2018 15:20:20 +0800 Subject: [PATCH 1/5] expose the resource group for load balancer, and wrap config for availability set. --- .../lib/cloud/azure/models/config.rb | 26 +++++ .../lib/cloud/azure/models/vm_cloud_props.rb | 96 ++++++++++++++----- .../lib/cloud/azure/restapi/azure_client.rb | 17 ++-- .../lib/cloud/azure/vms/vm_manager.rb | 2 +- .../azure/vms/vm_manager_availability_set.rb | 6 +- .../lib/cloud/azure/vms/vm_manager_network.rb | 6 +- .../unit/azure_client/azure_client_spec.rb | 10 +- .../unit/azure_client/get_operation_spec.rb | 4 +- .../models/availability_set_config_spec.rb | 19 ++++ .../unit/models/load_balancer_config_spec.rb | 18 ++++ .../spec/unit/models/vm_cloud_props_spec.rb | 75 +++++++++++++++ .../create/availability_set_spec.rb | 32 +++---- .../vm_manager/create/invalid_option_spec.rb | 16 ++-- .../unit/vm_manager/create/shared_stuff.rb | 2 +- .../create/vm_is_not_created_spec.rb | 13 ++- .../spec/unit/vm_manager/delete_spec.rb | 2 +- 16 files changed, 269 insertions(+), 75 deletions(-) create mode 100644 src/bosh_azure_cpi/spec/unit/models/availability_set_config_spec.rb create mode 100644 src/bosh_azure_cpi/spec/unit/models/load_balancer_config_spec.rb create mode 100644 src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb index 7ca795621..81f85c6ed 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb @@ -1,6 +1,32 @@ # frozen_string_literal: true module Bosh::AzureCloud + class LoadBalancerConfig + attr_reader :name, :resource_group_name + def initialize(resource_group_name, name) + @resource_group_name = resource_group_name + @name = name + end + + def to_s + "name: #{@name}, resource_group_name: #{@resource_group_name}" + end + end + + class AvailabilitySetConfig + attr_reader :name + attr_reader :platform_update_domain_count, :platform_fault_domain_count + def initialize(name, platform_update_domain_count, platform_fault_domain_count) + @name = name + @platform_update_domain_count = platform_update_domain_count + @platform_fault_domain_count = platform_fault_domain_count + end + + def to_s + "name: #{@name}, platform_update_domain_count: #{@platform_update_domain_count} platform_fault_domain_count: #{@platform_fault_domain_count}" + end + end + class AzureStackConfig attr_reader :domain, :authentication, :resource, :endpoint_prefix attr_writer :authentication diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb index 2b1fb2ca6..c36d164fc 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb @@ -5,33 +5,23 @@ class VMCloudProps include Helpers attr_reader :resource_group_name, :instance_type attr_reader :storage_account_type, :storage_account_kind, :storage_account_max_disk_number, :storage_account_name - attr_reader :availability_zone, :availability_set + attr_reader :availability_zone + attr_reader :availability_set attr_reader :tags attr_reader :caching attr_reader :root_disk, :ephemeral_disk - attr_reader :ip_forwarding, :accelerated_networking, :assign_dynamic_public_ip, :load_balancer, :application_gateway + attr_reader :ip_forwarding, :accelerated_networking, :assign_dynamic_public_ip, :application_gateway + attr_reader :load_balancer attr_reader :application_security_groups, :security_group - attr_reader :platform_update_domain_count, :platform_fault_domain_count - attr_writer :availability_zone, :availability_set + attr_writer :availability_zone + attr_writer :availability_set attr_writer :assign_dynamic_public_ip - # In AzureStack, availability sets can only be configured with 1 update domain. - # In Azure, the max update domain count of a managed/unmanaged availability set is 5. - def default_update_domain_count(global_azure_config) - global_azure_config.environment == ENVIRONMENT_AZURESTACK ? 1 : 5 - end - # In AzureStack, availability sets can only be configured with 1 fault domain and 1 update domain. - # In Azure, the max fault domain count of an unmanaged availability set is 3; - # the max fault domain count of a managed availability set is 2 in some regions. - # When all regions support 3 fault domains, the default value should be changed to 3. - def default_fault_domain_count(global_azure_config) - if global_azure_config.environment == ENVIRONMENT_AZURESTACK - 1 - else - global_azure_config.use_managed_disks ? 2 : 3 - end - end + AVAILABILITY_SET_KEY = 'availability_set' + LOAD_BALANCER_KEY = 'load_balancer' + RESOURCE_GROUP_NAME_KEY = 'resource_group_name' + NAME_KEY = 'name' def initialize(vm_properties, global_azure_config) @vm_properties = vm_properties.dup @@ -40,7 +30,7 @@ def initialize(vm_properties, global_azure_config) @resource_group_name = vm_properties.fetch('resource_group_name', global_azure_config.resource_group_name) @storage_account_type = vm_properties['storage_account_type'] @availability_zone = vm_properties['availability_zone'] - @availability_set = vm_properties['availability_set'] + @availability_set = _parse_availability_set_config(vm_properties, global_azure_config) @storage_account_name = vm_properties['storage_account_name'] @storage_account_max_disk_number = vm_properties.fetch('storage_account_max_disk_number', 30) @storage_account_kind = vm_properties.fetch('storage_account_kind', STORAGE_ACCOUNT_KIND_GENERAL_PURPOSE_V1) @@ -50,7 +40,9 @@ def initialize(vm_properties, global_azure_config) @ip_forwarding = vm_properties['ip_forwarding'] @accelerated_networking = vm_properties['accelerated_networking'] @assign_dynamic_public_ip = vm_properties['assign_dynamic_public_ip'] - @load_balancer = vm_properties['load_balancer'] + + @load_balancer = _parse_load_balancer_config(vm_properties, global_azure_config) + @application_gateway = vm_properties['application_gateway'] @application_security_groups = vm_properties['application_security_groups'] @@ -64,9 +56,65 @@ def initialize(vm_properties, global_azure_config) ephemeral_disk_hash['size'], ephemeral_disk_hash['type'] ) + end + + private + + # In AzureStack, availability sets can only be configured with 1 update domain. + # In Azure, the max update domain count of a managed/unmanaged availability set is 5. + def _default_update_domain_count(global_azure_config) + global_azure_config.environment == ENVIRONMENT_AZURESTACK ? 1 : 5 + end + + # In AzureStack, availability sets can only be configured with 1 fault domain and 1 update domain. + # In Azure, the max fault domain count of an unmanaged availability set is 3; + # the max fault domain count of a managed availability set is 2 in some regions. + # When all regions support 3 fault domains, the default value should be changed to 3. + def _default_fault_domain_count(global_azure_config) + if global_azure_config.environment == ENVIRONMENT_AZURESTACK + 1 + else + global_azure_config.use_managed_disks ? 2 : 3 + end + end + + def _parse_load_balancer_config(vm_properties, global_azure_config) + if vm_properties[LOAD_BALANCER_KEY].is_a?(Hash) + resource_group_name = if vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY].nil? + global_azure_config.resource_group_name + else + vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY] + end + 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] + ) + end + end - @platform_update_domain_count = vm_properties['platform_update_domain_count'] || default_update_domain_count(global_azure_config) - @platform_fault_domain_count = vm_properties['platform_fault_domain_count'] || default_fault_domain_count(global_azure_config) + 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) + platform_fault_domain_count = vm_properties[AVAILABILITY_SET_KEY]['platform_fault_domain_count'] || _default_fault_domain_count(global_azure_config) + Bosh::AzureCloud::AvailabilitySetConfig.new( + vm_properties[AVAILABILITY_SET_KEY][NAME_KEY], + platform_update_domain_count, + platform_fault_domain_count + ) + else + platform_update_domain_count = vm_properties['platform_update_domain_count'] || _default_update_domain_count(global_azure_config) + platform_fault_domain_count = vm_properties['platform_fault_domain_count'] || _default_fault_domain_count(global_azure_config) + Bosh::AzureCloud::AvailabilitySetConfig.new( + vm_properties[AVAILABILITY_SET_KEY], + platform_update_domain_count, + platform_fault_domain_count + ) + end end end end diff --git a/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb b/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb index 30db70321..5ebd5cabb 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb @@ -120,7 +120,7 @@ def rest_api_url(resource_provider, resource_type, resource_group_name: nil, nam url end - def parse_name_from_id(id) + def _parse_name_from_id(id) ret = id.match('/subscriptions/([^/]*)/resourceGroups/([^/]*)/providers/([^/]*)/([^/]*)/([^/]*)(.*)') raise AzureError, "\"#{id}\" is not a valid URL." if ret.nil? @@ -1263,9 +1263,10 @@ def delete_public_ip(resource_group_name, name) # # @See https://docs.microsoft.com/en-us/rest/api/loadbalancer/get-information-about-a-load-balancer # - def get_load_balancer_by_name(name) - url = rest_api_url(REST_API_PROVIDER_NETWORK, REST_API_LOAD_BALANCERS, name: name) - get_load_balancer(url) + + def get_load_balancer_by_name(resource_group_name, name) + url = rest_api_url(REST_API_PROVIDER_NETWORK, REST_API_LOAD_BALANCERS, resource_group_name: resource_group_name, name: name) + _get_load_balancer(url) end # Get a load balancer's information @@ -1275,7 +1276,7 @@ def get_load_balancer_by_name(name) # # @See https://docs.microsoft.com/en-us/rest/api/loadbalancer/get-information-about-a-load-balancer # - def get_load_balancer(url) + def _get_load_balancer(url) load_balancer = nil result = get_resource_by_id(url) unless result.nil? @@ -1959,15 +1960,15 @@ def parse_network_interface(result, recursive: true) 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_name]) + 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'] } end end unless ip_configuration_properties['applicationGatewayBackendAddressPools'].nil? if recursive - names = parse_name_from_id(ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id']) + names = _parse_name_from_id(ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id']) interface[:application_gateway] = get_application_gateway_by_name(names[:resource_name]) else interface[:application_gateway] = { id: ip_configuration_properties['applicationGatewayBackendAddressPools'][0]['id'] } diff --git a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager.rb b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager.rb index 2ebea1ab8..a2a8f2df6 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager.rb @@ -29,7 +29,7 @@ def create(bosh_vm_meta, location, vm_props, network_configurator, env) vm_name = instance_id.vm_name # When both availability_zone and availability_set are specified, raise an error - cloud_error("Only one of 'availability_zone' and 'availability_set' is allowed to be configured for the VM but you have configured both.") if !vm_props.availability_zone.nil? && !vm_props.availability_set.nil? + cloud_error("Only one of 'availability_zone' and 'availability_set' is allowed to be configured for the VM but you have configured both.") if !vm_props.availability_zone.nil? && !vm_props.availability_set.name.nil? zone = vm_props.availability_zone unless zone.nil? cloud_error("'#{zone}' is not a valid zone. Available zones are: #{AVAILABILITY_ZONES}") unless AVAILABILITY_ZONES.include?(zone.to_s) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_availability_set.rb b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_availability_set.rb index 141cf21af..920badeda 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_availability_set.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_availability_set.rb @@ -5,7 +5,7 @@ class VMManager private def _get_availability_set_name(vm_props, env) - availability_set_name = vm_props.availability_set + availability_set_name = vm_props.availability_set.name if availability_set_name.nil? unless env.nil? || env['bosh'].nil? || env['bosh']['group'].nil? availability_set_name = env['bosh']['group'] @@ -32,8 +32,8 @@ def _get_or_create_availability_set(resource_group_name, availability_set_name, name: availability_set_name, location: location, tags: AZURE_TAGS, - platform_update_domain_count: vm_props.platform_update_domain_count || default_update_domain_count, - platform_fault_domain_count: vm_props.platform_fault_domain_count || default_fault_domain_count, + platform_update_domain_count: vm_props.availability_set.platform_update_domain_count, + platform_fault_domain_count: vm_props.availability_set.platform_fault_domain_count, managed: @use_managed_disks } availability_set = nil diff --git a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb index b189def79..b585a22ff 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb @@ -74,9 +74,9 @@ def _get_public_ip(vip_network) def _get_load_balancer(vm_props) load_balancer = nil - unless vm_props.load_balancer.nil? - load_balancer_name = vm_props.load_balancer - load_balancer = @azure_client.get_load_balancer_by_name(load_balancer_name) + unless vm_props.load_balancer.name.nil? + load_balancer_name = vm_props.load_balancer.name + load_balancer = @azure_client.get_load_balancer_by_name(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) cloud_error("Cannot find the load balancer '#{load_balancer_name}'") if load_balancer.nil? end load_balancer diff --git a/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb b/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb index 5a70464af..626c03b01 100644 --- a/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb @@ -93,12 +93,12 @@ end end - describe '#parse_name_from_id' do + describe '#_parse_name_from_id' do context 'when id is empty' do it 'should raise an error' do id = '' expect do - azure_client.parse_name_from_id(id) + azure_client._parse_name_from_id(id) end.to raise_error /\"#{id}\" is not a valid URL./ end end @@ -107,7 +107,7 @@ id = '/subscriptions/a/resourceGroups/b/providers/c/d' it 'should raise an error' do expect do - azure_client.parse_name_from_id(id) + azure_client._parse_name_from_id(id) end.to raise_error /\"#{id}\" is not a valid URL./ end end @@ -121,7 +121,7 @@ result[:provider_name] = 'c' result[:resource_type] = 'd' result[:resource_name] = 'e' - expect(azure_client.parse_name_from_id(id)).to eq(result) + expect(azure_client._parse_name_from_id(id)).to eq(result) end end @@ -134,7 +134,7 @@ result[:provider_name] = 'c' result[:resource_type] = 'd' result[:resource_name] = 'e' - expect(azure_client.parse_name_from_id(id)).to eq(result) + expect(azure_client._parse_name_from_id(id)).to eq(result) end end end diff --git a/src/bosh_azure_cpi/spec/unit/azure_client/get_operation_spec.rb b/src/bosh_azure_cpi/spec/unit/azure_client/get_operation_spec.rb index abbfc90fa..90648795f 100644 --- a/src/bosh_azure_cpi/spec/unit/azure_client/get_operation_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/azure_client/get_operation_spec.rb @@ -513,7 +513,7 @@ headers: {} ) expect( - azure_client.get_load_balancer_by_name(load_balancer_name) + azure_client.get_load_balancer_by_name(default_resource_group_name, load_balancer_name) ).to be_nil end @@ -529,7 +529,7 @@ headers: {} ) expect( - azure_client.get_load_balancer_by_name(load_balancer_name) + azure_client.get_load_balancer_by_name(default_resource_group_name, load_balancer_name) ).to eq(fake_load_balancer) end end diff --git a/src/bosh_azure_cpi/spec/unit/models/availability_set_config_spec.rb b/src/bosh_azure_cpi/spec/unit/models/availability_set_config_spec.rb new file mode 100644 index 000000000..7c8e0f4e9 --- /dev/null +++ b/src/bosh_azure_cpi/spec/unit/models/availability_set_config_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Bosh::AzureCloud::AvailabilitySetConfig do + describe '#to_s' do + context 'when called' do + let(:name) { 'fake_av_set' } + let(:fault_domain) { 5 } + let(:update_domain) { 1 } + let(:availability_set_config) { Bosh::AzureCloud::AvailabilitySetConfig.new(name, update_domain, fault_domain) } + let(:av_set_string) { "name: #{name}, platform_update_domain_count: #{update_domain} platform_fault_domain_count: #{fault_domain}" } + + it 'should return the correct string' do + expect(availability_set_config.to_s).to eq(av_set_string) + end + end + end +end diff --git a/src/bosh_azure_cpi/spec/unit/models/load_balancer_config_spec.rb b/src/bosh_azure_cpi/spec/unit/models/load_balancer_config_spec.rb new file mode 100644 index 000000000..268193d03 --- /dev/null +++ b/src/bosh_azure_cpi/spec/unit/models/load_balancer_config_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Bosh::AzureCloud::LoadBalancerConfig do + describe '#to_s' do + context 'when called' do + let(:name) { 'fake_name' } + let(:resource_group_name) { 'fake_rg' } + let(:lb_string) { "name: #{name}, resource_group_name: #{resource_group_name}" } + let(:load_balancer_config) { Bosh::AzureCloud::LoadBalancerConfig.new(resource_group_name, name) } + + it 'should return the correct string' do + expect(load_balancer_config.to_s).to eq(lb_string) + end + end + end +end diff --git a/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb b/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb new file mode 100644 index 000000000..489c9588c --- /dev/null +++ b/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'unit/cloud/shared_stuff.rb' + +describe Bosh::AzureCloud::VMCloudProps do + include_context 'shared stuff' + describe '#initialize' do + context 'when availability_set is a hash' do + let(:av_set_name) { 'fake_av_set_name' } + let(:platform_update_domain_count) { 5 } + let(:platform_fault_domain_count) { 1 } + let(:vm_cloud_props) do + Bosh::AzureCloud::VMCloudProps.new( + { + 'instance_type' => 'Standard_D1', + 'availability_set' => { + 'name' => av_set_name, + 'platform_update_domain_count' => platform_update_domain_count, + 'platform_fault_domain_count' => platform_fault_domain_count + }, + 'platform_update_domain_count' => 4, + 'platform_fault_domain_count' => 1 + }, azure_config_managed + ) + end + + it 'should return the correct config' do + expect(vm_cloud_props.availability_set.name).to eq(av_set_name) + expect(vm_cloud_props.availability_set.platform_update_domain_count).to eq(platform_update_domain_count) + expect(vm_cloud_props.availability_set.platform_fault_domain_count).to eq(platform_fault_domain_count) + end + end + + context 'when load_balancer is a hash' do + let(:lb_name) { 'fake_lb_name' } + let(:resource_group_name) { 'fake_resource_group' } + context 'when resource group not empty' do + let(:vm_cloud_props) do + Bosh::AzureCloud::VMCloudProps.new( + { + 'instance_type' => 'Standard_D1', + 'load_balancer' => { + 'name' => lb_name, + 'resource_group_name' => resource_group_name + } + }, azure_config_managed + ) + end + + it 'should return the correct config' do + expect(vm_cloud_props.load_balancer.name).to eq(lb_name) + expect(vm_cloud_props.load_balancer.resource_group_name).to eq(resource_group_name) + end + end + context 'when resource group empty' do + let(:vm_cloud_props) do + Bosh::AzureCloud::VMCloudProps.new( + { + 'instance_type' => 'Standard_D1', + 'load_balancer' => { + 'name' => lb_name + } + }, azure_config_managed + ) + end + + it 'should return the correct config' do + expect(vm_cloud_props.load_balancer.name).to eq(lb_name) + expect(vm_cloud_props.load_balancer.resource_group_name).to eq(azure_config_managed.resource_group_name) + end + end + end + end +end diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/availability_set_spec.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/availability_set_spec.rb index ff220f4e3..4b4165ff7 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/availability_set_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/availability_set_spec.rb @@ -62,11 +62,11 @@ end let(:avset_params) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' }, - platform_update_domain_count: vm_props.platform_update_domain_count, - platform_fault_domain_count: vm_props.platform_fault_domain_count, + platform_update_domain_count: vm_props.availability_set.platform_update_domain_count, + platform_fault_domain_count: vm_props.availability_set.platform_fault_domain_count, managed: false } end @@ -78,7 +78,7 @@ before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(nil, availability_set) end @@ -119,11 +119,11 @@ end let(:avset_params) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' }, - platform_update_domain_count: vm_props.platform_update_domain_count, - platform_fault_domain_count: vm_props.platform_fault_domain_count, + platform_update_domain_count: vm_props.availability_set.platform_update_domain_count, + platform_fault_domain_count: vm_props.availability_set.platform_fault_domain_count, managed: false } end @@ -135,7 +135,7 @@ before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(nil, availability_set) end @@ -261,7 +261,7 @@ context 'when platform_update_domain_count and platform_fault_domain_count are set' do let(:avset_params) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' }, platform_update_domain_count: 4, @@ -339,7 +339,7 @@ end let(:avset_params) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' } } @@ -489,7 +489,7 @@ before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(availability_set) end @@ -527,7 +527,7 @@ before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(availability_set) end @@ -550,7 +550,7 @@ let(:existing_avset) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' }, platform_update_domain_count: 5, @@ -560,7 +560,7 @@ end let(:existing_avset_managed) do { - name: vm_props.availability_set, + name: vm_props.availability_set.name, location: location, tags: { 'user-agent' => 'bosh' }, platform_update_domain_count: 5, @@ -582,7 +582,7 @@ context 'existing avset is unmanaged' do before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(existing_avset) end @@ -605,7 +605,7 @@ context 'when using managed avset in unmanaged vm' do before do allow(azure_client).to receive(:get_availability_set_by_name) - .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set) + .with(MOCK_RESOURCE_GROUP_NAME, vm_props.availability_set.name) .and_return(existing_avset_managed) end diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/invalid_option_spec.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/invalid_option_spec.rb index db3da5708..936f11ca8 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/invalid_option_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/invalid_option_spec.rb @@ -68,7 +68,7 @@ .with(MOCK_RESOURCE_GROUP_NAME, vm_name) .and_return([]) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -95,7 +95,7 @@ .with(MOCK_RESOURCE_GROUP_NAME, vm_name) .and_return([]) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -125,7 +125,7 @@ allow(manual_network).to receive(:resource_group_name) .and_return('fake-resource-group-name') allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -174,7 +174,7 @@ context 'when public ip is not found' do before do allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -236,7 +236,7 @@ it 'should raise an error' do allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(nil) expect(azure_client).not_to receive(:delete_virtual_machine) @@ -257,7 +257,7 @@ it 'should raise an error' do allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -277,7 +277,7 @@ allow(azure_client).to receive(:get_network_subnet_by_name) .and_return(subnet) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) @@ -321,7 +321,7 @@ allow(azure_client).to receive(:get_network_subnet_by_name) .and_return(subnet) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_props.load_balancer) + .with(vm_props.load_balancer.resource_group_name, vm_props.load_balancer.name) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_props.application_gateway) diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb index e1309d9fd..8d6fc8b4e 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb @@ -124,7 +124,7 @@ .with(MOCK_RESOURCE_GROUP_NAME, MOCK_DEFAULT_SECURITY_GROUP) .and_return(default_security_group) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_properties['load_balancer']) + .with(MOCK_RESOURCE_GROUP_NAME, vm_properties['load_balancer']) .and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_properties['application_gateway']) diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/vm_is_not_created_spec.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/vm_is_not_created_spec.rb index d06f50297..21b331ff1 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/vm_is_not_created_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/vm_is_not_created_spec.rb @@ -45,7 +45,11 @@ end before do - vm_props.availability_set = availability_set_name + vm_props.availability_set = Bosh::AzureCloud::AvailabilitySetConfig.new( + availability_set_name, + 5, + 1 + ) allow(azure_client).to receive(:create_virtual_machine) .and_raise('virtual machine is not created') @@ -143,8 +147,11 @@ end before do - vm_props.availability_set = availability_set_name - + vm_props.availability_set = Bosh::AzureCloud::AvailabilitySetConfig.new( + availability_set_name, + 5, + 1 + ) allow(azure_client).to receive(:get_availability_set_by_name) .and_return(availability_set) diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/delete_spec.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/delete_spec.rb index 06ba7d42b..c97361828 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/delete_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/delete_spec.rb @@ -55,7 +55,7 @@ .and_return(vm_name) allow(azure_client).to receive(:get_load_balancer_by_name) - .with(vm_name).and_return(load_balancer) + .with(resource_group_name, vm_name).and_return(load_balancer) allow(azure_client).to receive(:get_application_gateway_by_name) .with(vm_name).and_return(application_gateway) allow(azure_client).to receive(:get_network_interface_by_name) From 30af79173df42d8dcc6004bf62ddde062198944f Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 27 Sep 2018 10:53:29 +0800 Subject: [PATCH 2/5] also for the security group. --- src/bosh_azure_cpi/lib/cloud/azure.rb | 1 + .../lib/cloud/azure/models/config.rb | 4 ++- .../lib/cloud/azure/models/security_group.rb | 32 +++++++++++++++++ .../lib/cloud/azure/models/vm_cloud_props.rb | 5 +-- .../cloud/azure/network/dynamic_network.rb | 2 +- .../lib/cloud/azure/network/manual_network.rb | 2 +- .../lib/cloud/azure/network/network.rb | 8 ++--- .../lib/cloud/azure/vms/vm_manager_network.rb | 34 ++++++++++++++----- .../spec/unit/dynamic_network_spec.rb | 5 +-- .../spec/unit/manual_network_spec.rb | 5 +-- .../spec/unit/models/security_group_spec.rb | 33 ++++++++++++++++++ .../spec/unit/models/vm_cloud_props_spec.rb | 1 + .../create/network_security_group_spec.rb | 10 ++++-- .../unit/vm_manager/create/shared_stuff.rb | 8 +++-- 14 files changed, 124 insertions(+), 26 deletions(-) create mode 100644 src/bosh_azure_cpi/lib/cloud/azure/models/security_group.rb create mode 100644 src/bosh_azure_cpi/spec/unit/models/security_group_spec.rb diff --git a/src/bosh_azure_cpi/lib/cloud/azure.rb b/src/bosh_azure_cpi/lib/cloud/azure.rb index e201a8e18..19ea12407 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure.rb @@ -60,6 +60,7 @@ module AzureCloud; end require 'cloud/azure/models/instance_id' require 'cloud/azure/models/props_factory' require 'cloud/azure/models/root_disk' +require 'cloud/azure/models/security_group' require 'cloud/azure/models/vm_cloud_props' require 'cloud/azure/cloud' require 'cloud/azure/restapi/azure_client' diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb index 81f85c6ed..77353a475 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/config.rb @@ -78,7 +78,9 @@ def initialize(azure_config_hash) @use_managed_disks = azure_config_hash['use_managed_disks'] @storage_account_name = azure_config_hash['storage_account_name'] - @default_security_group = azure_config_hash['default_security_group'] + @default_security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group( + azure_config_hash['default_security_group'] + ) # Troubleshooting @enable_vm_boot_diagnostics = azure_config_hash['enable_vm_boot_diagnostics'] diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/security_group.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/security_group.rb new file mode 100644 index 000000000..8aea0c4de --- /dev/null +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/security_group.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Bosh::AzureCloud + class SecurityGroup + attr_reader :resource_group_name, :name + + RESOURCE_GROUP_NAME_KEY = 'resource_group_name' + NAME_KEY = 'name' + def initialize(resource_group_name, name) + @resource_group_name = resource_group_name + @name = name + end + + def self.parse_security_group(security_group_field) + if security_group_field.is_a?(Hash) + new( + security_group_field[RESOURCE_GROUP_NAME_KEY], + security_group_field[NAME_KEY] + ) + else + new( + nil, + security_group_field + ) + end + end + + def to_s + "name: #{@name}, resource_group_name: #{@resource_group_name}" + end + end +end diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb index c36d164fc..cf74a6a94 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb @@ -12,7 +12,8 @@ class VMCloudProps attr_reader :root_disk, :ephemeral_disk attr_reader :ip_forwarding, :accelerated_networking, :assign_dynamic_public_ip, :application_gateway attr_reader :load_balancer - attr_reader :application_security_groups, :security_group + attr_reader :application_security_groups + attr_reader :security_group attr_writer :availability_zone attr_writer :availability_set @@ -46,7 +47,7 @@ def initialize(vm_properties, global_azure_config) @application_gateway = vm_properties['application_gateway'] @application_security_groups = vm_properties['application_security_groups'] - @security_group = vm_properties['security_group'] + @security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group(vm_properties['security_group']) root_disk_hash = vm_properties.fetch('root_disk', {}) ephemeral_disk_hash = vm_properties.fetch('ephemeral_disk', {}) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/network/dynamic_network.rb b/src/bosh_azure_cpi/lib/cloud/azure/network/dynamic_network.rb index cd7f4f3f0..697f7e926 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/network/dynamic_network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/network/dynamic_network.rb @@ -43,7 +43,7 @@ def initialize(azure_config, name, spec) @subnet_name = @cloud_properties['subnet_name'] end - @security_group = @cloud_properties['security_group'] + @security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group(@cloud_properties['security_group']) @application_security_groups = @cloud_properties.fetch('application_security_groups', []) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/network/manual_network.rb b/src/bosh_azure_cpi/lib/cloud/azure/network/manual_network.rb index a7294521f..d45a6a5dd 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/network/manual_network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/network/manual_network.rb @@ -47,7 +47,7 @@ def initialize(azure_config, name, spec) cloud_error('ip address required for manual network') if @ip.nil? - @security_group = @cloud_properties['security_group'] + @security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group(@cloud_properties['security_group']) @application_security_groups = @cloud_properties.fetch('application_security_groups', []) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/network/network.rb b/src/bosh_azure_cpi/lib/cloud/azure/network/network.rb index 1b00637b3..93fe700a4 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/network/network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/network/network.rb @@ -3,7 +3,9 @@ module Bosh::AzureCloud class Network attr_reader :resource_group_name + attr_reader :spec + RESOURCE_GROUP_NAME_KEY = 'resource_group_name' ## # Creates a new network # @@ -22,13 +24,11 @@ def initialize(azure_config, name, spec) @ip = spec['ip'] @cloud_properties = spec['cloud_properties'] @spec = spec - @resource_group_name = if @cloud_properties.nil? || @cloud_properties['resource_group_name'].nil? + @resource_group_name = if @cloud_properties.nil? || @cloud_properties[RESOURCE_GROUP_NAME_KEY].nil? @azure_config.resource_group_name else - @cloud_properties['resource_group_name'] + @cloud_properties[RESOURCE_GROUP_NAME_KEY] end end - - attr_reader :spec end end diff --git a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb index b585a22ff..896e2b0e5 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb @@ -12,21 +12,37 @@ def _get_network_subnet(network) def _get_network_security_group(vm_props, network) # Network security group name can be specified in vm_types or vm_extensions, networks and global configuration (ordered by priority) - network_security_group_name = vm_props.security_group.nil? ? network.security_group : vm_props.security_group - network_security_group_name = @azure_config.default_security_group if network_security_group_name.nil? - return nil if network_security_group_name.nil? + network_security_group_cfg = vm_props.security_group.name.nil? ? network.security_group : vm_props.security_group + network_security_group_cfg = @azure_config.default_security_group if network_security_group_cfg.name.nil? + return nil if network_security_group_cfg.name.nil? + + cloud_error('Cannot specify an empty string to the network security group') if network_security_group_cfg.name.empty? + + unless network_security_group_cfg.resource_group_name.nil? + network_security_group = @azure_client.get_network_security_group_by_name( + network_security_group_cfg.resource_group_name, + network_security_group_cfg.name + ) + cloud_error("Cannot found the security group: #{network_security_group_cfg.name} in the specified resource group: #{network_security_group_cfg.resource_group_name}") if network_security_group.nil? + return network_security_group + end - cloud_error('Cannot specify an empty string to the network security group') if network_security_group_name.empty? - # The resource group which the NSG belongs to can be specified in networks and global configuration (ordered by priority) resource_group_name = network.resource_group_name - network_security_group = @azure_client.get_network_security_group_by_name(resource_group_name, network_security_group_name) + + # The resource group which the NSG belongs to can be specified in networks and global configuration (ordered by priority) + network_security_group = @azure_client.get_network_security_group_by_name( + resource_group_name, + network_security_group_cfg.name + ) + # network.resource_group_name may return the default resource group name in global configurations. See network.rb. default_resource_group_name = @azure_config.resource_group_name if network_security_group.nil? && resource_group_name != default_resource_group_name - @logger.info("Cannot find the network security group '#{network_security_group_name}' in the resource group '#{resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'") - network_security_group = @azure_client.get_network_security_group_by_name(default_resource_group_name, network_security_group_name) + @logger.info("Cannot find the network security group '#{network_security_group_cfg.name}' in the resource group '#{network_security_group_cfg.resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'") + network_security_group = @azure_client.get_network_security_group_by_name(default_resource_group_name, network_security_group_cfg.name) end - cloud_error("Cannot find the network security group '#{network_security_group_name}'") if network_security_group.nil? + + cloud_error("Cannot find the network security group '#{network_security_group_cfg.name}'") if network_security_group.nil? network_security_group end diff --git a/src/bosh_azure_cpi/spec/unit/dynamic_network_spec.rb b/src/bosh_azure_cpi/spec/unit/dynamic_network_spec.rb index a723fbd8e..9e9f4f7b4 100644 --- a/src/bosh_azure_cpi/spec/unit/dynamic_network_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/dynamic_network_spec.rb @@ -33,7 +33,8 @@ expect(network.resource_group_name).to eq(rg_name) expect(network.virtual_network_name).to eq(vnet_name) expect(network.subnet_name).to eq(subnet_name) - expect(network.security_group).to eq(nsg_name) + expect(network.security_group.name).to eq(nsg_name) + expect(network.security_group.resource_group_name).to eq(nil) expect(network.application_security_groups).to eq(asg_names) expect(network.ip_forwarding).to eq(true) expect(network.accelerated_networking).to eq(true) @@ -139,7 +140,7 @@ it 'should return default values for the optional cloud properties' do network = Bosh::AzureCloud::DynamicNetwork.new(azure_config, 'default', network_spec) - expect(network.security_group).to be_nil + expect(network.security_group.name).to be_nil expect(network.application_security_groups).to eq([]) expect(network.ip_forwarding).to be false expect(network.accelerated_networking).to be false diff --git a/src/bosh_azure_cpi/spec/unit/manual_network_spec.rb b/src/bosh_azure_cpi/spec/unit/manual_network_spec.rb index c2cc3ed15..0015cd866 100644 --- a/src/bosh_azure_cpi/spec/unit/manual_network_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/manual_network_spec.rb @@ -36,7 +36,8 @@ expect(network.resource_group_name).to eq(rg_name) expect(network.virtual_network_name).to eq(vnet_name) expect(network.subnet_name).to eq(subnet_name) - expect(network.security_group).to eq(nsg_name) + expect(network.security_group.name).to eq(nsg_name) + expect(network.security_group.resource_group_name).to eq(nil) expect(network.application_security_groups).to eq(asg_names) expect(network.ip_forwarding).to eq(true) expect(network.accelerated_networking).to eq(true) @@ -180,7 +181,7 @@ it 'should return default values for the optional cloud properties' do network = Bosh::AzureCloud::ManualNetwork.new(azure_config, 'default', network_spec) - expect(network.security_group).to be_nil + expect(network.security_group.name).to be_nil expect(network.application_security_groups).to eq([]) expect(network.ip_forwarding).to be false expect(network.accelerated_networking).to be false diff --git a/src/bosh_azure_cpi/spec/unit/models/security_group_spec.rb b/src/bosh_azure_cpi/spec/unit/models/security_group_spec.rb new file mode 100644 index 000000000..897fd8f21 --- /dev/null +++ b/src/bosh_azure_cpi/spec/unit/models/security_group_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Bosh::AzureCloud::SecurityGroup do + let(:resource_group_name) { 'fake_resource_group' } + let(:sg_name) { 'fake_sg_name' } + let(:security_group_with_resource_group) do + { + 'resource_group_name' => resource_group_name, + 'name' => sg_name + } + end + describe '#self.parse_security_group' do + context 'when resource group specified' do + it 'the resource group value is correct' do + security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group(security_group_with_resource_group) + expect(security_group.resource_group_name).to eq(resource_group_name) + expect(security_group.name).to eq(sg_name) + end + end + end + + describe '#to_s' do + context 'when called' do + let(:sg_string) { "name: #{sg_name}, resource_group_name: #{resource_group_name}" } + it 'should return the correct string' do + security_group = Bosh::AzureCloud::SecurityGroup.parse_security_group(security_group_with_resource_group) + expect(security_group.to_s).to eq(sg_string) + end + end + end +end diff --git a/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb b/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb index 489c9588c..bfc0d46b7 100644 --- a/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/models/vm_cloud_props_spec.rb @@ -53,6 +53,7 @@ expect(vm_cloud_props.load_balancer.resource_group_name).to eq(resource_group_name) end end + context 'when resource group empty' do let(:vm_cloud_props) do Bosh::AzureCloud::VMCloudProps.new( diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/network_security_group_spec.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/network_security_group_spec.rb index b830b5e73..98a2b9349 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/network_security_group_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/network_security_group_spec.rb @@ -92,6 +92,12 @@ context ' and network specs' do let(:nsg_name_in_network_spec) { 'fake-nsg-name-specified-in-network-spec' } + let(:nsg_in_network_spec) do + Bosh::AzureCloud::SecurityGroup.new( + MOCK_RESOURCE_GROUP_NAME, + nsg_name_in_network_spec + ) + end let(:security_group_in_network_spec) do { name: nsg_name_in_network_spec @@ -99,8 +105,8 @@ end before do - allow(manual_network).to receive(:security_group).and_return(nsg_name_in_network_spec) - allow(dynamic_network).to receive(:security_group).and_return(nsg_name_in_network_spec) + allow(manual_network).to receive(:security_group).and_return(nsg_in_network_spec) + allow(dynamic_network).to receive(:security_group).and_return(nsg_in_network_spec) allow(azure_client).to receive(:get_network_security_group_by_name) .with(MOCK_RESOURCE_GROUP_NAME, nsg_name_in_network_spec) .and_return(security_group_in_network_spec) diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb index 8d6fc8b4e..f597ffd86 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb @@ -63,6 +63,9 @@ let(:network_configurator) { instance_double(Bosh::AzureCloud::NetworkConfigurator) } let(:env) { {} } + # Security Group + let(:empty_security_group) { Bosh::AzureCloud::SecurityGroup.new(nil,nil) } + # Instance ID let(:vm_name) { agent_id } let(:storage_account_name) { 'fake-storage-acount-name' } @@ -137,6 +140,7 @@ end # Network + let(:vip_network) { instance_double(Bosh::AzureCloud::VipNetwork) } let(:manual_network) { instance_double(Bosh::AzureCloud::ManualNetwork) } let(:dynamic_network) { instance_double(Bosh::AzureCloud::DynamicNetwork) } @@ -166,7 +170,7 @@ allow(manual_network).to receive(:private_ip) .and_return('private-ip') allow(manual_network).to receive(:security_group) - .and_return(nil) + .and_return(empty_security_group) allow(manual_network).to receive(:application_security_groups) .and_return([]) allow(manual_network).to receive(:ip_forwarding) @@ -181,7 +185,7 @@ allow(dynamic_network).to receive(:subnet_name) .and_return('fake-subnet-name') allow(dynamic_network).to receive(:security_group) - .and_return(nil) + .and_return(empty_security_group) allow(dynamic_network).to receive(:application_security_groups) .and_return([]) allow(dynamic_network).to receive(:ip_forwarding) From 808adddff4bcb4aa2e62962cc10e61f2d197a45f Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 27 Sep 2018 14:34:09 +0800 Subject: [PATCH 3/5] resolve comments. --- .../lib/cloud/azure/models/vm_cloud_props.rb | 6 +---- .../lib/cloud/azure/restapi/azure_client.rb | 26 +++++++++---------- .../unit/azure_client/azure_client_spec.rb | 8 +++--- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb index cf74a6a94..2831a5caf 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/models/vm_cloud_props.rb @@ -81,11 +81,7 @@ def _default_fault_domain_count(global_azure_config) def _parse_load_balancer_config(vm_properties, global_azure_config) if vm_properties[LOAD_BALANCER_KEY].is_a?(Hash) - resource_group_name = if vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY].nil? - global_azure_config.resource_group_name - else - vm_properties[LOAD_BALANCER_KEY][RESOURCE_GROUP_NAME_KEY] - end + 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] diff --git a/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb b/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb index 5ebd5cabb..bed75b49f 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/restapi/azure_client.rb @@ -120,19 +120,6 @@ def rest_api_url(resource_provider, resource_type, resource_group_name: nil, nam url end - def _parse_name_from_id(id) - ret = id.match('/subscriptions/([^/]*)/resourceGroups/([^/]*)/providers/([^/]*)/([^/]*)/([^/]*)(.*)') - raise AzureError, "\"#{id}\" is not a valid URL." if ret.nil? - - result = {} - result[:subscription_id] = ret[1] - result[:resource_group_name] = ret[2] - result[:provider_name] = ret[3] - result[:resource_type] = ret[4] - result[:resource_name] = ret[5] - result - end - def get_resource_by_id(url, params = {}) result = nil begin @@ -1863,6 +1850,19 @@ def update_tags_of_storage_account(name, tags) private + def _parse_name_from_id(id) + ret = id.match('/subscriptions/([^/]*)/resourceGroups/([^/]*)/providers/([^/]*)/([^/]*)/([^/]*)(.*)') + raise AzureError, "\"#{id}\" is not a valid URL." if ret.nil? + + result = {} + result[:subscription_id] = ret[1] + result[:resource_group_name] = ret[2] + result[:provider_name] = ret[3] + result[:resource_type] = ret[4] + result[:resource_name] = ret[5] + result + end + def parse_vm_size(result) vm_size = nil unless result.nil? diff --git a/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb b/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb index 626c03b01..23d4f281a 100644 --- a/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb +++ b/src/bosh_azure_cpi/spec/unit/azure_client/azure_client_spec.rb @@ -98,7 +98,7 @@ it 'should raise an error' do id = '' expect do - azure_client._parse_name_from_id(id) + azure_client.send(:_parse_name_from_id, id) end.to raise_error /\"#{id}\" is not a valid URL./ end end @@ -107,7 +107,7 @@ id = '/subscriptions/a/resourceGroups/b/providers/c/d' it 'should raise an error' do expect do - azure_client._parse_name_from_id(id) + azure_client.send(:_parse_name_from_id, id) end.to raise_error /\"#{id}\" is not a valid URL./ end end @@ -121,7 +121,7 @@ result[:provider_name] = 'c' result[:resource_type] = 'd' result[:resource_name] = 'e' - expect(azure_client._parse_name_from_id(id)).to eq(result) + expect(azure_client.send(:_parse_name_from_id, id)).to eq(result) end end @@ -134,7 +134,7 @@ result[:provider_name] = 'c' result[:resource_type] = 'd' result[:resource_name] = 'e' - expect(azure_client._parse_name_from_id(id)).to eq(result) + expect(azure_client.send(:_parse_name_from_id, id)).to eq(result) end end end From ad5a4f97763ac92110aacb8c38500b16ab25c653 Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 27 Sep 2018 14:36:22 +0800 Subject: [PATCH 4/5] minor kubo fix. --- src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb index f597ffd86..34b6cfe8d 100644 --- a/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb +++ b/src/bosh_azure_cpi/spec/unit/vm_manager/create/shared_stuff.rb @@ -64,7 +64,7 @@ let(:env) { {} } # Security Group - let(:empty_security_group) { Bosh::AzureCloud::SecurityGroup.new(nil,nil) } + let(:empty_security_group) { Bosh::AzureCloud::SecurityGroup.new(nil, nil) } # Instance ID let(:vm_name) { agent_id } From 115e49f60bd950ed6e81cf0a60ead2e890615507 Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 27 Sep 2018 15:23:35 +0800 Subject: [PATCH 5/5] fix. --- src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb index 896e2b0e5..a3b578b06 100644 --- a/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb +++ b/src/bosh_azure_cpi/lib/cloud/azure/vms/vm_manager_network.rb @@ -38,7 +38,7 @@ def _get_network_security_group(vm_props, network) # network.resource_group_name may return the default resource group name in global configurations. See network.rb. default_resource_group_name = @azure_config.resource_group_name if network_security_group.nil? && resource_group_name != default_resource_group_name - @logger.info("Cannot find the network security group '#{network_security_group_cfg.name}' in the resource group '#{network_security_group_cfg.resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'") + @logger.info("Cannot find the network security group '#{network_security_group_cfg.name}' in the resource group '#{resource_group_name}', trying to search it in the default resource group '#{default_resource_group_name}'") network_security_group = @azure_client.get_network_security_group_by_name(default_resource_group_name, network_security_group_cfg.name) end