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.

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

Conversation

tahmed-dev
Copy link
Contributor

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 tamer.ahmed@microsoft.com

- 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

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 <tamer.ahmed@microsoft.com>
@@ -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.

"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


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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants