Hawkular/add ssl support #460

josejulio · 2017-02-23T18:35:21Z

Adds support for tls endpoints on Hawkular MW provider

ToDo:

Rename container_security_options to endpoint_security_options

This PR ~~depends on PR #450 and~~ supports ManageIQ/manageiq#14054

@miq-bot add-label providers/hawkular

miq-bot · 2017-02-23T18:36:21Z

@josejulio Cannot apply the following label because they are not recognized: providers/hawkular

josejulio · 2017-02-23T18:36:27Z

@cben I modified some of your work, could you please take a look and let me know if I should duplicate the code or is OK what i did?

josejulio · 2017-02-23T18:37:47Z

app/controllers/mixins/ems_common_angular.rb

+    def container_security_options(security_protocol, certificate_authority)
+      {
+        :security_protocol     => security_protocol,
+        :verify_ssl            => %w(ssl-without-validation non-ssl).exclude?(security_protocol),


@cben
Added non-ssl to the options that shouldn't be verified by ssl.

Nice. Not all providers will use same logic, but good to share.
Let's rename the function to something more generic, say endpoint_security_options?

Sure. Will do once your PR is merged. (adding it as a todo).

josejulio · 2017-02-23T18:39:45Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

+%div{"ng-if" => defined?(tls_ca_certs_hide) ? false : true}
+  .form-group{"ng-if"=> "emsCommonModel.emstype == 'rhevm' || " +                                          |
+                        "((emsCommonModel.ems_controller == 'ems_container' || " +                         |
+                        "emsCommonModel.emstype == 'hawkular') && " +                               |


Also using this Trusted CA Certificates textarea.

josejulio · 2017-02-23T18:40:23Z

@miq-bot add-label WIP

cben · 2017-02-26T14:11:28Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

@@ -143,7 +153,8 @@

 %div{"ng-if" => defined?(tls_ca_certs_hide) ? false : true}
  .form-group{"ng-if"=> "emsCommonModel.emstype == 'rhevm' || " +                                          |
-                        "(emsCommonModel.ems_controller == 'ems_container' && " +                          |
+                        "((emsCommonModel.ems_controller == 'ems_container' || " +                         |
+                        "emsCommonModel.emstype == 'hawkular') && " +                               |


This is perhaps too nested, hard to read.
One option that might be more readable if you put the custom condition first (untested):

"(emsCommonModel.#{prefix}_security_protocol == 'ssl-with-validation-custom-ca' &&" | " (emsCommonModel.ems_controller == 'ems_container' || " + | " emsCommonModel.emstype == 'hawkular'))"} |

However the meaning of security_protocol is provider-dependent and this file already does a lot of copy-pasting to keep different providers independent.
What do you think of doing separate (container && custom) || (hawkular && custom)?

Grouping them different seems a good option that would give more flexibility to modify them as needed for each provider without having to copy/paste the whole block.

josejulio · 2017-03-02T18:46:05Z

@miq-bot add_label middleware, wip, pending core

miq-bot · 2017-03-03T17:28:20Z

This pull request is not mergeable. Please rebase and repush.

josejulio · 2017-03-03T19:46:30Z

@miq-bot rm_label wip

josejulio · 2017-03-03T19:48:15Z

#450 is merged already.
@cben I made the changes you suggested. As i modified some of your code, could you please review again? Thanks!

abonas · 2017-03-07T14:10:36Z

Wouldn't it be better to first put the protocol field, and then after the user selects it, to fill automatically the default port? (which user can of course change)

josejulio · 2017-03-07T15:43:51Z

@abonas I'm reusing UI from #450

What you propose is possible but would end being inconsistent with other providers as they define the port before the security protocol.

https://github.com/josejulio/manageiq-ui-classic/blob/b8d8736225787281808e52b8c7a9688d8f07e1d8/app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml#L27-L59

I could move our security_protocol field and fill the port field if that's not a concern..

cben · 2017-03-07T16:17:29Z

I think some providers do reset port based on security protocol:

manageiq-ui-classic/app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

Lines 360 to 368 in d304589

    
           $scope.openstackSecurityProtocolChanged = function() { 
        
             if ($scope.emsCommonModel.emstype === 'openstack' || $scope.emsCommonModel.emstype === 'openstack_infra') { 
        
               if ($scope.emsCommonModel.default_security_protocol === 'non-ssl') { 
        
                 $scope.emsCommonModel.default_api_port = $scope.getDefaultApiPort($scope.emsCommonModel.emstype); 
        
               } else { 
        
                 $scope.emsCommonModel.default_api_port = "13000"; 
        
               } 
        
             } 
        
           };

in which case yes, having protocol above port makes sense.

Don't assume #450 is perfect. I missed several spots there, and still chasing a bug there. Will cc you as soon I have a fix.

josejulio · 2017-03-07T16:30:48Z

If that makes sense for both of you, I'll move the port below the security protocol and fill default values.

abonas · 2017-03-14T12:05:01Z

@serenamarie125 and @Loicavenel , please have a look at the UX issue in regards of fields order for protocol, port, etc. Current way of things don't make a lot of sense (and causes a bit more work for the user). Please decide how it should look in all the providers.

Loicavenel · 2017-03-14T13:20:44Z

We should at minimum reverse Port and Protocol where it does exist. Some providers does not have Port but use URL as input. If protocols has different port, then default port should be presented and updated baed on the protocol selection

josejulio · 2017-03-14T16:17:49Z

@abonas @Loicavenel
Should i do that on this PR?
Please see #460 (comment)
There might be other fields between Port and Security Protocol.

If protocols has different port, then default port should be presented and updated baed on the protocol selection

Currently (on this provider) the Port is only updated (when selecting a protocol) IF port is empty OR port is any of the default ports OR port is invalid (e.g. NaN).

cben · 2017-03-14T16:32:50Z

Since order is shared to all providers, I think separate PR is natural. P.S. another order you haven't listed is put protocol even before hostname. (Kinda like proto://host:port URL structure.)

josejulio · 2017-03-14T16:33:58Z

That would be the third option! I'll edit #460 (comment)

josejulio · 2017-03-14T17:07:50Z

@AparnaKarve Updated code so Trusted CA Certificates now affects required validation.

Next steps would be:
Pass down a default value of default_security_protocol to the angular controller using this after is merged and preferably on a separate PR.
Change port / security protocol order (IMO (and @cben opinion) belongs in a separate PR)

Is there anything else you want me to address on this PR?

AparnaKarve · 2017-03-14T17:17:12Z

Updated code so Trusted CA Certificates now affects required validation.

@josejulio Good call on that.

Agree about addressing the other issues you mentioned in separate PRs.
And with that, this is GTG.

josejulio · 2017-03-14T17:20:07Z

@AparnaKarve wait a bit. I think i missed something.

josejulio · 2017-03-14T17:23:00Z

@AparnaKarve Ok, done, I had an extra ´}´ on the last commit. Just removed it.

miq-bot · 2017-03-14T17:23:04Z

Checked commits josejulio/manageiq-ui-classic@03a478a~...1a2d639 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

josejulio · 2017-03-14T17:48:23Z

Ummh, @AparnaKarve, #670 also address the Trusted CA certificates, so I'm removing it from this PR because is the same code.

abonas · 2017-03-15T13:30:28Z

@josejulio @cben please see @Loicavenel guideline regarding the protocol/port order, it could be applied in this PR or in a separate one, although I don't see much point doing it here in the incorrect way and then redoing it again in another PR.

AparnaKarve · 2017-03-15T14:24:35Z

@josejulio #670 has some code that you would need to fix this completely, so how about we wait for #670 to get merged first? (which I'm currently reviewing)

Once it's merged, it will be easy for you to identify what else needs to be fixed from your end.

josejulio · 2017-03-15T16:38:19Z

@AparnaKarve Sure: I can wait for it.

@abonas We are sharing the security_protocol with others providers. I just want to make sure I don't break anything from their end. By doing a separate PR i could at least try to have them look at only that fix.
It shouldn't break anything though.

josejulio · 2017-03-17T00:53:03Z

@AparnaKarve

Rebased and added the security_protocol_default for ems_middleware
Now when editing a previously added provider it shows Non SSL selected

AparnaKarve · 2017-03-17T20:48:30Z

@josejulio The screenshot looks good. So looks like you're all set now.

cben · 2017-03-19T13:23:01Z

LGTM 👍

abonas · 2017-03-20T16:53:45Z

@miq-bot assign @martinpovolny please review & merge

miq-bot · 2017-03-20T16:54:06Z

@abonas 'martinpovolny please review & merge' is an invalid assignee, ignoring...

abonas · 2017-03-20T16:54:46Z

@miq-bot assign @martinpovolny
@martinpovolny please review and merge

josejulio mentioned this pull request Feb 23, 2017

Be able to use tls when connecting to Hawkular ManageIQ/manageiq#14054

Merged

josejulio commented Feb 23, 2017

View reviewed changes

josejulio changed the title ~~Hawkular/add ssl support~~ [WIP] Hawkular/add ssl support Feb 23, 2017

miq-bot added the wip label Feb 23, 2017

cben reviewed Feb 26, 2017

View reviewed changes

cben mentioned this pull request Feb 27, 2017

Verify container TLS, support custom CA ManageIQ/manageiq#14019

Merged

7 tasks

josejulio force-pushed the hawkular/add_ssl_support branch 2 times, most recently from 25af1c1 to eed3141 Compare March 2, 2017 18:37

miq-bot added middleware pending core labels Mar 2, 2017

josejulio force-pushed the hawkular/add_ssl_support branch from eed3141 to 4d6ad1f Compare March 2, 2017 19:18

miq-bot added the unmergeable label Mar 3, 2017

josejulio force-pushed the hawkular/add_ssl_support branch from 4d6ad1f to b6d0c9d Compare March 3, 2017 19:33

miq-bot removed the unmergeable label Mar 3, 2017

josejulio force-pushed the hawkular/add_ssl_support branch from b6d0c9d to b8d8736 Compare March 3, 2017 19:44

josejulio changed the title ~~[WIP] Hawkular/add ssl support~~ Hawkular/add ssl support Mar 3, 2017

miq-bot removed the wip label Mar 3, 2017

chessbyte assigned abonas Mar 7, 2017

josejulio force-pushed the hawkular/add_ssl_support branch from 8efed47 to 1a2d639 Compare March 14, 2017 17:21

josejulio force-pushed the hawkular/add_ssl_support branch from 1a2d639 to 7413410 Compare March 14, 2017 17:47

josejulio mentioned this pull request Mar 16, 2017

Container TLS fixes #670

Merged

miq-bot assigned martinpovolny and unassigned abonas Mar 20, 2017

mzazrivec added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 20, 2017

mzazrivec merged commit fe72635 into ManageIQ:master Mar 20, 2017

josejulio deleted the hawkular/add_ssl_support branch March 21, 2017 17:27

This was referenced Mar 21, 2017

Sets default security_protocol for middleware #758

Merged

Move security protocol #759

Merged

Hawkular/add ssl support #460

Hawkular/add ssl support #460

Conversation

josejulio commented Feb 23, 2017 • edited

miq-bot commented Feb 23, 2017

josejulio commented Feb 23, 2017

josejulio Feb 23, 2017

Choose a reason for hiding this comment

cben Feb 26, 2017

Choose a reason for hiding this comment

josejulio Feb 27, 2017

Choose a reason for hiding this comment

josejulio Feb 23, 2017

Choose a reason for hiding this comment

josejulio commented Feb 23, 2017

cben Feb 26, 2017 • edited

Choose a reason for hiding this comment

josejulio Feb 27, 2017

Choose a reason for hiding this comment

josejulio commented Mar 2, 2017

miq-bot commented Mar 3, 2017

josejulio commented Mar 3, 2017

josejulio commented Mar 3, 2017

abonas commented Mar 7, 2017

josejulio commented Mar 7, 2017

cben commented Mar 7, 2017

josejulio commented Mar 7, 2017

abonas commented Mar 14, 2017

Loicavenel commented Mar 14, 2017

josejulio commented Mar 14, 2017

cben commented Mar 14, 2017 via email

josejulio commented Mar 14, 2017

josejulio commented Mar 14, 2017

AparnaKarve commented Mar 14, 2017

josejulio commented Mar 14, 2017

josejulio commented Mar 14, 2017

miq-bot commented Mar 14, 2017

josejulio commented Mar 14, 2017 • edited

abonas commented Mar 15, 2017

AparnaKarve commented Mar 15, 2017

josejulio commented Mar 15, 2017

josejulio commented Mar 17, 2017

AparnaKarve commented Mar 17, 2017

cben commented Mar 19, 2017

abonas commented Mar 20, 2017

miq-bot commented Mar 20, 2017

abonas commented Mar 20, 2017

josejulio commented Feb 23, 2017 •

edited

cben Feb 26, 2017 •

edited

josejulio commented Mar 14, 2017 •

edited