Add optional validate param to raw_connect #100

tumido · 2017-11-06T14:31:10Z

When provider is added the credentials validation is newly done via raw_connect (class method) instead of verify_credentials (needed EMS object). Further reading on this change can be found here: ManageIQ/manageiq-ui-classic#1580

Related to: ManageIQ/manageiq-ui-classic#2577

Similar PRs for other providers:

Fix exception handing for credential validation on raw_connect manageiq-providers-azure#161
Fix exception handing for credential validation on raw_connect manageiq-providers-google#28
Fix exception handing for credential validation on raw_connect manageiq-providers-scvmm#41
Fix exception handing for credential validation on raw_connect manageiq-providers-lenovo#108
Fix exception handing for credential validation on raw_connect manageiq-providers-nuage#40

This PR enables the new workflow on provider. With this PR both workflows are available.

tumido · 2017-11-06T14:35:06Z

cc @aljesusg, @abonas, @jntullo, please review

tumido · 2017-11-06T14:47:34Z

@miq-bot add_label gaprindashvili/yes bug

miq-bot · 2017-11-06T14:48:41Z

@tumido Cannot apply the following label because they are not recognized: gaprindashvili/yes bug

abonas · 2017-11-06T14:59:05Z

@agrare @chessbyte I can't add for some reason @jntullo as a reviewer. any idea why?

aljesusg · 2017-11-06T15:41:05Z

With Correct credentials should be correct I am getting Invalid Credentials

With incorrect credentials go OK

With a Incorrect Host go OK

tumido · 2017-11-06T16:10:24Z

@aljesusg it's not working because the mapping in your ManageIQ/manageiq-ui-classic#2577 is sending password instead of params[:default_password], when you fix it, it's gonna work.

aljesusg · 2017-11-06T16:16:58Z

@tumido in get_task_args the first line is
user, password = params[:default_userid], MiqPassword.encrypt(params[:default_password])
So are we working without encryption password?

tumido · 2017-11-06T16:19:46Z

@aljesusg I know.. It seems so. Without encryption it works for me. But I'm not sure how it behaves when the ssl is enabled, I haven't checked that.

aljesusg · 2017-11-06T16:20:56Z

Ok I tested and work without encryption I am gonna test this with SSL and CA @tumido
Maybe we should desyncript in our validate if no ssl because the actual solution maybe don't work with SSL let me a second to test this. Thanks

aljesusg · 2017-11-06T16:33:42Z

This fail with SSL
Credential validation was not successful: wrong argument (String)! (Expected kind of OpenSSL::X509::Store) thisis running in local with curl maybe we are missing something in the step of create ssl_cert_store, I tested with encrypted password and is failing too, after the call I'll debug this.

credentials object

{:username=>"jdoe", :password=>"password"}

options object

{:tenant=>"hawkular", :verify_ssl=>1, :ssl_cert_store=>"-----BEGIN CERTIFICATE-----\r\nMIIC/TCCAeWgAwIBAgIEZi3DSDANBgkqhkiG9w0BAQsFADAcMRowGAYDVQQDExFo\r\nYXdrdWxhci1zZXJ2aWNlczAgFw0xNzExMDIwOTQxMzVaGA8yMTE3MTAwOTA5NDEz\r\nNVowHDEaMBgGA1UEAxMRaGF3a3VsYXItc2VydmljZXMwggEiMA0GCSqGSIb3DQEB\r\nAQUAA4IBDwAwggEKAoIBAQCYWbkxaDx6e9mMK/T87QSJ6NS5KtlPzx3RItmegeOs\r\n8Z9vxHMC6rDKr+qlX3Ed/OhIk36az8rSrOYHT3ONdVVOK/em0JnOq8+lFyL1sfDT\r\nnN63s3ON4p8JS1jUxVcIafTJu12h//xf/nVbW5q3pcE242AXL4q0LIDIGczm2cV5\r\nYjeofTLJl/ra+8+yAygUm6+kT1MTkzb8TtNiJJDxAUTDDBF+iuISV4R5adLGmCmz\r\njiDZPxiwHI4TLO0I02jIzpQ9Rql+7Sd7jnEzea9oFJT7dr3E+CbknvUO1bso+Jwt\r\ns7T9CeYzKxBswW83pPTI44iYmydtWwenzPqUivMIwio3AgMBAAGjRTBDMCIGA1Ud\r\nEQQbMBmHBH8AAAGCEWhhd2t1bGFyLXNlcnZpY2VzMB0GA1UdDgQWBBTfPluQkZHh\r\nCsKExlfsrrzpNhJpNzANBgkqhkiG9w0BAQsFAAOCAQEAM5MDnDcgz5XKc6FL/fBr\r\nQ4TIgNG9T7qFn2lLEkOkqh4kDAKfyFa5bBRTrDRglQoGQBfdAJD4nnS/mxBsLrBD\r\neW8bS9+1aGIyUnZK8fYIVR7wXSvb3U/+D0foqJJDEtyBftSsscrL1dYNEaOMrhAO\r\nECj7xl4638zdjp3klxlcIvJ9NINdPON4ltr796nhZIoHs8OTDW04M3sztzdpMkiH\r\ncBz1seRQlPAmWSK29y1fpXNrr0PmlYijgMYVv59o7zm4/Gvv2PcTzWh9ykFK/WnZ\r\nY1MooB0peBI4X3vWx/VAuX1ybUyPthx/gzbtZks6ztVjJOgs50eawUz3Q7S3BtWv\r\n5g==\r\n-----END CERTIFICATE-----"}

