Skip to content

Commit

Permalink
fix: check err before use schedule and duration (#20043)
Browse files Browse the repository at this point in the history
* fix: check err before use schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* test: add tests for invalid schedule and duration

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* feat: change to return error when sync window is invalid

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use assert.Error or assert.NoError

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

* fix: use require instead of assert

Signed-off-by: daengdaengLee <gunho1020@gmail.com>

---------

Signed-off-by: daengdaengLee <gunho1020@gmail.com>
  • Loading branch information
daengdaengLee authored Oct 12, 2024
1 parent 92de225 commit 9f556a5
Show file tree
Hide file tree
Showing 8 changed files with 264 additions and 75 deletions.
11 changes: 6 additions & 5 deletions cmd/argocd/commands/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -585,8 +585,8 @@ func printAppSummaryTable(app *argoappv1.Application, appURL string, windows *ar
var status string
var allow, deny, inactiveAllows bool
if windows.HasWindows() {
active := windows.Active()
if active.HasWindows() {
active, err := windows.Active()
if err == nil && active.HasWindows() {
for _, w := range *active {
if w.Kind == "deny" {
deny = true
Expand All @@ -595,13 +595,14 @@ func printAppSummaryTable(app *argoappv1.Application, appURL string, windows *ar
}
}
}
if windows.InactiveAllows().HasWindows() {
inactiveAllowWindows, err := windows.InactiveAllows()
if err == nil && inactiveAllowWindows.HasWindows() {
inactiveAllows = true
}

s := windows.CanSync(true)
if deny || !deny && !allow && inactiveAllows {
if s {
s, err := windows.CanSync(true)
if err == nil && s {
status = "Manual Allowed"
} else {
status = "Sync Denied"
Expand Down
3 changes: 2 additions & 1 deletion cmd/argocd/commands/projectwindows.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,9 +352,10 @@ func printSyncWindows(proj *v1alpha1.AppProject) {
fmt.Fprintf(w, fmtStr, headers...)
if proj.Spec.SyncWindows.HasWindows() {
for i, window := range proj.Spec.SyncWindows {
isActive, _ := window.Active()
vals := []interface{}{
strconv.Itoa(i),
formatBoolOutput(window.Active()),
formatBoolOutput(isActive),
window.Kind,
window.Schedule,
window.Duration,
Expand Down
3 changes: 2 additions & 1 deletion controller/appcontroller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1690,7 +1690,8 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
app.Status.Summary = tree.GetSummary(app)
}

if project.Spec.SyncWindows.Matches(app).CanSync(false) {
canSync, _ := project.Spec.SyncWindows.Matches(app).CanSync(false)
if canSync {
syncErrCond, opMS := ctrl.autoSync(app, compareResult.syncStatus, compareResult.resources, compareResult.revisionUpdated)
setOpMs = opMS
if syncErrCond != nil {
Expand Down
25 changes: 18 additions & 7 deletions controller/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,18 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
state.Phase = common.OperationError
state.Message = fmt.Sprintf("Failed to load application project: %v", err)
return
} else if syncWindowPreventsSync(app, proj) {
// If the operation is currently running, simply let the user know the sync is blocked by a current sync window
if state.Phase == common.OperationRunning {
state.Message = "Sync operation blocked by sync window"
} else {
isBlocked, err := syncWindowPreventsSync(app, proj)
if isBlocked {
// If the operation is currently running, simply let the user know the sync is blocked by a current sync window
if state.Phase == common.OperationRunning {
state.Message = "Sync operation blocked by sync window"
if err != nil {
state.Message = fmt.Sprintf("%s: %v", state.Message, err)
}
}
return
}
return
}

if !isMultiSourceRevision {
Expand Down Expand Up @@ -557,13 +563,18 @@ func delayBetweenSyncWaves(phase common.SyncPhase, wave int, finalWave bool) err
return nil
}

func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject) bool {
func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject) (bool, error) {
window := proj.Spec.SyncWindows.Matches(app)
isManual := false
if app.Status.OperationState != nil {
isManual = !app.Status.OperationState.Operation.InitiatedBy.Automated
}
return !window.CanSync(isManual)
canSync, err := window.CanSync(isManual)
if err != nil {
// prevents sync because sync window has an error
return true, err
}
return !canSync, nil
}

// deriveServiceAccountToImpersonate determines the service account to be used for impersonation for the sync operation.
Expand Down
76 changes: 50 additions & 26 deletions pkg/apis/application/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -2379,11 +2379,11 @@ func (s *SyncWindows) HasWindows() bool {
}

// Active returns a list of sync windows that are currently active
func (s *SyncWindows) Active() *SyncWindows {
func (s *SyncWindows) Active() (*SyncWindows, error) {
return s.active(time.Now())
}

func (s *SyncWindows) active(currentTime time.Time) *SyncWindows {
func (s *SyncWindows) active(currentTime time.Time) (*SyncWindows, error) {
// If SyncWindows.Active() is called outside of a UTC locale, it should be
// first converted to UTC before we scan through the SyncWindows.
currentTime = currentTime.In(time.UTC)
Expand All @@ -2392,8 +2392,14 @@ func (s *SyncWindows) active(currentTime time.Time) *SyncWindows {
var active SyncWindows
specParser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow)
for _, w := range *s {
schedule, _ := specParser.Parse(w.Schedule)
duration, _ := time.ParseDuration(w.Duration)
schedule, sErr := specParser.Parse(w.Schedule)
if sErr != nil {
return nil, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr)
}
duration, dErr := time.ParseDuration(w.Duration)
if dErr != nil {
return nil, fmt.Errorf("cannot parse duration '%s': %w", w.Duration, dErr)
}

// Offset the nextWindow time to consider the timeZone of the sync window
timeZoneOffsetDuration := w.scheduleOffsetByTimeZone()
Expand All @@ -2403,20 +2409,20 @@ func (s *SyncWindows) active(currentTime time.Time) *SyncWindows {
}
}
if len(active) > 0 {
return &active
return &active, nil
}
}
return nil
return nil, nil
}

// InactiveAllows will iterate over the SyncWindows and return all inactive allow windows
// for the current time. If the current time is in an inactive allow window, syncs will
// be denied.
func (s *SyncWindows) InactiveAllows() *SyncWindows {
func (s *SyncWindows) InactiveAllows() (*SyncWindows, error) {
return s.inactiveAllows(time.Now())
}

func (s *SyncWindows) inactiveAllows(currentTime time.Time) *SyncWindows {
func (s *SyncWindows) inactiveAllows(currentTime time.Time) (*SyncWindows, error) {
// If SyncWindows.InactiveAllows() is called outside of a UTC locale, it should be
// first converted to UTC before we scan through the SyncWindows.
currentTime = currentTime.In(time.UTC)
Expand All @@ -2427,21 +2433,27 @@ func (s *SyncWindows) inactiveAllows(currentTime time.Time) *SyncWindows {
for _, w := range *s {
if w.Kind == "allow" {
schedule, sErr := specParser.Parse(w.Schedule)
if sErr != nil {
return nil, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr)
}
duration, dErr := time.ParseDuration(w.Duration)
if dErr != nil {
return nil, fmt.Errorf("cannot parse duration '%s': %w", w.Duration, dErr)
}
// Offset the nextWindow time to consider the timeZone of the sync window
timeZoneOffsetDuration := w.scheduleOffsetByTimeZone()
nextWindow := schedule.Next(currentTime.Add(timeZoneOffsetDuration - duration))

if !nextWindow.Before(currentTime.Add(timeZoneOffsetDuration)) && sErr == nil && dErr == nil {
if !nextWindow.Before(currentTime.Add(timeZoneOffsetDuration)) {
inactive = append(inactive, w)
}
}
}
if len(inactive) > 0 {
return &inactive
return &inactive, nil
}
}
return nil
return nil, nil
}

func (w *SyncWindow) scheduleOffsetByTimeZone() time.Duration {
Expand Down Expand Up @@ -2545,36 +2557,42 @@ func (w *SyncWindows) Matches(app *Application) *SyncWindows {
}

// CanSync returns true if a sync window currently allows a sync. isManual indicates whether the sync has been triggered manually.
func (w *SyncWindows) CanSync(isManual bool) bool {
func (w *SyncWindows) CanSync(isManual bool) (bool, error) {
if !w.HasWindows() {
return true
return true, nil
}

active := w.Active()
active, err := w.Active()
if err != nil {
return false, fmt.Errorf("invalid sync windows: %w", err)
}
hasActiveDeny, manualEnabled := active.hasDeny()

if hasActiveDeny {
if isManual && manualEnabled {
return true
return true, nil
} else {
return false
return false, nil
}
}

if active.hasAllow() {
return true
return true, nil
}

inactiveAllows := w.InactiveAllows()
inactiveAllows, err := w.InactiveAllows()
if err != nil {
return false, fmt.Errorf("invalid sync windows: %w", err)
}
if inactiveAllows.HasWindows() {
if isManual && inactiveAllows.manualEnabled() {
return true
return true, nil
} else {
return false
return false, nil
}
}

return true
return true, nil
}

// hasDeny will iterate over the SyncWindows and return if a deny window is found and if
Expand Down Expand Up @@ -2629,24 +2647,30 @@ func (w *SyncWindows) manualEnabled() bool {
}

// Active returns true if the sync window is currently active
func (w SyncWindow) Active() bool {
func (w SyncWindow) Active() (bool, error) {
return w.active(time.Now())
}

func (w SyncWindow) active(currentTime time.Time) bool {
func (w SyncWindow) active(currentTime time.Time) (bool, error) {
// If SyncWindow.Active() is called outside of a UTC locale, it should be
// first converted to UTC before search
currentTime = currentTime.UTC()

specParser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow)
schedule, _ := specParser.Parse(w.Schedule)
duration, _ := time.ParseDuration(w.Duration)
schedule, sErr := specParser.Parse(w.Schedule)
if sErr != nil {
return false, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr)
}
duration, dErr := time.ParseDuration(w.Duration)
if dErr != nil {
return false, fmt.Errorf("cannot parse duration '%s': %w", w.Duration, dErr)
}

// Offset the nextWindow time to consider the timeZone of the sync window
timeZoneOffsetDuration := w.scheduleOffsetByTimeZone()
nextWindow := schedule.Next(currentTime.Add(timeZoneOffsetDuration - duration))

return nextWindow.Before(currentTime.Add(timeZoneOffsetDuration))
return nextWindow.Before(currentTime.Add(timeZoneOffsetDuration)), nil
}

// Update updates a sync window's settings with the given parameter
Expand Down
Loading

0 comments on commit 9f556a5

Please sign in to comment.