Reconcile EKSConfig correctly for MachinePool and other Owner kinds

bootstrap/eks/controllers/eksconfig_controller.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"bytes"
 	"context"
+	"time"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -74,33 +75,36 @@ func (r *EKSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		log.Error(err, "Failed to get config")
 		return ctrl.Result{}, err
 	}
+	log = log.WithValues("EKSConfig", config.GetName())
 
 	// check owner references and look up owning Machine object
 	configOwner, err := bsutil.GetConfigOwner(ctx, r.Client, config)
 	if apierrors.IsNotFound(err) {
 		// no error here, requeue until we find an owner
-		return ctrl.Result{}, nil
+		log.Debug("eksconfig failed to look up owner reference, re-queueing")
+		return ctrl.Result{RequeueAfter: time.Minute}, nil
 	}
 	if err != nil {
-		log.Error(err, "Failed to get owner")
+		log.Error(err, "eksconfig failed to get owner")
 		return ctrl.Result{}, err
 	}
 	if configOwner == nil {
 		// no error, requeue until we find an owner
-		return ctrl.Result{}, nil
+		log.Debug("eksconfig has no owner reference set, re-queueing")
+		return ctrl.Result{RequeueAfter: time.Minute}, nil
 	}
 
 	log = log.WithValues(configOwner.GetKind(), configOwner.GetName())
 
 	cluster, err := util.GetClusterByName(ctx, r.Client, configOwner.GetNamespace(), configOwner.ClusterName())
 	if err != nil {
 		if errors.Is(err, util.ErrNoCluster) {
-			log.Info("EKSConfig does not belong to a cluster yet, re-queuing until it's partof a cluster")
-			return ctrl.Result{}, nil
+			log.Info("EKSConfig does not belong to a cluster yet, re-queuing until it's part of a cluster")
+			return ctrl.Result{RequeueAfter: time.Minute}, nil
 		}
 		if apierrors.IsNotFound(err) {
 			log.Info("Cluster does not exist yet, re-queueing until it is created")
-			return ctrl.Result{}, nil
+			return ctrl.Result{RequeueAfter: time.Minute}, nil
 		}
 		log.Error(err, "Could not get cluster with metadata")
 		return ctrl.Result{}, err
@@ -138,13 +142,14 @@ func (r *EKSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		}
 	}()
 
-	return r.joinWorker(ctx, cluster, config)
+	return r.joinWorker(ctx, cluster, config, configOwner)
 }
 
