Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds icmp type and code to Cisco object-group output #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abates
Copy link
Contributor

@abates abates commented Mar 4, 2022

icmp-type and icmp-code were not being rendered in the
ACL when using the object-group feature of the Cisco
renderer. This commit adds icmp type and code to the
output.

Fixes #298

icmp-type and icmp-code were not being rendered in the
ACL when using the object-group feature of the Cisco
renderer. This commit adds icmp type and code to the
output.
Copy link
Collaborator

@abhindes abhindes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this PR!

if self.platform == 'cisconx':
if self.af == 6:
proto = 'icmp' if proto == 'icmpv6' else proto
for icmp_type in icmp_types:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than looping here, making an explicit decision that this is for "icmp" handling only would make this correct, and readable.

In the case of icmp/icmpv6, this would handle it with the way you have, with self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport, icmp_type, icmp_code)

Else, it would handle it the way it originally was.
self._TermletToStr(_ACTION_TABLE.get(str(
self.term.action[0])), proto, saddr, sport, daddr, dport))

@@ -656,6 +673,21 @@ def testObjectGroup(self):
self.naming.GetNetAddr.assert_has_calls([mock.call('SOME_HOST'),
mock.call('SOME_HOST')])
self.naming.GetServiceByProto.assert_called_once_with('HTTP', 'tcp')

def testObjectGroupIcmp(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also please add a testcase for icmpv6 here, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cisco object-group ACLs don't properly render ICMP type and code
2 participants