diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 8c32ce34399e4..4c262a67fddca 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -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 @@ -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" diff --git a/cmd/argocd/commands/projectwindows.go b/cmd/argocd/commands/projectwindows.go index d824222306419..b04615e22fd41 100644 --- a/cmd/argocd/commands/projectwindows.go +++ b/cmd/argocd/commands/projectwindows.go @@ -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, diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 2263269c7385e..f56480750a30a 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1698,7 +1698,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 { diff --git a/controller/sync.go b/controller/sync.go index 522155977093e..5f9ce3985d2ab 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -179,12 +179,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 { @@ -579,13 +585,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. diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index d68982e6b3b75..75cfcce5c444a 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -2383,11 +2383,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) @@ -2398,13 +2398,11 @@ func (s *SyncWindows) active(currentTime time.Time) *SyncWindows { for _, w := range *s { schedule, sErr := specParser.Parse(w.Schedule) if sErr != nil { - log.Warnf("invalid sync window schedule '%v': %s", w.Schedule, sErr) - continue + return nil, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr) } duration, dErr := time.ParseDuration(w.Duration) if dErr != nil { - log.Warnf("invalid sync window duration '%v': %s", w.Duration, dErr) - continue + return nil, fmt.Errorf("cannot parse duration '%s': %w", w.Duration, dErr) } // Offset the nextWindow time to consider the timeZone of the sync window @@ -2415,20 +2413,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) @@ -2440,13 +2438,11 @@ func (s *SyncWindows) inactiveAllows(currentTime time.Time) *SyncWindows { if w.Kind == "allow" { schedule, sErr := specParser.Parse(w.Schedule) if sErr != nil { - log.Warnf("invalid sync window schedule '%v': %s", w.Schedule, sErr) - continue + return nil, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr) } duration, dErr := time.ParseDuration(w.Duration) if dErr != nil { - log.Warnf("invalid sync window duration '%v': %s", w.Duration, dErr) - continue + 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() @@ -2458,10 +2454,10 @@ func (s *SyncWindows) inactiveAllows(currentTime time.Time) *SyncWindows { } } if len(inactive) > 0 { - return &inactive + return &inactive, nil } } - return nil + return nil, nil } func (w *SyncWindow) scheduleOffsetByTimeZone() time.Duration { @@ -2565,36 +2561,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 @@ -2649,11 +2651,11 @@ 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() @@ -2661,20 +2663,18 @@ func (w SyncWindow) active(currentTime time.Time) bool { specParser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) schedule, sErr := specParser.Parse(w.Schedule) if sErr != nil { - log.Warnf("invalid sync window schedule '%v': %s", w.Schedule, sErr) - return false + return false, fmt.Errorf("cannot parse schedule '%s': %w", w.Schedule, sErr) } duration, dErr := time.ParseDuration(w.Duration) if dErr != nil { - log.Warnf("invalid sync window duration '%v': %s", w.Duration, dErr) - return false + 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 diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 3413666f4e9e1..a0c0a0fb22d31 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -1792,7 +1792,9 @@ func TestSyncWindows_HasWindows(t *testing.T) { func TestSyncWindows_Active(t *testing.T) { t.Run("WithTestProject", func(t *testing.T) { proj := newTestProjectWithSyncWindows() - assert.Len(t, *proj.Spec.SyncWindows.Active(), 1) + activeWindows, err := proj.Spec.SyncWindows.Active() + assert.Nil(t, err) + assert.Len(t, *activeWindows, 1) }) syncWindow := func(kind string, schedule string, duration string, timeZone string) *SyncWindow { @@ -1819,6 +1821,7 @@ func TestSyncWindows_Active(t *testing.T) { currentTime time.Time matchingIndex int expectedLength int + isErr bool }{ { name: "MatchFirst", @@ -1934,6 +1937,7 @@ func TestSyncWindows_Active(t *testing.T) { }, currentTime: timeWithHour(12, time.UTC), expectedLength: 0, + isErr: true, }, { name: "MatchNone-InvalidDuration", @@ -1943,12 +1947,18 @@ func TestSyncWindows_Active(t *testing.T) { }, currentTime: timeWithHour(12, time.UTC), expectedLength: 0, + isErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := tt.syncWindow.active(tt.currentTime) + result, err := tt.syncWindow.active(tt.currentTime) + if tt.isErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + } if result == nil { result = &SyncWindows{} } @@ -1965,7 +1975,9 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { t.Run("WithTestProject", func(t *testing.T) { proj := newTestProjectWithSyncWindows() proj.Spec.SyncWindows[0].Schedule = "0 0 1 1 1" - assert.Len(t, *proj.Spec.SyncWindows.InactiveAllows(), 1) + inactiveAllowWindows, err := proj.Spec.SyncWindows.InactiveAllows() + assert.Nil(t, err) + assert.Len(t, *inactiveAllowWindows, 1) }) syncWindow := func(kind string, schedule string, duration string, timeZone string) *SyncWindow { @@ -1992,6 +2004,7 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { currentTime time.Time matchingIndex int expectedLength int + isErr bool }{ { name: "MatchFirst", @@ -2124,6 +2137,7 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { }, currentTime: timeWithHour(17, time.UTC), expectedLength: 0, + isErr: true, }, { name: "MatchNone-InvalidDuration", @@ -2132,12 +2146,18 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { }, currentTime: timeWithHour(17, time.UTC), expectedLength: 0, + isErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := tt.syncWindow.inactiveAllows(tt.currentTime) + result, err := tt.syncWindow.inactiveAllows(tt.currentTime) + if tt.isErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + } if result == nil { result = &SyncWindows{} } @@ -2248,9 +2268,10 @@ func TestSyncWindows_CanSync(t *testing.T) { proj := newProjectBuilder().withInactiveDenyWindow(true).build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will allow manual sync if inactive-deny-window set with manual false", func(t *testing.T) { @@ -2259,9 +2280,10 @@ func TestSyncWindows_CanSync(t *testing.T) { proj := newProjectBuilder().withInactiveDenyWindow(false).build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny manual sync if one inactive-allow-windows set with manual false", func(t *testing.T) { @@ -2273,9 +2295,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync if on active-allow-window set with manual true", func(t *testing.T) { @@ -2286,9 +2309,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will allow manual sync if on active-allow-window set with manual false", func(t *testing.T) { @@ -2299,9 +2323,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will allow auto sync if on active-allow-window", func(t *testing.T) { @@ -2312,9 +2337,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will allow manual sync active-allow and inactive-deny", func(t *testing.T) { @@ -2326,9 +2352,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will allow auto sync active-allow and inactive-deny", func(t *testing.T) { @@ -2340,9 +2367,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny manual sync inactive-allow", func(t *testing.T) { @@ -2353,9 +2381,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync inactive-allow", func(t *testing.T) { @@ -2366,9 +2395,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync inactive-allow with ManualSync enabled", func(t *testing.T) { @@ -2379,9 +2409,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny auto sync inactive-allow with ManualSync enabled", func(t *testing.T) { @@ -2392,9 +2423,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny manual sync with inactive-allow and inactive-deny", func(t *testing.T) { @@ -2406,9 +2438,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync with inactive-allow and inactive-deny", func(t *testing.T) { @@ -2420,9 +2453,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will allow auto sync with active-allow and inactive-allow", func(t *testing.T) { @@ -2434,9 +2468,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny manual sync with active-deny", func(t *testing.T) { @@ -2447,9 +2482,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync with active-deny", func(t *testing.T) { @@ -2460,9 +2496,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync with active-deny with ManualSync enabled", func(t *testing.T) { @@ -2473,9 +2510,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny auto sync with active-deny with ManualSync enabled", func(t *testing.T) { @@ -2486,9 +2524,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny manual sync with many active-deny having one with ManualSync disabled", func(t *testing.T) { @@ -2502,9 +2541,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync with many active-deny having one with ManualSync disabled", func(t *testing.T) { @@ -2518,9 +2558,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will deny manual sync with active-deny and active-allow windows with ManualSync disabled", func(t *testing.T) { @@ -2532,9 +2573,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync with active-deny and active-allow windows with ManualSync enabled", func(t *testing.T) { @@ -2546,9 +2588,10 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(true) + canSync, err := proj.Spec.SyncWindows.CanSync(true) // then + assert.Nil(t, err) assert.True(t, canSync) }) t.Run("will deny auto sync with active-deny and active-allow windows with ManualSync enabled", func(t *testing.T) { @@ -2560,9 +2603,24 @@ func TestSyncWindows_CanSync(t *testing.T) { build() // when - canSync := proj.Spec.SyncWindows.CanSync(false) + canSync, err := proj.Spec.SyncWindows.CanSync(false) + + // then + assert.Nil(t, err) + assert.False(t, canSync) + }) + t.Run("will deny and return error with invalid windows", func(t *testing.T) { + // given + t.Parallel() + proj := newProjectBuilder(). + withInvalidWindows(). + build() + + // when + canSync, err := proj.Spec.SyncWindows.CanSync(false) // then + assert.NotNil(t, err) assert.False(t, canSync) }) } @@ -2612,8 +2670,9 @@ func TestSyncWindows_hasAllow(t *testing.T) { func TestSyncWindow_Active(t *testing.T) { window := &SyncWindow{Schedule: "* * * * *", Duration: "1h"} t.Run("ActiveWindow", func(t *testing.T) { - window.Active() - assert.True(t, window.Active()) + isActive, err := window.Active() + assert.Nil(t, err) + assert.True(t, isActive) }) syncWindow := func(kind string, schedule string, duration string) SyncWindow { @@ -2638,6 +2697,7 @@ func TestSyncWindow_Active(t *testing.T) { syncWindow SyncWindow currentTime time.Time expectedResult bool + isErr bool }{ { name: "Allow-active", @@ -2692,30 +2752,39 @@ func TestSyncWindow_Active(t *testing.T) { syncWindow: syncWindow("allow", "* 10 * * 7", "2h"), currentTime: timeWithHour(11, time.UTC), expectedResult: false, + isErr: true, }, { name: "Deny-inactive-InvalidSchedule", syncWindow: syncWindow("deny", "* 10 * * 7", "2h"), currentTime: timeWithHour(11, time.UTC), expectedResult: false, + isErr: true, }, { name: "Allow-inactive-InvalidDuration", syncWindow: syncWindow("allow", "* 10 * * *", "2a"), currentTime: timeWithHour(11, time.UTC), expectedResult: false, + isErr: true, }, { name: "Deny-inactive-InvalidDuration", syncWindow: syncWindow("deny", "* 10 * * *", "2a"), currentTime: timeWithHour(11, time.UTC), expectedResult: false, + isErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := tt.syncWindow.active(tt.currentTime) + result, err := tt.syncWindow.active(tt.currentTime) + if tt.isErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + } assert.Equal(t, tt.expectedResult, result) }) } @@ -2827,6 +2896,16 @@ func (b *projectBuilder) withInactiveDenyWindow(allowManual bool) *projectBuilde return b } +func (b *projectBuilder) withInvalidWindows() *projectBuilder { + b.proj.Spec.SyncWindows = append(b.proj.Spec.SyncWindows, + newSyncWindow("allow", "* 10 * * 7", false), + newSyncWindow("deny", "* 10 * * 7", false), + newSyncWindow("allow", "* 10 * * 7", true), + newSyncWindow("deny", "* 10 * * 7", true), + ) + return b +} + func inactiveCronSchedule() string { hourPlus10, _, _ := time.Now().Add(10 * time.Hour).Clock() return fmt.Sprintf("0 %d * * *", hourPlus10) diff --git a/server/application/application.go b/server/application/application.go index 3d67ea2601f80..e15e586a824b1 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1898,7 +1898,11 @@ func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncR s.inferResourcesStatusHealth(a) - if !proj.Spec.SyncWindows.Matches(a).CanSync(true) { + canSync, err := proj.Spec.SyncWindows.Matches(a).CanSync(true) + if err != nil { + return a, status.Errorf(codes.PermissionDenied, "cannot sync: invalid sync window: %v", err) + } + if !canSync { return a, status.Errorf(codes.PermissionDenied, "cannot sync: blocked by sync window") } @@ -2615,10 +2619,17 @@ func (s *Server) GetApplicationSyncWindows(ctx context.Context, q *application.A } windows := proj.Spec.SyncWindows.Matches(a) - sync := windows.CanSync(true) + sync, err := windows.CanSync(true) + if err != nil { + return nil, fmt.Errorf("invalid sync windows: %w", err) + } + activeWindows, err := windows.Active() + if err != nil { + return nil, fmt.Errorf("invalid sync windows: %w", err) + } res := &application.ApplicationSyncWindowsResponse{ - ActiveWindows: convertSyncWindows(windows.Active()), + ActiveWindows: convertSyncWindows(activeWindows), AssignedWindows: convertSyncWindows(windows), CanSync: &sync, } diff --git a/server/project/project.go b/server/project/project.go index 02d393564ddf0..62487b268a705 100644 --- a/server/project/project.go +++ b/server/project/project.go @@ -525,7 +525,10 @@ func (s *Server) GetSyncWindowsState(ctx context.Context, q *project.SyncWindows res := &project.SyncWindowsResponse{} - windows := proj.Spec.SyncWindows.Active() + windows, err := proj.Spec.SyncWindows.Active() + if err != nil { + return nil, err + } if windows.HasWindows() { res.Windows = *windows } else {