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

Fix huawei display acl all - port numbers replaced by names and IPV6 ACL name position #1222

Merged

Conversation

elavaud
Copy link
Contributor

@elavaud elavaud commented Nov 15, 2022

ISSUE TYPE

  • Bugfix Pull Request
  • Additional Testing

COMPONENT
Huawei VRP
display acl all

SUMMARY

First commit:
Huawei will automatically replace well known ports by their corresponding names. For example 80 by "www", or 23 by "telnet"
I also improved the test by adding a range of ports

Second commit:
When available, the position of the name change between IPv4 and IPv6.

Third commit:
ACL rules can have descriptions

Fourth commit:
Always record first the ACL with empty rules values, and then its rules. Otherwise it was not possible to record ACL that has no rules

@elavaud elavaud changed the title Fix huawei display acl all - port numbers replaced by names Fix huawei display acl all - port numbers replaced by names and IPV6 ACL name position Nov 15, 2022
@k-ribot k-ribot force-pushed the huawei-display-acl-all-ports-issue branch from 9db06a5 to 103c2f1 Compare December 5, 2022 07:08
@elavaud
Copy link
Contributor Author

elavaud commented Dec 14, 2022

Hey @itdependsnetworks,

I noticed another problem with ACLs, and I am unsure how to best handle it:

There are two types of object fetched by this template: the ACL, and a rule of an ACL.
In most cases we can easily handle it after and separate the ACL from the rule.
However if the ACL does not have any rule, its information are not parsed.
For example in the current raw test file we have:

Advanced ACL qsdqsd 3999, 0 rule
Acl's step is 5

We don't get it parsed.
And although its has no rules, it is important to know that the ACL 'qsdqsd' at 3999 exists...

So I guess the broader problem is: Are we fetching ACL? Or only its rules? Or both (and how to do it) ? :)

@itdependsnetworks
Copy link
Contributor

Yes, I think you should capture it, even if the rule is empty

@elavaud
Copy link
Contributor Author

elavaud commented Dec 14, 2022

Yes, I think you should capture it, even if the rule is empty

With the "rule description" being on an other line, the only solution I found was to first record each ACL name and number with empty rules values, then each rules.
Not super pretty but at least it is homogeneous. And again, handling 2 types of object with only one list/parsing is tricky...

@elavaud elavaud force-pushed the huawei-display-acl-all-ports-issue branch from 052f4b9 to 38484fc Compare December 14, 2022 07:50
@k-ribot k-ribot force-pushed the huawei-display-acl-all-ports-issue branch from 38484fc to 552efdc Compare March 21, 2023 09:34
@jvanderaa jvanderaa merged commit cae4bfd into networktocode:master Mar 26, 2023
pszulczewski pushed a commit that referenced this pull request Apr 12, 2023
…ACL name position (#1222)

* Fix huawei display acl all - port numbers replaced by names
* Fix Huawei - display acl (ipv6)? all - if name provided, order in IPv6 changes
* Huawei - display acl all - add rule description
* Huawei - display acl all - separately record ACLs and their rules

authored-by: Edouard Lavaud <[email protected]>
cppmonkey pushed a commit to cppmonkey/ntc-templates that referenced this pull request Oct 25, 2023
…ACL name position (networktocode#1222)

* Fix huawei display acl all - port numbers replaced by names
* Fix Huawei - display acl (ipv6)? all - if name provided, order in IPv6 changes
* Huawei - display acl all - add rule description
* Huawei - display acl all - separately record ACLs and their rules

authored-by: Edouard Lavaud <[email protected]>
@k-ribot k-ribot deleted the huawei-display-acl-all-ports-issue branch June 20, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants