Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support native ssh client #615

Merged
merged 18 commits into from
Jul 27, 2020
Merged

Support native ssh client #615

merged 18 commits into from
Jul 27, 2020

Conversation

lucklove
Copy link
Member

@lucklove lucklove commented Jul 20, 2020

At current we use easyssh as the ssh tunnel. However, sometimes users will use some ssh plugin such as LDAP. In this case the easyssh client can't work.

This PR introduces an option for users to choose the native ssh client in his host as the ssh client to connect the cluster. This can enable any ssh plugins once the user configs them correctly.

TODOLIST:

  • Add native ssh Executor struct
  • Implement Execute method for native ssh executor
    • Support connect host with default key file
    • Support connect with password (via sshpass)
    • Support connect with user specified key file
  • Implement Transfer method for native ssh executor
  • Enable --native-ssh global flag
  • Adopt Linux
    • OpenSSH_8.0p1
    • OpenSSH_7.4p1
    • OpenSSH_5.3p1
  • Adopt macos
  • Adopt different ssh client version (optional)
    • Adopt different password prompt
  • Add test

@lonng lonng added the type/new-feature Categorizes pr as related to a new feature. label Jul 20, 2020
@lonng lonng linked an issue Jul 20, 2020 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2020

Codecov Report

Merging #615 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #615      +/-   ##
==========================================
- Coverage   47.95%   47.90%   -0.06%     
==========================================
  Files         225      224       -1     
  Lines       16787    16818      +31     
==========================================
+ Hits         8051     8057       +6     
- Misses       7610     7631      +21     
- Partials     1126     1130       +4     
Flag Coverage Δ
#coverage 47.90% <ø> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...ithub.com/pingcap/tiup/pkg/cluster/executor/ssh.go 31.92% <0.00%> (-33.51%) ⬇️
...om/pingcap/tiup/components/cluster/command/root.go 42.77% <0.00%> (-0.63%) ⬇️
...ngcap/tiup/components/cluster/command/scale_out.go 9.05% <0.00%> (-0.04%) ⬇️
...rc/github.com/pingcap/tiup/pkg/cluster/task/ssh.go 71.05% <0.00%> (ø)
...om/pingcap/tiup/pkg/cluster/operation/operation.go 72.72% <0.00%> (ø)
.../pingcap/tiup/components/playground/utils/utils.go
...m/pingcap/tiup/components/playground/utils/http.go
go/src/github.com/pingcap/tiup/pkg/utils/exec.go 61.53% <0.00%> (ø)
.../pingcap/tiup/components/cluster/command/deploy.go 77.22% <0.00%> (+0.08%) ⬆️
...ithub.com/pingcap/tiup/pkg/cluster/task/builder.go 87.14% <0.00%> (+0.10%) ⬆️
... and 3 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 4ce9c65...07663f3. Read the comment docs.

args = append(args, "-i", e.Config.KeyFile)
}
if e.Config.Passphrase != "" {
args = append([]string{"sshpass", "-p", e.Config.Passphrase, "-P", "passphrase"}, args...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the sshpass always exists in the user environment?

Copy link
Member Author

@lucklove lucklove Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, It's a tradeoff. I think it's not a hard task to install it in the bastion host. It's not economical enough for us to implement sshpass in our code.

@lucklove lucklove added this to the v1.1.0 milestone Jul 22, 2020
@lucklove lucklove self-assigned this Jul 22, 2020
@lucklove lucklove force-pushed the ssh branch 5 times, most recently from 08c7e12 to c5feef7 Compare July 23, 2020 10:39
@lucklove lucklove changed the title Support native ssh Support native ssh client Jul 23, 2020
@lucklove lucklove marked this pull request as ready for review July 24, 2020 02:31
@lucklove lucklove requested a review from july2993 July 27, 2020 04:05
docker/secret/control.env Outdated Show resolved Hide resolved
Sudo: sudo,
}
if c.Password != "" || (c.KeyFile != "" && c.Passphrase != "") {
_, _, e.ConnectionTestResult = e.Execute(connectionTestCommand, false, executeDefaultTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't try to connect remote when we create the instance of SSHExecutor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't try it this time when the user actually executes the command, it may block as long as the command timeout value since the wrong prompt of ssh. What's worse is that there will be no possibility to get clear what happened at that time.

For example, the user can change the ssh prompt to:

root@172.16.5.141's secret:

But the sshpass may expect it be:

root@172.16.5.141's password:

And then the command will hang forever (until receiving the kill signal).

pkg/localdata/constant.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM
Well done.

.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
components/cluster/command/root.go Outdated Show resolved Hide resolved
components/cluster/command/root.go Outdated Show resolved Hide resolved
components/cluster/command/root.go Outdated Show resolved Hide resolved
pkg/cluster/executor/ssh.go Outdated Show resolved Hide resolved
pkg/exec/run.go Outdated Show resolved Hide resolved
lucklove added 3 commits July 27, 2020 17:45
Signed-off-by: lucklove <gnu.crazier@gmail.com>
- Support password

Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
lucklove and others added 13 commits July 27, 2020 17:45
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Co-authored-by: Lonng <heng@lonng.org>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
lucklove added 2 commits July 27, 2020 18:12
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Signed-off-by: lucklove <gnu.crazier@gmail.com>
Copy link
Contributor

@AstroProfundis AstroProfundis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 27, 2020
@lucklove lucklove merged commit f1142b1 into pingcap:master Jul 27, 2020
@lucklove lucklove deleted the ssh branch July 27, 2020 13:41
lucklove added a commit to lucklove/docs-cn that referenced this pull request Jul 28, 2020
document for pingcap/tiup#615

Signed-off-by: lucklove <gnu.crazier@gmail.com>
lucklove added a commit to lucklove/docs-cn that referenced this pull request Jul 28, 2020
document for pingcap/tiup#615

Signed-off-by: lucklove <gnu.crazier@gmail.com>
TomShawn added a commit to pingcap/docs-cn that referenced this pull request Aug 4, 2020
* Add document for tiup-cluster

document for pingcap/tiup#615

Signed-off-by: lucklove <gnu.crazier@gmail.com>

* Update tiup/tiup-cluster.md

Co-authored-by: Lonng <heng@lonng.org>

* Address comment

Signed-off-by: lucklove <gnu.crazier@gmail.com>

* Update tiup/tiup-cluster.md

Co-authored-by: Lonng <heng@lonng.org>
Co-authored-by: TomShawn <41534398+TomShawn@users.noreply.github.com>
@lucklove lucklove mentioned this pull request Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ssh LDAP extension
5 participants