(puppetlabsGH-1162) Expand configured paths relative to Boltdir

There are a number of config options for Bolt that accept filepaths,
both on the CLI and in the config file. Previously we expanded the path
relative to where Bolt was run in some cases, and relative to the
Boltdir in others. This standardizes all configurable paths to be
expanded relative to the Boltdir if they are not already absolute paths.
This provides consistent and predictable behavior for users. New path
expansions are gated on the `future` option being set to `true`.

Closes puppetlabs#1162

CHANGELOG.md

@@ -14,6 +14,12 @@
 
   The prompt plugin now prints messages to `stderr` instead of `stdout`.
 
+### Bug fixes
+
+* **Standardize configured paths to be relative to Boltdir** ([#1162](https://github.com/puppetlabs/bolt/issues/1162))
+
+  Previously we expanded some configured paths relative to the directory Bolt was run from, and other paths were expanded relative to the Boltdir. This standardizes all configured paths, including the modulepath, to be relative to the Boltdir. This fix is gated on the `future` config option, and will be available by default in Bolt 2.0
+
 ## 1.35.0
 
 ### Deprecation

documentation/bolt_configuration_options.md

@@ -45,7 +45,8 @@ ssh:
 -   `host-key-check`: Whether to perform host key validation when connecting over SSH. Default is `true`.
 -   `password`: Login password.
 -   `port`: Connection port. Default is `22`.
--   `private-key`: The path to the private key file to use for SSH authentication.
+-   `private-key`: Either the path to the private key file to use for SSH authentication, or a hash
+    with key `key-data` and the contents of the private key.
 -   `proxyjump`: A jump host to proxy SSH connections through, and an optional user to connect with, for example: jump.example.com or user1@jump.example.com.
 -   `run-as`: A different user to run commands as after login.
 -   `run-as-command`: The command to elevate permissions. Bolt appends the user and command strings to the configured run as a command before running it on the target. This command must not require an interactive password prompt, and the `sudo-password` option is ignored when `run-as-command` is specified. The run-as command must be specified as an array.
@@ -54,6 +55,22 @@ ssh:
 -   `tty`: Request a pseudo tty for the SSH session. This option is generally only used in conjunction with the `run_as` option when the sudoers policy requires a `tty`. Default is `false`.
 -   `user`: Login user. Default is `root`.
 
+For example:
+
+```yaml
+targets:
+  - name: host1.example.net
+    config:
+      transport: ssh
+      ssh:
+        host-key-check: true
+        port: 22
+        run-as-command: ['sudo', '-k', '-n']
+        private-key:
+          key-data: |
+            MY PRIVATE KEY CONTENT
+```
+
 
 ## OpenSSH configuration options
 
@@ -83,14 +100,14 @@ To illustrate, consider this example:
 `inventory.yaml`
 
 ```yaml
-nodes:
+targets:
   - name: host1.example.net
     config:
       transport: ssh
       ssh:
         host-key-check: true
         port: 22
-        private-key: /.ssh/id_rsa-example
+        private-key: ~/.ssh/id_rsa-example
 ```
 
 `ssh.config`

documentation/using_plugins.md

@@ -271,8 +271,8 @@ plugins:
 ```
 
 -   `keysize`: They size of the key to generate with `bolt secret createkeys`: default: `2048`
--   `private-key`: The path to the private key file. default: `keys/private_key.pkcs7.pem`
--   `public-key`: The path to the public key file. default: `keys/public_key.pkcs7.pem`
+-   `private-key`: The path to the private key file. default: `<boltdir>/keys/private_key.pkcs7.pem`
+-   `public-key`: The path to the public key file. default: `<boltdir>/keys/public_key.pkcs7.pem`
 
 ## Vault plugin
 

lib/bolt/bolt_option_parser.rb

@@ -372,7 +372,7 @@ def initialize(options)
         end
         @options[:password] = password
       end
-      define('--private-key KEY', 'Private ssh key to authenticate with') do |key|
+      define('--private-key KEY', 'Path to the private SSH key to authenticate with') do |key|
         @options[:'private-key'] = key
       end
       define('--[no-]host-key-check', 'Check host keys with SSH') do |host_key_check|
@@ -412,10 +412,11 @@ def initialize(options)
       define('-m', '--modulepath MODULES',
              "List of directories containing modules, separated by '#{File::PATH_SEPARATOR}'",
              'Directories are case-sensitive') do |modulepath|
-        # When specified from the CLI, modulepath entries are relative to pwd
-        @options[:modulepath] = modulepath.split(File::PATH_SEPARATOR).map do |moduledir|
+        # TODO: Remove this logic in 2.0 and just return modulepath
+        relative = modulepath.split(File::PATH_SEPARATOR).map do |moduledir|
           File.expand_path(moduledir)
         end
+        @options[:modulepath] = ->(x) { x ? modulepath : relative }
       end
       define('--boltdir FILEPATH',
              'Specify what Boltdir to load config from (default: autodiscovered from current working dir)') do |path|
@@ -430,7 +431,9 @@ def initialize(options)
         if ENV.include?(Bolt::Inventory::ENVIRONMENT_VAR)
           raise Bolt::CLIError, "Cannot pass inventory file when #{Bolt::Inventory::ENVIRONMENT_VAR} is set"
         end
-        @options[:inventoryfile] = File.expand_path(path)
+
+        # TODO: Remove this logic in 2.0 and just return path
+        @options[:inventoryfile] = ->(x) { x ? path : File.expand_path(path) }
       end
       define('--[no-]save-rerun', 'Whether to update the rerun file after this command.') do |save|
         @options[:'save-rerun'] = save

lib/bolt/cli.rb

@@ -244,7 +244,7 @@ def handle_parser_errors
 
     def puppetdb_client
       return @puppetdb_client if @puppetdb_client
-      puppetdb_config = Bolt::PuppetDB::Config.load_config(nil, config.puppetdb)
+      puppetdb_config = Bolt::PuppetDB::Config.load_config(nil, config.puppetdb, config.boltdir.path)
       @puppetdb_client = Bolt::PuppetDB::Client.new(puppetdb_config)
     end
 

lib/bolt/config.rb

@@ -173,6 +173,13 @@ def update_from_file(data)
       TRANSPORTS.each do |key, impl|
         if data[key.to_s]
           selected = impl.filter_options(data[key.to_s])
+          if @future
+            to_expand = %w[tmpdir private-key cacert token-file] & selected.keys
+            to_expand.each do |opt|
+              selected[opt] = File.expand_path(selected[opt], @boltdir.path) if opt.is_a?(String)
+            end
+          end
+
           @transports[key] = Bolt::Util.deep_merge(@transports[key], selected)
         end
         if @transports[key]['interpreters']
@@ -183,7 +190,26 @@ def update_from_file(data)
     private :update_from_file
 
     def apply_overrides(options)
-      %i[concurrency transport format trace modulepath inventoryfile color].each do |key|
+      # Can we use this future instead of $future?
+      if @future
+        # Expand modulepaths and inventoryfile path relative to Boltdir
+        if options[:modulepath]
+          path = options[:modulepath].call(@future)
+          @modulepath = path.split(File::PATH_SEPARATOR).map do |moduledir|
+            File.expand_path(moduledir, @boltdir.path)
+          end
+        end
+
+        if options[:inventoryfile]
+          @inventoryfile = File.expand_path(options[:inventoryfile].call(@future), @boltdir.path)
+        end
+      else
+        %i[modulepath inventoryfile].each do |key|
+          send("#{key}=", options[key].call(@future)) if options.key?(key)
+        end
+      end
+
+      %i[concurrency transport format trace color].each do |key|
         send("#{key}=", options[key]) if options.key?(key)
       end
 
@@ -198,9 +224,12 @@ def apply_overrides(options)
       TRANSPORTS.each_key do |transport|
         transport = @transports[transport]
         TRANSPORT_OPTIONS.each do |key|
-          if options[key]
-            transport[key.to_s] = Bolt::Util.walk_keys(options[key], &:to_s)
-          end
+          next unless options[key]
+          transport[key.to_s] = if %i[tmpdir private-key cacert token-file].include?(key) && @future
+                                  File.expand_path(options[key], @boltdir.path)
+                                else
+                                  Bolt::Util.walk_keys(options[key], &:to_s)
+                                end
         end
       end
 

lib/bolt/puppetdb/config.rb

@@ -19,7 +19,7 @@ class Config
 
       end
 
-      def self.load_config(filename, options)
+      def self.load_config(filename, options, boltdir_path = nil)
         config = {}
         global_path = Bolt::Util.windows? ? DEFAULT_CONFIG[:win_global] : DEFAULT_CONFIG[:global]
         if filename
@@ -43,11 +43,12 @@ def self.load_config(filename, options)
         end
 
         config = config.fetch('puppetdb', {})
-        new(config.merge(options))
+        new(config.merge(options), boltdir_path)
       end
 
-      def initialize(settings)
+      def initialize(settings, boltdir_path = nil)
         @settings = settings
+        @boltdir_path = boltdir_path
         expand_paths
       end
 
@@ -66,7 +67,14 @@ def token
 
       def expand_paths
         %w[cacert cert key token].each do |file|
-          @settings[file] = File.expand_path(@settings[file]) if @settings[file]
+          next unless @settings[file]
+          # rubocop:disable Style/GlobalVars
+          @settings[file] = if $future && @boltdir_path
+                              File.expand_path(@settings[file], @boltdir_path)
+                            else
+                              File.expand_path(@settings[file])
+                            end
+          # rubocop:enable Style/GlobalVars
         end
       end
 

spec/bolt/cli_spec.rb

@@ -488,18 +488,18 @@ def stub_config(file_content = {})
     end
 
     describe "modulepath" do
-      it "treats relative modulepath as relative to pwd" do
+      it "returns modulepath without expanding" do
         site = File.expand_path('site')
         modulepath = [site, 'modules'].join(File::PATH_SEPARATOR)
         cli = Bolt::CLI.new(%W[command run uptime --modulepath #{modulepath} --nodes foo])
-        expect(cli.parse).to include(modulepath: [site, File.expand_path('modules')])
+        expect(cli.parse[:modulepath].call(true)).to eq(modulepath)
       end
 
       it "accepts shorthand -m" do
         site = File.expand_path('site')
         modulepath = [site, 'modules'].join(File::PATH_SEPARATOR)
         cli = Bolt::CLI.new(%W[command run uptime -m #{modulepath} --nodes foo])
-        expect(cli.parse).to include(modulepath: [site, File.expand_path('modules')])
+        expect(cli.parse[:modulepath].call(true)).to eq(modulepath)
       end
 
       it "generates an error message if no value is given" do
@@ -2101,4 +2101,34 @@ def stub_config(file_content = {})
       end
     end
   end
+
+  describe 'with future set' do
+    let(:configdir) { File.join(__dir__, '..', 'fixtures', 'configs') }
+    after(:each) do
+      # rubocop:disable Style/GlobalVars
+      $future = nil
+      # rubocop:enable Style/GlobalVars
+    end
+
+    it 'expands CLI flag paths relative to the Boltdir' do
+      cli = Bolt::CLI.new(%W[command run uptime
+                             --configfile #{File.join(configdir, 'future.yml')}
+                             --nodes foo
+                             --private-key .ssh/private
+                             --inventoryfile ../inventory/empty.yml
+                             --tmpdir tmp/erature
+                             --modulepath mod#{File::PATH_SEPARATOR}ule/pat/h])
+      cli.parse
+      expect(cli.config.transports[:ssh]['private-key'])
+        .to eq(File.expand_path('.ssh/private', cli.config.boltdir.path))
+      expect(cli.config.transports[:ssh]['tmpdir'])
+        .to eq(File.expand_path('tmp/erature', cli.config.boltdir.path))
+      expect(cli.config.inventoryfile)
+        .to eq(File.expand_path('../inventory/empty.yml', cli.config.boltdir.path))
+      mod_path = File.expand_path('mod', cli.config.boltdir.path)
+      ule_path = File.expand_path('ule/pat/h', cli.config.boltdir.path)
+      expect(cli.config.modulepath)
+        .to eq([mod_path, ule_path])
+    end
+  end
 end

spec/bolt/config_spec.rb

@@ -169,6 +169,15 @@
       expect { Bolt::Config.new(boltdir, config) }.not_to raise_error
     end
 
+    it "expands the private-key hash with 'future' set" do
+      data = {
+        'ssh' => { 'private-key' => 'my-private-key' },
+        'future' => true
+      }
+      config = Bolt::Config.new(boltdir, data)
+      expect(config.transports[:ssh]['private-key']).to eq(File.expand_path('my-private-key', boltdir.path))
+    end
+
     it "does not accept a private-key hash without data" do
       config = {
         'ssh' => { 'private-key' => { 'not-data' => "foo" } }
@@ -232,4 +241,55 @@
       expect { Bolt::Config.new(boltdir, config) }.not_to raise_error
     end
   end
+
+  describe 'expanding paths' do
+    it "expands cacert relative to boltdir" do
+      expect(Bolt::Util)
+        .to receive(:validate_file)
+        .with('cacert', File.expand_path('ssl/ca.pem', boltdir.path))
+        .and_return(true)
+
+      data = {
+        'winrm' => { 'ssl' => true, 'cacert' => 'ssl/ca.pem' },
+        'future' => true
+      }
+
+      config = Bolt::Config.new(boltdir, data)
+      expect(config.transports[:winrm]['cacert'])
+        .to eq(File.expand_path('ssl/ca.pem', boltdir.path))
+    end
+
+    it "expands token-file relative to boltdir" do
+      data = {
+        'pcp' => { 'token-file' => 'token' },
+        'future' => true
+      }
+
+      config = Bolt::Config.new(boltdir, data)
+      expect(config.transports[:pcp]['token-file'])
+        .to eq(File.expand_path('token', boltdir.path))
+    end
+
+    it "expands local tmpdir relative to boltdir" do
+      data = {
+        'local' => { 'tmpdir' => 'tmp/orary' },
+        'future' => true
+      }
+
+      config = Bolt::Config.new(boltdir, data)
+      expect(config.transports[:local]['tmpdir'])
+        .to eq(File.expand_path('tmp/orary', boltdir.path))
+    end
+
+    it "expands inventoryfile relative to boltdir" do
+      data = {
+        'inventoryfile' => 'targets.yml',
+        'future' => true
+      }
+
+      config = Bolt::Config.new(boltdir, data)
+      expect(config.inventoryfile)
+        .to eq(File.expand_path('targets.yml', boltdir.path))
+    end
+  end
 end

spec/bolt/inventory_spec.rb

@@ -687,7 +687,7 @@ def common_data(transport)
   end
 
   describe 'add_facts' do
-    context 'whith and without $future flag' do
+    context 'with and without $future flag' do
       let(:inventory) { Bolt::Inventory.new({}) }
       let(:target) { get_target(inventory, 'foo') }
       let(:facts) { { 'foo' => 'bar' } }

spec/bolt/puppetdb/config_spec.rb

@@ -5,6 +5,27 @@
 require 'bolt/util'
 
 describe Bolt::PuppetDB::Config do
+  context "with boltdir available" do
+    let(:cacert) { File.expand_path('relative/to/cacert') }
+    let(:token) { File.expand_path('relative/to/token') }
+    let(:boltdir) { '~/dirbolt' }
+    let(:options) do
+      {
+        'server_urls' => ['https://puppetdb:8081'],
+        'cacert' => cacert,
+        'token' => token
+      }
+    end
+
+    let(:config) { Bolt::PuppetDB::Config.new(options, boltdir) }
+
+    it 'expands the cacert relative to the boltdir if boltdir is available' do
+      allow(config).to receive(:validate_file_exists).with('cacert').and_return true
+
+      expect(config.cacert).to eq(File.expand_path(cacert, boltdir))
+    end
+  end
+
   context "when validating that options" do
     let(:cacert) { File.expand_path('/path/to/cacert') }
     let(:token) { File.expand_path('/path/to/token') }

spec/fixtures/configs/future.yml

@@ -0,0 +1 @@
+future: true