Add shared software update schedule validation logic #38016

sgress454 · 2026-01-07T23:54:43Z

Related issue: For #33391

Details

This PR adds a shared method for validating a software auto-update configuration, and updates the datastore and API handler methods to use it.

Checklist for submitter

Testing

Added/updated automated tests
QA'd all new/changed functionality manually

Tested in the UI (to ensure valid calls still work) and via API calls (to test validation)

sgress454 · 2026-01-08T00:04:17Z

ee/server/service/vpp.go


+	// 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,
+		},
+	}


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.

codecov · 2026-01-08T00:23:15Z

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.82%. Comparing base (bc0c7f1) to head (4721be2).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
ee/server/service/vpp.go	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #38016   +/-   ##
=======================================
  Coverage   65.82%   65.82%           
=======================================
  Files        2387     2387           
  Lines      190158   190187   +29     
  Branches     8428     8428           
=======================================
+ Hits       125168   125200   +32     
+ Misses      53609    53605    -4     
- Partials    11381    11382    +1

Flag	Coverage Δ
backend	`67.67% <94.87%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lucasmrod

LGTM.

Added two questions/comments.

lucasmrod · 2026-01-08T09:26:30Z

server/fleet/software.go

+	if s.AutoUpdateEnabled == nil || !*s.AutoUpdateEnabled {
+		return true, nil
+	}


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.

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?

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.

server/service/vpp.go

nulmete

Logic LGTM, let me know what you think about my comments

server/fleet/software.go

nulmete · 2026-01-08T11:51:16Z

server/fleet/software.go

+		return false, errors.New("The update window must be at least one hour long")
+	}
+
+	return true, nil


I think just returning an error would make sense, and it would also simplify the check in the consumers (just check for err rather than ok + err)

I've seen it both ways but I agree there's no particular benefit in having both, I'm fine updating it.

server/fleet/software.go

nulmete · 2026-01-08T12:02:09Z

server/fleet/software.go

+	if s.AutoUpdateEnabled == nil || !*s.AutoUpdateEnabled {
+		return true, nil
+	}


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?

lucasmrod

Am ok with this given that it won't allow invalid data on the DB.

sgress454 added 3 commits January 7, 2026 16:41

validation

c24a3a3

update and use in code

bacd072

update in api layer

bf27375

sgress454 requested a review from a team as a code owner January 7, 2026 23:54

sgress454 temporarily deployed to Docker Hub January 7, 2026 23:54 — with GitHub Actions Inactive

sgress454 commented Jan 8, 2026

View reviewed changes

sgress454 assigned lucasmrod and nulmete Jan 8, 2026

lucasmrod reviewed Jan 8, 2026

View reviewed changes

nulmete reviewed Jan 8, 2026

View reviewed changes

sgress454 added 2 commits January 8, 2026 09:53

don't update schedule when enabled is false

902cc38

updates from review comments

4721be2

sgress454 temporarily deployed to Docker Hub January 8, 2026 15:57 — with GitHub Actions Inactive

nulmete approved these changes Jan 8, 2026

View reviewed changes

lucasmrod approved these changes Jan 8, 2026

View reviewed changes

sgress454 merged commit 35d9f42 into main Jan 8, 2026
48 checks passed

sgress454 deleted the sgress454/software-schedule-validation branch January 8, 2026 16:24

Add shared software update schedule validation logic #38016

Add shared software update schedule validation logic #38016

Conversation

sgress454 commented Jan 7, 2026

Details

Checklist for submitter

Testing

Uh oh!

sgress454 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lucasmrod left a comment

Choose a reason for hiding this comment

Uh oh!

lucasmrod Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulmete Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

sgress454 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nulmete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nulmete Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

sgress454 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nulmete Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

lucasmrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 8, 2026 •

edited

Loading

lucasmrod Jan 8, 2026 •

edited

Loading