(SIMP-10376) Fix simpkv plugin API issues (#62)

Fix two simpkv plugin API issues found when developing the LDAP plugin

* Fixed the mechanism a plugin uses to advertise its type.
  - Plugin type is now determined from its filename.
  - Previous mechanism did not work when when multiple plugins were used.
    It erroneously used a class method `type`, which for each anonymous
    plugin class, polluted the Class object namespace, instead of being
    associated with its anonymous class.
* Changed the plugin configuration API
  - Configuration has been split out into its own method, instead of being
    done in the plugin constructor.
  - This minimal change simplifies unit testing of configuration of complex
    plugins.

SIMP-10376 #close

.fixtures.yml

@@ -9,5 +9,6 @@ fixtures:
     compliance_markup: https://github.com/simp/pupmod-simp-compliance_markup.git
   symlinks:
     simpkv: "#{source_dir}"
-    test_plugins: "#{File.join(source_dir, 'spec', 'support', 'modules', 'test_plugins')}"
+    test_plugins1: "#{File.join(source_dir, 'spec', 'support', 'modules', 'test_plugins1')}"
+    test_plugins2: "#{File.join(source_dir, 'spec', 'support', 'modules', 'test_plugins2')}"
     simpkv_test: "#{File.join(source_dir, 'spec', 'support', 'modules', 'simpkv_test')}"

CHANGELOG

@@ -15,9 +15,17 @@
     function with a 'global' Boolean option.
     - Global keys are now specified by setting 'global' to true in lieu of
       setting 'environment' to ''.
-  - Restrict letter characters used in key and folder names to be
-    lowercase.
+  - Changed the key and folder name specification to restrict letter
+    characters to lowercase.
     - Change required for the LDAP plugin.
+  - Changed the plugin configuration API
+    - Configuration has been split out into its own method, instead of being
+      done in the plugin constructor.
+    - This minimal change simplifies unit testing of configuration of complex
+      plugins.
+  - Fixed the mechanism a plugin uses to advertise its type.
+    - Plugin type is now determined from its filename.
+    - Previous mechanism did not work when when multiple plugins were used.
 - Added
   - More detailed plugin exception reporting in order to pinpoint plugin
     logic problems.

docs/Design_Prototype2.md

@@ -168,25 +168,27 @@ simpkv must provide a backend plugin adapter that
 
 simpkv must supply a backend plugin API that provides
 
-  * Public API method signatures, including the constructor and a
-    method that reports the plugin type (typically backend it supports)
-  * Description of any universal plugin options that must be supported
-  * Ability to specify plugin-specific options
+  * Public API method signatures, including the constructor and `configure()`
+    method
+  * Description of any universal plugin configuration options that must be
+    supported
+  * Ability to specify plugin-specific configuration options
   * Explicit policy on error handling (how to report errors, what information
     the messages should contain for plugin proper identification, whether
     exceptions are allowed)
-  * Details on the code structure required for prevention of cross-environment
-    contamination
+  * Details on the code structure required for prevention of cross-Puppet-
+    environment contamination
   * Documentation requirements
   * Testing requirements
 
 Each plugin must conform to the plugin API and satisfy the following
 general requirements:
 
-* All plugins must be unique.
+* All plugin files, potentially from multiple modules, must be uniquely named.
 
-  * Plugin Ruby files can be named the same in different modules, but
-    their reported plugin types must be unique.
+  * The plugin type is derived from its filename: <plugin name>\_plugin.rb.
+  * Only one plugin of the same name will be loaded and a warning will be
+    emitted for all other conflicting plugin files.
 
 * All plugins must allow multiple instances of the plugin to be instantiated
   and used in a single catalog compile.
@@ -298,7 +300,7 @@ general requirements:
 This is a placeholder for miscellaneous, additional simpkv requirements
 to be addressed, once it moves beyond the prototype stage.
 
-* simpkv must provide a plugin for a remote key/value store, such as Consul
+* simpkv must provide a plugin for a remote key/value store, such as LDAP
 * simpkv must support audit operations on a key/value store
 
   * Auditing information to be provided must include:
@@ -318,7 +320,7 @@ to be addressed, once it moves beyond the prototype stage.
 * simpkv should provide a script to import existing
   `simplib::passgen()` passwords stored in the puppetserver cache
   directory, PKI secrets stored in `/var/simp/environments`, and Kerberos secrets
-  stored in `/var/simp/environments` to a backend.
+  stored in `/var/simp/environments` into a backend.
 * simpkv local file backend must encrypt each file it maintains.
 * simpkv local file backend must ensure multi-process-safe `put`,
   `delete`, and `deletetree` operations on a <insert shared file system
@@ -488,6 +490,9 @@ configuration.
 
 * `type` must be unique across all backend plugins, including those
   provided by other modules.
+
+  * The `type` of each is derived from the filename of its plugin software.
+
 * `id` must be unique for a each distinct configuration for a `type`.
 * Other keys for configuration specific to the backend may also be present.
 

lib/puppet_x/simpkv/file_plugin.rb

@@ -3,24 +3,35 @@
 #
 # Each plugin **MUST** be an anonymous class accessible only through
 # a `plugin_class` local variable.
+# DO NOT CHANGE THE LINE BELOW!!!!
 plugin_class = Class.new do
 
   require 'etc'
   require 'fileutils'
   require 'timeout'
 
-  # Reminder:  Do **NOT** try to set constants in this Class.new block.
-  #            They don't do what you expect (are not accessible within
-  #            any class methods) and pollute the Object namespace.
+  # NOTES FOR MAINTAINERS:
+  # - See simpkv/lib/puppet_x/simpkv/plugin_template.rb for important
+  #   information about plugin responsibilities and restrictions.
+  # - One OBTW that will drive you crazy are limitations on anonymous classes.
+  #   In typical Ruby code, using constants and class methods is quite normal.
+  #   Unfortunately, you cannot use constants or class methods in an anonymous
+  #   class, as they will be added to the Class Object, itself, and will not be
+  #   available to the anonymous class. In other words, you will be tearing your
+  #   hair out trying to figure out why normal Ruby code does not work here!
 
   ###### Public Plugin API ######
 
-  # @return String. backend type
-  def self.type
-    'file'
+  # Construct an instance of this plugin setting its instance name
+  #
+  # @param name Name to ascribe to this plugin instance
+  #
+  def initialize(name)
+    @name = name
+    Puppet.debug("#{@name} simpkv plugin constructed")
   end
 
-  # Constructs an instance of this plugin using global and plugin-specific
+  # Configure this plugin instance using global and plugin-specific
   # configuration found in options
   #
   # The plugin-specific configuration will be found in
@@ -33,12 +44,11 @@ def self.type
   #   on a file modifying operation before failing the operation; defaults
   #   to 5 seconds
   #
-  # @param name Name to ascribe to this plugin instance
   # @param options Hash of global simpkv and backend-specific options
   # @raise RuntimeError if any required configuration is missing from options,
   #   the root directory can be created when missing, or the root directory
   #   exists but cannnot be read/modified by this process
-  def initialize(name, options)
+  def configure(options)
     # backend config should already have been verified by simpkv adapter, but
     # just in case...
     unless (
@@ -56,7 +66,6 @@ def initialize(name, options)
       raise("Plugin misconfigured: #{options}")
     end
 
-    @name = name
 
     # set optional configuration
     backend = options['backend']
@@ -72,10 +81,11 @@ def initialize(name, options)
     ensure_folder_path('globals')
     ensure_folder_path('environments')
 
-    Puppet.debug("#{@name} simpkv plugin for #{@root_path} constructed")
+    Puppet.debug("#{@name} simpkv plugin for #{@root_path} configured")
   end
 
 
+
   # Deletes a `key` from the configured backend.
   #
   # @param key String key

lib/puppet_x/simpkv/plugin_template.rb

@@ -1,29 +1,69 @@
-# Copy this file to <plugin name>_plugin.rb and address the FIXMEs
+# Copy this file to <your module>/lib/puppet_x/simpkv/<plugin type>_plugin.rb,
+# read all the documentation in this file and address the FIXMEs.
 #
+###############################################################################
+# SIMPKV PLUGIN REQUIREMENTS
+# - The plugin type derived from the plugin's base filename must be unique
+#   over **all** plugins loaded.
+#   - The simpkv adapter will only load the first plugin it finds of any given
+#     type.
+#   - The simpkv adapter will emit a warning when multiple plugin files for the
+#     same type are detected.
+#
+# - The plugin code must implement the API in this template.
+#
+# - The plugin code must protect from cross-puppet-environment contamination.
+#   Different versions of the module containing this plugin may be loaded
+#   into the puppetserver at the same time. So, unlike normal Ruby library
+#   code for which only one version will be loaded at a time (e.g., gems
+#   installed in the puppetserver), you have to explicitly design this plugin
+#   code to prevent cross-environment-contamination.  This is why the plugin
+#   architecture requires this class to be anonymous and loads it appropriately.
+#   You must provide similar protections for any supporting Ruby code that you
+#   package in the module (e.g., a separate connector class).
+#
+# - The plugin code must allow multiple instances to be instantiated and run
+#   concurrently.
+#
+# - The plugin code is responsible for executing any appropriate retry logic
+#   on failed backend operations.
+#
+# - The plugin code must protect itself from hung operations.
+#
+# - When accessing the backend in the put(), get(), ... methods, the plugin code
+#   should catch exceptions, convert them to meaningful error messages and then
+#   return the failed status in its public API.
+###############################################################################
+
 
-# DO NOT CHANGE THIS LINE!!!!
 # Each plugin **MUST** be an anonymous class accessible only through
 # a `plugin_class` local variable.
+# DO NOT CHANGE THE LINE BELOW!!!!
 plugin_class = Class.new do
 
-  # Reminder:  Do **NOT** try to set constants in this Class.new block.
-  #            They don't do what you expect (are not accessible within
-  #            any class methods) and pollute the Object namespace.
+  # WARNING:
+  # In typical Ruby code, using constants and class methods is quite normal.
+  # Unfortunately, you cannot use constants or class methods in an anonymous
+  # class, as they will be added to the Class Object, itself, and will not be
+  # available to the anonymous class. In other words, you will be tearing your
+  # hair out trying to figure out why normal Ruby code does not work here!
 
   ###### Public Plugin API ######
 
-  # @return String. backend type
-  def self.type
-    # This is the value that will be in the 'type' attribute of a configuration
-    # block for this plugin.  The simpkv adapter uses to select the plugin class to
-    # use in order to create a plugin instance.
-    # This **MUST** be unique across all loaded plugins.  Only the first
-    # plugin of a particular type will be loaded!
-    'FIXME'
-  end
+  # Construct an instance of this plugin setting its instance name
+  #
+  # @param name Name to ascribe to this plugin instance
+  #
+  def initialize(name)
+    # save this off, because the simpkv adapter will access it through a getter
+    # (defined below), when constructing log messages
+    @name = name
 
+    # You can use the Puppet object for logging
+    Puppet.debug("#{@name} simpkv plugin constructed")
+  end
 
-  # Construct an instance of this plugin using global and plugin-specific
+  # Configure this plugin instance using global and plugin-specific
   # configuration found in options
   #
   # FIXME:  The description below is informational for you as a developer.
@@ -65,43 +105,39 @@ def self.type
   #     }
   #   }
   #
-  # @param name Name to ascribe to this plugin instance
   # @param options Hash of global simpkv and backend-specific options
   # @raise RuntimeError if any required configuration is missing from options
   #   or this object can't set up any stateful objects it needs to do its work
   #   (e.g., file directory, connection to a backend)
-  def initialize(name, options)
-    # save this off, because the simpkv adapter will access it through a getter
-    # (defined below) when constructing log messages
-    @name = name
+  def configure(options)
 
     # FIXME: insert validation and set up code here
     # Be sure to create 'globals' and 'environments' sub-folders off of the
     # root directory.
 
-    Puppet.debug("#{@name} simpkv plugin constructed")
+    Puppet.debug("#{@name} simpkv plugin configured")
   end
 
   # @return unique identifier assigned to this plugin instance
   def name
     @name
   end
 
-
   # The remaining methods in this API map one-for-one to those in
   # simpkv's Puppet function API.
   #
-  # IMPORTANT NOTES:
+  # IMPORTANT API NOTES:
   #
-  # - An instance of this plugin class will persist through a single catalog run.
+  # - An instance of this plugin class will persist through a single catalog
+  #   run.
   # - Other instances of this plugin class may be running concurrently in
   #   the same process.
   #
   #   * Make sure your code is multi-thread safe if you are using any
   #     mechanisms that would cause concurrency problems!
   #
-  # - All values persisted and returned are Strings.  Other software in the
-  #   simpkv function chain is responsible for serializing non-String
+  # - All key values persisted and returned are Strings.  Other software in
+  #   the simpkv function chain is responsible for serializing non-String
   #   values into Strings for plugins to persist and then deserializing
   #   Strings retrieved by plugins back into objects.
   #
@@ -121,8 +157,12 @@ def name
   #         >> to create useful error messages!   <<
   #
   # - If your plugin connects to an external service, you are strongly
-  #   encouraged to build retry logic and timeouts into your backend
+  #   encouraged to build timeouts and retry logic into your backend
   #   operations.
+  #
+  #   * The simpkv adapter does not currently protect against hung operations.
+  #   * Only you have domain knowledge to know when a connection is hung
+  #     and when a retry of a failed operaton is appropriate.
 
   # Deletes a `key` from the configured backend.
   #
@@ -134,7 +174,7 @@ def name
   #
   def delete(key)
 
-    # FIXME: insert code that connects to the backend an affects the delete
+    # FIXME: insert code that connects to the backend and affects the delete
     # operation
     #
     # - This delete should be done atomically
@@ -155,7 +195,7 @@ def delete(key)
   #
   def deletetree(keydir)
 
-    # FIXME: insert code that connects to the backend an affects the deletetree
+    # FIXME: insert code that connects to the backend and affects the deletetree
     # operation
     #
     # - If supported, this deletetree should be done atomically.  If not,
@@ -179,7 +219,7 @@ def deletetree(keydir)
   #
   def exists(key)
 
-    # FIXME: insert code that connects to the backend an affects the exists
+    # FIXME: insert code that connects to the backend and affects the exists
     # operation
     #
     # - Convert any exceptions into a failed status result with a meaningful
@@ -200,7 +240,7 @@ def exists(key)
   #
   def get(key)
 
-    # FIXME: insert code that connects to the backend an affects the get
+    # FIXME: insert code that connects to the backend and affects the get
     # operation
     #
     # - If possible, this get should be done atomically
@@ -233,7 +273,7 @@ def get(key)
   #
   def list(keydir)
 
-    # FIXME: insert code that connects to the backend an affects the list
+    # FIXME: insert code that connects to the backend and affects the list
     # operation
     #
     # - Convert any exceptions into a failed status result with a meaningful
@@ -254,7 +294,7 @@ def list(keydir)
   #
   def put(key, value)
 
-    # FIXME: insert code that connects to the backend an affects the put
+    # FIXME: insert code that connects to the backend and affects the put
     # operation
     #
     # - This delete should be done atomically