-
Notifications
You must be signed in to change notification settings - Fork 65
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
Modify DeactivationService to set scheduled deactivation time for active UserSignups #988
Changes from 51 commits
b744180
73d39ca
cd8819a
7d35f22
0ece3f2
fa5a1b8
30ae9a9
3d864f6
27eab89
76b3d25
7376ab0
8ac7c1c
141eda3
0018b07
96bd1d0
fdc0529
5fde800
bc400eb
ee76ba0
51582aa
cd5409c
d7f3ae4
fa1967d
7b5087b
91f822e
50aeeb2
645191e
e5f0ed6
5334e28
3496763
9fcee7c
74f9329
bdf613b
85b95f1
8333dea
67ec7fd
67082ca
e729c5a
193ac5b
cdd1bcd
cde7536
c7df220
087cdcb
2652fa2
e2dfbcb
5fcb467
d9c3511
5879cab
5a14763
a8bdb69
10bbfb6
473a24a
3fcd207
8486dc6
9f420e9
4f742c6
1cbcfa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
alexeykazakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err := statusUpdater.SetScheduledDeactivationStatus(ctx, usersignup, nil); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the expected UX for:
Do you want to show the same message/value for these two cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess since we can't actually differentiate between these two cases currently then a generic message or indicator would be most appropriate, perhaps for the "days remaining" box/field in the dashboard it can just display a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Or |
||
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") | ||
|
@@ -186,16 +224,33 @@ 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 { | ||
deactivatingCondition.Reason != toolchainv1alpha1.UserSignupDeactivatingNotificationCRCreatedReason && | ||
usersignup.Status.ScheduledDeactivationTimestamp != nil { | ||
// 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 | ||
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) | ||
alexeykazakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linter believes that the reconcile function is too complex, so we disabled that check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to re-enable it now? The code is less complex now then the initial one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the linter still doesn't like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you considered refactoring the code and splitting the method into multiple ones? TBH, the linter is correct that the method is really complex and should be simplified/refactored.