Add capk user properly when users already defined.

[pjaton]

What this PR does / why we need it:

Cluster-API's Kubeadm bootstrap provider exposes the capability to end-user to specify users to add to control or compute plane nodes. When a end-user does so, then the corresponding users section is generated in the cloud-init bootstrap data. This section conflicts with the one that is currently added by the Kubevirt provider to add the capk user that is used to check each node health.

This PR address this conflict by:

• adding the capk user to the users section if one exists already
• if a capk user exists already in this list of users, then, the former is overridden instead
• if there are not users section in the current cloud-init bootstrap userdata, then add the section with the capk user

Which issue this PR fixes:

fixes #149

Special notes for your reviewer:

Marshaling and unmarshaling yaml can be tricky as comment can easily be lost, ordering modified, etc. To avoid this, this changes relies on Golang yaml.Node to preserve as much of the original format as possible. The only change is on the indentation that might be streamlined (CAPI templates seem to produce inconsistent indentation between different section).

Ignoring the indentation differences, I validated that the difference between the cloud-init generated by the Kubeadm bootstrap provider and the one enhanced by CAPK is only the desired addition of the capk user.

By example, when no user is defined by the end-user:

239a240,246
> users:
>   - name: capk
>     gecos: CAPK User
>     sudo: ALL=(ALL) NOPASSWD:ALL
>     groups: users, admin
>     ssh_authorized_keys:
>       - ecdsa-sha2-nistp384 AAAA...

Release notes:

NONE

[davidvossel]

/ok-to-test

[davidvossel]

After looking into this a bit more, I question if we should be doing this cloud-init manipulation at all.

I think the kubeadm config is the place this should be occurring, https://capz.sigs.k8s.io/topics/ssh-access.html#provisioning-ssh-keys-using-machine-templates

It feels a bit like we're fighting against the kubeadm bootstrapper here if people can define their own users and cloudinit configuration in the bootstrapper config.

[davidvossel]

It feels a bit like we're fighting against the kubeadm bootstrapper here if people can define their own users and cloudinit configuration in the bootstrapper config.

The alternative here would be that we add something to our templates to inject the capk user and ssh keys.

[coveralls]

Pull Request Test Coverage Report for Build 2733450232

• 64 of 76 (84.21%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+2.09%) to 48.383%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/kubevirtmachine_controller.go	64	76	84.21%

Totals
Change from base Build 2732899781:	2.09%
Covered Lines:	778
Relevant Lines:	1608

---

💛 - Coveralls

[agradouski]

@davidvossel agree, it is indeed a hack from the start that only worked until we need to add another user for a tenant cluster via kubeadm config, which is the right way to do it ... however, the issue is that currently we also generate ssh keys dynamically when bootstrapping the cluster. we would also need to include a private key for capk user as part of the template, and I'm not sure if that would be a security issue, as users own tenant cluster configs, and capk user is used for cluster management by cluster-api, even though users technically have a role of cluster admin

so, we can delegate ssh key management for capk user to users, and require ssh keys in our template, as you suggest

or, another alternative can be that we include capk user in kubeadm config (and make it required), without ssh keys, but then still manipulate capk user in machine controller reconcile loop, replacing the ssh key in user config with the dynamically generated one

[pjaton]

@davidvossel , @agradouski, I agree that managing the cloud-init user-data should, as much as possible, remains the responsibility of Kubeadm bootstrapper. However, until we can update the logic to assess a node health so that it doesn't require SSH access, I feel that, from a user experience point-of-view, it better to have this provider generate the SSH keys pair and add the user.

It would feel odd to me as a end-user to have to specify a required user when I do not need it for my own use.

If we replace the logic and CAPK no longer requires this user, then keeping this logic encapsulated would also make the upgrade more transparent.

[pjaton]

As discussed offline with @agradouski and @davidvossel , opened #163 to keep track of the long term fix.

[pjaton]

@davidvossel , @agradouski , knowing that we now have #163 to properly address how CAPK checks nodes health, do you agree to move forward with this change to, at least, improve on the current SSH approach?

[agradouski]

@pjaton agree
/lgtm

[davidvossel]

/approve

long term we want to move away from this. But your change here is no worse than what currently exists

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, pjaton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [davidvossel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment