Merge pull request #263 from sabre1041/prune-group-parse-time

Correct logic for parsing and comparing sync time

Author: raffaelespazzoli

controllers/groupsync_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -107,7 +108,7 @@ func (r *GroupSyncReconciler) Reconcile(context context.Context, req ctrl.Reques
 			return r.wrapMetricsErrorWithMetrics(prometheusLabels, context, instance, err)
 		}
 
-		syncStartTime := ISO8601(time.Now())
+		syncStartTime := time.Now().Format(time.RFC3339)
 		// Perform Sync
 		groups, err := groupSyncer.Sync()
 
@@ -162,7 +163,7 @@ func (r *GroupSyncReconciler) Reconcile(context context.Context, req ctrl.Reques
 			ocpGroup.Labels[constants.SyncProvider] = providerLabel
 
 			// Add Gloabl Annotations/Labels
-			ocpGroup.Annotations[constants.SyncTimestamp] = ISO8601(time.Now())
+			ocpGroup.Annotations[constants.SyncTimestamp] = time.Now().UTC().Format(time.RFC3339)
 
 			ocpGroup.Users = group.Users
 
@@ -205,7 +206,7 @@ func (r *GroupSyncReconciler) Reconcile(context context.Context, req ctrl.Reques
 	if err == nil && instance.Spec.Schedule != "" {
 		sched, _ := cron.ParseStandard(instance.Spec.Schedule)
 
-		currentTime := time.Now()
+		currentTime := time.Now().UTC()
 		nextScheduledTime := sched.Next(currentTime)
 		nextScheduledSynchronization.With(prometheus.Labels{METRICS_CR_NAMESPACE_LABEL: instance.GetNamespace(), METRICS_CR_NAME_LABEL: instance.GetName()}).Set(float64(nextScheduledTime.UTC().Unix()))
 		successResult.RequeueAfter = nextScheduledTime.Sub(currentTime)
@@ -231,6 +232,14 @@ func (r *GroupSyncReconciler) wrapMetricsErrorWithMetrics(prometheusLabels prome
 
 func (r *GroupSyncReconciler) pruneGroups(context context.Context, instance *redhatcopv1alpha1.GroupSync, providerLabel string, syncStartTime string, logger logr.Logger) (int, error) {
 	prunedGroups := 0
+
+	syncStartDatetime, syncStartParseError := time.Parse(time.RFC3339, syncStartTime)
+
+	// Should not occur
+	if syncStartParseError != nil {
+		return prunedGroups, syncStartParseError
+	}
+
 	ocpGroups := &userv1.GroupList{}
 	opts := []client.ListOption{
 		client.InNamespace(""),
@@ -242,29 +251,32 @@ func (r *GroupSyncReconciler) pruneGroups(context context.Context, instance *red
 	}
 
 	for _, group := range ocpGroups.Items {
-		if group.Annotations[constants.SyncTimestamp] < syncStartTime {
-			logger.Info("pruneGroups", "Delete Group", group.Name)
-			err = r.GetClient().Delete(context, &group)
-			prunedGroups++
-			if err != nil {
-				return prunedGroups, err
+
+		if groupSyncTime, ok := group.Annotations[constants.SyncTimestamp]; ok {
+
+			groupSyncDatetime, groupSyncTimeParseErr := time.Parse(time.RFC3339, groupSyncTime)
+
+			if groupSyncTimeParseErr == nil {
+				if groupSyncDatetime.Before(syncStartDatetime) {
+					logger.Info("pruneGroups", "Delete Group", group.Name)
+					err = r.GetClient().Delete(context, &group)
+					prunedGroups++
+					if err != nil {
+						return prunedGroups, err
+					}
+				}
+			} else {
+				if groupSyncTimeParseErr != nil {
+					logger.Error(groupSyncTimeParseErr, "Error parsing group start time annotation", "Group Name", group.Name, "Time", syncStartTime)
+				}
 			}
+		} else {
+			logger.Error(errors.New("unable to locate sync timestamp annotation"), "Group Name", group.Name)
 		}
 	}
 	return prunedGroups, nil
 }
 
-func ISO8601(t time.Time) string {
-	var tz string
-	if zone, offset := t.Zone(); zone == "UTC" {
-		tz = "Z"
-	} else {
-		tz = fmt.Sprintf("%03d00", offset/3600)
-	}
-	return fmt.Sprintf("%04d-%02d-%02dT%02d:%02d:%02d%s",
-		t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), t.Second(), tz)
-}
-
 func mergeMap(m1, m2 map[string]string) map[string]string {
 
 	if m1 != nil {

pkg/provider/ldap/helpers/groupsyncer.go

@@ -172,19 +172,7 @@ func (s *LDAPGroupSyncer) makeOpenShiftGroup(ldapGroupUID string, usernames []st
 
 	// overwrite Group Users data
 	group.Users = usernames
-	group.Annotations[LDAPSyncTimeAnnotation] = ISO8601(time.Now())
+	group.Annotations[LDAPSyncTimeAnnotation] = time.Now().UTC().Format(time.RFC3339)
 
 	return group, nil
 }
-
-// ISO8601 returns an ISO 6801 formatted string from a time.
-func ISO8601(t time.Time) string {
-	var tz string
-	if zone, offset := t.Zone(); zone == "UTC" {
-		tz = "Z"
-	} else {
-		tz = fmt.Sprintf("%03d00", offset/3600)
-	}
-	return fmt.Sprintf("%04d-%02d-%02dT%02d:%02d:%02d%s",
-		t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), t.Second(), tz)
-}