WIP: Changes from PR feedback

config/settings.d/externalipam_netbox.yml.example

@@ -1,3 +1,6 @@
 ---
-:url: 'http://my-netbox-url.com'
-:token: 'netbox_token'
+# url is the hostname and path of the Netbox instance
+:url: 'http://netbox-example.com'
+
+# token is the Netbox API token
+:token: 'netbox_token'

config/settings.d/externalipam_phpipam.yml.example

@@ -1,4 +1,12 @@
 ---
-:url: 'http://my-phpipam-url.com'
+# url is the hostname and path of the phpIPAM instance.
+:url: 'http://phpipam-example.com'
+
+# The phpIPAM user name for authentication. Please note that an API Key also needs to be 
+# setup with the exact same name as the user name configured here. When setting up the API 
+# Key in phpIPAM, "User token" must be used for the "App Security" setting.
 :user: 'ipam_user'
+
+# The password for above user account. Note that this is the password of the user, and not 
+# the API Key itself.
 :password: 'ipam_password'

modules/dhcp_common/free_ips.rb

@@ -29,11 +29,11 @@ def mark_ip_as_allocated(ip_address)
     def clean_up_allocated_ips
       @m.synchronize do
         loop do
-          logger.debug("Starting allocated ip addresses cleanup pass...")
+          logger.trace("Starting allocated ip addresses cleanup pass for DHCP ...")
           break if @allocation_timestamps.first.nil? || @allocation_timestamps.first[1] > time_now
           ip, _ = @allocation_timestamps.shift
           @allocated_ips.delete(ip)
-          logger.debug("#{ip} marked as free.")
+          logger.trace("#{ip} marked as free for DHCP.")
         end
       end
     end

modules/externalipam/api_resource.rb

@@ -16,26 +16,22 @@ def initialize(params = {})
       @auth_header = params[:auth_header] || 'Authorization'
     end
 
-    def get(path)
-      uri = URI(@api_base + path)
+    def get(path, params = nil)
+      url = @api_base + path
+      url += "?#{URI.encode_www_form(params)}" if params
+      uri = URI(url)
       request = Net::HTTP::Get.new(uri)
       request[@auth_header] = @token
       request['Accept'] = 'application/json'
-
-      Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http|
-        http.request(request)
-      end
+      request(request, uri)
     end
 
     def delete(path)
       uri = URI(@api_base + path)
       request = Net::HTTP::Delete.new(uri)
       request[@auth_header] = @token
       request['Accept'] = 'application/json'
-
-      Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http|
-        http.request(request)
-      end
+      request(request, uri)
     end
 
     def post(path, body = nil)
@@ -45,7 +41,12 @@ def post(path, body = nil)
       request[@auth_header] = @token
       request['Accept'] = 'application/json'
       request['Content-Type'] = 'application/json'
+      request(request, uri)
+    end
+
+    private
 
+    def request(request, uri)
       Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http|
         http.request(request)
       end

modules/externalipam/externalipam_plugin.rb

@@ -5,7 +5,7 @@ module Proxy::Ipam
   class Plugin < ::Proxy::Plugin
     plugin :externalipam, ::Proxy::VERSION
     uses_provider
-    default_settings use_provider: 'externalipam_phpipam'
+    default_settings use_provider: nil
     rackup_path File.expand_path('http_config.ru', __dir__)
   end
 end

modules/externalipam/ip_cache.rb

@@ -17,65 +17,61 @@ class IpCache
 
     def initialize
       @m = Monitor.new
-      init_cache
+      @ip_cache = {'': {}}
       start_cleanup_task
     end
 
     def provider_name(provider)
       @provider = provider
     end
 
-    def set_group(group, value)
-      @ip_cache[group.to_sym] = value
+    def get_cidr(group_name, cidr)
+      @ip_cache.dig(group_name.to_sym, cidr.to_sym)
     end
 
-    def get_group(group)
-      @ip_cache[group.to_sym]
+    def get_ip(group_name, cidr, mac)
+      @ip_cache.dig(group_name.to_sym, cidr.to_sym, mac.to_sym, :ip)
     end
 
-    def get_cidr(group, cidr)
-      @ip_cache[group.to_sym][cidr.to_sym]
+    def ip_exists(group_name, cidr, ip)
+      subnet_hash = get_cidr(group_name, cidr)
+      return false if subnet_hash.nil?
+      subnet_hash&.any? { |mac, cached_ip| cached_ip[:ip] == ip }
     end
 
