From e4b2e9c6539ad8b90ecb53a8b0aa74a72e1f5aa8 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Wed, 15 May 2024 20:50:33 +1000 Subject: [PATCH] Modify DeactivationService to set scheduled deactivation time for active UserSignups (#988) * Allow registration service to get/list/watch UserTier resources * regenerated from API * regenerated * updated deactivation controller * regenerated api * updated api * refactor * restore missing code * revert change to deactivation controller, documented reasons why * added test * moved deactivation schedule logic to deactivation controller * moved testing * updated * added sufficient test coverage for all places where scheduled deactivation time is set * fix setting scheduled deactivation time to nil only when usersignup.Status is set to deactivated * disable gocyclo linter for reconcile function, breaking this function up will just make it more difficult to read * deactivation controller should reconcile after MUR is deleted in order to reset the scheduled deactivation timestamp * a smarter way to handle resetting the scheduled deactivation time to nil after deactivation * fix test * fixed broken test, improve coverage * set scheduled deactivation time to nil when user in domain exclusion list * oops * set temporary deactivation time * improve coverage * even more coverage * fixed lint * coverage * fixed linter * fix potential infinite loop * fix * removed commented code * updated comments * removed predicate, set scheduled deactivation time to nil when in deactivating state but no notification created * review comments * slightly improve logic * review comment * coverage --------- Co-authored-by: Francisc Munteanu --- .../deactivation/deactivation_controller.go | 65 +++- .../deactivation_controller_test.go | 324 +++++++++++++++++- controllers/deactivation/predicate.go | 4 +- controllers/usersignup/status_updater.go | 11 +- .../usersignup/usersignup_controller.go | 13 +- .../usersignup/usersignup_controller_test.go | 3 + 6 files changed, 396 insertions(+), 24 deletions(-) diff --git a/controllers/deactivation/deactivation_controller.go b/controllers/deactivation/deactivation_controller.go index 34d380d5a..24c5da889 100644 --- a/controllers/deactivation/deactivation_controller.go +++ b/controllers/deactivation/deactivation_controller.go @@ -3,6 +3,8 @@ package deactivation import ( "context" "fmt" + usersignup2 "github.com/codeready-toolchain/host-operator/controllers/usersignup" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "strings" "time" @@ -48,6 +50,7 @@ type Reconciler struct { // Note: // The Controller will requeue the Request to be processed again if the returned error is non-nil or // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. +// nolint: gocyclo func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) logger.Info("Reconciling Deactivation") @@ -57,6 +60,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, errs.Wrapf(err, "unable to get ToolchainConfig") } + statusUpdater := usersignup2.StatusUpdater{ + Client: r.Client, + } + // Fetch the MasterUserRecord instance mur := &toolchainv1alpha1.MasterUserRecord{} err = r.Client.Get(ctx, request.NamespacedName, mur) @@ -103,6 +110,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. for _, domain := range config.Deactivation().DeactivationDomainsExcluded() { if strings.HasSuffix(usersignup.Spec.IdentityClaims.Email, domain) { logger.Info("user cannot be automatically deactivated because they belong to the exclusion list", "domain", domain) + + // Also set the Scheduled deactivation time to nil if it's not already + if err = statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, nil); err != nil { + logger.Error(err, "failed to update usersignup status") + return reconcile.Result{}, err + } + return reconcile.Result{}, nil } } @@ -128,6 +142,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. if err := r.resetDeactivatingState(ctx, usersignup); err != nil { return reconcile.Result{}, err } + // Set the scheduled deactivation time to nil + if err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, nil); err != nil { + logger.Error(err, "failed to update usersignup status") + return reconcile.Result{}, err + } + logger.Info("User belongs to a tier that does not have a deactivation timeout. The user will not be automatically deactivated") // Users belonging to this tier will not be auto deactivated, no need to requeue. return reconcile.Result{}, nil @@ -153,6 +173,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } + // Reset the scheduled deactivation time if required + scheduledDeactivationTime := v1.NewTime((*mur.Status.ProvisionedTime).Add(deactivationTimeout)) + err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, &scheduledDeactivationTime) + if err != nil { + return reconcile.Result{}, err + } + // requeue until it will be time to send it requeueAfterTimeToNotify := deactivatingNotificationTimeout - timeSinceProvisioned logger.Info("requeueing request", "RequeueAfter", requeueAfterTimeToNotify, @@ -164,6 +191,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. if !states.Deactivating(usersignup) { states.SetDeactivating(usersignup, true) + // Before we update the UserSignup in order to set the deactivating state, we should reset the scheduled + // deactivation time if required just in case the current value is nil or has somehow changed. Since the UserSignup + // controller is going to be reconciling immediately after setting the deactivating state then it will be + // creating a deactivating notification, meaning that the scheduled deactivation time that we set here is going + // to be extremely temporary (as it will be recalculated after the next reconcile), however it is done for correctness + scheduledDeactivationTime := v1.NewTime((*mur.Status.ProvisionedTime).Add(deactivationTimeout)) + err = statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, &scheduledDeactivationTime) + if err != nil { + return reconcile.Result{}, err + } + logger.Info("setting usersignup state to deactivating") if err := r.Client.Update(ctx, usersignup); err != nil { logger.Error(err, "failed to update usersignup") @@ -185,17 +223,36 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. deactivatingCondition, found := condition.FindConditionByType(usersignup.Status.Conditions, toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated) - if !found || deactivatingCondition.Status != corev1.ConditionTrue || - deactivatingCondition.Reason != toolchainv1alpha1.UserSignupDeactivatingNotificationCRCreatedReason { + if (!found || deactivatingCondition.Status != corev1.ConditionTrue || + deactivatingCondition.Reason != toolchainv1alpha1.UserSignupDeactivatingNotificationCRCreatedReason && + usersignup.Status.ScheduledDeactivationTimestamp != nil) && deactivatingNotificationDays > 0 { // If the UserSignup has been marked as deactivating, however the deactivating notification hasn't been - // created yet, then wait - the notification should be created shortly by the UserSignup controller - // once the "deactivating" state has been set which should cause a new reconciliation to be triggered here + // created yet, then set the deactivation timestamp to nil temporarily - UNLESS the deactivating notification days + // is configured to be 0 (or less) in which case we don't care about setting the status and can go directly + // to deactivating the user + if err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, nil); err != nil { + logger.Error(err, "failed to update usersignup status") + return reconcile.Result{}, err + } + return reconcile.Result{}, nil } + // We calculate the actual deactivation due time based on when the deactivating condition was set. This may end up + // being significantly later than the scheduled deactivation time set in UserSignup.Status.ScheduledDeactivationTime, as + // in some rare circumstances the deactivating notification/status may fail to be set due to cluster downtime or + // other reasons. Because of this, the scheduled deactivation time that is set in the UserSignup.Status should be + // treated as informational only. deactivationDueTime := deactivatingCondition.LastTransitionTime.Time.Add(time.Duration(deactivatingNotificationDays*24) * time.Hour) if time.Now().Before(deactivationDueTime) { + // Update the ScheduledDeactivationTimestamp to the recalculated time base on when the deactivating notification was sent + ts := v1.NewTime(deactivationDueTime) + if err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, &ts); err != nil { + logger.Error(err, "failed to update usersignup status") + return reconcile.Result{}, err + } + // It is not yet time to deactivate so requeue when it will be requeueAfterExpired := time.Until(deactivationDueTime) diff --git a/controllers/deactivation/deactivation_controller_test.go b/controllers/deactivation/deactivation_controller_test.go index c9bcb3183..e2944a476 100644 --- a/controllers/deactivation/deactivation_controller_test.go +++ b/controllers/deactivation/deactivation_controller_test.go @@ -3,6 +3,8 @@ package deactivation import ( "context" "fmt" + "github.com/codeready-toolchain/toolchain-common/pkg/test/usersignup" + "github.com/pkg/errors" "os" "testing" "time" @@ -69,6 +71,7 @@ func TestReconcile(t *testing.T) { murProvisionedTime := metav1.Now() mur := murtest.NewMasterUserRecord(t, username, murtest.TierName(userTier30.Name), murtest.Account("cluster1"), murtest.ProvisionedMur(&murProvisionedTime), murtest.UserIDFromUserSignup(userSignupFoobar)) mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] = userSignupFoobar.Name + r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) // when timeSinceProvisioned := time.Since(murProvisionedTime.Time) @@ -79,7 +82,65 @@ func TestReconcile(t *testing.T) { actualTime := res.RequeueAfter diff := expectedTime - actualTime require.Truef(t, diff > 0 && diff < 2*time.Second, "expectedTime: '%v' is not within 2 seconds of actualTime: '%v' diff: '%v'", expectedTime, actualTime, diff) - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) + + // confirm that the scheduled deactivation time is set + require.NoError(t, cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) + require.NotNil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) + + // confirm that the scheduled deactivation time is ~30 days + expected := time.Now().Add(30 * time.Hour * 24) + comparison := expected.Sub(userSignupFoobar.Status.ScheduledDeactivationTimestamp.Time) + + // accept if we're within 1 hour of the expected deactivation time + require.Less(t, comparison, time.Hour) + + // Reload the usersignup + reloaded := &toolchainv1alpha1.UserSignup{} + err = cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, reloaded) + require.NoError(t, err) + + scheduledDeactivationTime := reloaded.Status.ScheduledDeactivationTimestamp + + // Reconcile again + r, req, cl = prepareReconcile(t, mur.Name, userTier30, mur, reloaded, config) + res, err = r.Reconcile(context.TODO(), req) + require.NoError(t, err) + + // Ensure that the scheduled deactivation time has not been changed + reloaded = &toolchainv1alpha1.UserSignup{} + err = cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, reloaded) + require.NoError(t, err) + require.Equal(t, scheduledDeactivationTime, reloaded.Status.ScheduledDeactivationTimestamp) + }) + + t.Run("usersignup should not be deactivated but client update fails", func(t *testing.T) { + // given + murProvisionedTime := metav1.Now() + mur := murtest.NewMasterUserRecord(t, username, murtest.TierName(userTier30.Name), murtest.Account("cluster1"), murtest.ProvisionedMur(&murProvisionedTime), murtest.UserIDFromUserSignup(userSignupFoobar)) + mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] = userSignupFoobar.Name + + userSignupFoobar.Status.ScheduledDeactivationTimestamp = nil + + r, req, fakeClient := prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) + + fakeClient.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return fakeClient.Client.Status().Update(ctx, obj) + } + } + + // when + _, err := r.Reconcile(context.TODO(), req) + // then + require.Error(t, err) + + // confirm that the scheduled deactivation time is not set due to the client update failure + require.NoError(t, fakeClient.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) + require.Nil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) }) // the time since the mur was provisioned is within the deactivation timeout period for the 'deactivate90' tier @@ -98,7 +159,18 @@ func TestReconcile(t *testing.T) { actualTime := res.RequeueAfter diff := expectedTime - actualTime require.Truef(t, diff > 0 && diff < 2*time.Second, "expectedTime: '%v' is not within 2 seconds of actualTime: '%v' diff: '%v'", expectedTime, actualTime, diff) - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) + + // confirm that the scheduled deactivation time is set + require.NoError(t, cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) + require.NotNil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) + + // confirm that the scheduled deactivation time is ~90 days + expected := time.Now().Add(90 * time.Hour * 24) + comparison := expected.Sub(userSignupFoobar.Status.ScheduledDeactivationTimestamp.Time) + + // accept if we're within 1 hour of the expected deactivation time + require.Less(t, comparison, time.Hour) }) // the tier does not have a deactivationTimeoutDays set so the time since the mur was provisioned is irrelevant, the user should not be deactivated @@ -114,7 +186,7 @@ func TestReconcile(t *testing.T) { require.NoError(t, err) require.False(t, res.Requeue, "requeue should not be set") require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set") - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) }) // a mur that has not been provisioned yet @@ -128,26 +200,58 @@ func TestReconcile(t *testing.T) { require.NoError(t, err) require.False(t, res.Requeue, "requeue should not be set") require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set") - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) }) // a user that belongs to the deactivation domain excluded list t.Run("user deactivation excluded", func(t *testing.T) { // given - config := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.Deactivation().DeactivatingNotificationDays(3)) + config := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.Deactivation().DeactivatingNotificationDays(3), + testconfig.Deactivation().DeactivationDomainsExcluded("@redhat.com")) + commonconfig.UpdateConfig(config, nil) restore := commontest.SetEnvVarAndRestore(t, "HOST_OPERATOR_DEACTIVATION_DOMAINS_EXCLUDED", "@redhat.com") defer restore() murProvisionedTime := &metav1.Time{Time: time.Now().Add(-time.Duration(expectedDeactivationTimeoutDeactivate30Tier*24) * time.Hour)} mur := murtest.NewMasterUserRecord(t, username, murtest.TierName(userTier30.Name), murtest.Account("cluster1"), murtest.ProvisionedMur(murProvisionedTime), murtest.UserIDFromUserSignup(userSignupRedhat)) mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] = userSignupRedhat.Name + + now := metav1.NewTime(time.Now()) + userSignupRedhat.Status.ScheduledDeactivationTimestamp = &now + r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupRedhat, config) + + // First cause the status update to fail + cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return cl.Client.Status().Update(ctx, obj) + } + } + // when + _, err := r.Reconcile(context.TODO(), req) + require.Error(t, err) + require.Equal(t, "mock error", err.Error()) + + // Remove the mock update + cl.MockStatusUpdate = nil + + // Attempt the reconcile again res, err := r.Reconcile(context.TODO(), req) + // then require.NoError(t, err) require.False(t, res.Requeue, "requeue should not be set") require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set") - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) + + // Reload the userSignup + require.NoError(t, cl.Get(context.TODO(), types.NamespacedName{Name: userSignupRedhat.Name, Namespace: operatorNamespace}, userSignupRedhat)) + + // Confirm the scheduled deactivation time is set to nil + require.Nil(t, userSignupRedhat.Status.ScheduledDeactivationTimestamp) }) }) // in these tests, the controller should (eventually) deactivate the user @@ -164,8 +268,28 @@ func TestReconcile(t *testing.T) { mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] = userSignupFoobar.Name r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) + + // First cause the status update to fail + cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return cl.Client.Status().Update(ctx, obj) + } + } + + // when + _, err := r.Reconcile(context.TODO(), req) + require.Error(t, err) + require.Equal(t, "mock error", err.Error()) + + // Remove the mock update + cl.MockStatusUpdate = nil + // when res, err := r.Reconcile(context.TODO(), req) + // then require.NoError(t, err) require.False(t, res.Requeue) @@ -175,10 +299,38 @@ func TestReconcile(t *testing.T) { require.True(t, states.Deactivating(userSignupFoobar)) require.False(t, states.Deactivated(userSignupFoobar)) + // The scheduled deactivation time should be set to the standard 30 days after the provisioned time (i.e. in exactly 2 days time) + expected := time.Now().Add(2 * time.Hour * 24) + comparison := expected.Sub(userSignupFoobar.Status.ScheduledDeactivationTimestamp.Time) + + // accept if we're within 1 hour of the expected deactivation time + require.Less(t, comparison, time.Hour) + t.Run("reconciliation should be requeued when notification not yet sent", func(t *testing.T) { r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) + + // First cause the status update to fail + cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return cl.Client.Status().Update(ctx, obj) + } + } + // when - res, err := r.Reconcile(context.TODO(), req) + _, err := r.Reconcile(context.TODO(), req) + + require.Error(t, err) + require.Equal(t, "mock error", err.Error()) + + // Remove the mock update + cl.MockStatusUpdate = nil + + // Attempt the reconcile again + res, err = r.Reconcile(context.TODO(), req) + // then require.NoError(t, err) require.False(t, res.Requeue) @@ -192,7 +344,14 @@ func TestReconcile(t *testing.T) { // deactivated state should still be false require.False(t, states.Deactivated(userSignupFoobar)) + // Scheduled deactivation time should be set to nil + require.Nil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) + t.Run("usersignup requeued after deactivating notification created for user", func(t *testing.T) { + + // Clear the scheduled deactivation time + userSignupFoobar.Status.ScheduledDeactivationTimestamp = nil + // Set the notification status condition as sent userSignupFoobar.Status.Conditions = []toolchainv1alpha1.Condition{ { @@ -205,8 +364,28 @@ func TestReconcile(t *testing.T) { r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) + // First cause the status update to fail + cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return cl.Client.Status().Update(ctx, obj) + } + } + // when - res, err := r.Reconcile(context.TODO(), req) + _, err := r.Reconcile(context.TODO(), req) + + require.Error(t, err) + require.Equal(t, "mock error", err.Error()) + + // Remove the mock update + cl.MockStatusUpdate = nil + + // Attempt the reconcile again + res, err = r.Reconcile(context.TODO(), req) + // then require.NoError(t, err) require.False(t, res.Requeue) @@ -222,6 +401,14 @@ func TestReconcile(t *testing.T) { // deactivated state should still be false require.False(t, states.Deactivated(userSignupFoobar)) + // The scheduled deactivation time should be set to 3 days after the LastTransitionTime of the + // deactivating notification (i.e. 3 days from now) + expected := time.Now().Add(3 * time.Hour * 24) + comparison := expected.Sub(userSignupFoobar.Status.ScheduledDeactivationTimestamp.Time) + + // accept if we're within 1 hour of the expected deactivation time + require.Less(t, comparison, time.Hour) + t.Run("usersignup should be deactivated", func(t *testing.T) { // Set the notification status condition as sent, but this time 3 days in the past userSignupFoobar.Status.Conditions = []toolchainv1alpha1.Condition{ @@ -250,7 +437,20 @@ func TestReconcile(t *testing.T) { // deactivated state should now be true require.True(t, states.Deactivated(userSignupFoobar)) + // Confirm that the scheduled deactivation time is not yet set to nil since the user is now deactivated + require.NotNil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) + t.Run("usersignup already deactivated", func(t *testing.T) { + deactivatedCondition := toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.UserSignupComplete, + Status: corev1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatedReason, + LastTransitionTime: metav1.Time{Time: time.Now()}, + } + userSignupFoobar.Status.Conditions = condition.AddStatusConditions(userSignupFoobar.Status.Conditions, + deactivatedCondition) + r, req, cl = prepareReconcile(t, mur.Name, userTier30, mur, userSignupFoobar, config) + // additional reconciles should find the usersignup is already deactivated res, err := r.Reconcile(context.TODO(), req) // then @@ -277,7 +477,7 @@ func TestReconcile(t *testing.T) { require.NoError(t, err) require.False(t, res.Requeue, "requeue should not be set") require.Equal(t, time.Duration(0), res.RequeueAfter, "requeue should not be set") - assertThatUserSignupDeactivated(t, cl, username, true) + assertThatUserSignupStateIsDeactivated(t, cl, username, true) AssertMetricsCounterEquals(t, 1, metrics.UserSignupAutoDeactivatedTotal) }) }) @@ -290,7 +490,7 @@ func TestReconcile(t *testing.T) { // Set usersignup state as already set to deactivating states.SetDeactivating(userSignupFoobar, true) - // Set the provisioned time so that we were just 2 days from the original expected 30 day deactivation time (28 days) + // Set the provisioned time so that we are now just 2 days from the original expected 30 day deactivation time (i.e. 28 days in the past) murProvisionedTime := &metav1.Time{Time: time.Now().Add(-time.Duration((expectedDeactivationTimeoutDeactivate30Tier-2)*24) * time.Hour)} // Now the MasterUserRecord has been promoted to the 90 day tier @@ -312,6 +512,14 @@ func TestReconcile(t *testing.T) { require.NoError(t, cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) require.False(t, states.Deactivating(userSignupFoobar)) require.False(t, states.Deactivated(userSignupFoobar)) + + // The scheduled deactivation time should have also been updated, and should now expire in ~62 days + expected := time.Now().Add(62 * time.Hour * 24) + comparison := expected.Sub(userSignupFoobar.Status.ScheduledDeactivationTimestamp.Time) + + // accept if we're within 1 hour of the expected deactivation time + require.Less(t, comparison, time.Hour) + }) t.Run("when provisioning state is set but user is moved to a tier without deactivation", func(t *testing.T) { @@ -320,6 +528,8 @@ func TestReconcile(t *testing.T) { // Set usersignup state as already set to deactivating states.SetDeactivating(userSignupFoobar, true) + dt := metav1.NewTime(time.Now().Add(30 * 24 * time.Hour)) + userSignupFoobar.Status.ScheduledDeactivationTimestamp = &dt // Set the provisioned time so that we were just 2 days from the original expected 30 day deactivation time (28 days) murProvisionedTime := &metav1.Time{Time: time.Now().Add(-time.Duration((expectedDeactivationTimeoutDeactivate30Tier-2)*24) * time.Hour)} @@ -342,6 +552,52 @@ func TestReconcile(t *testing.T) { require.NoError(t, cl.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) require.False(t, states.Deactivating(userSignupFoobar)) require.False(t, states.Deactivated(userSignupFoobar)) + + // The scheduled deactivation time should now be set to nil + require.Nil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) + }) + + t.Run("when provisioning state is set but user is moved to a tier without deactivation but client update fails", func(t *testing.T) { + // given + userSignupFoobar := userSignupWithEmail(username, "foo@bar.com") + + // Set usersignup state as already set to deactivating + states.SetDeactivating(userSignupFoobar, true) + dt := metav1.NewTime(time.Now().Add(30 * 24 * time.Hour)) + userSignupFoobar.Status.ScheduledDeactivationTimestamp = &dt + + // Set the provisioned time so that we were just 2 days from the original expected 30 day deactivation time (28 days) + murProvisionedTime := &metav1.Time{Time: time.Now().Add(-time.Duration((expectedDeactivationTimeoutDeactivate30Tier-2)*24) * time.Hour)} + + // Now the MasterUserRecord has been promoted to the tier without automatic deactivation + mur := murtest.NewMasterUserRecord(t, username, murtest.TierName(userTierNoDeactivation.Name), murtest.Account("cluster1"), + murtest.ProvisionedMur(murProvisionedTime), murtest.UserIDFromUserSignup(userSignupFoobar)) + mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] = userSignupFoobar.Name + + r, req, fakeClient := prepareReconcile(t, mur.Name, userTierNoDeactivation, mur, userSignupFoobar, config) + + fakeClient.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error { + switch obj.(type) { + case *toolchainv1alpha1.UserSignup: + return errors.New("mock error") + default: + return fakeClient.Client.Status().Update(ctx, obj) + } + } + + // when + _, err := r.Reconcile(context.TODO(), req) + + // then + require.Error(t, err) + + // Reload the userSignup + require.NoError(t, fakeClient.Get(context.TODO(), types.NamespacedName{Name: userSignupFoobar.Name, Namespace: operatorNamespace}, userSignupFoobar)) + require.False(t, states.Deactivating(userSignupFoobar)) + require.False(t, states.Deactivated(userSignupFoobar)) + + // The scheduled deactivation time should not be set to nil because the update failed + require.NotNil(t, userSignupFoobar.Status.ScheduledDeactivationTimestamp) }) }) @@ -358,7 +614,7 @@ func TestReconcile(t *testing.T) { // then require.Error(t, err) require.Contains(t, err.Error(), `usertiers.toolchain.dev.openshift.com "deactivate30" not found`) - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) }) // cannot get UserSignup @@ -420,11 +676,53 @@ func TestReconcile(t *testing.T) { require.Contains(t, err.Error(), "usersignup update error") require.False(t, res.Requeue, "requeue should not be set") require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set") - assertThatUserSignupDeactivated(t, cl, username, false) + assertThatUserSignupStateIsDeactivated(t, cl, username, false) }) }) } +func TestAutomaticDeactivation(t *testing.T) { + config := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.Deactivation().DeactivatingNotificationDays(0)) + + // UserTiers + userTier30 := testusertier.NewUserTier("deactivate30", 30) + + userSignupMember1 := usersignup.NewUserSignup( + usersignup.WithUsername("usertoautodeactivate"), + usersignup.WithEmail("usertoautodeactivate@redhat.com"), + usersignup.ApprovedManually()) + + tierDeactivationDuration := time.Duration(userTier30.Spec.DeactivationTimeoutDays+1) * time.Hour * 24 + + mur := murtest.NewMasterUserRecord(t, "usertoautodeactivate", + murtest.WithOwnerLabel(userSignupMember1.Name), + murtest.TierName(userTier30.Name), + murtest.Account("cluster1"), + murtest.ProvisionedMur(&metav1.Time{Time: time.Now().Add(-tierDeactivationDuration)}), + murtest.UserIDFromUserSignup(userSignupMember1)) + + r, req, cl := prepareReconcile(t, mur.Name, userTier30, mur, userSignupMember1, config) + // when + _, err := r.Reconcile(context.TODO(), req) + require.NoError(t, err) + + // The user should be deactivating + reloaded := &toolchainv1alpha1.UserSignup{} + err = cl.Get(context.TODO(), types.NamespacedName{Name: userSignupMember1.Name, Namespace: operatorNamespace}, reloaded) + require.NoError(t, err) + require.True(t, states.Deactivating(reloaded)) + + // reconcile again + r, req, cl = prepareReconcile(t, mur.Name, userTier30, mur, reloaded, config) + _, err = r.Reconcile(context.TODO(), req) + require.NoError(t, err) + + // Reload the usersignup, they should now be in a deactivated state + err = cl.Get(context.TODO(), types.NamespacedName{Name: userSignupMember1.Name, Namespace: operatorNamespace}, reloaded) + require.NoError(t, err) + require.True(t, states.Deactivated(reloaded)) +} + func prepareReconcile(t *testing.T, name string, initObjs ...runtime.Object) (reconcile.Reconciler, reconcile.Request, *commontest.FakeClient) { os.Setenv("WATCH_NAMESPACE", commontest.HostOperatorNs) metrics.Reset() @@ -478,7 +776,7 @@ func userSignupWithEmail(username, email string) *toolchainv1alpha1.UserSignup { return us } -func assertThatUserSignupDeactivated(t *testing.T, cl *commontest.FakeClient, name string, expected bool) { +func assertThatUserSignupStateIsDeactivated(t *testing.T, cl *commontest.FakeClient, name string, expected bool) { userSignup := &toolchainv1alpha1.UserSignup{} err := cl.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: operatorNamespace}, userSignup) require.NoError(t, err) diff --git a/controllers/deactivation/predicate.go b/controllers/deactivation/predicate.go index 1804227d6..ce420d772 100644 --- a/controllers/deactivation/predicate.go +++ b/controllers/deactivation/predicate.go @@ -1,6 +1,8 @@ package deactivation -import "sigs.k8s.io/controller-runtime/pkg/event" +import ( + "sigs.k8s.io/controller-runtime/pkg/event" +) // CreateAndUpdateOnlyPredicate will filter out all events except Create and Update type CreateAndUpdateOnlyPredicate struct { diff --git a/controllers/usersignup/status_updater.go b/controllers/usersignup/status_updater.go index 913684bac..7a38d22ba 100644 --- a/controllers/usersignup/status_updater.go +++ b/controllers/usersignup/status_updater.go @@ -2,9 +2,9 @@ package usersignup import ( "context" - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" commonCondition "github.com/codeready-toolchain/toolchain-common/pkg/condition" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" errs "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -195,6 +195,7 @@ func (u *StatusUpdater) setStatusDeactivationInProgress(ctx context.Context, use } func (u *StatusUpdater) setStatusDeactivated(ctx context.Context, userSignup *toolchainv1alpha1.UserSignup, message string) error { + userSignup.Status.ScheduledDeactivationTimestamp = nil return u.updateStatusConditions( ctx, userSignup, @@ -448,3 +449,11 @@ func (u *StatusUpdater) updateStatusHomeSpace(ctx context.Context, userSignup *t } return nil // Nothing changed } + +func (u *StatusUpdater) SetScheduledDeactivationStatus(ctx context.Context, userSignup *toolchainv1alpha1.UserSignup, scheduledTime *metav1.Time) error { + if !userSignup.Status.ScheduledDeactivationTimestamp.Equal(scheduledTime) { + userSignup.Status.ScheduledDeactivationTimestamp = scheduledTime + return u.Client.Status().Update(ctx, userSignup) + } + return nil +} diff --git a/controllers/usersignup/usersignup_controller.go b/controllers/usersignup/usersignup_controller.go index 8ab9dac89..b79496cba 100644 --- a/controllers/usersignup/usersignup_controller.go +++ b/controllers/usersignup/usersignup_controller.go @@ -3,8 +3,6 @@ package usersignup import ( "context" "fmt" - "strconv" - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/toolchainconfig" "github.com/codeready-toolchain/host-operator/pkg/capacity" @@ -21,10 +19,9 @@ import ( "github.com/codeready-toolchain/toolchain-common/pkg/spacebinding" "github.com/codeready-toolchain/toolchain-common/pkg/states" "github.com/codeready-toolchain/toolchain-common/pkg/usersignup" - "github.com/redhat-cop/operator-utils/pkg/util" - "github.com/go-logr/logr" errs "github.com/pkg/errors" + "github.com/redhat-cop/operator-utils/pkg/util" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -37,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + "strconv" ) type StatusUpdaterFunc func(ctx context.Context, userAcc *toolchainv1alpha1.UserSignup, message string) error @@ -140,7 +138,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. } } - // If the usersignup is not banned and not within the pre-deactivation period then ensure the deactivation notification + // If the usersignup is not banned and not within the pre-deactivation period then ensure the deactivating notification // status is set to false. This is especially important for cases when a user is deactivated and then reactivated // because the status is used to trigger sending of the notification. If a user is reactivated a notification should // be sent to the user again. @@ -626,6 +624,11 @@ func (r *Reconciler) provisionMasterUserRecord( ) error { logger := log.FromContext(ctx) + // If the Annotations property is nil then initialize it + if userSignup.Annotations == nil { + userSignup.Annotations = map[string]string{} + } + // Set the last-target-cluster annotation so that if the user signs up again later on, they can be provisioned to the same cluster userSignup.Annotations[toolchainv1alpha1.UserSignupLastTargetClusterAnnotationKey] = targetCluster.getClusterName() if err := r.Client.Update(ctx, userSignup); err != nil { diff --git a/controllers/usersignup/usersignup_controller_test.go b/controllers/usersignup/usersignup_controller_test.go index c2adfbb18..3d63c1326 100644 --- a/controllers/usersignup/usersignup_controller_test.go +++ b/controllers/usersignup/usersignup_controller_test.go @@ -2274,6 +2274,9 @@ func TestUserSignupDeactivatedAfterMURCreated(t *testing.T) { "1,external": 2, }) + // Confirm that the scheduled deactivation time has been set to nil + require.Nil(t, userSignup.Status.ScheduledDeactivationTimestamp) + // A deactivated notification should have been created notifications := &toolchainv1alpha1.NotificationList{} err = r.Client.List(context.TODO(), notifications)