Closed sdatta09 closed 1 year ago
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 https://github.com/google/capirca/pull/347#discussion_r1295250867:
@@ -0,0 +1,877 @@ +# Copyright 2011 Google Inc. All Rights Reserved.
please update
In capirca/lib/versa.py https://github.com/google/capirca/pull/347#discussion_r1295251002:
+"""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 https://github.com/google/capirca/pull/347#discussion_r1295251641:
- """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 https://github.com/google/capirca/pull/347#discussion_r1295252485:
+
- _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 https://github.com/google/capirca/pull/347#discussion_r1295253679:
- }
- 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 https://github.com/google/capirca/pull/347#discussion_r1295254239:
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 https://github.com/google/capirca/pull/347#pullrequestreview-1579618264, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARIYF7VJC4L7PQJUOBVTZELXVQF2LANCNFSM6AAAAAA3KNNYLA . You are receiving this because you authored the thread.Message ID: @.***>
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