Make 'LoginUtils.requireLogin()' to not block the execution.

[vrubezhny]

Checking whether the user is logged in should not block 'oc' binary execution in case login is required

[codecov]

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (bb194b9) 33.20% compared to head (b799123) 33.12%.

Files	Patch %	Lines
src/openshift/cluster.ts	0.00%	16 Missing ⚠️
src/cli.ts	0.00%	13 Missing ⚠️
src/util/loginUtil.ts	23.07%	10 Missing ⚠️
src/explorer.ts	0.00%	2 Missing ⚠️
src/oc/ocWrapper.ts	0.00%	2 Missing ⚠️
src/util/kubeUtils.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3674      +/-   ##
==========================================
- Coverage   33.20%   33.12%   -0.08%     
==========================================
  Files          84       84              
  Lines        6159     6195      +36     
  Branches     1258     1268      +10     
==========================================
+ Hits         2045     2052       +7     
- Misses       4114     4143      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[datho7561]

I think the reason this patch work is that it uses execFile instead of exec. exec creates a shell to run the program in, whereas execFile directly executes the executable. The oc binary is likely able to detect whether or not it's being run in a shell, so it won't produce the prompt for username and password when not being invoked through a shell. This might cause issues on Windows; I'll have to double check.

I don't think this timeout does anything, since it's mispelt: https://github.com/redhat-developer/vscode-openshift-tools/pull/3674/files#diff-fb06bee2638b8012f20db654d2191ea1dd8448a73ebe4da36750b489a188ecbbR469 .

[datho7561]

I did some testing on Windows and non-OpenShift clusters, and it seems to be working there as well (with the caveat that you can't log in/out of non-OpenShift clusters).

[vrubezhny]

I think the reason this patch work is that it uses execFile instead of exec. exec creates a shell to run the program in, whereas execFile directly executes the executable. The oc binary is likely able to detect whether or not it's being run in a shell, so it won't produce the prompt for username and password when not being invoked through a shell. This might cause issues on Windows; I'll have to double check.

May be, but oc is actually prompts for a username to be entered, which you can check in debugger. The only thing changed here is a timeout added, which kills the oc process when timeout expires - thus resolving the hang-up problem

I don't think this timeout does anything, since it's mispelt: https://github.com/redhat-developer/vscode-openshift-tools/pull/3674/files#diff-fb06bee2638b8012f20db654d2191ea1dd8448a73ebe4da36750b489a188ecbbR469 .

There is 10 seconds default value for the timeout used... So, misspelling actually doesn't break the things (But yes, it is misspelled)

[vrubezhny]

I think we should be good to remove this function and replace it with Oc.Instance.isLoggedIn() everywhere it's used.

@datho7561 I thought we can make oc.Instance.getCurrentUser() to use execFile+timeout so it can be used to check if a user is logged in in LoginUtil.Instance.requireLogin() without any additional calls to Odo.Instance.getProjects() and adding any new isLoggedIn() method... (because actually this new isLoggedIn method executes the same oc whoami as oc.Instance.getCurrentUser() does, it just does it a bit different way).

[datho7561]

I thought we can make oc.Instance.getCurrentUser() to use execFile+timeout so it can be used to check if a user is logged in in LoginUtil.Instance.requireLogin() without any additional calls to Odo.Instance.getProjects() and adding any new isLoggedIn() method...

Good call. I like that refactoring better.

Maybe this will end up fixing the issue Mohit has been running into (and me occasionally) where the view takes a while to load if the kubecontext is set to an inaccessible cluster.

[vrubezhny]

Maybe this will end up fixing the issue Mohit has been running into (and me occasionally) where the view takes a while to load if the kubecontext is set to an inaccessible cluster.

I don't think so... I haven't dig into this yet, maybe it'll speed up the things a little bit, but not too much.

[vrubezhny]

where the view takes a while to load if the kubecontext is set to an inaccessible cluster.

If a cluster "is inaccessible" we're actually aren't able to speed up anything. We still need to log in and this will take as much time as it takes (if possible at all). The only thing we can do is limiting the time to connect/login with a timeout (and make such timeout to be configurable through a user preference - because the normal time needed to login to different clusters may vary a lot, so it's difficult to imagine/calculate what default value could it be).

[vrubezhny]

I did some testing ...(with the caveat that you can't log in/out of non-OpenShift clusters).

Do we have any special binary/API to be invoked for non-OpenShift clusters? I might be missing something but I thought we always use oc login / oc logout, don't we?

[datho7561]

Most non-OpenShift clusters don't have a username/password system. Instead, you configure an access token in your kubeconfig, and any Kubernetes clients (eg. kubectl, oc, any libraries) should use those.

[vrubezhny]

Most non-OpenShift clusters don't have a username/password system. Instead, you configure an access token in your kubeconfig, and any Kubernetes clients (eg. kubectl, oc, any libraries) should use those.

So, the solution for Logout might be the following:

• call oc logout, then:
  • if this succeeds: leave as is (if it was an openshift-like cluster, the :"current-context" property will not be modified on ~/.kube/config)
  • if this fails: this is highly-likely a non-openshift cluster, so execute oc config unset current-context (this will clear the "current-context" property, making OS Tools to show "Login" and other buttons.

Also, I don't know if we really need this, but as we cannot login to a non-openshift cluster (because we cannot provide user credentials nor a token (like for a Kind-cluster), we probably can skip User Credentials/Token selection (and the following try to login using them).. So for Login we can do the following:

• Select cluster URL and try to "fetch", if this succeeds:
  • Try finding in ~/.kube/config cluster name for server === <cluster URL> and execute oc config use-context <cluster name>
  • if this succeeds, execute oc cluster-info and check if the cluster is running on specified cluster URL:
    • if succeeds - end of login cycle - we're successfully "logged in" to a Kind-like cluster
  • otherwise, we're not on a Kind-like cluster (or it's improperly setup/configured/not running/etc.) - we need to continue with User Credentials/Token selection (and the following try to login using them) - thus trying to log in to an openshift-like cluster

[vrubezhny]

The demo of login/logout to Kind and OS Clusters, and switching to the these clusters in case we're already logged in:
Screencast from 2023-12-15 20-04-45.webm

This should be changed now - you can login to /logout from non-OpenShift clusters

[vrubezhny]

@datho7561 Could you, please, take a look again. Now 'Login' works (during my tests) for any type of cluster including Kind and Sandbox, the only problem is a ~3 seconds delay that may occur while checking whether we're logged in in case we're logged in to an OS cluster (potentially any remote/slow/etc. cluster) where oc status takes about 3 seconds to successfully complete in my case.

[datho7561]

Sorry about the delay in the review. This is working very well! I found two small things in the code.

[datho7561]

Looks good and works great. Thanks Victor!