-func (r *EKSConfigReconciler) joinWorker(ctx context.Context, cluster *clusterv1.Cluster, config *eksbootstrapv1.EKSConfig) (ctrl.Result, error) {
+func (r *EKSConfigReconciler) joinWorker(ctx context.Context, cluster *clusterv1.Cluster, config *eksbootstrapv1.EKSConfig, configOwner *bsutil.ConfigOwner) (ctrl.Result, error) {
 	log := logger.FromContext(ctx)
 
-	if config.Status.DataSecretName != nil {
+	// only need to reconcile the secret for Machine kinds once, but MachinePools need updates for new launch templates
+	if config.Status.DataSecretName != nil && configOwner.GetKind() == "Machine" {
 		secretKey := client.ObjectKey{Namespace: config.Namespace, Name: *config.Status.DataSecretName}
 		log = log.WithValues("data-secret-name", secretKey.Name)
 		existingSecret := &corev1.Secret{}
@@ -289,7 +294,7 @@ func (r *EKSConfigReconciler) storeBootstrapData(ctx context.Context, cluster *c
 		Namespace: config.Namespace,
 	}, secret); err != nil {
 		if apierrors.IsNotFound(err) {
-			if err := r.createBootstrapSecret(ctx, cluster, config, data); err != nil {
+			if secret, err = r.createBootstrapSecret(ctx, cluster, config, data); err != nil {
 				return errors.Wrap(err, "failed to create bootstrap data secret for EKSConfig")
 			}
 			log.Info("created bootstrap data secret for EKSConfig", "secret", klog.KObj(secret))
@@ -382,7 +387,7 @@ func (r *EKSConfigReconciler) ClusterToEKSConfigs(o client.Object) []ctrl.Reques
 }
 
 // Create the Secret containing bootstrap userdata.
-func (r *EKSConfigReconciler) createBootstrapSecret(ctx context.Context, cluster *clusterv1.Cluster, config *eksbootstrapv1.EKSConfig, data []byte) error {
+func (r *EKSConfigReconciler) createBootstrapSecret(ctx context.Context, cluster *clusterv1.Cluster, config *eksbootstrapv1.EKSConfig, data []byte) (*corev1.Secret, error) {
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      config.Name,
@@ -405,11 +410,14 @@ func (r *EKSConfigReconciler) createBootstrapSecret(ctx context.Context, cluster
 		},
 		Type: clusterv1.ClusterSecretType,
 	}
-	return r.Client.Create(ctx, secret)
+	return secret, r.Client.Create(ctx, secret)
 }
 
 // Update the userdata in the bootstrap Secret.
 func (r *EKSConfigReconciler) updateBootstrapSecret(ctx context.Context, secret *corev1.Secret, data []byte) (bool, error) {
+	if secret.Data == nil {
+		secret.Data = make(map[string][]byte)
+	}
 	if !bytes.Equal(secret.Data["value"], data) {
 		secret.Data["value"] = data
 		return true, r.Client.Update(ctx, secret)

bootstrap/eks/controllers/eksconfig_controller_reconciler_test.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-aws/v2/bootstrap/eks/internal/userdata"
 	ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 )
@@ -55,7 +56,7 @@ func TestEKSConfigReconciler(t *testing.T) {
 		}
 		t.Logf(fmt.Sprintf("Calling reconcile on cluster '%s' and config '%s' should requeue", cluster.Name, config.Name))
 		g.Eventually(func(gomega Gomega) {
-			result, err := reconciler.joinWorker(ctx, cluster, config)
+			result, err := reconciler.joinWorker(ctx, cluster, config, configOwner("Machine"))
 			gomega.Expect(err).NotTo(HaveOccurred())
 			gomega.Expect(result.Requeue).To(BeFalse())
 		}).Should(Succeed())
@@ -74,16 +75,26 @@ func TestEKSConfigReconciler(t *testing.T) {
 
 		g.Expect(string(secret.Data["value"])).To(Equal(string(expectedUserData)))
 	})
-
 	t.Run("Should reconcile an EKSConfig and update data Secret", func(t *testing.T) {
 		g := NewWithT(t)
 		amcp := newAMCP("test-cluster")
 		cluster := newCluster(amcp.Name)
-		machine := newMachine(cluster, "test-machine")
-		config := newEKSConfig(machine)
+		mp := newMachinePool(cluster, "test-machine")
+		config := newEKSConfig(nil)
+		config.ObjectMeta.Name = mp.Name
+		config.ObjectMeta.UID = types.UID(fmt.Sprintf("%s uid", mp.Name))
+		config.ObjectMeta.OwnerReferences = []metav1.OwnerReference{
+			{
+				Kind:       "MachinePool",
+				APIVersion: v1beta1.GroupVersion.String(),
+				Name:       mp.Name,
+				UID:        types.UID(fmt.Sprintf("%s uid", mp.Name)),
+			},
+		}
+		config.Status.DataSecretName = &mp.Name
 		t.Logf(dump("amcp", amcp))
 		t.Logf(dump("config", config))
-		t.Logf(dump("machine", machine))
+		t.Logf(dump("machinepool", mp))
 		t.Logf(dump("cluster", cluster))
 		oldUserData, err := newUserData(cluster.Name, map[string]string{"test-arg": "test-value"})
 		g.Expect(err).To(BeNil())
@@ -100,7 +111,7 @@ func TestEKSConfigReconciler(t *testing.T) {
 		}
 		t.Logf(fmt.Sprintf("Calling reconcile on cluster '%s' and config '%s' should requeue", cluster.Name, config.Name))
 		g.Eventually(func(gomega Gomega) {
-			result, err := reconciler.joinWorker(ctx, cluster, config)
+			result, err := reconciler.joinWorker(ctx, cluster, config, configOwner("MachinePool"))
 			gomega.Expect(err).NotTo(HaveOccurred())
 			gomega.Expect(result.Requeue).To(BeFalse())
 		}).Should(Succeed())
@@ -125,7 +136,7 @@ func TestEKSConfigReconciler(t *testing.T) {
 		}
 		t.Logf(dump("config", config))
 		g.Eventually(func(gomega Gomega) {
-			result, err := reconciler.joinWorker(ctx, cluster, config)
+			result, err := reconciler.joinWorker(ctx, cluster, config, configOwner("MachinePool"))
 			gomega.Expect(err).NotTo(HaveOccurred())
 			gomega.Expect(result.Requeue).To(BeFalse())
 		}).Should(Succeed())
@@ -141,6 +152,57 @@ func TestEKSConfigReconciler(t *testing.T) {
 			gomega.Expect(string(secret.Data["value"])).To(Equal(string(expectedUserData)))
 		}).Should(Succeed())
 	})
+
+	t.Run("Should reconcile an EKSConfig and not update data if secret exists and config owner is Machine kind", func(t *testing.T) {
+		g := NewWithT(t)
+		amcp := newAMCP("test-cluster")
+		cluster := newCluster(amcp.Name)
+		machine := newMachine(cluster, "test-machine")
+		config := newEKSConfig(machine)
+		t.Logf(dump("amcp", amcp))
+		t.Logf(dump("config", config))
+		t.Logf(dump("machine", machine))
+		t.Logf(dump("cluster", cluster))
+		expectedUserData, err := newUserData(cluster.Name, map[string]string{"test-arg": "test-value"})
+		g.Expect(err).To(BeNil())
+		g.Expect(testEnv.Client.Create(ctx, amcp)).To(Succeed())
+
+		secret := &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "default",
+				Name:      machine.Name,
+			},
+		}
+		g.Expect(testEnv.Client.Create(ctx, secret)).To(Succeed())
+
+		amcpList := &ekscontrolplanev1.AWSManagedControlPlaneList{}
+		testEnv.Client.List(ctx, amcpList)
+		t.Logf(dump("stored-amcps", amcpList))
+
+		reconciler := EKSConfigReconciler{
+			Client: testEnv.Client,
+		}
+		t.Logf(fmt.Sprintf("Calling reconcile on cluster '%s' and config '%s' should requeue", cluster.Name, config.Name))
+		g.Eventually(func(gomega Gomega) {
+			result, err := reconciler.joinWorker(ctx, cluster, config, configOwner("Machine"))
+			gomega.Expect(err).NotTo(HaveOccurred())
+			gomega.Expect(result.Requeue).To(BeFalse())
+		}).Should(Succeed())
+
+		t.Logf(fmt.Sprintf("Secret '%s' should exist and be out of date", config.Name))
+		secretList := &corev1.SecretList{}
+		testEnv.Client.List(ctx, secretList)
+		t.Logf(dump("secrets", secretList))
+
+		secret = &corev1.Secret{}
+		g.Eventually(func(gomega Gomega) {
+			gomega.Expect(testEnv.Client.Get(ctx, client.ObjectKey{
+				Name:      config.Name,
+				Namespace: "default",
+			}, secret)).To(Succeed())
+			gomega.Expect(string(secret.Data["value"])).To(Not(Equal(string(expectedUserData))))
+		}).Should(Succeed())
+	})
 }
 
 // newCluster return a CAPI cluster object.
@@ -204,6 +266,40 @@ func newMachine(cluster *clusterv1.Cluster, name string) *clusterv1.Machine {
 	return machine
 }
 
+// newMachinePool returns a CAPI machine object; if cluster is not nil, the MachinePool  is linked to the cluster as well.
+func newMachinePool(cluster *clusterv1.Cluster, name string) *v1beta1.MachinePool {
+	generatedName := fmt.Sprintf("%s-%s", name, util.RandomString(5))
+	mp := &v1beta1.MachinePool{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "MachinePool",
+			APIVersion: v1beta1.GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+			Name:      generatedName,
+		},
+		Spec: v1beta1.MachinePoolSpec{
+			Template: clusterv1.MachineTemplateSpec{
+				Spec: clusterv1.MachineSpec{
+					Bootstrap: clusterv1.Bootstrap{
+						ConfigRef: &corev1.ObjectReference{
+							Kind:       "EKSConfig",
+							APIVersion: eksbootstrapv1.GroupVersion.String(),
+						},
+					},
+				},
+			},
+		},
+	}
+	if cluster != nil {
+		mp.Spec.ClusterName = cluster.Name
+		mp.ObjectMeta.Labels = map[string]string{
+			clusterv1.ClusterLabelName: cluster.Name,
+		}
+	}
+	return mp
+}
+
 // newEKSConfig return an EKSConfig object; if machine is not nil, the EKSConfig is linked to the machine as well.
 func newEKSConfig(machine *clusterv1.Machine) *eksbootstrapv1.EKSConfig {
 	config := &eksbootstrapv1.EKSConfig{
@@ -219,6 +315,7 @@ func newEKSConfig(machine *clusterv1.Machine) *eksbootstrapv1.EKSConfig {
 				"test-arg": "test-value",
 			},
 		},
+		Status: eksbootstrapv1.EKSConfigStatus{},
 	}
 	if machine != nil {
 		config.ObjectMeta.Name = machine.Name
@@ -231,6 +328,7 @@ func newEKSConfig(machine *clusterv1.Machine) *eksbootstrapv1.EKSConfig {
 				UID:        types.UID(fmt.Sprintf("%s uid", machine.Name)),
 			},
 		}
