Re: [Puppet-dev] [PATCH/puppet 1/2] Feature #3383 RAL over REST

[Luke Kanies]

A couple of comments below.

On Mar 18, 2010, at 12:10 PM, Jesse Wolfe wrote:

ralsh --host works now, and is using REST.
A node running puppetd --listen will allow ralsh to find, search, and
modify live resources, via REST.

Signed-off-by: Jesse Wolfe <jes5...@gmail.com>
---
lib/puppet/application/ralsh.rb        |   56 ++++----------
lib/puppet/indirector/resource/ral.rb  |   48 ++++++++++++
lib/puppet/indirector/resource/rest.rb |    5 +
lib/puppet/indirector/rest.rb          |    2 +-
lib/puppet/network/rest_authconfig.rb  |    1 +
lib/puppet/resource.rb                 |   17 ++++-
lib/puppet/resource/catalog.rb         |    3 +-
lib/puppet/type.rb                     |    6 ++
lib/puppet/util/settings.rb            |    1 -
spec/unit/application/ralsh.rb         |   86 +++++++++++-----------

spec/unit/indirector/resource/ral.rb | 129 +++++++++++++++++++++++ +++++++++

spec/unit/indirector/resource/rest.rb  |   11 +++
spec/unit/indirector/rest.rb           |   15 +++-
spec/unit/network/rest_authconfig.rb   |    1 +
spec/unit/resource.rb                  |   19 +++++
15 files changed, 309 insertions(+), 91 deletions(-)
create mode 100644 lib/puppet/indirector/resource/ral.rb
create mode 100644 lib/puppet/indirector/resource/rest.rb
create mode 100644 spec/unit/indirector/resource/ral.rb
create mode 100755 spec/unit/indirector/resource/rest.rb

diff --git a/lib/puppet/application/ralsh.rb b/lib/puppet/ application/ralsh.rb

index b9f7a58..87cb68d 100644
--- a/lib/puppet/application/ralsh.rb
+++ b/lib/puppet/application/ralsh.rb
@@ -71,52 +71,24 @@ Puppet::Application.new(:ralsh) do
            trans.to_manifest
        }

-        text = if @host

- client = Puppet::Network::Client.resource.new(:Server => @host, :Port => Puppet[:puppetport])

-            unless client.read_cert
-                raise "client.read_cert failed"
-            end
-            begin
-                # They asked for a single resource.
-                if name
-                    transbucket = [client.describe(type, name)]
-                else
-                    # Else, list the whole thing out.
-                    transbucket = client.instances(type)
-                end
-            rescue Puppet::Network::XMLRPCClientError => exc
-                raise "client.list(#{type}) failed: #{exc.message}"
-            end

- transbucket.sort { |a,b| a.name <=> b.name }.collect(&format)

+        if @host
+            Puppet::Resource.indirection.terminus_class = :rest
+            port = Puppet[:puppetport]

+ key = ["https://#{host}:#{port}";, "production", "resources", type, name].join('/')

        else
-            if name

- obj = typeobj.instances.find { |o| o.name == name } || typeobj.new(:name => name, :check => properties)

-                vals = obj.retrieve
-
-                unless params.empty?
-                    params.each do |param, value|
-                        obj[param] = value
-                    end
-                    catalog = Puppet::Resource::Catalog.new
-                    catalog.add_resource obj
-                    begin
-                        catalog.apply
-                    rescue => detail
-                        if Puppet[:trace]
-                            puts detail.backtrace
-                        end
-                    end
+            key = [type, name].join('/')
+        end

-                end
-                [format.call(obj.to_trans(true))]
+        text = if name
+            if params.empty?
+                [ Puppet::Resource.find( key ) ]
            else
-                typeobj.instances.collect do |obj|

- next if ARGV.length > 0 and ! ARGV.include? obj.name

-                    trans = obj.to_trans(true)
-                    format.call(trans)
-                end

+ request = Puppet::Indirector::Request.new(:resource, :save, key) # Yuck. + [ Puppet::Resource.new( type, name, params ).save( request ) ]

Again, why the direct use of a request here?

            end
-        end.compact.join("\n")
+        else
+            Puppet::Resource.search( key, {} )
+        end.map(&format).join("\n")

        if options[:edit]
            file = "/tmp/x2puppet-#{Process.pid}.pp"

diff --git a/lib/puppet/indirector/resource/ral.rb b/lib/puppet/ indirector/resource/ral.rb

