Skip to content

Commit

Permalink
Reconcile EKSConfig correctly for MachinePool and other Owner kinds
Browse files Browse the repository at this point in the history
  • Loading branch information
@cnmcavoy @Ankitasw
cnmcavoy authored and Ankitasw committed Jun 20, 2023
1 parent df1342c commit a3331a5
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 94 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 @@ -20,6 +20,7 @@ import (
"bytes"
"context"
"fmt"
"time"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -76,33 +77,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 @@ -140,7 +144,7 @@ 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) resolveFiles(ctx context.Context, cfg *eksbootstrapv1.EKSConfig) ([]eksbootstrapv1.File, error) {
Expand Down Expand Up @@ -178,10 +182,11 @@ func (r *EKSConfigReconciler) resolveSecretFileContent(ctx context.Context, ns s
return data, nil
}

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 @@ -332,7 +337,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 @@ -425,7 +430,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 @@ -448,11 +453,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
134 changes: 55 additions & 79 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,7 @@ 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")
Expand Down Expand Up @@ -191,78 +203,6 @@ func TestEKSConfigReconciler(t *testing.T) {
gomega.Expect(string(secret.Data["value"])).To(Not(Equal(string(expectedUserData))))
}).Should(Succeed())
})
t.Run("Should Reconcile an EKSConfig with a secret file reference", func(t *testing.T) {
g := NewWithT(t)
amcp := newAMCP("test-cluster")
//nolint: gosec // these are not credentials
secretPath := "/etc/secret.txt"
secretContent := "secretValue"
cluster := newCluster(amcp.Name)
machine := newMachine(cluster, "test-machine")
config := newEKSConfig(machine)
config.Spec.Files = append(config.Spec.Files, eksbootstrapv1.File{
ContentFrom: &eksbootstrapv1.FileSource{
Secret: eksbootstrapv1.SecretFileSource{
Name: "my-secret",
Key: "secretKey",
},
},
Path: secretPath,
})
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "my-secret",
},
Data: map[string][]byte{
"secretKey": []byte(secretContent),
},
}
t.Logf(dump("amcp", amcp))
t.Logf(dump("config", config))
t.Logf(dump("machine", machine))
t.Logf(dump("cluster", cluster))
t.Logf(dump("secret", secret))
g.Expect(testEnv.Client.Create(ctx, secret)).To(Succeed())
g.Expect(testEnv.Client.Create(ctx, amcp)).To(Succeed())

// create a userData with the secret content and check if reconile.joinWorker
// resolves the userdata properly
expectedUserData, err := userdata.NewNode(&userdata.NodeInput{
ClusterName: amcp.Name,
Files: []eksbootstrapv1.File{
{
Content: secretContent,
Path: secretPath,
},
},
KubeletExtraArgs: map[string]string{
"test-arg": "test-value",
},
})
g.Expect(err).To(BeNil())
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())

secretList := &corev1.SecretList{}
testEnv.Client.List(ctx, secretList)
t.Logf(dump("secrets", secretList))
gotSecret := &corev1.Secret{}
g.Eventually(func(gomega Gomega) {
gomega.Expect(testEnv.Client.Get(ctx, client.ObjectKey{
Name: config.Name,
Namespace: "default",
}, gotSecret)).To(Succeed())
}).Should(Succeed())
g.Expect(string(gotSecret.Data["value"])).To(Equal(string(expectedUserData)))
})
}

// newCluster return a CAPI cluster object.
Expand Down Expand Up @@ -326,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 @@ -341,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 @@ -353,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 TestEKSConfigReconciler_ReturnEarlyIfClusterInfraNotReady(t *testing.T) {
Expand All @@ -42,7 +44,7 @@ func TestEKSConfigReconciler_ReturnEarlyIfClusterInfraNotReady(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 TestEKSConfigReconciler_ReturnEarlyIfClusterControlPlaneNotInitialized(t *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())
}

func configOwner(kind string) *bsutil.ConfigOwner {
unstructuredOwner := unstructured.Unstructured{
Object: map[string]interface{}{"kind": kind},
}
configOwner := bsutil.ConfigOwner{Unstructured: &unstructuredOwner}
return &configOwner
}
1 change: 0 additions & 1 deletion pkg/logger/logger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ limitations under the License.
*/

// Package logger
//nolint: logrlint
package logger

import (
Expand Down
1 change: 1 addition & 0 deletions test/e2e/shared/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ func newBootstrapTemplate(e2eCtx *E2EContext) *cfn_bootstrap.Template {
"elasticfilesystem:DescribeAccessPoints",
"elasticfilesystem:DescribeFileSystems",
"elasticfilesystem:CreateAccessPoint",
"elasticfilesystem:TagResource",
"ec2:DescribeAvailabilityZones",
},
},
Expand Down

0 comments on commit a3331a5

Please sign in to comment.