+		config.Status.DataSecretName = &machine.Name
 		machine.Spec.Bootstrap.ConfigRef.Name = config.Name
 		machine.Spec.Bootstrap.ConfigRef.Namespace = config.Namespace
 	}

bootstrap/eks/controllers/eksconfig_controller_test.go

@@ -21,9 +21,11 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	bsutil "sigs.k8s.io/cluster-api/bootstrap/util"
 )
 
 func TestEKSConfigReconcilerReturnEarlyIfClusterInfraNotReady(t *testing.T) {
@@ -42,7 +44,7 @@ func TestEKSConfigReconcilerReturnEarlyIfClusterInfraNotReady(t *testing.T) {
 	}
 
 	g.Eventually(func(gomega Gomega) {
-		result, err := reconciler.joinWorker(context.Background(), cluster, config)
+		result, err := reconciler.joinWorker(context.Background(), cluster, config, configOwner("Machine"))
 		gomega.Expect(result).To(Equal(reconcile.Result{}))
 		gomega.Expect(err).NotTo(HaveOccurred())
 	}).Should(Succeed())
@@ -64,8 +66,16 @@ func TestEKSConfigReconcilerReturnEarlyIfClusterControlPlaneNotInitialized(t *te
 	}
 
 	g.Eventually(func(gomega Gomega) {
-		result, err := reconciler.joinWorker(context.Background(), cluster, config)
+		result, err := reconciler.joinWorker(context.Background(), cluster, config, configOwner("Machine"))
 		gomega.Expect(result).To(Equal(reconcile.Result{}))
 		gomega.Expect(err).NotTo(HaveOccurred())
 	}).Should(Succeed())
 }
+
+func configOwner(kind string) *bsutil.ConfigOwner {
+	unstructuredOwner := unstructured.Unstructured{
+		Object: map[string]interface{}{"kind": kind},
+	}
+	configOwner := bsutil.ConfigOwner{Unstructured: &unstructuredOwner}
+	return &configOwner
+}