Have done a significant reorganization of how clients work, also, along with some interesting trouble shooting on components

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@585 980ebf18-57e1-0310-9a29-db15c13687c0

7 files changed, 247 insertions, 301 deletions

diff --git a/lib/puppet/client.rb b/lib/puppet/client.rb
index bc5869a76..ee2645852 100644
--- a/lib/puppet/client.rb
+++ b/lib/puppet/client.rb
@@ -40,6 +40,7 @@ module Puppet
 
                 interface.methods.each { |ary|
                     method = ary[0]
+                    Puppet.info "Defining %s.%s" % [namespace, method]
                     self.send(:define_method,method) { |*args|
                         #Puppet.info "peer cert is %s" % @http.peer_cert
                         #Puppet.info "cert is %s" % @http.cert
@@ -113,18 +114,19 @@ module Puppet
         end
     end
 
+    # FIXME this still isn't a good design, because none of the handlers overlap
+    # so i could just as easily include them all in the main module
+    # but at least it's better organized for now
     class Client
         include Puppet
-        include Puppet::Daemon
-        attr_accessor :local, :secureinit
 
-        def Client.facts
-            facts = {}
-            Facter.each { |name,fact|
-                facts[name] = fact.downcase
-            }
+        # FIXME the cert stuff should only come up with networking, so it
+        # should be in the network client, not the normal client
+        include Puppet::Daemon
+        attr_reader :local, :secureinit
 
-            facts
+        class << self
+            attr_reader :drivername
         end
 
         def initialize(hash)
@@ -145,186 +147,254 @@ module Puppet
                 @cache = true
             end
 
+            driverparam = self.class.drivername
             if hash.include?(:Server)
-                case hash[:Server]
-                when String:
-                    if $noclientnetworking
-                        raise NetworkClientError.new("Networking not available: %s" %
-                            $nonetworking)
-                    end
-
-                    args = {}
-                    [:Port, :Server].each { |arg|
-                        if hash.include?(:Port)
-                            args[arg] = hash[arg]
-                        end
-                    }
+                if $noclientnetworking
+                    raise NetworkClientError.new("Networking not available: %s" %
+                        $nonetworking)
+                end
 
-                    if self.readcert
-                        args[:Certificate] = @cert
-                        args[:Key] = @key
-                        args[:CAFile] = @cacertfile
+                args = {}
+                [:Port, :Server].each { |arg|
+                    if hash.include?(:Port)
+                        args[arg] = hash[arg]
                     end
+                }
 
-                    @driver = Puppet::NetworkClient.new(args)
-                    @local = false
-                when Puppet::Server::Master:
-                    @driver = hash[:Server]
-                    @local = true
-                else
-                    raise ClientError.new("Server must be a hostname or a " +
-                        "Puppet::Server::Master object")
+                if self.readcert
+                    args[:Certificate] = @cert
+                    args[:Key] = @key
+                    args[:CAFile] = @cacertfile
                 end
+
+                @driver = Puppet::NetworkClient.new(args)
+                @local = false
+            elsif hash.include?(driverparam)
+                @driver = hash[driverparam]
+                @local = true
             else
-                raise ClientError.new("Must pass :Server to client")
+                raise ClientError, "%s must be passed a Server or %s" %
+                    [self.class, driverparam]
             end
         end
 
-        def getconfig
-            #client.loadproperty('files/sslclient.properties')
-            Puppet.debug("getting config")
+        class MasterClient < Puppet::Client
+            @drivername = :Master
 
-            facts = Client.facts
+            def self.facts
+                facts = {}
+                Facter.each { |name,fact|
+                    facts[name] = fact.downcase
+                }
 
-            unless facts.length > 0
-                raise Puppet::ClientError.new(
-                    "Could not retrieve any facts"
-                )
+                facts
             end
 
-            objects = nil
-            if @local
-                objects = @driver.getconfig(facts)
+            def getconfig
+                #client.loadproperty('files/sslclient.properties')
+                Puppet.debug("getting config")
+
+                facts = self.class.facts
 
-                if objects == ""
-                    raise Puppet::Error, "Could not retrieve configuration"
+                unless facts.length > 0
+                    raise Puppet::ClientError.new(
+                        "Could not retrieve any facts"
+                    )
                 end
-            else
-                textfacts = CGI.escape(Marshal::dump(facts))
 
-                # error handling for this is done in the network client
-                textobjects = @driver.getconfig(textfacts)
+                objects = nil
+                if @local
+                    objects = @driver.getconfig(facts)
 
-                unless textobjects == ""
-                    begin
-                        textobjects = CGI.unescape(textobjects)
-                    rescue => detail
-                        raise Puppet::Error, "Could not CGI.unescape configuration"
+                    if objects == ""
+                        raise Puppet::Error, "Could not retrieve configuration"
                     end
-                end
+                else
+                    textfacts = CGI.escape(Marshal::dump(facts))
 
-                if @cache
-                    if textobjects == ""
-                        if FileTest.exists?(Puppet[:localconfig])
-                            textobjects = File.read(Puppet[:localconfig])
-                        else
-                            raise Puppet::Error.new(
-                                "Cannot connect to server and there is no cached configuration"
-                            )
+                    # error handling for this is done in the network client
+                    textobjects = @driver.getconfig(textfacts)
+
+                    unless textobjects == ""
+                        begin
+                            textobjects = CGI.unescape(textobjects)
+                        rescue => detail
+                            raise Puppet::Error, "Could not CGI.unescape configuration"
                         end
-                    else
-                        # we store the config so that if we can't connect next time, we
-                        # can just run against the most recently acquired copy
-                        confdir = File.dirname(Puppet[:localconfig])
-                        unless FileTest.exists?(confdir)
-                            Puppet.recmkdir(confdir, 0770)
+                    end
+
+                    if @cache
+                        if textobjects == ""
+                            if FileTest.exists?(Puppet[:localconfig])
+                                textobjects = File.read(Puppet[:localconfig])
+                            else
+                                raise Puppet::Error.new(
+                                    "Cannot connect to server and there is no cached configuration"
+                                )
+                            end
+                        else
+                            # we store the config so that if we can't connect next time, we
+                            # can just run against the most recently acquired copy
+                            confdir = File.dirname(Puppet[:localconfig])
+                            unless FileTest.exists?(confdir)
+                                Puppet.recmkdir(confdir, 0770)
+                            end
+                            File.open(Puppet[:localconfig], "w", 0660) { |f|
+                                f.print textobjects
+                            }
                         end
-                        File.open(Puppet[:localconfig], "w", 0660) { |f|
-                            f.print textobjects
-                        }
+                    elsif textobjects == ""
+                        raise Puppet::Error, "Could not retrieve configuration"
+                    end
+
+                    begin
+                        objects = Marshal::load(textobjects)
+                    rescue => detail
+                        raise Puppet::Error.new("Could not understand configuration")
                     end
-                elsif textobjects == ""
-                    raise Puppet::Error, "Could not retrieve configuration"
                 end
+                if objects.is_a?(Puppet::TransBucket)
+                    @objects = objects
+                else
+                    Puppet.warning objects.inspect
+                    raise NetworkClientError.new(objects.class)
+                end
+            end
 
+            # this method is how the client receives the tree of Transportable
+            # objects
+            # for now, just descend into the tree and perform and necessary
+            # manipulations
+            def config
+                unless defined? @objects
+                    raise Puppet::Error, "Cannot config; objects not defined"
+                end
+                Puppet.debug("Calling config")
+
+                # XXX this is kind of a problem; if the user changes the state file
+                # after this, then we have to reload the file and everything...
                 begin
-                    objects = Marshal::load(textobjects)
+                    Puppet::Storage.init
+                    Puppet::Storage.load
                 rescue => detail
-                    raise Puppet::Error.new("Could not understand configuration")
+                    Puppet.err "Corrupt state file %s" % Puppet[:checksumfile]
+                    begin
+                        File.unlink(Puppet[:checksumfile])
+                        retry
+                    rescue => detail
+                        raise Puppet::Error.new("Cannot remove %s: %s" %
+                            [Puppet[statefile], detail])
+                    end
                 end
+
+                container = @objects.to_type
+                #if @local
+                #    container = @objects.to_type
+                #else
+                #    container = Marshal::load(@objects).to_type
+                #end
+
+                # this is a gross hack... but i don't see a good way around it
+                # set all of the variables to empty
+                Puppet::Transaction.init
+
+                # for now we just evaluate the top-level container, but eventually
+                # there will be schedules and such associated with each object,
+                # and probably with the container itself
+                transaction = container.evaluate
+                #transaction = Puppet::Transaction.new(objects)
+                transaction.toplevel = true
+                transaction.evaluate
+                Puppet::Metric.gather
+                Puppet::Metric.tally
+                if Puppet[:rrdgraph] == true
+                    Metric.store
+                    Metric.graph
+                end
+                Puppet::Storage.store
+
+                return transaction
             end
-            if objects.is_a?(Puppet::TransBucket)
-                @objects = objects
-            else
-                Puppet.warning objects.inspect
-                raise NetworkClientError.new(objects.class)
+
+            def initcerts
+                unless self.readcert
+                    unless self.requestcert
+                        return nil
+                    end
+                end
+
+                unless @driver
+                    return true
+                end
+
+                Puppet.info "setting cert and key and such"
+                @driver.cert = @cert
+                @driver.key = @key
+                @driver.ca_file = @cacertfile
             end
         end
 
-        # this method is how the client receives the tree of Transportable
-        # objects
-        # for now, just descend into the tree and perform and necessary
-        # manipulations
-        def config
-            unless defined? @objects
-                raise Puppet::Error, "Cannot config; objects not defined"
-            end
-            Puppet.debug("Calling config")
-
-            # XXX this is kind of a problem; if the user changes the state file
-            # after this, then we have to reload the file and everything...
-            begin
-                Puppet::Storage.init
-                Puppet::Storage.load
-            rescue => detail
-                Puppet.err "Corrupt state file %s" % Puppet[:checksumfile]
-                begin
-                    File.unlink(Puppet[:checksumfile])
-                    retry
-                rescue => detail
-                    raise Puppet::Error.new("Cannot remove %s: %s" %
-                        [Puppet[statefile], detail])
+        class Dipper < Puppet::Client
+            @drivername = :Bucket
+
+            def initialize(hash = {})
+                if hash.include?(:Path)
+                    bucket = Puppet::Server::FileBucket.new(
+                        :Bucket => hash[:Path]
+                    )
+                    hash.delete(:Path)
+                    hash[:Bucket] = bucket
                 end
+
+                super(hash)
             end
 
-            container = @objects.to_type
-            #if @local
-            #    container = @objects.to_type
-            #else
-            #    container = Marshal::load(@objects).to_type
-            #end
-
-            # this is a gross hack... but i don't see a good way around it
-            # set all of the variables to empty
-            Puppet::Transaction.init
-
-            # for now we just evaluate the top-level container, but eventually
-            # there will be schedules and such associated with each object,
-            # and probably with the container itself
-            transaction = container.evaluate
-            #transaction = Puppet::Transaction.new(objects)
-            transaction.toplevel = true
-            transaction.evaluate
-            Puppet::Metric.gather
-            Puppet::Metric.tally
-            if Puppet[:rrdgraph] == true
-                Metric.store
-                Metric.graph
+            def backup(file)
+                unless FileTest.exists?(file)
+                    raise(BucketError, "File %s does not exist" % file, caller)
+                end
+                contents = File.open(file) { |of| of.read }
+
+                string = Base64.encode64(contents)
+                #puts "string is created"
+
+                sum = @driver.addfile(string,file)
+                #puts "file %s is added" % file
+                return sum
             end
-            Puppet::Storage.store
 
-            return transaction
-            #self.shutdown
-        end
+            def restore(file,sum)
+                restore = true
+                if FileTest.exists?(file)
+                    contents = File.open(file) { |of| of.read }
 
-        def initcerts
-            unless self.readcert
-                unless self.requestcert
+                    cursum = Digest::MD5.hexdigest(contents)
+
+                    # if the checksum has changed...
+                    # this might be extra effort
+                    if cursum == sum
+                        restore = false
+                    end
+                end
+
+                if restore
+                    #puts "Restoring %s" % file
+                    newcontents = Base64.decode64(@driver.getfile(sum))
+                    newsum = Digest::MD5.hexdigest(newcontents)
+                    File.open(file,File::WRONLY|File::TRUNC) { |of|
+                        of.print(newcontents)
+                    }
+                    #puts "Done"
+                    return newsum
+                else
                     return nil
                 end
-            end
 
-            unless @driver
-                return true
             end
-
-            Puppet.info "setting cert and key and such"
-            @driver.cert = @cert
-            @driver.key = @key
-            @driver.ca_file = @cacertfile
         end
+
     end
-    #---------------------------------------------------------------
+#---------------------------------------------------------------
 end
 
 # $Id$
diff --git a/lib/puppet/server.rb b/lib/puppet/server.rb
index c3ba0b394..47b53a27b 100644
--- a/lib/puppet/server.rb
+++ b/lib/puppet/server.rb
@@ -26,6 +26,7 @@ module Puppet
     #---------------------------------------------------------------
     if $noservernetworking
         Puppet.err "Could not create server: %s" % $noservernetworking
+        class Server; end
     else
         class Server < WEBrick::HTTPServer
             include Puppet::Daemon
@@ -154,3 +155,4 @@ require 'puppet/server/master'
 require 'puppet/server/ca'
 require 'puppet/server/fileserver'
 require 'puppet/server/filebucket'
+require 'puppet/client'
diff --git a/lib/puppet/server/filebucket.rb b/lib/puppet/server/filebucket.rb
index f0e717146..fa02d967a 100755
--- a/lib/puppet/server/filebucket.rb
+++ b/lib/puppet/server/filebucket.rb
@@ -15,37 +15,26 @@ require 'facter'
 require 'digest/md5'
 require 'base64'
 
-class BucketError < RuntimeError; end
+module Puppet
+class Server
+    class BucketError < RuntimeError; end
+    class FileBucket < Handler
+        DEFAULTPORT = 8139
+
+        @interface = XMLRPC::Service::Interface.new("puppetbucket") { |iface|
+            iface.add_method("string addfile(string, string)")
+            iface.add_method("string getfile(string)")
+        }
 
-module FileBucket
-    DEFAULTPORT = 8139
-    # this doesn't work for relative paths
-    def FileBucket.mkdir(dir)
-        if FileTest.exist?(dir)
-            return false
-        else
-            tmp = dir.sub(/^\//,'')
-            path = [File::SEPARATOR]
-            tmp.split(File::SEPARATOR).each { |dir|
-                path.push dir
-                unless FileTest.exist?(File.join(path))
-                    Dir.mkdir(File.join(path))
-                end
-            }
-            return true
+        # this doesn't work for relative paths
+        def FileBucket.paths(base,md5)
+            return [
+                File.join(base, md5),
+                File.join(base, md5, "contents"),
+                File.join(base, md5, "paths")
+            ]
         end
-    end
 
-    def FileBucket.paths(base,md5)
-        return [
-            File.join(base, md5),
-            File.join(base, md5, "contents"),
-            File.join(base, md5, "paths")
-        ]
-    end
-
-    #---------------------------------------------------------------
-    class Bucket
         def initialize(hash)
             # build our AST
 
@@ -67,12 +56,11 @@ module FileBucket
                 end
             end
 
-            # XXX this should really be done using Puppet::Type instances...
-            FileBucket.mkdir(@bucket)
+            Puppet.recmkdir(@bucket)
         end
 
         # accept a file from a client
-        def addfile(string,path)
+        def addfile(string,path, request = nil)
             #puts "entering addfile"
             contents = Base64.decode64(string)
             #puts "string is decoded"
@@ -83,7 +71,7 @@ module FileBucket
             bpath, bfile, pathpath = FileBucket.paths(@bucket,md5)
 
             # if it's a new directory...
-            if FileBucket.mkdir(bpath)
+            if Puppet.recmkdir(bpath)
                 # ...then just create the file
                 #puts "creating file"
                 File.open(bfile, File::WRONLY|File::CREAT) { |of|
@@ -141,7 +129,7 @@ module FileBucket
             return md5
         end
 
-        def getfile(md5)
+        def getfile(md5, request = nil)
             bpath, bfile, bpaths = FileBucket.paths(@bucket,md5)
 
             unless FileTest.exists?(bfile)
@@ -155,123 +143,7 @@ module FileBucket
 
             return Base64.encode64(contents)
         end
-
-        private
-
-        def on_init
-            @default_namespace = 'urn:filebucket-server'
-            add_method(self, 'addfile', 'string', 'path')
-            add_method(self, 'getfile', 'md5')
-        end
-
-        def cert(filename)
-            OpenSSL::X509::Certificate.new(File.open(File.join(@dir, filename)) { |f|
-                f.read
-            })
-        end
-
-        def key(filename)
-            OpenSSL::PKey::RSA.new(File.open(File.join(@dir, filename)) { |f|
-                f.read
-            })
-        end
-
-    end
-    #---------------------------------------------------------------
-
-    class BucketWebserver < WEBrick::HTTPServer
-        def initialize(hash)
-            unless hash.include?(:Port)
-                hash[:Port] = FileBucket::DEFAULTPORT
-            end
-            servlet = XMLRPC::WEBrickServlet.new
-            @bucket = FileBucket::Bucket.new(hash)
-            #puts @bucket
-            servlet.add_handler("bucket",@bucket)
-            super
-
-            self.mount("/RPC2", servlet)
-        end
+        #---------------------------------------------------------------
     end
-
-    class BucketClient < XMLRPC::Client
-        @@methods = [ :addfile, :getfile ]
-
-        @@methods.each { |method|
-            self.send(:define_method,method) { |*args|
-                begin
-                    call("bucket.%s" % method.to_s,*args)
-                rescue => detail
-                    #puts detail
-                end
-            }
-        }
-
-        def initialize(hash)
-            hash[:Path] ||= "/RPC2"
-            hash[:Server] ||= "localhost"
-            hash[:Port] ||= FileBucket::DEFAULTPORT
-            super(hash[:Server],hash[:Path],hash[:Port])
-        end
-    end
-
-    class Dipper
-        def initialize(hash)
-            if hash.include?(:Server)
-                @bucket = FileBucket::BucketClient.new(
-                    :Server => hash[:Server]
-                )
-            elsif hash.include?(:Bucket)
-                @bucket = hash[:Bucket]
-            elsif hash.include?(:Path)
-                @bucket = FileBucket::Bucket.new(
-                    :Path => hash[:Path]
-                )
-            end
-        end
-
-        def backup(file)
-            unless FileTest.exists?(file)
-                raise(BucketError, "File %s does not exist" % file, caller)
-            end
-            contents = File.open(file) { |of| of.read }
-
-            string = Base64.encode64(contents)
-            #puts "string is created"
-
-            sum = @bucket.addfile(string,file)
-            #puts "file %s is added" % file
-            return sum
-        end
-
-        def restore(file,sum)
-            restore = true
-            if FileTest.exists?(file)
-                contents = File.open(file) { |of| of.read }
-
-                cursum = Digest::MD5.hexdigest(contents)
-
-                # if the checksum has changed...
-                # this might be extra effort
-                if cursum == sum
-                    restore = false
-                end
-            end
-
-            if restore
-                #puts "Restoring %s" % file
-                newcontents = Base64.decode64(@bucket.getfile(sum))
-                newsum = Digest::MD5.hexdigest(newcontents)
-                File.open(file,File::WRONLY|File::TRUNC) { |of|
-                    of.print(newcontents)
-                }
-                #puts "Done"
-                return newsum
-            else
-                return nil
-            end
-
-        end
-    end
-    #---------------------------------------------------------------
+end
 end
diff --git a/lib/puppet/server/servlet.rb b/lib/puppet/server/servlet.rb
index 4db9f6c0e..70f47a069 100644
--- a/lib/puppet/server/servlet.rb
+++ b/lib/puppet/server/servlet.rb
@@ -38,6 +38,7 @@ class Server
                     obj.call(*args)
                 rescue => detail
                     Puppet.warning obj.inspect
+                    Puppet.warning args.inspect
                     Puppet.err "Could not call: %s" % detail.to_s
                 end
             }
@@ -49,7 +50,7 @@ class Server
                 Puppet.info "client cert is %s" % @request.client_cert
             end
             if @request.server_cert
-                Puppet.info "server cert is %s" % @request.server_cert
+                #Puppet.info "server cert is %s" % @request.server_cert
             end
             #p @request
             begin
diff --git a/lib/puppet/type/component.rb b/lib/puppet/type/component.rb
index a996bd591..f34e238b0 100644
--- a/lib/puppet/type/component.rb
+++ b/lib/puppet/type/component.rb
@@ -33,7 +33,7 @@ module Puppet
         end
 
         def self.recurse(obj, inlist, list)
-            return if list.include?(obj.object_id)
+            return if inlist.include?(obj.object_id)
             obj.eachdependency { |req|
                 self.recurse(req, inlist, list)
             }
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 3c233e79e..cb68ee347 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -217,7 +217,7 @@ module Puppet
                 self.is = sum
 
                 # if we don't have a 'should' value, then go ahead and mark it
-                if @should == -2
+                if ! defined? @should or @should == -2
                     @should = sum
                     self.updatesum
                 end
@@ -240,6 +240,7 @@ module Puppet
                     self.retrieve
 
                     if @is == @should
+                        Puppet.debug "Checksum is already in sync"
                         return nil
                     end
                     #Puppet.debug "%s(%s): after refresh, is '%s'" %
diff --git a/lib/puppet/type/pfilebucket.rb b/lib/puppet/type/pfilebucket.rb
index da931244c..c5d1eeb22 100755
--- a/lib/puppet/type/pfilebucket.rb
+++ b/lib/puppet/type/pfilebucket.rb
@@ -33,7 +33,7 @@ module Puppet
                 if @parameters.include?(:server)
                     @parameters[:port] ||= FileBucket::DEFAULTPORT
                     begin
-                        @bucket = FileBucket::Dipper.new( 
+                        @bucket = Puppet::Client::Dipper.new( 
                             :Server => @parameters[:server],
                             :Port => @parameters[:port]
                         )
@@ -45,7 +45,7 @@ module Puppet
                 else
                     @parameters[:path] ||= Puppet[:bucketdir]
                     begin
-                        @bucket = FileBucket::Dipper.new(
+                        @bucket = Puppet::Client::Dipper.new(
                             :Path => @parameters[:path]
                         )
                     rescue => detail