🐛 avoid in-place capbk mutation, duplicate files

[alexeldeib]

What this PR does / why we need it:

we write the certificate files twice into cloud init, once as writeFiles and again as additionalFiles which get written back to writeFiles.

we also mutate the kubeadm config spec in place when converting contentFrom -> content. avoid doing that.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

/hold

for some tests

[CecileRobertMichon]

is this unit tested?

[CecileRobertMichon]

edit: just saw you have a hold for tests, nvm

[fabriziopandini]

Good catch!
Wondering if we can remove the merge parameter from resolveFiles, since it seems it is never used now.

Additional question, from a quick glance at this feauture, it seems to me that the AdditionalFiles are never added to cloud-init for a joining control plane node. see

cluster-api/bootstrap/kubeadm/internal/cloudinit/controlplane_init.go

Lines 58 to 59 in d910413

	input.WriteFiles = input.Certificates.AsFiles()
	input.WriteFiles = append(input.WriteFiles, input.AdditionalFiles...)

vs

cluster-api/bootstrap/kubeadm/internal/cloudinit/controlplane_join.go

Line 55 in d910413

input.WriteFiles = input.Certificates.AsFiles()

Is this intentional or another potential bug hidden by the duplication of files?

[alexeldeib]

I think for the join case, we take

cluster-api/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

Line 531 in 2afc70d

AdditionalFiles: files,

and plumb it through to

cluster-api/bootstrap/kubeadm/internal/cloudinit/controlplane_join.go

Lines 57 to 59 in 2afc70d

	if err := input.prepare(); err != nil {
	return nil, err
	}

which handles the same logic for us:

cluster-api/bootstrap/kubeadm/internal/cloudinit/cloudinit.go

Line 57 in 2afc70d

input.WriteFiles = append(input.WriteFiles, input.AdditionalFiles...)

will see if I can align these, or if there's a reason not to 👍

[alexeldeib]

I guess prepare() only really makes sense for join, since this code isn't helpful for init:

cluster-api/bootstrap/kubeadm/internal/cloudinit/cloudinit.go

Lines 58 to 66 in 2afc70d

	input.KubeadmCommand = fmt.Sprintf(standardJoinCommand, input.KubeadmVerbosity)
	if input.UseExperimentalRetry {
	input.KubeadmCommand = retriableJoinScriptName
	joinScriptFile, err := generateBootstrapScript(input)
	if err != nil {
	return errors.Wrap(err, "failed to generate user data for machine joining control plane")
	}
	input.WriteFiles = append(input.WriteFiles, *joinScriptFile)
	}

[fabriziopandini]

@alexeldeib thanks for investigating this.
I think we are ok with prepare for join (eventually fixing this is out of scope of this PR)

[vincepri]

Suggested change

	collected := make([]bootstrapv1.File, len(cfg.Spec.Files)+len(merge))
	collected := make([]bootstrapv1.File, 0, len(cfg.Spec.Files)+len(merge))

Then we don't need index, but we can just append()

[alexeldeib]

not actually merging anything anymore, so got rid of this altogether and switched to append

[vincepri]

/milestone v0.3.9

[alexeldeib]

@CecileRobertMichon added tests

[alexeldeib]

/hold cancel

[CecileRobertMichon]

was about to hit enter on a comment asking exactly for this :D

[CecileRobertMichon]

/lgtm

[CecileRobertMichon]

/lgtm

[vincepri]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

• bootstrap/kubeadm/OWNERS [vincepri]

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