Skip to content

Commit

Permalink
Improve KCP to use only one workload cluster for each reconcile
Browse files Browse the repository at this point in the history
  • Loading branch information
@fabriziopandini
fabriziopandini committed Jun 23, 2023
1 parent 3c9e154 commit 479ca19
Show file tree
Hide file tree
Showing 14 changed files with 423 additions and 233 deletions.
31 changes: 30 additions & 1 deletion controlplane/kubeadm/internal/control_plane.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,13 @@ type ControlPlane struct {
// See discussion on https://github.com/kubernetes-sigs/cluster-api/pull/3405
KubeadmConfigs map[string]*bootstrapv1.KubeadmConfig
InfraResources map[string]*unstructured.Unstructured

managementCluster ManagementCluster
workloadCluster WorkloadCluster
}

// NewControlPlane returns an instantiated ControlPlane.
func NewControlPlane(ctx context.Context, client client.Client, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane, ownedMachines collections.Machines) (*ControlPlane, error) {
func NewControlPlane(ctx context.Context, managementCluster ManagementCluster, client client.Client, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane, ownedMachines collections.Machines) (*ControlPlane, error) {
infraObjects, err := getInfraResources(ctx, client, ownedMachines)
if err != nil {
return nil, err
Expand All @@ -80,6 +83,7 @@ func NewControlPlane(ctx context.Context, client client.Client, cluster *cluster
KubeadmConfigs: kubeadmConfigs,
InfraResources: infraObjects,
reconciliationTime: metav1.Now(),
managementCluster: managementCluster,
}, nil
}

Expand Down Expand Up @@ -268,3 +272,28 @@ func (c *ControlPlane) SetPatchHelpers(patchHelpers map[string]*patch.Helper) {
c.machinesPatchHelpers[machineName] = patchHelper
}
}

// GetWorkloadCluster builds a cluster object.
// The cluster comes with an etcd client generator to connect to any etcd pod living on a managed machine.
func (c *ControlPlane) GetWorkloadCluster(ctx context.Context) (WorkloadCluster, error) {
if c.workloadCluster != nil {
return c.workloadCluster, nil
}

workloadCluster, err := c.managementCluster.GetWorkloadCluster(ctx, client.ObjectKeyFromObject(c.Cluster))
if err != nil {
return nil, err
}
c.workloadCluster = workloadCluster
return c.workloadCluster, nil
}

// InjectTestManagementCluster allows to inject a test ManagementCluster during tests.
// NOTE: This approach allows to keep the managementCluster field private, which will
// prevent people from using managementCluster.GetWorkloadCluster because it creates a new
// instance of WorkloadCluster at every call. People instead should use ControlPlane.GetWorkloadCluster
// that creates only a single instance of WorkloadCluster for each reconcile.
func (c *ControlPlane) InjectTestManagementCluster(managementCluster ManagementCluster) {
c.managementCluster = managementCluster
c.workloadCluster = nil
}
221 changes: 130 additions & 91 deletions controlplane/kubeadm/internal/controllers/controller.go
View file

Large diffs are not rendered by default.

95 changes: 71 additions & 24 deletions controlplane/kubeadm/internal/controllers/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,9 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
managementClusterUncached: fmc,
}

g.Expect(r.reconcile(ctx, cluster, kcp)).To(Equal(ctrl.Result{}))
_, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(adoptableMachineFound).To(BeTrue())

machineList := &clusterv1.MachineList{}
g.Expect(fakeClient.List(ctx, machineList, client.InNamespace(cluster.Namespace))).To(Succeed())
Expand Down Expand Up @@ -614,7 +616,9 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
managementClusterUncached: fmc,
}

g.Expect(r.reconcile(ctx, cluster, kcp)).To(Equal(ctrl.Result{}))
_, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(adoptableMachineFound).To(BeTrue())

machineList := &clusterv1.MachineList{}
g.Expect(fakeClient.List(ctx, machineList, client.InNamespace(cluster.Namespace))).To(Succeed())
Expand Down Expand Up @@ -698,10 +702,9 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
managementClusterUncached: fmc,
}

result, err := r.reconcile(ctx, cluster, kcp)
g.Expect(result).To(Equal(ctrl.Result{}))
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("has just been deleted"))
_, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(adoptableMachineFound).To(BeFalse())

machineList := &clusterv1.MachineList{}
g.Expect(fakeClient.List(ctx, machineList, client.InNamespace(cluster.Namespace))).To(Succeed())
Expand All @@ -711,7 +714,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
}
})

