From 928aad194412b53ecfdaa87fade7740e771fd7f2 Mon Sep 17 00:00:00 2001 From: Yaqiang Zhu Date: Thu, 16 Feb 2023 02:23:39 +0800 Subject: [PATCH] [dhcp_relay] Remove exist check while adding dhcpv6 relay (#13822) Why I did it DHCPv6 relay config entry is not useful while del dhcpv6 relay config. How I did it Remove dhcpv6_relay entry if it is empty and not check entry exist while adding dhcpv6 relay --- .../cli-plugin-tests/conftest.py | 13 +++++- .../test_config_dhcp_relay.py | 40 ++++++++++++++++++- .../cli/config/plugins/dhcp_relay.py | 18 ++++++--- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/dockers/docker-dhcp-relay/cli-plugin-tests/conftest.py b/dockers/docker-dhcp-relay/cli-plugin-tests/conftest.py index 37aec0b8b251..5f0b981ccaee 100644 --- a/dockers/docker-dhcp-relay/cli-plugin-tests/conftest.py +++ b/dockers/docker-dhcp-relay/cli-plugin-tests/conftest.py @@ -19,14 +19,23 @@ def mock_cfgdb(): } def get_entry(table, key): + if table not in CONFIG or key not in CONFIG[table]: + return {} return CONFIG[table][key] def set_entry(table, key, data): CONFIG[table].setdefault(key, {}) - CONFIG[table][key] = data + + if data is None: + CONFIG[table].pop(key) + else: + CONFIG[table][key] = data + + def get_keys(table): + return CONFIG[table].keys() cfgdb.get_entry = mock.Mock(side_effect=get_entry) cfgdb.set_entry = mock.Mock(side_effect=set_entry) + cfgdb.get_keys = mock.Mock(side_effect=get_keys) yield cfgdb - diff --git a/dockers/docker-dhcp-relay/cli-plugin-tests/test_config_dhcp_relay.py b/dockers/docker-dhcp-relay/cli-plugin-tests/test_config_dhcp_relay.py index 2c9a5c19a93b..a42d5cce7a8a 100644 --- a/dockers/docker-dhcp-relay/cli-plugin-tests/test_config_dhcp_relay.py +++ b/dockers/docker-dhcp-relay/cli-plugin-tests/test_config_dhcp_relay.py @@ -111,9 +111,25 @@ def test_plugin_registration(self): cli = mock.MagicMock() dhcp_relay.register(cli) - def test_config_dhcp_relay_add_del_with_nonexist_vlanid(self, ip_version, op): + def test_config_dhcp_relay_add_del_with_nonexist_vlanid_ipv4(self, op): runner = CliRunner() + ip_version = "ipv4" + with mock.patch("utilities_common.cli.run_command") as mock_run_command: + result = runner.invoke(dhcp_relay.dhcp_relay.commands[ip_version] + .commands[IP_VER_TEST_PARAM_MAP[ip_version]["command"]] + .commands[op], ["1001", IP_VER_TEST_PARAM_MAP[ip_version]["ips"][0]]) + print(result.exit_code) + print(result.stdout) + assert result.exit_code != 0 + assert "Error: Vlan1001 doesn't exist" in result.output + assert mock_run_command.call_count == 0 + + def test_config_dhcp_relay_del_with_nonexist_vlanid_ipv6(self): + runner = CliRunner() + + op = "del" + ip_version = "ipv6" with mock.patch("utilities_common.cli.run_command") as mock_run_command: result = runner.invoke(dhcp_relay.dhcp_relay.commands[ip_version] .commands[IP_VER_TEST_PARAM_MAP[ip_version]["command"]] @@ -262,3 +278,25 @@ def test_config_add_del_duplicate_dhcp_relay(self, mock_cfgdb, ip_version, op): assert result.exit_code != 0 assert "Error: Find duplicate DHCP relay ip {} in {} list".format(test_ip, op) in result.output assert mock_run_command.call_count == 0 + + def test_config_add_dhcp_relay_ipv6_with_non_entry(self, mock_cfgdb): + op = "add" + ip_version = "ipv6" + test_ip = IP_VER_TEST_PARAM_MAP[ip_version]["ips"][0] + runner = CliRunner() + db = Db() + db.cfgdb = mock_cfgdb + table = IP_VER_TEST_PARAM_MAP[ip_version]["table"] + db.cfgdb.set_entry(table, "Vlan1000", None) + assert db.cfgdb.get_entry(table, "Vlan1000") == {} + assert len(db.cfgdb.get_keys(table)) == 0 + + with mock.patch("utilities_common.cli.run_command") as mock_run_command: + result = runner.invoke(dhcp_relay.dhcp_relay.commands[ip_version] + .commands[IP_VER_TEST_PARAM_MAP[ip_version]["command"]] + .commands[op], ["1000", test_ip], obj=db) + print(result.exit_code) + print(result.output) + assert result.exit_code == 0 + assert db.cfgdb.get_entry(table, "Vlan1000") == {"dhcpv6_servers": [test_ip]} + assert mock_run_command.call_count == 3 diff --git a/dockers/docker-dhcp-relay/cli/config/plugins/dhcp_relay.py b/dockers/docker-dhcp-relay/cli/config/plugins/dhcp_relay.py index 57848161f653..aea8e491f05b 100644 --- a/dockers/docker-dhcp-relay/cli/config/plugins/dhcp_relay.py +++ b/dockers/docker-dhcp-relay/cli/config/plugins/dhcp_relay.py @@ -22,11 +22,13 @@ def validate_ips(ctx, ips, ip_version): ctx.fail("{} is not IPv{} address".format(ip, ip_version)) -def get_dhcp_servers(db, vlan_name, ctx, table_name, dhcp_servers_str): - table = db.cfgdb.get_entry(table_name, vlan_name) - if len(table.keys()) == 0: - ctx.fail("{} doesn't exist".format(vlan_name)) +def get_dhcp_servers(db, vlan_name, ctx, table_name, dhcp_servers_str, check_is_exist=True): + if check_is_exist: + keys = db.cfgdb.get_keys(table_name) + if vlan_name not in keys: + ctx.fail("{} doesn't exist".format(vlan_name)) + table = db.cfgdb.get_entry(table_name, vlan_name) dhcp_servers = table.get(dhcp_servers_str, []) return dhcp_servers, table @@ -49,7 +51,10 @@ def add_dhcp_relay(vid, dhcp_relay_ips, db, ip_version): ctx = click.get_current_context() # Verify ip addresses are valid validate_ips(ctx, dhcp_relay_ips, ip_version) - dhcp_servers, table = get_dhcp_servers(db, vlan_name, ctx, table_name, dhcp_servers_str) + + # It's unnecessary for DHCPv6 Relay to verify entry exist + check_config_exist = True if ip_version == 4 else False + dhcp_servers, table = get_dhcp_servers(db, vlan_name, ctx, table_name, dhcp_servers_str, check_config_exist) added_ips = [] for dhcp_relay_ip in dhcp_relay_ips: @@ -100,6 +105,9 @@ def del_dhcp_relay(vid, dhcp_relay_ips, db, ip_version): else: table[dhcp_servers_str] = dhcp_servers + if ip_version == 6 and len(table.keys()) == 0: + table = None + db.cfgdb.set_entry(table_name, vlan_name, table) click.echo("Removed DHCP relay address [{}] from {}".format(",".join(dhcp_relay_ips), vlan_name)) try: