Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing error where aws returns DNS name as empty string #124

Merged
merged 2 commits into from
May 21, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,9 @@ Style/Next:

Style/DoubleNegation:
Enabled: false

Metrics/CyclomaticComplexity:
Max: 30

Metrics/PerceivedComplexity:
Max: 30
33 changes: 21 additions & 12 deletions lib/kitchen/driver/ec2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@
require "json"
require "aws"
require "kitchen"
require "kitchen/driver/ec2_version"
require_relative "ec2_version"
require_relative "aws/client"
require_relative "aws/instance_generator"
require "aws-sdk-core/waiters/errors"

module Kitchen

Expand Down Expand Up @@ -60,6 +61,7 @@ class Ec2 < Kitchen::Driver::Base # rubocop:disable Metrics/ClassLength
end
default_config :username, nil
default_config :associate_public_ip, nil
default_config :interface, nil

required_config :aws_ssh_key_id
required_config :image_id
Expand Down Expand Up @@ -194,14 +196,21 @@ def create(state) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
t = config[:retryable_tries] * config[:retryable_sleep]
info "Waited #{c}/#{t}s for instance <#{state[:server_id]}> to become ready."
end
server = server.wait_until(
:max_attempts => config[:retryable_tries],
:delay => config[:retryable_sleep],
:before_attempt => wait_log
) do |s|
hostname = hostname(s)
# Euca instances often report ready before they have an IP
s.state.name == "running" && !hostname.nil? && hostname != "0.0.0.0"
begin
server = server.wait_until(
:max_attempts => config[:retryable_tries],
:delay => config[:retryable_sleep],
:before_attempt => wait_log
) do |s|
hostname = hostname(s, config[:interface])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only problem I can see with this is if a user has interface set in their .kitchen.yml to public and associate_public_ip = false then kitchen will never detect its running

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, but I would call that user error - the user has told Test Kitchen to query a parameter that is never going to be available. A kitchen diagnose should reveal this.

# Euca instances often report ready before they have an IP
s.exists? && s.state.name == "running" && !hostname.nil? && hostname != "0.0.0.0"
end
rescue ::Aws::Waiters::Errors::WaiterFailed
error("Ran out of time waiting for the server with id [#{state[:server_id]}]" \
" to become ready, attempting to destroy it")
destroy(state)
raise
end

info("EC2 instance <#{state[:server_id]}> ready.")
Expand All @@ -217,7 +226,7 @@ def destroy(state)
server = ec2.get_instance(state[:server_id])
unless server.nil?
instance.transport.connection(state).close
server.terminate unless server.nil?
server.terminate
end
if state[:spot_request_id]
debug("Deleting spot request <#{state[:server_id]}>")
Expand All @@ -235,8 +244,6 @@ def default_ami
region && region[instance.platform.name]
end

private

def ec2
@ec2 ||= Aws::Client.new(
config[:region],
Expand Down Expand Up @@ -354,6 +361,8 @@ def hostname(server, interface_type = nil)
potential_hostname = nil
INTERFACE_TYPES.values.each do |type|
potential_hostname ||= server.send(type)
# AWS returns an empty string if the dns name isn't populated yet

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not default to private_ip unless specified?

It seems like it would be the most likely to be available, followed by public_ip and then DNS. Looks like there could be reduced complexity unless there is something I am missing.

What is the behavior of not defining the hostname? (returning nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private_ip is the most likely to be available but not the most likely to be reachable by SSH. The logic here is currently

# if `interface_type` is provided:
  # query only that interface_type
# else
  # first, try dns
  # second, try public_ip
  # third, try private_ip
  # return the first value that is non-nil and non-empty

This brings the hostname logic in line with what is in the README. I don't want to break the backwards compatibility with logic that has been there for a while.

I also like the 'try from easiest to SSH into to hardest to SSH into' logic rather than 'try from what is most likely to be available to what is least likely to be available'. The goal is to get a hostname we can SSH into and then install Chef. If a user really wants to use private_ip they can specify interface: private in their .kitchen.yml.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I was thinking of this soley from inside a VPC, good call. Little short-sighted on my part.

potential_hostname = nil if potential_hostname == ""
end
potential_hostname
end
Expand Down
82 changes: 81 additions & 1 deletion spec/kitchen/driver/ec2_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,92 @@
end
end

describe "finalize_config!" do
describe "#finalize_config!" do
it "defaults the availability zone if not provided" do
expect(config[:availability_zone]).to eq(nil)
driver.finalize_config!(instance)
expect(config[:availability_zone]).to eq("us-east-1b")
end
end

describe "#hostname" do
let(:public_dns_name) { nil }
let(:public_ip_address) { nil }
let(:private_ip_address) { nil }
let(:server) {
double("server",
:public_dns_name => public_dns_name,
:public_ip_address => public_ip_address,
:private_ip_address => private_ip_address
)
}

it "returns nil if all sources are nil" do
expect(driver.hostname(server)).to eq(nil)
end

it "raises an error if provided an unknown interface" do
expect { driver.hostname(server, "foobar") }.to raise_error(Kitchen::UserError)
end

shared_examples "an interface type provided" do
it "returns public_dns_name when requested" do
expect(driver.hostname(server, "dns")).to eq(public_dns_name)
end
it "returns public_ip_address when requested" do
expect(driver.hostname(server, "public")).to eq(public_ip_address)
end
it "returns private_ip_address when requested" do
expect(driver.hostname(server, "private")).to eq(private_ip_address)
end
end

context "private_ip_address is populated" do
let(:private_ip_address) { "10.0.0.1" }

it "returns the private_ip_address" do
expect(driver.hostname(server)).to eq(private_ip_address)
end

include_examples "an interface type provided"
end

context "public_ip_address is populated" do
let(:private_ip_address) { "10.0.0.1" }
let(:public_ip_address) { "127.0.0.1" }

it "returns the public_ip_address" do
expect(driver.hostname(server)).to eq(public_ip_address)
end

include_examples "an interface type provided"
end

context "public_dns_name is populated" do
let(:private_ip_address) { "10.0.0.1" }
let(:public_ip_address) { "127.0.0.1" }
let(:public_dns_name) { "public_dns_name" }

it "returns the public_dns_name" do
expect(driver.hostname(server)).to eq(public_dns_name)
end

include_examples "an interface type provided"
end

context "public_dns_name returns as empty string" do
let(:public_dns_name) { "" }
it "returns nil" do
expect(driver.hostname(server)).to eq(nil)
end

context "and private_ip_address is populated" do
let(:private_ip_address) { "10.0.0.1" }
it "returns the private_ip_address" do
expect(driver.hostname(server)).to eq(private_ip_address)
end
end
end
end

end