Merge pull request #57 from lanej/fix-dns-get

[dns] fix Records#get, mock records and proper errors

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', 'NoSuchChange' 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)

lib/fog/aws/models/dns/records.rb

@@ -89,24 +89,18 @@ module Fog
           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
+          # look for an exact match in the records
-            record = new(data)
+          (data['ResourceRecordSets'] || []).map do |record_data|
-            # make sure everything matches
+            record = new(record_data)
-            if record.name == record_name
+
-              if (!record_type.nil? && record.type != record_type) ||
+            if (record.name == record_name) &&
-                  (!record_identifier.nil? && record.set_identifier != record_identifier)
+                (record_type.nil? || (record.type == record_type)) &&
-                nil
+                (record_identifier.nil? || (record.set_identifier == record_identifier))
-              else
               record
             end
-            end
+          end.compact.first
-          else
+        rescue Fog::DNS::AWS::NotFound
-            nil
-          end
-        rescue Excon::Errors::NotFound
           nil
         end
 

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

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,17 +183,17 @@ module Fog
                       :ttl => change[:ttl].to_s,
                     }
                   end
-                  zone[:records][change[:type]][change[:name]] = {
+                  zone[:records][change[:type]][change_name] = {
                     :change_id        => change_id,
                     :resource_records => change[:resource_records] || [],
-                    :name => change[:name],
+                    :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"
                 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
@@ -208,23 +213,18 @@ module Fog
               }
               response
             else
-              response.status = 400
+              raise Fog::DNS::AWS::Error.new("InvalidChangeBatch => #{errors.join(", ")}")
-              response.body = "<?xml version=\"1.0\"?><InvalidChangeBatch xmlns=\"https://route53.amazonaws.com/doc/2012-02-29/\"><Messages>#{errors.map {|e| "<Message>#{e}</Message>"}.join()}</Messages></InvalidChangeBatch>"
-              raise(Excon::Errors.status_error({:expects => 200}, response))
             end
           else
-            response.status = 404
+            raise Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist.")
-            response.body = "<?xml version=\"1.0\"?><Response><Errors><Error><Code>NoSuchHostedZone</Code><Message>A hosted zone with the specified hosted zone ID does not exist.</Message></Error></Errors><RequestID>#{Fog::AWS::Mock.request_id}</RequestID></Response>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
           end
         end
       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
+        }.last
-        elb_hosted_zone_mapping[k]
       end
 
       def self.elb_hosted_zone_mapping

lib/fog/aws/requests/dns/create_hosted_zone.rb

@@ -97,9 +97,7 @@ module Fog
             }
             response
           else
-            response.status = 400
+            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.")
-            response.body = "<?xml version=\"1.0\"?><Response><Errors><Error><Code>DelegationSetNotAvailable</Code><Message>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.</Message></Error></Errors><RequestID>#{Fog::AWS::Mock.request_id}</RequestID></Response>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
           end
         end
       end

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?}
+          key = [zone_id, "/hostedzone/#{zone_id}"].find { |k| !self.data[:zones][k].nil? } ||
-          if key
+            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 = "<?xml version=\"1.0\"?><ErrorResponse xmlns=\"https://route53.amazonaws.com/doc/2012-02-29/\"><Error><Type>Sender</Type><Code>NoSuchHostedZone</Code><Message>The specified hosted zone does not exist.</Message></Error><RequestId>#{Fog::AWS::Mock.request_id}</RequestId></ErrorResponse>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
-          end
         end
       end
     end

lib/fog/aws/requests/dns/get_change.rb

@@ -35,9 +35,9 @@ 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 = {
@@ -47,11 +47,6 @@ module Fog
             'SubmittedAt' => change[:submitted_at]
           }
           response
-          else
-            response.status = 404
-            response.body = "<?xml version=\"1.0\"?><ErrorResponse xmlns=\"https://route53.amazonaws.com/doc/2012-02-29/\"><Error><Type>Sender</Type><Code>NoSuchChange</Code><Message>Could not find resource with ID: #{change_id}</Message></Error><RequestId>#{Fog::AWS::Mock.request_id}</RequestId></ErrorResponse>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
-          end
         end
       end
     end

lib/fog/aws/requests/dns/get_hosted_zone.rb

@@ -51,9 +51,7 @@ module Fog
             }
             response
           else
-            response.status = 404
+            raise Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist.")
-            response.body = "<?xml version=\"1.0\"?><ErrorResponse xmlns=\"https://route53.amazonaws.com/doc/2012-02-29/\"><Error><Type>Sender</Type><Code>NoSuchHostedZone</Code><Message>The specified hosted zone does not exist.</Message></Error><RequestId>#{Fog::AWS::Mock.request_id}</RequestId></ErrorResponse>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
           end
         end
       end

lib/fog/aws/requests/dns/list_resource_record_sets.rb

@@ -64,12 +64,8 @@ module Fog
 
           response = Excon::Response.new
 
-          zone = self.data[:zones][zone_id]
+          zone = self.data[:zones][zone_id] ||
-          if zone.nil?
+            raise(Fog::DNS::AWS::NotFound.new("NoSuchHostedZone => A hosted zone with the specified hosted zone ID does not exist."))
-            response.status = 404
-            response.body = "<?xml version=\"1.0\"?>\n<ErrorResponse xmlns=\"https://route53.amazonaws.com/doc/2012-02-29/\"><Error><Type>Sender</Type><Code>NoSuchHostedZone</Code><Message>No hosted zone found with ID: #{zone_id}</Message></Error><RequestId>#{Fog::AWS::Mock.request_id}</RequestId></ErrorResponse>"
-            raise(Excon::Errors.status_error({:expects => 200}, response))
-          end
 
           records = if options[:type]
                       records_type = zone[:records][options[:type]]
@@ -86,7 +82,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]

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') }
-
-      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
+      response = @r53_connection.change_resource_record_sets(@zone_id, change_batch, options)
+
+      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)
+      @r53_connection.delete_hosted_zone(@zone_id).status == 200
-
-      response.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