add `rabbitmq_config` resource #1639

arlimus · 2017-04-07T17:54:56Z

rabbitmq_config

This PR introduces the rabbitmq_config resource. It parses the full configuration file and represents it internally as a hash that can be accessed for InSpec tests.

It may require a bit more alignment with other resources and some optimization. (also recognizing more shared patterns)

implications

There is 2 large implications with this move:

Adding a complex parser
Adding the parslet gem as a dependency

1. Adding a complex parser to the core may require more effort down the line. I'm happy with the way it turned out in the end, but I'm aware that the underlying mechanism still carries a number of corner-cases we must be mindful of. They may only apply to very few use-cases (or put differently: all config files I ever used have been tested well), but it may bite us. The best example of this was the inclusion of parsing bit-streams (took 50% of the parser code)

2. Adding parslet a 78k ruby lib without any additional dependencies (which we already have in our dependency tree! see edit below). The implication is to have this library available for other rather complex config files, including Nginx, Apache, HCL, and custom configs.

I investigated treetop, parslet, and citrus, all of which are great for this task. Of those, parslet is actively maintained with recent contributions a large community. It also functions fully in Ruby without creating dynamic classes.

This may become a great foundation for other parsers down the line. What do you think?

edit: Apparently TOML already requires parslet! The dependency is rather outdated (3years+). I went down a few versions to prevent conflicts and might take a stab at the toml lib.

chris-rock

Thank you @arlimus This is awesome. I added a note regarding Windows support, since the configuration is also available on Windows machines

chris-rock · 2017-04-09T17:27:48Z

lib/resources/rabbitmq_conf.rb

+    name 'rabbitmq_config'
+    desc 'Use the rabbitmq_config InSpec resource to test configuration data '\
+         'for the RabbitMQ service located in /etc/rabbitmq/rabbitmq.config on '\
+         'Linux and UNIX platforms.'


Is there any reason why Windows rabitmq configurations cannot be parsed?

Good point! Not at all actually, just haven't implemented default paths for Windows yet.

chris-rock · 2017-04-09T17:28:24Z

lib/resources/rabbitmq_conf.rb

+         'for the RabbitMQ service located in /etc/rabbitmq/rabbitmq.config on '\
+         'Linux and UNIX platforms.'
+    example "
+      describe rabbitmq_config.params('rabbit', 'ssl_listeners') do


I like params to be established as a pattern to retrieve all values for all configuration files

yes please 😁

chris-rock · 2017-04-09T17:31:31Z

lib/resources/rabbitmq_conf.rb

+    "
+
+    def initialize(conf_path = nil)
+      @conf_path = conf_path || '/etc/rabbitmq/rabbitmq.config'


Are we going to supports the default per OS? See https://www.rabbitmq.com/configure.html#configuration-file

Great idea, let's add that in the next PR?

adamleff

This is a heck of a change. Nothing I have is a blocker but it would probably good to add some more detail to the #to_s method as described. Otherwise, I say we ship it and see what we uncover in the field.

adamleff · 2017-04-12T14:27:07Z

lib/resources/rabbitmq_conf.rb

+    end
+
+    def to_s
+      'RabbitMQ Configuration'


Should we include @conf_path here so it's clear in the output what config file we read from? Thinking about the use case where a user is reading multiple RabbitMQ config files. We do this pattern with mysql_conf, etc.

