From cacb06a5e5aff897308bf87eba851a929b91417f Mon Sep 17 00:00:00 2001 From: Kunho Lee Date: Thu, 24 Oct 2024 21:13:37 +0900 Subject: [PATCH] fix: check err before use schedule and duration (#20043) (#20371) --- 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 613de6804af6c..464a7cfc308e3 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -572,8 +572,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 @@ -582,13 +582,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 5b9c07452efdd..c92a0da96a356 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1612,7 +1612,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) setOpMs = opMS if syncErrCond != nil { diff --git a/controller/sync.go b/controller/sync.go index c182935acb47d..8064e1b5dae63 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -167,12 +167,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 { @@ -533,11 +539,16 @@ 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 } diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 1cf1aaece81b2..2aa0c0ee34b81 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -2263,11 +2263,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) @@ -2276,8 +2276,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() @@ -2287,20 +2293,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) @@ -2311,21 +2317,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 { @@ -2429,36 +2441,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 @@ -2513,24 +2531,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 fcfd682536a46..22ef37badb24f 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -1631,7 +1631,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 { @@ -1658,6 +1660,7 @@ func TestSyncWindows_Active(t *testing.T) { currentTime time.Time matchingIndex int expectedLength int + isErr bool }{ { name: "MatchFirst", @@ -1765,11 +1768,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{} } @@ -1786,7 +1814,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 { @@ -1813,6 +1843,7 @@ func TestSyncWindows_InactiveAllows(t *testing.T) { currentTime time.Time matchingIndex int expectedLength int + isErr bool }{ { name: "MatchFirst", @@ -1938,11 +1969,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{} } @@ -2053,9 +2107,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) { @@ -2064,9 +2119,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) { @@ -2078,9 +2134,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) { @@ -2091,9 +2148,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) { @@ -2104,9 +2162,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) { @@ -2117,9 +2176,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) { @@ -2131,9 +2191,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) { @@ -2145,9 +2206,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) { @@ -2158,9 +2220,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) { @@ -2171,9 +2234,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) { @@ -2184,9 +2248,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) { @@ -2197,9 +2262,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) { @@ -2211,9 +2277,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) { @@ -2225,9 +2292,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) { @@ -2239,9 +2307,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) { @@ -2252,9 +2321,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) { @@ -2265,9 +2335,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) { @@ -2278,9 +2349,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) { @@ -2291,9 +2363,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) { @@ -2307,9 +2380,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) { @@ -2323,9 +2397,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) { @@ -2337,9 +2412,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) { @@ -2351,9 +2427,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) { @@ -2365,9 +2442,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) }) } @@ -2417,8 +2509,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 { @@ -2443,6 +2536,7 @@ func TestSyncWindow_Active(t *testing.T) { syncWindow SyncWindow currentTime time.Time expectedResult bool + isErr bool }{ { name: "Allow-active", @@ -2492,11 +2586,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) }) } @@ -2608,6 +2735,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 dfbe4af2d1ede..a2eb7d66c1bb6 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1891,7 +1891,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") } @@ -2608,10 +2612,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 74e7cf7bf0008..44f5f69bc62ff 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 {