Many, many changes toward a completely functional system. The only current problems with my home config are that apache's stupid init script does not do status and that packages are not working as non-root users (which makes sense).

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

15 files changed, 323 insertions, 172 deletions

diff --git a/lib/puppet/client.rb b/lib/puppet/client.rb
index cbb7dbedc..9bc46717b 100644
--- a/lib/puppet/client.rb
+++ b/lib/puppet/client.rb
@@ -1,5 +1,3 @@
-#!/usr/local/bin/ruby -w
-
 # the available clients
 
 require 'puppet'
@@ -43,20 +41,26 @@ module Puppet
                     method = ary[0]
                     Puppet.info "Defining %s.%s" % [namespace, method]
                     self.send(:define_method,method) { |*args|
+                        Puppet.info "Calling %s" % method
                         #Puppet.info "peer cert is %s" % @http.peer_cert
                         #Puppet.info "cert is %s" % @http.cert
                         begin
                             call("%s.%s" % [namespace, method.to_s],*args)
                         rescue OpenSSL::SSL::SSLError => detail
-                            Puppet.err "Could not call %s.%s: Untrusted certificates" %
-                                [namespace, method]
+                            #Puppet.err "Could not call %s.%s: Untrusted certificates" %
+                            #    [namespace, method]
                             raise NetworkClientError,
                                 "Certificates were not trusted"
                         rescue XMLRPC::FaultException => detail
-                            Puppet.err "Could not call %s.%s: %s" %
-                                [namespace, method, detail.faultString]
-                            raise NetworkClientError,
-                                "XMLRPC Error: %s" % detail.faultString
+                            #Puppet.err "Could not call %s.%s: %s" %
+                            #    [namespace, method, detail.faultString]
+                            #raise NetworkClientError,
+                            #    "XMLRPC Error: %s" % detail.faultString
+                            raise NetworkClientError, detail.faultString
+                        rescue Errno::ECONNREFUSED => detail
+                            msg = "Could not connect to %s on port %s" % [@host, @port]
+                            #Puppet.err msg
+                            raise NetworkClientError, msg
                         #rescue => detail
                         #    Puppet.err "Could not call %s.%s: %s" %
                         #        [namespace, method, detail.inspect]
@@ -78,7 +82,7 @@ module Puppet
             end
 
             def cert=(cert)
-                Puppet.info "Adding certificate"
+                Puppet.debug "Adding certificate"
                 @http.cert = cert
                 @http.verify_mode = OpenSSL::SSL::VERIFY_PEER
             end
@@ -216,7 +220,7 @@ module Puppet
             def self.facts
                 facts = {}
                 Facter.each { |name,fact|
-                    facts[name] = fact
+                    facts[name] = fact.downcase
                 }
 
                 facts
@@ -341,8 +345,8 @@ module Puppet
                 if objects.is_a?(Puppet::TransBucket)
                     @objects = objects
                 else
-                    Puppet.warning objects.inspect
-                    raise NetworkClientError.new(objects.class)
+                    raise NetworkClientError,
+                        "Invalid returned objects of type %s" % objects.class
                 end
             end
         end
@@ -426,10 +430,11 @@ module Puppet
                         begin
                             @driver.send(method, *args)
                         rescue XMLRPC::FaultException => detail
-                            Puppet.err "Could not call %s.%s: %s" %
-                                [namespace, method, detail.faultString]
-                            raise NetworkClientError,
-                                "XMLRPC Error: %s" % detail.faultString
+                            #Puppet.err "Could not call %s.%s: %s" %
+                            #    [namespace, method, detail.faultString]
+                            #raise NetworkClientError,
+                            #    "XMLRPC Error: %s" % detail.faultString
+                            raise NetworkClientError, detail.faultString
                         end
                     }
                 }
diff --git a/lib/puppet/parser/ast.rb b/lib/puppet/parser/ast.rb
index 7fa1a156d..602067436 100644
--- a/lib/puppet/parser/ast.rb
+++ b/lib/puppet/parser/ast.rb
@@ -239,6 +239,10 @@ module Puppet
             # meant completely different things.
             class ObjectInst < ASTArray; end
 
+            # Another simple container class to make sure we can correctly arrayfy
+            # things.
+            class CompArgument < ASTArray; end
+
             # The base class for all of the leaves of the parse trees.  These
             # basically just have types and values.  Both of these parameters
             # are simple values, not AST objects.
@@ -1289,8 +1293,10 @@ module Puppet
                         # arguments...
                         self.args.each { |arg, default|
                             unless hash.include?(arg)
-                                if defined? default
+                                if defined? default and ! default.nil?
                                     hash[arg] = default
+                                    Puppet.debug "Got default %s for %s in %s" %
+                                        [default.inspect, arg.inspect, objname.inspect]
                                 else
                                     error = Puppet::ParseError.new(
                                         "Must pass %s to %s of type %s" %
diff --git a/lib/puppet/parser/grammar.ra b/lib/puppet/parser/grammar.ra
index 78fb9aa47..5fe801e15 100644
--- a/lib/puppet/parser/grammar.ra
+++ b/lib/puppet/parser/grammar.ra
@@ -25,18 +25,18 @@ program:    statements {
 
     # this is mainly so we can test the parser separately from the
     # interpreter
-    if Puppet[:parseonly]
-        begin
-            if Puppet[:debug]
-                puts result.tree(0)
-            end
-        rescue NoMethodError => detail
-            Puppet.err detail
-            #exit(78)
-        end
-        #require 'puppet/parser/interpreter'
-        #result = Puppet::Server.new(result)
-    end
+#    if Puppet[:parseonly]
+#        begin
+#            if Puppet[:debug]
+#                puts result.tree(0)
+#            end
+#        rescue NoMethodError => detail
+#            Puppet.err detail
+#            #exit(78)
+#        end
+#        #require 'puppet/parser/interpreter'
+#        #result = Puppet::Server.new(result)
+#    end
 }
 
 statements:   statement
@@ -447,7 +447,7 @@ import: IMPORT QTEXT {
     Dir.chdir(dir) {
         Dir.glob(val[1]).each { |file|
             parser = Puppet::Parser::Parser.new()
-            parser.stack = self.stack
+            parser.files = self.files
             Puppet.debug("importing '%s'" % file)
             begin
                 parser.file = file
@@ -554,7 +554,7 @@ argumentlist: nothing
 
 arguments:    argument
             | arguments COMMA argument {
-    if val[0].is_a?(AST::ASTArray)
+    if val[0].instance_of?(AST::ASTArray)
         val[0].push(val[2])
         result = val[0]
     else
@@ -567,14 +567,14 @@ arguments:    argument
 }
 
 argument:     name EQUALS rvalue {
-    result = AST::ASTArray.new(
+    result = AST::CompArgument.new(
         :line => @lexer.line,
         :file => @lexer.file,
         :children => [val[0],val[2]]
     )
 }
             | name {
-    result = AST::ASTArray.new(
+    result = AST::CompArgument.new(
         :line => @lexer.line,
         :file => @lexer.file,
         :children => [val[0]]
@@ -635,14 +635,15 @@ Puppet[:typecheck] = true
 Puppet[:paramcheck] = true
 
 ---- inner ----
-attr_reader :file, :files
+attr_reader :file
+attr_accessor :files
 
 # Create an AST array out of all of the args
 def aryfy(*args)
     if args[0].is_a?(AST::ASTArray)
         result = args.shift
         args.each { |arg|
-            args.push arg
+            result.push arg
         }
     else
         result = AST::ASTArray.new(
diff --git a/lib/puppet/parser/interpreter.rb b/lib/puppet/parser/interpreter.rb
index cd7ac46d6..e076bc1ca 100644
--- a/lib/puppet/parser/interpreter.rb
+++ b/lib/puppet/parser/interpreter.rb
@@ -45,15 +45,19 @@ module Puppet
                     if @usenodes
                         unless client
                             raise Puppet::Error,
-                                "Cannot evaluate no nodes with a nil client"
+                                "Cannot evaluate nodes with a nil client"
                         end
 
                         # We've already evaluated the AST, in this case
-                        @scope.evalnode(names, facts)
+                        retval = @scope.evalnode(names, facts)
+                        return retval
                     else
+                        # We've already evaluated the AST, in this case
                         @scope = Puppet::Parser::Scope.new() # no parent scope
                         @scope.interp = self
-                        @scope.evaluate(@ast, facts)
+                        @scope.type = "puppet"
+                        @scope.name = "top"
+                        return @scope.evaluate(@ast, facts)
                     end
                     #@ast.evaluate(@scope)
                 rescue Puppet::DevError, Puppet::Error, Puppet::ParseError => except
@@ -82,14 +86,14 @@ module Puppet
                 # to pass to the client
                 # this will be heirarchical, and will (at this point) contain
                 # only TransObjects and TransSettings
-                @scope.name = "top"
-                @scope.type = "puppet"
-                begin
-                    topbucket = @scope.to_trans
-                rescue => detail
-                    Puppet.warning detail
-                    raise
-                end
+                #@scope.name = "top"
+                #@scope.type = "puppet"
+                #begin
+                #    topbucket = @scope.to_trans
+                #rescue => detail
+                #    Puppet.warning detail
+                #    raise
+                #end
 
                 # add our settings to the front of the array
                 # at least, for now
@@ -104,7 +108,7 @@ module Puppet
                 #retlist = TransObject.list
                 #Puppet.debug "retobject length is %s" % retlist.length
                 #TransObject.clear
-                return topbucket
+                #return topbucket
             end
 
             def scope
@@ -120,6 +124,8 @@ module Puppet
 
                 if @usenodes
                     @scope = Puppet::Parser::Scope.new() # no parent scope
+                    @scope.name = "top"
+                    @scope.type = "puppet"
                     @scope.interp = self
                     Puppet.debug "Nodes defined"
                     @ast.safeevaluate(@scope)
diff --git a/lib/puppet/parser/parser.rb b/lib/puppet/parser/parser.rb
index df92e9c2e..733de138c 100644
--- a/lib/puppet/parser/parser.rb
+++ b/lib/puppet/parser/parser.rb
@@ -32,15 +32,16 @@ module Puppet
 
     class Parser < Racc::Parser
 
-module_eval <<'..end grammar.ra modeval..id4705e629f1', 'grammar.ra', 638
-attr_reader :file, :files
+module_eval <<'..end grammar.ra modeval..idb211ea7fd3', 'grammar.ra', 638
+attr_reader :file
+attr_accessor :files
 
 # Create an AST array out of all of the args
 def aryfy(*args)
     if args[0].is_a?(AST::ASTArray)
         result = args.shift
         args.each { |arg|
-            args.push arg
+            result.push arg
         }
     else
         result = AST::ASTArray.new(
@@ -144,7 +145,7 @@ end
 def string=(string)
     @lexer.string = string
 end
-..end grammar.ra modeval..id4705e629f1
+..end grammar.ra modeval..idb211ea7fd3
 
 ##### racc 1.4.4 generates ###
 
@@ -561,18 +562,18 @@ module_eval <<'.,.,', 'grammar.ra', 40
 
     # this is mainly so we can test the parser separately from the
     # interpreter
-    if Puppet[:parseonly]
-        begin
-            if Puppet[:debug]
-                puts result.tree(0)
-            end
-        rescue NoMethodError => detail
-            Puppet.err detail
-            #exit(78)
-        end
-        #require 'puppet/parser/interpreter'
-        #result = Puppet::Server.new(result)
-    end
+#    if Puppet[:parseonly]
+#        begin
+#            if Puppet[:debug]
+#                puts result.tree(0)
+#            end
+#        rescue NoMethodError => detail
+#            Puppet.err detail
+#            #exit(78)
+#        end
+#        #require 'puppet/parser/interpreter'
+#        #result = Puppet::Server.new(result)
+#    end
    result
   end
 .,.,
@@ -1118,7 +1119,7 @@ module_eval <<'.,.,', 'grammar.ra', 469
     Dir.chdir(dir) {
         Dir.glob(val[1]).each { |file|
             parser = Puppet::Parser::Parser.new()
-            parser.stack = self.stack
+            parser.files = self.files
             Puppet.debug("importing '%s'" % file)
             begin
                 parser.file = file
@@ -1252,7 +1253,7 @@ module_eval <<'.,.,', 'grammar.ra', 553
 
 module_eval <<'.,.,', 'grammar.ra', 567
   def _reduce_80( val, _values, result )
-    if val[0].is_a?(AST::ASTArray)
+    if val[0].instance_of?(AST::ASTArray)
         val[0].push(val[2])
         result = val[0]
     else
@@ -1268,7 +1269,7 @@ module_eval <<'.,.,', 'grammar.ra', 567
 
 module_eval <<'.,.,', 'grammar.ra', 575
   def _reduce_81( val, _values, result )
-    result = AST::ASTArray.new(
+    result = AST::CompArgument.new(
         :line => @lexer.line,
         :file => @lexer.file,
         :children => [val[0],val[2]]
@@ -1279,7 +1280,7 @@ module_eval <<'.,.,', 'grammar.ra', 575
 
 module_eval <<'.,.,', 'grammar.ra', 582
   def _reduce_82( val, _values, result )
-    result = AST::ASTArray.new(
+    result = AST::CompArgument.new(
         :line => @lexer.line,
         :file => @lexer.file,
         :children => [val[0]]
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 3730af606..054a0e7b7 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -128,6 +128,7 @@ module Puppet
                 # it will be removed during translation.
 
                 # And now return the whole thing
+                #return self.to_trans
                 return self.to_trans
             end
 
@@ -485,8 +486,8 @@ module Puppet
 
             # Convert our scope to a list of Transportable objects.
             def to_trans
-                Puppet.debug "Translating scope %s at level %s" %
-                    [self.object_id,self.level]
+                #Puppet.debug "Translating scope %s at level %s" %
+                #    [self.object_id,self.level]
 
                 results = []
                 
@@ -494,8 +495,8 @@ module Puppet
                 @children.each { |child|
                     if child.is_a?(Scope)
                         cresult = child.to_trans
-                        Puppet.debug "Got %s from scope %s" %
-                            [cresult.class,child.object_id]
+                        #Puppet.debug "Got %s from scope %s" %
+                        #    [cresult.class,child.object_id]
 
                         # Scopes normally result in a TransBucket, but they could
                         # also result in a normal array; if that happens, get rid
@@ -553,8 +554,10 @@ module Puppet
                         error.stack = caller
                         raise error
                     end
-                    Puppet.debug "TransBucket with name %s and type %s in scope %s" %
-                        [@name,@type,self.object_id]
+                    #Puppet.debug(
+                    #    "TransBucket with name %s and type %s in scope %s" %
+                    #    [@name,@type,self.object_id]
+                    #)
 
                     # now find metaparams
                     @symtable.each { |var,value|
@@ -569,7 +572,7 @@ module Puppet
                     #    [bucket.name,self.object_id]
                     return bucket
                 else
-                    #Puppet.debug "nameless scope; just returning a list"
+                    Puppet.debug "nameless scope; just returning a list"
                     return results
                 end
             end
diff --git a/lib/puppet/server.rb b/lib/puppet/server.rb
index 15b27f849..6737b2664 100644
--- a/lib/puppet/server.rb
+++ b/lib/puppet/server.rb
@@ -81,6 +81,8 @@ module Puppet
 
                 super(hash)
 
+                Puppet.info "Listening on port %s" % hash[:Port]
+
                 # this creates a new servlet for every connection,
                 # but all servlets have the same list of handlers
                 # thus, the servlets can have their own state -- passing
diff --git a/lib/puppet/server/fileserver.rb b/lib/puppet/server/fileserver.rb
index 519d4aa0c..f792c7cd3 100755
--- a/lib/puppet/server/fileserver.rb
+++ b/lib/puppet/server/fileserver.rb
@@ -205,8 +205,7 @@ class Server
                 end
             end
 
-            @mounts.clear
-
+            newmounts = {}
             begin
                 File.open(@config) { |f|
                     mount = nil
@@ -217,24 +216,24 @@ class Server
                         when /^\s*$/: next # skip blank lines
                         when /\[(\w+)\]/:
                             name = $1
-                            if mount
-                                unless mount.path
-                                    raise Puppet::Error, "Mount %s has no path specified" %
-                                        mount.name
-                                end
-                            end
-                            if @mounts.include?(name)
+                            if newmounts.include?(name)
                                 raise FileServerError, "%s is already mounted at %s" %
-                                    [@mounts[name], name]
+                                    [newmounts[name], name]
                             end
                             mount = Mount.new(name)
-                            @mounts[name] = mount
-                        when /\s*(\w+)\s+(.+)$/:
+                            newmounts[name] = mount
+                        when /^\s*(\w+)\s+(.+)$/:
                             var = $1
                             value = $2
                             case var
                             when "path":
-                                mount.path = value
+                                begin
+                                    mount.path = value
+                                rescue FileServerError => detail
+                                    Puppet.err "Removing mount %s: %s" %
+                                        [mount.name, detail]
+                                    newmounts.delete(mount.name)
+                                end
                             when "allow":
                                 value.split(/\s*,\s*/).each { |val|
                                     begin
@@ -242,7 +241,7 @@ class Server
                                             [val, mount.name]
                                         mount.allow(val)
                                     rescue AuthStoreError => detail
-                                        raise Puppet::Error, "%s at line %s of %s" %
+                                        raise FileServerError, "%s at line %s of %s" %
                                             [detail.to_s, count, @config]
                                     end
                                 }
@@ -253,27 +252,39 @@ class Server
                                             [val, mount.name]
                                         mount.deny(val)
                                     rescue AuthStoreError => detail
-                                        raise Puppet::Error, "%s at line %s of %s" %
+                                        raise FileServerError, "%s at line %s of %s" %
                                             [detail.to_s, count, @config]
                                     end
                                 }
                             else
-                                raise Puppet::Error,
-                                    "Invalid argument %s at line %s" % [var, count]
+                                raise FileServerError,
+                                    "Invalid argument '%s' at line %s" % [var, count]
                             end
                         else
-                            raise Puppet::Error,
-                                "Invalid line %s: %s" % [count, line]
+                            raise FileServerError, "Invalid line %s: %s" % [count, line]
                         end
                         count += 1
                     }
                 }
             rescue Errno::EACCES => detail
-                raise Puppet::Error, "Cannot read %s" % @config
+                Puppet.err "FileServer error: Cannot read %s; cannot serve" % @config
+                #raise Puppet::Error, "Cannot read %s" % @config
             rescue Errno::ENOENT => detail
-                raise Puppet::Error, "%s does not exit" % @config
+                Puppet.err "FileServer error: '%s' does not exit; cannot serve" %
+                    @config
+                #raise Puppet::Error, "%s does not exit" % @config
+            #rescue FileServerError => detail
+            #    Puppet.err "FileServer error: %s" % detail
             end
 
+            # Verify each of the mounts are valid.
+            # We let the check raise an error, so that it can raise an error
+            # pointing to the specific problem.
+            newmounts.each { |name, mount|
+                mount.valid?
+            }
+            @mounts = newmounts
+
             @configstamp = File.stat(@config).ctime
             @configstatted = Time.now
         end
@@ -283,7 +294,8 @@ class Server
             mount, path = splitpath(file)
 
             unless (@mounts.include?(mount))
-                raise Puppet::Server::FileServerError, "%s not mounted" % mount
+                raise Puppet::Server::FileServerError,
+                    "FileServer module '%s' not mounted" % mount
             end
 
             unless @mounts[mount].allowed?(client, clientip)
@@ -370,14 +382,15 @@ class Server
                 path = dir.sub(%r{/#{mount}/?}, '')
 
                 unless @mounts.include?(mount)
-                    raise FileServerError, "%s not mounted" % mount
+                    raise FileServerError, "Fileserver module '%s' not mounted" % mount
                 end
 
                 unless @mounts[mount].path
-                    raise FileServerError, "Mount %s does not have a path set" % mount
+                    raise FileServerError,
+                        "Fileserver error: Mount '%s' does not have a path set" % mount
                 end
             else
-                raise FileServerError, "Invalid path '%s'" % dir
+                raise FileServerError, "Fileserver error: Invalid path '%s'" % dir
             end
 
             if path == ""
@@ -410,6 +423,8 @@ class Server
 
                 if path
                     self.path = path
+                else
+                    @path = nil
                 end
 
                 super()
@@ -425,6 +440,14 @@ class Server
             def to_s
                 @path
             end
+
+            # Verify our configuration is valid.  This should really check to
+            # make sure at least someone will be allowed, but, eh.
+            def valid?
+                unless @path
+                    raise FileServerError, "No path specified"
+                end
+            end
         end
     end
 end
diff --git a/lib/puppet/server/master.rb b/lib/puppet/server/master.rb
index 08706bea7..03ff7c6ac 100644
--- a/lib/puppet/server/master.rb
+++ b/lib/puppet/server/master.rb
@@ -70,6 +70,10 @@ class Server
                 end
             end
 
+            unless client
+                client = facts["hostname"]
+                clientip = facts["ipaddress"]
+            end
             Puppet.debug("Running interpreter")
             begin
                 retobjects = @interpreter.run(client, facts)
diff --git a/lib/puppet/server/servlet.rb b/lib/puppet/server/servlet.rb
index 2bc2dffc1..e35a1d518 100644
--- a/lib/puppet/server/servlet.rb
+++ b/lib/puppet/server/servlet.rb
@@ -14,23 +14,46 @@ class Server
             self.new(server, *options)
         end
 
+        def add_handler(interface, handler)
+            @loadedhandlers << interface.prefix
+            super
+        end
+
+        # Verify that our client has access.  We allow untrusted access to
+        # puppetca methods but none others.
         def authorize(request, method)
+            namespace = method.sub(/\..+/, '')
+            client = request.peeraddr[2]
+            ip = request.peeraddr[3]
             if request.client_cert
                 Puppet.info "Allowing %s(%s) trusted access to %s" %
-                    [request.peeraddr[2], request.peeraddr[3], method]
+                    [client, ip, method]
                 return true
             else
                 if method =~ /^puppetca\./
                     Puppet.notice "Allowing %s(%s) untrusted access to CA methods" %
-                        [request.peeraddr[2], request.peeraddr[3]]
+                        [client, ip]
                 else
                     Puppet.err "Unauthenticated client %s(%s) cannot call %s" %
-                        [request.peeraddr[2], request.peeraddr[3], method]
+                        [client, ip, method]
                     return false
                 end
             end
         end
 
+        def available?(method)
+            namespace = method.sub(/\..+/, '')
+            client = request.peeraddr[2]
+            ip = request.peeraddr[3]
+            if @loadedhandlers.include?(namespace)
+                return true
+            else
+                Puppet.warning "Client %s(%s) requested unavailable functionality %s" %
+                    [client, ip, namespace]
+                return false
+            end
+        end
+
         def initialize(server, handlers)
             #Puppet.info server.inspect
             
@@ -41,6 +64,7 @@ class Server
             # and we can consume them all ourselves
             super()
 
+            @loadedhandlers = []
             handlers.each { |handler|
                 Puppet.debug "adding handler for %s" % handler.class
                 self.add_handler(handler.class.interface, handler)
@@ -58,22 +82,22 @@ class Server
                 begin
                     obj.call(*args)
                 rescue Puppet::Server::AuthorizationError => detail
-                    Puppet.warning obj.inspect
-                    Puppet.warning args.inspect
+                    #Puppet.warning obj.inspect
+                    #Puppet.warning args.inspect
                     Puppet.err "Permission denied: %s" % detail.to_s
                     raise XMLRPC::FaultException.new(
                         1, detail.to_s
                     )
                 rescue Puppet::Error => detail
-                    Puppet.warning obj.inspect
-                    Puppet.warning args.inspect
-                    Puppet.err "Puppet error: %s" % detail.to_s
+                    #Puppet.warning obj.inspect
+                    #Puppet.warning args.inspect
+                    Puppet.err detail.to_s
                     raise XMLRPC::FaultException.new(
                         1, detail.to_s
                     )
                 rescue => detail
-                    Puppet.warning obj.inspect
-                    Puppet.warning args.inspect
+                    #Puppet.warning obj.inspect
+                    #Puppet.warning args.inspect
                     Puppet.err "Could not call: %s" % detail.to_s
                     raise error
                 end
@@ -118,6 +142,13 @@ class Server
         def dispatch(methodname, *args)
 
             if defined? @request and @request
+                unless self.available?(methodname)
+                    raise XMLRPC::FaultException.new(
+                        ERR_UNAUTHORIZED,
+                        "Functionality %s not available" %
+                            methodname.sub(/\..+/, '')
+                    )
+                end
                 unless self.authorize(@request, methodname)
                     raise XMLRPC::FaultException.new(
                         ERR_UNAUTHORIZED,
diff --git a/lib/puppet/statechange.rb b/lib/puppet/statechange.rb
index 0c817e909..7e670e975 100644
--- a/lib/puppet/statechange.rb
+++ b/lib/puppet/statechange.rb
@@ -29,17 +29,21 @@ module Puppet
 
 		#---------------------------------------------------------------
         def go
-            if @state.noop
-                #Puppet.debug "%s is noop" % @state
-                return nil
-            end
-
             if @state.is == @state.should
                 Puppet.info "%s.%s is already in sync" %
                     [@state.parent.name, @state.name]
                 return nil
             end
 
+            if @state.noop
+                @state.parent.log "%s should be %s" %
+                    [@state, @should]
+                #Puppet.debug "%s is noop" % @state
+                return nil
+            else
+                Puppet.notice "Noop is %s" % @state.noop
+            end
+
             begin
                 events = @state.sync
                 if events.nil?
@@ -68,7 +72,7 @@ module Puppet
                         #:state => @state,
                         #:object => @state.parent,
                     # FIXME this is where loglevel stuff should go
-                    Puppet.notice @state.change_to_s
+                    @state.parent.log @state.change_to_s
                     Puppet::Event.new(
                         :event => event,
                         :change => self,
@@ -87,7 +91,7 @@ module Puppet
                 #    pname = pname.id2name
                 #end
                     #:state => @state,
-                Puppet.notice "Failed: " + @state.change_to_s
+                @state.parent.log "Failed: " + @state.change_to_s
                 return Puppet::Event.new(
                     :event => pname + "_failed",
                     :change => self,
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 88f3ff613..9e2d9f78c 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -787,7 +787,7 @@ class Type < Puppet::Element
 
         # set defalts
         @noop = false
-        @metaparams[:loglevel] = :info
+        @metaparams[:loglevel] = :notice
         # keeping stats for the total number of changes, and how many were
         # completely sync'ed
         # this isn't really sufficient either, because it adds lots of special cases
@@ -812,6 +812,11 @@ class Type < Puppet::Element
         order.flatten.each { |name|
             if hash.include?(name)
                 begin
+                    if name == "owner" or name == :owner
+                        if hash[name].nil?
+                            puts caller
+                        end
+                    end
                     self[name] = hash[name]
                 rescue => detail
                     raise Puppet::DevError.new( 
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index a04346201..42aff1777 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -534,6 +534,7 @@ module Puppet
             def addis(state,value)
                 if stateklass = self.class.validstate?(state)
                     @states[state] = stateklass.new(:parent => self)
+                    Puppet.debug "Adding is for %s as %s" % [state, value]
                     @states[state].is = value
                 elsif self.class.validparameter?(state)
                     self[state] = value
@@ -577,15 +578,23 @@ module Puppet
                         [self.name, error.to_s]
 
                     @states.delete(:install)
+                    return
                 end
 
-                if hash.nil?
+                if hash.nil? and @states.include?(:install)
+                    Puppet.info "Removing install"
                     @states[:install].is = nil
+                    #return
                 end
 
                 hash.each { |name,value|
                     if self.class.validstate?(name)
                         if @states.include?(name)
+                            unless @states[name]
+                                raise Puppet::DevError,
+                                    "State %s on package %s is %s" %
+                                    [name, @name, @states[name]]
+                            end
                             @states[name].is = value
                         else
                             # silently ignore any returned states
@@ -622,6 +631,7 @@ module Puppet
             end
         end # Puppet::Type::Package
     end
+
     # this is how we retrieve packages
     class PackageSource
         attr_accessor :uri
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 8e5048662..8dbdcc541 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -28,7 +28,7 @@ module Puppet
                 if stat = @parent.stat(true)
                     @is = stat.ftype
                 else
-                    @is = -1
+                    @is = :notfound
                 end
 
                 # so this state is never marked out of sync
@@ -57,9 +57,9 @@ module Puppet
                     @should = "file"
                 when "directory", /^d/:
                     @should = "directory"
-                when "-1", -1:
+                when :notfound:
                     # this is where a creation is being rolled back
-                    @should = -1
+                    @should = :notfound
                 else
                     error = Puppet::Error.new "Cannot create files of type %s" %
                         value
@@ -71,7 +71,7 @@ module Puppet
                 if stat = @parent.stat(true)
                     @is = stat.ftype
                 else
-                    @is = -1
+                    @is = :notfound
                 end
 
                 #Puppet.debug "'exists' state is %s" % self.is
@@ -105,7 +105,7 @@ module Puppet
                             Dir.mkdir(@parent.name)
                         end
                         event = :directory_created
-                    when -1:
+                    when :notfound:
                         # this is where the file should be deleted...
                         unless FileTest.size(@parent.name) == 0
                             raise Puppet::Error.new(
@@ -151,8 +151,8 @@ module Puppet
                         @should = -2
                     end
                 else
-                    # We can't use -1 here, because then it'll match on non-existent
-                    # files
+                    # We can't use :notfound here, because then it'll match on
+                    # non-existent files
                     @should = -2
                 end
             end
@@ -164,7 +164,7 @@ module Puppet
 
                 unless FileTest.exists?(@parent.name)
                     Puppet.err "File %s does not exist" % @parent.name
-                    self.is = -1
+                    self.is = :notfound
                     return
                 end
 
@@ -238,7 +238,7 @@ module Puppet
                     raise error
                 end
 
-                if @is == -1
+                if @is == :notfound
                     self.retrieve
 
                     if @is == @should
@@ -250,7 +250,7 @@ module Puppet
 
                     # if we still can't retrieve a checksum, it means that
                     # the file still doesn't exist
-                    if @is == -1
+                    if @is == :notfound
                         # if they're copying, then we won't worry about the file
                         # not existing yet
                         unless @parent.state(:source)
@@ -283,11 +283,11 @@ module Puppet
                     state[@parent.name] = Hash.new
                 end
 
-                if @is == -1
+                if @is == :notfound
                     error = Puppet::Error.new("%s has invalid checksum" %
                         @parent.name)
                     raise error
-                #elsif @should == -1
+                #elsif @should == :notfound
                 #    error = Puppet::Error.new("%s has invalid 'should' checksum" %
                 #        @parent.name)
                 #    raise error
@@ -328,36 +328,38 @@ module Puppet
 #                    @parent.delete(self.name)
 #                    @should = nil
 #                    @is = nil
-#                    unless defined? @@notified
+#                    unless defined? @@notifieduid
 #                        Puppet.notice "Cannot manage ownership unless running as root"
-#                        @@notified = true
+#                        @@notifieduid = true
 #                        return
 #                    end
 #                end
 
                 unless stat = @parent.stat(true)
-                    @is = -1
+                    @is = :notfound
                     return
                 end
 
                 self.is = stat.uid
             end
 
+            # If we're not root, we can check the values but we cannot change them
             def should=(value)
                 unless Process.uid == 0
                     @should = nil
                     @is = nil
-                    unless defined? @@notified
+                    unless defined? @@notifieduid
                         Puppet.notice "Cannot manage ownership unless running as root"
                         #@parent.delete(self.name)
-                        @@notified = true
+                        @@notifieduid = true
                     end
-                    if @parent.state(:owner)
-                        @parent.delete(:owner)
-                    end
-                    raise Puppet::Error.new(
-                        "Cannot manage ownership unless running as root"
-                    )
+                    return
+                    #if @parent.state(:owner)
+                    #    @parent.delete(:owner)
+                    #end
+                    #raise Puppet::Error.new(
+                    #    "Cannot manage ownership unless running as root"
+                    #)
                 end
                 if value.is_a?(Integer)
                     # verify the user is a valid user
@@ -404,14 +406,19 @@ module Puppet
 
             def sync
                 unless Process.uid == 0
+                    unless defined? @@notifieduid
+                        Puppet.notice "Cannot manage ownership unless running as root"
+                        #@parent.delete(self.name)
+                        @@notifieduid = true
+                    end
                     # there's a possibility that we never got retrieve() called
                     # e.g., if the file didn't exist
                     # thus, just delete ourselves now and don't do any work
-                    @parent.delete(self.name)
+                    #@parent.delete(self.name)
                     return nil
                 end
 
-                if @is == -1
+                if @is == :notfound
                     @parent.stat(true)
                     self.retrieve
                     #Puppet.debug "%s: after refresh, is '%s'" % [self.class.name,@is]
@@ -424,11 +431,10 @@ module Puppet
                 end
 
                 begin
-                    File.chown(self.should,-1,@parent[:path])
+                    File.chown(self.should,nil,@parent[:path])
                 rescue => detail
-                    error = Puppet::Error.new("failed to chown '%s' to '%s': %s" %
-                        [@parent[:path],self.should,detail])
-                    raise error
+                    raise Puppet::Error, "Failed to set owner of '%s' to '%s': %s" %
+                        [@parent[:path],self.should,detail]
                 end
 
                 return :inode_changed
@@ -496,23 +502,22 @@ module Puppet
                         end
                     end
                 else
-                    self.is = -1
+                    self.is = :notfound
                 end
 
                 #Puppet.debug "chmod state is %o" % self.is
             end
 
             def sync
-                if @is == -1
+                if @is == :notfound
                     @parent.stat(true)
                     self.retrieve
                     #Puppet.debug "%s: after refresh, is '%s'" % [self.class.name,@is]
-                end
-
-                unless @parent.stat
-                    Puppet.err "File '%s' does not exist; cannot chmod" %
-                        @parent[:path]
-                    return nil
+                    if @is == :notfound
+                        @parent.log "%s does not exist; cannot set mode" %
+                            @parent.name
+                        return nil
+                    end
                 end
 
                 unless defined? @fixed
@@ -556,6 +561,16 @@ module Puppet
                 gid = nil
                 gname = nil
 
+                unless Process.uid == 0
+                    unless defined? @@notifiedgroup
+                        Puppet.notice(  
+                            "Cannot manage group unless running as root"
+                        )
+                        @@notifiedgroup = true
+                    end
+                    return
+                end
+
                 if value.is_a?(Integer)
                     method = :getgrgid
                 else
@@ -610,8 +625,18 @@ module Puppet
             end
 
             def sync
+                unless Process.uid == 0
+                    unless defined? @@notifiedgroup
+                        Puppet.notice(  
+                            "Cannot manage group ownership unless running as root"
+                        )
+                        @@notifiedgroup = true
+                    end
+                    return nil
+                end
+
                 Puppet.debug "setting chgrp state to %s" % self.should
-                if @is == -1
+                if @is == :notfound
                     @parent.stat(true)
                     self.retrieve
                     #Puppet.debug "%s: after refresh, is '%s'" % [self.class.name,@is]
@@ -649,7 +674,13 @@ module Puppet
                 sourceobj, path = @parent.uri2obj(source)
                 server = sourceobj.server
 
-                desc = server.describe(path)
+                begin
+                    desc = server.describe(path)
+                rescue NetworkClientError => detail
+                    Puppet.err "Could not describe %s: %s" %
+                        [path, detail]
+                    return nil
+                end
 
                 args = {}
                 Puppet::Type::PFile::PINPARAMS.zip(
@@ -666,13 +697,17 @@ module Puppet
                     args.delete(:owner)
                 end
 
+                Puppet.notice "returning describe args %s" % args.inspect
                 return args
             end
 
             def retrieve
                 sum = nil
                 
-                @stats = self.describe
+                unless @stats = self.describe
+                    @is = :notdescribed
+                    return nil
+                end
 
                 @stats.each { |stat, value|
                     next if stat == :checksum
@@ -692,15 +727,15 @@ module Puppet
                 when "file":
                     if sum = @parent.state(:checksum)
                         if sum.is
-                            if sum.is == -1
+                            if sum.is == :notfound
                                 sum.retrieve
                             end
                             @is = sum.is
                         else
-                            @is = -1
+                            @is = :notfound
                         end
                     else
-                        @is = -1
+                        @is = :notfound
                     end
 
                     @should = @stats[:checksum]
@@ -742,8 +777,13 @@ module Puppet
             end
 
             def sync
-                if @is == -1
+                Puppet.notice "syncing %s" % @parent.name
+                if @is == :notdescribed
                     self.retrieve # try again
+                    if @is == :notdescribed
+                        @parent.log "Could not retreive information on %s" % @parent.name
+                        return nil
+                    end
                     if @is == @should
                         return nil
                     end
@@ -756,7 +796,15 @@ module Puppet
 
                 sourceobj, path = @parent.uri2obj(@source)
 
-                contents = sourceobj.server.retrieve(path)
+                begin
+                    contents = sourceobj.server.retrieve(path)
+                rescue NetworkClientError => detail
+                    Puppet.err "Could not retrieve %s: %s" %
+                        [path, detail]
+                    return nil
+                end
+
+                Puppet.notice "retrieved %s" % path
 
                 unless sourceobj.server.local
                     contents = CGI.unescape(contents)
@@ -778,8 +826,8 @@ module Puppet
 
                 # try to create it with the correct modes to start
                 # we should also be changing our effective uid/gid, but...
-                if @parent[:mode]
-                    args.push @parent[:mode]
+                if @parent.should(:mode) and @parent.should(:mode) != :notfound
+                    args.push @parent.should(:mode)
                 end
 
                 # FIXME we should also change our effective user and group id
@@ -1091,8 +1139,8 @@ module Puppet
                 if child = klass[path]
                     unless @children.include?(child)
                         raise Puppet::Error,
-                            "Planned child file %s already exists with parent %s" %
-                            [path, child.parent]
+                            "Planned child file %s of %s already exists with parent %s" %
+                            [path, self.name, child.parent]
                     end
                     args.each { |var,value|
                         next if var == :path
@@ -1253,7 +1301,7 @@ module Puppet
                 unless stat = self.stat(true)
                     Puppet.debug "File %s does not exist" % self.name
                     @states.each { |name,state|
-                        state.is = -1
+                        state.is = :notfound
                     }
                     return
                 end
@@ -1309,6 +1357,7 @@ module Puppet
                     if uri.port
                         args[:Port] = uri.port
                     end
+                    # FIXME We should cache a copy of this server
                     #sourceobj.server = Puppet::NetworkClient.new(args)
                     sourceobj.server = Puppet::Client::FileClient.new(args)
 
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index a4ec29a68..0d5d84b6f 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -77,13 +77,14 @@ class State < Puppet::Element
     end
     #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
-    # we aren't actually comparing the states themselves, we're only
-    # comparing the "should" value with the "is" value
+    # Determine whether the state is in-sync or not.  If @should is
+    # not defined or is set to a non-true value, then we do not have
+    # a valid value for it and thus consider the state to be in-sync
+    # since we cannot fix it.
     def insync?
         #debug "%s value is '%s', should be '%s'" %
         #    [self,self.is.inspect,self.should.inspect]
-        unless defined? @should
+        unless defined? @should and @should
             return true
         end
         self.is == self.should