new file mode 100644
index 0000000..f2c3f84
--- /dev/null
+++ b/lib/puppet/indirector/resource/ral.rb
@@ -0,0 +1,48 @@
+class Puppet::Resource::Ral < Puppet::Indirector::Code
+    def find( request )
+        # find by name

+ res = type(request).instances.find { |o| o.name == resource_name(request) } + res ||= type(request).new(:name => resource_name(request), :check => type(request).properties.collect { |s| s.name })

+
+        return res.to_resource
+    end
+
+    def search( request )
+        conditions = request.options.dup

+ conditions[:name] = resource_name(request) if resource_name(request)

+
+        type(request).instances.map do |res|
+            res.to_resource
+        end.find_all do |res|

+ conditions.all? {|property, value| res.to_resource[property].to_s == value.to_s}

+        end.sort do |a,b|
+            a.title <=> b.title
+        end
+    end
+
+    def save( request )

+ # In RAL-land, to "save" means to actually try to change machine state

+        res = request.instance
+        ral_res = res.to_ral
+
+        catalog = Puppet::Resource::Catalog.new
+        catalog.add_resource ral_res
+        catalog.apply
+
+        return ral_res.to_resource
+    end
+
+    private
+
+    def type_name( request )
+        request.key.split('/')[0]
+    end
+
+    def resource_name( request )
+        request.key.split('/')[1]
+    end
+
+    def type( request )

+ Puppet::Type.type(type_name(request)) or raise Puppet::Error "Could not find type #{type}"

+    end
+end

diff --git a/lib/puppet/indirector/resource/rest.rb b/lib/puppet/ indirector/resource/rest.rb

new file mode 100644
index 0000000..7848ae6
--- /dev/null
+++ b/lib/puppet/indirector/resource/rest.rb
@@ -0,0 +1,5 @@
+require 'puppet/indirector/status'
+require 'puppet/indirector/rest'
+
+class Puppet::Resource::Rest < Puppet::Indirector::REST
+end

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/ rest.rb

index a89e986..4fd3859 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb

@@ -67,7 +67,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus

    def find(request)

return nil unless result = deserialize(network(request).get(indirection2uri(request), headers))

-        result.name = request.key
+        result.name = request.key if result.respond_to?(:name=)
        result
    end

diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/ network/rest_authconfig.rb

index 01ed412..7c0ef9c 100644
--- a/lib/puppet/network/rest_authconfig.rb
+++ b/lib/puppet/network/rest_authconfig.rb
@@ -16,6 +16,7 @@ module Puppet

{ :acl => "/certificate/", :method => :find, :authenticated => false }, { :acl => "/certificate_request", :method => [:find, :save], :authenticated => false }, { :acl => "/status", :method => [:find], :authenticated => true }, + { :acl => "/resource", :method => [:find, :save, :search], :authenticated => true },

        ]

        def self.main
diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
index 91dd547..cbb6528 100644
--- a/lib/puppet/resource.rb
+++ b/lib/puppet/resource.rb
@@ -1,6 +1,6 @@
require 'puppet'
require 'puppet/util/tagging'
-require 'puppet/resource/reference'
+#require 'puppet/resource/reference'
require 'puppet/util/pson'

# The simplest resource class.  Eventually it will function as the
@@ -12,6 +12,10 @@ class Puppet::Resource
    attr_accessor :file, :line, :catalog, :exported, :virtual
    attr_writer :type, :title

+    require 'puppet/indirector'
+    extend Puppet::Indirector
+    indirects :resource, :terminus_class => :ral
+
    ATTRIBUTES = [:file, :line, :exported]

    def self.from_pson(pson)
@@ -221,6 +225,17 @@ class Puppet::Resource
        return result
    end

+    def name
+        # this is potential namespace conflict
+        # between the notion of an "indirector name"
+        # and a "resource name"
+        [ type, title ].join('/')
+    end
+
+    def to_resource
+        self
+    end
+
    private

    # Produce a canonical method name.

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/ catalog.rb

index cb61285..ff967b3 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -1,3 +1,4 @@
+require 'puppet/resource'
require 'puppet/node'
require 'puppet/indirector'
require 'puppet/simple_graph'

@@ -79,7 +80,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph

            # If the name and title differ, set up an alias

- if resource.respond_to?(:name) and resource.respond_to? (:title) and resource.name != resource.title + if resource.respond_to?(:name) and resource.respond_to? (:title) and resource.respond_to?(:isomorphic?) and resource.name != resource.title self.alias(resource, resource.name) if resource.isomorphic?

            end

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 2f7b57a..f377eb6 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -2079,6 +2079,12 @@ class Type
        return trans
    end

