On 05/05/2011 07:35 AM, Mark McLoughlin wrote:
Hi Mo,
On Thu, 2011-05-05 at 00:41 -0400, Mo Morsi wrote:
Introduces a generic web puppet resource type to access various web apis from puppet. Implements a provider of this type using curl to issue the requests.
Then uses this provider to issue requests to the conductor api to create providers and hardware profiles, analyzing the results for errors
Tricky stuff! Even though curl.rb is considerably more complex than exec-ing the curl command lines, the resulting web resources are much nicer. Cool stuff :)
Thanks. Agree, on the backend its a little more complicated than just calling out to curl, but the result is the interface is much more cleaner and we can do alot more in terms of verifying the result and output to make sure the operation proceeded as intended.
ACK from my FWIW, but some comments inline
diff --git a/recipes/aeolus_recipe/lib/puppet/provider/web/curl.rb b/recipes/aeolus_recipe/lib/puppet/provider/web/curl.rb new file mode 100644 index 0000000..b5f4468 --- /dev/null +++ b/recipes/aeolus_recipe/lib/puppet/provider/web/curl.rb
....
+# Puppet provider definition +Puppet::Type.type(:web).provide :curl do
- desc "Use curl to access web resources"
- def http_method
- @request_method
- end
- def http_method=(request_method)
- begin
# Set request method and generate a unique session key
@request_method = request_method
session = "/tmp/#{UUID.new.generate}"
# Invoke a login request if necessary
...
I'm a bit surprised to see all this happening in http_method=
Care to explain why, just for my own education? :)
Sure. When it comes to puppet resource types and providers you can only perform operations in properties, for example in the 'service' provider type, setting the 'ensure' property to 'running' will invoke 'service <name> start', while setting the 'enable' property to 'true' will invoke 'chkconfig <name> on'.
Honestly wasn't sure what the best name for the property to actual perform the operation in in this case, if you have a better suggestion than 'http_method', I'm all ears.
+define aeolus::conductor::provider($type="",$url=""){
- web{ "provider-$name":
- uri => "https://localhost/conductor/admin/providers",
- http_method => 'post',
- parameters => { 'provider[name]' => $name, 'provider[url]' => $url,
'provider[provider_type_codename]' => $type },
- returns => '200',
- verify => '.*Provider added.*',
- follow => true,
- unless => { 'http_method' => 'get',
'uri' => 'https://localhost/conductor/admin/providers',
'verify' => ".*$name.*" },
Would something like this be better:
unless => { http_method => 'get', uri => 'https://localhost/conductor/admin/providers', parameters => { 'q' => $name }, verify => ".*/admin/providers/.*>$name</a>.*" }
i.e. run a search query and use a more specific regexp?
Well it actually does run a search query as is right now. And the nice thing is that we can set the regex to whatever we want to look for in the aeolus-configuration recipe without modifying the backend web provider itself.
At some point though I would like to expand this to not only be able to supply a regex but an xpath location at which that regex should appear.
Also, now that I type that ... it looks like "unless" doesn't re-use the web type, right? i.e. parameters are automatically provided? It'd be cool if it re-used the same type somehow
It does reuse the web type. You can specify the parameters as you suggested as is right now.
At some point I would like to add an 'if' parameter, eg only invoke the web request if the 'if' request yields true.
+define aeolus::conductor::hwp($memory='', $cpu='', $storage='', $architecture=''){
- web{ "hwp-$name":
- uri => "https://localhost/conductor/admin/hardware_profiles",
- http_method => 'post',
- parameters => {'hardware_profile[name]' => $name,
'hardware_profile[memory_attributes][value]' => $memory,
'hardware_profile[cpu_attributes][value]' => $cpu,
'hardware_profile[storage_attributes][value]' => $storage,
'hardware_profile[architecture_attributes][value]' => $architecture,
'hardware_profile[memory_attributes][name]' => 'memory',
'hardware_profile[memory_attributes][unit]' => 'MB',
'hardware_profile[cpu_attributes][name]' => 'cpu',
'hardware_profile[cpu_attributes][unit]' => 'count',
'hardware_profile[storage_attributes][name]' => 'storage',
'hardware_profile[storage_attributes][unit]' => 'GB',
'hardware_profile[architecture_attributes][name]' => 'architecture',
'hardware_profile[architecture_attributes][unit]' => 'label',
'commit' => 'Save'},
- returns => '200',
- #verify => '.*Hardware profile added.*',
- follow => true,
- unless => { 'http_method' => 'get',
'uri' => 'https://localhost/conductor/admin/hardware_profiles',
'verify' => ".*$name.*" },
Is there something weird going on with GET on the hardware_profiles resource?
Without 'Accept: text/html', I'm seeing:
The error occurred while evaluating nil.[]): app/controllers/admin/hardware_profiles_controller.rb:206:in `build_hardware_profile' app/controllers/admin/hardware_profiles_controller.rb:14:in `index' app/controllers/admin/hardware_profiles_controller.rb:11:in `index' compass (0.10.4) lib/compass/app_integration/rails/actionpack2/action_controller.rb:7:in `process'
i.e. it's following the "format.js" path and breaking. Is that a conductor bug?
Not fully sure what the issue your describing here. Do we need to specify this additional header in the request for this to work? How does format.js play into this?
It seemed to be fully working when I tried it out, not sure what the difference between our environments is. If there is an error in the conductor logs due to this request, I'm tempted to say its an issue on that side (since the request looks like it should be valid). We'll need to debug this further tho.
If you supply 'Accept: text/html' does the commented out "verify" above start working? An accept header seems like a reasonable parameter for the web type to support
The reason verify is commented out in this case, is that even if the request succeeds, the conductor wui doesn't display 'HWP added' or anything similar. This should be added to the conductor so that we can verify the operation here, or better yet we should provide a restful api in the conductor so that we can verify the resulting http status code.
diff --git a/recipes/aeolus_recipe/manifests/defaults.pp b/recipes/aeolus_recipe/manifests/defaults.pp index f5c74c3..57b79f6 100644 --- a/recipes/aeolus_recipe/manifests/defaults.pp +++ b/recipes/aeolus_recipe/manifests/defaults.pp @@ -8,3 +8,16 @@
Package {provider => 'rpm'}
+$admin_user='admin' +$admin_password='password'
+# Setup the default login/logout targets for web requests +Web{
- login => { 'http_method' => 'post',
'uri' => 'https://localhost/conductor/user_session',
'user_session[login]' => "$admin_user",
'user_session[password]' => "$admin_password",
'commit' => 'submit' },
- logout => { 'http_method' => 'post',
'uri' => 'https://localhost/conductor/logout' }
A GET on /logout will work too, a POST isn't needed
Ah good point, will change this before I push the patch.
Thanks for the feedback / ACK.
-Mo