Foreman DNS: Converting IP's to PTR-Records

helge000 · February 22, 2016, 11:50am

Hello dev team,

currently, when creating/deleting PTR's Foreman converts IP's to PTR
records [1] (hopefully the right place) it seems before sending them off
as attributes to smart-proxy. Turns out, all the tests in smart-proxy
are not honoring this fact. [2]

Can somebody explain to me the reasoning behind this?

I am asking of course because I question it as it leads to issues
downstream with smart-proxy, as I am currently experiencing them [3]:

I think the provider / smart-proxy should be checking for duplicates
as foreman might use other / unreachable name resolution. Nevertheless,
the implemented providers do so already
Reverse DNS find, which we use to search for duplicates, only works
with IP addresses AFAIK
There is no reliable way I know of to to convert a PTR record back to
an IP address. While this might be done with some questionable
.split('.').reverse with IPv4, this gets much more complicated with IPv6

We should remove complexion from Foreman, always send the IP to
smart-proxy and let the provider figure out what method would be best to
set it.
This is a breaking API change; though I think PTRs are currently noop
anyway the way smart-proxy is implementing them.

Thanks!

[1]

github.com

theforeman/foreman/blob/8bbfa2c36cb07d6c9d48631384b051c2532c4e67/lib/net/dns/ptr_record.rb#L41


    def valid?
      logger.debug "Fetching DNS reservation for #{self}"
      self == dns_lookup(ip)
    end


    def a
      dns_lookup(hostname)
    end


    def attrs
      { :fqdn => hostname, :value => to_arpa, :type => type }
    end


    private


    # Returns: String containing the ip in the in-addr.arpa zone
    def to_arpa
      ip.split(/\./).reverse.join(".") + ".in-addr.arpa"
    end
  end
end

[2]

github.com

theforeman/smart-proxy/blob/develop/test/dns_dnscmd/dnscmd_test.rb#L53


def test_remove_address_records_with_longest_zone_match
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:enum_records).with('bar.domain.local', 'host.foo.bar.domain.local',  'A').returns(['1.1.1.1','2.2.2.2'])
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_specific_record_from_zone).with('bar.domain.local', 'host.foo.bar.domain.local', '1.1.1.1', 'A').returns(true)
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_specific_record_from_zone).with('bar.domain.local', 'host.foo.bar.domain.local', '2.2.2.2', 'A').returns(true)
  assert_nil @server.do_remove('host.foo.bar.domain.local', 'A')
end


def test_remove_ptr_records
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:enum_records).with('33.168.192.in-addr.arpa', '33.33.168.192.in-addr.arpa',  'PTR').returns(['host.domain.local'])
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:remove_specific_record_from_zone).with('33.168.192.in-addr.arpa', '33.33.168.192.in-addr.arpa', 'host.domain.local', 'PTR').returns(true)
  assert_nil @server.do_remove('33.33.168.192.in-addr.arpa', 'PTR')
end


def test_remove_specific_a_record_from_zone
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:execute).with('/RecordDelete domain.local host.domain.local. A 192.168.33.33 /f', anything).returns(true)
  assert_nil @server.remove_specific_record_from_zone('domain.local', 'host.domain.local', '192.168.33.33', 'A')
end


def test_remove_specific_ptr_record_from_zone
  Proxy::Dns::Dnscmd::Record.any_instance.expects(:execute).with('/RecordDelete 33.168.192.in-addr.arpa 33.33.168.192.in-addr.arpa. PTR host.domain.local /f', anything).returns(true)
  assert_nil @server.remove_specific_record_from_zone('33.168.192.in-addr.arpa', '33.33.168.192.in-addr.arpa', 'host.domain.local', 'PTR')

github.com

theforeman/smart-proxy/blob/develop/test/dns_nsupdate/dns_nsupdate_test.rb#L35




def test_create_aaaa_record
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_connect).returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate).with('update add some.host. 100 AAAA 2001:db8::1').returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_disconnect).returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_close)


  assert_nil Proxy::Dns::Nsupdate::Record.new(nil, 100, nil).do_create('some.host', '2001:db8::1', 'AAAA')
end


def test_create_cname_record
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_connect).returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate).with('update add alias.host. 100 CNAME some.host').returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_disconnect).returns(true)
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_close)


  assert_nil Proxy::Dns::Nsupdate::Record.new(nil, 100, nil).do_create('alias.host', 'some.host', 'CNAME')
end


def test_remove_ptr_v4_record
  Proxy::Dns::Nsupdate::Record.any_instance.expects(:nsupdate_connect).returns(true)

[3] https://github.com/theforeman/smart-proxy/pull/379

···

-- Daniel Helgenberger (helge000) daniel@helgenberger.net

Dmitri_Dolguikh · February 22, 2016, 12:03pm

> Hello dev team,
>
> currently, when creating/deleting PTR's Foreman converts IP's to PTR
> records [1] (hopefully the right place) it seems before sending them off
> as attributes to smart-proxy. Turns out, all the tests in smart-proxy
> are not honoring this fact. [2]
>
> Can somebody explain to me the reasoning behind this?
>
> I am asking of course because I question it as it leads to issues
> downstream with smart-proxy, as I am currently experiencing them [3]:
>
> - I think the provider / smart-proxy should be checking for duplicates
> as foreman might use other / unreachable name resolution. Nevertheless,
> the implemented providers do so already