Will do, thank you! (curious, I didn't see it in mysql_conf.rb, but it is used in pip, x509 etc)

adamleff · 2017-04-12T14:28:05Z

lib/resources/rabbitmq_conf.rb

+    private
+
+    def read_content
+      return @content if defined?(@content)


I've been bitten enough times by defined? not returning what I expect it to that I usually follow the pattern of return @content unless @content.nil? in these situations. Not a blocker, just something to consider...

The reason I started to use this convention initially was to avoid queries to the system for files that might be nil. It typically happens with missing permissions, permission the file read may be attempted a large number of times. It is only meant as an optimization.

That said, this part as well as the pieces below really need to be abstracted away, since these steps are used frequently.

adamleff · 2017-04-12T14:29:05Z

lib/resources/rabbitmq_conf.rb

+      end
+
+      @content = file.content
+      if @content.empty? && !file.empty?


Curious what situation would cause one of these to be true while the other is false...

Good catch, I copied this from another resource and will remove it.
We had this case in the past, when files would return an empty string due to missing permissions (and in line with serverspec behavior) on some transport connections. We seem to have unified this in train now, so it is not needed anymore. Thank you!

adamleff · 2017-04-12T14:39:42Z

lib/resources/rabbitmq_conf.rb

+      return @params = {} if read_content.nil?
+      lex = ErlangParser.new.parse(read_content)
+      tree = ErlangTransform.new.apply(lex)
+      @params = turn_to_hash(tree)


I imagine this is going to be a pretty common need... should we instead make a #to_h method inside the ErlangTransform class?

Good idea, we might need a small class in between. That hash-conversion only works with a Array-Tuple combination, which is typical for RabbitMQ files. If we use this in other places the syntax might be very different. I'll add it to another class that abstracts the parser and transformer away

Signed-off-by: Dominik Richter <dominik.richter@gmail.com>

adamleff · 2017-04-12T19:07:00Z

Looks good to me, @arlimus! If @chris-rock is good with it, I'm good. Let's just squash down to a single commit (which we can do via GitHub squash-and-merge, you don't actually have to do it... and thank you for adding incremental commits, makes it SO much easier to review).

chris-rock

Cool @arlimus Lets add Windows support in another PR.

arlimus added 4 commits April 7, 2017 17:01

rabbitmq parser: create erlang syntax lexer

ea1137b

rabbitmq parser: tree transformation to ruby objects

860750d

rabbitmq parser: add simple binary handling

4197b5a

rabbitmq parser: support complex bit-stream handling

21258f7

arlimus added the in progress label Apr 7, 2017

arlimus force-pushed the dr/rabbitmq_config branch 2 times, most recently from dfc925f to 52a6b0d Compare April 7, 2017 17:58

arlimus requested review from alexpop, chris-rock and adamleff April 7, 2017 18:13

chris-rock reviewed Apr 9, 2017

View reviewed changes

adamleff approved these changes Apr 12, 2017

View reviewed changes

add rabbitmq config resource

02e435b

Signed-off-by: Dominik Richter <dominik.richter@gmail.com>

arlimus force-pushed the dr/rabbitmq_config branch from 52a6b0d to 02e435b Compare April 12, 2017 18:51

adamleff approved these changes Apr 12, 2017

View reviewed changes

chris-rock approved these changes Apr 12, 2017

View reviewed changes

chris-rock merged commit 29e286f into master Apr 12, 2017

chris-rock deleted the dr/rabbitmq_config branch April 12, 2017 19:19

chris-rock removed the in progress label Apr 12, 2017

chris-rock mentioned this pull request Apr 12, 2017

add rabbitmq server controls dev-sec/openstack-baseline#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `rabbitmq_config` resource #1639

add `rabbitmq_config` resource #1639

arlimus commented Apr 7, 2017 •

edited

Loading

chris-rock left a comment

chris-rock Apr 9, 2017

arlimus Apr 11, 2017

chris-rock Apr 9, 2017

arlimus Apr 11, 2017

chris-rock Apr 9, 2017

arlimus Apr 11, 2017

adamleff left a comment

adamleff Apr 12, 2017

arlimus Apr 12, 2017 •

edited

Loading

adamleff Apr 12, 2017

arlimus Apr 12, 2017

adamleff Apr 12, 2017

arlimus Apr 12, 2017

adamleff Apr 12, 2017

arlimus Apr 12, 2017

adamleff commented Apr 12, 2017

chris-rock left a comment

add rabbitmq_config resource #1639

add rabbitmq_config resource #1639

Conversation

arlimus commented Apr 7, 2017 • edited Loading

rabbitmq_config

implications

chris-rock 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

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlimus Apr 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff commented Apr 12, 2017

chris-rock left a comment

Choose a reason for hiding this comment

add `rabbitmq_config` resource #1639

add `rabbitmq_config` resource #1639

arlimus commented Apr 7, 2017 •

edited

Loading

arlimus Apr 12, 2017 •

edited

Loading