Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sensu Redis - Support boolean redis_tls parameter (cont #967) #969

Merged
merged 2 commits into from
Sep 5, 2018

Conversation

treydock
Copy link
Collaborator

Pull Request Checklist

Description

Add on to work in #967

Related Issue

Fixes #900 and extends #967 .

Motivation and Context

Ensure redis_tls is a proper boolean property and ensure the provider is checking the symbol value.

How Has This Been Tested?

I modified tests/sensu-server.pp to add redis_tls => true:

[root@sensu-server ~]# puppet apply sensu-server.pp
Notice: Compiled catalog for sensu-server.example.com in environment production in 1.87 seconds
Notice: /Stage[main]/Sensu::Redis::Config/Sensu_redis_config[sensu-server.example.com]/tls: tls changed 'false' to 'true'
Notice: /Stage[main]/Sensu::Api/Service[sensu-api]: Triggered 'refresh' from 2 events
Notice: /Stage[main]/Sensu::Server::Service/Service[sensu-server]: Triggered 'refresh' from 1 events
Notice: Applied catalog in 3.21 seconds

Resulting config:

[root@sensu-server ~]# cat /etc/sensu/conf.d/redis.json
{
  "redis": {
    "port": 6379,
    "host": "127.0.0.1",
    "reconnect_on_error": true,
    "db": 0,
    "auto_reconnect": true,
    "tls": {
    }
  }
}

General

  • New parameters are documented

  • New parameters have tests

  • Tests pass - bundle exec rake validate lint spec

@treydock
Copy link
Collaborator Author

Worth noting testing with tests/sensu-server.pp is not great because once tls support is added to redis.json it breaks sensu-server service since the Vagrant environment doesn't configure Redis for TLS, is my guess.

context "with redis_tls specified as #{value}" do
let(:params) { { :redis_tls => value } }

it { should contain_sensu_redis_config('testhost.domain.com').with_tls(value) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is false doesn't the provider delete the tls key?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but this just tests the parameter getting passed to sensu_redis_config resource which is either true or false. This doesn't test the provider behavior.

@ghoneycutt
Copy link
Collaborator

@jaredledvina Could you please confirm this works for you.

@jaredledvina
Copy link
Contributor

@ghoneycutt - Yep, I'm AFK but will check once I'm back at a computer (probably early AM Tuesday). Thanks!

@jaredledvina
Copy link
Contributor

Alright, sorry for the delay, I applied this branch to our internal version of the module and undid my forked changes. Gave it a run on our Sensu instances and it works like a charm.
And our redis.json file looks solid!

> cat /etc/sensu/conf.d/redis.json
{
  "redis": {
    "port": 6379,
    "host": "SNIP",
    "password": "SNIP",
    "reconnect_on_error": true,
    "db": 0,
    "auto_reconnect": true,
    "tls": {
    }
  }
}

Amazing, thanks so much @treydock and @ghoneycutt !

@ghoneycutt ghoneycutt merged commit 57b0d6d into sensu:master Sep 5, 2018
@ghoneycutt
Copy link
Collaborator

Released in v2.54.0

@treydock treydock deleted the 967 branch September 6, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Support Redis ssl configuration
3 participants