openshift / cluster-operator

52 stars 35 forks source link

Avoid cloud ssh public key name collision #306

Closed twiest closed 6 years ago

twiest commented 6 years ago
twiest commented 6 years ago

This is a WIP because I'd like the team to weigh in on the decisions made.

Specific questions:

dgoodwin commented 6 years ago

Regarding the key only being used during provisioning, it remains on the systems though right? I.e. this may be a common way admins get access in the event of a serious failure. I'm not sure we've got that daemonset fully hooked up yet either, and I'm not sure where it's headed for 4.0 anymore.

For a typical account I would expect they plan to upload one master key and want to use it for all their clusters in the account. As such I think we should keep the door open for specifying it as part of your cluster deployment spec, and then a CO deployment or the tooling in front of it can do either as desired. If you're spinning up dozens of clusters in a new UI, it would be nice not to force you to paste in the same ssh key every time.

Clobbering keys could still be an issue for our development playbooks though, and we could tackle the problem there if desired. I'm not terribly worried about clobbering libra as that's really all we need to use, but if we're running into issues I could see us handling it in the playbooks by assuming to create an ssh key in AWS with the cluster name, each time we run create-cluster. Worth noting however that nothing can clean those up, as it's not a part of deprovision we can assume to do, and there is no deprovision playbook. We'd have to write something to scan and delete.

twiest commented 6 years ago

@dgoodwin I believe I've addressed your feedback given in this PR and in the meeting today. Would you please mind reviewing again?

The only thing not addressed yet is deprovision, which @joelddiaz thinks there's a flag to deprovision the cloud public ssh key in the deprovision playbook.

If the provisioning in this PR looks good to you, then I'd like to discuss the deprovisioning tomorrow sometime.

twiest commented 6 years ago

@dgoodwin @csrwng I added uninstalling the key and unit tests to verify the functionality.

I think this is ready to merge. I've removed the WIP tag from the title.

Can I please get a re-review from either one of you?

dgoodwin commented 6 years ago

/lgtm