Skip to content

Commit

Permalink
Retry removing all finalizers (#289)
Browse files Browse the repository at this point in the history
* Retry removing all finalizers

* Update CHANGELOG

* Break retry patch cycle

* Persist changes to logger values in context

* Rename var
  • Loading branch information
mnitchev authored Apr 11, 2024
1 parent 395cff0 commit 9cc6dab
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 102 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Add retry logic for removing the finalizer to all reconcilers. This fixes the same bug as in 0.17.1 but for all reconcilers.

## [0.21.0] - 2024-03-20

### Changed
Expand Down
36 changes: 15 additions & 21 deletions controllers/awsmachinepool_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ import (
awsclientgo "github.com/aws/aws-sdk-go/aws/client"
"github.com/aws/aws-sdk-go/service/iam/iamiface"
"github.com/giantswarm/microerror"
"github.com/go-logr/logr"
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
expcapa "sigs.k8s.io/cluster-api-provider-aws/exp/api/v1beta1"
"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/log"

"github.com/giantswarm/capa-iam-operator/pkg/awsclient"
"github.com/giantswarm/capa-iam-operator/pkg/iam"
Expand All @@ -40,7 +40,6 @@ import (
// AWSMachinePoolReconciler reconciles a AWSMachinePool object
type AWSMachinePoolReconciler struct {
client.Client
Log logr.Logger
IAMClientFactory func(awsclientgo.ConfigProvider, string) iamiface.IAMAPI
AWSClient awsclient.AwsClientInterface
}
Expand All @@ -50,8 +49,7 @@ type AWSMachinePoolReconciler struct {
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awsmachinepools/finalizers,verbs=update

func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
var err error
logger := r.Log.WithValues("namespace", req.Namespace, "awsMachinePool", req.Name)
logger := log.FromContext(ctx)

awsMachinePool := &expcapa.AWSMachinePool{}
if err := r.Get(ctx, req.NamespacedName, awsMachinePool); err != nil {
Expand All @@ -74,6 +72,7 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque
}

logger = logger.WithValues("cluster", clusterName)
ctx = log.IntoContext(ctx, logger)

if awsMachinePool.Spec.AWSLaunchTemplate.IamInstanceProfile == "" {
logger.Info("AWSMachinePool has empty .Spec.AWSLaunchTemplate.IamInstanceProfile, not reconciling IAM role")
Expand Down Expand Up @@ -115,12 +114,14 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque
}

if awsMachinePool.DeletionTimestamp != nil {
return r.reconcileDelete(ctx, awsMachinePool, iamService, logger)
return r.reconcileDelete(ctx, awsMachinePool, iamService)
}
return r.reconcileNormal(ctx, awsMachinePool, iamService, logger)
return r.reconcileNormal(ctx, awsMachinePool, iamService)
}

func (r *AWSMachinePoolReconciler) reconcileDelete(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService, logger logr.Logger) (ctrl.Result, error) {
func (r *AWSMachinePoolReconciler) reconcileDelete(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService) (ctrl.Result, error) {
logger := log.FromContext(ctx)

roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, awsMachinePool.Spec.AWSLaunchTemplate.IamInstanceProfile)
if err != nil {
return ctrl.Result{}, errors.WithStack(err)
Expand All @@ -133,25 +134,18 @@ func (r *AWSMachinePoolReconciler) reconcileDelete(ctx context.Context, awsMachi
}
}

// remove finalizer from AWSMachinePool
if controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) {
patchHelper, err := patch.NewHelper(awsMachinePool, r.Client)
if err != nil {
return ctrl.Result{}, errors.WithStack(err)
}
controllerutil.RemoveFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole))
err = patchHelper.Patch(ctx, awsMachinePool)
if err != nil {
logger.Error(err, "failed to remove finalizer from AWSMachinePool")
return ctrl.Result{}, errors.WithStack(err)
}
logger.Info("successfully removed finalizer from AWSMachinePool", "finalizer_name", iam.NodesRole)
err = removeFinalizer(ctx, r.Client, awsMachinePool, iam.NodesRole)
if err != nil {
logger.Error(err, "failed to remove finalizer from AWSMachinePool")
return ctrl.Result{}, errors.WithStack(err)
}

return ctrl.Result{}, nil
}

func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService, logger logr.Logger) (ctrl.Result, error) {
func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, awsMachinePool *expcapa.AWSMachinePool, iamService *iam.IAMService) (ctrl.Result, error) {
logger := log.FromContext(ctx)

// add finalizer to AWSMachinePool
if !controllerutil.ContainsFinalizer(awsMachinePool, key.FinalizerName(iam.NodesRole)) {
patchHelper, err := patch.NewHelper(awsMachinePool, r.Client)
Expand Down
4 changes: 2 additions & 2 deletions controllers/awsmachinepool_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ var _ = Describe("AWSMachinePoolReconciler", func() {

reconciler = &controllers.AWSMachinePoolReconciler{
Client: k8sClient,
Log: ctrl.Log,
AWSClient: mockAwsClient,
IAMClientFactory: func(session awsclientupstream.ConfigProvider, region string) iamiface.IAMAPI {
return mockIAMClient
Expand Down Expand Up @@ -138,7 +137,8 @@ var _ = Describe("AWSMachinePoolReconciler", func() {
}

sess, err = session.NewSession(&aws.Config{
Region: aws.String("eu-west-1")},
Region: aws.String("eu-west-1"),
},
)
Expect(err).NotTo(HaveOccurred())
})
Expand Down
69 changes: 14 additions & 55 deletions controllers/awsmachinetemplate_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,32 +23,28 @@ import (
awsclientgo "github.com/aws/aws-sdk-go/aws/client"
"github.com/aws/aws-sdk-go/service/iam/iamiface"
"github.com/giantswarm/microerror"
"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
errutils "k8s.io/apimachinery/pkg/util/errors"

"k8s.io/apimachinery/pkg/types"
capa "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
"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/log"

"github.com/giantswarm/capa-iam-operator/pkg/awsclient"
"github.com/giantswarm/capa-iam-operator/pkg/iam"
"github.com/giantswarm/capa-iam-operator/pkg/key"
)

const maxPatchRetries = 5

// AWSMachineTemplateReconciler reconciles a AWSMachineTemplate object
type AWSMachineTemplateReconciler struct {
client.Client
EnableKiamRole bool
EnableRoute53Role bool
Log logr.Logger
AWSClient awsclient.AwsClientInterface
IAMClientFactory func(awsclientgo.ConfigProvider, string) iamiface.IAMAPI
}
Expand All @@ -62,8 +58,7 @@ type AWSMachineTemplateReconciler struct {
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
var err error
logger := r.Log.WithValues("namespace", req.Namespace, "awsMachineTemplate", req.Name)
logger := log.FromContext(ctx)

awsMachineTemplate := &capa.AWSMachineTemplate{}
if err := r.Get(ctx, req.NamespacedName, awsMachineTemplate); err != nil {
Expand Down Expand Up @@ -96,6 +91,7 @@ func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.R
}

logger = logger.WithValues("cluster", clusterName, "role", role)
ctx = log.IntoContext(ctx, logger)

if awsMachineTemplate.Spec.Template.Spec.IAMInstanceProfile == "" {
logger.Info("AWSMachineTemplate has empty .Spec.Template.Spec.IAMInstanceProfile, not creating IAM role")
Expand Down Expand Up @@ -138,12 +134,14 @@ func (r *AWSMachineTemplateReconciler) Reconcile(ctx context.Context, req ctrl.R
}

if awsMachineTemplate.DeletionTimestamp != nil {
return r.reconcileDelete(ctx, iamService, awsMachineTemplate, logger, clusterName, req.Namespace, role)
return r.reconcileDelete(ctx, iamService, awsMachineTemplate, clusterName, req.Namespace, role)
}
return r.reconcileNormal(ctx, iamService, awsMachineTemplate, logger, awsCluster, clusterName, role)
return r.reconcileNormal(ctx, iamService, awsMachineTemplate, awsCluster, clusterName, role)
}

func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, logger logr.Logger, clusterName, namespace, role string) (ctrl.Result, error) {
func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, clusterName, namespace, role string) (ctrl.Result, error) {
logger := log.FromContext(ctx)

roleUsed, err := isRoleUsedElsewhere(ctx, r.Client, awsMachineTemplate.Spec.Template.Spec.IAMInstanceProfile)
if err != nil {
return ctrl.Result{}, err
Expand All @@ -169,14 +167,14 @@ func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamS
logger.Error(err, "failed to get awsCluster")
return ctrl.Result{}, err
}
err = r.removeFinalizer(ctx, logger, awsCluster)
err = removeFinalizer(ctx, r.Client, awsCluster, iam.ControlPlaneRole)
if err != nil {
logger.Error(err, "Failed to remove finalizer from AWSCluster")
return ctrl.Result{}, err
}

// remove finalizer from AWSMachineTemplate
err = r.removeFinalizer(ctx, logger, awsMachineTemplate)
err = removeFinalizer(ctx, r.Client, awsMachineTemplate, iam.ControlPlaneRole)
if err != nil {
logger.Error(err, "Failed to remove finalizer from AWSMachineTemplate")
return ctrl.Result{}, err
Expand All @@ -195,7 +193,7 @@ func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamS
return ctrl.Result{}, errors.WithStack(client.IgnoreNotFound(err))
}

err = r.removeFinalizer(ctx, logger, cm)
err = removeFinalizer(ctx, r.Client, cm, iam.ControlPlaneRole)
if err != nil {
logger.Error(err, "Failed to remove finalizer from ConfigMap")
return ctrl.Result{}, err
Expand All @@ -204,7 +202,9 @@ func (r *AWSMachineTemplateReconciler) reconcileDelete(ctx context.Context, iamS
return ctrl.Result{}, nil
}

func (r *AWSMachineTemplateReconciler) reconcileNormal(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, logger logr.Logger, awsCluster *capa.AWSCluster, clusterName, role string) (ctrl.Result, error) {
func (r *AWSMachineTemplateReconciler) reconcileNormal(ctx context.Context, iamService *iam.IAMService, awsMachineTemplate *capa.AWSMachineTemplate, awsCluster *capa.AWSCluster, clusterName, role string) (ctrl.Result, error) {
logger := log.FromContext(ctx)

// add finalizer to AWSMachineTemplate
if !controllerutil.ContainsFinalizer(awsMachineTemplate, key.FinalizerName(iam.ControlPlaneRole)) {
patchHelper, err := patch.NewHelper(awsMachineTemplate, r.Client)
Expand Down Expand Up @@ -283,44 +283,3 @@ func (r *AWSMachineTemplateReconciler) SetupWithManager(mgr ctrl.Manager) error
For(&capa.AWSMachineTemplate{}).
Complete(r)
}

func (r *AWSMachineTemplateReconciler) removeFinalizer(ctx context.Context, logger logr.Logger, object client.Object) error {
if !controllerutil.ContainsFinalizer(object, key.FinalizerName(iam.ControlPlaneRole)) {
logger.Info("finalizer already removed")
return nil
}

for i := 1; i <= maxPatchRetries; i++ {
patchHelper, err := patch.NewHelper(object, r.Client)
if err != nil {
logger.Error(err, "failed to create patch helper")
return errors.WithStack(err)
}
controllerutil.RemoveFinalizer(object, key.FinalizerName(iam.ControlPlaneRole))
err = patchHelper.Patch(ctx, object)

// If another controller has removed its finalizer while we're
// reconciling this will fail with "Forbidden: no new finalizers can be
// added if the object is being deleted". The actual response code is
// 422 Unprocessable entity, which maps to StatusReasonInvalid in the
// k8serrors package. We have to get the cluster again with the now
// removed finalizer(s) and try again.
invalidErr := errutils.FilterOut(err, func(err error) bool {
return !k8serrors.IsInvalid(err)
})

if invalidErr != nil && i < maxPatchRetries {
logger.Info(fmt.Sprintf("patching object failed, trying again: %s", err.Error()))
if err := r.Get(ctx, client.ObjectKeyFromObject(object), object); err != nil {
return microerror.Mask(err)
}
continue
}
if err != nil {
logger.Error(err, "failed to remove finalizers")
return microerror.Mask(err)
}
}
logger.Info("successfully removed finalizer")
return nil
}
1 change: 0 additions & 1 deletion controllers/awsmachinetemplate_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ var _ = Describe("AWSMachineTemplateReconciler", func() {
Client: k8sClient,
EnableKiamRole: true,
EnableRoute53Role: true,
Log: ctrl.Log,
AWSClient: mockAwsClient,
IAMClientFactory: func(session awsclientupstream.ConfigProvider, region string) iamiface.IAMAPI {
return mockIAMClient
Expand Down
29 changes: 9 additions & 20 deletions controllers/awsmanagedcontrolplane_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ import (
awsclientgo "github.com/aws/aws-sdk-go/aws/client"
"github.com/aws/aws-sdk-go/service/iam/iamiface"
"github.com/giantswarm/microerror"
"github.com/go-logr/logr"
apierrors "k8s.io/apimachinery/pkg/api/errors"
eks "sigs.k8s.io/cluster-api-provider-aws/controlplane/eks/api/v1beta1"
"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/log"

"github.com/giantswarm/capa-iam-operator/pkg/awsclient"
"github.com/giantswarm/capa-iam-operator/pkg/iam"
Expand All @@ -39,17 +39,15 @@ import (
// AWSManagedControlPlaneReconciler reconciles a AWSManagedControlPlane object
type AWSManagedControlPlaneReconciler struct {
client.Client
Log logr.Logger
AWSClient awsclient.AwsClientInterface
IAMClientFactory func(awsclientgo.ConfigProvider, string) iamiface.IAMAPI
}

func (r *AWSManagedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
var err error
logger := r.Log.WithValues("namespace", req.Namespace, "AWSManagedControlPlane", req.Name)
logger := log.FromContext(ctx)

eksCluster := &eks.AWSManagedControlPlane{}
if err = r.Get(ctx, req.NamespacedName, eksCluster); err != nil {
if err := r.Get(ctx, req.NamespacedName, eksCluster); err != nil {
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
Expand Down Expand Up @@ -105,22 +103,13 @@ func (r *AWSManagedControlPlaneReconciler) Reconcile(ctx context.Context, req ct
if err != nil {
return ctrl.Result{}, microerror.Mask(err)
}
// remove finalizer from AWSManagedControlPlane
{
if controllerutil.ContainsFinalizer(eksCluster, key.FinalizerName(iam.IRSARole)) {
patchHelper, err := patch.NewHelper(eksCluster, r.Client)
if err != nil {
return ctrl.Result{}, microerror.Mask(err)
}
controllerutil.RemoveFinalizer(eksCluster, key.FinalizerName(iam.IRSARole))
err = patchHelper.Patch(ctx, eksCluster)
if err != nil {
logger.Error(err, "failed to remove finalizer on AWSManagedControlPlane")
return ctrl.Result{}, microerror.Mask(err)
}
logger.Info("successfully removed finalizer from AWSManagedControlPlane", "finalizer_name", iam.IRSARole)
}

err = removeFinalizer(ctx, r.Client, eksCluster, iam.IRSARole)
if err != nil {
logger.Error(err, "failed to remove finalizer on AWSManagedControlPlane")
return ctrl.Result{}, microerror.Mask(err)
}

} else {
// add finalizer to AWSManagedControlPlane
if !controllerutil.ContainsFinalizer(eksCluster, key.FinalizerName(iam.IRSARole)) {
Expand Down
Loading

0 comments on commit 9cc6dab

Please sign in to comment.