google / capirca

Multi-platform ACL generation system
Apache License 2.0
770 stars 208 forks source link

Add Cisco Nexus support #39

Closed GoogleCodeExporter closed 7 years ago

GoogleCodeExporter commented 9 years ago
Cisco Nexus series is currently unsupported. With sed I can change most of the 
problematic ace's, but it would be great if it could be included natively.

Original issue reported on code.google.com by smma...@gmail.com on 27 May 2015 at 8:12

GoogleCodeExporter commented 9 years ago
Hi,
Can you give me more details on what is different between the syntax?
I do not have a nexus device to test things out on.

Original comment by anke...@gmail.com on 23 Jun 2015 at 5:26

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Our Nexus acl's are quite simple. The differences i found until now:

- Keyword 'extended' needs to be removed
- Protocol numbers need to be replaced by their respective name (eg 6 is tcp, 
17 is udp etc)
- Only when using in switchprofile mode (in that case acl's are automatically 
distributed between multiple switches) it is necessary to number the list 
entries. But not every Nexus user uses that i guess.

A (very) simple and dirty conversion script i use at the moment:
./aclgen.py
find ./ -iname '*.acl' | while read f
do
sed  -i 's/ extended / /g' "$f"
sed  -i 's/ 6 / tcp /g' "$f"
sed  -i 's/ 17 / udp /g' "$f"
head -n 4 "$f" > "$f".head
sed '1,4d' "$f" > "$f".sed 
nl -i10 -v10 -s "  " "$f".sed | grep -v " end" >> "$f".head
rm "$f".sed
mv "$f".head "$f"
done

Original comment by smma...@gmail.com on 24 Jun 2015 at 1:41

nyanto commented 9 years ago

One other difference is, NX-OS supports CIDR notation for subnets. I do have a language generator for NX-OS - I have uploaded the relevant diffs to my git tree at https://github.com/nyanto/capirca - feel free to check it out. The module is called cisconx.py. There are quite a few other changes to the cisco.py module which we use but are unrelated to the cisconx module - unfortunately, some minimal changes to the cisco.py module are required for the cisconx.py module to work. I can break those out in a smaller patch if needed.

ankenyr commented 8 years ago

Not sure if this is relevant anymore. This issue was imported from the old code.google.com site and I am unsure if the original person is looking at these comments.

@nyanto I have not worked with the NX series much but is CIDR notation actually required? ciscoxr.py and cisconx.py look awfully similar besides for just that.

nyanto commented 8 years ago

I don't think the CIDR notation is required to enter an ACL on NX. But it is converted to CIDR in the configuration, and we have a verification step where we require the configured ACL on the device to match the filter. I agree with your assessment that this issue is probably not too relevant to most people.

On Wed, Apr 20, 2016 at 12:52 PM, ankenyr notifications@github.com wrote:

Not sure if this is relevant anymore. This issue was imported from the old code.google.com site and I am unsure if the original person is looking at these comments.

@nyanto https://github.com/nyanto I have not worked with the NX series much but is CIDR notation actually required? ciscoxr.py and cisconx.py look awfully similar besides for just that.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/google/capirca/issues/39#issuecomment-212578135

ankenyr commented 8 years ago

Actually that is good information to know. We have changed our generators before because they were outputting things that, while valid, would change when requesting the config after commit.

I have not worked on the XR systems myself but do they also take CIDR notation? If so then maybe unifying the XR and NX code and making a platform option would be a better way to handle this?

nyanto commented 8 years ago

In my experience XR uses a syntax which is much closer to traditional Cisco IOS - in our deployments, we have used the regular cisco generator and massaged the output with a few sed string substitutions... For consistency, I would probably like to see either all three (IOS, XR, NX) handled as part of the cisco generator, or, XR and NX generators derived from the generic cisco class, which is how I had implemented my NX generator.

On Thu, Apr 21, 2016 at 3:05 PM, ankenyr notifications@github.com wrote:

Actually that is good information to know. We have changed our generators before because they were outputting things that, while valid, would change when requesting the config after commit.

I have not worked on the XR systems myself but do they also take CIDR notation? If so then maybe unifying the XR and NX code and making a platform option would be a better way to handle this?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/google/capirca/issues/39#issuecomment-213131534

jzohrab commented 8 years ago

We need this as well. I've taken a single commit from @nyanto's fork, commit c2d6c8 and am working on it now in a branch. This commit breaks some of the regression tests I added in PR #56, so I'll work with these changes to ensure they're backwards compatible with the existing code (that is, have the same behaviour), and will add some tests for Nexus ACL generation. I'll work with a teammate re the tests for these.

I'll push these changes up to my fork, at branch https://github.com/jzohrab/capirca/commits/wip_nxos. I'll rebase everything when it's done, at the moment it's just hacking.

@nyanto thanks for commenting on this PR!

smmadge commented 8 years ago

I am the original poster. Would still come in handy, but as said before: for my requirements sed works fine at the moment.

jzohrab commented 8 years ago

I'm going to work on merging Capirca's code into mine, for release back up to Capirca master. This will include the Nexus code.

jzohrab commented 8 years ago

Added nyanto's code, with some modifications, in PR #68.

syyna commented 8 years ago

Any chance this is going to get merged into the maintree? I see that the main tree still has no nexus support.

Thanks

ankenyr commented 8 years ago

Hi @usagichan The code currently exists in #68 but since that builds ontop of other PRs I cannot accept it in the current form. I have not had time to take a look at those PRs. If @jzohrab or @nyanto want to send me a PR with just that code I should be able to accept it much easier.

smmadge commented 8 years ago

Is there any progress on the NXOS merge/integration yet?

ankenyr commented 8 years ago

Not much yet, got held up with some work and a different PR. Will try to pull those changes in once we resolve that as it changes some of the setup of a generator.

sjtarik commented 7 years ago

@ankenyr I just ported the NXOS part of @jzohrab commit with some basic tests. Could you take a look at it here?

ankenyr commented 7 years ago

Will take a look in the new year.

On Dec 30, 2016 7:05 AM, "tarik" notifications@github.com wrote:

@ankenyr https://github.com/ankenyr I just ported the NXOS part of @jzohrab https://github.com/jzohrab commit with some basic tests. Could you take a look at it here https://github.com/google/capirca/pull/105?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/capirca/issues/39#issuecomment-269781501, or mute the thread https://github.com/notifications/unsubscribe-auth/ABeLLksuUuaaiXpgyFsts8IwSWHo8dPkks5rNR2ygaJpZM4Fp4i- .

sjtarik commented 7 years ago

@ankenyr reading the previous comments again, I realized validation/verification purposes in mind protocol name resolution is necessary for some people, I will revise and resend the pull request to address that. Thanks.

ankenyr commented 7 years ago

No worries, thanks for the work! Let me know when it is complete.

sjtarik commented 7 years ago

Hi @ankenyr,

I have tested this patch on 3K and 5K switches. The remaining tasks for verification ease is first tcp/udp port number translation, host vs /32, duplicate remarks combined by nxos.

I think if you can review the patch, we can work on the remaining items separately. I think port translation requires changes on ios, xr and asa. @jzohrab's patch has the support but need some work. I have seen some inconsistency where sometimes defined networks return /32 and sometimes host. Why number of host does not work consistently, I am investigating that right now. Currently it does not cause any problems just aesthetics and consistency matter. Third part, I need to test on ios and xr to check if behavior is the same. If two remarks have the same content, the second one is skipped regardless how many aces in between.

Thanks, Tarik

ankenyr commented 7 years ago

Going to close this issue, if someone wants to contribute please make a PR