🐛 KCP should bypass the controller cache when initializing

Signed-off-by: Vince Prignano <[email protected]>
kubernetes-sigs · May 4, 2020 · 26f5c70 · 26f5c70
1 parent 60eae74
commit 26f5c70
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 15 deletions.
diff --git a/controllers/remote/cluster.go b/controllers/remote/cluster.go
@@ -40,12 +40,11 @@ func NewClusterClient(ctx context.Context, c client.Client, cluster client.Objec
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to create client for Cluster %s/%s", cluster.Namespace, cluster.Name)
 	}
-
 	return ret, nil
 }
 
 // RESTConfig returns a configuration instance to be used with a Kubernetes client.
-func RESTConfig(ctx context.Context, c client.Client, cluster client.ObjectKey) (*restclient.Config, error) {
+func RESTConfig(ctx context.Context, c client.Reader, cluster client.ObjectKey) (*restclient.Config, error) {
 	kubeConfig, err := kcfg.FromSecret(ctx, c, cluster)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to retrieve kubeconfig secret for Cluster %s/%s", cluster.Namespace, cluster.Name)

diff --git a/controlplane/kubeadm/controllers/controller.go b/controlplane/kubeadm/controllers/controller.go
@@ -65,7 +65,8 @@ type KubeadmControlPlaneReconciler struct {
 	controller controller.Controller
 	recorder   record.EventRecorder
 
-	managementCluster internal.ManagementCluster
+	managementCluster         internal.ManagementCluster
+	managementClusterUncached internal.ManagementCluster
 }
 
 func (r *KubeadmControlPlaneReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
@@ -97,6 +98,9 @@ func (r *KubeadmControlPlaneReconciler) SetupWithManager(mgr ctrl.Manager, optio
 	if r.managementCluster == nil {
 		r.managementCluster = &internal.Management{Client: r.Client}
 	}
+	if r.managementClusterUncached == nil {
+		r.managementClusterUncached = &internal.Management{Client: mgr.GetAPIReader()}
+	}
 
 	return nil
 }

diff --git a/controlplane/kubeadm/controllers/controller_test.go b/controlplane/kubeadm/controllers/controller_test.go
@@ -525,6 +525,15 @@ kubernetesVersion: metav1.16.1`,
 				Status: internal.ClusterStatus{},
 			},
 		},
+		managementClusterUncached: &fakeManagementCluster{
+			Management: &internal.Management{Client: fakeClient},
+			Workload: fakeWorkloadCluster{
+				Workload: &internal.Workload{
+					Client: fakeClient,
+				},
+				Status: internal.ClusterStatus{},
+			},
+		},
 	}
 
 	result, err := r.Reconcile(ctrl.Request{NamespacedName: util.ObjectKey(kcp)})

diff --git a/controlplane/kubeadm/controllers/scale.go b/controlplane/kubeadm/controllers/scale.go
@@ -25,6 +25,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
+	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/machinefilters"
 	capierrors "sigs.k8s.io/cluster-api/errors"
 	"sigs.k8s.io/cluster-api/util"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -33,6 +34,20 @@ import (
 func (r *KubeadmControlPlaneReconciler) initializeControlPlane(ctx context.Context, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane, controlPlane *internal.ControlPlane) (ctrl.Result, error) {
 	logger := controlPlane.Logger()
 
+	// Perform an uncached read of all the owned machines. This check is in place to make sure
+	// that the controller cache is not misbehaving and we end up initializing the cluster more than once.
+	ownedMachines, err := r.managementClusterUncached.GetMachinesForCluster(ctx, util.ObjectKey(cluster), machinefilters.OwnedControlPlaneMachines(kcp.Name))
+	if err != nil {
+		logger.Error(err, "failed to perform an uncached read of control plane machines for cluster")
+		return ctrl.Result{}, err
+	}
+	if len(ownedMachines) > 0 {
+		return ctrl.Result{}, errors.Errorf(
+			"control plane has already been initialized, found %d owned machine for cluster %s/%s: controller cache or management cluster is misbehaving",
+			len(ownedMachines), cluster.Namespace, cluster.Name,
+		)
+	}
+
 	bootstrapSpec := controlPlane.InitialControlPlaneConfig()
 	fd := controlPlane.FailureDomainWithFewestMachines()
 	if err := r.cloneConfigsAndGenerateMachine(ctx, cluster, kcp, bootstrapSpec, fd); err != nil {

diff --git a/controlplane/kubeadm/controllers/scale_test.go b/controlplane/kubeadm/controllers/scale_test.go
@@ -50,6 +50,10 @@ func TestKubeadmControlPlaneReconciler_initializeControlPlane(t *testing.T) {
 		Client:   fakeClient,
 		Log:      log.Log,
 		recorder: record.NewFakeRecorder(32),
+		managementClusterUncached: &fakeManagementCluster{
+			Management: &internal.Management{Client: fakeClient},
+			Workload:   fakeWorkloadCluster{},
+		},
 	}
 	controlPlane := &internal.ControlPlane{
 		Cluster: cluster,
@@ -101,10 +105,11 @@ func TestKubeadmControlPlaneReconciler_scaleUpControlPlane(t *testing.T) {
 		fakeClient := newFakeClient(g, initObjs...)
 
 		r := &KubeadmControlPlaneReconciler{
-			Client:            fakeClient,
-			managementCluster: fmc,
-			Log:               log.Log,
-			recorder:          record.NewFakeRecorder(32),
+			Client:                    fakeClient,
+			managementCluster:         fmc,
+			managementClusterUncached: fmc,
+			Log:                       log.Log,
+			recorder:                  record.NewFakeRecorder(32),
 		}
 		controlPlane := &internal.ControlPlane{
 			KCP:      kcp,
@@ -156,10 +161,11 @@ func TestKubeadmControlPlaneReconciler_scaleUpControlPlane(t *testing.T) {
 			}
 
 			r := &KubeadmControlPlaneReconciler{
-				Client:            fakeClient,
-				managementCluster: fmc,
-				Log:               log.Log,
-				recorder:          record.NewFakeRecorder(32),
+				Client:                    fakeClient,
+				managementCluster:         fmc,
+				managementClusterUncached: fmc,
+				Log:                       log.Log,
+				recorder:                  record.NewFakeRecorder(32),
 			}
 			controlPlane := &internal.ControlPlane{
 				KCP:      kcp,

diff --git a/controlplane/kubeadm/controllers/upgrade_test.go b/controlplane/kubeadm/controllers/upgrade_test.go
@@ -53,6 +53,12 @@ func TestKubeadmControlPlaneReconciler_upgradeControlPlane(t *testing.T) {
 			ControlPlaneHealthy: true,
 			EtcdHealthy:         true,
 		},
+		managementClusterUncached: &fakeManagementCluster{
+			Management:          &internal.Management{Client: fakeClient},
+			Workload:            fakeWorkloadCluster{Status: internal.ClusterStatus{Nodes: 1}},
+			ControlPlaneHealthy: true,
+			EtcdHealthy:         true,
+		},
 	}
 	controlPlane := &internal.ControlPlane{
 		KCP:      kcp,

diff --git a/controlplane/kubeadm/internal/cluster.go b/controlplane/kubeadm/internal/cluster.go
@@ -46,7 +46,7 @@ type ManagementCluster interface {
 
 // Management holds operations on the management cluster.
 type Management struct {
-	Client ctrlclient.Client
+	Client ctrlclient.Reader
 }
 
 // GetMachinesForCluster returns a list of machines that can be filtered or not.

diff --git a/util/kubeconfig/kubeconfig.go b/util/kubeconfig/kubeconfig.go
@@ -40,7 +40,7 @@ var (
 )
 
 // FromSecret fetches the Kubeconfig for a Cluster.
-func FromSecret(ctx context.Context, c client.Client, cluster client.ObjectKey) ([]byte, error) {
+func FromSecret(ctx context.Context, c client.Reader, cluster client.ObjectKey) ([]byte, error) {
 	out, err := secret.Get(ctx, c, cluster, secret.Kubeconfig)
 	if err != nil {
 		return nil, err

diff --git a/util/secret/secret.go b/util/secret/secret.go
@@ -28,13 +28,13 @@ import (
 
 // Get retrieves the specified Secret (if any) from the given
 // cluster name and namespace.
-func Get(ctx context.Context, c client.Client, cluster client.ObjectKey, purpose Purpose) (*corev1.Secret, error) {
+func Get(ctx context.Context, c client.Reader, cluster client.ObjectKey, purpose Purpose) (*corev1.Secret, error) {
 	return GetFromNamespacedName(ctx, c, cluster, purpose)
 }
 
 // GetFromNamespacedName retrieves the specified Secret (if any) from the given
 // cluster name and namespace.
-func GetFromNamespacedName(ctx context.Context, c client.Client, clusterName client.ObjectKey, purpose Purpose) (*corev1.Secret, error) {
+func GetFromNamespacedName(ctx context.Context, c client.Reader, clusterName client.ObjectKey, purpose Purpose) (*corev1.Secret, error) {
 	secret := &corev1.Secret{}
 	secretKey := client.ObjectKey{
 		Namespace: clusterName.Namespace,