Skip to content
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.

Sign up for GitHub

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

[filter-fdb]: Filter Out FDB Entries With Switch MAC Address #1331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[filter-fdb]: Filter Out FDB Entries With Switch MAC Address #1331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions fdbutil/filter_fdb_entries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from collections import defaultdict
from ipaddress import ip_address, ip_network, ip_interface

def get_vlan_cidr_map(filename):
def get_vlan_cidr_map_and_switch_mac(filename):
"""
Generate Vlan CIDR information from Config DB file

Expand Down Expand Up @@ -37,9 +37,9 @@ def get_vlan_cidr_map(filename):
vlan_cidr[vlan] = {4: ip_address("0.0.0.0"), 6: ip_address("::")}
vlan_cidr[vlan][ip_interface(cidr).version] = ip_interface(cidr).network

return vlan_cidr
return vlan_cidr, config_db_entries["DEVICE_METADATA"]["localhost"]["mac"].replace(':', '-').upper()

def get_arp_entries_map(arp_filename, config_db_filename):
def get_arp_entries_map_and_switch_mac(arp_filename, config_db_filename):
"""
Generate map for ARP entries

Expand All @@ -53,7 +53,7 @@ def get_arp_entries_map(arp_filename, config_db_filename):
Returns:
arp_map(dict) map of ARP entries using MAC as key.
"""
vlan_cidr = get_vlan_cidr_map(config_db_filename)
vlan_cidr, switch_mac = get_vlan_cidr_map_and_switch_mac(config_db_filename)

with open(arp_filename, 'r') as fp:
arp_entries = json.load(fp)
Expand All @@ -69,7 +69,7 @@ def get_arp_entries_map(arp_filename, config_db_filename):
and "neigh" in config:
arp_map[config["neigh"].replace(':', '-').upper()] = ""

return arp_map
return arp_map, switch_mac

def filter_fdb_entries(fdb_filename, arp_filename, config_db_filename, backup_file):
"""
Expand All @@ -87,15 +87,16 @@ def filter_fdb_entries(fdb_filename, arp_filename, config_db_filename, backup_fi
Returns:
None
"""
arp_map = get_arp_entries_map(arp_filename, config_db_filename)
arp_map, switch_mac = get_arp_entries_map_and_switch_mac(arp_filename, config_db_filename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since switch_mac is used only in this function, can we have it as a separate API to fetch instead of changing multiple existing APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, I just did not want to open the json files twice.


with open(fdb_filename, 'r') as fp:
fdb_entries = json.load(fp)

def filter_fdb_entry(fdb_entry):
for key, _ in fdb_entry.items():
if 'FDB_TABLE' in key:
return key.split(':')[-1].upper() in arp_map
mac = key.split(':')[-1].upper()
return mac != switch_mac and mac in arp_map
Copy link
Contributor

@lguohan lguohan Dec 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we see mac = switch_mac, it is kind of very strange, can we get a WARNING log here? maybe something is wrong. meanwhile, for mac in fdb but not in arp, we can continue ignore without any log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will add logs


# malformed entry, default to False so it will be deleted
return False
Expand Down
154 changes: 154 additions & 0 deletions tests/filter_fdb_input/test_vectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
"fdb": [
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
}
},
"expected_fdb": [
],
Expand Down Expand Up @@ -39,6 +44,11 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1000": {}
},
Expand Down Expand Up @@ -78,6 +88,11 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1000": {}
},
Expand Down Expand Up @@ -116,6 +131,11 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1": {}
},
Expand Down Expand Up @@ -153,6 +173,62 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1": {}
},
"VLAN_INTERFACE": {
"Vlan1|25.103.178.1/21": {},
"Vlan1": {},
"Vlan1|fc02:1000::1/64": {}
},
},
"expected_fdb": [
{
"FDB_TABLE:Vlan1:50-2f-a8-cb-76-7c": {
"type": "dynamic",
"port": "Ethernet22"
},
"OP": "SET"
},
],
},
{
"arp":[
{
"NEIGH_TABLE:Vlan1000:192.168.0.10": {
"neigh": "72:06:00:01:01:16",
"family": "IPv4"
},
"OP": "SET"
},
{
"NEIGH_TABLE:Vlan1:fc02:1000::100": {
"neigh": "50:2f:a8:cb:76:7c",
"family": "IPv6"
},
"OP": "SET"
},
],
"fdb": [
{
"FDB_TABLE:Vlan1:50-2f-a8-cb-76-7c": {
"type": "dynamic",
"port": "Ethernet22"
},
"OP": "SET"
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1": {}
},
Expand Down Expand Up @@ -199,6 +275,11 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1000": {}
},
Expand Down Expand Up @@ -237,6 +318,11 @@
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1": {}
},
Expand All @@ -257,6 +343,11 @@
"arp": "tests/filter_fdb_input/arp.json",
"fdb": "tests/filter_fdb_input/fdb.json",
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1": {}
},
Expand All @@ -271,8 +362,71 @@
"arp": "tests/filter_fdb_input/arp.json",
"fdb": "tests/filter_fdb_input/fdb.json",
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
}
},
"expected_fdb": [
],
},
{
"arp":[
{
"NEIGH_TABLE:Vlan1000:192.168.0.10": {
"neigh": "72:06:00:01:00:08",
"family": "IPv4"
},
"OP": "SET"
},
{
"NEIGH_TABLE:Vlan1000:192.168.0.10": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this valid scenario, having one neighbor entry map to two different macs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, existing code already filter out such mac, i remember previously we did correlate the fdb and arp entries, and will insert fdb entries if they exist. However, in this case, we may have fdb entry of switch mac, but I highly doubt we will also have an arp entry that points to the switch mac. Therefore, the current code should be ok.

meanwhile, I think it is good to have the check, but I think we should assign a different neighbor IP here. I do not think we can have same IP assigned to two different macs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Guohan.. the current code already filters out if the arp is not present and there is no valid scenario we get an arp entry (for nbr) with switch mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I invented it as I did not see the arp from the switch. I'll change the IPs. The reason to have this test case is to ensure that condition mac != switch_mac is hit. Will update the IP address to a different one

"neigh": "f4:8e:38:16:bc:8d",
"family": "IPv4"
},
"OP": "SET"
},
],
"fdb": [
{
"FDB_TABLE:Vlan1000:72-06-00-01-00-08": {
"type": "dynamic",
"port": "Ethernet22"
},
"OP": "SET"
},
{
"FDB_TABLE:Vlan1000:F4-8E-38-16-BC-8D": {
"type": "dynamic",
"port": "Ethernet22"
},
"OP": "SET"
},
],
"config_db": {
"DEVICE_METADATA": {
"localhost": {
"mac": "f4:8e:38:16:bc:8d"
}
},
"VLAN": {
"Vlan1000": {}
},
"VLAN_INTERFACE": {
"Vlan1000": {},
"Vlan1000|192.168.0.1/21": {},
"Vlan1000|fc02:1000::1/64": {}
},
},
"expected_fdb": [
{
"FDB_TABLE:Vlan1000:72-06-00-01-00-08": {
"type": "dynamic",
"port": "Ethernet22"
},
"OP": "SET"
},
],
},
]