From 7f32371a249024b68ce2e0e71dd939cf73c43f99 Mon Sep 17 00:00:00 2001 From: Jamie van Dyke Date: Thu, 1 Sep 2016 19:36:16 +0100 Subject: [PATCH 1/2] add the ability to set the network property 'source/destination check' --- src/bosh_aws_cpi/lib/cloud/aws/instance.rb | 4 ++++ src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb | 1 + src/bosh_aws_cpi/spec/spec_helper.rb | 3 ++- src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb | 10 ++++++++-- src/bosh_aws_cpi/spec/unit/instance_spec.rb | 7 +++++++ 5 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/bosh_aws_cpi/lib/cloud/aws/instance.rb b/src/bosh_aws_cpi/lib/cloud/aws/instance.rb index 17929262..22ecd804 100644 --- a/src/bosh_aws_cpi/lib/cloud/aws/instance.rb +++ b/src/bosh_aws_cpi/lib/cloud/aws/instance.rb @@ -25,6 +25,10 @@ def disassociate_elastic_ip @aws_instance.disassociate_elastic_ip end + def source_dest_check=(state) + @aws_instance.source_dest_check = state + end + def wait_for_running # If we time out, it is because the instance never gets from state running to started, # so we signal the director that it is ok to retry the operation. At the moment this diff --git a/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb b/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb index b0c47563..94228288 100644 --- a/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb +++ b/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb @@ -34,6 +34,7 @@ def create(agent_id, stemcell_id, resource_pool, networks_spec, disk_locality, e instance.wait_for_running instance.attach_to_load_balancers(resource_pool['elbs'] || []) instance.update_routing_tables(resource_pool['advertised_routes'] || []) + instance.source_dest_check = resource_pool['source_dest_check'] || true rescue => e @logger.warn("Failed to configure instance '#{instance.id}': #{e.inspect}") begin diff --git a/src/bosh_aws_cpi/spec/spec_helper.rb b/src/bosh_aws_cpi/spec/spec_helper.rb index 550ff6e4..644b62eb 100644 --- a/src/bosh_aws_cpi/spec/spec_helper.rb +++ b/src/bosh_aws_cpi/spec/spec_helper.rb @@ -17,7 +17,8 @@ def mock_cloud_options 'region' => 'us-east-1', 'default_key_name' => 'sesame', 'default_security_groups' => [], - 'max_retries' => 8 + 'max_retries' => 8, + 'source_dest_check' => false }, 'registry' => { 'endpoint' => 'localhost:42288', diff --git a/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb b/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb index 2a3869cb..e1fe8a66 100644 --- a/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb +++ b/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb @@ -24,7 +24,8 @@ module Bosh::AwsCloud let(:stemcell_id) { 'stemcell-id' } let(:resource_pool) { { 'instance_type' => 'm1.small', - 'availability_zone' => 'us-east-1a' + 'availability_zone' => 'us-east-1a', + 'source_dest_check' => false } } let(:networks_spec) do { @@ -90,6 +91,7 @@ module Bosh::AwsCloud it 'should ask AWS to create an instance in the given region, with parameters built up from the given arguments' do instance_manager = InstanceManager.new(ec2, registry, elb, param_mapper, block_device_manager, logger) allow(instance_manager).to receive(:get_created_instance_id).with("run-instances-response").and_return('i-12345678') + allow(aws_instance).to receive(:source_dest_check=).with(true) expect(aws_client).to receive(:run_instances).with({ fake: 'instance-params', min_count: 1, max_count: 1 }).and_return("run-instances-response") instance_manager.create( @@ -111,6 +113,7 @@ module Bosh::AwsCloud it 'should ask AWS to create a SPOT instance in the given region, when resource_pool includes spot_bid_price' do allow(ec2).to receive(:client).and_return(aws_client) + allow(aws_instance).to receive(:source_dest_check=).with(true) # need to translate security group names to security group ids sg1 = instance_double('AWS::EC2::SecurityGroup', id:'sg-baz-1234') @@ -193,6 +196,7 @@ module Bosh::AwsCloud end it 'creates an on demand instance' do + allow(aws_instance).to receive(:source_dest_check=).with(true) expect(aws_client).to receive(:run_instances) .with({ fake: 'instance-params', min_count: 1, max_count: 1 }) @@ -209,6 +213,7 @@ module Bosh::AwsCloud it 'does not log a warning' do expect(logger).to_not receive(:warn) + allow(aws_instance).to receive(:source_dest_check=).with(true) instance_manager.create( agent_id, @@ -233,6 +238,7 @@ module Bosh::AwsCloud expect(aws_client).to receive(:run_instances).with({ fake: 'instance-params', min_count: 1, max_count: 1 }).and_return("run-instances-response") allow(ResourceWait).to receive(:for_instance).with(instance: aws_instance, state: :running) + allow(aws_instance).to receive(:source_dest_check=).with(true) expect(logger).to receive(:warn).with(/IP address was in use/).once instance_manager.create( @@ -246,7 +252,7 @@ module Bosh::AwsCloud ) end - context 'when waiting it to become running fails' do + context 'when waiting for the instance to be running fails' do let(:instance) { instance_double('Bosh::AwsCloud::Instance', id: 'fake-instance-id') } let(:create_err) { StandardError.new('fake-err') } diff --git a/src/bosh_aws_cpi/spec/unit/instance_spec.rb b/src/bosh_aws_cpi/spec/unit/instance_spec.rb index 66b8b208..e315051e 100644 --- a/src/bosh_aws_cpi/spec/unit/instance_spec.rb +++ b/src/bosh_aws_cpi/spec/unit/instance_spec.rb @@ -168,6 +168,13 @@ module Bosh::AwsCloud end end + describe '#source_dest_check=' do + it 'propagates source_dest_check=' do + expect(aws_instance).to receive(:source_dest_check=).with(false) + instance.source_dest_check = false + end + end + describe '#attach_to_load_balancers' do before { allow(elb).to receive(:load_balancers).and_return(load_balancers) } let(:load_balancers) { { 'fake-lb1-id' => load_balancer1, 'fake-lb2-id' => load_balancer2 } } From 10fd76b6d8cb44ef21f385da66dc08fd8f5257df Mon Sep 17 00:00:00 2001 From: Jamie van Dyke Date: Fri, 2 Sep 2016 10:01:47 +0100 Subject: [PATCH 2/2] add a true and false test and fix the implementation --- src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb | 2 +- src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb | 4 ++-- src/bosh_aws_cpi/spec/unit/instance_spec.rb | 7 ++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb b/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb index 94228288..7fbf6c0f 100644 --- a/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb +++ b/src/bosh_aws_cpi/lib/cloud/aws/instance_manager.rb @@ -34,7 +34,7 @@ def create(agent_id, stemcell_id, resource_pool, networks_spec, disk_locality, e instance.wait_for_running instance.attach_to_load_balancers(resource_pool['elbs'] || []) instance.update_routing_tables(resource_pool['advertised_routes'] || []) - instance.source_dest_check = resource_pool['source_dest_check'] || true + instance.source_dest_check = resource_pool.fetch('source_dest_check', true) rescue => e @logger.warn("Failed to configure instance '#{instance.id}': #{e.inspect}") begin diff --git a/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb b/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb index e1fe8a66..a9b8900a 100644 --- a/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb +++ b/src/bosh_aws_cpi/spec/unit/instance_manager_spec.rb @@ -91,7 +91,7 @@ module Bosh::AwsCloud it 'should ask AWS to create an instance in the given region, with parameters built up from the given arguments' do instance_manager = InstanceManager.new(ec2, registry, elb, param_mapper, block_device_manager, logger) allow(instance_manager).to receive(:get_created_instance_id).with("run-instances-response").and_return('i-12345678') - allow(aws_instance).to receive(:source_dest_check=).with(true) + allow(aws_instance).to receive(:source_dest_check=).with(false) expect(aws_client).to receive(:run_instances).with({ fake: 'instance-params', min_count: 1, max_count: 1 }).and_return("run-instances-response") instance_manager.create( @@ -238,7 +238,7 @@ module Bosh::AwsCloud expect(aws_client).to receive(:run_instances).with({ fake: 'instance-params', min_count: 1, max_count: 1 }).and_return("run-instances-response") allow(ResourceWait).to receive(:for_instance).with(instance: aws_instance, state: :running) - allow(aws_instance).to receive(:source_dest_check=).with(true) + allow(aws_instance).to receive(:source_dest_check=).with(false) expect(logger).to receive(:warn).with(/IP address was in use/).once instance_manager.create( diff --git a/src/bosh_aws_cpi/spec/unit/instance_spec.rb b/src/bosh_aws_cpi/spec/unit/instance_spec.rb index e315051e..abc9733f 100644 --- a/src/bosh_aws_cpi/spec/unit/instance_spec.rb +++ b/src/bosh_aws_cpi/spec/unit/instance_spec.rb @@ -169,10 +169,15 @@ module Bosh::AwsCloud end describe '#source_dest_check=' do - it 'propagates source_dest_check=' do + it 'propagates source_dest_check= true' do expect(aws_instance).to receive(:source_dest_check=).with(false) instance.source_dest_check = false end + + it 'propagates source_dest_check= false' do + expect(aws_instance).to receive(:source_dest_check=).with(true) + instance.source_dest_check = true + end end describe '#attach_to_load_balancers' do