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

[VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands #2333

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

dgsudharsan
Copy link
Collaborator

What I did

Added CLI checks to verify if Vrf is created and valid when used in interface bind and static route commands

How I did it

Check for presence of Vrf in VRF table or in case if it is mgmt vrf, verify if mgmt vrf is enabled/

How to verify it

Added UT to verify it.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@dgsudharsan
Copy link
Collaborator Author

@venkatmahalingam @prsunny Can you please review?

@@ -998,6 +1012,8 @@ def cli_sroute_to_config(ctx, command_str, strict_nh = True):
if 'prefix' in prefix_str and 'vrf' in prefix_str:
# prefix_str: ['prefix', 'vrf', Vrf-name, ip]
vrf_name = prefix_str[2]
if not is_vrf_exists(config_db, vrf_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have check to ensure vrf is removed only after all interface bindings are removed? Otherwise this is only a check in the add-path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only a check in the add path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact the current vrf removal logic removes all these bindings implicitly using the function del_interface_bind_to_vrf

return True
elif vrf_name == "mgmt":
entry = config_db.get_entry("MGMT_VRF_CONFIG", "vrf_global")
if entry and entry.get("mgmtVrfEnabled") == "true":
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we prevent from using non-existing VRF in the INTERFACE table during config load? this seems to address only in the Click CLI.

If we can do the YANG level validation for all north bound interfaces (CLick, config load..etc), it would be an one time check in the back-end rather than doing it on all front-end (e.g this check in Click), what is your thought?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @venkatmahalingam
Your comment is applicable for any table in config_db.json.
This PR is focussed on only the Click CLI. I believe we have a separate discussion on using yang for validation which is the work in progress and would take sometime.
For now It is expected of the end user to use Click CLI to make any incremental configuration changes.
Let me know if the changes look good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update we do with config-db should be YANG validated is what the expectation but the infra is not ready yet, we can merge this code for now.

@dgsudharsan
Copy link
Collaborator Author

@yxieca While cherry-picking please ensure #2309 is cherry-picked before this to avoid build failures

yxieca pushed a commit that referenced this pull request Sep 1, 2022
…tatic route commands (#2333)

- What I did
Added CLI checks to verify if Vrf is created and valid when used in interface bind and static route commands

- How I did it
Check for presence of Vrf in VRF table or in case if it is mgmt vrf, verify if mgmt vrf is enabled/

- How to verify it
Added UT to verify it.
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Sep 7, 2022
Update sonic-utilities submodule pointer to include the following:
* [route_check]: Ignore standalone tunnel routes (sonic-net#2325) ([sonic-net#2346](sonic-net/sonic-utilities#2346))
* [VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands ([sonic-net#2333](sonic-net/sonic-utilities#2333))
* Subinterface vrf bind issue fix ([sonic-net#2211](sonic-net/sonic-utilities#2211))
* [decode-syseeprom] Fix setting use_db based on support_eeprom_db ([sonic-net#2270](sonic-net/sonic-utilities#2270))
* Fix vrf UT failed issue ([sonic-net#2309](sonic-net/sonic-utilities#2309))

Signed-off-by: dprital <drorp@nvidia.com>
yxieca pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Sep 8, 2022
Update sonic-utilities submodule pointer to include the following:
* [route_check]: Ignore standalone tunnel routes (#2325) ([#2346](sonic-net/sonic-utilities#2346))
* [VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands ([#2333](sonic-net/sonic-utilities#2333))
* Subinterface vrf bind issue fix ([#2211](sonic-net/sonic-utilities#2211))
* [decode-syseeprom] Fix setting use_db based on support_eeprom_db ([#2270](sonic-net/sonic-utilities#2270))
* Fix vrf UT failed issue ([#2309](sonic-net/sonic-utilities#2309))

Signed-off-by: dprital <drorp@nvidia.com>

Signed-off-by: dprital <drorp@nvidia.com>
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
…tatic route commands (sonic-net#2333)

- What I did
Added CLI checks to verify if Vrf is created and valid when used in interface bind and static route commands

- How I did it
Check for presence of Vrf in VRF table or in case if it is mgmt vrf, verify if mgmt vrf is enabled/

- How to verify it
Added UT to verify it.
@dgsudharsan dgsudharsan deleted the vrf_checks branch March 9, 2023 02:06
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.

None yet

5 participants