With SSL withpout validation is running ok so maybe is a problem working with the CA

tumido · 2017-11-06T18:25:37Z

@aljesusg ok, do you think this should be covered in this PR? Is it caused by me or just exposed?

josejulio · 2017-11-06T18:32:14Z

I think you should take a look at the PR[1] that did that work.
The certificate is parsed to a OpenSSL::X509::Store by the method: ssl_cert_store

Edit, this [2] is could also be helpful.

I'm uncertain on how currently is handled, but these calls are needed to transform the text into an actual X509::Store.

[1] https://github.com/ManageIQ/manageiq/pull/13721/files#diff-5d3f1a5ab701ca4a756d3c921e36e9f6R22
[2] https://github.com/ManageIQ/manageiq-ui-classic/pull/450/files#diff-f402e411cd6dc158c724b14d72ead612R463

aljesusg · 2017-11-06T18:34:06Z

So why is getting this error @josejulio? I need to debug this.

josejulio · 2017-11-06T18:35:21Z

I suppose that call needs to be done when passing the arguments on manageiq-ui-classic.

aljesusg · 2017-11-06T18:39:22Z

But in MiddlewareManager the connect call to default_endpoint.ssl_cert_store that do this transform

josejulio · 2017-11-06T19:29:14Z

From the error mentioned before [1], that function isn't being called. Certainly debug is needed there, sorry for not having more information.

[1] #100 (comment)

israel-hdez

There are no tests for verify_credentials nor raw_connect.
Are there any chances that you can add some tests?

israel-hdez · 2017-11-06T22:50:01Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

@@ -104,8 +112,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


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

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

@israel-hdez fixed here #101

israel-hdez · 2017-11-06T22:53:14Z

app/models/manageiq/providers/hawkular/middleware_manager.rb


      true
    end

+    def self.validate_connection(connection, hostname = nil, port = nil)


Mark this as private

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

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.

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

@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.

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).

israel-hdez · 2017-11-06T22:53:22Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

+      end
+    end
+
+    def self.connection_rescue_block(hostname = nil, port = nil)


Mark this as private

This name is not descriptive, as well.

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

tumido · 2017-11-07T07:19:13Z

@israel-hdez I'll work on the tests, I'm not sure if it should block this to be merged though.

When provider is added the credentials validation is newly done via `raw_connect` (class method) instead of `verify_credentials` (needed EMS object)

jshaughn · 2017-11-07T13:54:11Z

By applying locally this PR, and also ManageIQ/manageiq-ui-classic#2577, I was finally able to create a non-ssl provider and so hopefully can now make some progress. It seems it's been a week being unable to create a provider, that is a long time. If a merge is not imminent I'd suggest maybe posting in dev-list how to work around this problem.

jshaughn · 2017-11-07T13:55:45Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

+      # 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


@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.

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.

aljesusg · 2017-11-07T14:06:59Z

Now It's working again after ManageIQ/manageiq-ui-classic#2643 with normal servers and SSL with CA. Tested !!! Do we need this PR?

tumido · 2017-11-07T14:14:37Z

@aljesusg if we want to be prepared for the new dialog, we need this PR. It's not breaking the "old way" it's just enabling the "new way". I vote for being prepared and have your and mine PRs merged.

aljesusg · 2017-11-07T14:36:52Z

Now its working with SSL and CA too @tumido

aljesusg · 2017-11-06T15:22:51Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

@@ -94,7 +102,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)
      credentials = {


I don't know if validate argument should be true

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.

aljesusg · 2017-11-06T15:23:30Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

+        connection.inventory.list_feeds
+      end
+    end
+


This throw an error in the status of a provider

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

aljesusg · 2017-11-07T14:54:48Z

Tested with:

ManageIQ/manageiq-ui-classic#2577
#100
#101

and revert

ManageIQ/manageiq-ui-classic#2643

and it's working with hawkular server, ssl and CA

abonas · 2017-11-07T18:40:04Z

By applying locally this PR, and also ManageIQ/manageiq-ui-classic#2577, I was finally able to create a non-ssl provider and so hopefully can now make some progress. It seems it's been a week being unable to create a provider, that is a long time. If a merge is not imminent I'd suggest maybe posting in dev-list how to work around this problem.

@jshaughn there was a regression due to a broader change in core. core team fixed it yesterday and merged. so now it should be ok on master.
in order to fix it to better fix a new dialog behavior for the future, there are several PRs in the works (incl this one).

miq-bot · 2017-11-07T20:44:45Z

Checked commits tumido/manageiq-providers-hawkular@b2d24b8~...d1efd7b with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 1 offense detected

app/models/manageiq/providers/hawkular/middleware_manager.rb

❗ - Line 78, Col 7 - Style/GlobalVars - Do not introduce global variables.

tumido · 2017-11-09T11:33:08Z

@israel-hdez test coverage is available here #102

cfcosta · 2017-11-09T13:37:59Z

app/models/manageiq/providers/hawkular/middleware_manager.rb

@@ -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)


