Feature/remove public key after destroyed #910

9547 · 2020-11-16T22:57:05Z

What problem does this PR solve?

Try to implement #898

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release notes:

Remove SSH public key if the cluster was destroyed

codecov-io · 2020-11-16T22:59:25Z

Codecov Report

Merging #910 (8f82e4f) into master (1f0cbac) will decrease coverage by 0.89%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #910      +/-   ##
==========================================
- Coverage   55.70%   54.81%   -0.90%     
==========================================
  Files         263      263              
  Lines       19389    19427      +38     
==========================================
- Hits        10800    10648     -152     
- Misses       6875     7080     +205     
+ Partials     1714     1699      -15

Flag	Coverage Δ
cluster	`42.23% <51.72%> (-1.22%)`	⬇️
dm	`24.14% <42.42%> (+0.21%)`	⬆️
integrate	`49.02% <50.00%> (-0.93%)`	⬇️
playground	`20.21% <ø> (ø)`
tiup	`16.90% <0.00%> (-0.02%)`	⬇️
unittest	`23.01% <0.00%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/cluster/operation/operation.go	`78.26% <ø> (ø)`
components/dm/command/scale_in.go	`65.33% <37.50%> (-2.32%)`	⬇️
pkg/cluster/operation/destroy.go	`50.15% <46.34%> (-4.74%)`	⬇️
pkg/cluster/executor/executor.go	`60.60% <50.00%> (-2.36%)`	⬇️
pkg/cluster/operation/action.go	`58.25% <100.00%> (-1.30%)`	⬇️
pkg/cluster/operation/scale_in.go	`46.49% <100.00%> (-13.38%)`	⬇️
pkg/cluster/task/env_init.go	`55.88% <100.00%> (+2.39%)`	⬆️
pkg/cluster/task/task.go	`74.10% <100.00%> (+0.23%)`	⬆️
components/cluster/command/prune.go	`23.80% <0.00%> (-33.34%)`	⬇️
pkg/cluster/api/binlog.go	`12.17% <0.00%> (-26.96%)`	⬇️
... and 16 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 1f0cbac...8f82e4f. Read the comment docs.

AstroProfundis

And as we support deploying cluster with user specified SSH key (instead of generating new), so it would be better to check for duplicate during deploy process, and also not deleting the public key by default on scaling in and destroying.

AstroProfundis · 2020-11-17T03:24:45Z

components/cluster/command/prune.go

@@ -73,7 +73,7 @@ func destroyTombstoneIfNeed(clusterName string, metadata *spec.ClusterMeta, opt
 		return perrs.AddStack(err)
 	}

-	nodes, err := operator.DestroyTombstone(ctx, topo, true /* returnNodesOnly */, opt, tlsCfg)
+	nodes, err := operator.DestroyTombstone(ctx, topo, true /* returnNodesOnly */, opt, tlsCfg, ctx.PublicKeyPath)


ctx is passed to DestroyTombstone() so I don't think it's necessary to pass ctx.PublicKeyPath again.

The first argument inside DestoryTombestone() is ExecutorGetter interface:

tiup/pkg/cluster/operation/destroy.go

Lines 414 to 420 in cc41ad5

func DestroyTombstone(

getter ExecutorGetter,

cluster *spec.Specification,

returNodesOnly bool,

options Options,

tlsCfg *tls.Config,

) (nodes []string, err error) {

So I need to cast it into task.Context, but this results in import cycle error. Please let me know if I've misssed something.

Maybe we can define another interface for the first param of DestroyTombstone:

type SSHExecutorGetter interface { ExecutorGetter GetSSHKeySet() (string, string) // return both priv and public key }

@lucklove Or should we extend the original ExecutorGetter interface with:

type ExecutorGetter interface { Get(host string) (e executor.Executor) GetSSHKeySet() (string, string) }

@lucklove I did it this way first, if you have any question, please let me know.

@lucklove Or should we extend the original ExecutorGetter interface with:

type ExecutorGetter interface { Get(host string) (e executor.Executor) GetSSHKeySet() (string, string) }

It's OK at this time. In the future there may be some executors that don't relay on ssh, at that time we can refactor this.

components/dm/command/scale_in.go

pkg/cluster/manager.go

pkg/cluster/operation/destroy.go

lucklove · 2020-11-17T03:35:50Z

Seems test broken: cannot stat '~/.tiup/storage/dm/clusters/test_cmd/ssh/id_rsa'
May related to this PR.

lucklove · 2020-11-20T08:34:30Z

What's the status of this? It seems still using ctx.PublicKeyPath? https://github.com/pingcap/tiup/pull/910/files#diff-c2cb8d42eb23179c5056c3d5c626c7f2ddabad3db5171c8f869d59f75ddffc42R76

9547 · 2020-11-23T23:54:56Z

@lucklove PTAL

lucklove

Rest LGTM

components/dm/command/scale_in.go

pkg/cluster/operation/destroy.go

AstroProfundis · 2020-11-30T08:24:46Z

/merge

ti-chi-bot · 2020-11-30T08:24:49Z

Can merge label has been added.

Git tree hash: 8f82e4f

ti-chi-bot · 2020-11-30T08:24:58Z

@9547: Your PR has out-of-dated and I have automatically updated it for you.

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 tidb-community-bots/ti-community-prow repository.

CLAassistant · 2020-11-30T08:25:06Z

All committers have signed the CLA.

9547 force-pushed the feature/remove-public-key-after-destroyed branch from 5c9d625 to 4d3b121 Compare November 16, 2020 23:16

AstroProfundis reviewed Nov 17, 2020

View reviewed changes

9547 force-pushed the feature/remove-public-key-after-destroyed branch from 4d3b121 to bf61123 Compare November 17, 2020 14:53

9547 requested review from lucklove and AstroProfundis November 19, 2020 12:59

jsvisa added 8 commits November 23, 2020 23:10

feat(cluster): delete public key if node was destroyed/pruned

508ba5e

feat(dm): delete public key if node was scaled in

4f589db

fix(cluster): trim public key's space

ed2b10e

test(cluster,dm): test public key should be deleted from remote

a86a3b9

fix(tests): don't double quote '~'

dac9d09

feat(cluster): extend ExecutorGetter to get sshkey

5ce654c

fix(tests/tiup-cluster): wrong cluster path

730c926

fix(cluster,dm): don't pass ctx.PublicKeyPath

7c7cfea

9547 force-pushed the feature/remove-public-key-after-destroyed branch from f7590f2 to 7c7cfea Compare November 23, 2020 15:10

lucklove reviewed Nov 24, 2020

View reviewed changes

components/dm/command/scale_in.go Outdated Show resolved Hide resolved

pkg/cluster/operation/destroy.go Outdated Show resolved Hide resolved

jsvisa added 5 commits November 25, 2020 05:20

style(*): a = a+1 -> a++

c38df19

style(*): format the other instCount++ style

db8265a

fix(tests): add StrictHostKeyChecking=no to disable prompt input

c221ee7

style(cluster): duplicate log

5c2f7cd

fix(tests): when set -e the error raised first then pipe

4e5f4e9

lucklove approved these changes Nov 25, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 25, 2020

Merge branch 'master' into feature/remove-public-key-after-destroyed

8f82e4f

AstroProfundis approved these changes Nov 30, 2020

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 30, 2020

Merge branch 'master' into feature/remove-public-key-after-destroyed

a19e618

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 30, 2020

ti-chi-bot merged commit 5366a48 into pingcap:master Nov 30, 2020

9547 deleted the feature/remove-public-key-after-destroyed branch December 5, 2020 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/remove public key after destroyed #910

Feature/remove public key after destroyed #910

9547 commented Nov 16, 2020

codecov-io commented Nov 16, 2020 •

edited

Loading

AstroProfundis left a comment

AstroProfundis Nov 17, 2020

9547 Nov 17, 2020

lucklove Nov 17, 2020

9547 Nov 17, 2020

9547 Nov 17, 2020

lucklove Nov 19, 2020

lucklove commented Nov 17, 2020

lucklove commented Nov 20, 2020

9547 commented Nov 23, 2020

lucklove left a comment

AstroProfundis commented Nov 30, 2020

ti-chi-bot commented Nov 30, 2020

ti-chi-bot commented Nov 30, 2020

CLAassistant commented Nov 30, 2020 •

edited

Loading

	func DestroyTombstone(
	getter ExecutorGetter,
	cluster *spec.Specification,
	returNodesOnly bool,
	options Options,
	tlsCfg *tls.Config,
	) (nodes []string, err error) {

Feature/remove public key after destroyed #910

Feature/remove public key after destroyed #910

Conversation

9547 commented Nov 16, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Nov 16, 2020 • edited Loading

Codecov Report

AstroProfundis left a comment

Choose a reason for hiding this comment

AstroProfundis Nov 17, 2020

Choose a reason for hiding this comment

9547 Nov 17, 2020

Choose a reason for hiding this comment

lucklove Nov 17, 2020

Choose a reason for hiding this comment

9547 Nov 17, 2020

Choose a reason for hiding this comment

9547 Nov 17, 2020

Choose a reason for hiding this comment

lucklove Nov 19, 2020

Choose a reason for hiding this comment

lucklove commented Nov 17, 2020

lucklove commented Nov 20, 2020

9547 commented Nov 23, 2020

lucklove left a comment

Choose a reason for hiding this comment

AstroProfundis commented Nov 30, 2020

ti-chi-bot commented Nov 30, 2020

ti-chi-bot commented Nov 30, 2020

CLAassistant commented Nov 30, 2020 • edited Loading

codecov-io commented Nov 16, 2020 •

edited

Loading

CLAassistant commented Nov 30, 2020 •

edited

Loading