+    def to_resource

+ # this 'type instance' versus 'resource' distinction seems artificial

+        # I'd like to see it collapsed someday ~JW
+        self.to_trans.to_resource

Oh yes, definitely.

+    end
+
    %w{exported virtual}.each do |m|
        define_method(m+"?") do
            self.send(m)
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index e6e1333..cf63e1a 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -1,6 +1,5 @@
require 'puppet'
require 'sync'
-require 'puppet/transportable'
require 'getoptlong'

require 'puppet/external/event-loop'

diff --git a/spec/unit/application/ralsh.rb b/spec/unit/application/ ralsh.rb

index b0fa3bd..37e5a24 100755
--- a/spec/unit/application/ralsh.rb
+++ b/spec/unit/application/ralsh.rb
@@ -179,76 +179,76 @@ describe "ralsh" do
            before :each do
                @ralsh.stubs(:puts)
                @ralsh.host = 'host'
-                @client = stub_everything 'client'
-                @client.stubs(:read_cert).returns(true)
-                @client.stubs(:instances).returns([])

- Puppet::Network::Client.resource.stubs(:new).returns(@client)

+
+                Puppet::Resource.stubs(:find  ).never
+                Puppet::Resource.stubs(:search).never
+                Puppet::Resource.stubs(:save  ).never
            end

-            it "should connect to it" do

- Puppet::Network::Client.resource.expects(:new).with { |h| h[:Server] == 'host' }.returns(@client)

+            it "should search for resources" do

+ Puppet::Resource.expects(:search).with('https://host:8139/production/resources/type/' , {}).returns([])

                @ralsh.main
            end

-            it "should raise an error if there are no certs" do
-                @client.stubs(:read_cert).returns(nil)
-
-                lambda { @ralsh.main }.should raise_error
+            it "should describe the given resource" do
+                push_args('type','name')
+                x = stub_everything 'resource'
+                
Puppet::Resource.expects(:find).with('https://host:8139/production/resources/type/name').returns(x)
+                @ralsh.main
+                pop_args
            end

- it "should retrieve all the instances if there is no name" do

-                @client.expects(:instances).returns([])
+            it "should add given parameters to the object" do
+                push_args('type','name','param=temp')

-                @ralsh.main
-            end
+                res = stub "resource"

+ res.expects(:save).with{|x| x.uri == 'https://host:8139/production/resources/type/name' }.returns(res)

+                res.expects(:collect)
+                res.expects(:to_manifest)

+ Puppet::Resource.expects(:new).with('type', 'name', {'param' => 'temp'}).returns(res)

-            it "should describe the given resource" do
-                push_args('type','name')
-                @client.expects(:describe).returns(stub_everything)
                @ralsh.main
                pop_args
            end
+
        end

        describe "without a host" do
            before :each do
                @ralsh.stubs(:puts)
                @ralsh.host = nil
-            end

- it "should retrieve all the instances if there is no name" do

-                @type.expects(:instances).returns([])
+                Puppet::Resource.stubs(:find  ).never
+                Puppet::Resource.stubs(:search).never
+                Puppet::Resource.stubs(:save  ).never
+            end

+            it "should search for resources" do

+ Puppet::Resource.expects(:search).with('type/', {}).returns([])

                @ralsh.main
            end

-            describe 'but with a given name' do
-                before :each do
-                    push_args('type','name')
-                    @type.stubs(:new).returns(:bob)
-                end
-
-                after :each do
-                    pop_args
-                end
-

- it "should retrieve a specific instance if it exists" do

-                    pending
-                end
+            it "should describe the given resource" do
+                push_args('type','name')
+                x = stub_everything 'resource'

+ Puppet::Resource.expects(:find).with('type/ name').returns(x)

+                @ralsh.main
+                pop_args
+            end

+            it "should add given parameters to the object" do
+                push_args('type','name','param=temp')

- it "should create a stub instance if it doesn't exist" do

-                    pending
-                end
+                res = stub "resource"

+ res.expects(:save).with{|x| x.uri == nil}.returns(res)

+                res.expects(:collect)
+                res.expects(:to_manifest)

+ Puppet::Resource.expects(:new).with('type', 'name', {'param' => 'temp'}).returns(res)

