fix (shell) : Improve CRC shell detection on unix environments (#3767)

[rohanKanojia]

Description

Fixes: #3797

Relates to: #3797

• Currently, we rely on SHELL environment variable for detecting active shell type. This will work when the environment variable is set. As per my observations, this environment variable is not set explicitly by various Linux shell environments (bash,zsh,fish). We should detect the currently active shell by checking the active processes instead.
• While generating statements for export statements on Windows, we shall make sure that we have converted windows paths to Linux paths.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Proposed changes

• Instead of marking shell_test.go to not run on Windows, move unix-specific tests to shell_unix_test.go, add generic platform-independent tests to shell_test.go
• Add bash and zsh to the list of supported shell environments on Windows.
• Instead of instructing user to use --shell flag to print shell specific instructions, detect shell automatically by inspecting currently open processes:
  • On Linux use ps command to get list of active shell processes and pick the most recent process id.
  • On WSL (Windows Subsystem for Linux), use wsl tool to execute linux ps process to get list of active shell processes and return the one with most recent process id.
• In the presence of UNIX-like shell environments, convert Windows paths to UNIX paths.
  • If WSL (Windows Subsystem for Linux) is detected, use wslpath utility tool to perform this conversion.
  • For other scenarios (like case of Git Bash), simply convert windows path delimiters to linux and remove the windows drive prefix

With these changes, we should see the correct output of crc podman-env and crc oc-env on:

• Windows Subsystem for Linux
  • Bash (default shell)
  • fish
  • zsh
• Regular Linux distributions
  • Bash (default shell)
  • fish
  • zsh

Running crc oc-env on Git Bash:

Old Behavior

$ crc oc-env
SET PATH=C:\Users\rokum\.crc\bin\oc;%PATH%
REM Run this command to configure your shell:
REM @FOR /f "tokens=*" %i IN ('crc oc-env') DO @call %i

New Behavior

$ /c/Users/rokum/go/bin/crc oc-env
export PATH="/C/Users/rokum/.crc/bin/oc:$PATH"
# Run this command to configure your shell:
# eval $(crc oc-env)

Testing

1. Start CRC cluster
2. Open some linux environment on windows (like Git Bash) and check result of crc podman-env / crc oc-env commands. They should not print output with respect to current shell in use.

Contribution Checklist

• I have read the contributing guidelines
• My code follows the style guidelines of this project
• I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I tested my code on specified platforms
  • Linux
  • Windows
  • MacOS

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Code Review Process.

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

[praveenkumar]

I can see that in that function you are then trying to split the line and ignoring 0 index which can be done using following so we don't have to ignore it later.

$ ps -o pid=,comm=
  57609 bash
 242709 ps

[rohanKanojia]

You're right. Thanks for the hint! I didn't know about this.

[praveenkumar]

This function is doing lot of things, may be add a comment what actually this is doing with example so would be easy for review.

[rohanKanojia]

Thanks, I'll update it as per your comments.

[rohanKanojia]

I've added comments to this method and other related methods.

[praveenkumar]

why we are implementing this sorting function for slice?

[rohanKanojia]

Here we want to get the shell process with most recent process id (latest). Therefore I am sorting the arraylist in reverse order and getting first element for detecting currently active shell.

[rohanKanojia]

I've removed this sorting method. We can achieve same thing using --sort flag in ps command.

[praveenkumar]

In this method you are not performing any sorting because the ps command which is used already performing the sort with pid so better to update that info. https://play.golang.com/p/QhNeqa5tnEO

[rohanKanojia]

@praveenkumar : Sorry, I forgot to update the method comment after making changes to the method.

Should be okay now.

[openshift-ci]

@rohanKanojia: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration-crc	4314e47	link	true	/test integration-crc
ci/prow/e2e-crc	4314e47	link	true	/test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[gbraad]

As mentioned in the first message, and related to #4537:

---

• I tested my code on specified platforms
  • Linux
  • Windows
  • MacOS

---

Have tests on MacOS been performed by reviewer(s)?

[praveenkumar]

Have tests on MacOS been performed by reviewer(s)?

No, I did the review and didn't perform any test on mac for same, my understanding was since it have unit tests which run for each platform but those were mocked one :(

[rohanKanojia]

@praveenkumar @gbraad : I apologize for introducing this regression. In unit tests, I had mocked the command output that's why they were not able to catch this failure :-(

[cfergeau]

use ps command to get list of active shell processes and pick the most recent process id.

I don't understand how the most recent process id relates to the shell I'm running crc in? I could have launched a bash shell at the beginning of my session, then spawn a zsh shell to do some testing, and get back to the initial bash shell to use crc?
A higher PID also does not guarantee the process is newer as PIDs can wrap around and start again at 1.

If we want to do some more extensive shell guessing by iterating over processes, could we not limit ourselves to the parents of the crc process?
This is actually partially done this way in the windows code

crc/pkg/os/shell/shell_windows.go

Lines 79 to 96 in cebafa4

	pid := os.Getppid()
	if pid < 0 || pid > math.MaxUint32 {
	return "", fmt.Errorf("integer overflow for pid: %v", pid)
	}
	shell, shellppid, err := getNameAndItsPpid(uint32(pid))
	if err != nil {
	return "cmd", err // defaulting to cmd
	}
	shell = shellType(shell, "")
	if shell == "" {
	shell, _, err := getNameAndItsPpid(shellppid)
	if err != nil {
	return "cmd", err // defaulting to cmd
	}
	return shellType(shell, "cmd"), nil
	}
	return shell, nil
	}

and now when this code calls shellType we run again ps in some cases.
I'd prefer that we extend this approach and drop the calls to an external command, and the parsing which comes with it.

[rohanKanojia]

I don't understand how the most recent process id relates to the shell I'm running crc in? I could have launched a bash shell at the beginning of my session, then spawn a zsh shell to do some testing, and get back to the initial bash shell to use crc?

@cfergeau : Could you please elaborate on this failing scenario? I tried creating a new terminal instance from the current bash session and created zsh and fish instances respectively. However, in the first bash session, I could not see the other shell processes with ps -o pid,comm:

$ ps  -o pid,comm
    PID COMMAND
 218741 bash
 290485 ps

From what I remember this getNameAndItsPpid l was returning the parent process name as wsl.exe (which was not what I was expecting).

We had tried other options like $$ to get the process id of the current shell but unfortunately, it doesn't work for fish shells.

[cfergeau]

Ah, I always use ps with -a, but its default behavior is a lot more restrictive as you described:

By default, ps selects all processes with the same effective user ID (euid=EUID) as the current user and associated with the same terminal as the invoker.

I can get this to misbehave though with 2 ssh sessions to a mac machine, run zsh in one, run bash in the other one, get back to the first one and run ps, I see the processes for all ssh sessions.

From what I remember this getNameAndItsPpid was returning the parent process name as wsl.exe (which was not what I was expecting).

getNameAndItsPpid may not have the behaviour you want, but can it be fixed to do what we need? Imo, this is what we want to do, start from the current process, and look at the process names/command lines until one is a shell process.

In the scenario described above on macos, with pstree I get this so I can find the right terminal without relying on "latest launched"

 |-+= 04833 root sshd: teuf [priv] 
 | \-+- 04835 teuf sshd: teuf@ttys007 
 |   \-+= 04836 teuf -zsh
 |     \-+= 04884 teuf bash
 |       \-+= 07461 teuf pstree
 |         \--- 07462 root ps -axwwo user,pid,ppid,pgid,command
 |-+= 04866 root sshd: teuf [priv] 
 | \-+- 04868 teuf sshd: teuf@ttys008 
 |   \--= 04869 teuf -zsh

(I'm not suggesting to use pstree directly, but to do something similar in go)

[rohanKanojia]

@cfergeau : Thanks, I've moved the discussion to a new issue #4562

Feel free to add any information that I might have missed. I'll start working on this issue soon.