-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Security Solution] Randomize task schedules when bulk scheduling #269991
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.
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
Changes from 1 commit
7cde863
08379a0
f3d3e0a
ae89883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,15 +148,20 @@ export class TaskScheduling { | |
| ? agent.currentTraceparent | ||
| : ''; | ||
| const modifiedTasks = await Promise.all( | ||
| taskInstances.map(async (taskInstance) => { | ||
| taskInstances.map(async (taskInstance, i) => { | ||
| const { taskInstance: modifiedTask } = await this.middleware.beforeSave({ | ||
| ...omit(options, 'apiKey', 'request'), | ||
| taskInstance: ensureDeprecatedFieldsAreCorrected(taskInstance, this.logger), | ||
| }); | ||
| const enabled = modifiedTask.enabled ?? true; | ||
| // Run the first task now. Run all other tasks a random number of ms in the future, | ||
| // with a maximum of 5 minutes or the task interval, whichever is smaller. | ||
| const runAt = enabled && i > 0 ? addJitter(modifiedTask.schedule?.interval) ?? {} : {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: since the variable contain an object with more fields than runAt -
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @darnautov, addressed in 08379a0 I went with var name |
||
| return { | ||
| ...modifiedTask, | ||
| traceparent: traceparent || '', | ||
| enabled: modifiedTask.enabled ?? true, | ||
| enabled, | ||
| ...runAt, | ||
| }; | ||
| }) | ||
| ); | ||
|
|
@@ -200,11 +205,9 @@ export class TaskScheduling { | |
| if (runSoon) { | ||
| // Run the first task now. Run all other tasks a random number of ms in the future, | ||
| // with a maximum of 5 minutes or the task interval, whichever is smaller. | ||
| const taskToRun = | ||
| i === 0 | ||
| ? { ...task, runAt: new Date(), scheduledAt: new Date() } | ||
| : randomlyOffsetRunTimestamp(task); | ||
| return { ...taskToRun, enabled: true }; | ||
| return i === 0 | ||
| ? { ...task, enabled: true, runAt: new Date(), scheduledAt: new Date() } | ||
| : { ...task, enabled: true, ...addJitter(task.schedule?.interval ?? '0s') }; | ||
| } | ||
| return { ...task, enabled: true }; | ||
| }, | ||
|
|
@@ -375,17 +378,15 @@ export class TaskScheduling { | |
| } | ||
| } | ||
|
|
||
| const randomlyOffsetRunTimestamp: (task: ConcreteTaskInstance) => ConcreteTaskInstance = (task) => { | ||
| const addJitter = (interval?: string): { runAt: Date; scheduledAt: Date } | undefined => { | ||
| if (!interval) return undefined; | ||
|
|
||
| const now = Date.now(); | ||
| const maximumOffsetTimestamp = now + 1000 * 60 * 5; // now + 5 minutes | ||
| const taskIntervalInMs = parseIntervalAsMillisecond(task.schedule?.interval ?? '0s'); | ||
| const taskIntervalInMs = parseIntervalAsMillisecond(interval); | ||
| const maximumRunAt = Math.min(now + taskIntervalInMs, maximumOffsetTimestamp); | ||
|
|
||
| // Offset between 1 and maximumRunAt ms | ||
| const runAt = new Date(now + Math.floor(Math.random() * (maximumRunAt - now) + 1)); | ||
| return { | ||
| ...task, | ||
| runAt, | ||
| scheduledAt: runAt, | ||
| }; | ||
| return { runAt, scheduledAt: runAt }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I guess this comment was carried over from
bulkEnable, butbulkScheduledoesn't actually setrunAt: new Date()for i == 0, it just spreads{}and lets the store default to now.Could we either adjsut the wording to match what's happening, or mirror
bulkEnableand setrunAt: new Date(), scheduledAt: new Date()explicitly fori == 0?I think it'd be easier to have consistent logic for both methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @darnautov, addressed in 08379a0
Logic is the same now across
bulkEnableandbulkSchedule. You're right the comment was not clear that the magic was happening inside the store. Seeing the dates set explicitly makes this clear on this end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added this change to make sure that adhoc tasks are set with a date explicitly (to run now).