Skip to content

os_versions endpoint performance improvements#34897

Merged
getvictor merged 46 commits intomainfrom
victor-os-versions
Nov 3, 2025
Merged

os_versions endpoint performance improvements#34897
getvictor merged 46 commits intomainfrom
victor-os-versions

Conversation

@getvictor
Copy link
Copy Markdown
Member

@getvictor getvictor commented Oct 28, 2025

Related issue: Resolves #34500 and Resolves #33758

Video demo: https://www.youtube.com/watch?v=4HZlKG0G1B0

  • Added a new aggregation table operating_system_version_vulnerabilities for faster queries. The table is currently used only for Linux vulnerabilities, but could be used for other OS vulnerabilities.
  • Added max_vulnerabilities parameter per API doc
  • Also added max_vulnerabilities parameter to os_versions/{id} endpoint, but not making it public since that endpoint is still slow and needs other API changes. bug Missing docs: max_vulnerabilities in GET os_versions/:id and GET /os_versions #34974
  • Removed "kernels": [] from os_versions endpoint result

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.

  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)

Testing

Database migrations

  • Checked schema for all modified table for columns that will auto-update timestamps during migration.
  • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
  • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

Summary by CodeRabbit

  • New Features

    • Added ability to limit the number of vulnerabilities displayed for operating system versions via an optional parameter.
    • Introduced vulnerability count tracking for operating system versions, now visible in API responses and UI displays.
    • Enhanced operating system vulnerability visualization with improved count-based rendering.
  • Tests

    • Added comprehensive test coverage for vulnerability limiting behavior across multiple operating system versions and architectures.

ghernandez345
ghernandez345 previously approved these changes Oct 30, 2025
Copy link
Copy Markdown
Contributor

@mostlikelee mostlikelee left a comment

Choose a reason for hiding this comment

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

Great choice on the caching approach. My initial reaction, though, is that we’re trading a fair bit of complexity for performance gains, and wondering if there are ways to simplify.

It might make sense to further decouple Linux vulnerabilities from the rest of the query path since they behave so differently.

AND os.name = ? AND os.version = ?
GROUP BY v.cve

UNION
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we're not deduping here, guessing that will result in over counting?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

operating_system_vulnerabilities should only have non-Linux vulnerabilities and operating_system_version_vulnerabilities should only have Linux vulnerabilities.

operating_system_vulnerabilities are being deduped with GROUP BY v.cve. operating_system_version_vulnerabilities are deduped during aggregation, so they shouldn't even need GROUP BY. But I wasn't focusing on optimizing this os_versions/{id} endpoint, so I think it is OK to leave as is and re-review in the follow up bug.

}, nil
}

func (ds *Datastore) listVulnsWithoutCVSS(ctx context.Context, linuxTeamFilter string, maxVulnerabilities *int, args []any) (fleet.OSVulnerabilitiesWithCount,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

glad you broke this out, much easier to reason about

Comment thread server/datastore/mysql/operating_system_vulnerabilities.go Outdated
Comment thread server/datastore/mysql/operating_system_vulnerabilities.go
@getvictor
Copy link
Copy Markdown
Member Author

Great choice on the caching approach. My initial reaction, though, is that we’re trading a fair bit of complexity for performance gains, and wondering if there are ways to simplify.

It might make sense to further decouple Linux vulnerabilities from the rest of the query path since they behave so differently.

@mostlikelee Yes, it is complex. We could eliminate operating_system_vulnerabilities and just use operating_system_version_vulnerabilities, or we could decouple by making Linix and non-Linux separate Datastore or Service layer methods.

@mostlikelee
Copy link
Copy Markdown
Contributor

@mostlikelee Yes, it is complex. We could eliminate operating_system_vulnerabilities and just use operating_system_version_vulnerabilities, or we could decouple by making Linix and non-Linux separate Datastore or Service layer methods.

@getvictor Up to you, as I know this is an urgent request. We could look into refactoring down the line.

@getvictor getvictor merged commit ba5f02f into main Nov 3, 2025
42 of 44 checks passed
@getvictor getvictor deleted the victor-os-versions branch November 3, 2025 19:07
getvictor added a commit that referenced this pull request Nov 3, 2025
<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**Related issue:** Resolves #34500 and Resolves #33758

Video demo: https://www.youtube.com/watch?v=4HZlKG0G1B0

- Added a new aggregation table
`operating_system_version_vulnerabilities` for faster queries. The table
is currently used only for Linux vulnerabilities, but could be used for
other OS vulnerabilities.
- Added `max_vulnerabilities` parameter per [API
doc](#33533)
- Also added `max_vulnerabilities` parameter to `os_versions/{id}`
endpoint, but not making it public since that endpoint is still slow and
needs other API changes. bug #34974
- Removed `"kernels": []` from `os_versions` endpoint result

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.

- [x] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented (using placeholders for values in statements)

## Testing

- [x] Added/updated automated tests
- [x] Where appropriate, [automated tests simulate multiple hosts and
test for host
isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing)
(updates to one hosts's records do not affect another)

- [x] QA'd all new/changed functionality manually

## Database migrations

- [x] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [x] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [x] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
* Added ability to limit the number of vulnerabilities displayed for
operating system versions via an optional parameter.
* Introduced vulnerability count tracking for operating system versions,
now visible in API responses and UI displays.
* Enhanced operating system vulnerability visualization with improved
count-based rendering.

* **Tests**
* Added comprehensive test coverage for vulnerability limiting behavior
across multiple operating system versions and architectures.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

(cherry picked from commit ba5f02f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants