Allow specifying a SSH key name for AWS #3215
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @johnzeringue. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/assign @zmerlynn |
| @@ -116,14 +116,21 @@ func (b *KopsModelContext) LinkToIAMInstanceProfile(ig *kops.InstanceGroup) *aws | |||
| return &awstasks.IAMInstanceProfile{Name: &name} | |||
| } | |||
|
|
|||
| // SSHKeyName computes a unique SSH key name, combining the cluster name and the SSH public key fingerprint | |||
| // SSHKeyName computes a unique SSH key name, combining the cluster name and the SSH public key fingerprint. | |||
| // If an SSH key name is provided in the cluster configuration, it will use that instead. | |||
| func (c *KopsModelContext) SSHKeyName() (string, error) { | |||
There was a problem hiding this comment.
Dumb questions, but when we do a delete a cluster do we delete the key? We have an approach of having shared items, such as https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/awstasks/vpc.go#L43. We would probably want that as well, and it may help with delete.
There was a problem hiding this comment.
when we do a delete a cluster do we delete the key?
I don't think we should. I'm not sure what's been done in the past, but I think that things created by kops should be deleted by kops and things not created by kops should not be deleted by kops.
I don't fully understand the current behavior on deletion (the task lifecycle is hard to follow in my IDE), but my intention is not to delete.
There was a problem hiding this comment.
@johnzeringue yes we should not delete a shared pre-existing ssh key. Does kops try with your changes?
There was a problem hiding this comment.
Also, this is just re-using the aws key, not the ssh public key that is installed on the users' machine? Correct?
There was a problem hiding this comment.
@johnzeringue yes we should not delete a shared pre-existing ssh key. Does kops try with your changes?
In our testing, it seems not.
Also, this is just re-using the aws key, not the ssh public key that is installed on the users' machine? Correct?
Yes
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/assign |
|
Code looks good - thanks @johnzeringue I had to check, but looking at the existing code we do compute the AWS-style-fingerprint and thus don't rely on the name. We currently embed the fingerprint in the name to avoid collisions and because we can't change a public key - and nor can we tag SSH public keys. And that means we don't delete the key as part of cluster deletion. But ... I'm wondering about the use case here. I never thought of the ability to create an SSH key as something particularly powerful (but do LMK!) We have the lifecycle work going on, which will separate out IAM & SSH permissions, and I know some people have wanted to split those further. But if the motivation here is to minimize the amount of permissions needed, it seems like the IAM permissions are the big one, not the permission to register an AWS public key! That said I don't think these are blocking issues to the PR, I would just like to better understand the use case. I was pondering the name - whether we should put this into an SSH section, but we don't have any other SSH fields (other than |
|
@justinsb the use case is separating the ability to provision EC2 instances with the ability to access those instances. There are other ways to do it, but preventing key creation is an easy way to create a strong access barrier. |
johnzeringue
force-pushed
the
specify-ssh-key-name
branch
from
76302f8
to
a3ec5b4
Compare
|
Documentation added |
|
@justinsb any more feedback on this? I'm happy to hop on Slack if you want to talk about use case more in depth |
|
You may need a rebase to get e2e passing. |
Related to kubernetes#2309, this allows naming an existing key pair using the cluster spec field `sshKeyName`. In our use case, kops can now be used without providing the ability to create EC2 key pairs.
johnzeringue
force-pushed
the
specify-ssh-key-name
branch
from
a3ec5b4
to
13d22fd
Compare
k8s-ci-robot
added
the
size/M
|
@chrislovecnm I tried rebasing, but e2e is still failing. Not quite sure what's wrong from the logs. Any tips? |
|
@johnzeringue we have been having problems with e2e. Most likely flakey testing not your code. How have you tested? /retest |
|
@chrislovecnm we created a new cluster in AWS with a preexisting SSH key. We verified that we could SSH into instances using the preexisting key. We destroyed the cluster and saw that it did not delete or attempt to delete the preexisting key. |
|
I've felt the need for this use-case too. Is this ready to be merged? @chrislovecnm I think you mentioned someone else instead of @justinsb by mistake 😅 |
|
@justinsb we good with this PR? I may need this as well. |
|
Thanks @ApsOps |
|
I know certain parties are super busy, merging this in, we can iterate if needs be |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrislovecnm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
@johnzeringue you mind filing an issue to add validation that this is aws only? I think, may be wrong, that GCE does not have this feature |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. . |
Related to #2309, this allows naming an existing key pair using the
cluster spec field
sshKeyName.In our use case, kops can now be used without providing the ability to
create EC2 key pairs.