From 4877e2e84a1f22fb1f68ceaf44ef64bd26239ae1 Mon Sep 17 00:00:00 2001
From: Modestas Vainius <modestas@inventi.lt>
Date: Fri, 27 May 2016 15:36:20 +0300
Subject: [PATCH 1/2] Add support for Redis Sentinels

Based on @RiRa12621 work: https://github.com/RiRa12621/sensu-puppet
---
 .../provider/sensu_redis_config/json.rb       |  36 +++++-
 lib/puppet/type/sensu_redis_config.rb         |  50 ++++++++-
 manifests/init.pp                             |   5 +
 manifests/redis/config.pp                     |  10 +-
 spec/classes/sensu_redis_spec.rb              |  37 ++++++-
 spec/unit/sensu_redis_config_spec.rb          | 103 +++++++++++++++++-
 6 files changed, 225 insertions(+), 16 deletions(-)

diff --git a/lib/puppet/provider/sensu_redis_config/json.rb b/lib/puppet/provider/sensu_redis_config/json.rb
index ab09f64bfb..fef18688d6 100644
--- a/lib/puppet/provider/sensu_redis_config/json.rb
+++ b/lib/puppet/provider/sensu_redis_config/json.rb
@@ -16,6 +16,14 @@ def conf
   end
 
   def flush
+    # Clean nil valued properties, esp. sentinel related
+    # Related to https://github.com/sensu/sensu-puppet/issues/394.
+    self.class.resource_type.validproperties.each do |prop|
+      if resource.should(prop).nil?
+        conf['redis'].delete prop.to_s
+      end
+    end
+
     File.open(config_file, 'w') do |f|
       f.puts JSON.pretty_generate(conf)
     end
@@ -38,19 +46,27 @@ def exists?
   end
 
   def port
-    conf['redis']['port'].to_s
+    if conf['redis']['port'] then conf['redis']['port'].to_s else :absent end
   end
 
   def port=(value)
-    conf['redis']['port'] = value.to_i
+    if value == :absent
+      conf['redis'].delete 'port'
+    else
+      conf['redis']['port'] = value.to_i
+    end
   end
 
   def host
-    conf['redis']['host']
+    conf['redis']['host'] || :absent
   end
 
   def host=(value)
-    conf['redis']['host'] = value
+    if value == :absent
+      conf['redis'].delete 'host'
+    else
+      conf['redis']['host'] = value
+    end
   end
 
   def password
@@ -77,6 +93,18 @@ def db=(value)
     conf['redis']['db'] = value.to_i
   end
 
+  def sentinels
+    conf['redis']['sentinels'] || []
+  end
+
+  def sentinels=(value)
+    if value == []
+      conf['redis'].delete 'sentinels'
+    else
+      conf['redis']['sentinels'] = value
+    end
+  end
+
   def auto_reconnect
     conf['redis']['auto_reconnect']
   end
diff --git a/lib/puppet/type/sensu_redis_config.rb b/lib/puppet/type/sensu_redis_config.rb
index 4361691d43..c1061c93db 100644
--- a/lib/puppet/type/sensu_redis_config.rb
+++ b/lib/puppet/type/sensu_redis_config.rb
@@ -1,3 +1,4 @@
+require 'set'
 require File.expand_path(File.join(File.dirname(__FILE__), '..', '..',
                                    'puppet_x', 'sensu', 'boolean_property.rb'))
 
@@ -13,6 +14,22 @@ def initialize(*args)
     ].select { |ref| catalog.resource(ref) }
   end
 
+  def has_sentinels?
+    sentinels = self.should(:sentinels)
+    return sentinels && !sentinels.empty?
+  end
+
+  def pre_run_check
+    if self.has_sentinels? then
+      if self.should(:host) && self.should(:host) != :absent then
+        raise Puppet::Error, "Redis 'host' (#{self.should(:host)}) must not be specified when sentinels are specified"
+      end
+      if self.should(:port) && self.should(:port) != :absent then
+        raise Puppet::Error, "Redis 'port' (#{self.should(:port)}) must not be specified when sentinels are specified"
+      end
+    end
+  end
+
   ensurable do
     newvalue(:present) do
       provider.create
