osquerybeat: add RRULE scheduling - #48767
Conversation
🤖 GitHub commentsJust comment with:
|
…and configuration
…se ID handling in queries
…uled query handling
Resolve osquerybeat conflicts: keep RRULE scheduling and NativeSchedule metadata, integrate upstream pack_id/space_id/query profiling and publisher signatures. Update recurrence handler and config plugin tests accordingly.
Validate pack-level native vs RRULE defaults, merge defaults into pack queries, and reject mixed schedule modes. Omit RRULE-only queries from osqueryd JSON, raise MaxSplay to 12h with a single definition in config, feed the RRULE handler from ConfigPlugin.EffectiveOsqueryConfig after Set, and document fatal Set failures in the osquery runner. Assisted-By: Cursor
…dule_id Split query schedule metadata into CommonScheduleConfig and NativeSchedule, use NativeSchedule for pack default_native_schedule, drop pack-level schedule_id defaults (keep default_space_id and default_rrule_schedule), and refresh reference docs and tests. Assisted-By: Cursor
|
Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform) |
📝 WalkthroughWalkthroughAdds RRULE-based scheduling to osquerybeat: new config types for per-query 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x-pack/osquerybeat/beater/osquerybeat.go (1)
488-503:⚠️ Potential issue | 🟠 MajorDefer RRULE updates until osqueryd has applied the same policy snapshot.
Set()only stages the new policy; osqueryd keeps running the previous native schedule until it pulls config again, whilerruleHandler.UpdateFromConfig(...)starts applying the new RRULE schedule immediately here. On a native↔RRULE mode change, that can duplicate executions/responses or create a gap for up to one refresh interval. Please drive both schedulers from the same post-GenerateConfig/ osqueryd-applied state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/osquerybeat/beater/osquerybeat.go` around lines 488 - 503, The RRULE update is happening immediately after configPlugin.Set(InputConfigs) which only stages a new policy; instead defer calling rruleHandler.UpdateFromConfig(...) until the staged config has been applied by osqueryd (the same post-GenerateConfig / osqueryd-applied state used to populate configPlugin.EffectiveOsqueryConfig()). Modify the flow so configPlugin.Set(...) still stages the config but you only call rruleHandler.UpdateFromConfig(configPlugin.EffectiveOsqueryConfig()) after receiving the confirmation that osqueryd pulled/applied the generated config (e.g. by waiting for EffectiveOsqueryConfig() to become the applied snapshot or hooking into the GenerateConfig/apply-complete signal), and keep the existing error handling that clears RRULE by calling UpdateFromConfig(nil) on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x-pack/osquerybeat/beater/recurrence_query_handler.go`:
- Around line 70-75: The refresh currently logs and skips queries when
h.createScheduledQuery(name, q) returns an error, which causes
Scheduler.UpdateQueries() to treat the missing entry as a deletion; change this
so the refresh fails fast: whenever createScheduledQuery (and the similar block
at lines ~88-97) returns an error, propagate/return that error from the refresh
function instead of continuing and appending, so the update is aborted and no
partial replacement is applied; ensure the function calling
Scheduler.UpdateQueries() returns a non-nil error on any createScheduledQuery
failure so the caller can avoid treating the update as successful.
In `@x-pack/osquerybeat/internal/scheduler/schedule.go`:
- Around line 57-59: The error wrapping in the schedule logic uses
fmt.Errorf("%w: %v", ErrInvalidRRule, err) which prevents proper unwrapping;
update the error construction in the block around rrule.StrToRRule(s.RRule) so
the inner error is wrapped with %w (e.g., fmt.Errorf("%w: %w", ErrInvalidRRule,
err) or combine into a single wrapped error) referencing ErrInvalidRRule and the
returned err so callers can use errors.Is/errors.As on the resulting error.
In `@x-pack/osquerybeat/internal/scheduler/scheduler_test.go`:
- Around line 20-21: Replace the use of logp.DevelopmentSetup() and
logp.NewLogger("test") with logptest.NewTestingLogger(t, "") and pass that
logger into the code under test: remove the lines creating 'log' via logp and
instead create logger := logptest.NewTestingLogger(t, "") (importing
github.com/elastic/elastic-agent-libs/logp/logptest) and update any references
to the local variable 'log' to use this logger or pass it as a parameter to
functions under test (apply in all test functions where logp.DevelopmentSetup()
+ logp.NewLogger() appear).
In `@x-pack/osquerybeat/internal/scheduler/scheduler.go`:
- Around line 263-270: The ExecutionIndex is being computed from runTime (after
splay/delay) which can slip past the intended recurrence; change the index
computation to use the planned occurrence timestamp nextRun instead of runTime:
call schedule.ExecutionIndex(nextRun) and use that result when invoking
s.executeQuery(ctx, sq, runTime, nextRun, executionIndex) so the emitted
schedule_execution_count matches the plannedScheduleTime; update the variable
computation near where runTime, executionIndex and s.executeQuery are set.
- Around line 152-200: Update UpdateQueries to stop leaving old goroutines
running by handling inactive/expired schedules and comparing all relevant
schedule fields: when you hit the "if sq.Schedule == nil ||
!sq.Schedule.IsActive()" branch, check s.queries for an existing entry
(s.queries[sq.Name]) and if present call existing.cancel(), delete
s.queries[sq.Name], and log removal (same as the deletion path above). Also
replace the tight equality check that only compares
existing.query.Schedule.RRule and existing.query.Query with a comparison that
includes StartDate, EndDate, Splay, Timeout, and ScheduleID (in addition to
RRule and Query); if any of those differ, cancel the existing job, delete it,
and recreate the scheduledJob and goroutine (runJob) as you currently do.
---
Outside diff comments:
In `@x-pack/osquerybeat/beater/osquerybeat.go`:
- Around line 488-503: The RRULE update is happening immediately after
configPlugin.Set(InputConfigs) which only stages a new policy; instead defer
calling rruleHandler.UpdateFromConfig(...) until the staged config has been
applied by osqueryd (the same post-GenerateConfig / osqueryd-applied state used
to populate configPlugin.EffectiveOsqueryConfig()). Modify the flow so
configPlugin.Set(...) still stages the config but you only call
rruleHandler.UpdateFromConfig(configPlugin.EffectiveOsqueryConfig()) after
receiving the confirmation that osqueryd pulled/applied the generated config
(e.g. by waiting for EffectiveOsqueryConfig() to become the applied snapshot or
hooking into the GenerateConfig/apply-complete signal), and keep the existing
error handling that clears RRULE by calling UpdateFromConfig(nil) on failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43d23480-7dd8-41cd-9b2b-77e1e27a59a2
📒 Files selected for processing (17)
changelog/fragments/1770723983-osquerybeat-rrule-scheduling.yamlchangelog/fragments/1775587200-osquerybeat-schedule-config-hardening.yamlx-pack/osquerybeat/_meta/config/beat.reference.yml.tmplx-pack/osquerybeat/beater/config_plugin.gox-pack/osquerybeat/beater/config_plugin_test.gox-pack/osquerybeat/beater/osquerybeat.gox-pack/osquerybeat/beater/recurrence_query_handler.gox-pack/osquerybeat/internal/config/osquery.gox-pack/osquerybeat/internal/config/osquery_test.gox-pack/osquerybeat/internal/config/pack_schedule.gox-pack/osquerybeat/internal/config/pack_schedule_test.gox-pack/osquerybeat/internal/scheduler/schedule.gox-pack/osquerybeat/internal/scheduler/schedule_test.gox-pack/osquerybeat/internal/scheduler/scheduler.gox-pack/osquerybeat/internal/scheduler/scheduler_test.gox-pack/osquerybeat/internal/scheduler/types.gox-pack/osquerybeat/osquerybeat.reference.yml
tomsonpl
left a comment
There was a problem hiding this comment.
Skimmed through, looks good to me, thank you for the changes 👍
Use process snapshots before/after RRULE executions (same path as live queries), optional local profile store, and PublishQueryProfile when policy enables profile. Add buildRuntimeQueryProfile with rrule source, extract init/complete profiling helpers on recurrenceQueryHandler, and document process-level metrics vs serialized client vs native osqueryd load. Assisted-By: Cursor
Carry merged pack default query options into RRULE execution so platform, version, and result type semantics match native scheduled queries while still allowing query-level overrides. Assisted-By: Cursor
TL;DRAll 5 failed Windows jobs share the same root cause: Remediation
Investigation detailsRoot CauseThis is a test failure (not a production logic bug).
Evidence
Verification
Follow-upOnce the test input is made cross-platform, the 5 Windows failures should resolve together since they are all the same failing test. Note 🔒 Integrity filter blocked 3 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | noneWhat is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Use an OS-agnostic platform value in the RRULE scheduled query options test so Windows CI exercises the intended option assertions instead of skipping the query through platform filtering. Assisted-By: Cursor
brian-mckinney
left a comment
There was a problem hiding this comment.
Looks good, but is a monster. I only had a few nits.
Reject sub-daily RRULE recurrence intervals and fail closed when version constraints cannot be parsed, so scheduled queries do not run with ambiguous policy constraints. Assisted-By: Cursor
|
/ai |
|
I received What is this? | From workflow: Mention in PR Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Drive all wait timing from Next(), return start_date directly for the first occurrence, and normalize RRULE start/end dates to UTC for consistency with native schedule metadata. Assisted-By: Cursor
|
Tick the box to add this pull request to the merge queue (same as
|
tomsonpl
left a comment
There was a problem hiding this comment.
LGTM 👍 Tested to some extend, and it works as expected :)
|
Tick the box to add this pull request to the merge queue (same as
|
…ies (#52242) Pack schedule-mode validation added with RRULE scheduling (#48767) rejected packs mixing native interval queries with unscheduled ones, failing the whole policy and blocking agent upgrades to 9.5.0. Such packs exist in the field, commonly because query names containing dots are split into nested maps by config parsing, leaving a ghost query without interval or SQL. Accept the native/unscheduled mix again and log a warning naming the unscheduled queries. Keep rejecting packs that mix rrule_schedule with other schedule modes, which is the conflict the validation was introduced for. Fixes #51450 Related to elastic/elastic-agent#15814
…ies (#52242) (#52244) Pack schedule-mode validation added with RRULE scheduling (#48767) rejected packs mixing native interval queries with unscheduled ones, failing the whole policy and blocking agent upgrades to 9.5.0. Such packs exist in the field, commonly because query names containing dots are split into nested maps by config parsing, leaving a ghost query without interval or SQL. Accept the native/unscheduled mix again and log a warning naming the unscheduled queries. Keep rejecting packs that mix rrule_schedule with other schedule modes, which is the conflict the validation was introduced for. Fixes #51450 Related to elastic/elastic-agent#15814 (cherry picked from commit 3439066) Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
Proposed commit message
feat(osquerybeat): add RRULE scheduling
Summary
Add RRULE-driven scheduled queries in osquerybeat so policies can run on recurrence rules outside osqueryd’s native interval model, while leaving native interval scheduling unchanged where used.
Pack defaults (default_native_schedule, default_rrule_schedule, default_space_id) are merged and validated so modes stay consistent; schedule_id is only per-query (not inherited from the pack).
Osqueryd JSON omits RRULE-only queries via forOsqueryd().
The RRULE handler uses ConfigPlugin.EffectiveOsqueryConfig() after a successful Set so it tracks the same merged policy as rendering.
MaxSplay is defined once in internal/config and reused by the scheduler. Invalid policy fails Set and stops the osquery runner until policy is fixed.
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.