diff --git a/controller.go b/controller.go index 4e3103ee..bf1f399f 100644 --- a/controller.go +++ b/controller.go @@ -62,7 +62,7 @@ func ControllerCommand() *cobra.Command { invEmailBackend := cmd.Flags().String("email-backend", "stdout", "Backend to use for sending invitation mails (one of stdout, mailgun)") invEmailSender := cmd.Flags().String("email-sender", "noreply@appuio.cloud", "Sender address for invitation mails") invEmailSubject := cmd.Flags().String("email-subject", "You have been invited to APPUiO Cloud", "Subject for invitation mails") - invEmailRetryInterval := cmd.Flags().Duration("email-retry-interval", 5*time.Minute, "Retry interval for sending e-mail messages") + invEmailBaseRetryDelay := cmd.Flags().Duration("email-base-retry-interval", 15*time.Second, "Retry interval for sending e-mail messages. There is also an exponential back-off applied by the controller.") invEmailMailgunToken := cmd.Flags().String("mailgun-token", "CHANGEME", "Token used to access Mailgun API") invEmailMailgunDomain := cmd.Flags().String("mailgun-domain", "example.com", "Mailgun Domain to use") @@ -108,7 +108,7 @@ func ControllerCommand() *cobra.Command { *beRefreshJitter, *invTokenValidFor, *redeemedInvitationTTL, - *invEmailRetryInterval, + *invEmailBaseRetryDelay, mailSender, ctrl.Options{ Scheme: scheme, @@ -142,7 +142,7 @@ func setupManager( beRefreshJitter, invTokenValidFor time.Duration, redeemedInvitationTTL time.Duration, - invEmailRetryInterval time.Duration, + invEmailBaseRetryDelay time.Duration, mailSender mailsenders.MailSender, opt ctrl.Options, ) (ctrl.Manager, error) { @@ -219,11 +219,11 @@ func setupManager( return nil, err } invmail := &controllers.InvitationEmailReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("invitation-cleanup-controller"), - RetryInterval: invEmailRetryInterval, - MailSender: mailSender, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Recorder: mgr.GetEventRecorderFor("invitation-email-controller"), + BaseRetryDelay: invEmailBaseRetryDelay, + MailSender: mailSender, } if err = invmail.SetupWithManager(mgr); err != nil { return nil, err diff --git a/controllers/invitation_email_controller.go b/controllers/invitation_email_controller.go index 0d6e3775..a3e17566 100644 --- a/controllers/invitation_email_controller.go +++ b/controllers/invitation_email_controller.go @@ -5,12 +5,16 @@ import ( "fmt" "time" + "go.uber.org/multierr" + "golang.org/x/time/rate" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/log" "github.com/appuio/control-api/mailsenders" @@ -18,6 +22,8 @@ import ( userv1 "github.com/appuio/control-api/apis/user/v1" ) +const ReasonSendFailed = "SendFailed" + // InvitationEmailReconciler reconciles invitations and sends invitation emails if appropriate type InvitationEmailReconciler struct { client.Client @@ -25,8 +31,8 @@ type InvitationEmailReconciler struct { Recorder record.EventRecorder Scheme *runtime.Scheme - MailSender mailsenders.MailSender - RetryInterval time.Duration + MailSender mailsenders.MailSender + BaseRetryDelay time.Duration } //+kubebuilder:rbac:groups="rbac.appuio.io",resources=invitations,verbs=get;list;watch @@ -48,35 +54,26 @@ func (r *InvitationEmailReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - if inv.Status.Token == "" { + if inv.Status.Token == "" || inv.Spec.Email == "" { return ctrl.Result{}, nil } if apimeta.IsStatusConditionTrue(inv.Status.Conditions, userv1.ConditionEmailSent) { return ctrl.Result{}, nil } - condition := apimeta.FindStatusCondition(inv.Status.Conditions, userv1.ConditionEmailSent) - - if condition == nil { - return ctrl.Result{}, nil - } email := inv.Spec.Email id, err := r.MailSender.Send(ctx, email, inv.Name, inv.Status.Token) - if err != nil { log.V(0).Error(err, "Error in e-mail backend") - // Only update status if the error changes - if condition.Reason != err.Error() { - apimeta.SetStatusCondition(&inv.Status.Conditions, metav1.Condition{ - Type: userv1.ConditionEmailSent, - Status: metav1.ConditionFalse, - Reason: err.Error(), - }) - return ctrl.Result{}, r.Client.Status().Update(ctx, &inv) - } - return ctrl.Result{RequeueAfter: r.RetryInterval}, nil + apimeta.SetStatusCondition(&inv.Status.Conditions, metav1.Condition{ + Type: userv1.ConditionEmailSent, + Status: metav1.ConditionFalse, + Reason: ReasonSendFailed, + Message: err.Error(), + }) + return ctrl.Result{}, multierr.Append(err, r.Client.Status().Update(ctx, &inv)) } var message string @@ -96,5 +93,17 @@ func (r *InvitationEmailReconciler) Reconcile(ctx context.Context, req ctrl.Requ func (r *InvitationEmailReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&userv1.Invitation{}). + WithOptions(controller.Options{ + RateLimiter: r.rateLimiter(), + }). Complete(r) } + +func (r *InvitationEmailReconciler) rateLimiter() workqueue.RateLimiter { + // This is the default rate limiter for controllers with higher baseDelay if the reconciliation fails. + return workqueue.NewMaxOfRateLimiter( + workqueue.NewItemExponentialFailureRateLimiter(r.BaseRetryDelay, 5*time.Minute), + // 10 qps, 100 bucket size. This is only for retry speed and its only the overall factor (not per item) + &workqueue.BucketRateLimiter{Limiter: rate.NewLimiter(rate.Limit(10), 100)}, + ) +} diff --git a/controllers/invitation_email_controller_test.go b/controllers/invitation_email_controller_test.go index 98a37a36..8acdce72 100644 --- a/controllers/invitation_email_controller_test.go +++ b/controllers/invitation_email_controller_test.go @@ -30,14 +30,7 @@ func (s *SenderWithConstantId) Send(context.Context, string, string, string) (st func Test_InvitationEmailReconciler_Reconcile_Success(t *testing.T) { ctx := context.Background() - subject := &userv1.Invitation{ - ObjectMeta: metav1.ObjectMeta{ - Name: "subject", - }, - Status: userv1.InvitationStatus{ - Token: "abc", - }, - } + subject := baseInvitation() apimeta.SetStatusCondition(&subject.Status.Conditions, metav1.Condition{ Type: userv1.ConditionEmailSent, Status: metav1.ConditionFalse, @@ -58,14 +51,7 @@ func Test_InvitationEmailReconciler_Reconcile_Success(t *testing.T) { func Test_InvitationEmailReconciler_Reconcile_WithSendingFailure_Success(t *testing.T) { ctx := context.Background() - subject := &userv1.Invitation{ - ObjectMeta: metav1.ObjectMeta{ - Name: "subject", - }, - Status: userv1.InvitationStatus{ - Token: "abc", - }, - } + subject := baseInvitation() apimeta.SetStatusCondition(&subject.Status.Conditions, metav1.Condition{ Type: userv1.ConditionEmailSent, Status: metav1.ConditionFalse, @@ -75,26 +61,20 @@ func Test_InvitationEmailReconciler_Reconcile_WithSendingFailure_Success(t *test r := invitationEmailReconcilerWithFailingSender(c) _, err := r.Reconcile(ctx, requestFor(subject)) - require.NoError(t, err) + require.Error(t, err) require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(subject), subject)) require.False(t, apimeta.IsStatusConditionTrue(subject.Status.Conditions, userv1.ConditionEmailSent)) condition := apimeta.FindStatusCondition(subject.Status.Conditions, userv1.ConditionEmailSent) require.NotNil(t, condition) - require.Equal(t, "Err0r", condition.Reason) + require.Equal(t, ReasonSendFailed, condition.Reason) } func Test_InvitationEmailReconciler_Reconcile_NoEmail_Success(t *testing.T) { ctx := context.Background() - subject := &userv1.Invitation{ - ObjectMeta: metav1.ObjectMeta{ - Name: "subject", - }, - Status: userv1.InvitationStatus{ - Token: "abc", - }, - } + subject := baseInvitation() + subject.Spec.Email = "" c := prepareTest(t, subject) @@ -108,20 +88,34 @@ func Test_InvitationEmailReconciler_Reconcile_NoEmail_Success(t *testing.T) { func invitationEmailReconciler(c client.WithWatch) *InvitationEmailReconciler { return &InvitationEmailReconciler{ - Client: c, - Scheme: c.Scheme(), - Recorder: record.NewFakeRecorder(3), - MailSender: &SenderWithConstantId{}, - RetryInterval: time.Minute, + Client: c, + Scheme: c.Scheme(), + Recorder: record.NewFakeRecorder(3), + MailSender: &SenderWithConstantId{}, + BaseRetryDelay: time.Minute, } } func invitationEmailReconcilerWithFailingSender(c client.WithWatch) *InvitationEmailReconciler { return &InvitationEmailReconciler{ - Client: c, - Scheme: c.Scheme(), - Recorder: record.NewFakeRecorder(3), - MailSender: &FailingSender{}, - RetryInterval: time.Minute, + Client: c, + Scheme: c.Scheme(), + Recorder: record.NewFakeRecorder(3), + MailSender: &FailingSender{}, + BaseRetryDelay: time.Minute, + } +} + +func baseInvitation() *userv1.Invitation { + return &userv1.Invitation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subject", + }, + Spec: userv1.InvitationSpec{ + Email: "subject@example.com", + }, + Status: userv1.InvitationStatus{ + Token: "abc", + }, } }