@@ -37,13 +54,25 @@ def initialize(*args)
   newproperty(:port) do
     desc "The port that Redis is listening on"
 
-    defaultto '6379'
+    defaultto {
+      if !@resource.has_sentinels? then '6379' else :absent end
+    }
+    def insync?(is)
+      if should.is_a?(Symbol) then should == is else super(is) end
+    end
   end
 
   newproperty(:host) do
     desc "The hostname that Redis is listening on"
 
-    defaultto '127.0.0.1'
+    defaultto {
+      # Use absent to ensure that config is flushed
+      # when property gets unset
+      if !@resource.has_sentinels? then '127.0.0.1' else :absent end
+    }
+    def insync?(is)
+      if should.is_a?(Symbol) then should == is else super(is) end
+    end
   end
 
   newproperty(:password) do
@@ -68,6 +97,23 @@ def initialize(*args)
     defaultto :true
   end
 
+  newproperty(:sentinels, :array_matching => :all) do
+    desc "Redis Sentinel configuration for HA clustering"
+    defaultto []
+
+    def insync?(is)
+      # this probably needs more checks, for duplicate values, etc
+      # but for now it works fine
+      Set.new(is) == Set.new(should)
+    end
+
+    munge do |value|
+      Hash[value.map do |k, v|
+        [k, if k == "port" then v.to_i else v.to_s end]
+      end]
+    end
+  end
+
   autorequire(:package) do
     ['sensu']
   end
diff --git a/manifests/init.pp b/manifests/init.pp
index 9c91a5b4d6..686cf4fe44 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -173,6 +173,10 @@
 #   Integer.  The Redis instance DB to use/select
 #   Default: 0
 #
+# [*redis_sentinels*]
+#   Array. Redis Sentinel configuration and connection information for one or more Sentinels
+#   Default: Not configured
+#
 # [*redis_auto_reconnect*]
 #   Boolean.  Reconnect to Redis in the event of a connection failure
 #   Default: true
@@ -354,6 +358,7 @@
   $redis_reconnect_on_error       = false,
   $redis_db                       = 0,
   $redis_auto_reconnect           = true,
+  $redis_sentinels                = undef,
   $api_bind                       = '0.0.0.0',
   $api_host                       = '127.0.0.1',
   $api_port                       = 4567,
diff --git a/manifests/redis/config.pp b/manifests/redis/config.pp
index a68ed13a81..600c3e3433 100644
--- a/manifests/redis/config.pp
+++ b/manifests/redis/config.pp
@@ -23,15 +23,21 @@
     before => Sensu_redis_config[$::fqdn],
   }
 
+  $has_sentinels = !($sensu::redis_sentinels == undef or $sensu::redis_sentinels == [])
+  $host = $has_sentinels ? { false => $sensu::redis_host, true  => undef, }
+  $port = $has_sentinels ? { false => $sensu::redis_port, true  => undef, }
+  $sentinels = $has_sentinels ? { true  => $sensu::redis_sentinels, false => undef, }
+
   sensu_redis_config { $::fqdn:
     ensure             => $ensure,
     base_path          => "${sensu::etc_dir}/conf.d",
-    host               => $sensu::redis_host,
-    port               => $sensu::redis_port,
+    host               => $host,
+    port               => $port,
     password           => $sensu::redis_password,
     reconnect_on_error => $sensu::redis_reconnect_on_error,
     db                 => $sensu::redis_db,
     auto_reconnect     => $sensu::redis_auto_reconnect,
+    sentinels          => $sentinels,
   }
 
 }
diff --git a/spec/classes/sensu_redis_spec.rb b/spec/classes/sensu_redis_spec.rb
index 3dfc9e7e4b..0f08c9db17 100644
--- a/spec/classes/sensu_redis_spec.rb
+++ b/spec/classes/sensu_redis_spec.rb
@@ -14,7 +14,7 @@
       )}
     end # default settings
 
-    context 'be configurable' do
+    context 'be configurable without sentinels' do
       let(:params) { {
         :redis_host           => 'redis.domain.com',
         :redis_port           => 1234,
@@ -28,9 +28,40 @@
         :port           => 1234,
         :password       => 'password',
         :db             => 1,
-        :auto_reconnect => false        
+        :auto_reconnect => false,
+        :sentinels      => nil,
       )}