-                it "should add given parameters to the object" do
-                    push_args('type','name','param=temp')
-                    pending
-                    @object.expects(:[]=).with('param','temp')
-                    @ralsh.main
-                    pop_args
-                end
+                @ralsh.main
+                pop_args
            end
+
        end
    end
end

diff --git a/spec/unit/indirector/resource/ral.rb b/spec/unit/ indirector/resource/ral.rb

new file mode 100644
index 0000000..f74bf3d
--- /dev/null
+++ b/spec/unit/indirector/resource/ral.rb
@@ -0,0 +1,129 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+describe "Puppet::Resource::Ral" do
+    describe "find" do
+        before do
+            @request = stub 'request', :key => "user/root"
+        end
+
+        it "should find an existing instance" do
+            my_resource    = stub "my user resource"
+
+            wrong_instance = stub "wrong user", :name => "bob"

+ my_instance = stub "my user", :name => "root", :to_resource => my_resource

+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ wrong_instance, my_instance, wrong_instance ]) + Puppet::Resource::Ral.new.find(@request).should == my_resource

+        end
+
+        it "if there is no instance, it should create one" do
+            wrong_instance = stub "wrong user", :name => "bob"
+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ wrong_instance, wrong_instance ])

+            result = Puppet::Resource::Ral.new.find(@request)
+            result.should be_is_a Puppet::Resource
+            result.title.should == "root"
+        end
+    end
+
+    describe "search" do
+        before do

+ @request = stub 'request', :key => "user/", :options => {}

+        end
+
+        it "should convert ral resources into regular resources" do
+            my_resource = stub "my user resource"

+ my_instance = stub "my user", :name => "root", :to_resource => my_resource

+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ my_instance ]) + Puppet::Resource::Ral.new.search(@request).should == [my_resource]

+        end
+

+ it "should filter results by name if there's a name in the key" do

+            my_resource    = stub "my user resource"
+            my_resource.stubs(:to_resource).returns(my_resource)
+            my_resource.stubs(:[]).with(:name).returns("root")
+
+            wrong_resource = stub "wrong resource"

+ wrong_resource.stubs(:to_resource).returns(wrong_resource)

+            wrong_resource.stubs(:[]).with(:name).returns("bad")
+

+ my_instance = stub "my user", :to_resource => my_resource + wrong_instance = stub "wrong user", :to_resource => wrong_resource

+

+ @request = stub 'request', :key => "user/ root", :options => {}

+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ my_instance, wrong_instance ]) + Puppet::Resource::Ral.new.search(@request).should == [my_resource]

+        end
+
+        it "should filter results by query parameters" do
+            wrong_resource = stub "my user resource"

+ wrong_resource.stubs(:to_resource).returns(wrong_resource)

+            wrong_resource.stubs(:[]).with(:name).returns("root")
+
+            my_resource = stub "wrong resource"
+            my_resource.stubs(:to_resource).returns(my_resource)
+            my_resource.stubs(:[]).with(:name).returns("bob")
+

+ my_instance = stub "my user", :to_resource => my_resource + wrong_instance = stub "wrong user", :to_resource => wrong_resource

+

+ @request = stub 'request', :key => "user/", :options => {:name => "bob"}

+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ my_instance, wrong_instance ]) + Puppet::Resource::Ral.new.search(@request).should == [my_resource]

+        end
+
+        it "should return sorted results" do
+            a_resource = stub "alice resource"
+            a_resource.stubs(:to_resource).returns(a_resource)
+            a_resource.stubs(:title).returns("alice")
+
+            b_resource = stub "bob resource"
+            b_resource.stubs(:to_resource).returns(b_resource)
+            b_resource.stubs(:title).returns("bob")
+

+ a_instance = stub "alice user", :to_resource => a_resource + b_instance = stub "bob user", :to_resource => b_resource

+

+ @request = stub 'request', :key => "user/", :options => {}

+
+            require 'puppet/type/user'

+ Puppet::Type::User.expects(:instances).returns([ b_instance, a_instance ]) + Puppet::Resource::Ral.new.search(@request).should == [a_resource, b_resource]

+        end
+    end
+
+    describe "save" do
+        before do
+            @rebuilt_res = stub 'rebuilt instance'

+ @ral_res = stub 'ral resource', :to_resource => @rebuilt_res

+            @instance    = stub 'instance', :to_ral => @ral_res

+ @request = stub 'request', :key => "user/", :instance => @instance

