Cluster-api: Audit KCP test coverage

[fabriziopandini]

[originally this was proposed as https://github.com/kubernetes-sigs/cluster-api/issues/3287, but the discussion took a different direction there 😜]

As already done at the beginning of v0.3.x series, we should re-audit test coverage for KCP and ensure that it is adequate for the complexity of this critical component

@wfernandes If I'm not wrong you make last analysis. Could you kindly link the results to this issue?

[vincepri]

We should probably audit tests throughout the codebase, including infrastructure providers. That's a much larger effort though.

[wfernandes]

Hmm...I don't remember if I did the KCP audit. @gab-satchi were you involved with this?
I remember we had a github issue and a public spreadsheet that tracked all of the tests required. I'll link them if I can find them.

[vincepri]

We might be talking about #2753

[fabriziopandini]

This is a gist with the current test coverage results; We should take a look at it and derive some action items.
Also https://github.com/kubernetes-sigs/cluster-api/pull/3310 will make this easier to repeat in future

[vincepri]

/milestone v0.3.x

[fabriziopandini]

Some actions for KCP (this can be address in multiple PRs as well)

• reconcileKubeconfig is missing a test on the client certificate renewal feature https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/helpers.go#L77-L87
• cloneConfigsAndGenerateMachine is missing a test on cleanup in case of errors https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/helpers.go#L160-L162
• upgradeControlPlane is missing a test for upgrading image repository/etcd version in the kubeadm config https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/upgrade.go#L72-L75 and https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/upgrade.go#L79-L82
• GetWorkloadCluster is not tested
• MatchesTemplateClonedFrom is not tested
• MatchesKubernetesVersion is not tested
• UpdateCoreDNS is not tested
• ReconcileEtcdMembers is not tested

/help

[k8s-ci-robot]

@fabriziopandini:
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:

Some actions for KCP (this can be address in multiple PRs as well)

• reconcileKubeconfig is missing a test on the client certificate renewal feature https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/helpers.go#L77-L87
• cloneConfigsAndGenerateMachine is missing a test on cleanup in case of errors https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/helpers.go#L160-L162
• upgradeControlPlane is missing a test for upgrading image repository/etcd version in the kubeadm config https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/upgrade.go#L72-L75 and https://github.com/kubernetes-sigs/cluster-api/blob/4e07b0c23f6782fa5ca3729c3f9ed005a6f9a203/controlplane/kubeadm/controllers/upgrade.go#L79-L82
• GetWorkloadCluster is not tested
• MatchesTemplateClonedFrom is not tested
• MatchesKubernetesVersion is not tested
• UpdateCoreDNS is not tested
• ReconcileEtcdMembers is not tested

/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.

[wfernandes]

I was looking into writing a test for cloneConfigsAndGenerateMachine. The errors to be returned are primarily from fakeClient.Create which is hard to setup. Is there some way to let fakeClient return an error?

[wfernandes]

I'll work on tests for UpdateCoreDNS.
/assign

[wfernandes]

Moving on to machine filters - MatchesKubernetesVersion, MatchesTemplateClonedFrom

[wfernandes]

Moving on to GetWorkloadCluster

[wfernandes]

So writing a unit test for GetWorkloadCluster for KCP is not as simple since the code for connecting and creating a client is within the GetWorkloadCluster method.
https://github.com/kubernetes-sigs/cluster-api/blob/dfe7022e94b8f61eb27a9606e270e09c84d29767/controlplane/kubeadm/internal/cluster.go#L94-L103

There are a couple of options:

1. We use envtest to create an environment and use it's restConfig for client.New. But I'm not sure if envtest would be appropriate for "unit" tests.
2. We refactor the Management struct and inject behavior to get the workload client using an options pattern; something like WithWorkloadClient. But that could mean a substantial refactor/changes throughout.

I'm leaning towards the second one. Any thoughts?

UPDATE: So I noticed https://github.com/kubernetes-sigs/cluster-api/pull/3349 discussing standardizing our tests towards envtest. I'll try and use it and see how it goes.

[wfernandes]

/lifecycle active

[wfernandes]

Note to self.
Started work on testing ReconcileEtcdMembers. I created a branch wfernandes:add-test-reconcile-etcd-members from the wfernandes:add-get-workload-cluster-test branch since I want to build upon the envtest work.

[vincepri]

/milestone v0.3.9

[vincepri]

Can we close this in favor of more specific issues? I'm not sure if all the PRs will make it in v0.3.9

[wfernandes]

I'm fine with closing this issue in favor of more specific issues.

Regarding the PRs that might not make it in v0.3.9, are you referring to PRs #3408 and #3350?

[vincepri]

/close

Thanks @wfernandes! Yes, I was referring to those PRs, let's open more specific ones and avoid having umbrella-like issues

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

/close

Thanks @wfernandes! Yes, I was referring to those PRs, let's open more specific ones and avoid having umbrella-like issues

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.