Improve zabbix_proxy type

lib/puppet/provider/zabbix.rb

@@ -76,13 +76,6 @@ def check_host(host)
     false
   end
 
-  # Check if proxy exists. When error raised, return false.
-  def check_proxy(host)
-    zbx.proxies.get_id(host: host)
-  rescue Puppet::ExecutionFailure
-    false
-  end
-
   # Get the template id from the name.
   def get_template_id(zbx, template)
     return template if a_number?(template)

lib/puppet/provider/zabbix_proxy/ruby.rb

@@ -2,26 +2,16 @@
 Puppet::Type.type(:zabbix_proxy).provide(:ruby, parent: Puppet::Provider::Zabbix) do
   confine feature: :zabbixapi
 
-  def self.instances
-    proxies = zbx.query(
-      method: 'proxy.get',
-      params: {
-        output: 'extend',
-        selectInterface: %w[interfaceid type main ip port useip]
-      }
-    )
+  def initialize(value = {})
+    super(value)
+    @property_flush = {}
+  end
+
+  mk_resource_methods
 
+  def self.instances
     proxies.map do |p|
-      # p['interface'] is an Array if the host is a active proxy
-      # p['interface'] is a Hash if the host is a passive proxy
-      new(
-        ensure: :present,
-        name: p['host'],
-        ipaddress: p['interface'].is_a?(Hash) ? p['interface']['ip'] : nil,
-        use_ip: p['interface'].is_a?(Hash) ? p['interface']['use_ip'] : nil,
-        mode: p['status'].to_i - 5,
-        port: p['interface'].is_a?(Hash) ? p['interface']['port'] : nil
-      )
+      new(proxy_properties_hash(p))
     end
   end
 
@@ -33,39 +23,178 @@ def self.prefetch(resources)
     end
   end
 
+  def self.proxies(proxyids = nil)
+    zbx.query(
+      method: 'proxy.get',
+      params: {
+        proxyids: proxyids,
+        output: 'extend',
+        selectInterface: %w[interfaceid type main ip port useip]
+      }.compact
+    )
+  end
+
+  # Convert from proxy hash returned by API into a resource properties hash
+  def self.proxy_properties_hash(p)
+    {
+      ensure: :present,
+      # Proxy object properties
+      proxyid: p['proxyid'].to_i,
+      name:    p['host'],
+      mode:    status_to_mode(p['status']),
+      # Proxy Interface object properties
+      interfaceid: p['interface'].is_a?(Hash) ? p['interface']['interfaceid'] : nil,
+      ipaddress:   p['interface'].is_a?(Hash) ? p['interface']['ip'] : nil,
+      use_ip:      if p['interface'].is_a?(Hash) then p['interface']['useip'] == '1' ? :true : :false end,
+      port:        p['interface'].is_a?(Hash) ? p['interface']['port'].to_i : nil
+    }
+  end
+
   def create
-    # Set some vars
-    host = @resource[:hostname]
-    ipaddress = @resource[:ipaddress]
-
-    # Normally 0 is active and 1 is passive, in the API, its 5 and 6
-    proxy_mode = @resource[:mode] + 5
-
-    use_ip = @resource[:use_ip]
-    port = @resource[:port]
-
-    # Check if we need to connect via ip or fqdn
-    use_ip = use_ip ? 1 : 0
-
-    zbx.proxies.create_or_update(
-      host: host,
-      status: proxy_mode,
-      interfaces: [
-        ip: ipaddress,
-        dns: host,
-        useip: use_ip,
-        port: port
-      ]
+    mode = @resource[:mode] || :active
+
+    if mode == :passive
+      useip = if @resource[:use_ip].nil?
+                1
+              else
+                @resource[:use_ip] == :true ? 1 : 0
+              end
+      interface = {
+        ip: @resource[:ipaddress] || '127.0.0.1',
+        dns: @resource[:hostname],
+        useip: useip,
+        port: @resource[:port] || 10_051
+      }
+    else
+      interface = nil
+    end
+
+    zbx.proxies.create(
+      {
+        host: @resource[:hostname],
+        status: self.class.mode_to_status(mode),
+        interface: interface
+      }.compact
     )
+    @property_flush[:created] = true
   end
 
   def exists?
-    check_proxy(@resource[:hostname])
+    @property_hash[:ensure] == :present
   end
 
   def destroy
     zbx.proxies.delete([zbx.proxies.get_id(host: @resource[:hostname])].flatten)
+    @property_flush[:destroyed] = true
   end
 
-  mk_resource_methods
+  def flush
+    update unless @property_flush[:created] || @property_flush[:destroyed]
+
+    # Update @property_hash so that the output of puppet resource is correct
+    if @property_flush[:destroyed]
+      @property_hash.clear
+      @property_hash[:ensure] = :absent
+    else
+      proxy = self.class.proxies(@property_hash[:proxyid]).find { |p| p['host'] == @resource[:hostname] }
+      @property_hash = self.class.proxy_properties_hash(proxy)
+    end
+  end
+
+  def mode=(value)
+    @property_flush[:mode] = value
+  end
+
+  def change_to_passive_proxy
+    Puppet.debug("We're changing to a passive proxy")
+    # We need to call zbx.proxies.update with an `interface` hash.
+
+    useip = if @resource[:use_ip].nil?
+              1 # Default to true
+            else
+              @resource[:use_ip] == :true ? 1 : 0
+            end
+
+    interface = {
+      ip: @resource[:ipaddress] || @property_hash[:ipaddress] || '127.0.0.1',
+      dns: @resource[:hostname], # This is the namevar and will always exist
+      useip: useip,
+      port: @resource[:port] || @property_hash[:port] || 10_051
+    }
+
+    zbx.proxies.update(
+      {
+        proxyid: @property_hash[:proxyid],
+        status: self.class.mode_to_status(:passive),
+        interface: interface
+      },
+      true
+    )
+  end
+
+  def change_to_active_proxy
+    Puppet.debug("We're changing to an active proxy")
+    zbx.proxies.update(
+      {
+        proxyid: @property_hash[:proxyid],
+        status: self.class.mode_to_status(:active)
+      },
+      true
+    )
+  end
+
+  def update_interface_properties
+    useip = if @resource[:use_ip].nil?
+              nil # Don't provide a default. Keep use_ip unmanaged.
+            else
+              @resource[:use_ip] == :true ? 1 : 0
+            end
+
+    interface = {
+      interfaceid: @property_hash[:interfaceid],
+      ip: @resource[:ipaddress],
+      useip: useip,
+      port: @resource[:port]
+    }.compact
+
+    zbx.proxies.update(
+      {
+        proxyid: @property_hash[:proxyid],
+        interface: interface
+      },
+      true
+    )
+  end
+
+  def update
+    if @property_flush[:mode] == :passive
+      change_to_passive_proxy
+      return
+    end
+
+    if @property_flush[:mode] == :active
+      change_to_active_proxy
+      return
+    end
+
+    # At present, the only properties other than mode that can be updated are the interface properties applicable to passive proxies only.
+    raise Puppet::Error, "Can't update proxy interface properties for an active proxy" unless @property_hash[:mode] == :passive
+    update_interface_properties
+  end
+
+  def self.status_to_mode(status)
+    case status.to_i
+    when 5
+      :active
+    when 6
+      :passive
+    else
+      raise Puppet::Error, 'zabbix API returned invalid value for `status`'
+    end
+  end
+
+  def self.mode_to_status(mode)
+    return 5 if mode == :active
+    6
+  end
 end

lib/puppet/type/zabbix_proxy.rb

@@ -4,24 +4,91 @@
     defaultto :present
   end
 
+  def munge_boolean_to_symbol(value)
+    # insync? doesn't work with actual booleans
+    # see https://tickets.puppetlabs.com/browse/PUP-2368
+    value = value.downcase if value.respond_to? :downcase
+
+    case value
+    when true, :true, 'true', :yes, 'yes'
+      :true
+    when false, :false, 'false', :no, 'no'
+      :false
+    else
+      raise ArgumentError, 'expected a boolean value'
+    end
+  end
+
+  # Standard properties
   newparam(:hostname, namevar: true) do
-    desc 'FQDN of the machine.'
+    desc 'FQDN of the proxy.'
+  end
+
+  newproperty(:mode) do
+    desc 'The kind of mode the proxy running. Active (0) or passive (1).'
+
+    newvalues(:active, :passive, 0, 1, '0', '1')
+
+    munge do |value|
+      case value
+      when 0, '0'
+        :active
+      when 1, '1'
+        :passive
+      else
+        super(value)
+      end
+    end
+  end
+
+  newproperty(:proxyid) do
+    desc '(readonly) ID of the proxy'
+    validate { |_val| raise Puppet::Error, 'proxyid is read-only' }
   end
 
+  # Interface properties (applicable to passive proxies)
   newproperty(:ipaddress) do
     desc 'The IP address of the machine running zabbix proxy.'
+
+    validate do |value|
+      require 'ipaddr'
+
+      begin
+        IPAddr.new(value)
+      rescue => e
+        raise Puppet::Error, e.to_s
+      end
+    end
   end
 
   newproperty(:use_ip) do
     desc 'Using ipadress instead of dns to connect. Is used by the zabbix-api command.'
-  end
 
-  newproperty(:mode) do
-    desc 'The kind of mode the proxy running. Active (0) or passive (1).'
+    munge { |value| @resource.munge_boolean_to_symbol(value) }
   end
 
   newproperty(:port) do
     desc 'The port that the zabbix proxy is listening on.'
+
+    validate do |value|
+      if value.is_a?(String)
+        raise Puppet::Error, 'invalid port' unless value =~ %r{^\d+$}
+        raise Puppet::Error, 'invalid port' unless value.to_i.between?(1, 65_535)
+        return
+      end
+      if value.is_a?(Integer)
+        raise Puppet::Error, 'invalid port' unless value.between?(1, 65_535)
+        return
+      end
+      raise Puppet::Error, 'invalid port'
+    end
+
+    munge(&:to_i)
+  end
+
+  newproperty(:interfaceid) do
+    desc '(readonly) ID of the interface'
+    validate { |_val| raise Puppet::Error, 'interfaceid is read-only' }
   end
 
   autorequire(:file) { '/etc/zabbix/api.conf' }