Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

(#15975) Refactor/3.x/extract http methods from rest indirector #1040

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

cprice404 commented Aug 15, 2012

There were some useful HTTP methods (http_get, http_post, ...,
http_request) in the Puppet::Indirector::REST class. Most noteworthy
is the http_request method, which provides some useful error
handling so that users get less confusing error messages when something
goes wrong with their certificates / SSL setup.

These methods would be useful in several other places (among them: the
HTTP reports processor and several places in the puppetdb termini), but
prior to this pull req it was very difficult (or impossible) to use them
without doing a very confusing dance with unnecessary ruby mixins and
inheritance from classes in the Indirectory hierarchy.

This pull request factors these methods out into the new
Puppet::Network::HTTP::Connection class, and tweaks
Puppet::Network::HttpPool to return instances of this new class.

Chris Price added some commits Aug 15, 2012

(#15975) Extract HTTP methods from REST Indirector
There were some useful HTTP methods (http_get, http_post, ...,
http_request) in the Puppet::Indirector::REST class. Most noteworthy
is the http_request method, which provides some useful error
handling so that users get less confusing error messages when something
goes wrong with their certificates / SSL setup.

These methods would be useful in several other places (among them: the
HTTP reports processor and several places in the puppetdb termini), but
prior to this commit it was very difficult (or impossible) to use them
without doing a very confusing dance with unnecessary ruby mixins and
inheritance from classes in the Indirectory hierarchy.

This commit factors these methods out into the new
Puppet::Network::HTTP::Connection class, and tweaks
Puppet::Network::HttpPool to return instances of this new class.
(#15975) Refactor HTTP report processor to use Puppet::Network::HTTP:…
…:Connection

Prior to this commit, the HTTP report processor relied on some
methods defined by Net::HTTP (#start, in particular) that didn't
quite line up with our HTTP utility methods (as defined in
Puppet::Network::HTTP::Connection).  This commit tweaks the
HTTP report processor to work properly with the Connection
object, which should allow it to benefit from the error
handling therein.

@pcarlisle pcarlisle and 1 other commented on an outdated diff Aug 15, 2012

lib/puppet/network/http/connection.rb
+ # --daniel 2012-06-03
+
+ # Ruby 1.8 defaulted to this, but 1.9 defaults to peer verify, and we
+ # almost always talk to a dedicated, not-standard CA that isn't trusted
+ # out of the box. This forces the expected state.
+ @http_conn.verify_mode = OpenSSL::SSL::VERIFY_NONE
+ end
+ end
+
+ # This method largely exists for testing purposes, so that we can
+ # mock the actual HTTP connection.
+ def create_http_conn(*args)
+ Net::HTTP.new(*args)
+ end
+
+ # Use the global localhost instance.
@cprice404

cprice404 Aug 15, 2012

Contributor

This was copied verbatim from the previous implementation... yeah, it sucks. sorry.

@pcarlisle pcarlisle and 1 other commented on an outdated diff Aug 15, 2012

lib/puppet/network/http/connection.rb
+
+ def address
+ http_conn.address
+ end
+
+ def port
+ http_conn.port
+ end
+
+ def use_ssl?
+ http_conn.use_ssl?
+ end
+
+ private
+
+ def http_conn
@pcarlisle

pcarlisle Aug 15, 2012

Contributor

why is http_con better than connection

@cprice404

cprice404 Aug 15, 2012

Contributor

It's not.

@pcarlisle pcarlisle and 1 other commented on an outdated diff Aug 15, 2012

lib/puppet/network/http_pool.rb
module Puppet::Network; end
+# This class is basically a placeholder for managing a pool of HTTP connections; at present
+# it does not actually attempt to pool them, but provides an abstraction that is used in
+# other places in the code should we decide to implement pooling at some point in the
+# future.
@pcarlisle

pcarlisle Aug 15, 2012

Contributor

The real reason it's here is that we used to have pooling, not that we plan on adding it.

@cprice404

cprice404 Aug 15, 2012

Contributor

Ah. Do you think the wording of this comment is misleading for future devs, then?

@pcarlisle pcarlisle commented on the diff Aug 15, 2012

lib/puppet/reports/http.rb
use_ssl = url.scheme == 'https'
conn = Puppet::Network::HttpPool.http_instance(url.host, url.port, use_ssl)
- conn.start {|http|
- response = http.request(req)
- unless response.kind_of?(Net::HTTPSuccess)
- Puppet.err "Unable to submit report to #{Puppet[:reporturl].to_s} [#{response.code}] #{response.msg}"
- end
- }
+ response = conn.post(url.path, body, headers)
+ unless response.kind_of?(Net::HTTPSuccess)
+ Puppet.err "Unable to submit report to #{Puppet[:reporturl].to_s} [#{response.code}] #{response.msg}"
+ end
@pcarlisle

pcarlisle Aug 15, 2012

Contributor

i think by moving away from the block you are leaving out some cleanup

@cprice404

cprice404 Aug 15, 2012

Contributor

I was a bit concerned about that but wasn't sure how to tell what I'd be missing. Also, the type of the conn object is different now; it's no longer a Net::HTTP, it's instead a Puppet::Network::HTTP::Connection. So it may be that if this cleanup is important then it needs to go inside of the Connection class, rather than here?

@zaphod42

zaphod42 Aug 15, 2012

Contributor

Looking at the docs I don't see any kind of cleanup. The start method is for reusing a single connection for multiple requests, but that wasn't happening there anyway.

@pcarlisle pcarlisle commented on the diff Aug 17, 2012

lib/puppet/network/http_pool.rb
- # certificate details if we have them? The original code didn't.
- # --daniel 2012-06-03
-
- # Ruby 1.8 defaulted to this, but 1.9 defaults to peer verify, and we
- # almost always talk to a dedicated, not-standard CA that isn't trusted
- # out of the box. This forces the expected state.
- http.verify_mode = OpenSSL::SSL::VERIFY_NONE
- end
- end
-
- def self.ssl_configuration
- @ssl_configuration ||= Puppet::SSL::Configuration.new(
- Puppet[:localcacert],
- :ca_chain_file => Puppet[:ssl_client_ca_chain],
- :ca_auth_file => Puppet[:ssl_client_ca_auth])
- end
# Retrieve a cached http instance if caching is enabled, else return
# a new one.
def self.http_instance(host, port, use_ssl = true)
@pcarlisle

pcarlisle Aug 17, 2012

Contributor

Should we deprecate this?

@pcarlisle pcarlisle commented on the diff Aug 17, 2012

lib/puppet/reports/http.rb
@@ -12,16 +12,13 @@
def process
url = URI.parse(Puppet[:reporturl])
- req = Net::HTTP::Post.new(url.path)
- req.body = self.to_yaml
- req.content_type = "application/x-yaml"
+ body = self.to_yaml
+ headers = { "Content-Type" => "application/x-yaml" }
use_ssl = url.scheme == 'https'
conn = Puppet::Network::HttpPool.http_instance(url.host, url.port, use_ssl)
@pcarlisle

pcarlisle Aug 17, 2012

Contributor

Might as well call the new class directly

Contributor

cprice404 commented Aug 20, 2012

Patrick merged this in 6858f02

@cprice404 cprice404 closed this Aug 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment