Add loopback_users parameter (adds ability to allow guest user to login remotely) #699

jjuarez · 2018-04-19T21:08:46Z

Pull Request (PR) description

The aim of this pull request is to give the possibility to allow remote connections with the guest user, of course this is not the default configuration, because it implies some security risks

This Pull Request (PR) fixes the following issues

Fixes #698

…fault policy of the loopback access

…t change the default behaviour of the module

wyardley · 2018-04-19T21:20:08Z

Just wondering, should this be allow_remote_guest_connections or similar (even though it's super long)?

jjuarez · 2018-04-20T06:00:58Z

I'm not sure, the semantic of the parameter is not the best definitely, because the aim of this attribute is to allow the remote connections for a group of users, but by default with this configuration:

{rabbit, [{loopback_users: []}]}

you're allowing the connection to another bind (not only the loopback interface) to the guest user, in my opinion this seems a bit extrange, and there's not so much documentation about this configuration attribute

jjuarez · 2018-04-23T07:40:48Z

All right them, now the new module variable is called allow_remote_guest_connection as you suggested @wyardley I think it would be more appropriate

wyardley · 2018-04-23T20:15:49Z

I guess another way to do it would be to make a parameter loopback_users with the default value, and let an empty array be passed in to achieve the same result. I'm not sure which design is better.

I'm going to request feedback from @bastelfreak

bastelfreak

Hey people,
IMO the approach from @wyardley is slightly better because it allows more flexibility.

wyardley · 2018-04-23T20:42:08Z

Thanks @bastelfreak
@jjuarez - does that make sense to you? I'm really sorry I didn't think of it in that way earlier :/
Either way, thanks for submitting this and for taking the time to write tests.

jjuarez · 2018-04-24T06:28:08Z

Yes, of course, It make sense for me, the problem with this approach is the semantic of this configuration parameter, because I'm not sure what would be the default value for this list, I guest it could be something like that:

{loopback_users, [<<"guest">>]}

but I'm not sure and I didn't find good information about this, only a configuration example but it's enough. So let me make a try with this new approach.

…opback interfaces

…est' user explicitly

wyardley

One minor change (see notes); other than that, and fixing the rubocop failures (https://travis-ci.org/voxpupuli/puppet-rabbitmq/jobs/370510357), seems sensible and well implemented.

If you're able, can you try to squash the whole thing down to one commit in your feature branch?

wyardley · 2018-04-24T16:56:53Z

manifests/init.pp

@@ -278,6 +279,7 @@
  Boolean $ssl_erl_dist                                            = $rabbitmq::params::ssl_erl_dist,
  Optional[String] $rabbitmqadmin_package                          = $rabbitmq::params::rabbitmqadmin_package,
  Array $archive_options                                           = $rabbitmq::params::archive_options,
+  Optional[Array] $loopback_users                                  = $rabbitmq::params::loopback_users,


This is a strange quirk of how Puppet works, but a param can't be Optional if it has a default. Well, technically, it can, but it can only be overridden with undef using hiera, so it's safer to not make it optional, and pass in an empty array (many people's preferred pattern anyway) to set it to an empty list when necessary.

Yep, you're right, I'm working with puppet 4 and the automatic hiera bind then I forgot the "old" params pattern; I think an empty list as default value for this parameter is not the best choice to achieve the behaviour that you could expect, so I could make not optional this parameter a fix the default value to the list with the guest user, let's see

Right, I think the right fix is to make the parameter not optional, and if users want to allow the guest user to login remotely as well, they can pass in an empty array. You could explicitly mention that in the param docs if you want to make it more clear.

But the default value should be ['guest'] and then just Array $loopback_users. I think we're saying the same thing 😃

Yes, we're in the same page

wyardley · 2018-04-24T17:00:08Z

ps - https://www.rabbitmq.com/configure.html#configuration-file does confirm that 'guest' is the default setting for that parameter, so I think we're good there.

Changes were made

wyardley · 2018-04-24T20:08:56Z

@jjuarez Merged, and thanks for the contribution!

jjuarez · 2018-04-24T20:12:28Z

It has been a pleasure to collaborate, thanks @wyardley

wyardley · 2018-04-26T17:48:51Z

@jjuarez also interesting workaround noted here: #480 (comment)

Obviously, not needed now, but might be handy if you need to tweak other config variables; I should have thought of that.

jjuarez · 2018-04-28T18:58:29Z

Thanks @wyardley it looks very useful to help me “temporary” with my configuration issue

Sorta resurrecting https://review.opendev.org/#/c/324016/ now that voxpupuli/puppet-rabbitmq#699 merged. This commit prevents a duplicate entry for loopback_users in the rabbitmq.config file: {loopback_users, [<<"guest">>]}, {loopback_users, []}, keeping only the expected empty array. Change-Id: I0c291bf5611f02983c430179206556bae3b38736

* Update tripleo-heat-templates from branch 'master' - Merge "Properly set loopback_users via rabbitmq::loopback_users" - Properly set loopback_users via rabbitmq::loopback_users Sorta resurrecting https://review.opendev.org/#/c/324016/ now that voxpupuli/puppet-rabbitmq#699 merged. This commit prevents a duplicate entry for loopback_users in the rabbitmq.config file: {loopback_users, [<<"guest">>]}, {loopback_users, []}, keeping only the expected empty array. Change-Id: I0c291bf5611f02983c430179206556bae3b38736

Sorta resurrecting https://review.opendev.org/#/c/324016/ now that voxpupuli/puppet-rabbitmq#699 merged. This commit prevents a duplicate entry for loopback_users in the rabbitmq.config file: {loopback_users, [<<"guest">>]}, {loopback_users, []}, keeping only the expected empty array. Change-Id: I0c291bf5611f02983c430179206556bae3b38736 (cherry picked from commit 04a5937)

Sorta resurrecting https://review.opendev.org/#/c/324016/ now that voxpupuli/puppet-rabbitmq#699 merged. This commit prevents a duplicate entry for loopback_users in the rabbitmq.config file: {loopback_users, [<<"guest">>]}, {loopback_users, []}, keeping only the expected empty array. Note: not a 100% clean cherry pick due to tht path changes. (cherry picked from commit 04a5937) Change-Id: I0c291bf5611f02983c430179206556bae3b38736

…in remotely) (voxpupuli#699) * Adds new parameter loopback_users, defaulting to ["guest"]

Javier Juarez added 5 commits April 19, 2018 22:58

Introduces a new parameter allow_remote_connections, to change the de…

d06fd80

…fault policy of the loopback access

Default value for the allow_remote_connections parameter, this doesn'…

70fa0c6

…t change the default behaviour of the module

Pass the new allow_remote_connections parameter

ee01521

Handles the configuration file with the new parameter

cd0d316

The test for the new parameter

b00acb6

Renames the new module variable to allow_remote_guest_connections

371c818

wyardley requested a review from bastelfreak April 23, 2018 20:15

bastelfreak previously requested changes Apr 23, 2018

View reviewed changes

Javier Juarez added 5 commits April 24, 2018 12:38

Adds the new parameter loopback_users, to manage the access to the lo…

01241d5

…opback interfaces

Gets the new lookback_user parameter

1d24a6f

Configures the default value for the loopback_users list with the 'gu…

99e993b

…est' user explicitly

Handles the loopback_users list in the configuration template

1aebcf7

Adds a new test case for a configured list of users

45b94fb

wyardley requested changes Apr 24, 2018

View reviewed changes

Solves a rubocop offense related with a list

42044fb

wyardley added the enhancement New feature or request label Apr 24, 2018

wyardley changed the title ~~Configuration to allow remote connections using the guest user (loopback_users)~~ Add loopback_users parameter (adds ability to allow guest user to login remotely) Apr 24, 2018

Makes not optional the loopback_users parameter

84b65d9

wyardley approved these changes Apr 24, 2018

View reviewed changes

wyardley merged commit 0ada399 into voxpupuli:master Apr 24, 2018

wyardley mentioned this pull request Apr 26, 2018

Add support for configuration of loopback_users #480

Closed

cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021

Add loopback_users parameter (adds ability to allow guest user to log…

428b323

…in remotely) (voxpupuli#699) * Adds new parameter loopback_users, defaulting to ["guest"]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add loopback_users parameter (adds ability to allow guest user to login remotely) #699

Add loopback_users parameter (adds ability to allow guest user to login remotely) #699

jjuarez commented Apr 19, 2018

wyardley commented Apr 19, 2018

jjuarez commented Apr 20, 2018 •

edited

Loading

jjuarez commented Apr 23, 2018

wyardley commented Apr 23, 2018

bastelfreak left a comment

wyardley commented Apr 23, 2018

jjuarez commented Apr 24, 2018 •

edited

Loading

wyardley left a comment

wyardley Apr 24, 2018

jjuarez Apr 24, 2018

wyardley Apr 24, 2018

wyardley Apr 24, 2018

jjuarez Apr 24, 2018

wyardley commented Apr 24, 2018

wyardley commented Apr 24, 2018

jjuarez commented Apr 24, 2018

wyardley commented Apr 26, 2018

jjuarez commented Apr 28, 2018

Add loopback_users parameter (adds ability to allow guest user to login remotely) #699

Add loopback_users parameter (adds ability to allow guest user to login remotely) #699

Conversation

jjuarez commented Apr 19, 2018

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

wyardley commented Apr 19, 2018

jjuarez commented Apr 20, 2018 • edited Loading

jjuarez commented Apr 23, 2018

wyardley commented Apr 23, 2018

bastelfreak left a comment

Choose a reason for hiding this comment

wyardley commented Apr 23, 2018

jjuarez commented Apr 24, 2018 • edited Loading

wyardley left a comment

Choose a reason for hiding this comment

wyardley Apr 24, 2018

Choose a reason for hiding this comment

jjuarez Apr 24, 2018

Choose a reason for hiding this comment

wyardley Apr 24, 2018

Choose a reason for hiding this comment

wyardley Apr 24, 2018

Choose a reason for hiding this comment

jjuarez Apr 24, 2018

Choose a reason for hiding this comment

wyardley commented Apr 24, 2018

wyardley commented Apr 24, 2018

jjuarez commented Apr 24, 2018

wyardley commented Apr 26, 2018

jjuarez commented Apr 28, 2018

jjuarez commented Apr 20, 2018 •

edited

Loading

jjuarez commented Apr 24, 2018 •

edited

Loading