Fix #1483 - use REST to transmit reports over the wire

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

8 files changed, 254 insertions, 24 deletions

diff --git a/bin/puppetd b/bin/puppetd
index efd182426..ecea2d894 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -327,6 +327,8 @@ end
 # be local.
 Puppet::SSL::Host.ca_location = :remote
 
+Puppet::Transaction::Report.terminus_class = :rest
+
 # We need tomake the client either way, we just don't start it
 # if --no-client is set.
 client = Puppet::Network::Client.master.new(args)
diff --git a/ext/puppet-test b/ext/puppet-test
index dc0aeca92..41759d489 100755
--- a/ext/puppet-test
+++ b/ext/puppet-test
@@ -364,6 +364,49 @@ Suite.new :rails, "Rails Interactions" do
     end
 end
 
+Suite.new :report, "Reports interactions" do
+    def prepare
+        Puppet::Transaction::Report.terminus_class = :rest
+    end
+
+    newtest :empty, "send empty report" do
+        report = Puppet::Transaction::Report.new
+        report.time = Time.now
+        report.save
+    end
+
+    newtest :fake, "send fake report" do
+        report = Puppet::Transaction::Report.new
+
+        resourcemetrics = {
+            :total => 12,
+            :out_of_sync => 20,
+            :applied => 45,
+            :skipped => 1,
+            :restarted => 23,
+            :failed_restarts => 1,
+            :scheduled => 10
+        }
+        report.newmetric(:resources, resourcemetrics)
+
+        timemetrics = {
+            :resource1 => 10,
+            :resource2 => 50,
+            :resource3 => 40,
+            :resource4 => 20,
+        }
+        report.newmetric(:times, timemetrics)
+
+        report.newmetric(:changes,
+            :total => 20
+        )
+
+        report.time = Time.now
+        report.save
+    end
+end
+
+
 $cmdargs = [
 	[ "--compile",    "-c",			GetoptLong::NO_ARGUMENT ],
 	[ "--describe",     			GetoptLong::REQUIRED_ARGUMENT ],
diff --git a/lib/puppet/indirector/report/rest.rb b/lib/puppet/indirector/report/rest.rb
new file mode 100644
index 000000000..905b71a5d
--- /dev/null
+++ b/lib/puppet/indirector/report/rest.rb
@@ -0,0 +1,5 @@
+require 'puppet/indirector/rest'
+
+class Puppet::Transaction::Report::Rest < Puppet::Indirector::REST
+    desc "Get server report over HTTP via REST."
+end
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 0f94f5a12..83017222f 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -505,23 +505,13 @@ class Transaction
         
         if Puppet[:report]
             begin
-                reportclient().report(report)
+                report.save()
             rescue => detail
                 Puppet.err "Reporting failed: %s" % detail
             end
         end
     end
 
-    def reportclient
-        unless defined? @reportclient
-            @reportclient = Puppet::Network::Client.report.new(
-                :Server => Puppet[:reportserver]
-            )
-        end
-
-        @reportclient
-    end
-
     # Roll all completed changes back.
     def rollback
         @targets.clear
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 89da7ed9c..e5b8650bb 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -11,7 +11,13 @@ class Puppet::Transaction::Report
     indirects :report, :terminus_class => :processor
 
     attr_accessor :logs, :metrics, :time, :host
-    
+
+    # This is necessary since Marshall doesn't know how to
+    # dump hash with default proc (see below @records)
+    def self.default_format
+        :yaml
+    end
+
     def <<(msg)
         @logs << msg
         return self
diff --git a/spec/integration/indirector/report/rest.rb b/spec/integration/indirector/report/rest.rb
new file mode 100644
index 000000000..7aa063f2f
--- /dev/null
+++ b/spec/integration/indirector/report/rest.rb
@@ -0,0 +1,91 @@
+#!/usr/bin/env ruby
+
+Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
+
+require 'puppet/transaction/report'
+require 'puppet/network/server'
+require 'puppet/network/http/webrick/rest'
+
+describe "Report REST Terminus" do
+    before do
+        Puppet[:masterport] = 34343
+        Puppet[:server] = "localhost"
+
+        # Get a safe temporary file
+        @tmpfile = Tempfile.new("webrick_integration_testing")
+        @dir = @tmpfile.path + "_dir"
+
+        Puppet.settings[:confdir] = @dir
+        Puppet.settings[:vardir] = @dir
+        Puppet.settings[:server] = "127.0.0.1"
+        Puppet.settings[:masterport] = "34343"
+        Puppet.settings[:http_enable_post_connection_check] = false
+
+        Puppet::Util::Cacher.expire
+
+        Puppet[:servertype] = 'webrick'
+        Puppet[:server] = '127.0.0.1'
+        Puppet[:certname] = '127.0.0.1'
+
+        # Generate the certificate with a local CA
+        Puppet::SSL::Host.ca_location = :local
+        ca = Puppet::SSL::CertificateAuthority.new
+        ca.generate(Puppet[:certname]) unless Puppet::SSL::Certificate.find(Puppet[:certname])
+        ca.generate("foo.madstop.com") unless Puppet::SSL::Certificate.find(Puppet[:certname])
+
+        @host = Puppet::SSL::Host.new(Puppet[:certname])
+
+        @params = { :address => "127.0.0.1", :port => 34343, :handlers => [ :report ] }
+        @server = Puppet::Network::Server.new(@params)
+        @server.listen
+
+        # Let's use REST for our reports :-)
+        Puppet::Transaction::Report.terminus_class = :rest
+
+        # LAK:NOTE We need to have a fake model here so that our indirected methods get
+        # passed through REST; otherwise we'd be stubbing 'save', which would cause an immediate
+        # return.
+        @report = stub_everything 'report'
+        @mock_model = stub_everything 'faked model', :name => "report", :convert_from => @report
+        Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:model).returns(@mock_model)
+    end
+
+    after do
+        Puppet::Network::HttpPool.expire
+        Puppet::SSL::Host.ca_location = :none
+        Puppet.settings.clear
+        @server.unlisten
+    end
+
+    it "should be able to send a report to the server" do
+        @report.expects(:save)
+
+        report = Puppet::Transaction::Report.new
+
+        resourcemetrics = {
+            :total => 12,
+            :out_of_sync => 20,
+            :applied => 45,
+            :skipped => 1,
+            :restarted => 23,
+            :failed_restarts => 1,
+            :scheduled => 10
+        }
+        report.newmetric(:resources, resourcemetrics)
+
+        timemetrics = {
+            :resource1 => 10,
+            :resource2 => 50,
+            :resource3 => 40,
+            :resource4 => 20,
+        }
+        report.newmetric(:times, timemetrics)
+
+        report.newmetric(:changes,
+            :total => 20
+        )
+
+        report.time = Time.now
+        report.save
+    end
+end
diff --git a/spec/unit/indirector/report/rest.rb b/spec/unit/indirector/report/rest.rb
new file mode 100644
index 000000000..a51ebca2a
--- /dev/null
+++ b/spec/unit/indirector/report/rest.rb
@@ -0,0 +1,11 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+require 'puppet/indirector/report/rest'
+
+describe Puppet::Transaction::Report::Rest do
+    it "should be a sublcass of Puppet::Indirector::REST" do
+        Puppet::Transaction::Report::Rest.superclass.should equal(Puppet::Indirector::REST)
+    end
+end
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 196f8cbb8..2c1998587 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -5,20 +5,24 @@ require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/transaction'
 
 describe Puppet::Transaction do
