-
Notifications
You must be signed in to change notification settings - Fork 681
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
[filter-fdb]: Filter Out FDB Entries With Switch MAC Address #1331
base: master
Are you sure you want to change the base?
[filter-fdb]: Filter Out FDB Entries With Switch MAC Address #1331
Conversation
An entry with switch/router MAC address could be added to the FDB table via fast-boot FDB reprogramming. This PR extend FDB filter to detect those entreis and remove them. signed-off-by: Tamer Ahmed <[email protected]>
@@ -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) |
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.
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?
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.
It could, I just did not want to open the json files twice.
"OP": "SET" | ||
}, | ||
{ | ||
"NEIGH_TABLE:Vlan1000:192.168.0.10": { |
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.
is this valid scenario, having one neighbor entry map to two different macs?
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.
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.
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.
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.
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 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
|
||
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 |
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.
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.
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.
OK, will add logs
An entry with switch/router MAC address could be added to the FDB
table via fast-boot FDB reprogramming. This PR extend FDB filter
to detect those entries and remove them.
signed-off-by: Tamer Ahmed [email protected]
- What I did
Filter out FDB entries that switch/router MAC address
- How I did it
new code to filter those entries. The code retrieves the device MAC address and uses this MAC address to filter any FDB entry that has it.
- How to verify it
unit test added
- Previous command output (if the output of a command-line utility has changed)
N/A
- New command output (if the output of a command-line utility has changed)
N/A