Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move iam logic to a separate controller #7145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 2 additions & 33 deletions controllers/cluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

anywherev1 "github.com/aws/eks-anywhere/pkg/api/v1alpha1"
Expand Down Expand Up @@ -46,7 +45,6 @@ const (
type ClusterReconciler struct {
client client.Client
providerReconcilerRegistry ProviderClusterReconcilerRegistry
awsIamAuth AWSIamConfigReconciler
clusterValidator ClusterValidator
packagesClient PackagesClient
machineHealthCheck MachineHealthCheckReconciler
Expand All @@ -64,13 +62,6 @@ type ProviderClusterReconcilerRegistry interface {
Get(datacenterKind string) clusters.ProviderClusterReconciler
}

// AWSIamConfigReconciler manages aws-iam-authenticator installation and configuration for an eks-a cluster.
type AWSIamConfigReconciler interface {
EnsureCASecret(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error)
Reconcile(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error)
ReconcileDelete(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) error
}

// MachineHealthCheckReconciler manages machine health checks for an eks-a cluster.
type MachineHealthCheckReconciler interface {
Reconcile(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) error
Expand All @@ -85,11 +76,10 @@ type ClusterValidator interface {
type ClusterReconcilerOption func(*ClusterReconciler)

// NewClusterReconciler constructs a new ClusterReconciler.
func NewClusterReconciler(client client.Client, registry ProviderClusterReconcilerRegistry, awsIamAuth AWSIamConfigReconciler, clusterValidator ClusterValidator, pkgs PackagesClient, machineHealthCheck MachineHealthCheckReconciler, opts ...ClusterReconcilerOption) *ClusterReconciler {
func NewClusterReconciler(client client.Client, registry ProviderClusterReconcilerRegistry, clusterValidator ClusterValidator, pkgs PackagesClient, machineHealthCheck MachineHealthCheckReconciler, opts ...ClusterReconcilerOption) *ClusterReconciler {
c := &ClusterReconciler{
client: client,
providerReconcilerRegistry: registry,
awsIamAuth: awsIamAuth,
clusterValidator: clusterValidator,
packagesClient: pkgs,
machineHealthCheck: machineHealthCheck,
Expand Down Expand Up @@ -210,7 +200,7 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
log.Info("Reconciling cluster")
if err := r.client.Get(ctx, req.NamespacedName, cluster); err != nil {
if apierrors.IsNotFound(err) {
return reconcile.Result{}, nil
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -351,13 +341,6 @@ func (r *ClusterReconciler) reconcile(ctx context.Context, log logr.Logger, clus

func (r *ClusterReconciler) preClusterProviderReconcile(ctx context.Context, log logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error) {
// Run some preflight validations that can't be checked in webhook
if cluster.HasAWSIamConfig() {
if result, err := r.awsIamAuth.EnsureCASecret(ctx, log, cluster); err != nil {
return controller.Result{}, err
} else if result.Return() {
return result, nil
}
}
if cluster.IsManaged() {
if err := r.clusterValidator.ValidateManagementClusterName(ctx, log, cluster); err != nil {
log.Error(err, "Invalid cluster configuration")
Expand Down Expand Up @@ -390,14 +373,6 @@ func (r *ClusterReconciler) preClusterProviderReconcile(ctx context.Context, log
}

func (r *ClusterReconciler) postClusterProviderReconcile(ctx context.Context, log logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error) {
if cluster.HasAWSIamConfig() {
if result, err := r.awsIamAuth.Reconcile(ctx, log, cluster); err != nil {
return controller.Result{}, err
} else if result.Return() {
return result, nil
}
}

if err := r.machineHealthCheck.Reconcile(ctx, log, cluster); err != nil {
return controller.Result{}, err
}
Expand Down Expand Up @@ -498,12 +473,6 @@ func (r *ClusterReconciler) reconcileDelete(ctx context.Context, log logr.Logger

}

if cluster.HasAWSIamConfig() {
if err := r.awsIamAuth.ReconcileDelete(ctx, log, cluster); err != nil {
return ctrl.Result{}, err
}
}

if cluster.IsManaged() {
if err := r.packagesClient.ReconcileDelete(ctx, log, r.client, cluster); err != nil {
return ctrl.Result{}, fmt.Errorf("deleting packages for cluster %q: %w", cluster.Name, err)
Expand Down
112 changes: 112 additions & 0 deletions controllers/iamconfig_reconciler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package controllers

import (
"context"

"github.com/go-logr/logr"
apierrors "k8s.io/apimachinery/pkg/api/errors"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/cluster-api/util/patch"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/source"

anywherev1 "github.com/aws/eks-anywhere/pkg/api/v1alpha1"
"github.com/aws/eks-anywhere/pkg/controller"
"github.com/aws/eks-anywhere/pkg/controller/handlers"
)

const IAMConfigClusterFinalizerName = "clusters.anywhere.eks.amazonaws.com/iam-finalizer"

// IAMConfigClusterReconciler reconciles the IAM installation for Clusters.
type IAMConfigClusterReconciler struct {
client client.Client
awsIamAuth AWSIamConfigReconciler
}

// AWSIamConfigReconciler manages aws-iam-authenticator installation and configuration for an eks-a cluster.
type AWSIamConfigReconciler interface {
EnsureCASecret(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error)
Reconcile(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) (controller.Result, error)
ReconcileDelete(ctx context.Context, logger logr.Logger, cluster *anywherev1.Cluster) error
}

// SetupWithManager sets up the controller with the Manager.
func (r *IAMConfigClusterReconciler) SetupWithManager(mgr ctrl.Manager, log logr.Logger) error {
childObjectHandler := handlers.ChildObjectToClusters(log)

return ctrl.NewControllerManagedBy(mgr).
For(&anywherev1.Cluster{}).
Watches(
&source.Kind{Type: &anywherev1.AWSIamConfig{}},
handler.EnqueueRequestsFromMapFunc(childObjectHandler),
).
Complete(r)
}

// Reconcile installs AWS IAM components in a cluster. It implements controller-runtime Reconciler.
func (r *IAMConfigClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, reterr error) {
log := ctrl.LoggerFrom(ctx)
cluster := &anywherev1.Cluster{}
if err := r.client.Get(ctx, req.NamespacedName, cluster); err != nil {
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
}

if cluster.HasAWSIamConfig() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this condition be the opposite? We want to exit the reconcile if it doesn't have it right?

return ctrl.Result{}, nil
}

patchHelper, err := patch.NewHelper(cluster, r.client)
if err != nil {
return ctrl.Result{}, err
}

defer func() {
if err := patchCluster(ctx, patchHelper, cluster); err != nil {
reterr = kerrors.NewAggregate([]error{reterr, err})
}
}()

// AddFinalizer is idempotent
controllerutil.AddFinalizer(cluster, ClusterFinalizerName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be IAMConfigClusterFinalizerName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing :)


if !cluster.DeletionTimestamp.IsZero() {
result, reterr = r.reconcileDelete(ctx, log, cluster)
return result, reterr
}

result, reterr = r.reconcile(ctx, log, cluster)
return result, reterr
}

func (r *IAMConfigClusterReconciler) reconcile(ctx context.Context, log logr.Logger, cluster *anywherev1.Cluster) (ctrl.Result, error) {
if result, err := r.awsIamAuth.EnsureCASecret(ctx, log, cluster); err != nil {
return ctrl.Result{}, err
} else if result.Return() {
return result.ToCtrlResult(), nil
}

if conditions.IsFalse(cluster, anywherev1.ControlPlaneReadyCondition) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with this separation of responsibility between different controllers, are we going to want to add a status to the Cluster resource to track the state of these? It doesn't seem necessary, but after it sounds like we'd have separate controller logs to monitor the status on these resources.

return ctrl.Result{}, nil
}

result, err := r.awsIamAuth.Reconcile(ctx, log, cluster)

return result.ToCtrlResult(), err
}

func (r *IAMConfigClusterReconciler) reconcileDelete(ctx context.Context, log logr.Logger, cluster *anywherev1.Cluster) (ctrl.Result, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentionally reconcileDelete instead of ReconcileDelete? the interface declares a public ReconcileDelete method I believe?

if err := r.awsIamAuth.ReconcileDelete(ctx, log, cluster); err != nil {
return ctrl.Result{}, err
}

controllerutil.RemoveFinalizer(cluster, ClusterFinalizerName)

return ctrl.Result{}, nil
}
Loading