For MS Dns this makes sense for create PTR operations only, deletes
will always succeed, whether record exists or not.

> - Reverse DNS find, which we use to search for duplicates, only works
> with IP addresses AFAIK

Please see Fixes #13536 - working ptr's for dnscmd by helge000 · Pull Request #379 · theforeman/smart-proxy · GitHub,
where I suggest a solution that will work.

> - There is no reliable way I know of to to convert a PTR record back to
> an IP address. While this might be done with some questionable
> .split('.').reverse with IPv4, this gets much more complicated with IPv6

See above.

>
>
> We should remove complexion from Foreman, always send the IP to
> smart-proxy and let the provider figure out what method would be best to
> set it.
> This is a breaking API change; though I think PTRs are currently noop
> anyway the way smart-proxy is implementing them.

I think it’s highly unlikely that a breaking change such as the one
you are suggesting will be implemented, esp. considering there’s a way
to make provider work without changing the api. That aside, versioning
of api has to happen before such a change can be safely implemented.

Cheers,
-d

···

On Mon, Feb 22, 2016 at 11:50 AM, Daniel Helgenberger wrote:

Thanks!

[1]
foreman/lib/net/dns/ptr_record.rb at 8bbfa2c36cb07d6c9d48631384b051c2532c4e67 · theforeman/foreman · GitHub
[2]
smart-proxy/test/dns_dnscmd/dnscmd_test.rb at develop · theforeman/smart-proxy · GitHub
smart-proxy/test/dns_nsupdate/dns_nsupdate_test.rb at develop · theforeman/smart-proxy · GitHub
[3] Fixes #13536 - working ptr's for dnscmd by helge000 · Pull Request #379 · theforeman/smart-proxy · GitHub

Daniel Helgenberger (helge000)
daniel@helgenberger.net

–
You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

ekohl · February 22, 2016, 12:02pm

> Hello dev team,
>
> currently, when creating/deleting PTR's Foreman converts IP's to PTR
> records [1] (hopefully the right place) it seems before sending them off
> as attributes to smart-proxy. Turns out, all the tests in smart-proxy
> are not honoring this fact. [2]
>
> Can somebody explain to me the reasoning behind this?

It is a bug in our tests. I have a commit[4] in PR 371[5] that fixes
this. I'll create a separate PR for this.

> I am asking of course because I question it as it leads to issues
> downstream with smart-proxy, as I am currently experiencing them [3]:
>
> - I think the provider / smart-proxy should be checking for duplicates
> as foreman might use other / unreachable name resolution. Nevertheless,
> the implemented providers do so already
> - Reverse DNS find, which we use to search for duplicates, only works
> with IP addresses AFAIK
> - There is no reliable way I know of to to convert a PTR record back to
> an IP address. While this might be done with some questionable
> .split('.').reverse with IPv4, this gets much more complicated with IPv6

It's trivial to convert a PTR record to an IP, both for IPv4 and IPv6.

> We should remove complexion from Foreman, always send the IP to
> smart-proxy and let the provider figure out what method would be best to
> set it.
> This is a breaking API change; though I think PTRs are currently noop
> anyway the way smart-proxy is implementing them.

I'm pretty sure it currently works. You can use my integration test[6]
to verify it. You only need to modify it to point to the correct
nameserver, smart-proxy and remove the purge_cache (or adapt it to your
backend if it needs it).

I can expand on this if you like or maybe poke me on IRC (ewoud) to
debug together.

> [1]
> https://github.com/theforeman/foreman/blob/8bbfa2c36cb07d6c9d48631384b051c2532c4e67/lib/net/dns/ptr_record.rb#L41
> [2]
> https://github.com/theforeman/smart-proxy/blob/develop/test/dns_dnscmd/dnscmd_test.rb#L53
> https://github.com/theforeman/smart-proxy/blob/develop/test/dns_nsupdate/dns_nsupdate_test.rb#L35
> [3] https://github.com/theforeman/smart-proxy/pull/379

[4] https://github.com/ekohl/smart-proxy/commit/7f6d5bc7d59afa09854ade3ce07e0786157e5f90
[5] https://github.com/theforeman/smart-proxy/pull/371
[6] https://github.com/theforeman/smart_proxy_dns_powerdns/blob/master/test/integration/integration_test.rb

···

On Mon, Feb 22, 2016 at 12:50:41PM +0100, Daniel Helgenberger wrote:

ekohl · February 22, 2016, 2:51pm

I don't think we need to change the Foreman <-> Proxy API for this.
Besides that, we created a consistent internal API with create_record
and remove_record. The only inconsistent thing there is
remove_ptr_record is passed a parameter named ip while it's the reverse
IP notation.

I submitted https://github.com/theforeman/smart-proxy/pull/384 to make
the unit tests consistent with the real API.

···

On Mon, Feb 22, 2016 at 12:03:08PM +0000, Dmitri Dolguikh wrote: > On Mon, Feb 22, 2016 at 11:50 AM, Daniel Helgenberger > wrote: > > We should remove complexion from Foreman, always send the IP to > > smart-proxy and let the provider figure out what method would be best to > > set it. > > This is a breaking API change; though I think PTRs are currently noop > > anyway the way smart-proxy is implementing them. > > I think it’s highly unlikely that a breaking change such as the one > you are suggesting will be implemented, esp. considering there’s a way > to make provider work without changing the api. That aside, versioning > of api has to happen before such a change can be safely implemented.