Skip to content

Commit c1843fa

Browse files
anders-nexthopanders-nexthop
authored
Issue sonic-net#22759: Prevent CLI from adding invalid routed interfaces (sonic-net#3901)
What I did Currently, config interface ip add will accept EthernetX interfaces which do not have an underlying port or portchannel associated with them. This violates the yang model, so if these entries are added to ConfigDB, 'config replace' and 'config reload' don't work properly. To make matters worse, the implementation of config interface ip remove does not detect these invalid entries, so once they are added they cannot be removed through the CLI. I modified the command to reject invalid cases. Fixes: sonic-net#22759 How I did it Add a check for this case into the config interface ip add CLI command to avoid invalid cases of this form by checking to ensure that the interface which is passed is associated with a valid port or port channel if it is an Ethernet or PortChannel interface. There is already an existing check for vlan interfaces. How to verify it Modify existing test cases to handle expected behavior, verify that they all pass. Previous command output (if the output of a command-line utility has changed) admin@gold113-dut:~$ sudo config interface ip add EthernetNR 1.2.3.4 admin@gold113-dut:~$ sudo config interface ip add EthernetJustMadeUp 1.2.3.4 admin@gold113-dut:~$ sudo config interface ip add Ethernet99999999 1.2.3.4 admin@gold113-dut:~$ sudo config interface ip add Ethernet-totally-not-real 1.2.3.4 admin@gold113-dut:~$ sudo config interface ip remove EthernetNR 1.2.3.4 Cannot find device "EthernetNR" admin@gold113-dut:~$ sudo config replace -d /etc/sonic/config_db.json ** DRY RUN EXECUTION ** Config Replacer: Config replacement starting. Config Replacer: Target config length: 50412. Config Replacer: Getting current config db. Config Replacer: Generating patch between target config and current config db. Config Replacer: Applying patch using 'Patch Applier'. Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [{"op": "remove", "path": "/INTERFACE/EthernetJustMadeUp"}, {"op": "remove", "path": "/INTERFACE/Ethernet-totally-not-real"}, {"op": "remove", "path": "/INTERFACE/EthernetNR"}, {"op": "remove", "path": "/INTERFACE/Ethernet99999999"}, {"op": "remove", "path": "/INTERFACE/EthernetJustMadeUp|1.2.3.4~132"}, {"op": "remove", "path": "/INTERFACE/Ethernet99999999|1.2.3.4~132"}, {"op": "remove", "path": "/INTERFACE/Ethernet-totally-not-real|1.2.3.4~132"}, {"op": "remove", "path": "/INTERFACE/EthernetNR|1.2.3.4~132"}] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Failed to replace config Usage: config replace [OPTIONS] TARGET_FILE_PATH Try "config replace -h" for help. Error: Data Loading Failed Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "EthernetNR" points to a non-existing leaf. New command output (if the output of a command-line utility has changed) admin@gold113-dut:~$ sudo config interface ip add EthernetNR.10 1.2.3.4 Usage: config interface ip add [OPTIONS] <interface_name> <ip_addr> <default gateway IP address> Try "config interface ip add -h" for help. Error: Interface EthernetNR.10 does not exist
1 parent 7c5378e commit c1843fa

4 files changed

Lines changed: 371 additions & 210 deletions

File tree

‎config/main.py‎

Lines changed: 59 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -5222,47 +5222,7 @@ def add_interface_ip(ctx, interface_name, ip_addr, gw, secondary):
52225222
interface_name = interface_alias_to_name(config_db, interface_name)
52235223
if interface_name is None:
52245224
ctx.fail("'interface_name' is None!")
5225-
# Add a validation to check this interface is not a member in vlan before
5226-
# changing it to a router port mode
5227-
vlan_member_table = config_db.get_table('VLAN_MEMBER')
5228-
5229-
if (interface_is_in_vlan(vlan_member_table, interface_name)):
5230-
click.echo("Interface {} is a member of vlan\nAborting!".format(interface_name))
5231-
return
5232-
52335225

