Initial development for versa application to reserve enum. #347

sdatta09 · 2023-08-09T18:52:05Z

Enum added for Versa App development since Versa routers are service aware router like Paolo Alto Routers.
Once this Enum is reviewed and approved , other changes can be checked in.
pylint has been run and no new warnings introduced by the code change

pylint has been run and no new warnings introduced.

pylint run : no warnings versa_test.py: tests for Versa acls added pylint run. Only see unused import warnings. Tests were run without any errors versa.md: Documentation for Versa ACLs Please note that these are initial commits. More commits wil be done as needed.

abhindes

I'm still going through this.

abhindes · 2023-08-16T00:01:26Z

capirca/lib/versa.py

@@ -0,0 +1,877 @@
+# Copyright 2011 Google Inc. All Rights Reserved.


please update

Updated in the next commit

abhindes · 2023-08-16T00:01:48Z

capirca/lib/versa.py

+#import six
+
+
+ICMP_TERM_LIMIT = 8


Is this being used?

abhindes · 2023-08-16T00:03:05Z

capirca/lib/versa.py

+
+    if self.term.versa_application:
+      predef_str = 'predefined-services-list ['
+      for predef in self.term.versa_application:


We probably need to do some validation checking here on what valid versa applications are.

versa.application is a list and it is check in Line 237

abhindes · 2023-08-16T00:05:07Z

capirca/lib/versa.py

