Where does :old come from in Orchestration::DNS?

XMol · August 13, 2024, 2:51pm

Hello Foreman Developers!

I’ve started a new custom Foreman Plugin for managing CNAMEs to a FQDN: GitHub - XMol/foreman_cnames: Make Foreman maintain a list of CNAME records for an interface. It is based off the Foreman Plugin Template and I stole most of the changes from an old pull request, which was motivated by a feature request to add support of CNAMEs to Foreman UI (#9388).
I’ve made quite some progress since the other two topics with questions on plugin development (Foreman plugin does not store interfaces changes to database, Cannot effectively overlay methods of DnsInterface by prepending a module, diff). However, I’ve reached the next road block: DNS orchestration.

Problem

Every patch to a NIC is considered a change on the CNAMEs, including the update with zero changes.

Foreman and Proxy versions

RedHat Satellite 6.13.7, which includes (among others)…

satellite-6.13.7-1.el8sat
katello-4.7.0-1.el8sat
foreman-3.5.1.24-1.el8sat
foreman-proxy-3.5.1-1.el8sat

Distribution and version

My development Foreman instance is running on

Red Hat Enterprise Linux release 8.9 “Ootpa” (4.18.0-513.24.1.el8_9.x86_64)

Other relevant data

The issue arises from the patched function pending_dns_record_changes?. Here, old and self are both a Nic::Managed object. But the comparison attr_equivalent?(old.cnames, cnames) always yields false, because old has no cnames. The reason for that in turn is, that old has no nic_id and therefor Rails fails to extract the data from the associated tables.

Here is the current example host alias obtained through foreman-rake console:

irb(main):001:0> ForemanCnames::HostAlias.select("id, nic_id, name, domain_id").first
=> #<ForemanCnames::HostAlias id: 3, name: "satellite-test", nic_id: 7958, domain_id: 1>

irb(main):002:0> Nic::Managed.find(7958).fqdn
=> "satellite-test-01.scc.kit.edu"

irb(main):003:0> Nic::Managed.find(7958).host_aliases.pluck(:id, :nic_id, :name, :domain_id)
=> [[3, 7958, "satellite-test", 1]]

irb(main):004:0> Nic::Managed.find(7958).cnames
=> ["satellite-test.scc.kit.edu"]

By dumping old and self inside pending_dns_record_changes?, I find the following differences:

Attribute	old	self
id	nil	7958
host_id	nil	1937
created_at	nil	2021-05-14 15:29:55.876959000 +0200
updated_at	nil	2024-08-13 09:57:19.855433000 +0200

So there is sufficient information stored in old to run ip, ip6 and hostname, but not for host_aliases.

I found that old is defined shorthandedly in the Orchestration model. From that I conclude that this is not a standard Rails feature. Though I failed to find any line of code that initializes old.

Could somebody kindly give me hints to that?

Thank you very much for your time,
Xavier Mol.

XMol · August 13, 2024, 3:03pm

I had the idea of using old.name to find the NIC back…

def pending_dns_record_changes?
  old_nic = self.type.constantize.find_by_name(old.name)
  !attr_equivalent?(old_nic.cnames, cnames) || super
end

But that might fail when the update to the interface includes a new name, breaking the lookup, right?

jeremylenz · August 13, 2024, 3:05pm

This is an :attr_reader which comes from Ruby itself. It means that you can do nic.old and it will read the value of the instance variable @old.

I see two places it could be defined:

github.com

theforeman/foreman/blob/e80d5558767bb713c13264e821122baea577af76/app/models/nic/managed.rb#L117


      
              # always set to [] so we lose interfaces errors)
              def failure(msg, exception_or_backtrace = nil, dest = :base)
                result = super
                host.errors.add(dest, msg)
                result
              end
          
              # we must also clone host object so we can detect host attributes changes
              def setup_clone
                return if new_record?
                @old = super
                @old.host = host.setup_clone
                @old
              end
            end
          
            Base.register_type(Managed)
          end
          
          require_dependency 'nic/bmc'
          require_dependency 'nic/bond'

github.com

theforeman/foreman/blob/e80d5558767bb713c13264e821122baea577af76/app/models/concerns/interface_cloning.rb#L6


      
          module InterfaceCloning
            extend ActiveSupport::Concern
            # we keep the before update host object in order to compare changes
            def setup_clone(&block)
              return if new_record?
              @old = setup_object_clone(self, &block)
            end
          
            def setup_object_clone(object)
              clone = object.dup
              yield(clone) if block_given?
              # we can't assign using #attributes= because of mass-assign protected attributes (e.g. type)
              (object.changed_attributes.keys - ["updated_at"]).each do |key|
                clone.send "#{key}=", object.changed_attributes[key]
              end
              clone

XMol · August 13, 2024, 3:12pm

Thank you for this incredibly fast reply! I’ll follow up on those leads tomorrow.

XMol · August 14, 2024, 12:07pm

Nic::Managed.setup_clone indeed calls for InterfaceCloning.setup_clone.

irb(main):001:0> Nic::Managed.instance_method(:setup_clone).super_method
=> #<UnboundMethod: InterfaceCloning#setup_clone(&block) /usr/share/foreman/app/models/concerns/interface_cloning.rb:4>

It also sets old.host to a clone of host with its own setup_clone

irb(main):002:0> Host::Managed.instance_method(:setup_clone).source_location
=> ["/usr/share/foreman/app/models/host/base.rb", 334]

But super from there (surprisingly for me) ends up at InterfaceCloning, too!

irb(main):003:0> Host::Managed.instance_method(:setup_clone).super_method
=> #<UnboundMethod: InterfaceCloning#setup_clone(&block) /usr/share/foreman/app/models/concerns/interface_cloning.rb:4>

Same as setup_object_clone, which is mentioned in the block.

irb(main):004:0> Host::Managed.instance_method(:setup_object_clone).source_location
=> ["/usr/share/foreman/app/models/concerns/interface_cloning.rb", 9]

Host::Managed.setup_clone explicitly clones each individual interface, because the host model does not provide details on its interfaces - same as with Nic:Managed and my CNAMEs!

XMol · August 15, 2024, 3:23pm

Still fighting with old. So much in fact that I was tempted to give up and rely on Rails’ Autosave Associations in order to determine whether there are pending changes to CNAMEs.
It works reliably for triggering orchestration, but there are a couple of flaws with it:

Even if just one CNAME changes, Foreman rebuilds all DNS records for that interface. A bit overkill, but that’s just how it has been working all the time before, too.
By virtue of the sequential iteration through the supported record types, CNAMEs are always created and deleted after all other record types. It is likely that DNSes refuse to delete any A/AAAA records, for which CNAME records exist. Thus I somehow need to make Foreman queue the CNAME record changes/deletions first (ideally without changing the entire existing logic).
While deletion of CNAMEs is triggered when appropriate, the wrong CNAMEs are deleted! Orchestration::DNS.queue_dns_update deletes those records affiliated with the old model (which I did not populate with the CNAME records).

Maybe I should stop trying to hook myself into Foreman’s orchestration and instead set up my own callbacks?

Coming back to the original topic, I struggle to differentiate between the old and new CNAMEs.

The NIC model has no details for the existing CNAMEs (Nic::Base has_many :host_aliases, while HostAlias belongs_to :nic)
host_aliases depends on the NIC-ID - no ID, as with old, no host_aliases
Even with an ID, old and self NICs should have the same ID anyway, so old.host_aliases == host_aliases

Somewhere I’m mistaken. When does host_aliases produce database records and when the request records?

XMol · August 16, 2024, 6:20am

host_aliases depends on the NIC-ID - no ID, as with old, no host_aliases

That sparked an idea…

module ForemanCnames::NicExtensions
  def setup_clone
    @old = super
    @old.id = @old.class.find_by_name(@old.name).id
    logger.debug "old cnames #{@old.host_aliases.map(&:cname)}"
    logger.debug "host_aliases in setup_clone #{host_aliases.map(&:cname)}"
  end
end

# in engine.rb
Nic::Managed.prepend ForemanCnames::NicExtensions

After then running a change for the CNAMEs (rename “satellite-test.scc.kit.edu” into “alt.scc.kit.edu”)…

2024-08-16T08:12:01 [D|for|90bd3ab4] old cnames ["satellite-test.scc.kit.edu"]
2024-08-16T08:12:01 [D|for|90bd3ab4] host_aliases in setup_clone ["alt.scc.kit.edu"]

With a receiver, host_aliases queries the database. Without receiver, it refers to the query data?!

XMol · August 16, 2024, 6:30am

With a receiver, host_aliases queries the database. Without receiver, it refers to the query data?!

That is not true, as can be proven easily:

# add one more line for logging in NicExtensions
Foreman::Logging.logger('foreman_cnames').debug "host_aliases of self #{self.host_aliases.map(&:cname)}"

2024-08-16T08:25:11 [D|for|3141837f] old cnames ["alt.scc.kit.edu"]
2024-08-16T08:25:11 [D|for|3141837f] host_aliases in setup_clone ["satellite-test.scc.kit.edu"]
2024-08-16T08:25:11 [D|for|3141837f] host_aliases of self ["satellite-test.scc.kit.edu"]

host_aliases is send to self implicitly, which I was expecting. But why then does it yield a different result then when send to old? After all, both (now) have the same NIC-ID!