5234-
portchannel_member_table = config_db.get_table('PORTCHANNEL_MEMBER')
5235-
5236-
if interface_is_in_portchannel(portchannel_member_table, interface_name):
5237-
ctx.fail("{} is configured as a member of portchannel."
5238-
.format(interface_name))
5239-
5240-
# Add a validation to check this interface is in routed mode before
5241-
# assigning an IP address to it
5242-
5243-
sub_intf = False
5244-
5245-
if clicommon.is_valid_port(config_db, interface_name):
5246-
is_port = True
5247-
elif clicommon.is_valid_portchannel(config_db, interface_name):
5248-
is_port = False
5249-
else:
5250-
sub_intf = True
5251-
5252-
if not sub_intf:
5253-
interface_mode = None
5254-
if is_port:
5255-
interface_data = config_db.get_entry('PORT', interface_name)
5256-
else:
5257-
interface_data = config_db.get_entry('PORTCHANNEL', interface_name)
5258-
5259-
if "mode" in interface_data:
5260-
interface_mode = interface_data["mode"]
5261-
5262-
if interface_mode == "trunk" or interface_mode == "access":
5263-
click.echo("Interface {} is in {} mode and needs to be in routed mode!".format(
5264-
interface_name, interface_mode))
5265-
return
52665226
try:
52675227
ip_address = ipaddress.ip_interface(ip_addr)
52685228
except ValueError as err:
@@ -5293,7 +5253,65 @@ def add_interface_ip(ctx, interface_name, ip_addr, gw, secondary):
52935253

52945254
table_name = get_interface_table_name(interface_name)
52955255
if table_name == "":
5296-
ctx.fail("'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]")
5256+
ctx.fail(f"{interface_name} is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]")
5257+
5258+
# Add a validation to check this interface is in routed mode before
5259+
# assigning an IP address to it. For sub-interfaces, check that the base
5260+
# interface is in the right mode
5261+
5262+
base_interface_name = interface_name
5263+
5264+
# Handle short name
5265+
match = re.search(r'\d', interface_name)
5266+
if match:
5267+
index = match.start()
5268+
intf_type = interface_name[:index]
5269+
intf_val = interface_name[index:]
5270+
if intf_type == "Eth":
5271+
intf_type = "Ethernet"
5272+
elif intf_type == "Po":
5273+
intf_type = "PortChannel"
5274+
base_interface_name = intf_type + intf_val
5275+
5276+
base_table_name = table_name
5277+
if table_name == "VLAN_SUB_INTERFACE":
5278+
interface_name_parts = base_interface_name.split(".")
5279+
base_interface_name = interface_name_parts[0] if len(interface_name_parts) >= 2 else interface_name
5280+
base_table_name = get_interface_table_name(base_interface_name)
5281+
if base_table_name == "":
5282+
ctx.fail(f"{interface_name} is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]")
5283+
5284+
portchannel_member_table = config_db.get_table('PORTCHANNEL_MEMBER')
5285+
if interface_is_in_portchannel(portchannel_member_table, base_interface_name):
5286+
ctx.fail(f"{base_interface_name} is configured as a member of portchannel.")
5287+
5288+
# Add a validation to check this interface is not a member in vlan before
5289+
vlan_member_table = config_db.get_table('VLAN_MEMBER')
5290+
if (interface_is_in_vlan(vlan_member_table, base_interface_name)):
5291+
ctx.fail("Interface {} is a member of vlan\nAborting!".format(base_interface_name))
5292+
return
5293+
5294+
if base_table_name == "INTERFACE" or base_table_name == "PORTCHANNEL_INTERFACE":
5295+
if clicommon.is_valid_port(config_db, base_interface_name):
5296+
is_port = True
5297+
elif clicommon.is_valid_portchannel(config_db, base_interface_name):
5298+
is_port = False
5299+
else:
5300+
ctx.fail("Interface {} does not exist".format(interface_name))
5301+
5302+
interface_mode = None
5303+
if is_port:
5304+
interface_data = config_db.get_entry('PORT', base_interface_name)
5305+
else:
5306+
interface_data = config_db.get_entry('PORTCHANNEL', base_interface_name)
5307+
5308+
if "mode" in interface_data:
5309+
interface_mode = interface_data["mode"]
5310+
5311+
if interface_mode == "trunk" or interface_mode == "access":
5312+
click.echo("Interface {} is in {} mode and needs to be in routed mode!".format(
5313+
interface_name, interface_mode))
5314+
return
52975315

52985316
if table_name == "VLAN_INTERFACE":
52995317
if not validate_vlan_exists(config_db, interface_name):

‎tests/ip_config_test.py‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,38 @@ def test_add_vlan_interface_ipv4(self):
8181
assert result.exit_code != 0
8282
assert NOT_EXIST_VLAN_ERROR_MSG in result.output
8383

84+
# config int ip add Ethernet12 1.1.1.1/24
85+
result = runner.invoke(
86+
config.config.commands["interface"].commands["ip"].commands["add"],
87+
["Ethernet12", "1.1.1.1/24"],
88+
obj=obj)
89+
print(result.exit_code, result.output)
90+
assert "Interface Ethernet12 is a member of vlan" in result.output
91+
92+
# config int ip add Ethernet12.10 1.1.1.1/24
93+
result = runner.invoke(
94+
config.config.commands["interface"].commands["ip"].commands["add"],
95+
["Ethernet12.10", "1.1.1.1/24"],
96+
obj=obj)
97+
print(result.exit_code, result.output)
98+
assert "Interface Ethernet12 is a member of vlan" in result.output
99+
100+
# config int ip add PortChannel1001 1.1.1.1/24
101+
result = runner.invoke(
102+
config.config.commands["interface"].commands["ip"].commands["add"],
103+
["PortChannel1001", "1.1.1.1/24"],
104+
obj=obj)
105+
print(result.exit_code, result.output)
106+
assert "Interface PortChannel1001 is a member of vlan" in result.output
107+
108+
# config int ip add PortChannel1001.10 1.1.1.1/24
109+
result = runner.invoke(
110+
config.config.commands["interface"].commands["ip"].commands["add"],
111+
["PortChannel1001.10", "1.1.1.1/24"],
112+
obj=obj)
113+
print(result.exit_code, result.output)
114+
assert "Interface PortChannel1001 is a member of vlan" in result.output
115+
84116

