SSH feature hangs on attempted login

changed milestone to %17.4

added Category:Workspaces devopscreate groupremote development typebug workspaces-workflowprioritized labels

assigned to @Saahmed

made the issue confidential

changed the description

I explain the possible cause/fix in the "Possible fixes" section @vtak LMK your thoughts please! Thanks cc @rmongare @adebayo_a

@Saahmed

I locally reverted those lines to run in their own goroutine as they did before the diff and I am able to ssh into the workspace I spun up locally. I could not do this before the patch I did. The customer's log does not match mine due to a difference in setup I am assuming, but they did report they were stuck after the SSH command (which is essentially what I experienced before the patch I did)

Could you share the patch?

diff --git a/pkg/sshproxy/server.go b/pkg/sshproxy/server.go
index a8832fe..11b1ee0 100644
--- a/pkg/sshproxy/server.go
+++ b/pkg/sshproxy/server.go
@@ -127,10 +127,10 @@ func (p *SSHProxy) handleSSHConnection(ctx context.Context, incomingConn net.Con
 		return
 	}
 
-	p.copyRequests(connCtx, clientReqChannel, backendConn)
-	p.copyRequests(connCtx, backendReqChannel, clientConn)
-	p.copyData(connCtx, clientConn, backendChannel)
-	p.copyData(connCtx, backendConn, clientChannel)
+	go p.copyRequests(connCtx, clientReqChannel, backendConn)
+	go p.copyRequests(connCtx, backendReqChannel, clientConn)
+	go p.copyData(connCtx, clientConn, backendChannel)
+	go p.copyData(connCtx, backendConn, clientChannel)
 
 	go func() {
 		waitErr := clientConn.Wait()

Is this patch enough?

Good find @Saahmed

Let's open up an MR to fix this quickly. ~~Maybe we should a failing test if possible?~~

Maybe we should a failing test if possible?

Scratch that. It will be too much work which is going to get removed when we rearchitect the proxy.

It Looks like a quick fix, but I would like to also ensure the patch does not break other assumptions. I would still want to write a test to ensure correctness as long as I do not need to overrefactor (since we will be replacing this approach anyway ). Is this urgent and blocking the customer? If yes I can put this on the top of my todos, if not I can schedule it later within the current sprint.

cc @adebayo_a @vtak

@vtak @Saahmed thanks for collaborating on this so far. Looks like gut feelings from both of you was to get some test coverage in there. What would you weigh this as with that in mind?

I am not too familiar with the SSH module but if the revert in the patch is all that is needed, I would say a 2 with testing included, without testing a 1.

If there could be other issues as a result of this, might need to be a 3. I would let @vtak decide since he has more experience with that area of the codebase, otherwise, I could do a spike to research on what should be done and come up with a well-refined issue someone could pick (will take longer though)

cc @adebayo_a

I don't want us to do any refactoring of the codebase or any additional improvements. Adding unit tests for the SSH code is very complicated, if not impossible. This is why we had written some integration tests. I want us to only make minimal changes in the code(as shown above) and fixing the integration tests.

Especially because we have plans to replace this. So any effort in refactoring and unit tests would be a wasted effort. However, integration tests would still be applicable in the new architecture as well.

I would weigh it as a 2. This involves the effort to identify why the integration test did not fail and fix it.

Is this urgent and blocking the customer?

Technically, it is. We have had a bad release with a broken functionality. But I'll let @adebayo_a take a call on the prioritization.

Is this urgent and blocking the customer? If yes I can put this on the top of my todos, if not I can schedule it later within the current sprint.

@Saahmed let’s get this out of the way now. You’re already in the details. Please add an appropriate weight to it. Got @michelle-chen’s on this already.

Thanks for jumping on this @Saahmed since it looks like it was going to be 1-2 weight and with you having good knowledge already makes sense to get this knocked off the list. If this grows beyond a 2 do let us know

@(confidential) this issue had a specific milestone but did not have a weight, so the milestone has been set back to %Next 1-3 releases.

changed milestone to %Next 1-3 releases

Is there a reason this needs to be confidential?

Did not notice it. Made it public.

@Saahmed - FYI, for future reference, by default, we should make public issues.

made the issue visible to everyone

added refined workflowready for development labels

added customer label

Adding bugfunctional severity1

added bugfunctional severity1 labels

added gitlab-org#12601 as parent epic

changed iteration to Workspaces Sep 9, 2024 - Sep 22, 2024

changed milestone to %17.5

changed the description

@Saahmed this issue had a specific milestone but did not have a weight, so the milestone has been set back to %Next 1-3 releases.

changed milestone to %Next 1-3 releases

Giving this a 2 weight so we can assign a milestone.

changed milestone to %17.5

set weight to 2

mentioned in merge request !9 (merged)

changed the description

local exploratory testing confirms functionality works.

With all 3 MRs merged: MR: Fix blocking SSH connections (!9 - merged)

MR: bump version (!11 - merged)

MR: Bump version for gitlab workspaces proxy (gitlab-org/gitlab!166426 - merged)

And we have follow-up issues to improve automated testing created. I'd say this is done. Should we proceed to instruct the customer to install the new version?

cc @adebayo_a @vtak @rmongare