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.

Sign up for GitHub

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

Jump to bottom

SONiC Yang model support for NAT #7051

Merged
merged 18 commits into from
Jun 10, 2021
Merged

SONiC Yang model support for NAT #7051

merged 18 commits into from
Jun 10, 2021

Conversation

AkhileshSamineni
Copy link
Contributor

@AkhileshSamineni AkhileshSamineni commented Mar 13, 2021 •
edited
Loading

SONiC Yang model support for NAT

Signed-off-by: Akhilesh Samineni akhilesh.samineni@broadcom.com

@anshuv-mfst anshuv-mfst added the YANG YANG model related changes label Mar 24, 2021
@AkhileshSamineni
Copy link
Contributor Author

/Azurepipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 7051 in repo Azure/sonic-buildimage

@AkhileshSamineni
Added testcases for NAT sonic-yang.
8dbb83e 
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
@AkhileshSamineni
Copy link
Contributor Author

@praveen-li @arlakshm @lguohan @venkatmahalingam
Please re-review the changes.
I have addressed all the review comments and added test cases as well.

venkatmahalingam
venkatmahalingam reviewed Apr 20, 2021
View reviewed changes
src/sonic-yang-models/yang-models/sonic-nat.yang Outdated

description "NAT_GLOBAL table part of config_db.json";

list NAT_GLOBAL_LIST {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace list (with max-elements 1) with a container? e.g container NAT_GLOBAL { container values {...}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like List is mandatory here, seeing errors when I replace list with container.
Is it supported ? If yes, could you please give me some reference here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please refer TACPLUS container in file src/sonic-yang-models/yang-models/sonic-system-tacacs.yang
PR - https://github.com/Azure/sonic-buildimage/pull/7671/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced list with a container.

@venkatmahalingam
Copy link
Collaborator

@AkhileshSamineni Please fix the build failures.

@AkhileshSamineni
Copy link
Contributor Author

@praveen-li @arlakshm @lguohan @venkatmahalingam
This time Build is passed in Broadcom platform, it is failing on Mellanox and VS.
Could someone check it and help me out.

@arlakshm
Copy link
Contributor

@kperumalbfn, can you please take a look at this PR

@arlakshm
Copy link
Contributor

@AkhileshSamineni the sonic-utilities build is failing with the below error.
Can you check.


Failed to validate data tree
Data Validation Failed
[ERROR] Port breakout Failed!!! Opting Out
Failed to break out Port. Error:

@venkatmahalingam
Copy link
Collaborator

@AkhileshSamineni Please mark the comments addressed as resolved to track the outstanding comments better,

@AkhileshSamineni
Copy link
Contributor Author

@AkhileshSamineni Please mark the comments addressed as resolved to track the outstanding comments better,

@venkatmahalingam I have addressed all the comments and mentioned those as resolved.

@AkhileshSamineni
Copy link
Contributor Author

@praveen-li @arlakshm @lguohan @venkatmahalingam
The build failure - [ERROR] Port breakout Failed!!! Opting Out
Not sure why I am seeing this issue, Someone give me some pointers here please.
sonic-utilities build is failing sometimes and passing sometimes.

@praveen-li
Copy link
Collaborator

praveen-li commented May 3, 2021 via email

@AkhileshSamineni
Copy link
Contributor Author

AkhileshSamineni commented May 3, 2021 via email

@AkhileshSamineni
Copy link
Contributor Author

@praveen-li @arlakshm @lguohan @venkatmahalingam
I have addressed all the review comments, Please merge this PR.

@AkhileshSamineni
Copy link
Contributor Author

@arlakshm @lguohan Can someone merge this PR please.

@arlakshm
Copy link
Contributor

@AkhileshSamineni can you address the latest comment from @venkatmahalingam ?

@AkhileshSamineni
Copy link
Contributor Author

@AkhileshSamineni can you address the latest comment from @venkatmahalingam ?

@arlakshm Addressed the review comment.

@arlakshm arlakshm merged commit 714894c into sonic-net:master Jun 10, 2021
Junchao-Mellanox pushed a commit to Junchao-Mellanox/sonic-buildimage that referenced this pull request Jun 24, 2021
@AkhileshSamineni @Junchao-Mellanox
[Yang] SONiC Yang model support for NAT (sonic-net#7051)
720e816 
This change has SONiC Yang model support for NAT
- Created SONiC Yang model for NAT
- Tables: STATIC_NAPT, STATIC_NAT, NAT_GLOBAL, NAT_POOL, NAT_BINDINGS.

How I did it
Defined Yang models for NAT based on Guideline doc:
https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md
and
https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md

How to verify it
Added test cases to verify it.
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
@AkhileshSamineni
[Yang] SONiC Yang model support for NAT (sonic-net#7051)
60cf0b9 
This change has SONiC Yang model support for NAT
- Created SONiC Yang model for NAT
- Tables: STATIC_NAPT, STATIC_NAT, NAT_GLOBAL, NAT_POOL, NAT_BINDINGS.

How I did it
Defined Yang models for NAT based on Guideline doc:
https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md
and
https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md

How to verify it
Added test cases to verify it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lguohan lguohan lguohan left review comments

@praveen-li praveen-li praveen-li left review comments

@kperumalbfn kperumalbfn kperumalbfn approved these changes

@venkatmahalingam venkatmahalingam venkatmahalingam approved these changes

@arlakshm arlakshm arlakshm approved these changes

Assignees
No one assigned
Labels
YANG YANG model related changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@AkhileshSamineni @venkatmahalingam @arlakshm @praveen-li @lguohan @kperumalbfn @anshuv-mfst