-    end # be configurable
+    end # be configurable without sentinels
+
+    context 'be configurable with sentinels' do
+      let(:params) { {
+        :redis_password       => 'password',
+        :redis_db             => 1,
+        :redis_auto_reconnect => false,
+        :redis_sentinels      => [{
+            'host' => 'redis1.domain.com',
+            'port' => 1234
+        }, {
+            'host' => 'redis2.domain.com',
+            'port' => '5678'
+        }]
+      } }
+
+      it { should contain_sensu_redis_config('testhost.domain.com').with(
+        :host           => nil,
+        :port           => nil,
+        :password       => 'password',
+        :db             => 1,
+        :auto_reconnect => false,
+        :sentinels      => [{
+            'host' => 'redis1.domain.com',
+            'port' => 1234
+        }, {
+            'host'  => 'redis2.domain.com',
+            'port'  => 5678
+        }]
+      )}
+    end # be configurable with sentinels
 
     context 'with server' do
       let(:params) { { :server => true } }
diff --git a/spec/unit/sensu_redis_config_spec.rb b/spec/unit/sensu_redis_config_spec.rb
index 6c73a6d22a..550762edfe 100644
--- a/spec/unit/sensu_redis_config_spec.rb
+++ b/spec/unit/sensu_redis_config_spec.rb
@@ -1,7 +1,16 @@
 require 'spec_helper'
 
 describe Puppet::Type.type(:sensu_redis_config) do
-  provider_class = described_class.provider(:json)
+  let :provider_class do
+    described_class.provider(:json)
+  end
+
+  def create_type_instance(resource_hash)
+    result = described_class.new(resource_hash)
+    provider_instance = provider_class.new(resource_hash)
+    result.provider = provider_instance
+    result
+  end
 
   let :resource_hash do
     {
@@ -11,10 +20,7 @@
   end
 
   let :type_instance do
-    result = described_class.new(resource_hash)
-    provider_instance = provider_class.new(resource_hash)
-    result.provider = provider_instance
-    result
+    create_type_instance(resource_hash)
   end
 
   describe 'reconnect_on_error property' do
@@ -44,5 +50,92 @@
 
   end
 
+  describe "sentinels" do
+    it "defaults (no sentinels)" do
+      expect(type_instance.parameter(:sentinels).value).to eq([])
+    end
+
+    context "single value" do
+      let :inst do
+        create_type_instance(resource_hash.merge({
+          :sentinels => {
+            'host' => 'redis.sentinel.1',
+            'port' => '12345',
+          }
+        }))
+      end
+
+      it "munges it (to array with port as int)" do
+        expect(inst.parameter(:sentinels).value).to eq([{
+          'host' => 'redis.sentinel.1',
+          'port' => 12345
+        }])
+      end
+
+      it "assumes insync? for the same values" do
+        expect(inst.parameter(:sentinels).safe_insync?([
+            'host' => 'redis.sentinel.1',
+            'port' => 12345
+        ])).to be true
+      end
+
+      [nil, []].each do |v|
+        it "assumes not insync? for empty value #{v.inspect}" do
+          expect(inst.parameter(:sentinels).safe_insync?([])).to be false
+        end
+      end
+
+      it "assumes not insync? for different value" do
+        expect(inst.parameter(:sentinels).safe_insync?([
+            'host' => 'redis.sentinel.1.foo',
+            'port' => 12345
+        ])).to be false
+      end
+    end
+
+    context "multiple values" do
+      let :inst do
+        create_type_instance(resource_hash.merge({
+          :sentinels => [{
+            'host' => 'redis.sentinel.1',
+            'port' => '12345',
+          }, {
+            'host' => 'redis.sentinel.2',
+            'port' => 6789,
+          }]
+        }))
+      end
+
+      it "can munge them" do
+        expect(inst.parameter(:sentinels).value).to eq([{
+          'host' => 'redis.sentinel.1',
+          'port' => 12345
+        }, {
+          'host' => 'redis.sentinel.2',
+          'port' => 6789,
+        }])
+      end
+
+      it "assumes insync? for the same values" do
+        expect(inst.parameter(:sentinels).safe_insync?([{
+          'host' => 'redis.sentinel.1',
+          'port' => 12345
+        }, {
+          'host' => 'redis.sentinel.2',
+          'port' => 6789,
+        }])).to be true
+      end
+
+      it "assumes insync? for the same values in different order" do
+        expect(inst.parameter(:sentinels).safe_insync?([{
+          'host' => 'redis.sentinel.2',
+          'port' => 6789,
+        }, {
+          'host' => 'redis.sentinel.1',
+          'port' => 12345
+        }])).to be true
+     end
+    end
+  end
 
 end

From 455d8d39e6072db57299b33e23d65f9b54dd0049 Mon Sep 17 00:00:00 2001
From: Modestas Vainius <modestas@inventi.lt>
Date: Tue, 31 May 2016 16:07:21 +0300
Subject: [PATCH 2/2] Add 'master' parameter for Redis Sentinel config.

Available since sensu 0.23 but currently undocumented. See:

https://github.com/sensu/sensu-docs/issues/380
---
 .../provider/sensu_redis_config/json.rb       | 12 +++++
 lib/puppet/type/sensu_redis_config.rb         | 10 ++++
 manifests/init.pp                             |  5 ++
 manifests/redis/config.pp                     |  2 +
 spec/classes/sensu_redis_spec.rb              |  7 ++-
 spec/unit/sensu_redis_config_spec.rb          | 50 +++++++++++++++++--
 6 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/lib/puppet/provider/sensu_redis_config/json.rb b/lib/puppet/provider/sensu_redis_config/json.rb
index fef18688d6..d0c299ccf6 100644
--- a/lib/puppet/provider/sensu_redis_config/json.rb
+++ b/lib/puppet/provider/sensu_redis_config/json.rb
@@ -105,6 +105,18 @@ def sentinels=(value)
     end
   end
 
+  def master
+    conf['redis']['master'] || :absent
+  end
+
+  def master=(value)
+    if value == :absent
+      conf['redis'].delete 'master'
+    else
+      conf['redis']['master'] = value.to_s
+    end
+  end
+
   def auto_reconnect
     conf['redis']['auto_reconnect']
   end
diff --git a/lib/puppet/type/sensu_redis_config.rb b/lib/puppet/type/sensu_redis_config.rb
index c1061c93db..da6b012152 100644
--- a/lib/puppet/type/sensu_redis_config.rb
+++ b/lib/puppet/type/sensu_redis_config.rb
@@ -114,6 +114,16 @@ def insync?(is)
     end
   end
 
+  newproperty(:master) do
+    desc "Redis master name in the sentinel configuration"
+    # Use absent to ensure that config is flushed
+    # when property gets unset
+    defaultto :absent
+    def insync?(is)
+      if should.is_a?(Symbol) then should == is else super(is) end
+    end
+  end
+
   autorequire(:package) do
     ['sensu']
   end
diff --git a/manifests/init.pp b/manifests/init.pp
index 686cf4fe44..5363b6b5f5 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -177,6 +177,10 @@
 #   Array. Redis Sentinel configuration and connection information for one or more Sentinels
 #   Default: Not configured
 #
+# [*redis_master*]
+#   String. Redis master name in the sentinel configuration
+#   Default: undef. In the end whatever sensu defaults to, which is "mymaster" currently.
+#
 # [*redis_auto_reconnect*]
 #   Boolean.  Reconnect to Redis in the event of a connection failure
 #   Default: true
@@ -359,6 +363,7 @@
   $redis_db                       = 0,
   $redis_auto_reconnect           = true,
   $redis_sentinels                = undef,
+  $redis_master                   = undef,
   $api_bind                       = '0.0.0.0',
   $api_host                       = '127.0.0.1',
   $api_port                       = 4567,
diff --git a/manifests/redis/config.pp b/manifests/redis/config.pp
index 600c3e3433..4771d294e8 100644
--- a/manifests/redis/config.pp
+++ b/manifests/redis/config.pp
@@ -27,6 +27,7 @@
   $host = $has_sentinels ? { false => $sensu::redis_host, true  => undef, }
   $port = $has_sentinels ? { false => $sensu::redis_port, true  => undef, }
   $sentinels = $has_sentinels ? { true  => $sensu::redis_sentinels, false => undef, }
+  $master = $has_sentinels ? { true => $sensu::redis_master, false => undef, }
 
   sensu_redis_config { $::fqdn:
     ensure             => $ensure,
@@ -38,6 +39,7 @@
     db                 => $sensu::redis_db,
     auto_reconnect     => $sensu::redis_auto_reconnect,
     sentinels          => $sentinels,
+    master             => $master,
   }
 
 }
