From 857b2903541bda3ff5817801888a5b40596c0064 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 11:47:19 +0000
Subject: [PATCH 01/14] Convert filters and terms to list instead of dict

---
 salt/modules/capirca_acl.py | 228 ++++++++++++++++++++++--------------
 1 file changed, 137 insertions(+), 91 deletions(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index fb3509705dd7..f63de542a3a0 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -352,6 +352,20 @@ def _clean_term_opts(term_opts):
     return clean_opts
 
 
+def lookup_element(lst, key):
+    '''
+    Find an dictionary in a list of dictionaries, given its main key.
+    '''
+    if not lst:
+        return {}
+    for ele in lst:
+        if not ele or not isinstance(ele, dict):
+            continue
+        if ele.keys()[0] == key:
+            return ele.values()[0]
+    return {}
+
+
 def _get_pillar_cfg(pillar_key,
                     pillarenv=None,
                     saltenv=None):
@@ -361,24 +375,30 @@ def _get_pillar_cfg(pillar_key,
     pillar_cfg = __salt__['pillar.get'](pillar_key,
                                         pillarenv=pillarenv,
                                         saltenv=saltenv)
-    if not isinstance(pillar_cfg, dict):
-        return {}
     return pillar_cfg
 
 
-def _merge_dict(source, dest):
+def _merge_list_of_dict(first, second):
     '''
-    Merge dictionaries.
+    Merge lists of dictionaries.
+    Each element of the list is a dictionary having one single key.
+    That key is then used as unique lookup.
+    The first element list has higher priority than the second
     '''
-    if not source:
-        source = dest
-    elif source.keys() != dest.keys():
-        source_keys = set(source.keys())
-        dest_keys = set(dest.keys())
-        merge_keys = dest_keys - source_keys
-        for key in merge_keys:
-            source[key] = dest[key]
-    return source
+    if not first and not second:
+        return []
+    if not first and second:
+        return second
+    if first and not second:
+        return first
+    merged = first[:]
+    for ele in second:
+        if not ele or not isinstance(ele, dict):
+            continue # Ignore empty elements
+        if not lookup_element(first, ele.keys()[0]):
+            # element not found in the first list
+            merged.append(ele)
+    return merged
 
 
 def _get_term_object(filter_name,
@@ -399,10 +419,12 @@ def _get_term_object(filter_name,
     term.name = term_name
     term_opts = {}
     if merge_pillar:
-        term_pillar_key = ':'.join((pillar_key, filter_name, term_name))
-        term_opts = _get_pillar_cfg(term_pillar_key,
-                                    saltenv=saltenv,
-                                    pillarenv=pillarenv)
+        acl_pillar_cfg = _get_pillar_cfg(pillar_key,
+                                         saltenv=saltenv,
+                                         pillarenv=pillarenv)
+        filter_pillar_cfg = lookup_element(acl_pillar_cfg, filter_name)
+        term_pillar_cfg = filter_pillar_cfg.get('terms', [])
+        term_opts = lookup_element(term_pillar_cfg, term_name)
         log.debug('Merging with pillar data:')
         log.debug(term_opts)
         term_opts = _clean_term_opts(term_opts)
@@ -436,8 +458,12 @@ def _get_policy_object(platform,
     policy = _Policy()
     policy_filters = []
     if not filters:
-        filters = {}
-    for filter_name, filter_config in six.iteritems(filters):
+        filters = []
+    for filter_ in filters:
+        if not filter_ or not isinstance(filter_, dict):
+            continue  # go to the next filter
+        filter_name = filter_.keys()[0]
+        filter_config = filter_.values()[0]
         header = aclgen.policy.Header()  # same header everywhere
         target_opts = [
             platform,
@@ -451,14 +477,17 @@ def _get_policy_object(platform,
         target = aclgen.policy.Target(target_opts)
         header.AddObject(target)
         filter_terms = []
-        for term_name, term_fields in six.iteritems(filter_config):
-            term = _get_term_object(filter_name,
-                                    term_name,
-                                    pillar_key=pillar_key,
-                                    pillarenv=pillarenv,
-                                    saltenv=saltenv,
-                                    merge_pillar=merge_pillar,
-                                    **term_fields)
+        for term_ in filter_config.get('terms', []):
+            if term_ and isinstance(term_, dict):
+                term_name = term_.keys()[0]
+                term_fields = term_.values()[0]
+                term = _get_term_object(filter_name,
+                                        term_name,
+                                        pillar_key=pillar_key,
+                                        pillarenv=pillarenv,
+                                        saltenv=saltenv,
+                                        merge_pillar=merge_pillar,
+                                        **term_fields)
             filter_terms.append(term)
         policy_filters.append(
             (header, filter_terms)
@@ -555,8 +584,9 @@ def get_term_config(platform,
         .. code-block:: yaml
 
             firewall:
-                my-filter:
-                    my-term:
+              - my-filter:
+                  terms:
+                    - my-term:
                         source_port: 1234
                         source_address:
                             - 1.2.3.4/32
@@ -755,25 +785,24 @@ def get_term_config(platform,
 
     .. code-block:: text
 
-        ! $Id:$
-        ! $Date:$
-        ! $Revision:$
+        ! $Date: 2017/03/22 $
         no ip access-list filter-name
         ip access-list filter-name
-         remark $Id:$
          remark term-name
          permit ip host 1.2.3.4 host 5.6.7.8
         exit
     '''
-    terms = {
+    terms = []
+    term = {
         term_name: {
         }
     }
-    terms[term_name].update(term_fields)
-    terms[term_name].update({
+    term[term_name].update(term_fields)
+    term[term_name].update({
         'source_service': _make_it_list({}, 'source_service', source_service),
         'destination_service': _make_it_list({}, 'destination_service', destination_service),
     })
+    terms.append(term)
     if not filter_options:
         filter_options = []
     return get_filter_config(platform,
@@ -820,7 +849,7 @@ def get_filter_config(platform,
         .. _options: https://github.com/google/capirca/wiki/Policy-format#header-section
 
     terms
-        Dictionary of terms for this policy filter.
+        List of terms for this policy filter.
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
@@ -836,7 +865,12 @@ def get_filter_config(platform,
         :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
 
     merge_pillar: ``True``
-        Merge the CLI variables with the pillar. Default: ``True``
+        Merge the CLI variables with the pillar. Default: ``True``.
+
+        .. note::
+
+            When this argument is specified as ``True``, the terms sent through the CLI
+            have higher priority so they will be prepended to the terms defined in the pillar.
 
     only_lower_merge: ``False``
         Specify if it should merge only the terms fields. Otherwise it will try
@@ -882,34 +916,41 @@ def get_filter_config(platform,
     .. code-block:: yaml
 
         netacl:
-          my-filter:
-            my-term:
-              source_port: [1234, 1235]
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
+          - my-filter:
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
     '''
     if not filter_options:
         filter_options = []
     if not terms:
-        terms = {}
+        terms = []
     if merge_pillar and not only_lower_merge:
-        filter_pillar_key = ':'.join((pillar_key, filter_name))
-        filter_pillar_cfg = _get_pillar_cfg(filter_pillar_key)
+        acl_pillar_cfg = _get_pillar_cfg(pillar_key,
+                                         saltenv=saltenv,
+                                         pillarenv=pillarenv)
+        filter_pillar_cfg = lookup_element(acl_pillar_cfg, filter_name)
         filter_options = filter_options or filter_pillar_cfg.pop('options', None)
-        terms = _merge_dict(terms, filter_pillar_cfg)
-        # merge the passed variable with the pillar data
-        # any filter term not defined here, will be appended from the pillar
-        # new terms won't be removed
-    filters = {
+        if filter_pillar_cfg:
+            # Only when it was able to find the filter in the ACL config
+            pillar_terms = filter_pillar_cfg.get('terms', [])  # No problem if empty in the pillar
+            terms = _merge_list_of_dict(terms, pillar_terms)
+            # merge the passed variable with the pillar data
+            # any filter term not defined here, will be appended from the pillar
+            # new terms won't be removed
+    filters = []
+    filters.append({
         filter_name: {
-            'options': _make_it_list({}, filter_name, filter_options)
+            'options': _make_it_list({}, filter_name, filter_options),
+            'terms': terms
         }
-    }
-    filters[filter_name].update(terms)
+    })
     return get_policy_config(platform,
                              filters=filters,
                              pillar_key=pillar_key,
@@ -941,7 +982,7 @@ def get_policy_config(platform,
         The name of the Capirca platform.
 
     filters
-        Dictionary of filters for this policy.
+        List of filters for this policy.
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
@@ -994,16 +1035,22 @@ def get_policy_config(platform,
                 ** $Revision:$
                 **
                 */
-                filter my-other-filter {
-                    interface-specific;
-                    term dummy-term {
+                filter my-filter {
+                    term my-term {
                         from {
-                            protocol [ tcp udp ];
+                            source-port [ 1234 1235 ];
                         }
                         then {
                             reject;
                         }
                     }
+                    term my-other-term {
+                        from {
+                            protocol tcp;
+                            source-port 5678-5680;
+                        }
+                        then accept;
+                    }
                 }
             }
         }
@@ -1016,23 +1063,16 @@ def get_policy_config(platform,
                 ** $Revision:$
                 **
                 */
-                filter my-filter {
+                filter my-other-filter {
                     interface-specific;
-                    term my-term {
+                    term dummy-term {
                         from {
-                            source-port [ 1234 1235 ];
+                            protocol [ tcp udp ];
                         }
                         then {
                             reject;
                         }
                     }
-                    term my-other-term {
-                        from {
-                            protocol tcp;
-                            source-port 5678-5680;
-                        }
-                        then accept;
-                    }
                 }
             }
         }
@@ -1042,32 +1082,38 @@ def get_policy_config(platform,
     .. code-block:: yaml
 
         netacl:
-          my-filter:
-            my-term:
-              source_port: [1234, 1235]
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
-          my-other-filter:
-            dummy-term:
-              protocol:
-                - tcp
-                - udp
-              action: reject
+          - my-filter:
+              options:
+                - not-interface-specific
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
+          - my-other-filter:
+              terms:
+                - dummy-term:
+                    protocol:
+                      - tcp
+                      - udp
+                    action: reject
     '''
     if not filters:
-        filters = {}
+        filters = []
     if merge_pillar and not only_lower_merge:
         # the pillar key for the policy config is the `pillar_key` itself
-        policy_pillar_cfg = _get_pillar_cfg(pillar_key)
+        policy_pillar_cfg = _get_pillar_cfg(pillar_key,
+                                            saltenv=saltenv,
+                                            pillarenv=pillarenv)
         # now, let's merge everything witht the pillar data
         # again, this will not remove any extra filters/terms
         # but it will merge with the pillar data
         # if this behaviour is not wanted, the user can set `merge_pillar` as `False`
-        filters = _merge_dict(filters, policy_pillar_cfg)
+        filters = _merge_list_of_dict(filters, policy_pillar_cfg)
     policy_object = _get_policy_object(platform,
                                        filters=filters,
                                        pillar_key=pillar_key,

From c95bcf36130f883062661be3502fe288f1f5e30a Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 12:29:10 +0000
Subject: [PATCH 02/14] Flexible position for new terms

---
 salt/modules/capirca_acl.py | 69 +++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 14 deletions(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index f63de542a3a0..1410ea519da2 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -378,26 +378,57 @@ def _get_pillar_cfg(pillar_key,
     return pillar_cfg
 
 
-def _merge_list_of_dict(first, second):
+def _cleanup(lst):
+    '''
+    Return a list of non-empty dictionaries.
+    '''
+    clean = []
+    for ele in lst:
+        if ele and isinstance(ele, dict):
+            clean.append(ele)
+    return clean
+
+
+def _merge_list_of_dict(first, second, prepend=True):
     '''
     Merge lists of dictionaries.
     Each element of the list is a dictionary having one single key.
     That key is then used as unique lookup.
-    The first element list has higher priority than the second
+    The first element list has higher priority than the second.
+    When there's an overlap between the two lists,
+    it won't change the position, but the content.
     '''
+    first = _cleanup(first)
+    second = _cleanup(second)
     if not first and not second:
         return []
     if not first and second:
         return second
     if first and not second:
         return first
-    merged = first[:]
+    # Determine overlaps
+    # So we dont change the position of the existing terms/filters
+    overlaps = []
+    merged = []
+    appended = []
+    for ele in first:
+        if lookup_element(second, ele.keys()[0]):
+            overlaps.append(ele)
+        elif prepend:
+            merged.append(ele)
+        elif not prepend:
+            appended.append(ele)
     for ele in second:
-        if not ele or not isinstance(ele, dict):
-            continue # Ignore empty elements
-        if not lookup_element(first, ele.keys()[0]):
-            # element not found in the first list
+        ele_key = ele.keys()[0]
+        if lookup_element(overlaps, ele_key):
+            # If theres an overlap, get the value from the first
+            # But inserted into the right position
+            ele_val_first = lookup_element(first, ele_key)
+            merged.append({ele_key: ele_val_first})
+        else:
             merged.append(ele)
+    if not prepend:
+        merged.extend(appended)
     return merged
 
 
@@ -824,6 +855,7 @@ def get_filter_config(platform,
                       filter_name,
                       filter_options=None,
                       terms=None,
+                      prepend=True,
                       pillar_key='acl',
                       pillarenv=None,
                       saltenv=None,
@@ -853,6 +885,12 @@ def get_filter_config(platform,
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of terms generated by merging
+        the terms from ``terms`` with those defined in the pillar (if any): new terms are prepended
+        at the beginning, while existing ones will preserve the position. To add the new terms
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -867,11 +905,6 @@ def get_filter_config(platform,
     merge_pillar: ``True``
         Merge the CLI variables with the pillar. Default: ``True``.
 
-        .. note::
-
-            When this argument is specified as ``True``, the terms sent through the CLI
-            have higher priority so they will be prepended to the terms defined in the pillar.
-
     only_lower_merge: ``False``
         Specify if it should merge only the terms fields. Otherwise it will try
         to merge also filters fields. Default: ``False``.
@@ -940,10 +973,11 @@ def get_filter_config(platform,
         if filter_pillar_cfg:
             # Only when it was able to find the filter in the ACL config
             pillar_terms = filter_pillar_cfg.get('terms', [])  # No problem if empty in the pillar
-            terms = _merge_list_of_dict(terms, pillar_terms)
+            terms = _merge_list_of_dict(terms, pillar_terms, prepend=prepend)
             # merge the passed variable with the pillar data
             # any filter term not defined here, will be appended from the pillar
             # new terms won't be removed
+    return terms
     filters = []
     filters.append({
         filter_name: {
@@ -966,6 +1000,7 @@ def get_filter_config(platform,
 
 def get_policy_config(platform,
                       filters=None,
+                      prepend=True,
                       pillar_key='acl',
                       pillarenv=None,
                       saltenv=None,
@@ -986,6 +1021,12 @@ def get_policy_config(platform,
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of filters generated by merging
+        the filters from ``filters`` with those defined in the pillar (if any): new filters are prepended
+        at the beginning, while existing ones will preserve the position. To add the new filters
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -1113,7 +1154,7 @@ def get_policy_config(platform,
         # again, this will not remove any extra filters/terms
         # but it will merge with the pillar data
         # if this behaviour is not wanted, the user can set `merge_pillar` as `False`
-        filters = _merge_list_of_dict(filters, policy_pillar_cfg)
+        filters = _merge_list_of_dict(filters, policy_pillar_cfg, prepend=prepend)
     policy_object = _get_policy_object(platform,
                                        filters=filters,
                                        pillar_key=pillar_key,

From 558fcd0c75343d093c985dad28f3a237646a90f5 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:05:24 +0000
Subject: [PATCH 03/14] Do not return the terms

---
 salt/modules/capirca_acl.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index 1410ea519da2..5bb6fae836ff 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -977,7 +977,6 @@ def get_filter_config(platform,
             # merge the passed variable with the pillar data
             # any filter term not defined here, will be appended from the pillar
             # new terms won't be removed
-    return terms
     filters = []
     filters.append({
         filter_name: {

From 8c6c5c816cfbd791a78eed60d3a9bca377a563ab Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:05:53 +0000
Subject: [PATCH 04/14] Fix netacl doc

---
 salt/modules/napalm_acl.py | 117 +++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 63 deletions(-)

diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index 597664ebad64..f25c909d651e 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -154,8 +154,9 @@ def load_term_config(filter_name,
         .. code-block:: yaml
 
             firewall:
-                my-filter:
-                    my-term:
+              - my-filter:
+                  terms:
+                    - my-term:
                         source_port: 1234
                         source_address:
                             - 1.2.3.4/32
@@ -217,7 +218,7 @@ def load_term_config(filter_name,
     **term_fields
         Term attributes.
         To see what fields are supported, please consult the list of supported keywords_.
-            Some platforms have few other optional_ keyworkds.
+        Some platforms have few other optional_ keywords.
 
             .. _keywords: https://github.com/google/capirca/wiki/Policy-format#keywords
             .. _optional: https://github.com/google/capirca/wiki/Policy-format#optionally-supported-keywords
@@ -377,9 +378,7 @@ def load_term_config(filter_name,
                 [edit firewall]
                 +    family inet {
                 +        /*
-                +         ** $Id:$
-                +         ** $Date:$
-                +         ** $Revision:$
+                +         ** $Date: 2017/03/22 $
                 +         **
                 +         */
                 +        filter filter-name {
@@ -402,9 +401,7 @@ def load_term_config(filter_name,
                     family inet {
                         replace:
                         /*
-                        ** $Id:$
-                        ** $Date:$
-                        ** $Revision:$
+                        ** $Date: 2017/03/22 $
                         **
                         */
                         filter filter-name {
@@ -528,6 +525,7 @@ def load_filter_config(filter_name,
         as ``loaded_config`` contaning the raw configuration loaded on the device.
 
     The output is a dictionary having the same form as :mod:`net.load_config <salt.modules.napalm_network.load_config>`.
+
     CLI Example:
 
     .. code-block:: bash
@@ -547,9 +545,7 @@ def load_filter_config(filter_name,
                 [edit firewall]
                 +    family inet {
                 +        /*
-                +         ** $Id:$
-                +         ** $Date:$
-                +         ** $Revision:$
+                +         ** $Date: 2017/03/22 $
                 +         **
                 +         */
                 +        filter my-filter {
@@ -576,9 +572,7 @@ def load_filter_config(filter_name,
                     family inet {
                         replace:
                         /*
-                        ** $Id:$
-                        ** $Date:$
-                        ** $Revision:$
+                        ** $Date: 2017/03/22 $
                         **
                         */
                         filter my-filter {
@@ -609,15 +603,16 @@ def load_filter_config(filter_name,
     .. code-block:: yaml
 
         netacl:
-          my-filter:
-            my-term:
-              source_port: [1234, 1235]
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
+          - my-filter:
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
     '''
     if not filter_options:
         filter_options = []
@@ -732,34 +727,23 @@ def load_policy_config(filters=None,
                 @@ -1228,9 +1228,24 @@
                  !
                 +ipv4 access-list my-filter
-                + 10 remark $Id:$
-                + 20 remark my-term
-                + 30 deny tcp host 1.2.3.4 eq 1234 any
-                + 40 deny udp host 1.2.3.4 eq 1234 any
-                + 50 deny tcp host 1.2.3.4 eq 1235 any
-                + 60 deny udp host 1.2.3.4 eq 1235 any
-                + 70 remark my-other-term
-                + 80 permit tcp any range 5678 5680 any
+                + 10 remark my-term
+                + 20 deny tcp host 1.2.3.4 eq 1234 any
+                + 30 deny udp host 1.2.3.4 eq 1234 any
+                + 40 deny tcp host 1.2.3.4 eq 1235 any
+                + 50 deny udp host 1.2.3.4 eq 1235 any
+                + 60 remark my-other-term
+                + 70 permit tcp any range 5678 5680 any
                 +!
                 +!
                 +ipv4 access-list block-icmp
-                + 10 remark $Id:$
-                + 20 remark first-term
-                + 30 deny icmp any any
+                + 10 remark first-term
+                + 20 deny icmp any any
                  !
             loaded_config:
-                ! $Id:$
-                ! $Date:$
-                ! $Revision:$
-                no ipv4 access-list block-icmp
-                ipv4 access-list block-icmp
-                 remark $Id:$
-                 remark first-term
-                 deny icmp any any
-                exit
+                ! $Date: 2017/03/22 $
                 no ipv4 access-list my-filter
                 ipv4 access-list my-filter
-                 remark $Id:$
                  remark my-term
                  deny tcp host 1.2.3.4 eq 1234 any
                  deny udp host 1.2.3.4 eq 1234 any
@@ -768,6 +752,11 @@ def load_policy_config(filters=None,
                  remark my-other-term
                  permit tcp any range 5678 5680 any
                 exit
+                no ipv4 access-list block-icmp
+                ipv4 access-list block-icmp
+                 remark first-term
+                 deny icmp any any
+                exit
             result:
                 True
 
@@ -776,24 +765,26 @@ def load_policy_config(filters=None,
     .. code-block:: yaml
 
         acl:
-          my-filter:
-            my-term:
-              source_port: [1234, 1235]
-              protocol:
-                - tcp
-                - udp
-              source_address: 1.2.3.4
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
-          block-icmp:
-            first-term:
-              protocol:
-                - icmp
-              action: reject
+          - my-filter:
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    protocol:
+                      - tcp
+                      - udp
+                    source_address: 1.2.3.4
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
+          - block-icmp:
+              terms:
+                - first-term:
+                    protocol:
+                      - icmp
+                    action: reject
     '''
     if not filters:
         filters = {}

From d8d758e51ccba930e1d558cb27214377d1b3da3e Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:06:35 +0000
Subject: [PATCH 05/14] Fix netacl defaults

---
 salt/modules/napalm_acl.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index f25c909d651e..f9f9d53bcfe7 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -617,7 +617,7 @@ def load_filter_config(filter_name,
     if not filter_options:
         filter_options = []
     if not terms:
-        terms = {}
+        terms = []
     platform = _get_capirca_platform()
     filter_config = __salt__['capirca.get_filter_config'](platform,
                                                           filter_name,
@@ -787,7 +787,7 @@ def load_policy_config(filters=None,
                     action: reject
     '''
     if not filters:
-        filters = {}
+        filters = []
     platform = _get_capirca_platform()
     policy_config = __salt__['capirca.get_policy_config'](platform,
                                                           filters=filters,

From 3fb20603b2c00f21dbaa609537f5d11f5b1c987b Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:11:01 +0000
Subject: [PATCH 06/14] modules.napalm_acl: add prepend arg

---
 salt/modules/napalm_acl.py | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index f9f9d53bcfe7..0589052e87d1 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -456,6 +456,7 @@ def load_filter_config(filter_name,
                        pillarenv=None,
                        saltenv=None,
                        merge_pillar=True,
+                       prepend=True,
                        only_lower_merge=False,
                        revision_id=None,
                        revision_no=None,
@@ -478,10 +479,16 @@ def load_filter_config(filter_name,
         .. _options: https://github.com/google/capirca/wiki/Policy-format#header-section
 
     terms
-        Dictionary of terms for this policy filter.
+        List of terms for this policy filter.
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of terms generated by merging
+        the terms from ``terms`` with those defined in the pillar (if any): new terms are prepended
+        at the beginning, while existing ones will preserve the position. To add the new terms
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -627,6 +634,7 @@ def load_filter_config(filter_name,
                                                           pillarenv=pillarenv,
                                                           saltenv=saltenv,
                                                           merge_pillar=merge_pillar,
+                                                          prepend=prepend,
                                                           only_lower_merge=only_lower_merge,
                                                           revision_id=revision_id,
                                                           revision_no=revision_no,
@@ -645,6 +653,7 @@ def load_policy_config(filters=None,
                        pillarenv=None,
                        saltenv=None,
                        merge_pillar=True,
+                       prepend=True,
                        only_lower_merge=False,
                        revision_id=None,
                        revision_no=None,
@@ -658,10 +667,16 @@ def load_policy_config(filters=None,
     Generate and load the configuration of the whole policy.
 
     filters
-        Dictionary of filters for this policy.
+        List of filters for this policy.
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of filters generated by merging
+        the filters from ``filters`` with those defined in the pillar (if any): new filters are prepended
+        at the beginning, while existing ones will preserve the position. To add the new filters
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -795,6 +810,7 @@ def load_policy_config(filters=None,
                                                           pillarenv=pillarenv,
                                                           saltenv=saltenv,
                                                           merge_pillar=merge_pillar,
+                                                          prepend=prepend,
                                                           only_lower_merge=only_lower_merge,
                                                           revision_id=revision_id,
                                                           revision_no=revision_no,

From f85ca7862fbf3b7b776c51ca8ef9fa5b61fdaaa8 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:21:20 +0000
Subject: [PATCH 07/14] More doc notes

---
 salt/modules/napalm_acl.py | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index 0589052e87d1..bb37cf763a05 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -452,11 +452,11 @@ def load_term_config(filter_name,
 def load_filter_config(filter_name,
                        filter_options=None,
                        terms=None,
+                       prepend=True,
                        pillar_key='acl',
                        pillarenv=None,
                        saltenv=None,
                        merge_pillar=True,
-                       prepend=True,
                        only_lower_merge=False,
                        revision_id=None,
                        revision_no=None,
@@ -469,6 +469,15 @@ def load_filter_config(filter_name,
     '''
     Generate and load the configuration of a policy filter.
 
+    .. note::
+
+        The order of the terms is very important. The configuration loaded
+        on the device respects the order defined in the ``terms`` and/or
+        inside the pillar.
+
+        When merging the ``terms`` with the pillar data, consider the
+        ``prepend`` argument to make sure the order is correct!
+
     filter_name
         The name of the policy filter.
 
@@ -501,7 +510,10 @@ def load_filter_config(filter_name,
         :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
 
     merge_pillar: ``True``
-        Merge the CLI variables with the pillar. Default: ``True``
+        Merge the CLI variables with the pillar. Default: ``True``.
+
+        The merge logic depends on the ``prepend`` argument and
+        the CLI has higher priority than the pillar.
 
     only_lower_merge: ``False``
         Specify if it should merge only the terms fields. Otherwise it will try
@@ -629,12 +641,12 @@ def load_filter_config(filter_name,
     filter_config = __salt__['capirca.get_filter_config'](platform,
                                                           filter_name,
                                                           terms=terms,
+                                                          prepend=prepend,
                                                           filter_options=filter_options,
                                                           pillar_key=pillar_key,
                                                           pillarenv=pillarenv,
                                                           saltenv=saltenv,
                                                           merge_pillar=merge_pillar,
-                                                          prepend=prepend,
                                                           only_lower_merge=only_lower_merge,
                                                           revision_id=revision_id,
                                                           revision_no=revision_no,
@@ -649,11 +661,11 @@ def load_filter_config(filter_name,
 
 @proxy_napalm_wrap
 def load_policy_config(filters=None,
+                       prepend=True,
                        pillar_key='acl',
                        pillarenv=None,
                        saltenv=None,
                        merge_pillar=True,
-                       prepend=True,
                        only_lower_merge=False,
                        revision_id=None,
                        revision_no=None,
@@ -666,6 +678,15 @@ def load_policy_config(filters=None,
     '''
     Generate and load the configuration of the whole policy.
 
+    .. note::
+
+        The order of the filters and their terms is very important.
+        The configuration loaded on the device respects the order
+        defined in the ``filters`` and/or inside the pillar.
+
+        When merging the ``filters`` with the pillar data, consider the
+        ``prepend`` argument to make sure the order is correct!
+
     filters
         List of filters for this policy.
         If not specified or empty, will try to load the configuration from the pillar,
@@ -691,6 +712,9 @@ def load_policy_config(filters=None,
     merge_pillar: ``True``
         Merge the CLI variables with the pillar. Default: ``True``.
 
+        The merge logic depends on the ``prepend`` argument and
+        the CLI has higher priority than the pillar.
+
     only_lower_merge: ``False``
         Specify if it should merge only the filters and terms fields. Otherwise it will try
         to merge everything at the policy level. Default: ``False``.
@@ -806,11 +830,11 @@ def load_policy_config(filters=None,
     platform = _get_capirca_platform()
     policy_config = __salt__['capirca.get_policy_config'](platform,
                                                           filters=filters,
+                                                          prepend=prepend,
                                                           pillar_key=pillar_key,
                                                           pillarenv=pillarenv,
                                                           saltenv=saltenv,
                                                           merge_pillar=merge_pillar,
-                                                          prepend=prepend,
                                                           only_lower_merge=only_lower_merge,
                                                           revision_id=revision_id,
                                                           revision_no=revision_no,

From 6698262a14b7855f85f5270760a13dc1e063e13f Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 13:59:44 +0000
Subject: [PATCH 08/14] More doc clarify for netacl

---
 salt/modules/napalm_acl.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index bb37cf763a05..f0eb2798edfa 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -175,6 +175,8 @@ def load_term_config(filter_name,
     merge_pillar: ``True``
         Merge the CLI variables with the pillar. Default: ``True``.
 
+        The properties specified through the CLI have higher priority than the pillar.
+
     revision_id
         Add a comment in the term config having the description for the changes applied.
 
@@ -224,7 +226,7 @@ def load_term_config(filter_name,
             .. _optional: https://github.com/google/capirca/wiki/Policy-format#optionally-supported-keywords
 
     .. note::
-        The following fields are accepted:
+        The following fields are accepted (some being platform-specific):
 
         - action
         - address

From 18462c86836a5348528cb386d50e2ac6aa1b7988 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 14:00:22 +0000
Subject: [PATCH 09/14] Allow source_service and destination_service in the
 pillar & log

---
 salt/modules/capirca_acl.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index 5bb6fae836ff..c3b4f3e76432 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -316,6 +316,10 @@ def _clean_term_opts(term_opts):
         # firstly we'll process special fields like source_service or destination_services
         # which will inject values directly in the source or destination port and protocol
         if field == 'source_service' and value:
+            if isinstance(value, str):
+                value = _make_it_list(clean_opts, field, value)
+            log.debug('Processing special source services:')
+            log.debug(value)
             for service in value:
                 if service and service in _services:
                     # if valid source_service
@@ -326,7 +330,15 @@ def _clean_term_opts(term_opts):
                     clean_opts['protocol'] = _make_it_list(clean_opts,
                                                            'protocol',
                                                            _services[service]['protocol'])
+            log.debug('Built source_port field, after processing special source services:')
+            log.debug(clean_opts['source_port'])
+            log.debug('Built protocol field, after processing special source services:')
+            log.debug(clean_opts['protocol'])
         elif field == 'destination_service' and value:
+            if isinstance(value, str):
+                value = _make_it_list(clean_opts, field, value)
+            log.debug('Processing special destination services:')
+            log.debug(value)
             for service in value:
                 if service and service in _services:
                     # if valid destination_service
@@ -337,6 +349,10 @@ def _clean_term_opts(term_opts):
                     clean_opts['protocol'] = _make_it_list(clean_opts,
                                                            'protocol',
                                                            _services[service]['protocol'])
+            log.debug('Built source_port field, after processing special destination services:')
+            log.debug(clean_opts['destination_service'])
+            log.debug('Built protocol field, after processing special destination services:')
+            log.debug(clean_opts['protocol'])
         # not a special field, but it has to be a valid one
         elif field in _TERM_FIELDS and value and value != _TERM_FIELDS[field]:
             # if not a special field type

From 1bf64a6df89278a05924f07c2a557597021b97b0 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 14:31:10 +0000
Subject: [PATCH 10/14] Documentation improvements for the netacl state

---
 salt/states/netacl.py | 279 ++++++++++++++++++++++++++++--------------
 1 file changed, 187 insertions(+), 92 deletions(-)

diff --git a/salt/states/netacl.py b/salt/states/netacl.py
index 0fc8fd7422db..26bfd2d7a8d9 100644
--- a/salt/states/netacl.py
+++ b/salt/states/netacl.py
@@ -173,6 +173,8 @@ def term(name,
     merge_pillar: ``False``
         Merge the CLI variables with the pillar. Default: ``False``.
 
+        The properties specified through the state arguments have higher priority than the pillar.
+
     revision_id
         Add a comment in the term config having the description for the changes applied.
 
@@ -364,7 +366,7 @@ def term(name,
 
     .. code-block:: yaml
 
-        edge01.sfo04:
+        edge01.bjm01:
         ----------
                   ID: update_icmp_first_term
             Function: netacl.term
@@ -394,7 +396,7 @@ def term(name,
                           +        }
                           +    }
 
-        Summary for edge01.sfo04
+        Summary for edge01.bjm01
         ------------
         Succeeded: 1 (unchanged=1, changed=1)
         Failed:    0
@@ -407,19 +409,20 @@ def term(name,
     .. code-block:: yaml
 
         firewall:
-          block-icmp:
-            first-term:
-              protocol:
-                - icmp
-              action: reject
+          - block-icmp:
+              terms:
+                - first-term:
+                    protocol:
+                      - icmp
+                    action: reject
 
     State SLS example:
 
-    .. code-block:: yaml
+    .. code-block:: jinja
 
         {%- set filter_name = 'block-icmp' -%}
         {%- set term_name = 'first-term' -%}
-        {%- set my_term_cfg = pillar['acl'][filter_name][term_name] -%}
+        {%- set my_term_cfg = salt.netacl.get_term_pillar('acl', filter_name, term_name) -%}
 
         update_icmp_first_term:
           netacl.term:
@@ -429,9 +432,28 @@ def term(name,
             - term_name: {{ term_name }}
             - {{ my_term_cfg | json }}
 
-    When passing retrieved pillar data into the state file, it is strongly
-    recommended to use the json serializer explicitly (`` | json``),
-    instead of relying on the default Python serializer.
+    Or directly referencing the pillar keys:
+
+    .. code-block:: yaml
+
+        update_icmp_first_term:
+          netacl.term:
+            - filter_name: block-icmp
+            - filter_options:
+                - not-interface-specific
+            - term_name: first-term
+            - merge_pillar: true
+
+    .. note::
+        The first method allows the user to eventually apply complex manipulation
+        and / or retrieve the data from external services before passing the
+        data to the state. The second one is more straighforward, for less
+        complex cases when loading the data directly from the pillar is sufficient.
+
+    .. note::
+        When passing retrieved pillar data into the state file, it is strongly
+        recommended to use the json serializer explicitly (`` | json``),
+        instead of relying on the default Python serializer.
     '''
     ret = _default_ret(name)
     test = __opts__['test'] or test
@@ -461,6 +483,7 @@ def filter(name,  # pylint: disable=redefined-builtin
            filter_name,
            filter_options=None,
            terms=None,
+           prepend=True,
            pillar_key='acl',
            pillarenv=None,
            saltenv=None,
@@ -490,6 +513,12 @@ def filter(name,  # pylint: disable=redefined-builtin
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of terms generated by merging
+        the terms from ``terms`` with those defined in the pillar (if any): new terms are prepended
+        at the beginning, while existing ones will preserve the position. To add the new terms
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -502,7 +531,15 @@ def filter(name,  # pylint: disable=redefined-builtin
         :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
 
     merge_pillar: ``False``
-        Merge the CLI variables with the pillar. Default: ``False``
+        Merge ``terms`` with the corresponding value from the pillar. Default: ``False``.
+
+        .. note::
+            By default this state does not merge, to avoid any unexpected behaviours.
+
+            The merge logic depends on the ``prepend`` argument.
+
+            The terms specified through the ``terms`` argument have higher priority
+            than the pillar.
 
     only_lower_merge: ``False``
         Specify if it should merge only the terms fields. Otherwise it will try
@@ -588,27 +625,28 @@ def filter(name,  # pylint: disable=redefined-builtin
     .. code-block:: yaml
 
         acl:
-          my-filter:
-            options:
-              - inet6
-            my-term:
-              source_port: [1234, 1235]
-              protocol:
-                - tcp
-                - udp
-              source_address: 1.2.3.4
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
+          - my-filter:
+              options:
+                - inet6
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    protocol:
+                      - tcp
+                      - udp
+                    source_address: 1.2.3.4
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
 
     State SLS Example:
 
-    .. code-block:: yaml
+    .. code-block:: jinja
 
-        {% set my_filter_cfg = pillar.get('acl').get('my-filter') -%}
+        {% set my_filter_cfg = salt.netacl.get_filter_pillar('firewall', 'my-filter') -%}
         my-filter_state:
           netacl.filter:
             - filter_name: my-filter
@@ -617,23 +655,44 @@ def filter(name,  # pylint: disable=redefined-builtin
             - revision_no: 5
             - debug: true
 
+    Or:
+
+    .. code-block:: yaml
+
+        my-filter_state:
+          netacl.filter:
+            - filter_name: my-filter
+            - merge_pillar: true
+            - pillar_key: firewall
+            - revision_date: false
+            - revision_no: 5
+            - debug: true
+
     In the example above, as ``inet6`` has been specified in the ``filter_options``,
     the configuration chunk referring to ``my-term`` has been ignored as it referred to
     IPv4 only (from ``source_address`` field).
 
-    When passing retrieved pillar data into the state file, it is strongly
-    recommended to use the json serializer explicitly (`` | json``),
-    instead of relying on the default Python serializer.
+    .. note::
+        The first method allows the user to eventually apply complex manipulation
+        and / or retrieve the data from external services before passing the
+        data to the state. The second one is more straighforward, for less
+        complex cases when loading the data directly from the pillar is sufficient.
+
+    .. note::
+        When passing retrieved pillar data into the state file, it is strongly
+        recommended to use the json serializer explicitly (`` | json``),
+        instead of relying on the default Python serializer.
     '''
     ret = _default_ret(name)
     test = __opts__['test'] or test
     if not filter_options:
         filter_options = []
     if not terms:
-        terms = {}
+        terms = []
     loaded = __salt__['netacl.load_filter_config'](filter_name,
                                                    filter_options=filter_options,
                                                    terms=terms,
+                                                   prepend=prepend,
                                                    pillar_key=pillar_key,
                                                    pillarenv=pillarenv,
                                                    saltenv=saltenv,
@@ -651,6 +710,7 @@ def filter(name,  # pylint: disable=redefined-builtin
 
 def managed(name,
             filters=None,
+            prepend=True,
             pillar_key='acl',
             pillarenv=None,
             saltenv=None,
@@ -671,6 +731,12 @@ def managed(name,
         If not specified or empty, will try to load the configuration from the pillar,
         unless ``merge_pillar`` is set as ``False``.
 
+    prepend: ``True``
+        When ``merge_pillar`` is set as ``True``, the final list of filters generated by merging
+        the filters from ``filters`` with those defined in the pillar (if any): new filters are prepended
+        at the beginning, while existing ones will preserve the position. To add the new filters
+        at the end of the list, set this argument to ``False``.
+
     pillar_key: ``acl``
         The key in the pillar containing the default attributes values. Default: ``acl``.
 
@@ -683,7 +749,15 @@ def managed(name,
         :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
 
     merge_pillar: ``False``
-        Merge the CLI variables with the pillar. Default: ``False``.
+        Merge the ``filters`` wil the corresponding values from the pillar. Default: ``False``.
+
+        .. note::
+            By default this state does not merge, to avoid any unexpected behaviours.
+
+            The merge logic depends on the ``prepend`` argument.
+
+            The filters specified through the ``filters`` argument have higher priority
+            than the pillar.
 
     only_lower_merge: ``False``
         Specify if it should merge only the filters and terms fields. Otherwise it will try
@@ -742,23 +816,6 @@ def managed(name,
                               +         ** $Revision: 2 $
                               +         **
                               +         */
-                              +        filter block-icmp {
-                              +            interface-specific;
-                              +            term first-term {
-                              +                from {
-                              +                    protocol icmp;
-                              +                }
-                              +                then {
-                              +                    reject;
-                              +                }
-                              +            }
-                              +        }
-                              +        /*
-                              +         ** $Id: netacl_example $
-                              +         ** $Date: 2017/07/03 $
-                              +         ** $Revision: 2 $
-                              +         **
-                              +         */
                               +        filter my-filter {
                               +            interface-specific;
                               +            term my-term {
@@ -781,6 +838,23 @@ def managed(name,
                               +                then accept;
                               +            }
                               +        }
+                              +        /*
+                              +         ** $Id: netacl_example $
+                              +         ** $Date: 2017/07/03 $
+                              +         ** $Revision: 2 $
+                              +         **
+                              +         */
+                              +        filter block-icmp {
+                              +            interface-specific;
+                              +            term first-term {
+                              +                from {
+                              +                    protocol icmp;
+                              +                }
+                              +                then {
+                              +                    reject;
+                              +                }
+                              +            }
+                              +        }
                               +    }
                           loaded:
                               firewall {
@@ -792,16 +866,27 @@ def managed(name,
                                       ** $Revision: 2 $
                                       **
                                       */
-                                      filter block-icmp {
+                                      filter my-filter {
                                           interface-specific;
-                                          term first-term {
+                                          term my-term {
                                               from {
-                                                  protocol icmp;
+                                                  source-address {
+                                                      1.2.3.4/32;
+                                                  }
+                                                  protocol [ tcp udp ];
+                                                  source-port [ 1234 1235 ];
                                               }
                                               then {
                                                   reject;
                                               }
                                           }
+                                          term my-other-term {
+                                              from {
+                                                  protocol tcp;
+                                                  source-port 5678-5680;
+                                              }
+                                              then accept;
+                                          }
                                       }
                                   }
                               }
@@ -814,27 +899,16 @@ def managed(name,
                                       ** $Revision: 2 $
                                       **
                                       */
-                                      filter my-filter {
+                                      filter block-icmp {
                                           interface-specific;
-                                          term my-term {
+                                          term first-term {
                                               from {
-                                                  source-address {
-                                                      1.2.3.4/32;
-                                                  }
-                                                  protocol [ tcp udp ];
-                                                  source-port [ 1234 1235 ];
+                                                  protocol icmp;
                                               }
                                               then {
                                                   reject;
                                               }
                                           }
-                                          term my-other-term {
-                                              from {
-                                                  protocol tcp;
-                                                  source-port 5678-5680;
-                                              }
-                                              then accept;
-                                          }
                                       }
                                   }
                               }
@@ -852,28 +926,30 @@ def managed(name,
     .. code-block:: yaml
 
         firewall:
-          my-filter:
-            my-term:
-              source_port: [1234, 1235]
-              protocol:
-                - tcp
-                - udp
-              source_address: 1.2.3.4
-              action: reject
-            my-other-term:
-              source_port:
-                - [5678, 5680]
-              protocol: tcp
-              action: accept
-          block-icmp:
-            first-term:
-              protocol:
-                - icmp
-              action: reject
+          - my-filter:
+              terms:
+                - my-term:
+                    source_port: [1234, 1235]
+                    protocol:
+                      - tcp
+                      - udp
+                    source_address: 1.2.3.4
+                    action: reject
+                - my-other-term:
+                    source_port:
+                      - [5678, 5680]
+                    protocol: tcp
+                    action: accept
+          - block-icmp:
+              terms:
+                - first-term:
+                    protocol:
+                      - icmp
+                    action: reject
 
     Example SLS file:
 
-    .. code-block:: yaml
+    .. code-block:: jinja
 
         {%- set fw_filters = pillar.get('firewall', {}) -%}
         netacl_example:
@@ -882,15 +958,34 @@ def managed(name,
             - revision_no: 2
             - debug: true
 
-    When passing retrieved pillar data into the state file, it is strongly
-    recommended to use the json serializer explicitly (`` | json``),
-    instead of relying on the default Python serializer.
+    Or:
+
+    .. code-block:: yaml
+
+        netacl_example:
+          netacl.managed:
+            - pillar_key: firewall
+            - merge_pillar: true
+            - revision_no: 2
+            - debug: true
+
+    .. note::
+        The first method allows the user to eventually apply complex manipulation
+        and / or retrieve the data from external services before passing the
+        data to the state. The second one is more straighforward, for less
+        complex cases when loading the data directly from the pillar is sufficient.
+
+    .. note::
+        When passing retrieved pillar data into the state file, it is strongly
+        recommended to use the json serializer explicitly (`` | json``),
+        instead of relying on the default Python serializer.
     '''
     ret = _default_ret(name)
     test = __opts__['test'] or test
     if not filters:
-        filters = {}
+        filters = []
     loaded = __salt__['netacl.load_policy_config'](filters=filters,
+                                                   prepend=prepend,
                                                    pillar_key=pillar_key,
                                                    pillarenv=pillarenv,
                                                    saltenv=saltenv,

From e562b65e6a121f3e40bd10c3223511ca284cf4c6 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 14:43:03 +0000
Subject: [PATCH 11/14] source and destination service can also be unicode

---
 salt/modules/capirca_acl.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index c3b4f3e76432..fad2316196c6 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -316,7 +316,7 @@ def _clean_term_opts(term_opts):
         # firstly we'll process special fields like source_service or destination_services
         # which will inject values directly in the source or destination port and protocol
         if field == 'source_service' and value:
-            if isinstance(value, str):
+            if isinstance(value, basestring):
                 value = _make_it_list(clean_opts, field, value)
             log.debug('Processing special source services:')
             log.debug(value)
@@ -331,11 +331,11 @@ def _clean_term_opts(term_opts):
                                                            'protocol',
                                                            _services[service]['protocol'])
             log.debug('Built source_port field, after processing special source services:')
-            log.debug(clean_opts['source_port'])
+            log.debug(clean_opts.get('source_port'))
             log.debug('Built protocol field, after processing special source services:')
-            log.debug(clean_opts['protocol'])
+            log.debug(clean_opts.get('protocol'))
         elif field == 'destination_service' and value:
-            if isinstance(value, str):
+            if isinstance(value, basestring):
                 value = _make_it_list(clean_opts, field, value)
             log.debug('Processing special destination services:')
             log.debug(value)
@@ -350,9 +350,9 @@ def _clean_term_opts(term_opts):
                                                            'protocol',
                                                            _services[service]['protocol'])
             log.debug('Built source_port field, after processing special destination services:')
-            log.debug(clean_opts['destination_service'])
+            log.debug(clean_opts.get('destination_service'))
             log.debug('Built protocol field, after processing special destination services:')
-            log.debug(clean_opts['protocol'])
+            log.debug(clean_opts.get('protocol'))
         # not a special field, but it has to be a valid one
         elif field in _TERM_FIELDS and value and value != _TERM_FIELDS[field]:
             # if not a special field type

From 870481fe0d3a90796ad220960ee1642beb368237 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 15:25:57 +0000
Subject: [PATCH 12/14] Add get_filter_pillar and get_term_pillar helpers

---
 salt/modules/capirca_acl.py | 85 ++++++++++++++++++++++++++++++++-----
 salt/modules/napalm_acl.py  | 62 +++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 11 deletions(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index fad2316196c6..47512858dff9 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -368,7 +368,7 @@ def _clean_term_opts(term_opts):
     return clean_opts
 
 
-def lookup_element(lst, key):
+def _lookup_element(lst, key):
     '''
     Find an dictionary in a list of dictionaries, given its main key.
     '''
@@ -428,7 +428,7 @@ def _merge_list_of_dict(first, second, prepend=True):
     merged = []
     appended = []
     for ele in first:
-        if lookup_element(second, ele.keys()[0]):
+        if _lookup_element(second, ele.keys()[0]):
             overlaps.append(ele)
         elif prepend:
             merged.append(ele)
@@ -436,10 +436,10 @@ def _merge_list_of_dict(first, second, prepend=True):
             appended.append(ele)
     for ele in second:
         ele_key = ele.keys()[0]
-        if lookup_element(overlaps, ele_key):
+        if _lookup_element(overlaps, ele_key):
             # If theres an overlap, get the value from the first
             # But inserted into the right position
-            ele_val_first = lookup_element(first, ele_key)
+            ele_val_first = _lookup_element(first, ele_key)
             merged.append({ele_key: ele_val_first})
         else:
             merged.append(ele)
@@ -466,12 +466,11 @@ def _get_term_object(filter_name,
     term.name = term_name
     term_opts = {}
     if merge_pillar:
-        acl_pillar_cfg = _get_pillar_cfg(pillar_key,
-                                         saltenv=saltenv,
-                                         pillarenv=pillarenv)
-        filter_pillar_cfg = lookup_element(acl_pillar_cfg, filter_name)
-        term_pillar_cfg = filter_pillar_cfg.get('terms', [])
-        term_opts = lookup_element(term_pillar_cfg, term_name)
+        term_opts = get_term_pillar(filter_name,
+                                    term_name,
+                                    pillar_key=pillar_key,
+                                    saltenv=saltenv,
+                                    pillarenv=pillarenv)
         log.debug('Merging with pillar data:')
         log.debug(term_opts)
         term_opts = _clean_term_opts(term_opts)
@@ -984,7 +983,7 @@ def get_filter_config(platform,
         acl_pillar_cfg = _get_pillar_cfg(pillar_key,
                                          saltenv=saltenv,
                                          pillarenv=pillarenv)
-        filter_pillar_cfg = lookup_element(acl_pillar_cfg, filter_name)
+        filter_pillar_cfg = _lookup_element(acl_pillar_cfg, filter_name)
         filter_options = filter_options or filter_pillar_cfg.pop('options', None)
         if filter_pillar_cfg:
             # Only when it was able to find the filter in the ACL config
@@ -1182,3 +1181,67 @@ def get_policy_config(platform,
                          revision_no=revision_no,
                          revision_date=revision_date,
                          revision_date_format=revision_date_format)
+
+
+def get_filter_pillar(filter_name,
+                      pillar_key='acl',
+                      pillarenv=None,
+                      saltenv=None):
+    '''
+    Helper that can be used inside a state SLS,
+    in order to get the filter configuration given its name.
+
+    filter_name
+        The name of the filter.
+
+    pillar_key
+        The root key of the whole policy config.
+
+    pillarenv
+        Query the master to generate fresh pillar data on the fly,
+        specifically from the requested pillar environment.
+
+    saltenv
+        Included only for compatibility with
+        :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
+    '''
+    pillar_cfg = _get_pillar_cfg(pillar_key,
+                                 pillarenv=pillarenv,
+                                 saltenv=saltenv)
+    return _lookup_element(pillar_cfg, filter_name)
+
+
+def get_term_pillar(filter_name,
+                    term_name,
+                    pillar_key='acl',
+                    pillarenv=None,
+                    saltenv=None):
+    '''
+    Helper that can be used inside a state SLS,
+    in order to get the term configuration given its name,
+    under a certain filter uniquely identified by its name.
+
+    filter_name
+        The name of the filter.
+
+    term_name
+        The name of the term.
+
+    pillar_key: ``acl``
+        The root key of the whole policy config. Default: ``acl``.
+
+    pillarenv
+        Query the master to generate fresh pillar data on the fly,
+        specifically from the requested pillar environment.
+
+    saltenv
+        Included only for compatibility with
+        :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
+    '''
+    filter_pillar_cfg = get_filter_pillar(filter_name,
+                                          pillar_key=pillar_key,
+                                          pillarenv=pillarenv,
+                                          saltenv=saltenv)
+    term_pillar_cfg = filter_pillar_cfg.get('terms', [])
+    term_opts = _lookup_element(term_pillar_cfg, term_name)
+    return term_opts
diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index f0eb2798edfa..2fdfd707bfe2 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -847,3 +847,65 @@ def load_policy_config(filters=None,
                                        commit=commit,
                                        debug=debug,
                                        inherit_napalm_device=napalm_device)  # pylint: disable=undefined-variable
+
+
+def get_filter_pillar(filter_name,
+                      pillar_key='acl',
+                      pillarenv=None,
+                      saltenv=None):
+    '''
+    Helper that can be used inside a state SLS,
+    in order to get the filter configuration given its name.
+
+    filter_name
+        The name of the filter.
+
+    pillar_key
+        The root key of the whole policy config.
+
+    pillarenv
+        Query the master to generate fresh pillar data on the fly,
+        specifically from the requested pillar environment.
+
+    saltenv
+        Included only for compatibility with
+        :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
+    '''
+    return  __salt__['capirca.get_filter_pillar'](filter_name,
+                                                  pillar_key=pillar_key,
+                                                  pillarenv=pillarenv,
+                                                  saltenv=saltenv)
+
+
+def get_term_pillar(filter_name,
+                    term_name,
+                    pillar_key='acl',
+                    pillarenv=None,
+                    saltenv=None):
+    '''
+    Helper that can be used inside a state SLS,
+    in order to get the term configuration given its name,
+    under a certain filter uniquely identified by its name.
+
+    filter_name
+        The name of the filter.
+
+    term_name
+        The name of the term.
+
+    pillar_key: ``acl``
+        The root key of the whole policy config. Default: ``acl``.
+
+    pillarenv
+        Query the master to generate fresh pillar data on the fly,
+        specifically from the requested pillar environment.
+
+    saltenv
+        Included only for compatibility with
+        :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
+    '''
+    return __salt__['capirca.get_term_pillar'](filter_name,
+                                               term_name,
+                                               pillar_key=pillar_key,
+                                               pillarenv=pillarenv,
+                                               saltenv=saltenv)

From 8610a1619e669a4a9acf190be871cd44214524ed Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 15:42:12 +0000
Subject: [PATCH 13/14] Improved and tested netacl examples

---
 salt/states/netacl.py | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/salt/states/netacl.py b/salt/states/netacl.py
index 26bfd2d7a8d9..f05744db1362 100644
--- a/salt/states/netacl.py
+++ b/salt/states/netacl.py
@@ -422,7 +422,7 @@ def term(name,
 
         {%- set filter_name = 'block-icmp' -%}
         {%- set term_name = 'first-term' -%}
-        {%- set my_term_cfg = salt.netacl.get_term_pillar('acl', filter_name, term_name) -%}
+        {%- set my_term_cfg = salt.netacl.get_term_pillar(filter_name, term_name) -%}
 
         update_icmp_first_term:
           netacl.term:
@@ -646,11 +646,13 @@ def filter(name,  # pylint: disable=redefined-builtin
 
     .. code-block:: jinja
 
-        {% set my_filter_cfg = salt.netacl.get_filter_pillar('firewall', 'my-filter') -%}
-        my-filter_state:
+        {%- set filter_name = 'my-filter' -%}
+        {%- set my_filter_cfg = salt.netacl.get_filter_pillar(filter_name, pillar_key='firewall') -%}
+        my_first_filter_state:
           netacl.filter:
-            - filter_name: my-filter
-            - terms: {{ my_filter_cfg | json }}
+            - filter_name: {{ filter_name }}
+            - options: {{ my_filter_cfg['options'] | json }}
+            - terms: {{ my_filter_cfg['terms'] | json }}
             - revision_date: false
             - revision_no: 5
             - debug: true
@@ -659,7 +661,7 @@ def filter(name,  # pylint: disable=redefined-builtin
 
     .. code-block:: yaml
 
-        my-filter_state:
+        my_first_filter_state:
           netacl.filter:
             - filter_name: my-filter
             - merge_pillar: true

From 6f32c24b5e0a215105c411e04b8b49c475f42142 Mon Sep 17 00:00:00 2001
From: Mircea Ulinic <mirucha@cloudflare.com>
Date: Wed, 22 Mar 2017 16:39:34 +0000
Subject: [PATCH 14/14] Lint

---
 salt/modules/capirca_acl.py | 4 ++--
 salt/modules/napalm_acl.py  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/salt/modules/capirca_acl.py b/salt/modules/capirca_acl.py
index 47512858dff9..d354da4f393c 100644
--- a/salt/modules/capirca_acl.py
+++ b/salt/modules/capirca_acl.py
@@ -316,7 +316,7 @@ def _clean_term_opts(term_opts):
         # firstly we'll process special fields like source_service or destination_services
         # which will inject values directly in the source or destination port and protocol
         if field == 'source_service' and value:
-            if isinstance(value, basestring):
+            if isinstance(value, six.string_types):
                 value = _make_it_list(clean_opts, field, value)
             log.debug('Processing special source services:')
             log.debug(value)
@@ -335,7 +335,7 @@ def _clean_term_opts(term_opts):
             log.debug('Built protocol field, after processing special source services:')
             log.debug(clean_opts.get('protocol'))
         elif field == 'destination_service' and value:
-            if isinstance(value, basestring):
+            if isinstance(value, six.string_types):
                 value = _make_it_list(clean_opts, field, value)
             log.debug('Processing special destination services:')
             log.debug(value)
diff --git a/salt/modules/napalm_acl.py b/salt/modules/napalm_acl.py
index 2fdfd707bfe2..2b186065e49f 100644
--- a/salt/modules/napalm_acl.py
+++ b/salt/modules/napalm_acl.py
@@ -871,7 +871,7 @@ def get_filter_pillar(filter_name,
         Included only for compatibility with
         :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
     '''
-    return  __salt__['capirca.get_filter_pillar'](filter_name,
+    return __salt__['capirca.get_filter_pillar'](filter_name,
                                                   pillar_key=pillar_key,
                                                   pillarenv=pillarenv,
                                                   saltenv=saltenv)