Fix minikube tunnel for Docker on Windows

[blueelvis]

With this PR, the following fixes are done -

1. Fixes tunnel on windows: "sudo": executable file not found in %PATH% #9078

Also, for the fix to work, one needs to have the latest package of openssh-portable installed on Windows. The default installation which comes in Windows 10 is very old and gives Linux port warnings about how 80 & 443 are only allowed with sudo.
MS has said that there was some miscommunication regarding the upgrade to latest package for SSH so that couldn't be pushed but they are aiming for the next major release which I don't know when that would be. Reference - PowerShell/Win32-OpenSSH#1693

Do you think it would be good to add a check and have some documentation on this so that we output an error if the latest version of SSH is not installed on Windows?

[medyagh]

I believe we should be able to fix the ingress and tunnel separately

can we make this PR only about minikube tunnel on windows and also ensure the integration tests that we "Skip" for windows Tunell are not skipped?

[blueelvis]

@medyagh

I believe we should be able to fix the ingress and tunnel separately

can we make this PR only about minikube tunnel on windows and also ensure the integration tests that we "Skip" for windows Tunell are not skipped?

I have created a separate PR for the Ingress fix over here - #9761

Regarding the tests, I see that they are currently working? I checked PR #9756 logs for Docker Windows and they seem to be working. If you say, I can add a test which tests the aspect mentioned in the bug i.e. Contour on Windows and ensure it works?

Also, what do you think about adding a check for the SSH version?

[priyawadhwa]

Hey @blueelvis are you still working on this?

[blueelvis]

@priyawadhwa - I am out for a wedding this week. I will finish this on the coming weekend. Thanks, Pranav

…

On Thu, Dec 3, 2020 at 6:44 AM Medya Ghazizadeh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/drivers/kic/kic.go <#9753 (comment)>: > + // Get the SID of the current user + currentUserSidCmd := exec.CommandContext(ctx, path, "-NoProfile","-NonInteractive","([System.Security.Principal.WindowsIdentity]::GetCurrent()).User.Value") + currentUserSidOut, currentUserSidErr := currentUserSidCmd.CombinedOutput() + if currentUserSidErr != nil { + return errors.Wrap(currentUserSidErr, "unable to determine current user's SID") + } + + icaclsArguments := fmt.Sprintf(`"%s" /grant:r *%s:F /inheritancelevel:r`, keyPath, strings.TrimSpace(string(currentUserSidOut))) + icaclsCmd := exec.CommandContext(ctx, path, "-NoProfile","-NonInteractive","icacls.exe", icaclsArguments) + icaclsCmdOut, icaclsCmdErr := icaclsCmd.CombinedOutput() + + if icaclsCmdErr != nil { + return errors.Wrap(icaclsCmdErr, "unable to execute icacls to set permissions") + } + + if !strings.Contains(string(icaclsCmdOut),"Successfully processed 1 files; Failed processing 0 files") { this is too specific message, it is quite possible it changes, lets choose a better way — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9753 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVJQI4ZAMXYXTFCA2275MDSS3Q55ANCNFSM4T5FXXPQ> .

[medyagh]

@priyawadhwa - I am out for a wedding this week. I will finish this on the coming weekend. Thanks, Pranav
…
On Thu, Dec 3, 2020 at 6:44 AM Medya Ghazizadeh @.> wrote: @.* commented on this pull request. ------------------------------ In pkg/drivers/kic/kic.go <#9753 (comment)>: > + // Get the SID of the current user + currentUserSidCmd := exec.CommandContext(ctx, path, "-NoProfile","-NonInteractive","([System.Security.Principal.WindowsIdentity]::GetCurrent()).User.Value") + currentUserSidOut, currentUserSidErr := currentUserSidCmd.CombinedOutput() + if currentUserSidErr != nil { + return errors.Wrap(currentUserSidErr, "unable to determine current user's SID") + } + + icaclsArguments := fmt.Sprintf("%s" /grant:r *%s:F /inheritancelevel:r, keyPath, strings.TrimSpace(string(currentUserSidOut))) + icaclsCmd := exec.CommandContext(ctx, path, "-NoProfile","-NonInteractive","icacls.exe", icaclsArguments) + icaclsCmdOut, icaclsCmdErr := icaclsCmd.CombinedOutput() + + if icaclsCmdErr != nil { + return errors.Wrap(icaclsCmdErr, "unable to execute icacls to set permissions") + } + + if !strings.Contains(string(icaclsCmdOut),"Successfully processed 1 files; Failed processing 0 files") { this is too specific message, it is quite possible it changes, lets choose a better way — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9753 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVJQI4ZAMXYXTFCA2275MDSS3Q55ANCNFSM4T5FXXPQ .

sounds good :) thanks for updating us

[blueelvis]

@medyagh - I tried to implement SSH Version detection but it doesn't seem to be very straight forward. On my Windows 10 machine alone, I found the following versions in different ways -

1. MobaXterm on Windows - OpenSSH_7.1p2, OpenSSL 1.0.1g 7 Apr 2014
2. WSL - OpenSSH_7.6p1 Ubuntu-4ubuntu0.3, OpenSSL 1.0.2n 7 Dec 2017
3. Latest SSH Package for Windows - OpenSSH_for_Windows_8.0p1, LibreSSL 2.6.5
4. SSH Package by default installed in Windows 10 - OpenSSH_for_Windows_7.7p1, LibreSSL 2.6.5

Should I just add a warning that the user might have to install the latest Open SSH Package in case any service is using ports which require sudo on linux? - https://chocolatey.org/packages/openssh/8.0.0.1

The issue is that the old version which is by default installed on Windows 10 is not a proper port and if a service tries to expose a port < 1024, it errors out along the lines - "requires root permissions"

[tstromberg]

Other than my comment, I think this PR is worth merging as is.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bl-ue, blueelvis
To complete the pull request process, please assign tstromberg after the PR has been reviewed.
You can assign the PR to them by writing /assign @tstromberg in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tstromberg]

Thank you!

[Code7Bit]

Thank you 😌