-    describe "when generating resources" do
-        before do
-            @generator_class = mkgenerator
-            @generator = mkgenerator.create(:name => "foo")
+    before do
+        @generator_class = mkgenerator
+        @generator = mkgenerator.create(:name => "foo")
 
-            @catalog = Puppet::Node::Catalog.new
-            @catalog.add_resource @generator
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @generator
 
-            @transaction = Puppet::Transaction.new(@catalog)
-        end
+        @report = stub_everything 'report'
+        Puppet::Transaction::Report.stubs(:new).returns(@report)
 
-        after do
-            Puppet::Type.rmtype(:generator)
-        end
+        @transaction = Puppet::Transaction.new(@catalog)
+    end
+
+    after do
+        Puppet::Type.rmtype(:generator)
+    end
+
+    describe "when generating resources" do
 
         it "should call the generate() method on all resources" do
             @generator.expects(:generate)
@@ -102,8 +106,86 @@ describe Puppet::Transaction do
                 @transaction.eval_generate(@generator)
             end
         end
+
     end
-    
+
+    describe "when generating a report" do
+
+        before :each do
+            Puppet.stubs(:[]).with(:report).returns(true)
+        end
+
+        it "should create a Puppet::Transaction::Report when the Transaction is created" do
+            Puppet::Transaction::Report.expects(:new).returns(@report)
+
+            Puppet::Transaction.new(@catalog)
+        end
+
+        it "should return a Puppet::Transaction::Report" do
+            @transaction.generate_report.should == @report
+        end
+
+        it "should have a metric for resources" do
+            @report.expects(:newmetric).with { |metric,hash| metric == :resources }
+
+            @transaction.generate_report
+        end
+
+        it "should have a metric for time" do
+            @report.expects(:newmetric).with { |metric,hash| metric == :time }
+
+            @transaction.generate_report
+        end
+
+        it "should have a metric for changes" do
+            @report.expects(:newmetric).with { |metric,hash| metric == :changes }
+
+            @transaction.generate_report
+        end
+
+        it "should store the current time" do
+            now = stub 'time'
+            Time.stubs(:now).returns(now)
+
+            @report.expects(:time=).with(now)
+
+            @transaction.generate_report
+        end
+
+    end
+
+    describe "when sending a report" do
+
+        before :each do
+            @transaction.stubs(:generate_report).returns(@report)
+            Puppet.stubs(:[]).with(:report).returns(true)
+            Puppet.stubs(:[]).with(:rrdgraph).returns(false)
+            Puppet.stubs(:[]).with(:summarize).returns(false)
+        end
+
+        it "should ask the transaction for a report" do
+            @transaction.expects(:generate_report)
+
+            @transaction.send_report
+        end
+
+        it "should ask the report for a graph if rrdgraph is enable" do
+            Puppet.stubs(:[]).with(:rrdgraph).returns(true)
+
+            @report.expects(:graph)
+
+            @transaction.send_report
+        end
+
+
+        it "should call report.save" do
+            @report.expects(:save)
+
+            @transaction.send_report
+        end
+
+    end
+
     def mkgenerator
         # Create a bogus type that generates new instances with shorter names
         type = Puppet::Type.newtype(:generator) do