Skip to content

Commit

Permalink
Merge pull request #4195 from cnmcavoy/cnmcavoy/eksconfig-reconciliation
Browse files Browse the repository at this point in the history 
Reconcile EKSConfig correctly for MachinePool and other Owner kinds
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Apr 5, 2023
2 parents fd5e11f + 589b726 commit ccb1020
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 21 deletions.
32 changes: 20 additions & 12 deletions bootstrap/eks/controllers/eksconfig_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controllers
import (
"bytes"
"context"
"time"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
112 changes: 105 additions & 7 deletions bootstrap/eks/controllers/eksconfig_controller_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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.
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand All @@ -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
}
Expand Down
14 changes: 12 additions & 2 deletions bootstrap/eks/controllers/eksconfig_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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())
Expand All @@ -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
}

0 comments on commit ccb1020

Please sign in to comment.