diff --git a/spec/classes/sensu_redis_spec.rb b/spec/classes/sensu_redis_spec.rb
index 0f08c9db17..91cd538ffe 100644
--- a/spec/classes/sensu_redis_spec.rb
+++ b/spec/classes/sensu_redis_spec.rb
@@ -30,6 +30,7 @@
         :db             => 1,
         :auto_reconnect => false,
         :sentinels      => nil,
+        :master         => nil
       )}
     end # be configurable without sentinels
 
@@ -44,7 +45,8 @@
         }, {
             'host' => 'redis2.domain.com',
             'port' => '5678'
-        }]
+        }],
+        :redis_master         => 'master-name'
       } }
 
       it { should contain_sensu_redis_config('testhost.domain.com').with(
@@ -59,7 +61,8 @@
         }, {
             'host'  => 'redis2.domain.com',
             'port'  => 5678
-        }]
+        }],
+        :master         => "master-name"
       )}
     end # be configurable with sentinels
 
diff --git a/spec/unit/sensu_redis_config_spec.rb b/spec/unit/sensu_redis_config_spec.rb
index 550762edfe..b30418ab74 100644
--- a/spec/unit/sensu_redis_config_spec.rb
+++ b/spec/unit/sensu_redis_config_spec.rb
@@ -51,11 +51,27 @@ def create_type_instance(resource_hash)
   end
 
   describe "sentinels" do
-    it "defaults (no sentinels)" do
-      expect(type_instance.parameter(:sentinels).value).to eq([])
+    context "with defaults (no sentinels)" do
+      it "no sentinels" do
+        expect(type_instance.parameter(:sentinels).value).to eq([])
+      end
+
+      it "master.should is :absent" do
+        expect(type_instance.parameter(:master).should).to eq(:absent)
+      end
+
+      it "assumes insync? for :absent value" do
+        expect(type_instance.parameter(:master).safe_insync?(:absent)).to be true
+      end
+
+      ["abc", "absent"].each do |v|
+        it "assumes not insync? for specified master value '#{v.inspect}'" do
+          expect(type_instance.parameter(:master).safe_insync?(v)).to be false
+        end
+      end
     end
 
-    context "single value" do
+    context "with single sentinel" do
       let :inst do
         create_type_instance(resource_hash.merge({
           :sentinels => {
@@ -93,7 +109,7 @@ def create_type_instance(resource_hash)
       end
     end
 
-    context "multiple values" do
+    context "with multiple sentinels" do
       let :inst do
         create_type_instance(resource_hash.merge({
           :sentinels => [{
@@ -136,6 +152,32 @@ def create_type_instance(resource_hash)
         }])).to be true
      end
     end
+
+    context "when master is specified" do
+      let :inst do
+        create_type_instance(resource_hash.merge({
+          :master => "master-name"
+        }))
+      end
+
+      it "master name is propagated" do
+        expect(inst.parameter(:master).value).to eq("master-name")
+      end
+
+      it "assumes insync? for the same master value" do
+        expect(inst.parameter(:master).safe_insync?("master-name")).to be true
+      end
+
+      [:absent, "", nil].each do |v|
+        it "assumes not insync? for empty master value #{v.inspect}" do
+          expect(inst.parameter(:master).safe_insync?(v)).to be false
+        end
+      end
+
+      it "assumes not insync? for different master value" do
+        expect(inst.parameter(:master).safe_insync?("abc")).to be false
+      end
+    end
   end
 
 end