Use existing SSHKeyName if no public key is created. #6886
Conversation
|
Hi @rralcala. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
cncf-cla: yes
pkg/model/sshkey.go
Outdated
| @@ -37,7 +37,9 @@ func (b *SSHKeyModelBuilder) Build(c *fi.ModelBuilderContext) error { | |||
| t := &awstasks.SSHKey{ | |||
| Name: s(name), | |||
| Lifecycle: b.Lifecycle, | |||
| PublicKey: fi.WrapResource(fi.NewStringResource(string(b.SSHPublicKeys[0]))), | |||
| } | |||
| if len(b.SSHPublicKeys) == 1 { | |||
There was a problem hiding this comment.
Can we make this >= 1 (so we don't change behaviour for people that have more than 1)
| @@ -108,6 +109,13 @@ func (e *SSHKey) Run(c *fi.Context) error { | |||
| } | |||
| glog.V(2).Infof("Computed SSH key fingerprint as %q", keyFingerprint) | |||
| e.KeyFingerprint = &keyFingerprint | |||
| } else if e.PublicKey == nil && c.Cluster.Spec.SSHKeyName != "" { | |||
There was a problem hiding this comment.
So in the tasks layer, we try not to refer back to the Cluster Spec; it's sort of crossing the layers - you might think of the tasks layer as being basically terraform. We probably should rename the Name field to be SSHKeyName in the task, but I think for the logic here you just want to check else if e.PublicKey == nil && e.Name != ""
| return err | ||
| } | ||
|
|
||
| e.KeyFingerprint = a.KeyFingerprint |
There was a problem hiding this comment.
I think in this case you need to do:
if a == nil {
return fmt.Errorf("unable to find specified SSH key %q", e.Name)
}
| @@ -176,6 +187,9 @@ func (_ *SSHKey) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *SS | |||
| } | |||
|
|
|||
| func (e *SSHKey) TerraformLink() *terraform.Literal { | |||
| if e.KeyFingerprint != nil && e.PublicKey == nil { | |||
There was a problem hiding this comment.
Does it suffice to just do if e.PublicKey == nil { here?
One thing that might make it clearer is to explicitly have a method func (e *SSHKey) IsExistingKey() bool { return e.PublicKey == nil } or maybe even a field which you would set in the model.
|
Thanks @rralcala - a few suggestions as to the flow of the tasks, but I think we should get this in. I presume you are setting this in a yaml file and doing |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Updated, Thank you @justinsb ! I can also add it to set_cluster |
you don't need to add an ssh public key, this change allows that combination to work on aws. Basically, if a key name is set on the spec and there's no admin key file, the key name will be used and the key will not be managed in terraform.
|
/retest |
|
Thanks @rralcala - this is a nice feature! /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, rralcala 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 |
k8s-ci-robot
added
the
approved
…ck-of-#6886-upstream-release-1.12 Automated Cherry Pick of #6886
…86-origin-release-1.13 Automated cherry pick of #6886: Based on the Readme, if you specify a SSHKeyName in the
…86-origin-release-1.14 Automated cherry pick of #6886: Based on the Readme, if you specify a SSHKeyName in the
Based on the Readme, if you specify a SSHKeyName in the cluster.spec, you don't need to add an ssh public key, this change allows that
combination to work on aws.
Basically, if a key name is set on the spec and there's no admin key
file, the key name will be used and the key will not be managed in
terraform.
I'd like to be able to use my own key in aws and not have it managed, I've seen similar issues created:
#4728
#3693