Cluster-api-provider-aws: Option to launch instances without an SSH key pair

[andrewmyhre]

/kind feature

Describe the solution you'd like
Our organization handles SSH authentication in a specific way, I'm not familiar with the internals and it ties in to our SSO / MFA and I believe it's completely proprietary, but the TL;DR is our policy is to not use SSH keys and instead execute a process during boot to enable SSH. As such we need the option to launch instances without using an SSH key pair.

Environment:

• Cluster-api-provider-aws version: v0.4.0
• Kubernetes version: (use kubectl version): v1.15.3
• OS (e.g. from /etc/os-release): ubuntu 16.04

[vincepri]

You should be able to specify an empty string value for sshKeyName https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/api/v1alpha2/types.go#L493 to achieve the suggested behavior

[vincepri]

/priority awaiting-more-evidence

[andrewmyhre]

Here's the relevant code:

if scope.AWSMachine.Spec.SSHKeyName != "" {
        input.SSHKeyName = aws.String(scope.AWSMachine.Spec.SSHKeyName)
    } else {
        input.SSHKeyName = aws.String(defaultSSHKeyName)
    }

Providing sshKeyName: "" on the AWSMachine specs causes the controller to try to use the default SSH key pair name which is default, which doesn't exist in our accounts (and we don't want to use key pairs at all).

[andrewmyhre]

Here's the key pair selection code for bastions:

    keyName := defaultSSHKeyName
    if s.scope.AWSCluster.Spec.SSHKeyName != "" {
        keyName = s.scope.AWSCluster.Spec.SSHKeyName
    }

Results in the same behavior.. there is no code path allowing for 'no key pair'.

[andrewmyhre]

I'm thinking something like this (in instances.go):

    if !scope.AWSMachine.Spec.NoSSHKeyPair {
        // Pick SSH key, if any.
    if scope.AWSMachine.Spec.SSHKeyName != "" {
        input.SSHKeyName = aws.String(scope.AWSMachine.Spec.SSHKeyName)
    } else {
        input.SSHKeyName = aws.String(defaultSSHKeyName)
    }
    }

But it's not great is it.

[detiber]

It sounds like if we want to support "", then we likely will need to turn SSHKeyName into a pointer to know when we'd want to set it to the default vs not.

[andrewmyhre]

If that's the case, making nil = defaultKeyPair and "" = no key pair might make sense, because it requires a user to explicitly specify 'no key pair', and I believe the idea is to default to using a key pair here.

[detiber]

One potential gotcha with updating to a pointer is that it will definitely be a breaking API change and we'll need to take care during the upgrade (ideally with a webhook-based migration) to update the current empty value of "" to nil during the migration.

[andrewmyhre]

That's a great call-out.. I haven't looked at the conversion code very closely yet but I guess I should amend that to handle this?
Also I found a small issue with my logic last night which I need to rectify, and I want to write some tests around it as well. So not ready to merge just yet.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen

[vincepri]

/help

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.