Ruby module for Docker DNS updates

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

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.

StackExchange Code Review Q#112654, answer score: 3