From bfbd572f7b1a2922192b336bc4c68bd3594dda15 Mon Sep 17 00:00:00 2001 From: Josh Lane Date: Thu, 19 Feb 2015 15:54:23 -0800 Subject: [PATCH 1/4] [dns] fix Records#get * the first result isn't the only result * refactor exact record matching --- lib/fog/aws/models/dns/records.rb | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/fog/aws/models/dns/records.rb b/lib/fog/aws/models/dns/records.rb index 13a08182b..a5aa16a59 100644 --- a/lib/fog/aws/models/dns/records.rb +++ b/lib/fog/aws/models/dns/records.rb @@ -81,31 +81,25 @@ module Fog record_type = record_type.upcase unless record_type.nil? options = { - :max_items => 1, - :name => record_name, - :type => record_type, + :max_items => 1, + :name => record_name, + :type => record_type, :identifier => record_identifier } options.delete_if {|key, value| value.nil?} data = service.list_resource_record_sets(zone.id, options).body - # Get first record - data = data['ResourceRecordSets'].shift - if data - record = new(data) - # make sure everything matches - if record.name == record_name - if (!record_type.nil? && record.type != record_type) || - (!record_identifier.nil? && record.set_identifier != record_identifier) - nil - else - record - end + # look for an exact match in the records + (data['ResourceRecordSets'] || []).map do |record_data| + record = new(record_data) + + if (record.name == record_name) && + (record_type.nil? || (record.type == record_type)) && + (record_identifier.nil? || (record.set_identifier == record_identifier)) + record end - else - nil - end + end.compact.first rescue Excon::Errors::NotFound nil end From e4622dc8edc55bbbac52941fda7ca60c47c6121c Mon Sep 17 00:00:00 2001 From: Josh Lane Date: Thu, 19 Feb 2015 15:55:18 -0800 Subject: [PATCH 2/4] [dns] normalize mock record names * always end in "." * refactor --- .../dns/change_resource_record_sets.rb | 30 +++++++++++-------- .../requests/dns/list_resource_record_sets.rb | 1 - 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/fog/aws/requests/dns/change_resource_record_sets.rb b/lib/fog/aws/requests/dns/change_resource_record_sets.rb index b66530593..efa1ac6ea 100644 --- a/lib/fog/aws/requests/dns/change_resource_record_sets.rb +++ b/lib/fog/aws/requests/dns/change_resource_record_sets.rb @@ -155,20 +155,25 @@ module Fog response = Excon::Response.new errors = [] + if (zone = self.data[:zones][zone_id]) response.status = 200 change_id = Fog::AWS::Mock.change_id change_batch.each do |change| + + change_name = change[:name] + change_name = change_name + "." unless change_name.end_with?(".") + case change[:action] when "CREATE" if zone[:records][change[:type]].nil? zone[:records][change[:type]] = {} end - if zone[:records][change[:type]][change[:name]].nil? + if zone[:records][change[:type]][change_name].nil? # raise change.to_s if change[:resource_records].nil? - zone[:records][change[:type]][change[:name]] = + zone[:records][change[:type]][change_name] = if change[:alias_target] record = { :alias_target => change[:alias_target] @@ -178,11 +183,11 @@ module Fog :ttl => change[:ttl].to_s, } end - zone[:records][change[:type]][change[:name]] = { - :change_id => change_id, + zone[:records][change[:type]][change_name] = { + :change_id => change_id, :resource_records => change[:resource_records] || [], - :name => change[:name], - :type => change[:type] + :name => change_name, + :type => change[:type] }.merge(record) else errors << "Tried to create resource record set #{change[:name]}. type #{change[:type]}, but it already exists" @@ -196,14 +201,14 @@ module Fog if errors.empty? change = { - :id => change_id, - :status => 'PENDING', + :id => change_id, + :status => 'PENDING', :submitted_at => Time.now.utc.iso8601 } self.data[:changes][change[:id]] = change response.body = { - 'Id' => change[:id], - 'Status' => change[:status], + 'Id' => change[:id], + 'Status' => change[:status], 'SubmittedAt' => change[:submitted_at] } response @@ -221,10 +226,9 @@ module Fog end def self.hosted_zone_for_alias_target(dns_name) - k = elb_hosted_zone_mapping.keys.find do |k| + elb_hosted_zone_mapping.select { |k, _| dns_name =~ /\A.+\.#{k}\.elb\.amazonaws\.com\.?\z/ - end - elb_hosted_zone_mapping[k] + }.last end def self.elb_hosted_zone_mapping diff --git a/lib/fog/aws/requests/dns/list_resource_record_sets.rb b/lib/fog/aws/requests/dns/list_resource_record_sets.rb index 911bd8660..8aa4d1f32 100644 --- a/lib/fog/aws/requests/dns/list_resource_record_sets.rb +++ b/lib/fog/aws/requests/dns/list_resource_record_sets.rb @@ -86,7 +86,6 @@ module Fog if options[:name] name = options[:name].gsub(zone[:name],"") records = records.select{|r| r[:name].gsub(zone[:name],"") >= name } - require 'pp' end next_record = records[maxitems] From 64178d52931b6670472fb53972ae7d8491242c50 Mon Sep 17 00:00:00 2001 From: Josh Lane Date: Thu, 19 Feb 2015 16:20:16 -0800 Subject: [PATCH 3/4] fix tests, use proper errors --- lib/fog/aws/dns.rb | 15 +++- lib/fog/aws/models/dns/records.rb | 2 +- lib/fog/aws/models/dns/zones.rb | 2 +- .../dns/change_resource_record_sets.rb | 10 +-- .../aws/requests/dns/create_hosted_zone.rb | 4 +- .../aws/requests/dns/delete_hosted_zone.rb | 12 ++-- lib/fog/aws/requests/dns/get_change.rb | 27 +++---- lib/fog/aws/requests/dns/get_hosted_zone.rb | 4 +- .../requests/dns/list_resource_record_sets.rb | 18 ++--- tests/requests/dns/dns_tests.rb | 71 ++++++++----------- 10 files changed, 73 insertions(+), 92 deletions(-) diff --git a/lib/fog/aws/dns.rb b/lib/fog/aws/dns.rb index 0b653b613..3ca88fb01 100644 --- a/lib/fog/aws/dns.rb +++ b/lib/fog/aws/dns.rb @@ -79,7 +79,7 @@ module Fog # :aws_secret_access_key in order to create a connection # # ==== Examples - # dns = Fog::AWS::DNS.new( + # dns = Fog::DNS::AWS.new( # :aws_access_key_id => your_aws_access_key_id, # :aws_secret_access_key => your_aws_secret_access_key # ) @@ -141,6 +141,19 @@ module Fog def _request(params, &block) @connection.request(params, &block) + rescue Excon::Errors::HTTPStatusError => error + match = Fog::AWS::Errors.match_error(error) + + if match.empty? + raise + else + case match[:code] + when 'NoSuchHostedZone' then + Fog::DNS::AWS::NotFound.slurp(error, match[:message]) + else + Fog::DNS::AWS::Error.slurp(error, "#{match[:code]} => #{match[:message]}") + end + end end def signature(params) diff --git a/lib/fog/aws/models/dns/records.rb b/lib/fog/aws/models/dns/records.rb index a5aa16a59..945f5536d 100644 --- a/lib/fog/aws/models/dns/records.rb +++ b/lib/fog/aws/models/dns/records.rb @@ -100,7 +100,7 @@ module Fog record end end.compact.first - rescue Excon::Errors::NotFound + rescue Fog::DNS::AWS::NotFound nil end diff --git a/lib/fog/aws/models/dns/zones.rb b/lib/fog/aws/models/dns/zones.rb index cffccbca1..b3a8c4203 100644 --- a/lib/fog/aws/models/dns/zones.rb +++ b/lib/fog/aws/models/dns/zones.rb @@ -20,7 +20,7 @@ module Fog def get(zone_id) data = service.get_hosted_zone(zone_id).body new(data) - rescue Excon::Errors::NotFound + rescue Fog::DNS::AWS::NotFound nil end end diff --git a/lib/fog/aws/requests/dns/change_resource_record_sets.rb b/lib/fog/aws/requests/dns/change_resource_record_sets.rb index efa1ac6ea..5628657be 100644 --- a/lib/fog/aws/requests/dns/change_resource_record_sets.rb +++ b/lib/fog/aws/requests/dns/change_resource_record_sets.rb @@ -193,7 +193,7 @@ module Fog errors << "Tried to create resource record set #{change[:name]}. type #{change[:type]}, but it already exists" end when "DELETE" - if zone[:records][change[:type]].nil? || zone[:records][change[:type]].delete(change[:name]).nil? + if zone[:records][change[:type]].nil? || zone[:records][change[:type]].delete(change_name).nil? errors << "Tried to delete resource record set #{change[:name]}. type #{change[:type]}, but it was not found" end end @@ -213,14 +213,10 @@ module Fog } response else - response.status = 400 - response.body = "#{errors.map {|e| "#{e}"}.join()}" - raise(Excon::Errors.status_error({:expects => 200}, response)) + raise Fog::DNS::AWS::Error.new("InvalidChangeBatch => #{errors.join(", ")}") end else - response.status = 404 - response.body = "NoSuchHostedZoneA hosted zone with the specified hosted zone ID does not exist.#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) + raise Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist.") end end end diff --git a/lib/fog/aws/requests/dns/create_hosted_zone.rb b/lib/fog/aws/requests/dns/create_hosted_zone.rb index ad8218aae..b4cb8d48f 100644 --- a/lib/fog/aws/requests/dns/create_hosted_zone.rb +++ b/lib/fog/aws/requests/dns/create_hosted_zone.rb @@ -97,9 +97,7 @@ module Fog } response else - response.status = 400 - response.body = "DelegationSetNotAvailableAmazon Route 53 allows some duplication, but Amazon Route 53 has a maximum threshold of duplicated domains. This error is generated when you reach that threshold. In this case, the error indicates that too many hosted zones with the given domain name exist. If you want to create a hosted zone and Amazon Route 53 generates this error, contact Customer Support.#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) + raise Fog::DNS::AWS::Error.new("DelegationSetNotAvailable => Amazon Route 53 allows some duplication, but Amazon Route 53 has a maximum threshold of duplicated domains. This error is generated when you reach that threshold. In this case, the error indicates that too many hosted zones with the given domain name exist. If you want to create a hosted zone and Amazon Route 53 generates this error, contact Customer Support.") end end end diff --git a/lib/fog/aws/requests/dns/delete_hosted_zone.rb b/lib/fog/aws/requests/dns/delete_hosted_zone.rb index a783b87eb..f515106a3 100644 --- a/lib/fog/aws/requests/dns/delete_hosted_zone.rb +++ b/lib/fog/aws/requests/dns/delete_hosted_zone.rb @@ -36,14 +36,17 @@ module Fog def delete_hosted_zone(zone_id) response = Excon::Response.new - key = [zone_id, "/hostedzone/#{zone_id}"].find{|k| !self.data[:zones][k].nil?} - if key + key = [zone_id, "/hostedzone/#{zone_id}"].find { |k| !self.data[:zones][k].nil? } || + raise(Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone does not exist.")) + change = { :id => Fog::AWS::Mock.change_id, :status => 'INSYNC', :submitted_at => Time.now.utc.iso8601 } + self.data[:changes][change[:id]] = change + response.status = 200 response.body = { 'ChangeInfo' => { @@ -54,11 +57,6 @@ module Fog } self.data[:zones].delete(key) response - else - response.status = 404 - response.body = "SenderNoSuchHostedZoneThe specified hosted zone does not exist.#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) - end end end end diff --git a/lib/fog/aws/requests/dns/get_change.rb b/lib/fog/aws/requests/dns/get_change.rb index 678513924..f66eda400 100644 --- a/lib/fog/aws/requests/dns/get_change.rb +++ b/lib/fog/aws/requests/dns/get_change.rb @@ -35,23 +35,18 @@ module Fog response = Excon::Response.new # find the record with matching change_id # records = data[:zones].values.map{|z| z[:records].values.map{|r| r.values}}.flatten - change = self.data[:changes][change_id] + change = self.data[:changes][change_id] || + raise(Fog::DNS::AWS::NotFound.new("NoSuchChange => Could not find resource with ID: #{change_id}")) - if change - response.status = 200 - submitted_at = Time.parse(change[:submitted_at]) - response.body = { - 'Id' => change[:id], - # set as insync after some time - 'Status' => (submitted_at + Fog::Mock.delay) < Time.now ? 'INSYNC' : change[:status], - 'SubmittedAt' => change[:submitted_at] - } - response - else - response.status = 404 - response.body = "SenderNoSuchChangeCould not find resource with ID: #{change_id}#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) - end + response.status = 200 + submitted_at = Time.parse(change[:submitted_at]) + response.body = { + 'Id' => change[:id], + # set as insync after some time + 'Status' => (submitted_at + Fog::Mock.delay) < Time.now ? 'INSYNC' : change[:status], + 'SubmittedAt' => change[:submitted_at] + } + response end end end diff --git a/lib/fog/aws/requests/dns/get_hosted_zone.rb b/lib/fog/aws/requests/dns/get_hosted_zone.rb index b6cb89007..32918f32c 100644 --- a/lib/fog/aws/requests/dns/get_hosted_zone.rb +++ b/lib/fog/aws/requests/dns/get_hosted_zone.rb @@ -51,9 +51,7 @@ module Fog } response else - response.status = 404 - response.body = "SenderNoSuchHostedZoneThe specified hosted zone does not exist.#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) + raise Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist.") end end end diff --git a/lib/fog/aws/requests/dns/list_resource_record_sets.rb b/lib/fog/aws/requests/dns/list_resource_record_sets.rb index 8aa4d1f32..4425b7c14 100644 --- a/lib/fog/aws/requests/dns/list_resource_record_sets.rb +++ b/lib/fog/aws/requests/dns/list_resource_record_sets.rb @@ -64,19 +64,15 @@ module Fog response = Excon::Response.new - zone = self.data[:zones][zone_id] - if zone.nil? - response.status = 404 - response.body = "\nSenderNoSuchHostedZoneNo hosted zone found with ID: #{zone_id}#{Fog::AWS::Mock.request_id}" - raise(Excon::Errors.status_error({:expects => 200}, response)) - end + zone = self.data[:zones][zone_id] || + raise(Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist.")) records = if options[:type] - records_type = zone[:records][options[:type]] - records_type.values if records_type - else - zone[:records].values.map{|r| r.values}.flatten - end + records_type = zone[:records][options[:type]] + records_type.values if records_type + else + zone[:records].values.map{|r| r.values}.flatten + end records ||= [] diff --git a/tests/requests/dns/dns_tests.rb b/tests/requests/dns/dns_tests.rb index 1f9f68e44..ff4617d0f 100644 --- a/tests/requests/dns/dns_tests.rb +++ b/tests/requests/dns/dns_tests.rb @@ -119,13 +119,12 @@ Shindo.tests('Fog::DNS[:aws] | DNS requests', ['aws', 'dns']) do change_batch << resource_record_set options = { :comment => 'add A record to domain'} response = @r53_connection.change_resource_record_sets(@zone_id, change_batch, options) - if response.status == 200 - change_id = response.body['Id'] - status = response.body['Status'] - @new_records << resource_record - end - response.status == 200 + Fog.wait_for { @r53_connection.get_change(response.body["Id"]).body["Status"] != "PENDING" } + + @new_records << resource_record + + @r53_connection.get_change(response.body["Id"]).body["Status"] == "INSYNC" } test("add a CNAME resource record") { @@ -139,13 +138,12 @@ Shindo.tests('Fog::DNS[:aws] | DNS requests', ['aws', 'dns']) do change_batch << resource_record_set options = { :comment => 'add CNAME record to domain'} response = @r53_connection.change_resource_record_sets( @zone_id, change_batch, options) - if response.status == 200 - change_id = response.body['Id'] - status = response.body['Status'] - @new_records << resource_record - end - response.status == 200 + Fog.wait_for { @r53_connection.get_change(response.body["Id"]).body["Status"] != "PENDING" } + + @new_records << resource_record + + @r53_connection.get_change(response.body["Id"]).body["Status"] == "INSYNC" } test("add a MX resource record") { @@ -159,13 +157,12 @@ Shindo.tests('Fog::DNS[:aws] | DNS requests', ['aws', 'dns']) do change_batch << resource_record_set options = { :comment => 'add MX record to domain'} response = @r53_connection.change_resource_record_sets( @zone_id, change_batch, options) - if response.status == 200 - change_id = response.body['Id'] - status = response.body['Status'] - @new_records << resource_record - end - response.status == 200 + Fog.wait_for { @r53_connection.get_change(response.body["Id"]).body["Status"] != "PENDING" } + + @new_records << resource_record + + @r53_connection.get_change(response.body["Id"]).body["Status"] == "INSYNC" } test("add an ALIAS resource record") { @@ -195,47 +192,37 @@ Shindo.tests('Fog::DNS[:aws] | DNS requests', ['aws', 'dns']) do puts "DNS Name (ELB): #{dns_name}" puts "Zone ID for Route 53: #{@zone_id}" - sleep 120 unless Fog.mocking? response = @r53_connection.change_resource_record_sets(@zone_id, change_batch, options) - if response.status == 200 - change_id = response.body['Id'] - status = response.body['Status'] - @new_records << resource_record - end - response.status == 200 + Fog.wait_for { @r53_connection.get_change(response.body["Id"]).body["Status"] != "PENDING" } + + @new_records << resource_record + + @r53_connection.get_change(response.body["Id"]).body["Status"] == "INSYNC" } + tests("list resource records").formats(AWS::DNS::Formats::LIST_RESOURCE_RECORD_SETS) { # get resource records for zone @r53_connection.list_resource_record_sets(@zone_id).body } test("delete #{@new_records.count} resource records") { - result = true + change_batch = @new_records.map { |record| record.merge(:action => 'DELETE') } + options = { :comment => 'remove records from domain'} - change_batch = [] - @new_records.each { |record| - resource_record_set = record.merge( :action => 'DELETE') - change_batch << resource_record_set - } - options = { :comment => 'remove records from domain'} response = @r53_connection.change_resource_record_sets(@zone_id, change_batch, options) - if response.status != 200 - result = false - break - end - result + Fog.wait_for { @r53_connection.get_change(response.body["Id"]).body["Status"] != "PENDING" } + + @r53_connection.get_change(response.body["Id"]).body["Status"] == "INSYNC" } test("delete hosted zone #{@zone_id}") { # cleanup the ELB as well @elb_connection.delete_load_balancer("fog") - response = @r53_connection.delete_hosted_zone(@zone_id) - - response.status == 200 + @r53_connection.delete_hosted_zone(@zone_id).status == 200 } end @@ -243,13 +230,13 @@ Shindo.tests('Fog::DNS[:aws] | DNS requests', ['aws', 'dns']) do tests('failure') do tests('create hosted zone using invalid domain name').raises(Excon::Errors::BadRequest) do pending if Fog.mocking? - response = @r53_connection.create_hosted_zone('invalid-domain') + @r53_connection.create_hosted_zone('invalid-domain') end tests('get hosted zone using invalid ID').raises(Excon::Errors::NotFound) do pending if Fog.mocking? zone_id = 'dummy-id' - response = @r53_connection.get_hosted_zone(zone_id) + @r53_connection.get_hosted_zone(zone_id) end end From a4e70582be9bae8cb0c79a2002bfb4b0adaf6a5c Mon Sep 17 00:00:00 2001 From: Josh Lane Date: Sat, 21 Feb 2015 13:16:31 -0800 Subject: [PATCH 4/4] NoSuchChange is a 404 response --- lib/fog/aws/dns.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fog/aws/dns.rb b/lib/fog/aws/dns.rb index 3ca88fb01..8d5491572 100644 --- a/lib/fog/aws/dns.rb +++ b/lib/fog/aws/dns.rb @@ -148,7 +148,7 @@ module Fog raise else case match[:code] - when 'NoSuchHostedZone' then + when 'NoSuchHostedZone', 'NoSuchChange' then Fog::DNS::AWS::NotFound.slurp(error, match[:message]) else Fog::DNS::AWS::Error.slurp(error, "#{match[:code]} => #{match[:message]}")