Repository: google/capirca

Issue:
Support multiple targets per header
Based on https://github.com/google/capirca/wiki/Policy-format#header-section I was under the impression that a header could have multiple targets, for example:
```
header {
  target:: juniper testpol4 inet
  target:: juniper testpol6 inet6
}
term default {
    comment:: "test"
    action:: deny
}
```

But running that only outputs:
```
firewall {
    family inet {
        /*
         ** $Id:$
         ** $Date:$
         ** $Revision:$
         **
         */
        replace: filter testpol4 {
            interface-specific;
            /*
             ** test
             */
            term default {
                then {
                    discard;
                }
            }
        }
    }
}
```
While I would have expected both inet and inet6 filters to be generated.
I'm wondering if it's a regression, bug or was just never supported, but it would be nice to keep policy files as lean as possible.

The workaround is to do:
```
header {
  target:: juniper testpol4 inet
}
#include 'testpol.inc'

header {
  target:: juniper testpol6 inet6
}
#include 'testpol.inc'
```
Which is more complex and creates clutter.

What do you think?

Thanks.



Discussion:
abhindes: Would it be more Capirca-native to support "mixed" option for Juniper, rather than creating a new behavior where multiple targets need to be supported (for different address families).

For example, the way the Cisco generator supports "mixed" - https://github.com/google/capirca/blob/master/doc/generator_patterns.md#when-the-platform-does-not-support-mixed-in-a-single-access-list

Instead of having 

```
header {
  target:: juniper testpol4 inet
  target:: juniper testpol6 inet6
}
term default {
    comment:: "test"
    action:: deny
}
```

You would have -
```
header {
  target:: juniper testpol mixed
}
term default {
    comment:: "test"
    action:: deny
}
```
XioNoX: Using mixed looks cleaner at first. However:
* It doesn't mix v4 and v6 like Cisco but generates 2 filters
* Doesn't let the user chose the filter names (eg. only add the 4 or 6 suffix to the filter name)

If you think that's fine, I can look at updating my PR.

Thanks.
XioNoX: @abhindes is there anything I can do to move forward on that PR? Thanks
abhindes: Hi, sorry for the delay in response.

I believe that the right way to do this in a Capirca native method, would be to support "mixed". Could you elaborate a little on 

> It doesn't mix v4 and v6 like Cisco but generates 2 filters

I believe the Cisco generator also creates 2 filters, one for IPv4 and one for IPv6, when "mixed" is used.

> Doesn't let the user chose the filter names (eg. only add the 4 or 6 suffix to the filter name)

I am not sure of the severity of this issue, we could look into a feature request on renaming filter names if required. If you are rendering an option using "mixed", then it is likely the filter names will be semantically related for IPv6 and for IPv6. 
XioNoX: @abhindes it's indeed much cleaner with the "mixed" option!
The CI errors don't seem to be related to this PR though. Please let me know if there is anything to change.
XioNoX: @abhindes is there anything more I need to do? Thanks
abhindes: Hi, I'll try and take a look at this (and the other issues/PRs), this week, thanks for your patience! :) 
abhindes: Please take a look at this and update test cases as specified in https://github.com/google/capirca/blob/master/doc/generator_patterns.md#when-the-platform-supports-mixed-in-a-single-access-list. 

While Juniper follows the "The desired approach will be to have Capirca output two filters, one for each address family, for platforms that do not support ‘mixed’.", we still require test cases that validate different flows. 

Please see JuniperSRX/JuniperMSMPC generators if you need some guidance on the tests :) 

Patch:
diff --git a/capirca/lib/juniper.py b/capirca/lib/juniper.py
index c0a36ad..bfb0286 100644
--- a/capirca/lib/juniper.py
+++ b/capirca/lib/juniper.py
@@ -163,7 +163,8 @@ class Term(aclgenerator.Term):
                            'daddr': 'ip-destination-address',
                            'protocol': 'ip-protocol',
                            'protocol-except': 'ip-protocol-except',
-                           'tcp-est': 'tcp-flags "(ack|rst)"'}}
+                           'tcp-est': 'tcp-flags "(ack|rst)"'}
+              }
 
   def __init__(self, term, term_type, enable_dsmo, noverbose):
     super().__init__(term)
@@ -857,7 +858,7 @@ class Juniper(aclgenerator.ACLGenerator):
 
   _PLATFORM = 'juniper'
   _DEFAULT_PROTOCOL = 'ip'
-  _SUPPORTED_AF = set(('inet', 'inet6', 'bridge'))
+  _SUPPORTED_AF = set(('inet', 'inet6', 'bridge', 'mixed'))
   _TERM = Term
   SUFFIX = '.jcl'
 
@@ -942,42 +943,57 @@ class Juniper(aclgenerator.ACLGenerator):
       if len(filter_options) > 1:
         filter_type = filter_options[1]
 
-      term_names = set()
-      new_terms = []
-      for term in terms:
+      if filter_type == 'mixed':
+        filter_types_to_process = ['inet', 'inet6']
+      else:
+        filter_types_to_process = [filter_type]
 
-        # if inactive is set, deactivate the term and remove the option.
-        if 'inactive' in term.option:
-          term.inactive = True
-          term.option.remove('inactive')
-
-        term.name = self.FixTermLength(term.name)
-
-        if term.name in term_names:
-          raise JuniperDuplicateTermError('You have multiple terms named: %s' %
-                                          term.name)
-        term_names.add(term.name)
-
-        term = self.FixHighPorts(term, af=filter_type)
-        if not term:
-          continue
-
-        if term.expiration:
-          if term.expiration <= exp_info_date:
-            logging.info('INFO: Term %s in policy %s expires '
-                         'in less than two weeks.', term.name, filter_name)
-          if term.expiration <= current_date:
-            logging.warning('WARNING: Term %s in policy %s is expired and '
-                            'will not be rendered.', term.name, filter_name)
-            continue
-        if 'is-fragment' in term.option and filter_type == 'inet6':
-          raise JuniperFragmentInV6Error('The term %s uses "is-fragment" but '
-                                         'is a v6 policy.' % term.name)
+      for filter_type in filter_types_to_process:
+
+        filter_name_suffix = ''
+        # If mixed filter_type, will append 4 or 6 to the filter name
+        if len(filter_types_to_process) > 1:
+          if filter_type == 'inet':
+            filter_name_suffix = '4'
+          if filter_type == 'inet6':
+            filter_name_suffix = '6'
+
+        term_names = set()
+        new_terms = []
+        for term in terms:
 
-        new_terms.append(self._TERM(term, filter_type, enable_dsmo, noverbose))
+          # if inactive is set, deactivate the term and remove the option.
+          if 'inactive' in term.option:
+            term.inactive = True
+            term.option.remove('inactive')
+
+          term.name = self.FixTermLength(term.name)
+
+          if term.name in term_names:
+            raise JuniperDuplicateTermError('You have multiple terms named: %s' %
+                                            term.name)
+          term_names.add(term.name)
+
+          term = self.FixHighPorts(term, af=filter_type)
+          if not term:
+            continue
 
-      self.juniper_policies.append((header, filter_name, filter_type,
-                                    interface_specific, new_terms))
+          if term.expiration:
+            if term.expiration <= exp_info_date:
+              logging.info('INFO: Term %s in policy %s expires '
+                          'in less than two weeks.', term.name, filter_name)
+            if term.expiration <= current_date:
+              logging.warning('WARNING: Term %s in policy %s is expired and '
+                              'will not be rendered.', term.name, filter_name)
+              continue
+          if 'is-fragment' in term.option and filter_type == 'inet6':
+            raise JuniperFragmentInV6Error('The term %s uses "is-fragment" but '
+                                          'is a v6 policy.' % term.name)
+
+          new_terms.append(self._TERM(term, filter_type, enable_dsmo, noverbose))
+
+        self.juniper_policies.append((header, filter_name + filter_name_suffix, filter_type,
+                                      interface_specific, new_terms))
 
   def __str__(self):
     config = Config()


Test patch:
diff --git a/tests/lib/juniper_test.py b/tests/lib/juniper_test.py
index 2846360..95557c3 100644
--- a/tests/lib/juniper_test.py
+++ b/tests/lib/juniper_test.py
@@ -20,6 +20,7 @@ from absl.testing import absltest
 from unittest import mock
 
 from absl import logging
+from absl.testing import parameterized
 from capirca.lib import aclgenerator
 from capirca.lib import juniper
 from capirca.lib import nacaddr