+                                         'tenant',
+                                         'policy'))
+  _VERSA_UNSUPPORTED_TERM_OPTIONS = set(('icmp_type',
+                                         'stateless_reply',


stateless_reply fields are used to skip the reply flows if the firewall is stateful. Since Versa is stateful, we can remove this from here.

Not being used. Deleted

abhindes · 2023-08-16T00:08:08Z

capirca/lib/versa.py

+    if len(val) > 0:
+      if 'template' in mstr:
+        self.templatename = val
+      elif 'tenant' in mstr:


Is tenant a required field? Could you provide an example of what might be populated in here?

Added a skeleton of the config.

abhindes · 2023-08-16T00:09:24Z

capirca/lib/versa.py

+        # we need to remove icmp from the protocol and add it to the
+        #pan-application list
+        if term.protocol and 'icmp' in ' '.join(term.protocol):
+          term.protocol.remove('icmp')
+          term.versa_application.append('ICMP')
+        # Because Versa terms can contain inet and inet6 addresses. We have to
+        # have ability to recover proper AF for ICMP type we need.
+        # If protocol is empty or we cannot map to inet or inet6 we insert bogus
+        # af_type name which will cause new_term.NormalizeIcmpTypes to fail.
+
+        #if not term.protocol:
+        #  icmp_af_type = 'unknown_af_icmp'
+        #else:
+        #  icmp_af_type = self._AF_ICMP_MAP.get(
+        #      term.protocol[0], 'unknown_af_icmp')
+        #tmp_icmptype = new_term.NormalizeIcmpTypes(
+        #    term.icmp_type, term.protocol, icmp_af_type)
+


Some cleanup necessary here to support icmpv6 correctly.

will deal with icmpv6 in the next round

sdatta09 · 2023-08-16T00:27:01Z

Abhineet There are a few extraneous things I need to take care of. However, I wanted to not delay my initial commit. I will take in account all the comments that you provide in the next set of commits. Thanks Sujay Datta

…

On Tue, Aug 15, 2023, 19:11 abhindes ***@***.***> wrote: ***@***.**** commented on this pull request. I'm still going through this. ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > @@ -0,0 +1,877 @@ +# Copyright 2011 Google Inc. All Rights Reserved. please update ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > +"""Versa generator.""" +# pylint: disable=super-init-not-called + + +import collections +import copy +#import datetime +import itertools + +from absl import logging +from capirca.lib import aclgenerator +from capirca.lib import nacaddr +#import six + + +ICMP_TERM_LIMIT = 8 Is this being used? ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > + """Build Term app""" + mstr = [] + if self.app: + apps = list(filter(lambda x: x['name'] == self.term.name, self.app)) + if len(apps) > 0: + slist = '' + for i in range(0,len(apps)): + for j in range(0,len(apps[i]['protocol'])): + slist = slist + ' ' + self.term.name + '-app' + str(j+1) + if len(slist) > 0: + slist = 'services-list [' + slist + ' ];' + mstr.append(slist) + + if self.term.versa_application: + predef_str = 'predefined-services-list [' + for predef in self.term.versa_application: We probably need to do some validation checking here on what valid versa applications are. ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > + + _PLATFORM = 'versa' + SUFFIX = '.vsp' + _SUPPORTED_AF = set(('inet', 'inet6', 'mixed')) + _ZONE_ADDR_BOOK = 'address-book-zone' + _GLOBAL_ADDR_BOOK = 'address-book-global' + _ADDRESSBOOK_TYPES = set((_ZONE_ADDR_BOOK, _GLOBAL_ADDR_BOOK)) + _NOVERBOSE = 'noverbose' + _SUPPORTED_TARGET_OPTIONS = set((_ZONE_ADDR_BOOK, + _GLOBAL_ADDR_BOOK, + _NOVERBOSE)) + _VERSA_SUPPORTED_TARGET_OPTIONS = set(('template', + 'tenant', + 'policy')) + _VERSA_UNSUPPORTED_TERM_OPTIONS = set(('icmp_type', + 'stateless_reply', stateless_reply fields are used to skip the reply flows if the firewall is stateful. Since Versa is stateful, we can remove this from here. ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > + } + + supported_sub_tokens.update( + {'action': {'accept', 'deny', 'reject', 'count', 'log', 'dscp'}, + }) + + del supported_sub_tokens['option'] + return supported_tokens, supported_sub_tokens + + + def HeaderParams(self, mstr, val): + """HeaderParams populates the template name and tenant name.""" + if len(val) > 0: + if 'template' in mstr: + self.templatename = val + elif 'tenant' in mstr: Is tenant a required field? Could you provide an example of what might be populated in here? ------------------------------ In capirca/lib/versa.py <#347 (comment)>: > + # we need to remove icmp from the protocol and add it to the + #pan-application list + if term.protocol and 'icmp' in ' '.join(term.protocol): + term.protocol.remove('icmp') + term.versa_application.append('ICMP') + # Because Versa terms can contain inet and inet6 addresses. We have to + # have ability to recover proper AF for ICMP type we need. + # If protocol is empty or we cannot map to inet or inet6 we insert bogus + # af_type name which will cause new_term.NormalizeIcmpTypes to fail. + + #if not term.protocol: + # icmp_af_type = 'unknown_af_icmp' + #else: + # icmp_af_type = self._AF_ICMP_MAP.get( + # term.protocol[0], 'unknown_af_icmp') + #tmp_icmptype = new_term.NormalizeIcmpTypes( + # term.icmp_type, term.protocol, icmp_af_type) + Some cleanup necessary here. — Reply to this email directly, view it on GitHub <#347 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARIYF7VJC4L7PQJUOBVTZELXVQF2LANCNFSM6AAAAAA3KNNYLA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

abhindes · 2023-08-16T21:34:15Z

capirca/lib/versa.py

+    return self.target
+
+  def PrintTreeInt(self,num=0):
+    """Internal function to the tree. Does recursion"""


We might need to check if the default recursion depths are sufficient for large policies here. Might want to move away from recursion if needed.

Already verified with multiple tests.

abhindes · 2023-08-16T23:41:41Z

capirca/lib/versa.py

+    self.addrbook = addrbook
+    self.app = []
+
+    if term.source_zone:


qq on Versa: Is intra-Zone traffic permitted by default, or denied by default?

abhindes · 2023-08-16T23:43:34Z

capirca/lib/versa.py

+    if self.term.action:
+      self.BuildTermLogging(access_pn)
+
+    #print("\n".join(set_term.PrintTree()))


nit: remove

abhindes · 2023-08-16T23:44:44Z

capirca/lib/versa.py

+    #current_date = datetime.datetime.utcnow().date()
+    #exp_info_date = current_date + datetime.timedelta(weeks=exp_info)


nit: remove

abhindes · 2023-08-16T23:45:03Z

capirca/lib/versa.py

+      #if self._NOVERBOSE in filter_options[4:]:
+      #  verbose = False
+
+      # TODO(robankeny): Clean up option section.


nit: remove

abhindes · 2023-08-16T23:47:38Z

capirca/lib/versa.py

+        # we need to remove icmp from the protocol and add it to the
+        #pan-application list


nit: remove

abhindes · 2023-08-16T23:54:55Z

tests/lib/versa_test.py

@@ -0,0 +1,476 @@
+# Copyright 2012 Google Inc. All Rights Reserved.


abhindes · 2023-08-16T23:56:20Z

tests/lib/versa_test.py

+    pol = policy.ParsePolicy(GOOD_HEADER_1 + ICMP_TYPE_TERM_1 , self.naming)
+    self.assertRaises(versa.VersaUnsupportedTerm, versa.Versa, pol, EXP_INFO)
+
+  def testIcmpV6(self):


We'll want to add some tests later to meet https://github.com/google/capirca/blob/master/doc/generator_patterns.md#icmp-and-icmpv6-handling

abhindes · 2023-08-16T23:57:59Z

tests/lib/versa_test.py

+_IPSET5 = [nacaddr.IP('10.0.0.0/24')]
+
+
+class VersaTest(absltest.TestCase):


We probably will need to add some more unit test coverage. The generator has a ton of different options and it will help to have more cases covered - https://github.com/google/capirca/blob/master/doc/generator_patterns.md#test-coverage

abhindes

I've done a second round of reviews and left comments. We can follow up on these.

Changes for expiration, platform, platform-exclude, logging Added tests for expiration, platform, platform-exclude, multiple terms, logging ipv4/v6. Renamed terms for better understanding. Ran pylint on versa.py and versa-test.py

Added Versa policy file and verified using aclgen.py

…ort. Fixes #347. PiperOrigin-RevId: 564745184

sdatta09 added 2 commits August 9, 2023 07:07

Initial development for versa application to reserve enum.

5791f34

pylint has been run and no new warnings introduced.

abhindes reviewed Aug 16, 2023

View reviewed changes

abhindes approved these changes Aug 17, 2023

View reviewed changes

sdatta09 added 2 commits August 21, 2023 08:48

Took care of comments.

c88362a

Changes for expiration, platform, platform-exclude, logging Added tests for expiration, platform, platform-exclude, multiple terms, logging ipv4/v6. Renamed terms for better understanding. Ran pylint on versa.py and versa-test.py

Changed licensing information

a92e4db

Added Versa policy file and verified using aclgen.py

copybara-service bot merged commit d145ca4 into google:master Aug 23, 2023
1 of 7 checks passed

copybara-service bot pushed a commit that referenced this pull request Sep 12, 2023

nsxt: Remove 0.0.0.0/... from rules. Replace 1-member ranges with p…

9e9812e

…ort. Fixes #347. PiperOrigin-RevId: 564745184

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial development for versa application to reserve enum. #347

Initial development for versa application to reserve enum. #347

sdatta09 commented Aug 9, 2023

abhindes left a comment

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023 •

edited

Loading

sdatta09 Aug 17, 2023

sdatta09 commented Aug 16, 2023 via email

abhindes Aug 16, 2023 •

edited

Loading

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes Aug 16, 2023

sdatta09 Aug 17, 2023

abhindes left a comment

		@@ -0,0 +1,877 @@
		# Copyright 2011 Google Inc. All Rights Reserved.

		#current_date = datetime.datetime.utcnow().date()
		#exp_info_date = current_date + datetime.timedelta(weeks=exp_info)

		# we need to remove icmp from the protocol and add it to the
		#pan-application list

		@@ -0,0 +1,476 @@
		# Copyright 2012 Google Inc. All Rights Reserved.

		_IPSET5 = [nacaddr.IP('10.0.0.0/24')]


		class VersaTest(absltest.TestCase):

		#import six


		ICMP_TERM_LIMIT = 8

Initial development for versa application to reserve enum. #347

Initial development for versa application to reserve enum. #347

Conversation

sdatta09 commented Aug 9, 2023

abhindes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhindes Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdatta09 commented Aug 16, 2023 via email

abhindes Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhindes left a comment

Choose a reason for hiding this comment

abhindes Aug 16, 2023 •

edited

Loading

abhindes Aug 16, 2023 •

edited

Loading