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

Ipv6 support #1905

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

brunopeter
Copy link

  • added support for ipv6 network and service groups including ipv6 addresses and masks

@mjbear
Copy link
Contributor

mjbear commented Nov 20, 2024

@brunopeter
This PR needs test data for the template changes that have been made.
Add another file of raw test data (which contains IPv6 objects) and its parsed equivalent.

@brunopeter
Copy link
Author

added raw test data and parsed

@mjbear
Copy link
Contributor

mjbear commented Nov 21, 2024

added raw test data and parsed

Thank you.

Functionally, is there an advantage to separate ICMP and ICMPv6 types?

@brunopeter
Copy link
Author

@mjbear no functional reason beyond the number of types is large and separating them made some of protocol differences more clear.

@mjbear
Copy link
Contributor

mjbear commented Nov 22, 2024

@mjbear no functional reason beyond the number of types is large and separating them made some of protocol differences more clear.

Makes sense. Thank you for replying.
I wonder if the template even needs the specific ICMP type names or if that could be a simpler regex pattern instead.

I may have to give that a try to see if it breaks anything, haha.
Of course it's also up for input from the project maintainers too. 😁

@brunopeter
Copy link
Author

@mjbear no functional reason beyond the number of types is large and separating them made some of protocol differences more clear.

Makes sense. Thank you for replying.

I wonder if the template even needs the specific ICMP type names or if that could be a simpler regex pattern instead.

I may have to give that a try to see if it breaks anything, haha.

Of course it's also up for input from the project maintainers too. 😁

If it's like the service objects, cisco will replace numbers with the name e.g. service 80 becomes service http.

PORT_RANGE_END: ''
PORT_RANGE_START: ''
PROTOCOL: ''
TYPE: V6-Network
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TYPE: V6-Network
TYPE: V6-Network

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the TYPE be a double quoted string too?

TYPE: "V6-Network"

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be obsoleted by brunopeter/pull/1

Copy link
Contributor

@jmcgill298 jmcgill298 left a comment

Choose a reason for hiding this comment

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

Thanks @brunopeter just a couple of minor whitespace fixes, and will need to update the YAML file to pass linting. There are some helper scripts to fix it up for you: https://ntc-templates.readthedocs.io/en/latest/dev/dev_parser/#development-helper-scripts

Comment on lines +10 to +138
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: echo-reply
NAME: TEST-v6-icmp
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Service
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: 2001:DB8::1:1111
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: 2001:DB8::2:2222
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: /48
NETWORK: "2001:DB8:111:1::"
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: /48
NETWORK: "2001:DB8:222:2::"
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network

This comment was marked as outdated.

Copy link
Contributor

@mjbear mjbear Nov 27, 2024

Choose a reason for hiding this comment

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

@brunopeter
I didn't realize this until now, but the keys in your YAML are in uppercase but they should be lowercase.

I'm going to work up a PR against your ipv6-support branch that you can easily merge in to fix this up.
Thank you!

Copy link
Contributor

@mjbear mjbear Nov 27, 2024

Choose a reason for hiding this comment

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

Canceling/hiding this inline review as they're obsoleted by brunopeter/pull/1
Please review that PR and merge it in. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

@brunopeter I think the fixes in this PR should have the tests ready to go through CI

Value NETWORK (\d+\.\d+\.\d+\.\d+)
Value NETMASK (\d+\.\d+\.\d+\.\d+)
Value NETWORK ((\d+\.\d+\.\d+\.\d+)|([A-Fa-f0-9:]+:+[A-Fa-f0-9:]+))
Value NETMASK ((\d+\.\d+\.\d+\.\d+)|((?:\/\d{1,3})))

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Obsoleted by brunopeter/pull/1

^${TYPE}\s+object\s+group\s+${NAME}\s*$$ -> Record
^\s+Description\s+${DESCRIPTION}$$ -> Record
^\s+group-object\s+${NESTED_GROUPS}\s*$$ -> Record
^\s+(host\s+${HOST}|range\s+${HOST_RANGE_START}\s+${HOST_RANGE_END}|${ANY}|${NETWORK}\s+${NETMASK})\s*$$ -> Record
^\s+icmp\s+${ICMP_TYPE}\s*$$ -> Record
^\s+(host\s+${HOST}|range\s+${HOST_RANGE_START}\s+${HOST_RANGE_END}|${ANY}|${NETWORK}\s*${NETMASK})\s*$$ -> Record

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Obsoleted by brunopeter/pull/1

@mjbear
Copy link
Contributor

mjbear commented Nov 27, 2024

@brunopeter
Lots going on here so my apologies for the flurry of things.

I've pulled together a PR against your fork of ntc-templates so you can merge some changes into your ipv6-support branch. Please review that PR and merge it into your ipv6-branch once you've read through it and follow the changes.

Any questions or comments, put them on the discussion for the PR brunopeter/pull/1 over at your forked repo.

Comment on lines +10 to +138
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: echo-reply
NAME: TEST-v6-icmp
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Service
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: 2001:DB8::1:1111
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: 2001:DB8::2:2222
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: ""
NETWORK: ""
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: /48
NETWORK: "2001:DB8:111:1::"
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
- ANY: ""
DESCRIPTION: ""
HOST: ""
HOST_RANGE_END: ""
HOST_RANGE_START: ""
ICMP6_TYPE: ""
ICMP_TYPE: ""
NAME: TEST-v6-obj
NESTED_GROUPS: ""
NETMASK: /48
NETWORK: "2001:DB8:222:2::"
PORT: ""
PORT_MATCH: ""
PORT_RANGE_END: ""
PORT_RANGE_START: ""
PROTOCOL: ""
TYPE: V6-Network
Copy link
Contributor

Choose a reason for hiding this comment

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

@brunopeter I think the fixes in this PR should have the tests ready to go through CI

@mjbear
Copy link
Contributor

mjbear commented Dec 2, 2024

@brunopeter Lots going on here so my apologies for the flurry of things.

I've pulled together a PR against your fork of ntc-templates so you can merge some changes into your ipv6-support branch. Please review that PR and merge it into your ipv6-branch once you've read through it and follow the changes.

Any questions or comments, put them on the discussion for the PR brunopeter/pull/1 over at your forked repo.

@brunopeter
Have you seen this?

@mjbear
Copy link
Contributor

mjbear commented Dec 11, 2024

@brunopeter Lots going on here so my apologies for the flurry of things.
I've pulled together a PR against your fork of ntc-templates so you can merge some changes into your ipv6-support branch. Please review that PR and merge it into your ipv6-branch once you've read through it and follow the changes.
Any questions or comments, put them on the discussion for the PR brunopeter/pull/1 over at your forked repo.

@brunopeter Have you seen this?

Hi @brunopeter
Could you take a look at this when you have time?
Thank you!

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.

3 participants