@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.

Yes, they are using it :(

@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...

tumido · 2018-02-19T13:24:01Z

Closing, won't merge. Repo abandoned. ⛔

miq-bot · 2018-02-19T13:24:26Z

@tumido Cannot apply the following label because they are not recognized: gaprindashvili/yes bug

tumido mentioned this pull request Nov 6, 2017

Fix credential validation in Hawkular provider about number of arguments ManageIQ/manageiq-ui-classic#2577

Merged

abonas added the gaprindashvili/yes label Nov 6, 2017

abonas requested review from israel-hdez, agrare and aljesusg November 6, 2017 14:51

abonas self-assigned this Nov 6, 2017

tumido force-pushed the fix_credentials_validate branch from 504e4ab to bd7488f Compare November 6, 2017 16:24

tumido mentioned this pull request Nov 6, 2017

Fix exception handing for credential validation on raw_connect ManageIQ/manageiq-providers-google#28

Merged

israel-hdez reviewed Nov 6, 2017

View reviewed changes

This was referenced Nov 7, 2017

Fix exception handing for credential validation on raw_connect ManageIQ/manageiq-providers-scvmm#41

Merged

Fix exception handing for credential validation on raw_connect ManageIQ/manageiq-providers-lenovo#108

Merged

Add optional validate param to raw_connect

b2d24b8

When provider is added the credentials validation is newly done via `raw_connect` (class method) instead of `verify_credentials` (needed EMS object)

tumido force-pushed the fix_credentials_validate branch from bd7488f to b2d24b8 Compare November 7, 2017 12:20

This was referenced Nov 7, 2017

Fix exception handing for credential validation on raw_connect ManageIQ/manageiq-providers-nuage#40

Merged

Queue authentication check only for ems_cloud and ems_infra providers. ManageIQ/manageiq-ui-classic#2643

Merged

jshaughn reviewed Nov 7, 2017

View reviewed changes

aljesusg approved these changes Nov 7, 2017

View reviewed changes

aljesusg mentioned this pull request Nov 7, 2017

Create cert_store if it's not created before validation #101

Merged

tumido mentioned this pull request Nov 7, 2017

Enable queued credentials validation for middleware ManageIQ/manageiq-ui-classic#2660

Closed

Set connection_rescue_block class methods private

d1efd7b

tumido mentioned this pull request Nov 9, 2017

Add test coverage for connect and verify_credentials #102

Closed

cfcosta reviewed Nov 9, 2017

View reviewed changes

abonas added gaprindashvili/no and removed gaprindashvili/yes labels Nov 9, 2017

tumido closed this Feb 19, 2018

		@@ -104,8 +112,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

Add optional validate param to raw_connect #100

Add optional validate param to raw_connect #100

Conversation

tumido commented Nov 6, 2017 • edited Loading

tumido commented Nov 6, 2017

tumido commented Nov 6, 2017

miq-bot commented Nov 6, 2017

abonas commented Nov 6, 2017 • edited Loading

aljesusg commented Nov 6, 2017

tumido commented Nov 6, 2017

aljesusg commented Nov 6, 2017

tumido commented Nov 6, 2017

aljesusg commented Nov 6, 2017 • edited Loading

aljesusg commented Nov 6, 2017 • edited Loading

tumido commented Nov 6, 2017

josejulio commented Nov 6, 2017 • edited Loading

aljesusg commented Nov 6, 2017

josejulio commented Nov 6, 2017

aljesusg commented Nov 6, 2017

josejulio commented Nov 6, 2017

israel-hdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tumido commented Nov 7, 2017

jshaughn commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljesusg commented Nov 7, 2017

tumido commented Nov 7, 2017

aljesusg commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljesusg commented Nov 7, 2017

abonas commented Nov 7, 2017

miq-bot commented Nov 7, 2017

tumido commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tumido commented Feb 19, 2018

miq-bot commented Feb 19, 2018

tumido commented Nov 6, 2017 •

edited

Loading

abonas commented Nov 6, 2017 •

edited

Loading

aljesusg commented Nov 6, 2017 •

edited

Loading

aljesusg commented Nov 6, 2017 •

edited

Loading

josejulio commented Nov 6, 2017 •

edited

Loading

aljesusg commented Nov 7, 2017 •

edited

Loading