Support for Consul 1.4.0+ ACL system #474

marius-meissner · 2019-04-08T08:16:15Z

Resolves #471

Example usage:

consul_policy {'test_policy':
  description   => 'test description',
  rules         => [
      {
          'resource'    => 'service_prefix',
          'segment'     => 'test_service',
          'disposition' => 'read'
      },
      {
          'resource'    => 'key',
          'segment'     => 'test_key',
          'disposition' => 'write'
      },
  ],
  acl_api_token => 'e33653a6-0320-4a71-b3af-75f14578e3aa',
}

consul_token {'test_token':
  policies_by_name  => [
   'test_policy'
  ],
  policies_by_id    => [
    '652f27c9-d08d-412b-8985-9becc9c42fb2'
  ],
}

The current concept treats tokens and policies individually. A token can be assigned both policies managed by puppet (by_name) and external policies (by_id).
The new Consul system does not support any user specified Token ID anymore (s. hashicorp/consul#4977), therefore the mapping is currently done by description.

Due to my limited Ruby or Puppet experience feedback and suggestions for improvements are highly welcome.

bastelfreak · 2019-04-08T21:49:17Z

README.md

@@ -199,6 +199,45 @@ consul to restart.

 ## ACL Definitions

+### Policy/Token system
+
+Starting with version 1.4.0, a new ACL system was introduces separating rules (policies) from tokens.


yey doc updates \o/

lib/puppet/provider/consul_policy/default.rb

bastelfreak · 2019-04-08T21:50:14Z

lib/puppet/provider/consul_policy/default.rb

@@ -0,0 +1,190 @@
+require 'json'
+require 'net/http'
+require 'pp'


Is pp actually required or just a leftover from debugging?

You are right, dependency has been removed.

bastelfreak · 2019-04-08T21:51:32Z

manifests/init.pp

@@ -158,6 +179,13 @@
 #
 class consul (
  Hash $acls                                 = $consul::params::acls,
+  Hash $tokens                               = $consul::params::tokens,


can you enforce the datatypes even more? For hashes, you can define the datatypes for the keys and the values. For example:

Suggested change

Hash $tokens = $consul::params::tokens,

Hash[String[1],String[1]] $tokens = $consul::params::tokens,

Good idea. I tried to add some stricter types in the newest version.

bastelfreak · 2019-04-08T21:52:17Z

manifests/init.pp

@@ -158,6 +179,13 @@
 #
 class consul (
  Hash $acls                                 = $consul::params::acls,
+  Hash $tokens                               = $consul::params::tokens,
+  Hash $policies                             = $consul::params::policies,
+  String $acl_api_hostname                   = $consul::params::acl_api_hostname,


You can enforce the minimal string length like this:

Suggested change

String $acl_api_hostname = $consul::params::acl_api_hostname,

String[1] $acl_api_hostname = $consul::params::acl_api_hostname,

Please also look through https://github.com/puppetlabs/puppetlabs-stdlib/tree/master/types, you might find mor strict datatypes that you could use.

bastelfreak · 2019-04-08T21:53:04Z

manifests/init.pp

+  Hash $policies                             = $consul::params::policies,
+  String $acl_api_hostname                   = $consul::params::acl_api_hostname,
+  String $acl_api_protocol                   = $consul::params::acl_api_protocol,
+  Integer $acl_api_port                      = $consul::params::acl_api_port,


Probably this type can be used https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/port.pp ?

Suggested change

Integer $acl_api_port = $consul::params::acl_api_port,

Stdlib::Port $acl_api_port = $consul::params::acl_api_port,

Good idea from my point of view.
As far as I know this type was added in version 4.25 of stdlib. Is it ok to adjust the current dependencies accordingly?

bastelfreak · 2019-04-08T21:55:07Z

Hey @SkydiveMarius, thanks for the awesome PR! I made some small inline comments. Can you take a look at the current acceptance tests (https://github.com/solarkennedy/puppet-consul/blob/master/spec/acceptance/class_spec.rb) and extend them with tests for the new types/provider?

bastelfreak · 2019-04-10T13:40:36Z

@SkydiveMarius thanks for the updates! Can you take a look at the failed travis job?

…ing CentOS6 Travis jobs

marius-meissner · 2019-04-12T14:12:41Z

Branch has been rebased, but some Travis builds are randomly failing with Timeout errors (ACL API requests). So far I was not able to reproduce this behavior locally.

In case of failure the content of /var/log/consul would probably help. Does rspec offer possibilities to display this information in case of an error?

…imprecise tests

marius-meissner · 2019-05-10T14:45:59Z

PR has been adjusted for the recent Consul 1.5.0 update.
As the module is now using the new accessor + secret ID predefinition feature, only Consul > 1.5.0 is supported.

bastelfreak · 2019-05-11T10:35:29Z

manifests/init.pp

-  Boolean                        $enable_beta_ui               = false,
-  Boolean                        $allow_binding_to_root_ports  = false,
+  Hash                           $acls                        = {},
+  Hash[String[1], Struct[{


can you please put those types into a custom datatype? That would make the file more readable.

Good idea. Replaced by type aliases.

bastelfreak · 2019-05-14T21:13:20Z

Hi @SkydiveMarius, thanks for the awesome work! @solarkennedy can you please take a look and merge it? In my opinion this is fine.

solarkennedy · 2019-05-15T04:37:53Z

looking...

solarkennedy · 2019-05-15T05:06:15Z

lib/puppet/provider/consul_policy/default.rb

+    if @resource[:ensure] == :absent
+      if @existing_policy
+        @client.delete_policy(@existing_policy.id)
+        Puppet.notice(" Deleted Consul ACL policy #{@existing_policy.name} (ID: #{@existing_policy.id})")


nit: There is a leading space here in the log line

Thx, space has been removed.

solarkennedy · 2019-05-15T05:06:28Z

lib/puppet/provider/consul_policy/default.rb

+      @existing_policy.rules = @rules_encoded
+
+      @client.update_policy(@existing_policy)
+      Puppet.notice(" Updated Consul ACL policy #{@existing_policy.name} (ID: #{@existing_policy.id})")


here too, intentional?

By far not intended, space is removed.

solarkennedy · 2019-05-15T05:23:25Z

This PR looks really solid @SkydiveMarius!
Tests, types, providers, etc.
It has been a long time since I wrote puppet code like this, but I'm a fan of getting the code out there and iterating.

So I say we ship!

marius-meissner · 2019-05-15T10:40:38Z

Thanks for the positive feedback!
Seems the current Travis fails have an unrelated reason:

curl: (22) The requested URL returned error: 404 Not Found
  	error: skipping http://yum.puppet.com/puppet6/puppet6-release-el-6.noarch.rpm - transfer failed
  	Retrieving http://yum.puppet.com/puppet6/puppet6-release-el-6.noarch.rpm

solarkennedy · 2019-05-15T14:42:18Z

Probably temporary. It really is 404, but I'll check back later in the day to make sure we have a green build and merge.

bastelfreak · 2019-05-15T14:43:43Z

I restarted the failed CI jobs

bastelfreak · 2019-05-15T18:51:17Z

The related issue for the broken mirror: https://groups.google.com/forum/#!topic/puppet-users/cCsGWKunBe4

solarkennedy · 2019-05-16T15:07:34Z

Build is green. Thanks again @SkydiveMarius for a really great first time contribution!

Support for Consul 1.4.0+ ACL system

bastelfreak added the enhancement New feature or request label Apr 8, 2019

bastelfreak reviewed Apr 8, 2019

View reviewed changes

lib/puppet/provider/consul_policy/default.rb Outdated Show resolved Hide resolved

bastelfreak reviewed Apr 8, 2019

View reviewed changes

bastelfreak added needs-tests needs-work not ready to merge just yet labels Apr 8, 2019

bastelfreak added the needs-rebase label Apr 11, 2019

marius-meissner added 7 commits April 12, 2019 12:05

acl_v2: Implemented new ACL system

39a7b29

acl_v2: Removed obsolete dependency + added new line

70e2cbb

acl_v2: Using stricter data types

324127a

acl_v2: Adjusted some length constraints

591e5bb

acl_v2: Added acceptance tests

f6021c9

acl_v2: dummy commit for restarting Travis

a14befa

acl_v2: Fixed wrong IP address in acceptance tests, which caused fail…

5493a1a

…ing CentOS6 Travis jobs

marius-meissner force-pushed the acl_v2 branch from 26420dc to 5493a1a Compare April 12, 2019 10:26

acl_v2: Removed obsolete empty line

83a2570

marius-meissner added 5 commits April 15, 2019 12:13

acl_v2: Using single reference/variable for ACL master token

9b7e7f8

acl_v2: Fixed broken update logic for token/policy relations + fixed …

4b2512c

…imprecise tests

acl_v2: Added support for Consul 1.5.0 and dropped support for 1.4.2

e9aece1

acl_v2: Adjusted README

6462fcc

acl_v2: Adjusted manifest types

7299a87

bastelfreak reviewed May 11, 2019

View reviewed changes

acl_v2: Addey type aliases for tokens and policies

4af8f2c

bastelfreak removed needs-rebase needs-tests needs-work not ready to merge just yet labels May 14, 2019

solarkennedy reviewed May 15, 2019

View reviewed changes

acl_v2: Removed leading space in notice message

fd9b5dd

acl_v2: Fixed invalid reference on policy.id property

87ecd34

solarkennedy merged commit 3d42a86 into voxpupuli:master May 16, 2019

NFarrington mentioned this pull request May 20, 2019

New 1.4+ ACL support is not idempotent #479

Closed

spuder pushed a commit to spuder/puppet-consul that referenced this pull request Feb 25, 2020

Merge pull request voxpupuli#474 from SkydiveMarius/acl_v2

84a8954

Support for Consul 1.4.0+ ACL system

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Consul 1.4.0+ ACL system #474

Support for Consul 1.4.0+ ACL system #474

marius-meissner commented Apr 8, 2019

bastelfreak Apr 8, 2019

bastelfreak Apr 8, 2019

marius-meissner Apr 10, 2019

bastelfreak Apr 8, 2019

marius-meissner Apr 10, 2019

bastelfreak Apr 8, 2019

bastelfreak Apr 8, 2019

marius-meissner Apr 10, 2019

bastelfreak commented Apr 8, 2019

bastelfreak commented Apr 10, 2019

marius-meissner commented Apr 12, 2019

marius-meissner commented May 10, 2019

bastelfreak May 11, 2019

marius-meissner May 13, 2019

bastelfreak commented May 14, 2019

solarkennedy commented May 15, 2019

solarkennedy May 15, 2019

marius-meissner May 15, 2019

solarkennedy May 15, 2019

marius-meissner May 15, 2019

solarkennedy commented May 15, 2019

marius-meissner commented May 15, 2019

solarkennedy commented May 15, 2019

bastelfreak commented May 15, 2019

bastelfreak commented May 15, 2019

solarkennedy commented May 16, 2019

	Hash $tokens = $consul::params::tokens,
	Hash[String[1],String[1]] $tokens = $consul::params::tokens,

	String $acl_api_hostname = $consul::params::acl_api_hostname,
	String[1] $acl_api_hostname = $consul::params::acl_api_hostname,

	Integer $acl_api_port = $consul::params::acl_api_port,
	Stdlib::Port $acl_api_port = $consul::params::acl_api_port,

Support for Consul 1.4.0+ ACL system #474

Support for Consul 1.4.0+ ACL system #474

Conversation

marius-meissner commented Apr 8, 2019

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

Choose a reason for hiding this comment

bastelfreak commented Apr 8, 2019

bastelfreak commented Apr 10, 2019

marius-meissner commented Apr 12, 2019

marius-meissner commented May 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bastelfreak commented May 14, 2019

solarkennedy commented May 15, 2019

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

solarkennedy commented May 15, 2019

marius-meissner commented May 15, 2019

solarkennedy commented May 15, 2019

bastelfreak commented May 15, 2019

bastelfreak commented May 15, 2019

solarkennedy commented May 16, 2019