-
Notifications
You must be signed in to change notification settings - Fork 666
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
[config] Fix config int add incorrect ip #1414
Conversation
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
config/main.py
Outdated
|
||
try: | ||
ip_addr = str(ipaddress.IPv4Address(split_ip_mask[0])) | ||
except ipaddress.AddressValueError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look good to me (catching as exception for IPv6 address).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Suggest using ipaddress.ip_address()
and checking whether the type of the result is IPv4Address
or IPv6Address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
config/main.py
Outdated
@@ -2656,6 +2675,8 @@ def add(ctx, interface_name, ip_addr, gw): | |||
if '/' not in ip_addr: | |||
ip_addr = str(net) | |||
|
|||
ip_addr = validate_ip_address(ctx, ip_addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you want to call this "validate". Since this is for IPv6, why dont we just call if address is v6 and update the leading zeros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not only for IPv6, it checks the range of digits for v4 and v6 IP addresses and additionally removes leading zeros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d-dashkov it seems to me the validation is only on the mast for ipv4 and ipv6. in this case we can do the following
- rename the function to validate_ip_mask
- add more validation points so it can be generic function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the code, and now all IP checking has been moved to my separate function and the try-except construct has been removed from the add and remove ip interface functions. So now it is a generic function that returns the IP string without leading zeros, or False if something is wrong.
Signed-off-by: d-dashkov <[email protected]>
adding unit test for those fixes? |
@lguohan, ok, I will provide unit tests for this fix |
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
@d-dashkov are you about to fix the conflicts so this PR will be able to be approved ? |
Yep, tomorrow I will rename function to validate_ip_mask as suggested Liat. Will that be enough or do you need something else from my side to resolve conflict |
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Signed-off-by: d-dashkov <[email protected]>
Is this PR planned to be cherry picked to 202012 ? |
* Added ip and mask check to config int Signed-off-by: d-dashkov <[email protected]>
@prsunny - Can you please merge also to 202012 ? |
* Added ip and mask check to config int Signed-off-by: d-dashkov <[email protected]>
* Added ip and mask check to config int Signed-off-by: d-dashkov <[email protected]>
[portsorch] add buffer drop FC group [bitmap_vnet] Fix VNET route priority issue (sonic-net#1421) [vnet] Maintain the reference count of the nexthop when creating a vn… (sonic-net#1414) [intfsorch] Retrieve Port object before setting NAT zone on router interfaces. (sonic-net#1372)
- What I did
Added checking if the IP address is correct or return error.
fixes sonic-net/sonic-buildimage#6693
fixes sonic-net/sonic-buildimage#6776
- How I did it
Added function that check correctness of IP address and mask.
- How to verify it
In some cases it will fix the IP and everything will work fine:
- New command output (if the output of a command-line utility has changed)
In case of wrong IP:
In case of wrong mask: