Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Ability to Set Connection Timeout Values #289

Merged
merged 10 commits into from
Aug 30, 2017
Merged

Adding Ability to Set Connection Timeout Values #289

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8a7f60f
add ability to set connection timeouts, and misc rubocop style cleanups.
sgreen-r7 Aug 24, 2017
19232f0
write to error message the number of reconnect attempts on timeout error
sgreen-r7 Aug 24, 2017
5b63253
adding @ for consistency when referring to connection attrs
sgreen-r7 Aug 24, 2017
f940c3d
attempt retry based on timeout
sgreen-r7 Aug 24, 2017
e5860f5
fix namespace errors for credential helper
sgreen-r7 Aug 25, 2017
4fd277e
set default timeouts to 2min
sgreen-r7 Aug 29, 2017
dd33eca
add logging for odd request errors, removing retries for Timeout::Errors
sgreen-r7 Aug 29, 2017
3bdea1e
adding links to ruby docs for timeout information
sgreen-r7 Aug 29, 2017
e8f54c4
editing links to ruby docs for timeout information
sgreen-r7 Aug 29, 2017
80c4e57
fixed timeout urls to point to most current version of ruby
sgreen-r7 Aug 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions lib/nexpose/ajax.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# encoding: utf-8

module Nexpose
# Accessor to the Nexpose AJAX API.
# These core methods should allow direct access to underlying controllers
Expand Down Expand Up @@ -133,7 +134,8 @@ def parameterize_uri(uri, parameters)
# Use the Nexpose::Connection to establish a correct HTTPS object.
def https(nsc, timeout = nil)
http = Net::HTTP.new(nsc.host, nsc.port)
http.read_timeout = timeout if timeout
http.read_timeout = (timeout || nsc.timeout)
http.open_timeout = nsc.open_timeout
http.use_ssl = true
if nsc.trust_store.nil?
http.verify_mode = OpenSSL::SSL::VERIFY_NONE
Expand Down Expand Up @@ -183,16 +185,16 @@ def get_api_error(request, response)
def get_request_api_version(request)
matches = request.path.match(API_PATTERN)
matches[:version].to_f
rescue
0.0
rescue
0.0
end

# Get an error message from the response body if the request url api version
# is 2.1 or greater otherwise use the request body
def get_error_message(request, response)
version = get_request_api_version(request)
data_request = use_response_error_message?(request, response)
return_response = (version >= 2.1 || data_request )
return_response = (version >= 2.1 || data_request)
(return_response && response.body) ? "response body: #{response.body}" : "request body: #{request.body}"
end

Expand All @@ -202,7 +204,7 @@ def use_response_error_message?(request, response)
if (request.path.include?('/data/') && !response.content_type.nil?)
response.content_type.include? 'text/plain'
else
return false
false
end
end

Expand Down Expand Up @@ -253,7 +255,7 @@ def get_rows(nsc, pref)
pref_key = "#{pref}.rows"
resp = get(nsc, uri)
json = JSON.parse(resp)
if json.has_key?(pref_key)
if json.key?(pref_key)
rows = json[pref_key].to_i
rows > 0 ? rows : 10
else
Expand Down
49 changes: 26 additions & 23 deletions lib/nexpose/api_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,17 @@ def prepare_http_client
else
@http.cert_store = @trust_store
end
@headers = {'Content-Type' => 'text/xml'}
@headers = { 'Content-Type' => 'text/xml' }
@success = false
end

def execute(options = {})
time_tracker = Time.now
@conn_tries = 0

begin
prepare_http_client
@http.read_timeout = options[:timeout] if options.key? :timeout
@http.open_timeout = options.key?(:open_timeout) ? options[:open_timeout] : 60
@http.read_timeout = options.key?(:timeout) ? options[:timeout] : 120
@http.open_timeout = options.key?(:open_timeout) ? options[:open_timeout] : 120
@raw_response = @http.post(@uri.path, @req, @headers)
@raw_response_data = @raw_response.read_body

Expand All @@ -62,7 +62,7 @@ def execute(options = {})
@success = true
else
@success = false
@error = "User requested raw XML response. Not parsing failures."
@error = 'User requested raw XML response. Not parsing failures.'
end
else
@res = parse_xml(@raw_response_data)
Expand All @@ -74,19 +74,19 @@ def execute(options = {})

@sid = attributes['session-id']

