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

Fixes #36548 - Avoid parsing interfaces with shared MACs #9761

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jun 29, 2023

In Azure it is possible to see the same interface share the same MAC address. Foreman wants to identify an interface by its MAC address, and doesn't handle it.

This patch makes parse_interfaces? take duplicate MAC addresses into account and ignores them. While it's not pretty, it at least allows other facts to be imported.

At the moment it's completely untested, but allows me to share the concept easier then describing it.

@theforeman-bot
Copy link
Member

Issues: #36548

@ekohl ekohl force-pushed the 36548-ignore-multiple-interfaces-with-the-same-mac branch from 6b3ad95 to 14149f2 Compare July 22, 2024 13:20
@ekohl ekohl marked this pull request as ready for review July 22, 2024 13:21
@ekohl
Copy link
Member Author

ekohl commented Jul 22, 2024

@nofaralfasi would you mind reviewing this?

@ekohl ekohl force-pushed the 36548-ignore-multiple-interfaces-with-the-same-mac branch from 14149f2 to f04eb9c Compare July 22, 2024 17:15
@ekohl
Copy link
Member Author

ekohl commented Jul 30, 2024

The tests are failing because in test/static_fixtures/facts/facts_with_certname.json there's a host where the bridge has the same MAC as the interface as well as various VLANs. I think this is fairly common so I'm not sure what to do with this now.

@ekohl
Copy link
Member Author

ekohl commented Aug 6, 2024

While going over the original bug I realized that the issue wasn't the duplicate MAC but the duplicate interface names. I'll rewrite the PR.

This is the same implementation as the parent class and simple
inheritance should achieve the same thing.
I needed to understand what these methods were doing and the data types.
This helped me and hopefully it helps future readers.
In Azure it is possible to see the same interface share the same MAC
address. Foreman wants to identify an interface by its MAC address, and
doesn't handle it.

This patch makes `parse_interfaces?` take duplicate MAC addresses into
account and ignores them. While it's not pretty, it at least allows
other facts to be imported.
@ekohl
Copy link
Member Author

ekohl commented Aug 6, 2024

I've done some further analysis and I believe my initial impression that the duplicate mac was the problem was correct. Just where it went wrong.

I now thing this is the problem:

changed_count = 0
parser.interfaces.each do |name, attributes|
iface = get_interface_scope(name, attributes).try(:first) || interface_class(name).new(:managed => false)
# create or update existing interface
changed_count += 1 if set_interface(attributes, name, iface)
end

We can see in the actual code there's matching by mac address:

def get_interface_scope(name, attributes, base = interfaces)
case interface_class(name).to_s
# we search bonds based on identifiers, e.g. ubuntu sets random MAC after each reboot se we can't
# rely on mac
when 'Nic::Bond', 'Nic::Bridge'
base.where(:identifier => name)
# for other interfaces we distinguish between virtual and physical interfaces
# for virtual devices we don't check only mac address since it's not unique,
# if we want to update the device it must have same identifier
else
begin
macaddress = Net::Validations.normalize_mac(attributes[:macaddress])
rescue Net::Validations::Error
logger.debug "invalid mac during parsing: #{attributes[:macaddress]}"
end
mac_based = base.where(:mac => macaddress)
if attributes[:virtual]
mac_based.virtual.where(:identifier => name) || find_by_attached_mac(base, mac_based, identifier, attributes)
elsif mac_based.physical.any?
mac_based.physical
elsif !managed
# Unmanaged host's interfaces are just used for reporting, so overwrite based on identifier first
base.where(:identifier => name)
end
end
end

That is not correct.

@ekohl ekohl force-pushed the 36548-ignore-multiple-interfaces-with-the-same-mac branch from f04eb9c to 47639d6 Compare August 6, 2024 13:20
@ekohl
Copy link
Member Author

ekohl commented Aug 6, 2024

I pushed some commits that separate it out. @nofaralfasi had a question about the chef fact parser and that's now a separate commit. There's also one that improves docs because I had to understand what was going on.

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

Successfully merging this pull request may close these issues.

2 participants