Adds support for all Consul 0.8 ACL rule scopes (#25800)

* Added in support for 'agent' and 'node' types.

* Tidies and moves `consul_acl` module closer to PEP8 compliance.

* Switched from using byspoke code to handle py2/3 string issues to using `to_text`.

* Made changes suggested by jrandall in https://github.com/ansible/ansible/pull/23467#pullrequestreview-34021967.

* Refactored consul_acl to support scopes with no pattern (and therefore a different HCL defintion).

* Corrects whitespace in Consul ACL HCL representation.

* Fixes Consul ACL to return the HCL equivalent JSON (according to the Consul docs) for the set ACLs.

* Repositioned import to align with Ansible standard (!= PEP8 standard).

* Adds Python 2.6 compatibility.

* Fixes PEP8 issues.

* Removes consul_acl.py as it now passes PEP8.

* Follows advice in the "Documenting Your Module" guide and moves imports up from the bottom.

* Tidies consul_acl module documentation.

* Updates link to guide about Consul ACLs.

* Removes new line spaces from error message string.

* Provide better error message if user forgets to associate a value to a Consul ACL rule.

* Minor refactoring of Consul ACL module.

* Fixes bug that was breaking idempotence in Consul ACL module.

* Detects redefinition of same rule.

* Adds test to check the Consul ACL module can set rules for all supported scopes.

* Fixes return when updating an ACL.

* Clean up of Consul ACL integration test file.

* Verify correct changes to existing Consul ACL rule.

* Adds tests for idempotence.

* Splits Consul ACL tests into cohesive modules.

* Adds test for deleting Consul ACLs.

* Test that Consul ACL module can set all rule scopes.

* Fixes issues surrounding the creation of ACLs.

Thanks for the comments by manos in https://github.com/ansible/ansible/pull/25800#issuecomment-310137889.

* Stops Consul ACL's name being "forgotten" if ACL updated by token.

* Fixes incorrect assignment when a Consul ACL is deleted.

* Fixes value of `changed` when Consul ACL is removed.

* Fixes tests for Consul ACL.

* Adds interal documentation.

* Refactors to separate update and create (also makes it possible to unit test this module).

* Improves documentation.

* Completes RETURN documentation for Consul ACL module.

* Fixes issue with equality checking for `None` in ACL Consul.

* Fixes Python 2 issue with making a decision based on `str` type.

* Fixes inequality check bug in Python 2.

* Adds tests for setting ACL with token.

* Adds support for creating an ACL with a given token.

* Outputs operation performed on Consul ACL when changed.

* Fixs issue with test for creating a Consul ACL with rules.

* Corrects property used to set ACL token in python-consul library.

* Fixes tear-down issue in test that creates a Consul ACL using a token.

lib/ansible/modules/clustering/consul_acl.py

@@ -11,21 +11,17 @@ ANSIBLE_METADATA = {'metadata_version': '1.0',
                     'status': ['preview'],
                     'supported_by': 'community'}
 
-
 DOCUMENTATION = """
 module: consul_acl
-short_description: "manipulate consul acl keys and rules"
+short_description: Manipulate Consul ACL keys and rules
 description:
- - allows the addition, modification and deletion of ACL keys and associated
+ - Allows the addition, modification and deletion of ACL keys and associated
    rules in a consul cluster via the agent. For more details on using and
-   configuring ACLs, see https://www.consul.io/docs/internals/acl.html.
-requirements:
-  - "python >= 2.6"
-  - python-consul
-  - pyhcl
-  - requests
+   configuring ACLs, see https://www.consul.io/docs/guides/acl.html.
 version_added: "2.0"
-author: "Steve Gargan (@sgargan)"
+author:
+  - Steve Gargan (@sgargan)
+  - Colin Nolan (@colin-nolan)
 options:
   mgmt_token:
     description:
@@ -38,8 +34,7 @@ options:
     default: present
   token_type:
     description:
-          - the type of token that should be created, either management or
-            client
+      - the type of token that should be created, either management or client
     choices: ['client', 'management']
     default: client
   name:
@@ -50,11 +45,11 @@ options:
   token:
     description:
       - the token key indentifying an ACL rule set. If generated by consul
-            this will be a UUID.
+        this will be a UUID
     required: false
   rules:
     description:
-          - an list of the rules that should be associated with a given token.
+      - a list of the rules that should be associated with a given token
     required: false
   host:
     description:
@@ -78,44 +73,98 @@ options:
     required: false
     default: True
     version_added: "2.1"
+requirements:
+  - "python >= 2.6"
+  - python-consul
+  - pyhcl
+  - requests
 """
 
-EXAMPLES = '''
-    - name: create an acl token with rules
+EXAMPLES = """
+- name: create an ACL with rules
   consul_acl:
-        mgmt_token: 'some_management_acl'
-        host: 'consul1.mycluster.io'
-        name: 'Foo access'
+    host: consul1.example.com
+    mgmt_token: some_management_acl
+    name: Foo access
     rules:
-          - key: 'foo'
+      - key: "foo"
         policy: read
-          - key: 'private/foo'
+      - key: "private/foo"
         policy: deny
 
-    - name: create an acl with specific token with both key and service rules
+- name: create an ACL with a specific token
   consul_acl:
-        mgmt_token: 'some_management_acl'
-        name: 'Foo access'
-        token: 'some_client_token'
+    host: consul1.example.com
+    mgmt_token: some_management_acl
+    name: Foo access
+    token: my-token
     rules:
-          - key: 'foo'
+      - key: "foo"
         policy: read
-          - service: ''
+
+- name: update the rules associated to an ACL token
+  consul_acl:
+    host: consul1.example.com
+    mgmt_token: some_management_acl
+    name: Foo access
+    token: some_client_token
+    rules:
+      - event: "bbq"
         policy: write
-          - service: 'secret-'
+      - key: "foo"
+        policy: read
+      - key: "private"
         policy: deny
+      - keyring: write
+      - node: "hgs4"
+        policy: write
+      - operator: read
+      - query: ""
+        policy: write
+      - service: "consul"
+        policy: write
+      - session: "standup"
+        policy: write
 
 - name: remove a token
   consul_acl:
-        mgmt_token: 'some_management_acl'
-        host: 'consul1.mycluster.io'
-        token: '172bd5c8-9fe9-11e4-b1b0-3c15c2c9fd5e'
+    host: consul1.example.com
+    mgmt_token: some_management_acl
+    token: 172bd5c8-9fe9-11e4-b1b0-3c15c2c9fd5e
     state: absent
-'''
+"""
+
+RETURN = """
+token:
+    description: the token associated to the ACL (the ACL's ID)
+    returned: success
+    type: string
+    sample: a2ec332f-04cf-6fba-e8b8-acf62444d3da
+rules:
+    description: the HCL JSON representation of the rules associated to the ACL, in the format described in the
+                 Consul documentation (https://www.consul.io/docs/guides/acl.html#rule-specification).
+    returned: I(status) == "present"
+    type: string
+    sample: {
+        "key": {
+            "foo": {
+                "policy": "write"
+            },
+            "bar": {
+                "policy": "deny"
+            }
+        }
+    }
+operation:
+    description: the operation performed on the ACL
+    returned: changed
+    type: string
+    sample: update
+"""
+
 
 try:
     import consul
-    from requests.exceptions import ConnectionError
     python_consul_installed = True
 except ImportError:
     python_consul_installed = False
@@ -126,225 +175,467 @@ try:
 except ImportError:
     pyhcl_installed = False
 
-from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils._text import to_bytes
+from collections import defaultdict
+from requests.exceptions import ConnectionError
+from ansible.module_utils.basic import to_text, AnsibleModule
 
 
-def execute(module):
+RULE_SCOPES = ["agent", "event", "key", "keyring", "node", "operator", "query", "service", "session"]
 
-    state = module.params.get('state')
+MANAGEMENT_PARAMETER_NAME = "mgmt_token"
+HOST_PARAMETER_NAME = "host"
+SCHEME_PARAMETER_NAME = "scheme"
+VALIDATE_CERTS_PARAMETER_NAME = "validate_certs"
+NAME_PARAMETER_NAME = "name"
+PORT_PARAMETER_NAME = "port"
+RULES_PARAMETER_NAME = "rules"
+STATE_PARAMETER_NAME = "state"
+TOKEN_PARAMETER_NAME = "token"
+TOKEN_TYPE_PARAMETER_NAME = "token_type"
 
-    if state == 'present':
-        update_acl(module)
-    else:
-        remove_acl(module)
+PRESENT_STATE_VALUE = "present"
+ABSENT_STATE_VALUE = "absent"
 
+CLIENT_TOKEN_TYPE_VALUE = "client"
+MANAGEMENT_TOKEN_TYPE_VALUE = "management"
 
-def update_acl(module):
+REMOVE_OPERATION = "remove"
+UPDATE_OPERATION = "update"
+CREATE_OPERATION = "create"
 
-    rules = module.params.get('rules')
-    token = module.params.get('token')
-    token_type = module.params.get('token_type')
-    mgmt = module.params.get('mgmt_token')
-    name = module.params.get('name')
-    consul = get_consul_api(module, mgmt)
-    changed = False
+_POLICY_JSON_PROPERTY = "policy"
+_RULES_JSON_PROPERTY = "Rules"
+_TOKEN_JSON_PROPERTY = "ID"
+_TOKEN_TYPE_JSON_PROPERTY = "Type"
+_NAME_JSON_PROPERTY = "Name"
+_POLICY_YML_PROPERTY = "policy"
+_POLICY_HCL_PROPERTY = "policy"
 
-    try:
-
-        if token:
-            existing_rules = load_rules_for_token(module, consul, token)
-            supplied_rules = yml_to_rules(module, rules)
-            changed = not existing_rules == supplied_rules
-            if changed:
-                y = supplied_rules.to_hcl()
-                token = consul.acl.update(
-                    token,
-                    name=name,
-                    type=token_type,
-                    rules=supplied_rules.to_hcl())
-        else:
-            try:
-                rules = yml_to_rules(module, rules)
-                if rules.are_rules():
-                    rules = rules.to_hcl()
-                else:
-                    rules = None
-
-                token = consul.acl.create(
-                    name=name, type=token_type, rules=rules)
-                changed = True
-            except Exception as e:
-                module.fail_json(
-                    msg="No token returned, check your management key and that \
-                         the host is in the acl datacenter %s" % e)
-    except Exception as e:
-        module.fail_json(msg="Could not create/update acl %s" % e)
-
-    module.exit_json(changed=changed,
-                     token=token,
-                     rules=rules,
-                     name=name,
-                     type=token_type)
-
-
-def remove_acl(module):
-    token = module.params.get('token')
-    mgmt = module.params.get('mgmt_token')
-
-    consul = get_consul_api(module, token=mgmt)
-    changed = token and consul.acl.info(token)
-    if changed:
-        token = consul.acl.destroy(token)
-
-    module.exit_json(changed=changed, token=token)
-
-def load_rules_for_token(module, consul_api, token):
-    try:
-        rules = Rules()
-        info = consul_api.acl.info(token)
-        if info and info['Rules']:
-            rule_set = hcl.loads(to_bytes(info['Rules'], errors='ignore', nonstring='passthru'))
-            for rule_type in rule_set:
-                for pattern, policy in rule_set[rule_type].items():
-                    rules.add_rule(rule_type, Rule(pattern, policy['policy']))
-    except Exception as e:
-        module.fail_json(
-            msg="Could not load rule list from retrieved rule data %s, %s" % (
-                token, e))
-
-    return rules
-
-
-def yml_to_rules(module, yml_rules):
-    rules = Rules()
-    if yml_rules:
-        for rule in yml_rules:
-            if ('key' in rule and 'policy' in rule):
-                rules.add_rule('key', Rule(rule['key'], rule['policy']))
-            elif ('service' in rule and 'policy' in rule):
-                rules.add_rule('service', Rule(rule['service'], rule['policy']))
-            elif ('event' in rule and 'policy' in rule):
-                rules.add_rule('event', Rule(rule['event'], rule['policy']))
-            elif ('query' in rule and 'policy' in rule):
-                rules.add_rule('query', Rule(rule['query'], rule['policy']))
-            else:
-                module.fail_json(msg="a rule requires a key/service/event or query and a policy.")
-    return rules
-
-template = '''%s "%s" {
-  policy = "%s"
+_ARGUMENT_SPEC = {
+    MANAGEMENT_PARAMETER_NAME: dict(required=True, no_log=True),
+    HOST_PARAMETER_NAME: dict(default='localhost'),
+    SCHEME_PARAMETER_NAME: dict(required=False, default='http'),
+    VALIDATE_CERTS_PARAMETER_NAME: dict(required=False, type='bool', default=True),
+    NAME_PARAMETER_NAME: dict(required=False),
+    PORT_PARAMETER_NAME: dict(default=8500, type='int'),
+    RULES_PARAMETER_NAME: dict(default=None, required=False, type='list'),
+    STATE_PARAMETER_NAME: dict(default=PRESENT_STATE_VALUE, choices=[PRESENT_STATE_VALUE, ABSENT_STATE_VALUE]),
+    TOKEN_PARAMETER_NAME: dict(required=False),
+    TOKEN_TYPE_PARAMETER_NAME: dict(required=False, choices=[CLIENT_TOKEN_TYPE_VALUE, MANAGEMENT_TOKEN_TYPE_VALUE],
+                                    default=CLIENT_TOKEN_TYPE_VALUE)
 }
-'''
 
-RULE_TYPES = ['key', 'service', 'event', 'query']
 
-class Rules:
+def set_acl(consul_client, configuration):
+    """
+    Sets an ACL based on the given configuration.
+    :param consul_client: the consul client
+    :param configuration: the run configuration
+    :return: the output of setting the ACL
+    """
+    acls_as_json = decode_acls_as_json(consul_client.acl.list())
+    existing_acls_mapped_by_name = dict((acl.name, acl) for acl in acls_as_json if acl.name is not None)
+    existing_acls_mapped_by_token = dict((acl.token, acl) for acl in acls_as_json)
+    assert None not in existing_acls_mapped_by_token, "expecting ACL list to be associated to a token: %s" \
+                                                      % existing_acls_mapped_by_token[None]
 
+    if configuration.token is None and configuration.name and configuration.name in existing_acls_mapped_by_name:
+        # No token but name given so can get token from name
+        configuration.token = existing_acls_mapped_by_name[configuration.name].token
+
+    if configuration.token and configuration.token in existing_acls_mapped_by_token:
+        return update_acl(consul_client, configuration)
+    else:
+        assert configuration.token not in existing_acls_mapped_by_token
+        assert configuration.name not in existing_acls_mapped_by_name
+        return create_acl(consul_client, configuration)
+
+
+def update_acl(consul_client, configuration):
+    """
+    Updates an ACL.
+    :param consul_client: the consul client
+    :param configuration: the run configuration
+    :return: the output of the update
+    """
+    existing_acl = load_acl_with_token(consul_client, configuration.token)
+    changed = existing_acl.rules != configuration.rules
+
+    if changed:
+        name = configuration.name if configuration.name is not None else existing_acl.name
+        rules_as_hcl = encode_rules_as_hcl_string(configuration.rules)
+        updated_token = consul_client.acl.update(
+            configuration.token, name=name, type=configuration.token_type, rules=rules_as_hcl)
+        assert updated_token == configuration.token
+
+    return Output(changed=changed, token=configuration.token, rules=configuration.rules, operation=UPDATE_OPERATION)
+
+
+def create_acl(consul_client, configuration):
+    """
+    Creates an ACL.
+    :param consul_client: the consul client
+    :param configuration: the run configuration
+    :return: the output of the creation
+    """
+    rules_as_hcl = encode_rules_as_hcl_string(configuration.rules) if len(configuration.rules) > 0 else None
+    token = consul_client.acl.create(
+        name=configuration.name, type=configuration.token_type, rules=rules_as_hcl, acl_id=configuration.token)
+    rules = configuration.rules
+    return Output(changed=True, token=token, rules=rules, operation=CREATE_OPERATION)
+
+
+def remove_acl(consul, configuration):
+    """
+    Removes an ACL.
+    :param consul: the consul client
+    :param configuration: the run configuration
+    :return: the output of the removal
+    """
+    token = configuration.token
+    changed = consul.acl.info(token) is not None
+    if changed:
+        consul.acl.destroy(token)
+    return Output(changed=changed, token=token, operation=REMOVE_OPERATION)
+
+
+def load_acl_with_token(consul, token):
+    """
+    Loads the ACL with the given token (token == rule ID).
+    :param consul: the consul client
+    :param token: the ACL "token"/ID (not name)
+    :return: the ACL associated to the given token
+    :exception ConsulACLTokenNotFoundException: raised if the given token does not exist
+    """
+    acl_as_json = consul.acl.info(token)
+    if acl_as_json is None:
+        raise ConsulACLNotFoundException(token)
+    return decode_acl_as_json(acl_as_json)
+
+
+def encode_rules_as_hcl_string(rules):
+    """
+    Converts the given rules into the equivalent HCL (string) representation.
+    :param rules: the rules
+    :return: the equivalent HCL (string) representation of the rules. Will be None if there is no rules (see internal
+    note for justification)
+    """
+    if len(rules) == 0:
+        # Note: empty string is not valid HCL according to `hcl.load` however, the ACL `Rule` property will be an empty
+        # string if there is no rules...
+        return None
+    rules_as_hcl = ""
+    for rule in rules:
+        rules_as_hcl += encode_rule_as_hcl_string(rule)
+    return rules_as_hcl
+
+
+def encode_rule_as_hcl_string(rule):
+    """
+    Converts the given rule into the equivalent HCL (string) representation.
+    :param rule: the rule
+    :return: the equivalent HCL (string) representation of the rule
+    """
+    if rule.pattern is not None:
+        return '%s "%s" {\n  %s = "%s"\n}\n' % (rule.scope, rule.pattern, _POLICY_HCL_PROPERTY, rule.policy)
+    else:
+        return '%s = "%s"\n' % (rule.scope, rule.policy)
+
+
+def decode_rules_as_hcl_string(rules_as_hcl):
+    """
+    Converts the given HCL (string) representation of rules into a list of rule domain models.
+    :param rules_as_hcl: the HCL (string) representation of a collection of rules
+    :return: the equivalent domain model to the given rules
+    """
+    rules_as_hcl = to_text(rules_as_hcl)
+    rules_as_json = hcl.loads(rules_as_hcl)
+    return decode_rules_as_json(rules_as_json)
+
+
+def decode_rules_as_json(rules_as_json):
+    """
+    Converts the given JSON representation of rules into a list of rule domain models.
+    :param rules_as_json: the JSON representation of a collection of rules
+    :return: the equivalent domain model to the given rules
+    """
+    rules = RuleCollection()
+    for scope in rules_as_json:
+        if not isinstance(rules_as_json[scope], dict):
+            rules.add(Rule(scope, rules_as_json[scope]))
+        else:
+            for pattern, policy in rules_as_json[scope].items():
+                rules.add(Rule(scope, policy[_POLICY_JSON_PROPERTY], pattern))
+    return rules
+
+
+def encode_rules_as_json(rules):
+    """
+    Converts the given rules into the equivalent JSON representation according to the documentation:
+    https://www.consul.io/docs/guides/acl.html#rule-specification.
+    :param rules: the rules
+    :return: JSON representation of the given rules
+    """
+    rules_as_json = defaultdict(dict)
+    for rule in rules:
+        if rule.pattern is not None:
+            assert rule.pattern not in rules_as_json[rule.scope]
+            rules_as_json[rule.scope][rule.pattern] = {
+                _POLICY_JSON_PROPERTY: rule.policy
+            }
+        else:
+            assert rule.scope not in rules_as_json
+            rules_as_json[rule.scope] = rule.policy
+    return rules_as_json
+
+
+def decode_rules_as_yml(rules_as_yml):
+    """
+    Converts the given YAML representation of rules into a list of rule domain models.
+    :param rules_as_yml: the YAML representation of a collection of rules
+    :return: the equivalent domain model to the given rules
+    """
+    rules = RuleCollection()
+    if rules_as_yml:
+        for rule_as_yml in rules_as_yml:
+            rule_added = False
+            for scope in RULE_SCOPES:
+                if scope in rule_as_yml:
+                    if rule_as_yml[scope] is None:
+                        raise ValueError("Rule for '%s' does not have a value associated to the scope" % scope)
+                    policy = rule_as_yml[_POLICY_YML_PROPERTY] if _POLICY_YML_PROPERTY in rule_as_yml \
+                        else rule_as_yml[scope]
+                    pattern = rule_as_yml[scope] if _POLICY_YML_PROPERTY in rule_as_yml else None
+                    rules.add(Rule(scope, policy, pattern))
+                    rule_added = True
+                    break
+            if not rule_added:
+                raise ValueError("A rule requires one of %s and a policy." % ('/'.join(RULE_SCOPES)))
+    return rules
+
+
+def decode_acl_as_json(acl_as_json):
+    """
+    Converts the given JSON representation of an ACL into the equivalent domain model.
+    :param acl_as_json: the JSON representation of an ACL
+    :return: the equivalent domain model to the given ACL
+    """
+    rules_as_hcl = acl_as_json[_RULES_JSON_PROPERTY]
+    rules = decode_rules_as_hcl_string(acl_as_json[_RULES_JSON_PROPERTY]) if rules_as_hcl.strip() != "" \
+        else RuleCollection()
+    return ACL(
+        rules=rules,
+        token_type=acl_as_json[_TOKEN_TYPE_JSON_PROPERTY],
+        token=acl_as_json[_TOKEN_JSON_PROPERTY],
+        name=acl_as_json[_NAME_JSON_PROPERTY]
+    )
+
+
+def decode_acls_as_json(acls_as_json):
+    """
+    Converts the given JSON representation of ACLs into a list of ACL domain models.
+    :param acls_as_json: the JSON representation of a collection of ACLs
+    :return: list of equivalent domain models for the given ACLs (order not guaranteed to be the same)
+    """
+    return [decode_acl_as_json(acl_as_json) for acl_as_json in acls_as_json]
+
+
+class ConsulACLNotFoundException(Exception):
+    """
+    Exception raised if an ACL with is not found.
+    """
+
+
+class Configuration:
+    """
+    Configuration for this module.
+    """
+    def __init__(self, management_token=None, host=None, scheme=None, validate_certs=None, name=None, port=None,
+                 rules=None, state=None, token=None, token_type=None):
+        self.management_token = management_token    # type: str
+        self.host = host    # type: str
+        self.scheme = scheme    # type: str
+        self.validate_certs = validate_certs    # type: bool
+        self.name = name    # type: str
+        self.port = port    # type: bool
+        self.rules = rules    # type: RuleCollection
+        self.state = state    # type: str
+        self.token = token    # type: str
+        self.token_type = token_type    # type: str
+
+
+class Output:
+    """
+    Output of an action of this module.
+    """
+    def __init__(self, changed=None, token=None, rules=None, operation=None):
+        self.changed = changed  # type: bool
+        self.token = token  # type: str
+        self.rules = rules  # type: RuleCollection
+        self.operation = operation  # type: str
+
+
+class ACL:
+    """
+    Consul ACL. See: https://www.consul.io/docs/guides/acl.html.
+    """
+    def __init__(self, rules, token_type, token, name):
+        self.rules = rules
+        self.token_type = token_type
+        self.token = token
+        self.name = name
+
+    def __eq__(self, other):
+        return other \
+            and isinstance(other, self.__class__) \
+            and self.rules == other.rules \
+            and self.token_type == other.token_type \
+            and self.token == other.token \
+            and self.name == other.name
+
+    def __hash__(self):
+        return hash(self.rules) ^ hash(self.token_type) ^ hash(self.token) ^ hash(self.name)
+
+
+class Rule:
+    """
+    ACL rule. See: https://www.consul.io/docs/guides/acl.html#acl-rules-and-scope.
+    """
+    def __init__(self, scope, policy, pattern=None):
+        self.scope = scope
+        self.policy = policy
+        self.pattern = pattern
+
+    def __eq__(self, other):
+        return isinstance(other, self.__class__) \
+            and self.scope == other.scope \
+            and self.policy == other.policy \
+            and self.pattern == other.pattern
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
+    def __hash__(self):
+        return (hash(self.scope) ^ hash(self.policy)) ^ hash(self.pattern)
+
+    def __str__(self):
+        return encode_rule_as_hcl_string(self)
+
+
+class RuleCollection:
+    """
+    Collection of ACL rules, which are part of a Consul ACL.
+    """
     def __init__(self):
-        self.rules = {}
-        for rule_type in RULE_TYPES:
-            self.rules[rule_type] = {}
+        self._rules = {}
+        for scope in RULE_SCOPES:
+            self._rules[scope] = {}
 
-    def add_rule(self, rule_type, rule):
-        self.rules[rule_type][rule.pattern] = rule
-
-    def are_rules(self):
-        return len(self) > 0
-
-    def to_hcl(self):
-
-        rules = ""
-        for rule_type in RULE_TYPES:
-            for pattern, rule in self.rules[rule_type].items():
-                rules += template % (rule_type, pattern, rule.policy)
-        return to_bytes(rules, errors='ignore', nonstring='passthru')
+    def __iter__(self):
+        all_rules = []
+        for scope, pattern_keyed_rules in self._rules.items():
+            for pattern, rule in pattern_keyed_rules.items():
+                all_rules.append(rule)
+        return iter(all_rules)
 
     def __len__(self):
         count = 0
-        for rule_type in RULE_TYPES:
-            count += len(self.rules[rule_type])
+        for scope in RULE_SCOPES:
+            count += len(self._rules[scope])
         return count
 
     def __eq__(self, other):
-        if not (other or isinstance(other, self.__class__)
-                or len(other) == len(self)):
-            return False
+        return isinstance(other, self.__class__) \
+            and set(self) == set(other)
 
-        for rule_type in RULE_TYPES:
-            for name, other_rule in other.rules[rule_type].items():
-                if not name in self.rules[rule_type]:
-                    return False
-                rule = self.rules[rule_type][name]
-
-                if not (rule and rule == other_rule):
-                    return False
-        return True
+    def __ne__(self, other):
+        return not self.__eq__(other)
 
     def __str__(self):
-        return self.to_hcl()
+        return encode_rules_as_hcl_string(self)
 
-class Rule:
+    def add(self, rule):
+        """
+        Adds the given rule to this collection.
+        :param rule: model of a rule
+        :raises ValueError: raised if there already exists a rule for a given scope and pattern
+        """
+        if rule.pattern in self._rules[rule.scope]:
+            patten_info = " and pattern '%s'" % rule.pattern if rule.pattern is not None else ""
+            raise ValueError("Duplicate rule for scope '%s'%s" % (rule.scope, patten_info))
+        self._rules[rule.scope][rule.pattern] = rule
 
-    def __init__(self, pattern, policy):
-        self.pattern = pattern
-        self.policy = policy
 
-    def __eq__(self, other):
-        return (isinstance(other, self.__class__)
-                and self.pattern == other.pattern
-                and self.policy == other.policy)
+def get_consul_client(configuration):
+    """
+    Gets a Consul client for the given configuration.
 
-    def __hash__(self):
-        return hash(self.pattern) ^ hash(self.policy)
+    Does not check if the Consul client can connect.
+    :param configuration: the run configuration
+    :return: Consul client
+    """
+    token = configuration.management_token
+    if token is None:
+        token = configuration.token
+    assert token is not None, "Expecting the management token to always be set"
+    return consul.Consul(host=configuration.host, port=configuration.port, scheme=configuration.scheme,
+                         verify=configuration.validate_certs, token=token)
 
-    def __str__(self):
-        return '%s %s' % (self.pattern, self.policy)
 
-def get_consul_api(module, token=None):
-    if not token:
-        token = module.params.get('token')
-    return consul.Consul(host=module.params.get('host'),
-                         port=module.params.get('port'),
-                         scheme=module.params.get('scheme'),
-                         verify=module.params.get('validate_certs'),
-                         token=token)
-
-def test_dependencies(module):
+def check_dependencies():
+    """
+    Checks that the required dependencies have been imported.
+    :exception ImportError: if it is detected that any of the required dependencies have not been iported
+    """
     if not python_consul_installed:
-        module.fail_json(msg="python-consul required for this module. "\
-              "see http://python-consul.readthedocs.org/en/latest/#installation")
+        raise ImportError("python-consul required for this module. "
+                          "See: http://python-consul.readthedocs.org/en/latest/#installation")
 
     if not pyhcl_installed:
-        module.fail_json( msg="pyhcl required for this module."\
-              " see https://pypi.python.org/pypi/pyhcl")
+        raise ImportError("pyhcl required for this module. "
+                          "See: https://pypi.python.org/pypi/pyhcl")
+
 
 def main():
-    argument_spec = dict(
-        mgmt_token=dict(required=True, no_log=True),
-        host=dict(default='localhost'),
-        scheme=dict(required=False, default='http'),
-        validate_certs=dict(required=False, type='bool', default=True),
-        name=dict(required=False),
-        port=dict(default=8500, type='int'),
-        rules=dict(default=None, required=False, type='list'),
-        state=dict(default='present', choices=['present', 'absent']),
-        token=dict(required=False, no_log=True),
-        token_type=dict(
-            required=False, choices=['client', 'management'], default='client')
-    )
-    module = AnsibleModule(argument_spec, supports_check_mode=False)
-
-    test_dependencies(module)
+    """
+    Main method.
+    """
+    module = AnsibleModule(_ARGUMENT_SPEC, supports_check_mode=False)
 
     try:
-        execute(module)
-    except ConnectionError as e:
-        module.fail_json(msg='Could not connect to consul agent at %s:%s, error was %s' % (
-            module.params.get('host'), module.params.get('port'), str(e)))
-    except Exception as e:
+        check_dependencies()
+    except ImportError as e:
         module.fail_json(msg=str(e))
 
+    configuration = Configuration(
+        management_token=module.params.get(MANAGEMENT_PARAMETER_NAME),
+        host=module.params.get(HOST_PARAMETER_NAME),
+        scheme=module.params.get(SCHEME_PARAMETER_NAME),
+        validate_certs=module.params.get(VALIDATE_CERTS_PARAMETER_NAME),
+        name=module.params.get(NAME_PARAMETER_NAME),
+        port=module.params.get(PORT_PARAMETER_NAME),
+        rules=decode_rules_as_yml(module.params.get(RULES_PARAMETER_NAME)),
+        state=module.params.get(STATE_PARAMETER_NAME),
+        token=module.params.get(TOKEN_PARAMETER_NAME),
+        token_type=module.params.get(TOKEN_TYPE_PARAMETER_NAME)
+    )
+    consul_client = get_consul_client(configuration)
 
-if __name__ == '__main__':
+    try:
+        if configuration.state == PRESENT_STATE_VALUE:
+            output = set_acl(consul_client, configuration)
+        else:
+            output = remove_acl(consul_client, configuration)
+    except ConnectionError as e:
+        module.fail_json(msg='Could not connect to consul agent at %s:%s, error was %s' % (
+            configuration.host, configuration.port, str(e)))
+        raise
+
+    return_values = dict(changed=output.changed, token=output.token, operation=output.operation)
+    if output.rules is not None:
+        return_values["rules"] = encode_rules_as_json(output.rules)
+    module.exit_json(**return_values)
+
+
+if __name__ == "__main__":
     main()

test/integration/consul.yml

@@ -16,7 +16,6 @@
       register: consul_running
 
   roles:
-
     - {role: test_consul_service,
        when: not consul_running.failed is defined}
 
@@ -30,7 +29,6 @@
        when: not consul_running.failed is defined}
 
   tasks:
-
     - name: setup services with passing check for consul inventory test
       consul:
         service_name: nginx
@@ -42,7 +40,6 @@
           - dev
           - master
 
-
     - name: setup failing service for inventory test
       consul:
         service_name: nginx
@@ -69,7 +66,6 @@
         rules:
           - key: ''
             policy: write
-      register: inventory_token
 
     - name: add metadata for the node through kv_store
       consul_kv: "key=ansible/metadata/dc1/consul-1 value='{{metadata_json}}'"

test/integration/roles/test_consul_acl/tasks/create-acl-with-rules.yml

@@ -0,0 +1,77 @@
+---
+
+- name: create an ACL with rules
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+    rules:
+      - event: "bbq"
+        policy: write
+      - key: "foo"
+        policy: read
+      - key: "private"
+        policy: deny
+      - keyring: write
+      - node: "hgs4"
+        policy: write
+      - operator: read
+      - query: ""
+        policy: write
+      - service: "consul"
+        policy: write
+      - session: "standup"
+        policy: write
+  register: created_acl
+
+- name: verify created ACL's rules
+  assert:
+    that:
+      - created_acl.changed
+      - created_acl.operation == "create"
+      - created_acl.token | length == 36
+      - (created_acl.rules | json_query("event.bbq.policy")) == "write"
+      - (created_acl.rules | json_query("key.foo.policy")) == "read"
+      - (created_acl.rules | json_query("key.private.policy")) == "deny"
+      - (created_acl.rules | json_query("keyring")) == "write"
+      - (created_acl.rules | json_query("node.hgs4.policy")) == "write"
+      - (created_acl.rules | json_query("operator")) == "read"
+      - (created_acl.rules | json_query('query."".policy')) == "write"
+      - (created_acl.rules | json_query("service.consul.policy")) == "write"
+      - (created_acl.rules | json_query("session.standup.policy")) == "write"
+
+- name: create same ACL
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+    rules:
+      - event: "bbq"
+        policy: write
+      - key: "foo"
+        policy: read
+      - key: "private"
+        policy: deny
+      - keyring: write
+      - node: "hgs4"
+        policy: write
+      - operator: read
+      - query: ""
+        policy: write
+      - service: "consul"
+        policy: write
+      - session: "standup"
+        policy: write
+  register: doubly_created_acl
+
+- name: verify idempotence when creating ACL
+  assert:
+    that:
+      - not doubly_created_acl.changed
+
+- name: clean up
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ doubly_created_acl.token }}"
+    state: absent

test/integration/roles/test_consul_acl/tasks/create-acl-with-token.yml

@@ -0,0 +1,41 @@
+---
+
+- name: create an ACL with a given token
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+    token: "{{ test_consul_acl_token_id }}"
+    rules:
+      - key: "foo"
+        policy: write
+  register: created_acl
+
+- name: verify ACL created with given token
+  assert:
+    that:
+      - created_acl.changed
+      - created_acl.operation == "create"
+      - created_acl.token == test_consul_acl_token_id
+
+- name: re-create ACL with the token
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ test_consul_acl_token_id }}"
+    rules:
+      - key: "foo"
+        policy: write
+  register: doubly_created_acl
+
+- name: verify idempotence when creating ACL with same token
+  assert:
+    that:
+      - not doubly_created_acl.changed
+
+- name: clean up
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    state: absent

test/integration/roles/test_consul_acl/tasks/create-acl-without-rules.yml

@@ -0,0 +1,35 @@
+---
+
+- name: create a new ACL without rules
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+  register: created_ruleless_acl
+
+- name: verify ACL created without rules
+  assert:
+    that:
+      - created_ruleless_acl.changed
+      - created_ruleless_acl.operation == "create"
+      - created_ruleless_acl.token | length == 36
+      - created_ruleless_acl.rules == {}
+
+- name: create same rule-less ACL
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+  register: doubly_created_ruleless_acl
+
+- name: verify idempotence when creating ruleless ACL tokens
+  assert:
+    that:
+      - not doubly_created_ruleless_acl.changed
+
+- name: clean up
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ doubly_created_ruleless_acl.token }}"
+    state: absent

test/integration/roles/test_consul_acl/tasks/main.yml

@@ -1,42 +1,11 @@
-- name: create a new acl token
-  consul_acl:
-    mgmt_token: '{{mgmt_token}}'
-    host: '{{acl_host}}'
-    name: 'New ACL'
-  register: new_ruleless
+---
 
-- name: verify ruleless key created
-  assert:
-    that:
-      - new_ruleless.token | length == 36
-      - new_ruleless.name == 'New ACL'
+- import_tasks: create-acl-without-rules.yml
 
-- name: add rules to an acl token
-  consul_acl:
-    mgmt_token: '{{mgmt_token}}'
-    host: '{{acl_host}}'
-    name: 'With rule'
-    rules:
-      - key: 'foo'
-        policy: read
-      - key: 'private/foo'
-        policy: deny
-  register: with_rules
+- import_tasks: create-acl-with-rules.yml
 
-- name: verify rules created
-  assert:
-    that:
-      - with_rules.token | length == 36
-      - with_rules.name == 'With rule'
-      - with_rules.rules | match('.*"foo".*')
-      - with_rules.rules | search(pattern='private/foo')
+- import_tasks: create-acl-with-token.yml
 
-- name: clear up
-  consul_acl:
-    mgmt_token: '{{mgmt_token}}'
-    host: '{{acl_host}}'
-    token: '{{item}}'
-    state: absent
-  with_items:
-    - '{{new_ruleless.token}}'
-    - '{{with_rules.token}}'
+- import_tasks: update-acl.yml
+
+- import_tasks: remove-acl.yml

test/integration/roles/test_consul_acl/tasks/remove-acl.yml

@@ -0,0 +1,37 @@
+---
+
+- name: create an ACL
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+  register: created_acl
+
+- name: remove the ACL
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    state: absent
+  register: removed_acl
+
+# TODO: This does little to actually verify that the ACL has been removed
+- name: verify ACL has been removed
+  assert:
+    that:
+      - removed_acl.changed
+      - removed_acl.operation == "remove"
+      - removed_acl.token | length == 36
+
+- name: remove the ACL again
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    state: absent
+  register: doubly_removed_acl
+
+- name: verify idempotence when deleting an ACL
+  assert:
+    that:
+      - not doubly_removed_acl.changed

test/integration/roles/test_consul_acl/tasks/update-acl.yml

@@ -0,0 +1,71 @@
+---
+
+- name: create an ACL
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    name: "{{ test_consul_acl_token_name }}"
+    rules:
+      - key: "foo"
+        policy: read
+  register: created_acl
+
+- name: update ACL's rules
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    rules:
+      - key: "foo"
+        policy: write
+      - key: "moo"
+        policy: deny
+  register: updated_acl
+
+- name: verify updated ACL's rules
+  assert:
+    that:
+      - updated_acl.changed
+      - updated_acl.operation == "update"
+      - updated_acl.token | length == 36
+      - (updated_acl.rules | json_query("key.foo.policy")) == "write"
+      - (updated_acl.rules | json_query("key.moo.policy")) == "deny"
+
+- name: update already updated rule
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    rules:
+      - key: "foo"
+        policy: write
+      - key: "moo"
+        policy: deny
+  register: doubly_updated_acl
+
+- name: verify idempotence when setting rules
+  assert:
+    that:
+      - not doubly_updated_acl.changed
+
+- name: update to remove all ACL's rules
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    rules: []
+  register: updated_acl
+
+- name: verify ACL has no rules
+  assert:
+    that:
+      - updated_acl.changed
+      - updated_acl.token | length == 36
+      - updated_acl.rules == {}
+
+- name: clean up
+  consul_acl:
+    host: "{{ acl_host }}"
+    mgmt_token: "{{ mgmt_token }}"
+    token: "{{ created_acl.token }}"
+    state: absent

test/integration/roles/test_consul_acl/vars/main.yml

@@ -0,0 +1,4 @@
+---
+
+test_consul_acl_token_name: example-token
+test_consul_acl_token_id: 60DEC4BC-DD47-4F4E-A95A-19D639407D2C

test/sanity/pep8/legacy-files.txt

@@ -186,7 +186,6 @@ lib/ansible/modules/cloud/webfaction/webfaction_app.py
 lib/ansible/modules/cloud/webfaction/webfaction_db.py
 lib/ansible/modules/cloud/webfaction/webfaction_domain.py
 lib/ansible/modules/cloud/webfaction/webfaction_site.py
-lib/ansible/modules/clustering/consul_acl.py
 lib/ansible/modules/clustering/consul_kv.py
 lib/ansible/modules/clustering/consul_session.py
 lib/ansible/modules/clustering/kubernetes.py