Skip to content

Conversation

iQQBot
Copy link
Contributor

@iQQBot iQQBot commented Jan 22, 2022

Description

When use golang ssh client connect to workspace and execute some command, it return a error Err:wait: remote command exited without exit status or exit signal

This is because golang client send EOF flag too early

Related Issue(s)

Fixes #7771

How to test

Release Notes

NONE

Documentation

@roboquat roboquat added release-note-none team: workspace Issue belongs to the Workspace team size/S labels Jan 22, 2022
@codecov
Copy link

codecov bot commented Jan 22, 2022

Codecov Report

Merging #7772 (20994db) into cw/fix-4779 (87ca302) will not change coverage.
The diff coverage is n/a.

❗ Current head 20994db differs from pull request most recent head e66e66d. Consider uploading reports for the commit e66e66d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           cw/fix-4779    #7772   +/-   ##
============================================
  Coverage        28.54%   28.54%           
============================================
  Files               39       39           
  Lines             3293     3293           
============================================
  Hits               940      940           
  Misses            2301     2301           
  Partials            52       52           
Flag Coverage Δ
components-gitpod-cli-app 10.28% <ø> (ø)
components-ws-proxy-app 68.32% <ø> (ø)
installer-raw-app 5.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ca302...e66e66d. Read the comment docs.

@iQQBot iQQBot changed the base branch from main to cw/fix-4779 January 22, 2022 17:39
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch from 0e2e6e6 to e60a4f2 Compare January 22, 2022 18:20
@roboquat roboquat added size/L and removed size/S labels Jan 22, 2022
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch 2 times, most recently from cd5b8c4 to 5030f85 Compare January 22, 2022 19:21
@roboquat roboquat added size/XL and removed size/L labels Jan 22, 2022
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch 3 times, most recently from d155f5c to c886169 Compare January 23, 2022 05:37
@iQQBot iQQBot changed the title Fixes SSH gateway compatibility with some automatic client Improve SSH gateway compatibility with some automatic client Jan 23, 2022
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch from c7097c3 to ceb4762 Compare January 23, 2022 06:10
@iQQBot iQQBot changed the title Improve SSH gateway compatibility with some automatic client Refactor and improve SSH gateway compatibility with some automatic client Jan 23, 2022
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch 2 times, most recently from 433efca to f99fde5 Compare January 23, 2022 06:43
@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 23, 2022

/werft run

👍 started the job as gitpod-build-pd-ssh-gateway-fix.12

@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch 2 times, most recently from 3285466 to 4d185f4 Compare January 23, 2022 07:21
@akosyakov
Copy link
Member

akosyakov commented Jan 23, 2022

How does one test that it does not break something else? Are there ways to add unit tests or something? Please fill in How to test section.

@akosyakov akosyakov requested review from csweichel and aledbf January 23, 2022 07:52
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch from a8f5eaa to 41a0ecc Compare January 23, 2022 07:59
@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 23, 2022

@akosyakov
I currently use a manual approach to testing, adding a lot of logs in key locations to verify that expectations are met, and there is no good way to unit test due to the large coupling with Gitpod internal

@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 23, 2022

/werft run with-clean-deployment

👍 started the job as gitpod-build-pd-ssh-gateway-fix.17

@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch 2 times, most recently from 103c010 to ed9b93e Compare January 23, 2022 09:17
@akosyakov
Copy link
Member

akosyakov commented Jan 23, 2022

I currently use a manual approach to testing, adding a lot of logs in key locations to verify that expectations are met, and there is no good way to unit test due to the large coupling with Gitpod internal

Please write down manual tests. In long term can we mock Gitpod internals and write tests against them to focus on SSH bits? I don't see how we can review and be sure about quality about one or another way of testing it.

@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch from ed9b93e to 20994db Compare January 23, 2022 10:21
@iQQBot iQQBot force-pushed the pd/ssh-gateway-fix branch from 20994db to e66e66d Compare January 23, 2022 11:13
@csweichel
Copy link
Contributor

csweichel commented Jan 24, 2022

Please write down manual tests. In long term can we mock Gitpod internals and write tests against them to focus on SSH bits? I don't see how we can review and be sure about quality about one or another way of testing it.

💯 agreed. We can mock the info provider within ws-proxy and start SSH server using golang's ssh package akin to httptest

@csweichel
Copy link
Contributor

@iQQBot as @akosyakov pointed out, I'd also appreciate some detail in the "How to test" section. I'd go through those tests then. Code-wise the changes look good - thank you for fixing the blunders in my original changeset (lesson learned: don't get cocky and don't rush code).

@csweichel
Copy link
Contributor

csweichel commented Jan 24, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-pd-ssh-gateway-fix.22

@csweichel
Copy link
Contributor

I ran the code outlined in #7771: it worked when run against this branch, it fails when run against production. I reckon that for now we're good. Considering the time-sensitive manner of this change, let's merge it.

@iQQBot Please, for all future PRs do fill in the "How to test" section - if it's manual tests that's fine, but others need to know how to test the PR.

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 282b72230d97a63deb90e91f901f91c50b950446

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel

Associated issue: #7771

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@roboquat roboquat merged commit 15577e0 into cw/fix-4779 Jan 24, 2022
@roboquat roboquat deleted the pd/ssh-gateway-fix branch January 24, 2022 08:22
@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 24, 2022

@csweichel Thanks for review, I will update "How to test" section later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSH Gateway is not compatible with golang ssh client
4 participants