SSH remoting cmdlet parameter set changes based on RFC 0010 by PaulHigin · Pull Request #2710 · PowerShell/PowerShell

PaulHigin · Nov 17, 2016

These changes implement the SSH remoting cmdlet parameter set changes as discussed in RFC 0010 and comments (https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0010-SSH-Remoting-Cmdlets.md).

These changes also include a fix that prevented built-in Windows PowerShell (FullCLR) from hosting a SSH remoting endpoint. The issue was with the managed code entry point attempting to update the console font before knowing if PowerShell is being run in console or server mode. SSH hosts PowerShell in server mode without a console handle and this was preventing PowerShell from starting as an endpoint. Fix is to move the font update code to console start where it knows what mode is running.

… RFCRemoteCmdlets

… for CoreCLR entry point when hosted in SSH as a subsystem.

iSazonov · Nov 18, 2016

+
+    It "New-PSSession SSHTransport parameter cannot have false value" {
+
+        [System.Management.Automation.ErrorRecord] $paramError = $null


It is a good candidate for BeforeEach.

Thanks, will change.

iSazonov · Nov 18, 2016

+
+Describe "SSHConnection parameter hashtable error conditions" -Tags 'CI' {
+
+    It "SSHConnection parameter hashtable cannot contain empty parameter names" {


Perhaps it would be better to use -Testcase in the Describe block.

Thanks, will change.

mirichmo · Nov 21, 2016

+                    // For NanoServer:
+                    // 1. There is no GetCurrentConsoleFontEx / SetCurrentConsoleFontEx on NanoServer;
+                    // 2. We don't handle CJK locales on NanoServer due to lack of win32 API supports on NanoServer.
+#if !CORECLR


The comment doesn't match the specified environment. Nano Server (and IoT) are (Platform.IsInbox && Platform.IsCoreCLR) aka Platform.IsNanoServer() and Platform.IsIoT(). However, line 285 indicates that the code is for Windows PowerShell on full SKUs

I don't understand your comment. This code was simply moved from one place to another. Please feel free to update the comment as needed.

My request is for you to improve the existing comment since you are touching that bit of code

mirichmo · Nov 21, 2016

        #endregion
    }

+    internal struct SSHConnection


Please comment this struct

mirichmo · Nov 21, 2016

+        /// Parse the Connection parameter HashTable array.
+        /// </summary>
+        /// <returns>Array of SSHConnection objects</returns>
+        private const string ComputerNameParameter = "ComputerName";


These should have their own region

mirichmo · Nov 21, 2016

+                    {
+                        connectionInfo.UserName = paramValue;
+                    }
+                    else if (paramName.Equals(KeyFilePathParameter, StringComparison.OrdinalIgnoreCase) || paramName.Equals(IdentityFilePathAlias, StringComparison.OrdinalIgnoreCase))


There are a lot of string comparisons here. I'm wondering about techniques to optimize the matching and if they are worth the perf gain.

What do you have in mind? Computer names, User names, and file paths cannot be made into enums since they are not known before hand. I am open to any suggestions.

After talking offline I think this is Ok as is. This parameter set is intended to support fan-out remoting where it is possible that 100s of target machines are specified in the hashtable. But I feel the parsing of these connection info objects is insignificant compared to setting up the remote runspaces needed for each connection. But this is something we can revisit if it turns out to be a perf problem.

mirichmo · Nov 21, 2016

+                    }
+                }
+
+                if (string.IsNullOrEmpty(connectionInfo.ComputerName))


Based on the checks of lines 829 and 835, the only way this can be true is if it is not provided via ComputerName or HostName. What about just checking for that condition as a separate IF check after the checks on 815 and 820?

It is possible for ComputerName or HostName to exist but contain only space characters. So the check would have to be a contains coupled with a not-empty check. It is easier to do the single check here.

mirichmo · Nov 21, 2016

+        #region Supress PSRemotingBaseCmdlet SSH hash parameter set
+
+        /// <summary>
+        /// Connection


Yes. Enter-PSSession cmdlet derives from a base cmdlet that defined SSHConnection parameter, which is not applicable here and must be suppressed. This is done throughout our code base.

My comment was specific to the content of the comment on line 180. It is not clear. In other places you state, "Suppress Connection"

Got it. Fixed.

mirichmo · Nov 21, 2016

    <value>The SSH client session has ended with no error message.</value>
  </data>
+  <data name="MissingRequiredSSHParameter" xml:space="preserve">
+    <value>The provided SSHConnection hash table is missing the required ComputerName or HostName parameter.</value>


Hashtable is a single word since it is referring to System.Collections.Hashtable

mirichmo · Nov 21, 2016

+## SSH Remoting cmdlet tests
+##
+
+Describe "SSHTransport switch parameter value" -Tags 'CI' {


These should be "Feature" tests because they are all negative.

mirichmo · Nov 21, 2016

+        }
+        catch { $paramError = $_ }
+
+        $paramError.FullyQualifiedErrorId | Should Match "ParameterArgumentValidationError"


Do this check within the catch block and make it explicit:

$_.FullyQualifiedErrorId | Should Be "ParameterArgumentValidationError,<...>" <--Needs additional stuff to match the FQEID

