Ruby module for Docker DNS updates

Question

I wrote a Ruby module to update DNS zones on certain events of the Docker API. Its purpose is to make container reachable by their name over a network. I know this is also possible through /etc/hosts entries, but in my use case I can not use the name resolution of the docker host.

I'm am pretty new to Ruby so I guess there are a lot of things that can be done better. I am also not a professional when it comes to code structuring, design patterns and things like that. I'm happy to get comments on this as well.

require "dockerdns/version"
require 'docker'
require 'dnsruby'
module Dockerdns
 class DockerDNS
 #==========================================================================
 def self.run!(config)
 DockerDNS.new(config).run!
 end
 #==========================================================================
 attr_reader :config
 def initialize(config)
 @config = config
 @domain = domain
 @reversezone = reversezone
 @dnsserver = dnsserver
 @dockerurl = dockerurl
 @ttl = ttl
 end
 def domain
 config["domain"]
 end
 def reversezone
 config["reversezone"]
 end
 def dnsserver
 config["dnsserver"]
 end
 def ttl
 config["ttl"]
 end
 def dockerurl
 if config["dockerurl"]
 return config["dockerurl"]
 end
 return "/var/run/docker.sock"
 end
 def run!
 Docker.url = dockerurl
 Docker.options[:read_timeout] = 5
 begin
 Docker::Event.stream do |event|
 if event.status == "create" then
 next
 elsif event.status == "start" then
 puts "caught event #{event.status} for container id #{event.id}"
 dnsAddOrUpdate(event.id, domain, reversezone, dnsserver)
 elsif event.status == "die" || event.status == "kill" || event.status == "stop" || event.status == "destroy" then
 puts "caught event #{event.status} for container id #{event.id}"
 dnsDelete(event.id)
 else
 puts "Ignoring Docker Event #{event.status}"
 end
 end
 rescue Docker::Error::TimeoutError
 retry
 rescue Excon::Errors::SocketError
 retry
 rescue Exception => e
 puts "Error while streaming events: #{e}"
 end
 end
 def getContainerIP(id)
 ipAddress = Docker::Container.get(id).json["NetworkSettings"]["IPAddress"]
 return ipAddress
 end
 def getContainerName(id)
 hostname = Docker::Container.get(id).json["Config"]["Hostname"]
 return hostname
 end
 def getARecord(fqdn)
 resolver = Dnsruby::Resolver.new(dnsserver).query(fqdn)
 ipAddress = resolver.answer[0].address.to_s
 return ipAddress
 end
 def getPtrRecord(ipAddress)
 resolver = Dnsruby::Resolver.new(dnsserver).query(ipAddress, "PTR")
 fqdn = resolver.answer[0].domainname.to_s
 return fqdn
 end
 def setARecord(ipAddress, hostname, domain)
 record = "#{hostname}.#{domain}"
 puts "setting a-record #{record}"
 resolver = Dnsruby::Resolver.new(dnsserver)
 update = Dnsruby::Update.new(domain)
 # make sure there is no record yet
 #update.absent(record, 'A')
 # add record
 puts "update.add(#{record}, 'A', #{@ttl}, #{ipAddress})"
 update.add(record, 'A', @ttl, ipAddress)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def deleteARecord(ipAddress, hostname, domain)
 record = "#{hostname}.#{domain}"
 puts "deleting a-record #{record}"
 resolver = Dnsruby::Resolver.new(dnsserver)
 update = Dnsruby::Update.new(domain)
 # delete record
 puts "update.delete(#{record})"
 update.delete(record)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def setPtrRecord(ipAddress, hostname, domain, reversezone)
 record = "#{ipAddress.split('.').last}.#{reversezone}"
 fqdn = "#{hostname}.#{domain}"
 puts "setting ptr-record #{record}"
 resolver = Dnsruby::Resolver.new(dnsserver)
 update = Dnsruby::Update.new(reversezone)
 # make sure there is no record yet
 #update.absent(record)
 # add record
 puts "update.add(#{record}, 'PTR', #{@ttl}, #{fqdn})"
 update.add(record, 'PTR', @ttl, fqdn)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def deletePtrRecord(ipAddress, hostname, domain, reversezone)
 record = "#{ipAddress.split('.').last}.#{reversezone}"
 fqdn = "#{hostname}.#{domain}"
 puts "deleting ptr-record #{record}"
 resolver = Dnsruby::Resolver.new(dnsserver)
 update = Dnsruby::Update.new(reversezone)
 # delete record
 puts "update.delete(#{record})"
 update.delete(record)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def dnsAddOrUpdate(id, domain, reversezone, dnsserver)
 hostname = getContainerName(id)
 ipAddress = getContainerIP(id)
 setARecord(ipAddress, hostname, domain)
 setPtrRecord(ipAddress, hostname, domain, reversezone)
 getARecord("#{hostname}.#{domain}")
 getPtrRecord(ipAddress)
 end
 def dnsDelete(id)
 hostname = getContainerName(id)
 ipAddress = getARecord("#{hostname}.#{domain}")
 deleteARecord(ipAddress, hostname, domain)
 deletePtrRecord(ipAddress, hostname, domain, reversezone)
 end
 end
