Add optional variables for SSL management-console #648

slm0n87 · 2017-10-12T07:43:04Z

Fixes #564

wyardley · 2017-10-12T16:35:05Z

👍 from me, can you squash that second commit?

wyardley

This looks good to me, can you squash that last commit?

wyardley · 2017-10-12T16:37:20Z

manifests/init.pp

@@ -243,7 +246,10 @@
  $ssl_port                                      = $rabbitmq::params::ssl_port,
  Optional[String] $ssl_interface                = undef,
  Integer $ssl_management_port                   = $rabbitmq::params::ssl_management_port,
-  Integer $ssl_stomp_port                                = $rabbitmq::params::ssl_stomp_port,
+  Optional[String]  $ssl_management_cacert       = $ssl_cacert,


Not sure if this could introduce any weird behavior. This is undef by default, since the params you're referencing are undef, but if someone set ssl_cacert and tried to explicitly set ssl_management_cacert to undef, it wouldn't work. I think this is still probably the best and most logical behavior (to take those as defaults).

slm0n87 · 2017-10-13T12:23:35Z

@wyardley thx for reviewing. You are right, its was not possible to not set an CA Cert for the mgmnt.
I solved this now by changing the Type to "String or Boolean".
Now the mgmnt CA Cert can be disabled by just setting it to "false".

wyardley · 2017-10-13T18:16:17Z

@slm0n87 maybe you could make it undef, and then, in the manifest, set it to the value of the other one?

I think setting it to false vs. undef is kind of an anti-pattern, @bastelfreak @alexjfisher - any thoughts on the best way to accomplish this?

bastelfreak · 2017-10-13T18:18:15Z

manifests/init.pp

@@ -243,7 +247,10 @@
  $ssl_port                                      = $rabbitmq::params::ssl_port,
  Optional[String] $ssl_interface                = undef,
  Integer $ssl_management_port                   = $rabbitmq::params::ssl_management_port,
-  Integer $ssl_stomp_port                                = $rabbitmq::params::ssl_stomp_port,
+  Variant[Undef, Boolean, String] $ssl_management_cacert = $ssl_cacert,
+  Optional[String] $ssl_management_cert          = $ssl_cert,


all params should be alligned at their =

bastelfreak · 2017-10-13T18:18:54Z

@wyardley I agree, this is an antipattern and should be avoided.

wyardley · 2017-10-14T17:03:29Z

I think you could do it this way:

default the params to undef
if management_ssl is true, and the params are undef, set $_ssl_management_cacert etc. to $rabbitmq::ssl_cacert (in the manifest, not in the class params)
if management_ssl is true and param is not undef, use that value instead.

That make sense?

wyardley · 2017-10-14T17:22:49Z

See also some changes in #649 that may affect this or require a rebase. Basically, I think the cert params should be Optional[Stdlib::Absolutepath] vs just Optional[String]

we should revisit based on discussion

alexjfisher · 2017-10-15T18:25:23Z

manifests/init.pp

@@ -243,7 +247,10 @@
  $ssl_port                                      = $rabbitmq::params::ssl_port,
  Optional[String] $ssl_interface                = undef,
  Integer $ssl_management_port                   = $rabbitmq::params::ssl_management_port,
-  Integer $ssl_stomp_port                                = $rabbitmq::params::ssl_stomp_port,
+  Variant[Undef, Boolean, String] $ssl_management_cacert = $ssl_cacert,


I think there's some issue trying to default one parameter based on another like this. @bastelfreak this came up on IRC IIRC??