-    def get_ip(group_name, cidr, mac)
-      @ip_cache[group_name.to_sym][cidr.to_sym][mac.to_sym][:ip]
+    def ip_expired(group_name, cidr, ip)
+      return true unless ip_exists(group_name, cidr, ip)
+      subnet_hash = get_cidr(group_name, cidr)
+      subnet_hash&.any? { |mac, cached_ip| cached_ip[:ip] == ip && expired(cached_ip[:timestamp]) }
     end
 
     def cleanup_interval
       DEFAULT_CLEANUP_INTERVAL
     end
 
-    def ip_exists(ip, cidr, group_name)
-      cidr_key = @ip_cache[group_name.to_sym][cidr.to_sym]&.to_s
-      cidr_key.include?(ip.to_s)
-    end
-
-    def add(ip, mac, cidr, group_name)
+    def add(group_name, cidr, ip, mac = nil)
       logger.debug("Adding IP '#{ip}' to cache for subnet '#{cidr}' in group '#{group_name}' for IPAM provider #{@provider}")
       @m.synchronize do
         mac_addr = mac.nil? || mac.empty? ? SecureRandom.uuid : mac
         group_hash = @ip_cache[group_name.to_sym]
 
-        group_hash.each do |key, values|
-          if values.key? mac_addr.to_sym
-            @ip_cache[group_name.to_sym][key].delete(mac_addr.to_sym)
-          end
-          @ip_cache[group_name.to_sym].delete(key) if @ip_cache[group_name.to_sym][key].nil? || @ip_cache[group_name.to_sym][key].empty?
-        end
-
-        if group_hash.key?(cidr.to_sym)
-          @ip_cache[group_name.to_sym][cidr.to_sym][mac_addr.to_sym] = {ip: ip.to_s, timestamp: Time.now.to_s}
+        if group_hash&.key?(cidr.to_sym)
+          @ip_cache[group_name.to_sym][cidr.to_sym][mac_addr.to_sym] = {ip: ip.to_s, timestamp: Time.now}
         else
-          @ip_cache = @ip_cache.merge({group_name.to_sym => {cidr.to_sym => {mac_addr.to_sym => {ip: ip.to_s, timestamp: Time.now.to_s}}}})
+          @ip_cache[group_name.to_sym] = {cidr.to_sym => {mac_addr.to_sym => {ip: ip.to_s, timestamp: Time.now}}}
         end
       end
     end
 
     private
 
+    def expired(ip_expiration)
+      Time.now - ip_expiration > DEFAULT_CLEANUP_INTERVAL
+    end
+
     def start_cleanup_task
-      logger.info("Starting ip cache maintenance for IPAM provider #{@provider}, used by /next_ip.")
-      @timer_task = Concurrent::TimerTask.new(execution_interval: DEFAULT_CLEANUP_INTERVAL) { init_cache }
+      logger.info("Starting ip cache maintenance for External IPAM provider, used by /next_ip.")
+      @timer_task = Concurrent::TimerTask.new(execution_interval: DEFAULT_CLEANUP_INTERVAL) { clean_cache }
       @timer_task.execute
     end
 
@@ -109,24 +105,26 @@ def start_cleanup_task
     #     }
     #   }
     # }
-    def init_cache
+    def clean_cache
       @m.synchronize do
-        if @ip_cache && !@ip_cache.empty?
-          logger.debug("Processing ip cache for IPAM provider #{@provider}")
-          @ip_cache.each do |group, subnets|
-            subnets.each do |cidr, macs|
-              macs.each do |mac, ip|
-                if Time.now - Time.parse(ip[:timestamp]) > DEFAULT_CLEANUP_INTERVAL
-                  @ip_cache[group][cidr].delete(mac)
-                end
+        entries_deleted = 0
+        total_entries = 0
+        @ip_cache.each do |group, subnets|
+          subnets.each do |cidr, macs|
+            macs.each do |mac, ip|
+              if expired(ip[:timestamp])
+                @ip_cache[group][cidr].delete(mac)
+                entries_deleted += 1
               end
-              @ip_cache[group].delete(cidr) if @ip_cache[group][cidr].nil? || @ip_cache[group][cidr].empty?
+              total_entries += 1
             end
+            @ip_cache[group].delete(cidr) if @ip_cache[group][cidr].nil? || @ip_cache[group][cidr].empty?
+            @ip_cache.delete(group) if @ip_cache[group].nil? || @ip_cache[group].empty?
           end
-        else
-          logger.debug("Clearing ip cache for IPAM provider #{@provider}")
-          @ip_cache = {'': {}}
         end
+        cache_count = total_entries - entries_deleted
+        logger.debug("Removing #{entries_deleted} entries from IP cache for IPAM provider #{@provider}") if entries_deleted > 0
+        logger.debug("Current count of IP cache entries for IPAM provider #{@provider}: #{cache_count}") if entries_deleted > 0
       end
     end
   end