Skip to content

Fixed IP validation in "config interface ip add/remove"#1709

Merged
bingwang-ms merged 1 commit intosonic-net:masterfrom
akokhan:config_ip_remove_fix
Jul 15, 2021
Merged

Fixed IP validation in "config interface ip add/remove"#1709
bingwang-ms merged 1 commit intosonic-net:masterfrom
akokhan:config_ip_remove_fix

Conversation

@akokhan
Copy link
Copy Markdown
Contributor

@akokhan akokhan commented Jul 7, 2021

Signed-off-by: Andriy Kokhan [email protected]

Why I did it

After executing command that should delete ipv6 address from LAG, ipv6 address still remains.
This affects t0 VLAN TCs.

What I did

Reworked IP validation for "config interface ip remove" command added by #1414

Since the IP address is used as a part of a key in Redis DB, we should not modify IP address format provided by the user (e.g., convert "FC00::71/126" to "fc00::71/126" which causes VLAN TCs failure). At the same time we probably should not tolerate extra zeros in IPv4...

So, reworked IP validation in "config interface ip add/remove" command:
- Renamed validate_ip_mask() to is_valid_ip_interface() as per code style
- Updated is_valid_ip_interface() to do not modify the IP address
- Updated UTs per changes

How to verify it

deploy t0
sudo config interface ip remove PortChannel0001 FC00::71/126

@akokhan
Copy link
Copy Markdown
Contributor Author

akokhan commented Jul 7, 2021

@prsunny , @jleveque , @d-dashkov ,
please approve and merge.

@akokhan
Copy link
Copy Markdown
Contributor Author

akokhan commented Jul 7, 2021

The fix must be cherry-picked into 202012 as well

prsunny
prsunny previously approved these changes Jul 7, 2021
@akokhan akokhan marked this pull request as draft July 7, 2021 16:48
@akokhan
Copy link
Copy Markdown
Contributor Author

akokhan commented Jul 7, 2021

In fact, it's wrong approach to convert v6 addresses to upper case as well. Looks like RIF can be loaded from config_db.json into config_db in both lower and upper formats. Ideally, we should not change case of letters.

@akokhan akokhan force-pushed the config_ip_remove_fix branch from 2a968e3 to dad8178 Compare July 7, 2021 18:54
@akokhan akokhan changed the title Fixed validate_ip_mask() to make IPv6 uppercase Removed IP validation for "config interface ip remove" command Jul 7, 2021
@akokhan akokhan marked this pull request as ready for review July 7, 2021 19:24
@akokhan akokhan requested a review from prsunny July 7, 2021 19:24
@akokhan akokhan force-pushed the config_ip_remove_fix branch from dad8178 to b5eff4d Compare July 13, 2021 11:02
@akokhan akokhan changed the title Removed IP validation for "config interface ip remove" command Fixed IP validation in "config interface ip add/remove" Jul 13, 2021
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Jul 13, 2021

This pull request introduces 1 alert when merging b5eff4d5309fd03d7caea5eae80eff84dccaef45 into e8b6c5c - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@akokhan akokhan force-pushed the config_ip_remove_fix branch from b5eff4d to 02a2552 Compare July 13, 2021 11:14
  - Renamed validate_ip_mask() to is_valid_ip_interface() as per code style
  - Updated is_valid_ip_interface() to do not modify the IP address
  - Updated UTs per changes

Signed-off-by: Andriy Kokhan <[email protected]>
@akokhan
Copy link
Copy Markdown
Contributor Author

akokhan commented Jul 13, 2021

@prsunny , @jleveque , please review. Thanks

@akokhan
Copy link
Copy Markdown
Contributor Author

akokhan commented Jul 13, 2021

@lguohan , please review.

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Jul 14, 2021

@d-dashkov can u pls review?

@bingwang-ms bingwang-ms merged commit e98bbb6 into sonic-net:master Jul 15, 2021
@bingwang-ms
Copy link
Copy Markdown
Contributor

Merged, thanks for the fix

qiluo-msft pushed a commit that referenced this pull request Jul 15, 2021
…1709)

- Renamed validate_ip_mask() to is_valid_ip_interface() as per code style
  - Updated is_valid_ip_interface() to do not modify the IP address
  - Updated UTs per changes

Signed-off-by: Andriy Kokhan <[email protected]>
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-utilities that referenced this pull request Aug 10, 2021
…onic-net#1709)

- Renamed validate_ip_mask() to is_valid_ip_interface() as per code style
  - Updated is_valid_ip_interface() to do not modify the IP address
  - Updated UTs per changes

Signed-off-by: Andriy Kokhan <[email protected]>
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.

5 participants