Foreman 3.17.2 messed up interface facts

Support
1

Problem:
I have just upgraded from 3.16.2 to 3.17.2.

We have a couple of servers which have a bond interface with a couple of vlan interfaces on top, one of those is the primary interface. We generate the nmconnection files for all the configured interfaces in foreman. With 3.16.2 this was no problem.

Now with the upgrade to 3.17.2 all the servers which have this kind of setup want to set the primary interface to bond instead of vlan.

In the interface configuration in foreman the interface bond0.601 now appears as “Type Bond” instead of “Type Interface” with “Virtual NIC” selected.

If I change the interface configuration of bond0.601 in foreman, the next puppet run on the server will reset it back to type bond. So it seems to pick that up from the server facts. But it’s unclear to me which it’s using. “facter” output looks as usual.

How does foreman determine the interface type from facts?

Expected outcome:
Recognizing primary bond0.601 interface as type vlan and not type bond.

Foreman and Proxy versions:
katello-4.19.2-1.el9.noarch
foreman-3.17.2-1.el9.noarch

Distribution and version:
AlmaLinux 9.7

2

If I am not mistaken this was introduced with this pr: Fixes #38870 - Update bond regex for bond interfaces numbered 10+ or hyphenated names by chris1984 · Pull Request #10744 · theforeman/foreman · GitHub

It sets the regex for bonds to

BONDS = /\A(bond[\d-].*)\Z|\A(lagg\d)\Z/

which means that bond0.601 matches now, which was not the case before (and which tests refuted before the commit).

On the other hand, the interface_class method

    def interface_class(name)
      case name
        when FactParser::BONDS
          Nic::Bond
        when FactParser::BRIDGES
          Nic::Bridge
        else
          Nic::Managed
      end
    end

does not check for VLANs at all. So either the BOND regex should be adjusted to reject interface names with a . (dot) in the name or the interface_class method should specifically check for VLANS first and return Nic::Managed then.

3

The commit was by @cintrix84 I think. @jeremylenz signed it off. (sorry or the poking…)

4

Quick and dirty workaround for the moment:

Change file /usr/share/foreman/app/services/fact_parser.rb, line 5 from

  BONDS = /\A(bond[\d-].*)\Z|\A(lagg\d)\Z/

to

  BONDS = /\A(bond[\d-][^.]*)\Z|\A(lagg\d)\Z/

This won’t match bond interfaces with a . (dot) in the name.

Now the network configuration is correct again and the primary vlan interface in the bond is a vlan interface again.

5

@jpasqualetto Are you able to test the bug again with the fix @gvde posted to see if it fixes his issue and still solves the issue we shipped? I know you had a way to reproduce the environment better than we could.

6

My change isn’t a fix, it’s just a quick workaround to get it properly working for me.

I think a : (colon) and possibly a _ (underscore) should also be left out. At least the other patterns for VIRTUAL etc. suggest they would be special, too. I think the pattern suggested in the initial text of the PR /\A(bond[\d-]+|bond-[a-z0-9-]+)\Z/ would also be better then the current one.

In general, of course, this is a can of worms. The patterns try to guess the interface type from the name based on various past and current naming schemes.

Generally, an network interface can be any name in any convention. Someone might call the bond interface abc or even eth0 (if that’s not used otherwise, I guess). I am somewhat surprised to see that the facts collected on the hosts do not include the actual interface type. That is the core of the whole issue.

Thus, technically, to really solve the whole issue, facter and whatever else might be used to collect facts on the hosts should be extended to include the interface type. Then the facts parser knows the actual interface type and doesn’t have to guess. The regex would only apply if the interface type fact is missing when using an old facter version.

The secondary issue at this point seems to me that foreman is actually using the derived interface type from those patterns and uses it to overwrite the interface type set in the current primary interface. The patterns come useful if the host is initially discovered and foreman tries to determine automatically what those interfaces are (and facts don’t include that information).

But once configured, it doesn’t seem like a good idea to modify the current host primary interface configuration based on those patterns. As mentioned above, the name can be anything and if I decide to name my bond abc I could configure it in foreman exactly like that without a problem. But during the next puppet agent run, foreman changes it…

foreman should not change the interface type later only because it doesn’t recognize abc as bond interface…

7

@cintrix84 I ran a quick test with @gvde workaround and it worked fine. It doesn’t break what we were fixing.

8

As mentioned before, you should also ignore alias interfaces with a colon.

I think those three test:

    refute_match FactParser::BONDS, 'bond0.0'
    refute_match FactParser::BONDS, 'bond0:0'
    refute_match FactParser::BONDS, 'bond0.0:0'

should be kept in there. They were removed. Those three interface names should not be considered bond interfaces…

1 Like