Add vlan validation in config interface ip add command #3155
Conversation
~ Signed-off-by: mati <[email protected]>
Signed-off-by: mati <[email protected]>
config/main.py
Outdated
| @@ -4576,6 +4579,12 @@ def add(ctx, interface_name, ip_addr, gw): | |||
| table_name = get_interface_table_name(interface_name) | |||
| if table_name == "": | |||
| ctx.fail("'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]") | |||
|
|
|||
| if table_name == "VLAN_INTERFACE": | |||
| if not validate_vlan_format(interface_name): | |||
There was a problem hiding this comment.
Shouldn't we be checking the presence of Vlan rather than validating the format? My recommendation is to query the VLAN_TABLE and confirm if Vlan is present before assigning an IP address
There was a problem hiding this comment.
From my POV, you should be allowed to configure IP interface on a Vlan before creating it.
Once Vlan is created, configuration will apply.
This kind of behavior is also common on other switch devices (for example, Catalyst)
There was a problem hiding this comment.
I tend to agree with @dgsudharsan here. Just creating VLAN interface is not handled in backend (swss etc) until a VLAN table is present. So, why not check VLAN table entry and return an error log? But @dgsudharsan , do you think we can add this check to Vlan creation?
There was a problem hiding this comment.
Added query of VLAN_TABLE as you suggest,
Also added option to ignore/skip this query, please let me know if you'd like this option removed.
Test that vlan exists Added an optional param (option) --ignore-vlan which will test only format ~ Signed-off-by: matiAlfaro <[email protected]>
config/main.py
Outdated
| @@ -4524,8 +4529,9 @@ def validate_vlan_format(text): | |||
| @click.argument('interface_name', metavar='<interface_name>', required=True) | |||
| @click.argument("ip_addr", metavar="<ip_addr>", required=True) | |||
| @click.argument('gw', metavar='<default gateway IP address>', required=False) | |||
| @click.option('-i','--ignore-vlan',is_flag=True, required=False, help="Allow configuration on not created Vlan") | |||
There was a problem hiding this comment.
I don't think this must be handled in this way. Lets simply throw an error if vlan does not exist asking user to create Vlan first?
Signed-off-by: matiAlfaro <[email protected]>
There was a problem hiding this comment.
This validation requirement is not reflected in the VLAN yang model: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-vlan.yang. Would you please improve the yang model so that it aligns with this requirement?
|
@isabelmsft - Please see sonic-net/sonic-buildimage#18207 |
Thanks @matiAlfaro, the requirement that vlan exists before IP address is configured is supported in yang model here: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-vlan.yang#L125 So this PR LGTM |
|
@yxieca Can you please cherry-pick this fix to 202311? |
|
Cherry-pick PR to 202311: #3320 |
mssonicbld
added
Included in 202311 Branch
and removed
Created PR to 202311 Branch
labels
~
What I did
Fix for sonic-net/sonic-buildimage#16975
Prevent configuration of illegal Vlan name in "config interface ip add" command
How I did it
Added Vlan name validation
How to verify it
Positive tests
config interface ip add Vlan100 1.1.1.1/24
config interface ip add Vlan4093 1.1.1.1/24
Negative tests
config interface ip add Vlan000000000000003 1.1.1.1/24
config interface ip add Vlan01 1.1.1.1/24
config interface ip add Vlan1.2 1.1.1.1/24
Previous command output (if the output of a command-line utility has changed)
On illegal Vlan name command succeeded with no error
New command output (if the output of a command-line utility has changed)
config interface ip add Vlan030 1.1.1.1/24
Error: Vlan030 is not a valid Vlan name