end

Answer 1

The code is pretty logically laid out. Let's start from the top.

---

attr_reader :config
def initialize(config)
 @config = config
 @domain = domain
 @reversezone = reversezone
 @dnsserver = dnsserver
 @dockerurl = dockerurl
 @ttl = ttl
end
def domain
 config["domain"]
end
def reversezone
 config["reversezone"]
end
def dnsserver
 config["dnsserver"]
end
def ttl
 config["ttl"]
end
def dockerurl
 if config["dockerurl"]
 return config["dockerurl"]
 end
 return "/var/run/docker.sock"
end

Why is there an attr_reader :config? Is it important to expose the configuration as part of the DockerDNS object's interface? (For that matter, do #domain, #reversezone, etc. need to be exposed as well?)

Why does the constructor have @domain = domain, @reversezone = reversezone, etc.? Only @ttl is ever used. Furthermore, the call sequence feels backwards. The job of the constructor is to initialize the object, as its name implies. What you have done, rather, is partially construct the object (@config = config), then call a helper function (#ttl), which in turn calls the attr_reader (#config), to let the constructor cache the value in an instance variable (@ttl = ...). The simpler way would be:

def initialize(config)
 @config = config
end
def domain
 @config['domain']
end
...
def ttl
 @config['ttl']
end
def dockerurl
 @config['dockerurl'] || '/var/run/docker.sock'
end

---

In #run!, the Docker::Event.stream block needs to be indented another level. The if event.status == ... checks would be better written using a case expression.

begin
 Docker::Event.stream do |event|
 case event.status
 when 'create'
 next
 when 'start'
 ...
 when 'die', 'kill', 'stop', 'destroy'
 ...
 else
 puts "Ignoring Docker Event #{event.status}"
 end
 end
rescue ...
 ...
end

When retrying after an error, it would be a good idea to add some delay. Otherwise, you could be retrying an operation furiously and making a bad situation worse.

---

You have a

def dnsserver
 config["dnsserver"]
end

... but that method is not as useful as it could be. What you really want is to simplify the resolver = Dnsruby::Resolver.new(dnsserver) calls that you have all over the place. So, instead of #dnsserver, it would be better to have

def resolver
 @resolver ||= Dnsruby::Resolver.new(@config['dnsserver'])
end

That lazily constructs a Resolver the first time it is needed, and keeps the same object around for subsequent uses.

---

I don't recommend handling exceptions at this level:

def setARecord(...)
 ...
 update.add(record, 'A', @ttl, ipAddress)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
end

The error will show up on screen, but the rest of your program (#dnsAddOrUpdate) has no idea that anything went wrong, and proceeds happily with other operations. I suggest just letting the exception propagate — perhaps all the way to the run loop.

Answer 2

Here are some notes from a quick once-over. I don't know the Docker API, nor have I played much with automated DNS updating, so I've just looked at the Ruby-ness of things.

1. The Ruby convention is snake_case for methods and variables. Not camelCase

2. Dockerdns::DockerDNS.... why wrap a class in a module of same name (but not quite the same name)? I mean, putting things in a module is a great way to namespace things, but the naming seems redundant and confusing.

3. There's no reason to have accessor methods for hash values and set instance variables (that you never use). So your initialize method can be reduced to one line.

4. Following the above: Why pass things to instance methods that the methods themselves can already access? E.g.

   dnsAddOrUpdate(event.id, domain, reversezone, dnsserver)
   

   could just be

   dnsAddOrUpdate(event.id)
   

5. Don't do this sort of thing

   def some_value
    x = expression.that_gets.the_value
    return x
   end
   

   For one, the return is implicit. For another, there's no reason to assign anything just to return it. So for instance getContainerIp (sic; should be snake_case) can just be this:

   def getContainerIP(id)
    Docker::Container.get(id).json["NetworkSettings"]["IPAddress"]
   end
   

   No variable assignment, no explicit return. Similar thing for dockerurl (which should really be docker_url) which can be reduced to

   def dockerurl
    config["dockerurl"] || "/var/run/docker.sock"
   end
   

6. Don't use then unless you're prefixing a conditional on one line. Mostly, though, you'd postfix the conditional, e.g.

   next if event.status == "create"
   

   Of course, this particular line doesn't actually matter much, as the following lines have their own conditionals and won't trigger on "create" anyway.

7. It'd probably be good limit the retrys. And don't rescue Exception; rescue StandardError instead.

   Or let exceptions bubble up - they could be important, and should no doubt be handled, not just printed and forgotten.

8. DRY - Don't Repeat Yourself. E.g. you call Docker::Container.get(id).json in multiple places; wrap that in a method. If the get(id) call results in an API request each time, you should probably cache it too. No reason to fetch the same JSON multiple times just to pull different values out.

   Same goes for Dnsruby::Resolver.new(dnsserver) and other operations that are very much repeated in multiple places.

9. Don't use some_array[0] - use some_array.first.

10. I'd place more logic in separate classes, so that each event, and the actions it might cause are encapsulated.

Here's an attempt at refactoring that takes most of the above into account. Mostly it just offloads responsibilities to a Container class, since that's the unit you're manipulating. Don't just run it - I haven't tested it, and I put it together just to show a different structure. Can't guarantee it'll even work right.

module DockerDns
class Updater
 def self.run!(config)
 Updater.new(config).run!
 end
 def initialize(config)
 @config = config
 # Note: Docker config should be set elsewhere as it's global.
 # It it not the responsibility of this class. I've left it in
 # to more easily compare the code, but it should go.
 Docker.url = @config["dockerurl"] || "/var/run/docker.sock"
 Docker.options[:read_timeout] = 5
 end
 def run!
 Docker::Event.stream do |event|
 container = Container.new(id, @config)
 case event.status
 when "start"
 config.create_or_update_dns_records!
 when "die", "kill", "stop", "destroy"
 config.delete_dns_records!
 else
 puts "Ignoring Docker Event #{event.status}"
 end
 end
 rescue Docker::Error::TimeoutError, Excon::Errors::SocketError
 retry
 rescue StandardError => e
 puts "Error while streaming events: #{e}"
 end
end
class Container
 def initialize(id, config)
 @id = id
 @config = config
 end
 def create_or_update_dns_records!
 set_a_record!
 set_ptr_record!
 end
 def delete_dns_records!
 delete_a_record!
 delete_ptr_record!
 end
 def ptr_record
 @ptr_record ||= resolver.query(ip_address, "PTR").answer.first.domainname.to_s
 end
 def a_record
 @a_record ||= resolver.query(fqdn).answer.first.address.to_s
 end
 def set_ptr_record!
 update = Dnsruby::Update.new(reversezone)
 update.add(record, 'PTR', @config["ttl"], fqdn)
 resolver.send_message(update)
 end
 def set_a_record!
 update = Dnsruby::Update.new(domain)
 update.add(fqdn, 'A', @config["ttl"], ip_address)
 resolver.send_message(update)
 end
 def delete_ptr_record!
 update = Dnsruby::Update.new(reversezone)
 update.delete(record)
 resolver.send_message(update)
 end
 def delete_a_record!
 update = Dnsruby::Update.new(domain)
 update.delete(record)
 resolver.send_message(update)
 end
 def fqdn
 "#{hostname}.#{domain}"
 end
 def domain
 @config["domain"]
 end
 def hostname
 json["Config"]["Hostname"]
 end
 def ip_address
 json["NetworkSettings"]["IPAddress"]
 end
 def record
 "#{ipAddress.split('.').last}.#{@config["reversezone"]}"
 end
 private
 def resolver
 @resolver ||= Dnsruby::Resolver.new(@config["dnsserver"])
 end
 def json
 @json ||= Docker::Container.get(@id).json
 end
end
end

Answer 3

Thanks for your review guys. Here's what I changed

---

require 'docker'
require 'dnsruby'
class DockerDNS
 #==========================================================================
 def self.run!(config)
 DockerDNS.new(config).run!
 end
 #==========================================================================
 def initialize(config)
 @config = config
 end
 def domain
 @config["domain"]
 end
 def reversezone
 @config["reversezone"]
 end
 def resolver
 @resolver ||= Dnsruby::Resolver.new(@config['dnsserver'])
 end
 def ttl
 @config["ttl"]
 end
 def docker_url
 @config["dockerurl"] || '/var/run/docker.sock'
 end

• reduced the initialize() and docker_url() method to one line
• changed dnsserver to resolver to get a resolver instance

---

def run!
 Docker.url = docker_url
 Docker.options[:read_timeout] = 5
 begin
 Docker::Event.stream do |event|
 case event.status
 when "create"
 next
 when "start"
 puts "caught event #{event.status} for container id #{event.id}"
 create_or_update_dns_records!(event.id, domain)
 when "die", "kill", "stop", "destroy"
 puts "caught event #{event.status} for container id #{event.id}"
 delete_dns_records!(event.id)
 else
 puts "Ignoring Docker Event #{event.status}"
 end
 end
 rescue Docker::Error::TimeoutError, Excon::Errors::SocketError
 retry
 rescue StandardError => e
 puts "Error while streaming events: #{e}"
 end
 end

• refactored to case statement
• catching StandardError now

---

def container_ip(id)
 Docker::Container.get(id).json["NetworkSettings"]["IPAddress"]
 end
 def container_name(id)
 Docker::Container.get(id).json["Config"]["Hostname"]
 end
 def a_record(fqdn)
 resolver.answer.first.address.to_s
 end
 def ptr_record(ipAddress)
 resolver.query(ipAddress, "PTR").answer.first.domainname.to_s
 end

• changed these methods to one liners too
• refactored method names to snake_case

---

def set_a_record(ipAddress, hostname)
 record = "#{hostname}.#{domain}"
 puts "setting a-record #{record}"
 update = Dnsruby::Update.new(domain)
 # add record
 puts "update.add(#{record}, 'A', #{ttl}, #{ipAddress})"
 update.add(record, 'A', ttl, ipAddress)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def delete_a_record(ipAddress, hostname)
 record = "#{hostname}.#{domain}"
 puts "deleting a-record #{record}"
 update = Dnsruby::Update.new(domain)
 # delete record
 puts "update.delete(#{record})"
 update.delete(record)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def set_ptr_record(ipAddress, hostname)
 record = "#{ipAddress.split('.').last}.#{reversezone}"
 fqdn = "#{hostname}.#{domain}"
 puts "setting ptr-record #{record}"
 update = Dnsruby::Update.new(reversezone)
 # add record
 puts "update.add(#{record}, 'PTR', #{ttl}, #{fqdn})"
 update.add(record, 'PTR', ttl, fqdn)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def delete_ptr_record(ipAddress, hostname)
 record = "#{ipAddress.split('.').last}.#{reversezone}"
 fqdn = "#{hostname}.#{domain}"
 puts "deleting ptr-record #{record}"
 update = Dnsruby::Update.new(reversezone)
 # delete record
 puts "update.delete(#{record})"
 update.delete(record)
 # send update
 begin
 reply = resolver.send_message(update)
 puts "Update succeeded"
 rescue Exception => e
 puts "Update failed: #{e}"
 end
 end
 def create_or_update_dns_records!(id)
 hostname = container_name(id)
 ipAddress = container_ip(id)
 set_a_record(ipAddress, hostname)
 set_ptr_record(ipAddress, hostname)
 a_record("#{hostname}.#{domain}")
 ptr_record(ipAddress)
 end
 def delete_dns_records!(id)
 hostname = container_name(id)
 ipAddress = a_record("#{hostname}.#{domain}")
 delete_a_record(ipAddress, hostname)
 delete_ptr_record(ipAddress, hostname)
 end
end

The rest of the method profit from these changes as they are a bit more compact. I also removed the unnecessary parameters.

If there were more use cases for this code, I'd consider splitting it up into separate classes as suggested by Flambino