Merge pull request #33 from github/kpaulisse-double-escape-facts

Double escape facts

.version

@@ -1 +1 @@
-0.5.6
+0.5.7

lib/octocatalog-diff/catalog/puppetmaster.rb

@@ -68,13 +68,17 @@ def build(logger = Logger.new(StringIO.new))
 
       # Returns a hash of parameters for each supported version of the Puppet Server Catalog API.
       # @return [Hash] Hash of parameters
+      #
+      # Note: The double escaping of the facts here is implemented to correspond to a long standing
+      # bug in the Puppet code. See https://github.com/puppetlabs/puppet/pull/1818 and
+      # https://docs.puppet.com/puppet/latest/http_api/http_catalog.html#parameters for explanation.
       def puppet_catalog_api
         {
           2 => {
             url: "https://#{@options[:puppet_master]}/#{@options[:branch]}/catalog/#{@node}",
             parameters: {
               'facts_format' => 'pson',
-              'facts' => @facts.fudge_timestamp.without('trusted').to_pson,
+              'facts' => CGI.escape(@facts.fudge_timestamp.without('trusted').to_pson),
               'transaction_uuid' => SecureRandom.uuid
             }
           },
@@ -83,7 +87,7 @@ def puppet_catalog_api
             parameters: {
               'environment' => @options[:branch],
               'facts_format' => 'pson',
-              'facts' => @facts.fudge_timestamp.without('trusted').to_pson,
+              'facts' => CGI.escape(@facts.fudge_timestamp.without('trusted').to_pson),
               'transaction_uuid' => SecureRandom.uuid
             }
           }

spec/octocatalog-diff/fixtures/facts/facts_esc.json

@@ -0,0 +1,9 @@
+{
+  "apt_update_last_success":1458162123,
+  "architecture":"amd64",
+  "datacenter":"xyz",
+  "fqdn":"rspec-node.xyz.github.net",
+  "math":"1+2=3",
+  "percent":"25%20=5",
+  "_timestamp":"2014-12-02 14:56:20 -0600"
+}

spec/octocatalog-diff/fixtures/facts/facts_esc.yaml

@@ -0,0 +1,11 @@
+#--- !ruby/object:Puppet::Node::Facts
+name: rspec-node.xyz.github.net
+values:
+  apt_update_last_success: 1458162123
+  architecture: amd64
+  datacenter: xyz
+  fqdn: rspec-node.xyz.github.net
+  math: "1+2=3"
+  percent: "25%20=5"
+  "_timestamp": 2014-12-02 12:56:20.865795 -08:00
+expiration: 2014-12-02 13:11:20.521667 -08:00

spec/octocatalog-diff/integration/puppetmaster_spec.rb

@@ -45,7 +45,7 @@ def self.parse_body(body)
       end
 
       def self.facts_match(body)
-        facts = JSON.parse(parse_body(body)['facts'])
+        facts = JSON.parse(CGI.unescape(parse_body(body)['facts']))
         facts.delete('_timestamp')
         desired_facts = {
           'name' => 'rspec-node.xyz.github.net',

spec/octocatalog-diff/tests/catalog/puppetmaster_spec.rb

@@ -11,7 +11,7 @@
       node: 'foo',
       branch: 'foobranch',
       puppet_master: 'fake-puppetmaster.non-existent-domain.com',
-      fact_file: OctocatalogDiff::Spec.fixture_path('facts/facts.yaml')
+      fact_file: OctocatalogDiff::Spec.fixture_path('facts/facts_esc.yaml')
     }
   end
 
@@ -99,9 +99,12 @@
           end
 
           it 'should post the correct facts to HTTParty' do
-            answer = JSON.parse(File.read(OctocatalogDiff::Spec.fixture_path('facts/facts.json')))
+            answer = JSON.parse(File.read(OctocatalogDiff::Spec.fixture_path('facts/facts_esc.json')))
             answer.delete('_timestamp')
-            result = JSON.parse(@post_data['facts'])['values']
+            # An extra 'unescape' is here because the facts are double escaped.
+            # See https://docs.puppet.com/puppet/latest/http_api/http_catalog.html#parameters
+            # and https://github.com/puppetlabs/puppet/pull/1818
+            result = JSON.parse(CGI.unescape(@post_data['facts']))['values']
             expect(result).to eq(answer)
           end
 
@@ -118,8 +121,8 @@
           it 'should log correctly' do
             logs = @logger_str.string
             expect(logs).to match(/Start retrieving facts for foo from OctocatalogDiff::Catalog::PuppetMaster/)
-            expect(logs).to match(%r{Retrieving facts from.*fixtures/facts/facts.yaml})
-            expect(logs).to match(%r{Retrieving facts from.*fixtures/facts/facts.yaml})
+            expect(logs).to match(%r{Retrieving facts from.*fixtures/facts/facts_esc.yaml})
+            expect(logs).to match(%r{Retrieving facts from.*fixtures/facts/facts_esc.yaml})
 
             answer = Regexp.new("Retrieve catalog from #{api_url[api_version]} environment foobranch")
             expect(logs).to match(answer)