Actually, since puppet 4.4.0 (I've been go back through the IRC logs), this should work. https://github.com/puppetlabs/puppet-specifications/blob/master/language/parameter_scope.md#the-default-expression-evaluation.

alexjfisher · 2017-10-15T18:43:41Z

It took me a little while to figure out why the false option for ssl_management_cacert was needed, but now I have, I actually think the code is OK.

If I understand correctly, you need to default all the parameters to the existing ones for backwards compatibility and because that's what most people will want to do anyway.
There are 2 ways of doing this. You either default them to undef and do it in the manifest, or since puppet 4.4, you can just default the parameters to the others directly in the declaration.

But you also want to account for users who want no ssl_management_cacert file. If you explicitly try to set the parameter to undef, nothing happens, you just get the parameter default. So you've got to allow false instead.

wyardley · 2017-10-15T19:25:31Z

It took me a little while to figure out why the false option for ssl_management_cacert was needed, but now I have, I actually think the code is OK.

I think if the boolean management_ssl is true, then the various cert files are probably all required, no (at least, a cert and key will be required, and from what I remember from testing, the ca cert may also be required)? So I'm not sure if there's a valid reason to have the ability to suppress these - probably should just default to the non-management ones, and allow overriding.

It doesn't look like the templates support the case where these don't exist, e.g.:

<% if @ssl && @management_ssl -%>
ssl = True
ssl_ca_cert_file = <%= @ssl_cacert %>
ssl_cert_file = <%= @ssl_cert %>
ssl_key_file = <%= @ssl_key %>
port = <%= @ssl_management_port %>

- move logic for backwards compatibility into the manifest

slm0n87 · 2017-10-16T09:16:26Z

reopen Issie (accidentally closed by forced rebasing my forked master)...

slm0n87 · 2017-10-16T09:37:28Z

I updated my patch once more by moving the overwrites for backwards compatibility into the manifest and set them to undef by default.

wyardley · 2017-10-16T16:47:59Z

templates/rabbitmq.config.erb

@@ -109,11 +109,11 @@
  <%- end -%>
      {port, <%= @ssl_management_port %>},
      {ssl, true},
-      {ssl_opts, [<%- if @ssl_cacert %>
-                  {cacertfile, "<%= @ssl_cacert %>"},
+      {ssl_opts, [<%- if @_ssl_management_cacert %>


I think regardless of how it's coded, ssl_management_cacert can't actually be undef, right? since it still defaults to ssl_cacert if it's not defined?

wyardley · 2017-10-16T16:49:43Z

@slm0n87 I think this will work, but I think @alexjfisher is right that this still won't allow actually suppressing it, because of the way the defaults work. Given that, I think the bit I commented on in the template is maybe extraneous? Also, the module supports >= 4.7.1 only, so maybe using the default in the params is cleaner after all, though I'm Ok with it as it is here.

Sorry for so much back and forth on this PR, and thanks for all the revisions.

slm0n87 · 2017-10-16T17:38:29Z

@wyardley thx for the feedback - you are welcome. regarding the forth and back, I am aware of the conflict keeping a module most configurablae but backwards compatible :)
So what do you think how we should finally resolve the ssl_management_cacerts? To make your condition in the template back working?
Creating a new boolean variable $_ssl_management_cacert_enable which defaults to true?
I can put the defaults back from the manifest to the declaration for cleaner code.

- based on discussions in voxpupuli#648

slm0n87 · 2017-10-16T18:11:06Z

like this slm0n87@3d5ba4b
if its OK I would properly test it and add it to this PR tomorrow.

wyardley · 2017-10-18T07:56:43Z

@slm0n87 yeah, I think that makes some sense. Having a param was the way I was thinking of suggesting. The current template doesn't seem to support suppressing the cafile, so I think it could be Ok if that's not supported, but maybe you need that feature?

- based on discussions in voxpupuli#648

wyardley · 2017-10-18T17:13:38Z

@slm0n87 Great, two questions then:

Can you add spec tests?
Do you think we should mirror the same option for the non-management SSL part? Or is the need to suppress the ca file not applicable there?

slm0n87 · 2017-10-19T07:03:02Z

I am not deep enough into spec tests to be able to write tests for my pullrequest. It would be perfect if someone could overtake this which also gives me a good example.
2. We dont have the fallback case for the amqp ssl. So setting variable ssl_cacert = undef (which is the default) results into setting no ssl_cacert in the template.

bastelfreak · 2017-10-19T08:03:15Z

manifests/config.pp

+  $ssl_port                     = $rabbitmq::ssl_port
+  $ssl_interface                = $rabbitmq::ssl_interface
+  $ssl_management_port          = $rabbitmq::ssl_management_port
+  $ssl_management_cacert_enable = $rabbitmq::ssl_management_cacert_enable


Can you please add datatypes for the new params?

I think this is because they're not class params, but variables in config.pp. in init.pp, these all have data types. IIRC, you can't specify data types for variables vs. class params. I think they're not settable since they're in the body of the manifest, though we could also add an explicit assert_private() too maybe?

wyardley · 2017-10-19T17:32:33Z

I am not deep enough into spec tests to be able to write tests for my pullrequest. It would be perfect if someone could overtake this which also gives me a good example.

I can work on this at some point if I've got the time and submit a PR to your PR. FWIW, I think this is the section, so it might not be as bad as you think to make cases for the specific use cases. While I don't mind adding them, I think it can actually be a pretty useful exercise to add tests yourself.

You'd want to create a describe or context block and test that the new behavior has the right config settings, and also make sure that the tests for the default behavior already ensure that the default behavior does what we think.

https://github.com/voxpupuli/puppet-rabbitmq/blob/master/spec/classes/rabbitmq_spec.rb#L721-L1135
https://github.com/voxpupuli/puppet-rabbitmq/blob/master/.github/CONTRIBUTING.md
also has some info on running tests, and we're happy to help on Slack / IRC.

We dont have the fallback case for the amqp ssl. So setting variable ssl_cacert = undef (which is the default) results into setting no ssl_cacert in the template.

Ah, you're right, misread that before. So this solves the case of needing to suppress the CA cert for the management in the case where the main SSL config needs it but management doesn't.

- based on discussions in voxpupuli#648

- fallback to old variable for backwards compatibility

slm0n87 · 2019-06-22T18:03:54Z

rebased & fix conflicts
create spec tests for new options

@wyardley @bastelfreak @alexjfisher can you review again?

wyardley

Looks good overall, can you explain the one comment I have (inline)?

manifests/init.pp

wyardley self-requested a review October 12, 2017 16:35

wyardley previously approved these changes Oct 12, 2017

View reviewed changes

wyardley added the enhancement New feature or request label Oct 12, 2017

slm0n87 force-pushed the master branch 2 times, most recently from 16b720c to 5fbdf22 Compare October 13, 2017 12:18

slm0n87 force-pushed the master branch from 5fbdf22 to 4d62541 Compare October 13, 2017 12:39

bastelfreak reviewed Oct 13, 2017

View reviewed changes

wyardley added the needs-rebase label Oct 14, 2017

alexjfisher reviewed Oct 15, 2017

View reviewed changes

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 16, 2017

Improvements based on the discussion voxpupuli#648

6ce8f74

- move logic for backwards compatibility into the manifest

slm0n87 closed this Oct 16, 2017

slm0n87 force-pushed the master branch from 4d62541 to cf32364 Compare October 16, 2017 09:04

slm0n87 reopened this Oct 16, 2017

wyardley reviewed Oct 16, 2017

View reviewed changes

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 16, 2017

move variable declaration, option for mgnt cacert

3d5ba4b

- based on discussions in voxpupuli#648

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 18, 2017

move variable declaration, option for mgnt cacert

2a082af

- based on discussions in voxpupuli#648

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 18, 2017

move variable declaration, option for mgnt cacert

52d6248

- based on discussions in voxpupuli#648

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 18, 2017

move variable declaration, option for mgnt cacert

888a1e0

- based on discussions in voxpupuli#648

slm0n87 force-pushed the master branch from 52d6248 to e01b70e Compare October 18, 2017 16:18

slm0n87 pushed a commit to slm0n87/puppet-rabbitmq that referenced this pull request Oct 18, 2017

move variable declaration, option for mgnt cacert

e01b70e

- based on discussions in voxpupuli#648

wyardley removed the needs-rebase label Oct 18, 2017

bastelfreak reviewed Oct 19, 2017

View reviewed changes

wyardley added the needs-tests label Oct 19, 2017

wyardley added the needs-rebase label Sep 5, 2018

slm0n87 force-pushed the master branch from e01b70e to b44d9f8 Compare March 7, 2019 08:46

slm0n87 added a commit to slm0n87/puppet-rabbitmq that referenced this pull request Mar 7, 2019

move variable declaration, option for mgnt cacert

b44d9f8

- based on discussions in voxpupuli#648

Add optional variables for SSL management-console

03cf356

- fallback to old variable for backwards compatibility

slm0n87 force-pushed the master branch from b44d9f8 to 03cf356 Compare June 22, 2019 17:41

wyardley reviewed Jun 22, 2019

View reviewed changes

manifests/init.pp Show resolved Hide resolved

wyardley approved these changes Jun 22, 2019

View reviewed changes

manifests/init.pp Show resolved Hide resolved

wyardley merged commit ab81dcc into voxpupuli:master Jun 22, 2019

wyardley removed needs-rebase needs-tests labels Jun 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional variables for SSL management-console #648

Add optional variables for SSL management-console #648

slm0n87 commented Oct 12, 2017 •

edited by wyardley

Loading

wyardley commented Oct 12, 2017

wyardley left a comment

wyardley Oct 12, 2017

slm0n87 commented Oct 13, 2017 •

edited

Loading

wyardley commented Oct 13, 2017

bastelfreak Oct 13, 2017

bastelfreak commented Oct 13, 2017

wyardley commented Oct 14, 2017

wyardley commented Oct 14, 2017

alexjfisher Oct 15, 2017

alexjfisher Oct 15, 2017

alexjfisher commented Oct 15, 2017

wyardley commented Oct 15, 2017 •

edited

Loading

slm0n87 commented Oct 16, 2017 •

edited

Loading

slm0n87 commented Oct 16, 2017

wyardley Oct 16, 2017 •

edited

Loading

wyardley commented Oct 16, 2017

slm0n87 commented Oct 16, 2017

slm0n87 commented Oct 16, 2017

wyardley commented Oct 18, 2017

wyardley commented Oct 18, 2017

slm0n87 commented Oct 19, 2017

bastelfreak Oct 19, 2017

wyardley Oct 19, 2017

wyardley commented Oct 19, 2017 •

edited

Loading

slm0n87 commented Jun 22, 2019

wyardley left a comment

Add optional variables for SSL management-console #648

Add optional variables for SSL management-console #648

Conversation

slm0n87 commented Oct 12, 2017 • edited by wyardley Loading

wyardley commented Oct 12, 2017

wyardley left a comment

Choose a reason for hiding this comment

wyardley Oct 12, 2017

Choose a reason for hiding this comment

slm0n87 commented Oct 13, 2017 • edited Loading

wyardley commented Oct 13, 2017

bastelfreak Oct 13, 2017

Choose a reason for hiding this comment

bastelfreak commented Oct 13, 2017

wyardley commented Oct 14, 2017

wyardley commented Oct 14, 2017

alexjfisher Oct 15, 2017

Choose a reason for hiding this comment

alexjfisher Oct 15, 2017

Choose a reason for hiding this comment

alexjfisher commented Oct 15, 2017

wyardley commented Oct 15, 2017 • edited Loading

slm0n87 commented Oct 16, 2017 • edited Loading

slm0n87 commented Oct 16, 2017

wyardley Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

wyardley commented Oct 16, 2017

slm0n87 commented Oct 16, 2017

slm0n87 commented Oct 16, 2017

wyardley commented Oct 18, 2017

wyardley commented Oct 18, 2017

slm0n87 commented Oct 19, 2017

bastelfreak Oct 19, 2017

Choose a reason for hiding this comment

wyardley Oct 19, 2017

Choose a reason for hiding this comment

wyardley commented Oct 19, 2017 • edited Loading

slm0n87 commented Jun 22, 2019

wyardley left a comment

Choose a reason for hiding this comment

slm0n87 commented Oct 12, 2017 •

edited by wyardley

Loading

slm0n87 commented Oct 13, 2017 •

edited

Loading

wyardley commented Oct 15, 2017 •

edited

Loading

slm0n87 commented Oct 16, 2017 •

edited

Loading

wyardley Oct 16, 2017 •

edited

Loading

wyardley commented Oct 19, 2017 •

edited

Loading