--ssh-options argument doesn't override ssh.options when using site:ssh

[minorOffense]

Describe the bug
When using a site alias file which has an entry for ssh options using the site:ssh command the alias file options aren't overridden by the command line argument value. But every other drush command favours the CLI argument over the alias file settings.

To Reproduce
Create a site alias file with some ssh options defined.

E.g.

ssh:
  options: -l username

Then call a drush command that uses ssh

e.g.

drush --ssh-options="-A" rsync @myalias @self

The alias ssh settings are overridden when rsync opens the SSH connection (which is good!)

However if you call site:ssh as the command

drush --ssh-options="-A" @myalias ssh echo "Hello"

Drush uses the settings from the alias file and ignores the --ssh-options parameter.

Expected behavior
Drush should always prefer the CLI arguments over the alias file defaults.

Actual behavior
It uses the ssh options from the alias file.

Workaround
Not really...

System Configuration

Q	A
Drush version?	9.5
Drupal version?	8.x
PHP version	7.1
OS?	Only tested on Linux but presume it would be for all.

[greg-1-anderson]

Aside: this is related to the current bug in 9.6.0-rc1 where --ssh-options are ignored. When we fix that, we should take care of this use-case as well.

[greg-1-anderson]

In progress at #3885

[weitzman]

lets talk there.

[greg-1-anderson]

#3885 did not fix this issue. Need a follow-up PR to continue this.

[greg-1-anderson]

Another interesting thing about this is that in Drush 9, --ssh-options are remapped to '-Dssh.options'. The definition of -D is that it provides default configuration values, e.g. that is overridden by configuration values that may appear in a drush.yml file or an alias.

There are definitely use-cases for providing default config values, though (e.g. bash aliases), so the definition of -D should not be changed. Perhaps there needs to be a different but similar construct to -D for config overrides.

[greg-1-anderson]

I'm starting to question whether we should fix this, or at least whether we should postpone the decision until after the Drush 9.6.0 release.

In Drush 9, there are three objects that need to interact in order for this to work as desired:

• InputInterface $input: The commandline arguments
• SiteAlias $site: The site alias for the selected site (or @self). This isa ConfigInterface.
• ConfigInterface $config: The combined configuration values from any drush.yml that was loaded.

As mentioned above, in Drush 9, --ssh-option is converted into a configuration value and written into the Drush config object under the key ssh.options. The site alias object contains all of the key:value pairs from its alias definition, but beyond that, it knows nothing about the configuration object.

When Drush 9 runs a remote command, e.g. site:ssh, as shown above, it now (as of Drush 9.6.x, anyway) uses the ProcessManager to handle the proc_open call and associated logic. The command gets a copy of the site alias from the SiteAliasManager and passes it to the process manager. The process manager has a reference to the Drush combined configuration that it got from the DI container during dependency injection.

The process manager looks up the settings it needs by asking the site alias to fetch it. The site alias has a method called getConfig() that takes a ConfigInterface. The process manager provides the Drush configuration to this call. The site alias satisfies the request if it can; if it does not have an entry for the requested key, it passes the call along to the provided configuration object.

The ask in this issue is that CLI options should take precedence over configuration values. However, the process manager does not have a reference to the CLI options. It would not be desirable to give the process manager a reference to the CLI options. The input options are considered to be dynamic. They are not available at dependency injection time, and not every client that might want to use the process manager will have an InputInterface available.

Currently, Drush resolves this conundrum by remapping options-that-are-configuration to an appropriate -D option during preflight; for example, --ssh-options="-A" is remapped to -Dssh.options="-A". However, there is a shortcoming to this method. -D was designed to provide a way to provide configuration values on the command line, e.g. perhaps you might have a Bash alias that calls a certain executable and includes some configuration via -D options. If the user of this alias provided equivalent long-form options on the command line, these overwrite the -D options.

The building blocks we need to fix this, then are:

• Something like -D, but for providing high-priority settings rather than low-priority settings.
• A way for SiteAlias::getConfig() to access those settings.

The logic in getConfig() would be something like this:

• If the requested option is available as high-priority config, return it.
• If the requested option exists in the site alias itself, return that
• Otherwise, call the provided config object (as happens today) to fetch default values

This is not impossible, but it gives me pause. The existing logic described above is complicated in the aggregate, but each individual component has a well-defined responsibility, and a simple implementation. Implementing a priority scheme like the above would not be simple.

There is an easier solution here; the process manager could specifically ask for ssh.options from the config object. I'm not sure this is right either, though, as it leads to more if statements / special checking everywhere a value like this is needed.

I've been thinking this over for a while, but need to think about it a little more. Input welcome.

[greg-1-anderson]

Actually, the "easier" solution is a non-starter, because it does not work. If the process manager asks the configuration object for ssh.options, it cannot tell whether that value was set via --ssh-options on the commandline, or whether it was set via an ssh.options setting in a drush.yml file. The former should override any ssh options in a site alias, whereas the later does not.

I think we need a third solution here.

[greg-1-anderson]

Here is the specific line where the ssh transport fetches ssh.options:

https://github.com/consolidation/site-process/blob/2.0.0-beta3/src/Transport/SshTransport.php#L73

The config object is injected into the transport by the process manager.

[weitzman]

I dont have much to add here. I'm OK with deferring this until after 9.6.0

[greg-1-anderson]

OK, hold my beer, I got this.

I'm going to proceed per option 1, with the following clarifications.

• Drush already maintains a config object with multiple configurations overlayed together. The one that receives the -D options is named the 'process' configuration.
• At Dependency injection time, the process configuration object is retrieved and injected into the process manager. The process manager still receives a reference to the regular Drush config object as well.
• At the time the process configuration is injected into the process manager, it is empty. However, it is filled with the applicable -D options et. al. before the process manager is used to launch a process.
• The existing process manager API is given a site alias to use when launching a process. The process manager combines the 'process' configuration with the site alias and the standard Drush configuration into an object designed expressly for this purpose.
• The combined config object isa site alias, and has the usual site alias convenience methods. The 'get' method first references the 'process' config, and falls back to the site alias followed by the standard configuration as needed to retrieve the requested value.
• The end result is that SiteAlias::getConfig() becomes deprecated, and the individual transport implementations become simpler, as they are now written as if they are using only a single site alias object.

I've started a PR, but it's not quite finished yet. I'll push it up when it's ready.