+            @catalog     = stub 'catalog'
+
+            Puppet::Resource::Catalog.stubs(:new).returns(@catalog)
+            @catalog.stubs(:apply)
+            @catalog.stubs(:add_resource)
+        end
+
+        it "should apply a new catalog with a ral object in it" do
+            Puppet::Resource::Catalog.expects(:new).returns(@catalog)
+            @catalog.expects(:add_resource).with(@ral_res)
+            @catalog.expects(:apply)
+            Puppet::Resource::Ral.new.save(@request)
+        end
+

+ it "should return a regular resource that used to be the ral resource" do + Puppet::Resource::Ral.new.save(@request).should == @rebuilt_res

+        end
+    end
+end

diff --git a/spec/unit/indirector/resource/rest.rb b/spec/unit/ indirector/resource/rest.rb

new file mode 100755
index 0000000..d5f2a9d
--- /dev/null
+++ b/spec/unit/indirector/resource/rest.rb
@@ -0,0 +1,11 @@
+#!/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/indirector/resource/rest'
+
+describe Puppet::Resource::Rest do
+    it "should be a sublcass of Puppet::Indirector::REST" do

+ Puppet::Resource::Rest.superclass.should equal(Puppet::Indirector::REST)

+    end
+end

diff --git a/spec/unit/indirector/rest.rb b/spec/unit/indirector/ rest.rb

index d12e3c6..1cb34c4 100755
--- a/spec/unit/indirector/rest.rb
+++ b/spec/unit/indirector/rest.rb
@@ -199,15 +199,26 @@ describe Puppet::Indirector::REST do
            @searcher.find(@request)
        end

-        it "should deserialize and return the http response" do

+ it "should deserialize and return the http response, setting name" do

            @connection.expects(:get).returns @response

-            instance = stub 'object', :name= => nil
+            instance = stub 'object'
+            instance.expects(:name=)

@searcher.expects(:deserialize).with(@response).returns instance

            @searcher.find(@request).should == instance
        end

+ it "should deserialize and return the http response, and not require name=" do

+            @connection.expects(:get).returns @response
+
+            instance = stub 'object'

+ @searcher.expects(:deserialize).with(@response).returns instance

+
+            @searcher.find(@request).should == instance
+        end
+
+
        it "should use the URI generated by the Handler module" do

@searcher.expects(:indirection2uri).with(@request).returns "/my/uri" @connection.expects(:get).with { |path, args| path == "/ my/uri" }.returns(@response) diff --git a/spec/unit/network/rest_authconfig.rb b/spec/unit/ network/rest_authconfig.rb

index 407fc43..fe17d56 100755
--- a/spec/unit/network/rest_authconfig.rb
+++ b/spec/unit/network/rest_authconfig.rb
@@ -17,6 +17,7 @@ describe Puppet::Network::RestAuthConfig do

{ :acl => "/certificate/", :method => :find, :authenticated => false }, { :acl => "/certificate_request", :method => [:find, :save], :authenticated => false }, { :acl => "/status", :method => [:find], :authenticated => true }, + { :acl => "/resource", :method => [:find, :save, :search], :authenticated => true },

    ]

    before :each do
diff --git a/spec/unit/resource.rb b/spec/unit/resource.rb
index b26f4f9..2b1d49d 100755
--- a/spec/unit/resource.rb
+++ b/spec/unit/resource.rb
@@ -492,4 +492,23 @@ describe Puppet::Resource do
            resource['foo'].should == %w{one}
        end
    end
+
+    describe "it should implement to_resource" do
+        resource = Puppet::Resource.new("file", "/my/file")
+        resource.to_resource.should == resource
+    end
+
+    describe "because it is an indirector model" do
+        it "should include Puppet::Indirector" do
+            Puppet::Resource.should be_is_a(Puppet::Indirector)
+        end
+
+        it "should have a default terminus" do

+ Puppet::Resource.indirection.terminus_class.should == :ral

+        end
+
+        it "should have a name" do

+ Puppet::Resource.new("file", "/my/file").name.should == "File//my/file"

+        end
+    end
end
--
1.6.3.3

--

You received this message because you are subscribed to the Google Groups "Puppet Developers" group.

To post to this group, send email to puppet-...@googlegroups.com.

To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com . For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en .

--
A government that robs Peter to pay Paul can always depend on the
support of Paul. -- George Bernard Shaw
---------------------------------------------------------------------
Luke Kanies  -|-   http://reductivelabs.com   -|-   +1(615)594-8199

--
You received this message because you are subscribed to the Google Groups "Puppet 
Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to 
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/puppet-dev?hl=en.