From 745810d4eefcd78ee1b75df747138a69f1bae087 Mon Sep 17 00:00:00 2001 From: Kunho Lee Date: Sun, 13 Oct 2024 02:32:46 +0900 Subject: [PATCH] fix: check err before use schedule and duration (#20043) * fix: check err before use schedule and duration Signed-off-by: daengdaengLee * test: add tests for invalid schedule and duration Signed-off-by: daengdaengLee * feat: change to return error when sync window is invalid Signed-off-by: daengdaengLee * fix: use assert.Error or assert.NoError Signed-off-by: daengdaengLee * fix: use require instead of assert Signed-off-by: daengdaengLee --------- Signed-off-by: daengdaengLee Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --- cmd/argocd/commands/app.go | 11 +- cmd/argocd/commands/projectwindows.go | 3 +- controller/appcontroller.go | 3 +- controller/sync.go | 25 ++- pkg/apis/application/v1alpha1/types.go | 76 +++++--- pkg/apis/application/v1alpha1/types_test.go | 199 +++++++++++++++++--- server/application/application.go | 17 +- server/project/project.go | 5 +- 8 files changed, 264 insertions(+), 75 deletions(-) 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 6db7d2dc3ed23..e7ef632bc478a 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1696,7 +1696,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 a0abc3d5573c0..3963f0afdf1a0 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -2386,11 +2386,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) @@ -2399,8 +2399,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() @@ -2410,20 +2416,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) @@ -2434,21 +2440,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 { @@ -2552,36 +2564,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 @@ -2636,24 +2654,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 diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index d614711b811db..1df2f5c74baea 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() + require.NoError(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", @@ -1926,11 +1929,36 @@ func TestSyncWindows_Active(t *testing.T) { matchingIndex: 0, expectedLength: 1, }, + { + name: "MatchNone-InvalidSchedule", + syncWindow: SyncWindows{ + syncWindow("allow", "* 10 * * 7", "3h", ""), + syncWindow("allow", "* 11 * * 7", "3h", ""), + }, + currentTime: timeWithHour(12, time.UTC), + expectedLength: 0, + isErr: true, + }, + { + name: "MatchNone-InvalidDuration", + syncWindow: SyncWindows{ + syncWindow("allow", "* 10 * * *", "3a", ""), + syncWindow("allow", "* 11 * * *", "3a", ""), + }, + 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 { + require.Error(t, err) + } else { + require.NoError(t, err) + } if result == nil { result = &SyncWindows{} } @@ -1947,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() + require.NoError(t, err) + assert.Len(t, *inactiveAllowWindows, 1) }) syncWindow := func(kind string, schedule string, duration string, timeZone string) *SyncWindow { @@ -1974,6 +2004,7 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { currentTime time.Time matchingIndex int expectedLength int + isErr bool }{ { name: "MatchFirst", @@ -2099,11 +2130,34 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { matchingIndex: 0, expectedLength: 1, }, + { + name: "MatchNone-InvalidSchedule", + syncWindow: SyncWindows{ + syncWindow("allow", "* 10 * * 7", "2h", ""), + }, + currentTime: timeWithHour(17, time.UTC), + expectedLength: 0, + isErr: true, + }, + { + name: "MatchNone-InvalidDuration", + syncWindow: SyncWindows{ + syncWindow("allow", "* 10 * * *", "2a", ""), + }, + 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 { + require.Error(t, err) + } else { + require.NoError(t, err) + } if result == nil { result = &SyncWindows{} } @@ -2214,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will allow manual sync if inactive-deny-window set with manual false", func(t *testing.T) { @@ -2225,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 + require.NoError(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) { @@ -2239,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 + require.NoError(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) { @@ -2252,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 + require.NoError(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) { @@ -2265,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will allow auto sync if on active-allow-window", func(t *testing.T) { @@ -2278,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will allow manual sync active-allow and inactive-deny", func(t *testing.T) { @@ -2292,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will allow auto sync active-allow and inactive-deny", func(t *testing.T) { @@ -2306,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will deny manual sync inactive-allow", func(t *testing.T) { @@ -2319,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync inactive-allow", func(t *testing.T) { @@ -2332,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync inactive-allow with ManualSync enabled", func(t *testing.T) { @@ -2345,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will deny auto sync inactive-allow with ManualSync enabled", func(t *testing.T) { @@ -2358,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will deny manual sync with inactive-allow and inactive-deny", func(t *testing.T) { @@ -2372,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync with inactive-allow and inactive-deny", func(t *testing.T) { @@ -2386,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will allow auto sync with active-allow and inactive-allow", func(t *testing.T) { @@ -2400,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will deny manual sync with active-deny", func(t *testing.T) { @@ -2413,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will deny auto sync with active-deny", func(t *testing.T) { @@ -2426,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 + require.NoError(t, err) assert.False(t, canSync) }) t.Run("will allow manual sync with active-deny with ManualSync enabled", func(t *testing.T) { @@ -2439,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 + require.NoError(t, err) assert.True(t, canSync) }) t.Run("will deny auto sync with active-deny with ManualSync enabled", func(t *testing.T) { @@ -2452,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 + require.NoError(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) { @@ -2468,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 + require.NoError(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) { @@ -2484,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 + require.NoError(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) { @@ -2498,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 + require.NoError(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) { @@ -2512,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 + require.NoError(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) { @@ -2526,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 + require.NoError(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 + require.Error(t, err) assert.False(t, canSync) }) } @@ -2578,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() + require.NoError(t, err) + assert.True(t, isActive) }) syncWindow := func(kind string, schedule string, duration string) SyncWindow { @@ -2604,6 +2697,7 @@ func TestSyncWindow_Active(t *testing.T) { syncWindow SyncWindow currentTime time.Time expectedResult bool + isErr bool }{ { name: "Allow-active", @@ -2653,11 +2747,44 @@ func TestSyncWindow_Active(t *testing.T) { currentTime: timeWithHour(13-4, utcM4Zone), expectedResult: false, }, + { + name: "Allow-inactive-InvalidSchedule", + 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 { + require.Error(t, err) + } else { + require.NoError(t, err) + } assert.Equal(t, tt.expectedResult, result) }) } @@ -2769,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 dbb6a85bd2d50..055bdca54f2b9 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 {