t.Run("refuses to adopt Machines that are more than one version old", func(t *testing.T) {
t.Run("Do not adopt Machines that are more than one version old", func(t *testing.T) {
g := NewWithT(t)

cluster, kcp, tmpl := createClusterWithControlPlane(metav1.NamespaceDefault)
Expand Down Expand Up @@ -753,7 +756,10 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
managementClusterUncached: fmc,
}

g.Expect(r.reconcile(ctx, cluster, kcp)).To(Equal(ctrl.Result{}))
_, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(adoptableMachineFound).To(BeTrue())

// Message: Warning AdoptionFailed Could not adopt Machine test/test0: its version ("v1.15.0") is outside supported +/- one minor version skew from KCP's ("v1.17.0")
g.Expect(recorder.Events).To(Receive(ContainSubstring("minor version")))

Expand Down Expand Up @@ -818,7 +824,8 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {

fakeClient := newFakeClient(objs...)

err = ensureCertificatesOwnerRef(ctx, fakeClient, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
r := KubeadmControlPlaneReconciler{Client: fakeClient}
err = r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
g.Expect(err).To(BeNil())

secrets := &corev1.SecretList{}
Expand Down Expand Up @@ -858,7 +865,9 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
}

fakeClient := newFakeClient(objs...)
err := ensureCertificatesOwnerRef(ctx, fakeClient, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)

r := KubeadmControlPlaneReconciler{Client: fakeClient}
err := r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
g.Expect(err).To(BeNil())

secrets := &corev1.SecretList{}
Expand Down Expand Up @@ -901,7 +910,9 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
}

fakeClient := newFakeClient(objs...)
err := ensureCertificatesOwnerRef(ctx, fakeClient, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)

r := KubeadmControlPlaneReconciler{Client: fakeClient}
err := r.ensureCertificatesOwnerRef(ctx, client.ObjectKeyFromObject(cluster), certificates, kcpOwner)
g.Expect(err).To(BeNil())

secrets := &corev1.SecretList{}
Expand Down Expand Up @@ -1088,18 +1099,20 @@ func TestReconcileCertificateExpiries(t *testing.T) {
machineWithoutNodeRefKubeadmConfig,
)

controlPlane, err := internal.NewControlPlane(ctx, fakeClient, cluster, kcp, ownedMachines)
g.Expect(err).ToNot(HaveOccurred())
managementCluster := &fakeManagementCluster{
Workload: fakeWorkloadCluster{
APIServerCertificateExpiry: &detectedExpiry,
},
}

r := &KubeadmControlPlaneReconciler{
Client: fakeClient,
managementCluster: &fakeManagementCluster{
Workload: fakeWorkloadCluster{
APIServerCertificateExpiry: &detectedExpiry,
},
},
Client: fakeClient,
managementCluster: managementCluster,
}

controlPlane, err := internal.NewControlPlane(ctx, managementCluster, fakeClient, cluster, kcp, ownedMachines)
g.Expect(err).ToNot(HaveOccurred())

_, err = r.reconcileCertificateExpiries(ctx, controlPlane)
g.Expect(err).NotTo(HaveOccurred())

Expand Down Expand Up @@ -2014,9 +2027,11 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
controllerutil.AddFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer)
initObjs := []client.Object{cluster.DeepCopy(), kcp.DeepCopy()}

machines := collections.New()
for i := 0; i < 3; i++ {
m, _ := createMachineNodePair(fmt.Sprintf("test-%d", i), cluster, kcp, true)
initObjs = append(initObjs, m)
machines.Insert(m)
}

fakeClient := newFakeClient(initObjs...)
Expand All @@ -2031,7 +2046,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileDelete(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
Machines: machines,
}

result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())
g.Expect(kcp.Finalizers).To(ContainElement(controlplanev1.KubeadmControlPlaneFinalizer))
Expand All @@ -2040,7 +2061,12 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
g.Expect(fakeClient.List(ctx, &controlPlaneMachines)).To(Succeed())
g.Expect(controlPlaneMachines.Items).To(BeEmpty())

result, err = r.reconcileDelete(ctx, cluster, kcp)
controlPlane = &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err = r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{}))
g.Expect(err).NotTo(HaveOccurred())
g.Expect(kcp.Finalizers).To(BeEmpty())
Expand All @@ -2064,9 +2090,11 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {

initObjs := []client.Object{cluster.DeepCopy(), kcp.DeepCopy(), workerMachine.DeepCopy()}

machines := collections.New()
for i := 0; i < 3; i++ {
m, _ := createMachineNodePair(fmt.Sprintf("test-%d", i), cluster, kcp, true)
initObjs = append(initObjs, m)
machines.Insert(m)
}

fakeClient := newFakeClient(initObjs...)
Expand All @@ -2080,7 +2108,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileDelete(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
Machines: machines,
}

result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -2113,9 +2147,11 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {

initObjs := []client.Object{cluster.DeepCopy(), kcp.DeepCopy(), workerMachinePool.DeepCopy()}

machines := collections.New()
for i := 0; i < 3; i++ {
m, _ := createMachineNodePair(fmt.Sprintf("test-%d", i), cluster, kcp, true)
initObjs = append(initObjs, m)
machines.Insert(m)
}

fakeClient := newFakeClient(initObjs...)
Expand All @@ -2129,7 +2165,13 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileDelete(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
Machines: machines,
}

result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: deleteRequeueAfter}))
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -2160,7 +2202,12 @@ func TestKubeadmControlPlaneReconciler_reconcileDelete(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileDelete(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err := r.reconcileDelete(ctx, controlPlane)
g.Expect(result).To(Equal(ctrl.Result{}))
g.Expect(err).NotTo(HaveOccurred())
g.Expect(kcp.Finalizers).To(BeEmpty())
Expand Down
12 changes: 6 additions & 6 deletions controlplane/kubeadm/internal/controllers/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,16 @@ import (
"sigs.k8s.io/cluster-api/util/secret"
)

func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane) (ctrl.Result, error) {
func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context, controlPlane *internal.ControlPlane) (ctrl.Result, error) {
log := ctrl.LoggerFrom(ctx)

endpoint := cluster.Spec.ControlPlaneEndpoint
endpoint := controlPlane.Cluster.Spec.ControlPlaneEndpoint
if endpoint.IsZero() {
return ctrl.Result{}, nil
}

controllerOwnerRef := *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))
clusterName := util.ObjectKey(cluster)
controllerOwnerRef := *metav1.NewControllerRef(controlPlane.KCP, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))
clusterName := util.ObjectKey(controlPlane.Cluster)
configSecret, err := secret.GetFromNamespacedName(ctx, r.Client, clusterName, secret.Kubeconfig)
switch {
case apierrors.IsNotFound(err):
Expand All @@ -76,12 +76,12 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
return ctrl.Result{}, errors.Wrap(err, "failed to retrieve kubeconfig Secret")
}

