Skip to content

Add shared software update schedule validation logic #38016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sgress454 merged 5 commits into from
Jan 8, 2026
Merged

Add shared software update schedule validation logic #38016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c24a3a3
validation
sgress454 Jan 7, 2026
bacd072
update and use in code
sgress454 Jan 7, 2026
bf27375
update in api layer
sgress454 Jan 7, 2026
902cc38
don't update schedule when enabled is `false`
sgress454 Jan 8, 2026
4721be2
updates from review comments
sgress454 Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ee/server/service/vpp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,20 @@ func (svc *Service) UpdateAppStoreApp(ctx context.Context, titleID uint, teamID
return nil, nil, err
}

// If there's an auto-update config, validate it.
// Note that applying this config is done in a separate service method.
schedule := fleet.SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: payload.AutoUpdateEnabled,
AutoUpdateStartTime: payload.AutoUpdateStartTime,
AutoUpdateEndTime: payload.AutoUpdateEndTime,
},
}
Comment on lines 783 to +792
Copy link
Contributor Author

@sgress454 sgress454 Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly tortured. I deliberately kept the auto-update config out of svc.UpdateAppStoreApp() because it has nothing to do with app store apps per se. But the endpoint runs this method first, so if we waited to do the validation inside of svc.UpdateSoftwareTitleAutoUpdateConfig() then we could end up in a situation where we update the app and then fail the API call because the schedule is invalid.

The simplest thing would be to do the validation in the endpoint handler, but our pattern is to do everything in service methods and in fact doing validations at the endpoint is not well supported; if you try to return a validation error you'll just end up with a 500 error because it'll fail authz (which is done in the service methods).

Tempted to just give up and refactor so that the auto-update schedule is handled at the end of UpdateAppStoreApp() after all, but to keep this low-touch I'm going with this for now.


if err := schedule.WindowIsValid(); err != nil {
return nil, nil, ctxerr.Wrap(ctx, err, "UpdateAppStoreApp: validating auto-update schedule")
}

var teamName string
if teamID != nil && *teamID != 0 {
tm, err := svc.ds.TeamLite(ctx, *teamID)
Expand Down
31 changes: 12 additions & 19 deletions server/datastore/mysql/software_titles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,29 +832,22 @@ WHERE
}

func (ds *Datastore) UpdateSoftwareTitleAutoUpdateConfig(ctx context.Context, titleID uint, teamID uint, config fleet.SoftwareAutoUpdateConfig) error {
// Validate start and end time.
invalidTimeErr := "invalid auto-update time format: must be in HH:MM 24-hour format"
for _, t := range []*string{config.AutoUpdateStartTime, config.AutoUpdateEndTime} {
if t == nil {
if config.AutoUpdateEnabled != nil && *config.AutoUpdateEnabled {
return fleet.NewInvalidArgumentError("auto_update_time", invalidTimeErr)
}
continue
}
duration, err := time.Parse("15:04", *t)
if err != nil {
return fleet.NewInvalidArgumentError("auto_update_time", invalidTimeErr)
}
if duration.Hour() < 0 || duration.Hour() > 23 || duration.Minute() < 0 || duration.Minute() > 59 {
return fleet.NewInvalidArgumentError("auto_update_time", invalidTimeErr)
}
// Validate schedule if enabled.
schedule := fleet.SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: config.AutoUpdateEnabled,
AutoUpdateStartTime: config.AutoUpdateStartTime,
AutoUpdateEndTime: config.AutoUpdateEndTime,
},
}
if err := schedule.WindowIsValid(); err != nil {
return ctxerr.Wrap(ctx, err, "validating auto-update schedule")
}

var startTime, endTime string
if config.AutoUpdateStartTime != nil {
if config.AutoUpdateEnabled != nil && *config.AutoUpdateEnabled && config.AutoUpdateStartTime != nil {
startTime = *config.AutoUpdateStartTime
}
if config.AutoUpdateEndTime != nil {
if config.AutoUpdateEnabled != nil && *config.AutoUpdateEnabled && config.AutoUpdateEndTime != nil {
endTime = *config.AutoUpdateEndTime
}

Expand Down
24 changes: 17 additions & 7 deletions server/datastore/mysql/software_titles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2655,7 +2655,7 @@ func testUpdateAutoUpdateConfig(t *testing.T, ds *Datastore) {
AutoUpdateEndTime: ptr.String(endTime),
})
require.Error(t, err)
require.Contains(t, err.Error(), "invalid auto-update time format")
require.Contains(t, err.Error(), "Error parsing start time")

// Attempt to enable auto-update with invalid end time.
startTime = "12:00"
Expand All @@ -2666,7 +2666,18 @@ func testUpdateAutoUpdateConfig(t *testing.T, ds *Datastore) {
AutoUpdateEndTime: ptr.String(endTime),
})
require.Error(t, err)
require.Contains(t, err.Error(), "invalid auto-update time format")
require.Contains(t, err.Error(), "Error parsing end time")

// Attempt to enable auto-update with less than an hour between start and end time.
startTime = "12:00"
endTime = "12:30"
err = ds.UpdateSoftwareTitleAutoUpdateConfig(ctx, titleID, *teamID, fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String(startTime),
AutoUpdateEndTime: ptr.String(endTime),
})
require.Error(t, err)
require.Contains(t, err.Error(), "The update window must be at least one hour long")

// Enable auto-update.
startTime = "02:00"
Expand All @@ -2687,6 +2698,7 @@ func testUpdateAutoUpdateConfig(t *testing.T, ds *Datastore) {
require.Equal(t, endTime, *titleResult.AutoUpdateEndTime)

// Add valid, disabled auto-update schedule for the other VPP app.
// The schedule should be ignored since it's disabled, but it should still be created.
err = ds.UpdateSoftwareTitleAutoUpdateConfig(ctx, title2ID, *teamID, fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(false),
AutoUpdateStartTime: ptr.String(startTime),
Expand All @@ -2706,8 +2718,8 @@ func testUpdateAutoUpdateConfig(t *testing.T, ds *Datastore) {
require.Equal(t, title2ID, schedules[1].TitleID)
require.Equal(t, team1.ID, schedules[1].TeamID)
require.False(t, *schedules[1].AutoUpdateEnabled)
require.Equal(t, startTime, *schedules[1].AutoUpdateStartTime)
require.Equal(t, endTime, *schedules[1].AutoUpdateEndTime)
require.Equal(t, "", *schedules[1].AutoUpdateStartTime)
require.Equal(t, "", *schedules[1].AutoUpdateEndTime)

// Filter by enabled only.
schedules, err = ds.ListSoftwareAutoUpdateSchedules(ctx, *teamID, "ipados_apps", fleet.SoftwareAutoUpdateScheduleFilter{
Expand All @@ -2727,9 +2739,7 @@ func testUpdateAutoUpdateConfig(t *testing.T, ds *Datastore) {

// Disable auto-update.
err = ds.UpdateSoftwareTitleAutoUpdateConfig(ctx, titleID, *teamID, fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(false),
AutoUpdateStartTime: nil,
AutoUpdateEndTime: nil,
AutoUpdateEnabled: ptr.Bool(false),
})
require.NoError(t, err)

Expand Down
30 changes: 30 additions & 0 deletions server/fleet/software.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,36 @@ type SoftwareAutoUpdateSchedule struct {
SoftwareAutoUpdateConfig
}

func (s SoftwareAutoUpdateSchedule) WindowIsValid() error {
if s.AutoUpdateEnabled == nil || !*s.AutoUpdateEnabled {
return nil
}
Comment on lines 274 to 276
Copy link
Member

@lucasmrod lucasmrod Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A e.g. 30m window can end up on the database by setting auto_update_enabled: false:

curl -vvv -k -X PATCH -H "Authorization: Bearer $API_TOKEN" https://localhost:8080/api/latest/fleet/software/titles/3362/app_store_app -d '{"team_id": 6,"auto_update_enabled": false,"auto_update_start_time": "00:00","auto_update_end_time": "00:30","labels_exclude_any": [],"labels_include_any": []}'

In which case we can drop the bool return and just rely on error.

Copy link
Member

@nulmete nulmete Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to update just the start and end time without flipping enabled? OR are all the three values always sent?

  • If we always expect the three values to be set, I'd move s.AutoUpdateEnabled == nil check to L277 as another OR condition.
  • If we support updating just the times without setting enabled, perhaps this check should be done in L299 after the start and end time are validated.

Thoughts?

Copy link
Contributor Author

@sgress454 sgress454 Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to update just the start and end time without flipping enabled? OR are all the three values always sent?

The more relevant point is that it's not possible to flip enabled to true without setting a start or end time. Which somewhat violates the spirit of a PATCH endpoint -- you can squint and look at the schedule as a single entity that you're patching, but it's not ideal. To fix that requires a bit of lifting -- we'd have to load the current schedule before validating the incoming one.

The lowest-touch solution here is to ignore start and end time values when enabled is false. We're already ignoring the config completely when auto_update_enabled is nil. The current API still won't let you toggle the enabled state independently of the schedule or vice versa. We can document that and move on, and if we decide to make the API more flexible later it won't be a breaking change. I'll do this for now and get a product opinion.

if s.AutoUpdateStartTime == nil || s.AutoUpdateEndTime == nil || *s.AutoUpdateStartTime == "" || *s.AutoUpdateEndTime == "" {
return errors.New("Start and end time must both be set")
}
// Validate that the times are in HH:MM format.
// Note that durations can be arbitrarily long, but parsing in this way
// automatically validates that the hours are between 0 and 23 and the minutes are between 0 and 59.
startDuration, err := time.Parse("15:04", *s.AutoUpdateStartTime)
if err != nil {
return fmt.Errorf("Error parsing start time: %w", err)
}
endDuration, err := time.Parse("15:04", *s.AutoUpdateEndTime)
if err != nil {
return fmt.Errorf("Error parsing end time: %w", err)
}
// Validate that the window is at least one hour long.
// If the end time is less than the start time, the window wraps to the next day, so we need to add 24 hours to the end time in that case.
if endDuration.Before(startDuration) {
endDuration = endDuration.Add(24 * time.Hour)
}
if endDuration.Sub(startDuration) < time.Hour {
return errors.New("The update window must be at least one hour long")
}

return nil
}

type SoftwareAutoUpdateScheduleFilter struct {
Enabled *bool
}
Expand Down
162 changes: 162 additions & 0 deletions server/fleet/software_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,165 @@ func TestHostSoftwareEntryMarshalJSON(t *testing.T) {

assert.JSONEq(t, expectedJSON, string(data))
}

func TestAutoUpdateScheduleValidation(t *testing.T) {
testCases := []struct {
name string
schedule SoftwareAutoUpdateSchedule
isValid bool
}{
{
name: "schedule disabled",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(false),
AutoUpdateStartTime: nil,
AutoUpdateEndTime: nil,
},
},
isValid: true,
},
{
name: "missing start time",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: nil,
AutoUpdateEndTime: ptr.String("15:30"),
},
},
isValid: false,
},
{
name: "missing end time",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: nil,
},
},
isValid: false,
},
{
name: "empty start time",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String(""),
AutoUpdateEndTime: ptr.String("15:30"),
},
},
isValid: false,
},
{
name: "empty end time",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: ptr.String(""),
},
},
isValid: false,
},
{
name: "valid schedule",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: ptr.String("15:30"),
},
},
isValid: true,
},
{
name: "valid schedule (wrapped around midnight)",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("23:30"),
AutoUpdateEndTime: ptr.String("00:30"),
},
},
isValid: true,
},
{
name: "start time invalid",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("invalid"),
AutoUpdateEndTime: ptr.String("15:30"),
},
},
isValid: false,
},
{
name: "end time invalid",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: ptr.String("invalid"),
},
},
isValid: false,
},
{
name: "start time hour out of range",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("24:00"),
AutoUpdateEndTime: ptr.String("15:30"),
},
},
isValid: false,
},
{
name: "end time hour out of range",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: ptr.String("24:00"),
},
},
isValid: false,
},
{
name: "window is less than one hour",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("14:30"),
AutoUpdateEndTime: ptr.String("15:29"),
},
},
isValid: false,
},
{
name: "window is less than one hour (wrapped around midnight)",
schedule: SoftwareAutoUpdateSchedule{
SoftwareAutoUpdateConfig: SoftwareAutoUpdateConfig{
AutoUpdateEnabled: ptr.Bool(true),
AutoUpdateStartTime: ptr.String("23:30"),
AutoUpdateEndTime: ptr.String("00:29"),
},
},
isValid: false,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.schedule.WindowIsValid()
if tc.isValid {
assert.NoError(t, err)
} else {
assert.Error(t, err)
}
})
}
}
5 changes: 5 additions & 0 deletions server/service/vpp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ func updateAppStoreAppEndpoint(ctx context.Context, request interface{}, svc fle
Categories: req.Categories,
Configuration: req.Configuration,
DisplayName: req.DisplayName,
SoftwareAutoUpdateConfig: fleet.SoftwareAutoUpdateConfig{
AutoUpdateEnabled: req.AutoUpdateEnabled,
AutoUpdateStartTime: req.AutoUpdateStartTime,
AutoUpdateEndTime: req.AutoUpdateEndTime,
},
})
if err != nil {
return updateAppStoreAppResponse{Err: err}, nil
Expand Down
Loading