Fixed catch block. We need "Should Match" because FQEIDs can and are augmented over time, which breaks tests.

mirichmo · Nov 21, 2016

+    }
+}
+
+Describe "SSHConnection parameter hashtable error conditions" -Tags 'CI' {


Same comments for the previous Describe apply here too.

Are you going to add any positive test cases for this feature? If so, those should be added as "CI."

Fixed. Functional tests will be added when SSH remoting can be enabled via Enable-SSHRemoting. Currently it is a manual process. See RFC.

SteveL-MSFT · Nov 18, 2016

+                        throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter);
+                    }
+
+                    if (paramName.Equals("ComputerName", StringComparison.OrdinalIgnoreCase) || paramName.Equals("HostName", StringComparison.OrdinalIgnoreCase))


We should return error if we encounter both ComputerName and HostName (should be mutually exclusive) as it's ambiguous of the intent and probably a mistake.

SteveL-MSFT · Nov 18, 2016

+                    {
+                        connectionInfo.UserName = paramValue;
+                    }
+                    else if (paramName.Equals("KeyFilePath", StringComparison.OrdinalIgnoreCase) || paramName.Equals("IdentityFilePath", StringComparison.OrdinalIgnoreCase))


KeyFilePath and IdentityFilePath should also be mutually exclusive

SteveL-MSFT · Nov 18, 2016

                   ParameterSetName = EnterPSSessionCommand.VMNameParameterSet)]
        public String ConfigurationName { get; set; }

+        #region Supress PSRemotingBaseCmdlet SSH hash parameter set


'supress' is spelled wrong, but not sure why this word is even here?

Fixed. This cmdlet derives from a base cmdlet that includes the SSHConnection parameter. But this cmdlet cannot support SSHConnection hashtable (since Enter-PSSession works only on single session) and we have to remove it from the parameter set. This is common in our code base and the word "suppress" is used for this and I continued using the term here.

This has not been addressed yet

Thanks. Not sure how I missed that. Fixed now.

…ting Invoke-Command parameter set

…ers to no longer work. This fix separates the SSH parameter sets from the existing WinRM parameter sets.

mirichmo · Dec 2, 2016

@SteveL-MSFT Do you have any other comments or concerns? If not, I'd like to merge this PR.

PaulHigin added 6 commits September 28, 2016 12:34

RFC SSH remoting cmdlet updates.

6a618a3

Tweaks to SSH remoting cmdlets

94b6bdd

Merge branch 'master' of https://github.com/PaulHigin/PowerShell into…

72ab5e3

… RFCRemoteCmdlets

Merge branch 'master' of https://github.com/PaulHigin/PowerShell into…

245f3f1

… RFCRemoteCmdlets

SSHTransport parameter is now optional instead of mandatory. Aded fix…

311554b

… for CoreCLR entry point when hosted in SSH as a subsystem.

Added tests

7845ae1

msftclas added the cla-not-required label Nov 17, 2016

PaulHigin added WG-Remoting PSRP issues with any transport layer and removed cla-not-required labels Nov 17, 2016

PowerShellTeam added the Review - Needed label Nov 17, 2016

mirichmo self-assigned this Nov 18, 2016

iSazonov reviewed Nov 18, 2016

View reviewed changes

Code review feedback

8738722

mirichmo suggested changes Nov 21, 2016

View reviewed changes

mirichmo added Review - Waiting on Author and removed Review - Needed labels Nov 21, 2016

SteveL-MSFT requested changes Nov 21, 2016

View reviewed changes

PowerShellTeam added Review - Needed and removed Review - Waiting on Author labels Nov 21, 2016

PaulHigin added 3 commits November 21, 2016 14:47

Code review comment changes

baae301

Making SSHTransport switch parameter mandatory because otherwise exis…

652f5f3

…ting Invoke-Command parameter set

The new SSH parameter set was causing legacy positional based paramet…

7e24d09

…ers to no longer work. This fix separates the SSH parameter sets from the existing WinRM parameter sets.

mirichmo added Review-Waiting on Author and removed Review - Needed The PR is being reviewed Review-Needed labels Nov 30, 2016

Fixed region name spelling error.

3523854

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 1, 2016

mirichmo approved these changes Dec 2, 2016

View reviewed changes

SteveL-MSFT approved these changes Dec 2, 2016

View reviewed changes

mirichmo merged commit d2bf629 into PowerShell:master Dec 2, 2016

PaulHigin deleted the RFCRemoteCmdlets branch December 7, 2016 17:08


		It "New-PSSession SSHTransport parameter cannot have false value" {

		[System.Management.Automation.ErrorRecord] $paramError = $null


		Describe "SSHConnection parameter hashtable error conditions" -Tags 'CI' {

		It "SSHConnection parameter hashtable cannot contain empty parameter names" {

Search code, repositories, users, issues, pull requests...

Conversation

PaulHigin commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirichmo Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirichmo Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirichmo commented Dec 2, 2016

Uh oh!

mirichmo Nov 21, 2016 •

edited

Loading

mirichmo Nov 21, 2016 •

edited

Loading