Skip to content
This repository was archived by the owner on Nov 15, 2019. It is now read-only.

Add optional validate param to raw_connect #100

Closed
wants to merge 2 commits into from
Closed

Add optional validate param to raw_connect #100

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2d24b8
Add optional validate param to raw_connect
tumido Nov 6, 2017
d1efd7b
Set connection_rescue_block class methods private
tumido Nov 7, 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
51 changes: 32 additions & 19 deletions app/models/manageiq/providers/hawkular/middleware_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,26 +51,35 @@ class Hawkular::MiddlewareManager < ManageIQ::Providers::MiddlewareManager
attr_accessor :client

def verify_credentials(_auth_type = nil, options = {})
begin
# As the connect will only give a handle
# we verify the credentials via an actual operation
connect(options).inventory.list_feeds
rescue URI::InvalidComponentError
raise MiqException::MiqHostError, "Host '#{hostname}' is invalid"
rescue ::Hawkular::ConnectionException
raise MiqException::MiqUnreachableError, "Unable to connect to #{hostname}:#{port}"
rescue ::Hawkular::Exception => he
raise MiqException::MiqInvalidCredentialsError, 'Invalid credentials' if he.status_code == 401
raise MiqException::MiqHostError, 'Hawkular not found on host' if he.status_code == 404
raise MiqException::MiqCommunicationsError, he.message
rescue => err
$log.error(err)
raise MiqException::Error, 'Unable to verify credentials'
end
self.class.validate_connection(connect(options), options[:hostname], options[:port])

true
end

def self.validate_connection(connection, hostname = nil, port = nil)
Copy link
Member

Choose a reason for hiding this comment

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

Mark this as private

Copy link
Member Author

Choose a reason for hiding this comment

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

Why it should be private? Neither VmWare nor OpenStack nor Amazon nor Azure has these methods as private.

Copy link
Member

Choose a reason for hiding this comment

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

All of them are mixins. Ruby modules have some lack of encapsulation features; you can do encapsulation but the code looks weird and in MiQ most of the time it's not done.
This method is not in a module, and I see it as internal implementation and not meant to be used outside the class. So, I would make it private.

Copy link
Member Author

Choose a reason for hiding this comment

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

@israel-hdez thanks very much for explanation! I didn't realized they are mixins. Cool, I'm making them private now! ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

@israel-hdez, ok, I've found out, the "old way" without queued validation is not working when this method is private (because it's called from instance via verify_credentials). The other method can be made private for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Private class methods are an anti-pattern. If we want to have this not be part of the interface then we should make it an instance method, or extract connection validation to another class (which I would prefer tbh).

# As the connect will only give a handle
# we verify the credentials via an actual operation
connection_rescue_block(hostname, port) do
connection.inventory.list_feeds
Copy link
Contributor

Choose a reason for hiding this comment

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

@josejulio I'm not sure, but with Inventory V4 is list_feeds available? This is not a problem for this PR but it may be a problem for applying it to the hawkular-1275 branch.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but i don't think there is other way to test it at this point, "status" is unprotected, so it won't be good way to test the connection.

end
end

Copy link
Member

Choose a reason for hiding this comment

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

This throw an error in the status of a provider
error

Copy link
Member Author

Choose a reason for hiding this comment

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

@aljesusg you sure you have the latest revision of this PR?

def self.connection_rescue_block(hostname = nil, port = nil)
Copy link
Member

Choose a reason for hiding this comment

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

Mark this as private

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

This name is not descriptive, as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think it would be a great idea to overwrite with_provider_connection with this, so that this error handling is used everywhere.

yield
rescue URI::InvalidComponentError
raise MiqException::MiqHostError, "Host '#{hostname}' is invalid"
rescue ::Hawkular::ConnectionException
raise MiqException::MiqUnreachableError, "Unable to connect to #{hostname}:#{port}"
rescue ::Hawkular::Exception => he
raise MiqException::MiqInvalidCredentialsError, 'Invalid credentials' if he.status_code == 401
raise MiqException::MiqHostError, 'Hawkular not found on host' if he.status_code == 404
raise MiqException::MiqCommunicationsError, he.message
rescue => err
$log.error(err)
raise MiqException::Error, 'Unable to verify credentials'
end
private_class_method :connection_rescue_block

def validate_authentication_status
{:available => true, :message => nil}
end
Expand All @@ -94,7 +103,7 @@ def self.entrypoint(host, port, security_protocol)
end

# Hawkular Client
def self.raw_connect(host, port, username, password, security_protocol, cert_store)
def self.raw_connect(host, port, username, password, security_protocol, cert_store, validate = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

@israel-hdez do you know if MiQ uses raw_connect? If not I think this should be on a different class, probably a delegator. What do you think, @tumido?

If so, it could be changed on a different PR, because it seems to me that adding optional params is increasing cyclomatic complexity on the class that is already our biggest offender on this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they are using it :(

Copy link
Member Author

Choose a reason for hiding this comment

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

@cfcosta, basically, the whole new workflow is about shifting the load from UI worker to spawning a queued job (created here) Also it tries to smaller the load by not creating a temporary EMS object for the credentials validation and instead validates them as is. So it requires a class method. And they decided, that using the already available raw_connect is the right way. For some providers it was already doing the job without any modifications, for others, like Hawkular, it means these adjustments...

credentials = {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if validate argument should be true

Copy link
Member Author

Choose a reason for hiding this comment

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

This is switched to true in the new dialog in your PR, for other calls it should be false. So old and new dialogs can live together and it also allows any call to connect to handle the connection right.

:username => username,
:password => password
Expand All @@ -104,8 +113,12 @@ def self.raw_connect(host, port, username, password, security_protocol, cert_sto
:verify_ssl => verify_ssl_mode(security_protocol),
:ssl_cert_store => cert_store
Copy link
Member

Choose a reason for hiding this comment

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

To fix the SSL error, please change this to:

:ssl_cert_store => if cert_store.kind_of?(String)
                     Endpoint.new(:certificate_authority => cert_store).ssl_cert_store
                   else
                     cert_store
                   end

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should come as a separate PR, since it's fixing different issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@israel-hdez fixed here #101

}
::Hawkular::Client.new(:entrypoint => entrypoint(host, port, security_protocol),
:credentials => credentials, :options => options)
connection = ::Hawkular::Client.new(:entrypoint => entrypoint(host, port, security_protocol),
:credentials => credentials, :options => options)

validate_connection(connection, host, port) if validate

connection
end

def connect(_options = {})
Expand Down