@@ -42,6 +43,11 @@ header {
   target:: juniper test-filter inet6
 }
 """
+GOOD_HEADER_MIXED = """
+header {
+  target:: juniper test-filter mixed
+}
+"""
 GOOD_HEADER_BRIDGE = """
 header {
   target:: juniper test-filter bridge
@@ -533,6 +539,16 @@ term flex-match-term-1 {
 }
 """
 
+MIXED_TESTING_TERM = """
+term good-term {
+  protocol:: tcp
+  source-address:: SOME_HOST
+  destination-port:: SMTP
+  destination-address:: SOME_OTHER_HOST
+  action:: accept
+}
+"""
+
 SUPPORTED_TOKENS = frozenset([
     'action',
     'address',
@@ -645,7 +661,7 @@ SUPPORTED_SUB_TOKENS = {
 EXP_INFO = 2
 
 
-class JuniperTest(absltest.TestCase):
+class JuniperTest(parameterized.TestCase):
 
   def setUp(self):
     super().setUp()
@@ -1533,6 +1549,136 @@ class JuniperTest(absltest.TestCase):
                       GOOD_HEADER_V6 + BAD_FLEX_MATCH_TERM_4,
                       self.naming)
 
+  @parameterized.named_parameters(
+      ('MIXED_TO_V4',
+       [[nacaddr.IPv4('0.0.0.0/1'),
+         nacaddr.IPv6('2001::/33')], [nacaddr.IPv4('192.168.0.0/24')]], [
+             '            term good-term {\n' +
+             '                from {\n' +
+             '                    source-address {\n' +
+             '                        0.0.0.0/1;\n' +
+             '                    }\n' +
+             '                    destination-address {\n' +
+             '                        192.168.0.0/24;\n' +
+             '                    }'
+         ], ['2001::/33']),
+      ('V4_TO_MIXED', [
+          [nacaddr.IPv4('192.168.0.0/24')],
+          [nacaddr.IPv4('0.0.0.0/1'),
+           nacaddr.IPv6('2001::/33')],
+      ], [
+          '            term good-term {\n' +
+          '                from {\n' +
+          '                    source-address {\n' +
+          '                        192.168.0.0/24;\n' +
+          '                    }\n' +
+          '                    destination-address {\n' +
+          '                        0.0.0.0/1;\n' +
+          '                    }'
+      ], ['2001::/33']),
+      ('MIXED_TO_V6',
+       [[nacaddr.IPv4('0.0.0.0/1'),
+         nacaddr.IPv6('2001::/33')], [nacaddr.IPv6('2201::/48')]], [
+             '            term good-term {\n' +
+             '                from {\n' +
+             '                    source-address {\n' +
+             '                        2001::/33;\n' +
+             '                    }\n' +
+             '                    destination-address {\n' +
+             '                        2201::/48;\n' +
+             '                    }'
+         ], ['0.0.0.0/1']),
+      ('V6_TO_MIXED', [[
+          nacaddr.IPv6('2201::/48')
+      ], [nacaddr.IPv4('0.0.0.0/1'),
+          nacaddr.IPv6('2001::/33')]], [
+              '            term good-term {\n' +
+              '                from {\n' +
+              '                    source-address {\n' +
+              '                        2201::/48;\n' +
+              '                    }\n' +
+              '                    destination-address {\n' +
+              '                        2001::/33;\n' +
+              '                    }'
+        ], ['0.0.0.0/1']),
+      ('MIXED_TO_MIXED', [[
+          nacaddr.IPv4('0.0.0.0/1'),
+          nacaddr.IPv6('2001::/33')
+      ], [nacaddr.IPv4('192.168.0.0/24'),
+          nacaddr.IPv6('2201::/48')]], [
+              '            term good-term {\n' +
+              '                from {\n' +
+              '                    source-address {\n' +
+              '                        0.0.0.0/1;\n' +
+              '                    }\n' +
+              '                    destination-address {\n' +
+              '                        192.168.0.0/24;\n' +
+              '                    }',
+              '            term good-term {\n' +
+              '                from {\n' +
+              '                    source-address {\n' +
+              '                        2001::/33;\n' +
+              '                    }\n' +
+              '                    destination-address {\n' +
+              '                        2201::/48;\n' +
+              '                    }'
+        ], []),
+      ('V4_TO_V4', [[nacaddr.IPv4('0.0.0.0/1')],
+                    [nacaddr.IPv4('192.168.0.0/24')]], [
+                        '            term good-term {\n' +
+                        '                from {\n' +
+                        '                    source-address {\n' +
+                        '                        0.0.0.0/1;\n' +
+                        '                    }\n' +
+                        '                    destination-address {\n' +
+                        '                        192.168.0.0/24;\n' +
+                        '                    }'
+      ], []),
+      ('V6_TO_V6', [[nacaddr.IPv6('2001::/33')], [nacaddr.IPv6('2201::/48')]], [
+          '            term good-term {\n' +
+          '                from {\n' +
+          '                    source-address {\n' +
+          '                        2001::/33;\n' +
+          '                    }\n' +
+          '                    destination-address {\n' +
+          '                        2201::/48;\n' +
+          '                    }'
+      ], []),
+      (
+          'V4_TO_V6',
+          [[nacaddr.IPv4('0.0.0.0/1')], [nacaddr.IPv6('2201::/48')]],
+          [],
+          ['0.0.0.0/1', '192.168.0.0/24', '2001::/33', '2201::/48'],
+      ),
+      (
+          'V6_TO_V4',
+          [[nacaddr.IPv6('2001::/33')], [nacaddr.IPv4('192.168.0.0/24')]],
+          [],
+          ['0.0.0.0/1', '192.168.0.0/24', '2001::/33', '2201::/48'],
+      ),
+      (
+          'PARTLY_UNSPECIFIED',
+          [[nacaddr.IPv6('2001::/33')], [nacaddr.IPv4('192.168.0.0/24')]],
+          ['term good_term_25 '],
+          [
+              '0.0.0.0/1', '192.168.0.0/24', '2001::/33', '2201::/48',
+              'term good-term-both-icmp-and-icmpv6-'
+          ],
+      ),
+  )
+  def testMixed(self, addresses, expected, notexpected):
+    self.naming.GetNetAddr.side_effect = addresses
+    self.naming.GetServiceByProto.return_value = ['25']
+    jcl = juniper.Juniper(
+        policy.ParsePolicy(
+            GOOD_HEADER_MIXED + MIXED_TESTING_TERM + GOOD_TERM_25, self.naming),
+        EXP_INFO)
+    output = str(jcl)
+    for expect in expected:
+      self.assertIn(expect, output, output)
+    for notexpect in notexpected:
+      self.assertNotIn(notexpect, output, output)
+
 
 if __name__ == '__main__':
   absltest.main()