85117
def test_add_del_interface_valid_ipv4(self):
86118
db = Db()
@@ -105,6 +137,24 @@ def test_add_del_interface_valid_ipv4(self):
105137
assert result.exit_code == 0
106138
assert ('Eth36.10', '32.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
107139

140+
# config int ip add PortChannel0001.10 32.11.10.1/24
141+
result = runner.invoke(
142+
config.config.commands["interface"].commands["ip"].commands["add"],
143+
["PortChannel0001.10", "32.11.10.1/24"],
144+
obj=obj)
145+
print(result.exit_code, result.output)
146+
assert result.exit_code == 0
147+
assert ('PortChannel0001.10', '32.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
148+
149+
# config int ip add Po0001.11 32.11.10.1/24
150+
result = runner.invoke(
151+
config.config.commands["interface"].commands["ip"].commands["add"],
152+
["Po0001.11", "32.11.10.1/24"],
153+
obj=obj)
154+
print(result.exit_code, result.output)
155+
assert result.exit_code == 0
156+
assert ('Po0001.11', '32.11.10.1/24') in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
157+
108158
# config int ip remove Ethernet64 10.10.10.1/24
109159
with mock.patch('utilities_common.cli.run_command') as mock_run_command:
110160
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"], ["Ethernet64", "10.10.10.1/24"], obj=obj)
@@ -137,6 +187,26 @@ def test_add_del_interface_valid_ipv4(self):
137187
assert mock_run_command.call_count == 1
138188
assert ('Eth36.10', '32.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
139189

190+
# config int ip remove PortChannel0001.10 10.11.10.1/24
191+
with mock.patch('utilities_common.cli.run_command') as mock_run_command:
192+
result = runner.invoke(
193+
config.config.commands["interface"].commands["ip"].commands["remove"],
194+
["PortChannel0001.10", "10.11.10.1/24"],
195+
obj=obj)
196+
print(result.exit_code, result.output)
197+
assert result.exit_code == 0
198+
assert mock_run_command.call_count == 1
199+
assert ('PortChannel0001.10', '10.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
200+
201+
# config int ip remove Po0001.11 32.11.10.1/24
202+
with mock.patch('utilities_common.cli.run_command') as mock_run_command:
203+
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["remove"],
204+
["Po0001.11", "32.11.10.1/24"], obj=obj)
205+
print(result.exit_code, result.output)
206+
assert result.exit_code == 0
207+
assert mock_run_command.call_count == 1
208+
assert ('Po0001.11', '32.11.10.1/24') not in db.cfgdb.get_table('VLAN_SUB_INTERFACE')
209+
140210
# config int ip add vlan1000 10.21.20.1/24 as secondary
141211
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"],
142212
["Vlan1000", "10.11.20.1/24", "--secondary"], obj=obj)
@@ -212,6 +282,80 @@ def test_ip_add_on_interface_which_is_member_of_portchannel(self):
212282
print(result.exit_code)
213283
assert 'Error: Ethernet32 is configured as a member of portchannel.' in result.output
214284

285+
result = runner.invoke(
286+
config.config.commands["interface"].commands["ip"].commands["add"],
287+
["Ethernet32.10", "100.10.10.1/24"],
288+
obj=obj)
289+
assert result.exit_code != 0
290+
print(result.output)
291+
print(result.exit_code)
292+
assert 'Error: Ethernet32 is configured as a member of portchannel.' in result.output
293+
294+
result = runner.invoke(
295+
config.config.commands["interface"].commands["ip"].commands["add"],
296+
["Eth32.10", "100.10.10.1/24"],
297+
obj=obj)
298+
assert result.exit_code != 0
299+
print(result.output)
300+
print(result.exit_code)
301+
assert 'Error: Ethernet32 is configured as a member of portchannel.' in result.output
302+
303+
def test_ip_add_on_interface_with_no_port(self):
304+
runner = CliRunner()
305+
db = Db()
306+
obj = {'config_db': db.cfgdb}
307+
308+
# config int ip add Ethernet51 100.10.10.1/24
309+
result = runner.invoke(
310+
config.config.commands["interface"].commands["ip"].commands["add"],
311+
["Ethernet51", "100.10.10.1/24"],
312+
obj=obj)
313+
assert result.exit_code != 0
314+
print(result.output)
315+
print(result.exit_code)
316+
assert 'Error: Interface Ethernet51 does not exist' in result.output
317+
318+
# config int ip add Ethernet51.10 100.10.10.1/24
319+
result = runner.invoke(
320+
config.config.commands["interface"].commands["ip"].commands["add"],
321+
["Ethernet51.10", "100.10.10.1/24"],
322+
obj=obj)
323+
assert result.exit_code != 0
324+
print(result.output)
325+
print(result.exit_code)
326+
assert 'Error: Interface Ethernet51.10 does not exist' in result.output
327+
328+
# config int ip add Eth51.10 32.11.10.1/24
329+
result = runner.invoke(
330+
config.config.commands["interface"].commands["ip"].commands["add"],
331+
["Eth51.10", "32.11.10.1/24"],
332+
obj=obj)
333+
assert result.exit_code != 0
334+
print(result.output)
335+
print(result.exit_code)
336+
assert 'Error: Interface Eth51.10 does not exist' in result.output
337+
338+
# config int ip add PortChanne51.10 32.11.10.1/24
339+
result = runner.invoke(
340+
config.config.commands["interface"].commands["ip"].commands["add"],
341+
["PortChannel51.10", "32.11.10.1/24"],
342+
obj=obj)
343+
assert result.exit_code != 0
344+
print(result.output)
345+
print(result.exit_code)
346+
assert 'Error: Interface PortChannel51.10 does not exist' in result.output
347+
348+
# config int ip add Po51.10 32.11.10.1/24
349+
result = runner.invoke(
350+
config.config.commands["interface"].commands["ip"].commands["add"],
351+
["Po51.10", "32.11.10.1/24"],
352+
obj=obj)
353+
assert result.exit_code != 0
354+
print(result.output)
355+
print(result.exit_code)
356+
assert 'Error: Interface Po51.10 does not exist' in result.output
357+
358+
215359
''' Tests for IPv6 '''
216360

217361
def test_add_del_interface_valid_ipv6(self):

‎tests/vlan_test.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1490,7 +1490,6 @@ def test_config_set_router_port_on_member_interface(self):
14901490
result = runner.invoke(config.config.commands["interface"].commands["ip"].commands["add"],
14911491
["Ethernet4", "10.10.10.1/24"], obj=obj)
14921492
print(result.exit_code, result.output)
1493-
assert result.exit_code == 0
14941493
assert 'Interface Ethernet4 is a member of vlan\nAborting!\n' in result.output
14951494

14961495
def test_config_vlan_add_member_of_portchannel(self):

0 commit comments

Comments
 (0)