if (attributes['success'] and attributes['success'].to_i == 1)
if (attributes['success'] && attributes['success'].to_i == 1)
@success = true
elsif @api_version =~ /1.2/ and @res and (@res.get_elements '//Exception').count < 1
elsif @api_version =~ /1.2/ && @res && (@res.get_elements '//Exception').count < 1
@success = true
else
@success = false
if @api_version =~ /1.2/
@res.elements.each('//Exception/Message') do |message|
@error = message.text.sub(/.*Exception: */, '')
@error = message.text.sub(/.*Exception: */, '')
end
@res.elements.each('//Exception/Stacktrace') do |stacktrace|
@trace = stacktrace.text
end
@res.elements.each('//Exception/Stacktrace') do |stacktrace|
@trace = stacktrace.text
end
else
@res.elements.each('//message') do |message|
@error = message.text.sub(/.*Exception: */, '')
Expand All @@ -97,27 +97,30 @@ def execute(options = {})
end
end
end
# This is a hack to handle corner cases where a heavily loaded Nexpose instance
# drops our HTTP connection before processing. We try 5 times to establish a
# connection in these situations. The actual exception occurs in the Ruby
# http library, which is why we use such generic error classes.
rescue OpenSSL::SSL::SSLError => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know code inspection complains about these but capturing the object makes runtime debugging easier, I would prefer they be kept.

# This is a hack to handle corner cases where a heavily loaded Nexpose instance
# drops our HTTP connection before processing. We try 5 times to establish a
# connection in these situations. The actual exception occurs in the Ruby
# http library, which is why we use such generic error classes.
rescue OpenSSL::SSL::SSLError
if @conn_tries < 5
@conn_tries += 1
retry
end
rescue ::ArgumentError, ::NoMethodError => e
rescue ::ArgumentError, ::NoMethodError
if @conn_tries < 5
@conn_tries += 1
retry
end
rescue ::Timeout::Error
rescue ::Timeout::Error => error
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless assignment to variable - error.

if @conn_tries < 5
@conn_tries += 1
# If an explicit timeout is set, don't retry.
retry unless options.key? :timeout
if options.key?(:timeout)
retry if (time_tracker + options[:timeout].to_i) > Time.now
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid more than 3 levels of block nesting.

Copy link
Contributor

@jmartin-tech jmartin-tech Aug 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not really be about this PR but more a general thought, should the retry here even occur now? Originally this was retried up to 5 times if no :timeout value was ever set, since there is now a default :timeout and the request surpassed it, would it make sense to simplify this now to just end at a single timeout either the default or user specified one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scott and I were talking about not having retries at all anymore, maybe. At least as far as time out retries go, they are often more of a problem than just failing as many Nexpose API endpoints will do a lot of work on a heavy-load console before returning a response. Retrying in that case just repeats the work being done which we don't want to do IMO.

else
retry
end
end
@error = "Nexpose did not respond within #{@http.read_timeout} seconds."
@error = "Nexpose did not respond within #{@http.read_timeout} seconds after #{@conn_tries} attempt(s)."
rescue ::Errno::EHOSTUNREACH, ::Errno::ENETDOWN, ::Errno::ENETUNREACH, ::Errno::ENETRESET, ::Errno::EHOSTDOWN, ::Errno::EACCES, ::Errno::EINVAL, ::Errno::EADDRNOTAVAIL
@error = 'Nexpose host is unreachable.'
# Handle console-level interrupts
Expand All @@ -129,15 +132,15 @@ def execute(options = {})
@error = "Error parsing response: #{exc.message}"
end

if !(@success or @error)
if !(@success || @error)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Favor unless over if for negative conditions.

@error = "Nexpose service returned an unrecognized response: #{@raw_response_data.inspect}"
end

@sid
end

def attributes(*args)
return if not @res.root
return unless @res.root
@res.root.attributes(*args)
end

Expand Down
44 changes: 24 additions & 20 deletions lib/nexpose/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,16 @@ class Connection
attr_reader :url
# The token used to login to the NSC
attr_reader :token

# The last XML request sent by this object, useful for debugging.
attr_reader :request_xml
# The last XML response received by this object, useful for debugging.
attr_reader :response_xml

# The trust store to validate connections against if any
attr_reader :trust_store
# The main HTTP read_timeout value
attr_accessor :timeout
# The optional HTTP open_timeout value
attr_accessor :open_timeout

# A constructor to load a Connection object from a URI
def self.from_uri(uri, user, pass, silo_id = nil, token = nil, trust_cert = nil)
Expand All @@ -76,23 +78,23 @@ def self.from_uri(uri, user, pass, silo_id = nil, token = nil, trust_cert = nil)
# @param [String] token The two-factor authentication (2FA) token for Nexpose sessions.
# @param [String] trust_cert The PEM-formatted web certificate of the Nexpose console. Used for SSL validation.
def initialize(ip, user, pass, port = 3780, silo_id = nil, token = nil, trust_cert = nil)
@host = ip
@port = port
@username = user
@password = pass
@token = token
@silo_id = silo_id
unless trust_cert.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to nit pick this is another case where code inspection is not friendly to debugging, by making this a single line call you can lose break point granularity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example of what you mean? are you saying you'd want to put a breakdown on the inside the unless statement? if that's the case, you'd be able to just put the breakpoint inside the create_trust_store method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again is just a convenience thing to me being able to break when trust_cert.nil? without setting a conditional on the break point that is already in the code and see @trust_store before and after the call to create_trust_store(trust_cert)

In this case it may not be all that useful since create_trust_store is local code, I am just adding context about some things that rubocop thinks are bad that do little to impact performance but can help debugging and readability. Especially if the method called is from some utility library that is used heavily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I see what you're saying. Personally, I am using breakpoints with binding.pry so for your example I would probably do binding.pry if trust_cert.nil? on a new line. Where I am assuming if we were to talk about "clicking an IDE" to create a breakpoint, that's what you're referring to.

That's a fair enough concern, and I'll keep it in mind going forward. On this particular line, do you have a preference if we leave it as is, or should I change it back?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to leave as is.

@trust_store = create_trust_store(trust_cert)
end
@session_id = nil
@url = "https://#{@host}:#{@port}/api/API_VERSION/xml"
@host = ip
@username = user
@password = pass
@port = port
@silo_id = silo_id
@token = token
@trust_store = create_trust_store(trust_cert) unless trust_cert.nil?
@session_id = nil
@url = "https://#{@host}:#{@port}/api/API_VERSION/xml"
@timeout = 120
@open_timeout = 120
end

# Establish a new connection and Session ID
def login
begin
login_hash = {'sync-id' => 0, 'password' => @password, 'user-id' => @username, 'token' => @token}
login_hash = { 'sync-id' => 0, 'password' => @password, 'user-id' => @username, 'token' => @token }
login_hash['silo-id'] = @silo_id if @silo_id
r = execute(make_xml('LoginRequest', login_hash))
if r.success
Expand All @@ -106,13 +108,15 @@ def login

# Logout of the current connection
def logout
r = execute(make_xml('LogoutRequest', {'sync-id' => 0}))
r = execute(make_xml('LogoutRequest', { 'sync-id' => 0 }))
return true if r.success
raise APIError.new(r, 'Logout failed')
end

# Execute an API request
def execute(xml, version = '1.1', options = {})
options.store(:timeout, @timeout) unless options.key?(:timeout)
options.store(:open_timeout, @open_timeout)
@request_xml = xml.to_s
@api_version = version
response = APIRequest.execute(@url, @request_xml, @api_version, options, @trust_store)
Expand All @@ -127,17 +131,17 @@ def execute(xml, version = '1.1', options = {})
# Would need to do something more sophisticated to grab
# all the associated image files.
def download(url, file_name = nil)
return nil if url.nil? or url.empty?
uri = URI.parse(url)
http = Net::HTTP.new(@host, @port)
return nil if (url.nil? || url.empty?)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use parentheses around the condition of an if.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually right, but currently wrong houndci-bot ... this can cause issues with compound if's . i would leave this

uri = URI.parse(url)
http = Net::HTTP.new(@host, @port)
http.use_ssl = true
if @trust_store.nil?
http.verify_mode = OpenSSL::SSL::VERIFY_NONE # XXX: security issue
else
http.cert_store = @trust_store
end
headers = {'Cookie' => "nexposeCCSessionID=#{@session_id}"}
resp = http.get(uri.to_s, headers)
headers = { 'Cookie' => "nexposeCCSessionID=#{@session_id}" }
resp = http.get(uri.to_s, headers)

if file_name
::File.open(file_name, 'wb') { |file| file.write(resp.body) }
Expand Down
6 changes: 3 additions & 3 deletions lib/nexpose/credential_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def set_remote_execution_service(username = nil, password = nil)
def set_ssh_service(username = nil, password = nil, elevation_type = nil, elevation_user = nil, elevation_password = nil)
self.user_name = username
self.password = password
self.permission_elevation_type = elevation_type || ElevationType::NONE
self.permission_elevation_type = elevation_type || Credential::ElevationType::NONE
self.permission_elevation_user = elevation_user
self.permission_elevation_password = elevation_password
self.service = Credential::Service::SSH
Expand All @@ -118,7 +118,7 @@ def set_ssh_key_service(username, pemkey, password = nil, elevation_type = nil,
self.user_name = username
self.password = password
self.pem_format_private_key = pemkey
self.permission_elevation_type = elevation_type || ElevationType::NONE
self.permission_elevation_type = elevation_type || Credential::ElevationType::NONE
self.permission_elevation_user = elevation_user
self.permission_elevation_password = elevation_password
self.service = Credential::Service::SSH_KEY
Expand All @@ -131,7 +131,7 @@ def set_snmp_service(community_name = nil)
end

# sets the Simple Network Management Protocol v3 service.
def set_snmpv3_service(authentication_type = AuthenticationType::NOAUTH, username = nil, password = nil, privacy_type = PrivacyType::NOPRIV, privacy_password = nil)
def set_snmpv3_service(authentication_type = Credential::AuthenticationType::NOAUTH, username = nil, password = nil, privacy_type = Credential::PrivacyType::NOPRIV, privacy_password = nil)
self.authentication_type = authentication_type
self.user_name = username
self.password = password
Expand Down