Feature/ssl support

[mindriot88]

Pull Request Checklist

• Add the ability to set the scheme redis client option which opens up support for redis server configurations which use SSL.

This can be set on any of the plugin checks using -T rediss or --transport rediss

General

• [ ] Update Changelog following the conventions laid out on Our CHANGELOG Guidelines

• [ ] Update README with any necessary configuration snippets

• [ ] Binstubs are created if needed

• [ ] RuboCop passes

• [ ] Existing tests pass

New Plugins

• [ ] Tests

• [ ] Add the plugin to the README

• [ ] Does it have a complete header as outlined here

Purpose

Known Compatibility Issues

[majormoses]

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

[majormoses]

Hmm looking at the code I would not have expected that to break the integration tests. I will try to take a closer look when I have more time.

[majormoses]

Im thinking scheme is not the right config option name. It is ambiguous and often conflicts with metric gathering scripts that offer a --scheme my.hostname.my_custom_service to control how the metrics are formatted before sending to your TSDB. I would opt for transport or protocol. Also is there a list of protocols that we can restrict it to? if so we can do something similar to this: https://github.com/sensu-plugins/sensu-plugins-redis/blob/3.0.1/lib/redis_client_options.rb#L75 so it helps users know what the options are.

[mindriot88]

@majormoses I will take a look at revising the implementation based on your feedback once I get a free moment, thanks

[mindriot88]

The options that it should be restricted to are redis and rediss as those are the only supported schemes in the redis client, I will make those changes.

[mindriot88]

@majormoses your comments should be addressed, ive added a test as well but wasn't able to run your kitchen tests locally, I see they are failing in travis as well at the moment.

Let me know if you need anything else changing once those are fixed.

[majormoses]

Overall looks good but I need to look into why the tests are failing.

Assuming you have a working ruby and docker setup you should be able to run the tests via:

$ bundle install
...
$ bundle exec kitchen converge ruby-241-debian-8
...
$ bundle exec kitchen verify ruby-241-debian-8

[majormoses]

I am able to replicate the tests failing locally, not sure if they are bad tests or if there was some other change.

[majormoses]

This one looks to me like maybe the output changed in redis versions? Need to validate and if so we can make it match either one.

1) Command "/usr/local/bin/check-redis-slave-status.rb -p 6380" stdout should match /The\ redis\ master\ links\ status\ is\ up!/
            Failure/Error: its(:stdout) { should match(Regexp.new(Regexp.escape('The redis master links status is up!'))) }
              expected "RedisSlaveCheck OK: This redis server is master\n" to match /The\ redis\ master\ links\ status\ is\ up!/
              Diff:
              @@ -1,2 +1,2 @@
              -/The\ redis\ master\ links\ status\ is\ up!/
              +RedisSlaveCheck OK: This redis server is master

[majormoses]

These looks like maybe they legitimate problems or something with how we are setting up the tests:

2) Command "/usr/local/bin/check-redis-slave-status.rb -p 6381" exit_status should eq 2
            Failure/Error: its(:exit_status) { should eq 2 }

              expected: 2
                   got: 0

              (compared using ==)
              /bin/sh -c /usr/local/bin/check-redis-slave-status.rb\ -p\ 6381
              RedisSlaveCheck OK: This redis server is master

 3) Command "/usr/local/bin/check-redis-slave-status.rb -p 6381" stdout should match /The\ redis\ master\ link\ status\ to:\ 127\.0\.0\.1\ is\ down!/
            Failure/Error: its(:stdout) { should match(Regexp.new(Regexp.escape('The redis master link status to: 127.0.0.1 is down!'))) }
              expected "RedisSlaveCheck OK: This redis server is master\n" to match /The\ redis\ master\ link\ status\ to:\ 127\.0\.0\.1\ is\ down!/
              Diff:
              @@ -1,2 +1,2 @@
              -/The\ redis\ master\ link\ status\ to:\ 127\.0\.0\.1\ is\ down!/
              +RedisSlaveCheck OK: This redis server is master

[majormoses]

I was able to validate those failures on master.

[majormoses]

Looks like the last passing build (and commit) on master was 8 months ago: https://travis-ci.org/sensu-plugins/sensu-plugins-redis/builds/359647878 so its probably something that changed in redis and those tests need to be updated. I am gonna be traveling this week for the holidays so I will probably take care of this when I get back.

[majormoses]

Sorry for the long delay, it looks like its an issue with the testing bootstrap. It's honestly been a while since I did much with redis so maybe I am misunderstanding how redis clustering works (and/or how redis-cli works). I converged the container and shelled in and ran the redis-cli command to gain access to redis on each of the three ports (verified that all were running after installing and running netstat) but the results for info are not what I would have expected:

# Replication
role:master
connected_slaves:0
master_replid:2cd4a946a0962ca379dc51767ef8787852cc3b4e
master_replid2:0000000000000000000000000000000000000000
master_repl_offset:0
second_repl_offset:-1
repl_backlog_active:0
repl_backlog_size:1048576
repl_backlog_first_byte_offset:0
repl_backlog_histlen:0

Specifically this tells us that all redis nodes are configured as masters and not slaves as expected:

role:master
connected_slaves:0

# Cluster
cluster_enabled:0

Which leads me to believe that the tests themselves are working as intended but there was a breaking change to the configuration interfaces we were using. As we are taking the latest stable that seems likely @Evesy not sure if you have any other insight as I believe you were the one who initially worked on this.

[majormoses]

released: https://rubygems.org/gems/sensu-plugins-redis/versions/3.1.0