Merge pull request #480 from SkydiveMarius/PC-479

Fixing idempotence issues of new ACL system + improved handling of properties

lib/puppet/provider/consul_policy/default.rb

@@ -35,7 +35,7 @@ def self.prefetch(resources)
         existing_policy = nil
       end
 
-      resource.provider = new({}, @client, rules_encoded, existing_policy)
+      resource.provider = new({}, @client, rules_encoded, existing_policy, resource)
     end
   end
 
@@ -60,17 +60,28 @@ def self.list_policies(acl_api_token, hostname, port, protocol, tries)
     @all_policies
   end
 
-  def initialize(messages, client = nil, rules_encoded = '', existing_policy = nil)
+  def initialize(messages, client = nil, rules_encoded = '', existing_policy = nil, resource = nil)
     super(messages)
     @property_flush = {}
 
     @client = client
     @rules_encoded = rules_encoded
     @existing_policy = existing_policy
+
+    if existing_policy
+      @property_hash = {
+          :id          => existing_policy.id,
+          :description => existing_policy.description
+      }
+
+      if rules_encoded == existing_policy.rules
+        @property_hash[:rules] = resource[:rules]
+      end
+    end
   end
 
   def exists?
-    @property_hash[:ensure] = :present
+    @existing_policy
   end
 
   def create

lib/puppet/provider/consul_token/default.rb

@@ -14,7 +14,7 @@ def self.prefetch(resources)
       tokens = list_tokens(resource[:acl_api_token], resource[:hostname], resource[:port], resource[:protocol], resource[:api_tries])
       token = tokens.select{|token| token.accessor_id == resource[:accessor_id]}
 
-      resource.provider = new({}, @client, token.any? ? token.first : nil)
+      resource.provider = new({}, @client, token.any? ? token.first : nil, resource)
     end
   end
 
@@ -29,16 +29,31 @@ def self.list_tokens(acl_api_token, hostname, port, protocol, tries)
     @token_collection
   end
 
-  def initialize(messages, client = nil, existing_token = nil)
+  def initialize(messages, client = nil, existing_token = nil, resource = nil)
     super(messages)
     @property_flush = {}
 
     @client = client
     @existing_token = existing_token
+
+    if resource
+      @property_hash = {
+          :secret_id  => resource[:secret_id],
+      }
+    end
+
+    if existing_token
+      @property_hash[:accessor_id] = existing_token.accessor_id
+
+      if existing_token.is_policy_list_equal(resource[:policies_by_id], resource[:policies_by_name])
+        @property_hash[:policies_by_id] = resource[:policies_by_id]
+        @property_hash[:policies_by_name] = resource[:policies_by_name]
+      end
+    end
   end
 
   def exists?
-    @property_hash[:ensure] = :present
+    @existing_token
   end
 
   def create

lib/puppet/type/consul_policy.rb

@@ -21,15 +21,27 @@
     defaultto ''
   end
 
-  newparam(:description) do
+  newproperty(:description) do
     desc 'Description of the policy'
     validate do |value|
       raise ArgumentError, "Description must be a string" if not value.is_a?(String)
     end
   end
 
-  newparam(:rules) do
+  newproperty(:rules, :array_matching => :all) do
     desc 'List of ACL rules for this policy'
+    validate do |value|
+      raise ArgumentError, "Policy rule must be a hash" unless value.is_a?(Hash)
+
+      raise ArgumentError, "Policy rule needs to specify a resource" unless value.key?('resource')
+      raise ArgumentError, "Policy rule needs to specify a segment" unless value.key?('segment')
+      raise ArgumentError, "Policy rule needs to specify a disposition" unless value.key?('disposition')
+
+      raise ArgumentError, "Policy rule resource must be a string" unless value['resource'].is_a?(String)
+      raise ArgumentError, "Policy rule segment must be a string" unless value['segment'].is_a?(String)
+      raise ArgumentError, "Policy rule disposition must be a string" unless value['disposition'].is_a?(String)
+    end
+
     defaultto []
   end
 

lib/puppet/type/consul_token.rb

@@ -28,19 +28,19 @@
     defaultto ''
   end
 
-  newparam(:policies_by_name) do
+  newproperty(:policies_by_name, :array_matching => :all) do
     desc 'List of policy names assigned to the token'
     validate do |value|
-      raise ArgumentError, "Policy name list must be an array" if not value.is_a?(Array)
+      raise ArgumentError, "Policy name list must be an array of strings" if not value.is_a?(String)
     end
 
     defaultto []
   end
 
-  newparam(:policies_by_id) do
+  newproperty(:policies_by_id, :array_matching => :all) do
     desc 'List of policy IDs assigned to the token'
     validate do |value|
-      raise ArgumentError, "Policy ID list must be an array" if not value.is_a?(Array)
+      raise ArgumentError, "Policy ID list must be an array of strings" if not value.is_a?(String)
     end
 
     defaultto []

spec/acceptance/class_spec.rb

@@ -188,6 +188,11 @@ class { 'consul':
             'test_token_xyz' => {
               'accessor_id'      => '7c4e3f11-786d-44e6-ac1d-b99546a1ccbd',
               'policies_by_name' => ['test_policy_abc']
+            },
+            'test_token_absent' => {
+              'accessor_id'      => '10381ad3-2837-43a6-b1ea-e27b7d53a749',
+              'policies_by_name' => ['test_policy_abc'],
+              'ensure'           => 'absent'
             }
           },
           policies => {
@@ -198,14 +203,21 @@ class { 'consul':
                 {'resource' => 'key', 'segment' => 'test_key', 'disposition' => 'write'},
                 {'resource' => 'node_prefix', 'segment' => '', 'disposition' => 'deny'},
               ],
+            },
+            'test_policy_absent' => {
+              'description' => "This policy should not exists",
+              'rules'       => [
+                {'resource' => 'service_prefix', 'segment' => 'test_segment', 'disposition' => 'read'}
+              ],
+              'ensure'      => 'absent'
             }
           }
         }
       EOS
 
       # Run it twice to test for idempotency
       apply_manifest(pp, catch_failures: true)
-      apply_manifest(pp, catch_changes: false)
+      apply_manifest(pp, catch_changes: true)
     end
 
     describe file('/opt/consul') do

spec/unit/puppet/type/consul_policy_spec.rb

@@ -4,7 +4,7 @@
 
   it 'should fail if no name is provided' do
     expect do
-      Puppet::Type.type(:consul_policy).new(:rules => {})
+      Puppet::Type.type(:consul_policy).new(:id => {}, :rules => {})
     end.to raise_error(Puppet::Error, /Title or name must be provided/)
   end
 
@@ -20,17 +20,127 @@
     end.to raise_error(Puppet::Error, /Description must be a string/)
   end
 
+  it 'should fail if rules is not a hash' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => 'abc'
+      )
+    end.to raise_error(Puppet::Error, /Policy rule must be a hash/)
+  end
+
+  it 'should fail if rule resource is missing' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'segment'     => 'test_service',
+                  'disposition' => 'read'
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule needs to specify a resource/)
+  end
+
+  it 'should fail if rule segment is missing' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'resource'    =>  'service_prefix',
+                  'disposition' => 'read'
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule needs to specify a segment/)
+  end
+
+  it 'should fail if rule disposition is missing' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'resource'    =>  'service_prefix',
+                  'segment'     => 'test_service',
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule needs to specify a disposition/)
+  end
+
+  it 'should fail if rule resource is not a string' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'resource'    => [],
+                  'segment'     => 'test_service',
+                  'disposition' => 'read'
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule resource must be a string/)
+  end
+
+  it 'should fail if rule segment is not a string' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'resource'    => 'key_prefix',
+                  'segment'     => [],
+                  'disposition' => 'read'
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule segment must be a string/)
+  end
+
+  it 'should fail if rule disposition is not a string' do
+    expect do
+      Puppet::Type.type(:consul_policy).new(
+          :name         => 'testing',
+          :id           => '39c75e12-7f43-0a40-dfba-9aa3fcda08d4',
+          :description  => 'test description',
+          :rules        => [
+              {
+                  'resource'    => 'key_prefix',
+                  'segment'     => 'test_service',
+                  'disposition' => []
+              }
+          ]
+      )
+    end.to raise_error(Puppet::Error, /Policy rule disposition must be a string/)
+  end
+
   context 'with name defined' do
     rules = [
         {
-            :resource    =>  'service_prefix',
-            :segment     => 'test_service',
-            :disposition => 'read'
+            'resource'    =>  'service_prefix',
+            'segment'     => 'test_service',
+            'disposition' => 'read'
         },
         {
-            :resource    =>  'key_prefix',
-            :segment     => 'key',
-            :disposition => 'write'
+            'resource'    =>  'key_prefix',
+            'segment'     => 'key',
+            'disposition' => 'write'
         }
     ]
 

spec/unit/puppet/type/consul_token_spec.rb

@@ -22,14 +22,14 @@
 
   it 'should fail if policy name list is not an array' do
     expect do
-      Puppet::Type.type(:consul_token).new(:name => 'foo', :policies_by_name => 'abc')
-    end.to raise_error(Puppet::Error, /Policy name list must be an array/)
+      Puppet::Type.type(:consul_token).new(:name => 'foo', :policies_by_name => [[]])
+    end.to raise_error(Puppet::Error, /Policy name list must be an array of strings/)
   end
 
   it 'should fail if policy ID list is not an array' do
     expect do
-      Puppet::Type.type(:consul_token).new(:name => 'foo', :policies_by_id => 'abc')
-    end.to raise_error(Puppet::Error, /Policy ID list must be an array/)
+      Puppet::Type.type(:consul_token).new(:name => 'foo', :policies_by_id => [[]])
+    end.to raise_error(Puppet::Error, /Policy ID list must be an array of strings/)
   end
 
   context 'with name defined' do

types/policystruct.pp

@@ -1,5 +1,6 @@
 type Consul::PolicyStruct = Struct[{
   id            => Optional[String[1]],
+  ensure        => Optional[Enum['present', 'absent']],
   description   => Optional[String[0]],
   rules         => Optional[Array[Struct[{
     resource    => String[1],

types/tokenstruct.pp

@@ -1,5 +1,6 @@
 type Consul::TokenStruct = Struct[{
   accessor_id      => String[1],
+  ensure           => Optional[Enum['present', 'absent']],
   secret_id        => Optional[String[1]],
   policies_by_name => Optional[Array[String]],
   policies_by_id   => Optional[Array[String]],