if err := r.adoptKubeconfigSecret(ctx, configSecret, kcp); err != nil {
if err := r.adoptKubeconfigSecret(ctx, configSecret, controlPlane.KCP); err != nil {
return ctrl.Result{}, err
}

// only do rotation on owned secrets
if !util.IsControlledBy(configSecret, kcp) {
if !util.IsControlledBy(configSecret, controlPlane.KCP) {
return ctrl.Result{}, nil
}

Expand Down
30 changes: 26 additions & 4 deletions controlplane/kubeadm/internal/controllers/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
"sigs.k8s.io/cluster-api/controllers/external"
controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/cluster-api/util/kubeconfig"
"sigs.k8s.io/cluster-api/util/secret"
Expand Down Expand Up @@ -77,7 +78,12 @@ func TestReconcileKubeconfigEmptyAPIEndpoints(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileKubeconfig(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err := r.reconcileKubeconfig(ctx, controlPlane)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(BeZero())

Expand Down Expand Up @@ -126,7 +132,12 @@ func TestReconcileKubeconfigMissingCACertificate(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileKubeconfig(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err := r.reconcileKubeconfig(ctx, controlPlane)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(Equal(ctrl.Result{RequeueAfter: dependentCertRequeueAfter}))

Expand Down Expand Up @@ -192,7 +203,12 @@ func TestReconcileKubeconfigSecretDoesNotAdoptsUserSecrets(t *testing.T) {
recorder: record.NewFakeRecorder(32),
}

result, err := r.reconcileKubeconfig(ctx, cluster, kcp)
controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err := r.reconcileKubeconfig(ctx, controlPlane)
g.Expect(err).To(Succeed())
g.Expect(result).To(BeZero())

Expand Down Expand Up @@ -251,7 +267,13 @@ func TestKubeadmControlPlaneReconciler_reconcileKubeconfig(t *testing.T) {
Client: fakeClient,
recorder: record.NewFakeRecorder(32),
}
result, err := r.reconcileKubeconfig(ctx, cluster, kcp)

controlPlane := &internal.ControlPlane{
KCP: kcp,
Cluster: cluster,
}

result, err := r.reconcileKubeconfig(ctx, controlPlane)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result).To(Equal(ctrl.Result{}))

Expand Down
Loading

0 comments on commit 479ca19

Please sign in to comment.