Repository navigation
fix(servicenow): add compact role-based DLS to reduce ACL memory - #4392
Conversation
Store role_id tokens on content documents instead of expanding every role member onto each record, and enrich identity docs with role membership. Defaults expand_role_members to false; public tables omit _allow_access_control. Requires a full content and ACL resync.
Omit empty ACL arrays to avoid silent deny-all, preserve legacy expansion on upgrade when expand_role_members is absent, always enrich identity docs with roles, and apply table ACLs for advanced-rules syncs when DLS is on.
Match SharePoint compact DLS: legacy per-user expansion stays the default; compact role tokens are opt-in via expand_role_members=false.
Distinguish public tables (omit ACL field) from unresolved private tables (store an empty ACL array to deny access until roles can be resolved).
|
|
||
| def _finalize_compact_access_control(self, table_name, role_sys_ids): | ||
| compact_acl = list({_prefix_role_id(role_id) for role_id in role_sys_ids}) | ||
| if not compact_acl: |
There was a problem hiding this comment.
If a table has no read ACL entries, I believe it is public. This will make it inaccessible even when the array is empty
There was a problem hiding this comment.
Updated compact path to omit _allow_access_control when no roles resolve (aligned with your read).
Note: this proposal slightly diverges from legacy’s empty-list behavior. We haven't validated real SN semantics end-to-end yet. We'll try to validate this E2E before merging and keep both legacy and compact modes in best shape possible.
| yield user | ||
|
|
||
| async def _fetch_user_roles_map(self): | ||
| """Build a map of user sys_id -> set of role sys_ids from sys_user_has_role.""" |
There was a problem hiding this comment.
This may OOM. We may want to page this sys_user_has_role table
There was a problem hiding this comment.
I'd push back on this comment for now.
Fetches are already paginated; we only hold an O(assignments) map (~tens of MB), not per-doc expansion. But I'd be happy to revisit streaming if ACL sync memory shows up in prod.
| ) | ||
| return self._finalize_compact_access_control(table_name, role_sys_ids) | ||
|
|
||
| roles_by_sys_id = await self._role_sys_id_to_name_map() |
There was a problem hiding this comment.
if _table_read_role_sys_ids() returns an empty array, we completely ignore this.
There was a problem hiding this comment.
Same as for your fourth comment - empty compact ACL now returns None (omit field).
Legacy divergence and lack of live SN validation.
Will try to validate during E2E testing.
There was a problem hiding this comment.
I think you misunderstand. If role_sys_ids is empty, we never need role_by_sys_id, as the for loop will never execute. It's minor
There was a problem hiding this comment.
You're right. I misunderstood initially. Pushed the fix now. Should be ok.
Guard deleted roles, gate identity role enrichment to compact mode, sort ACL arrays, cache sys_user_role per sync, treat empty compact ACL as public, and consolidate role map fetching.
|
Thank you @erikcurrin-elastic |
erikcurrin-elastic
left a comment
erikcurrin-elastic
left a comment
There was a problem hiding this comment.
Needs testing, but the code looks good to me
Fetch table read roles before sys_user_role so empty ACL tables short-circuit without an unused roles lookup.
Resolve NOTICE.txt conflict by taking main's dependency versions.
E2E validation summaryTested locally against a ServiceNow developer instance with self-managed connector ( Compact mode (
|
E2E validation — compact DLS toggle (ServiceNow)Environment: local ES 8.x + Kibana stack, Method: same index throughout; toggle Phase summary
Phases 1↔2 and 3↔4 confirm round-trip on the same index with config toggle + sync only. Sync performance (full content sync, 879 docs)
ACS sync: ~7–22 s, 636 identities (unchanged count across modes). Disk usage (primary store, post-
|
| Index | Legacy (phase 4) | Compact (phase 3) | Δ |
|---|---|---|---|
Content (879 docs) |
1.6 MB | 1.4 MB | +13% |
ACL filter (636 ids) |
152 KB | 188 KB | −19% |
| Total | 1.84 MB | 1.65 MB | +12% |
Compact reduces in-document ACL token volume by ~94% (25,717 → 1,567). On-disk content index is only modestly smaller; ACL index is slightly larger in compact mode due to role enrichment on identity docs.
Note: pre-merge sizes during A/B (e.g. 4.1 MB content right after legacy re-sync) reflect segment/tombstone bloat, not larger documents. Always merge before comparing store size.
Other observations
- 409 on
sync_done: benign optimistic-concurrency conflict when connector config was updated while a job was running; content sync completed, only final connector metadata write failed. Avoid config edits during active syncs / ensure a singleelastic-ingestworker. - Built-in tables (
services: *) still use hardcodedDEFAULT_SERVICE_NAMESfor ACLs in both modes — pre-existing, out of scope for this PR. - Custom public table (
u_test_public): compact omits_allow_access_control(world-readable); legacy writes[](deny-all). Toggle picked up without worker restart.
Verdict
Ready to merge for the stated goal: opt-in compact DLS (expand_role_members=false) correctly stores role_id: tokens on content docs, resolves membership via ACS, round-trips to legacy on the same index, and dramatically reduces per-document ACL payload with acceptable disk trade-offs at this scale.
Follow-ups (non-blocking): document DEFAULT_SERVICE_NAMES limitation; consider Kibana RCF / release label housekeeping.
## Summary Adds the `expand_role_members` rich configurable field for the ServiceNow native connector so Fleet shows the compact role-based DLS toggle. Related connectors PR: elastic/connectors#4392 ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [x] Low risk. This only exposes an existing connector configuration field in Fleet. No runtime behavior changes in Kibana. ## Release note ServiceNow connectors now expose an **Expand role members** toggle in Fleet for compact document-level security.
…#4396) ## Closes elastic/chat-program#48 Part of elastic/chat-program#47. SharePoint Online DLS expands every site group member onto each document's `_allow_access_control` array. Large site groups can produce multi-megabyte ACLs per document and OOM Elasticsearch during `_reindex`. This PR adds `expand_site_group_members` (default `true` to preserve existing behavior). When disabled (compact mode): - Content documents store a compact `site_group:<site_id>:<group_id>` token instead of every site group member - Direct user, Entra group, and site user permissions are unchanged on the document - ACL sync builds a site-group membership index and enriches identity docs so DLS term overlap still resolves access (including nested Entra groups and EEEU guest exclusion) - If a compact token cannot be written safely (e.g. missing `site_id`), the connector falls back to expanding site group members and logs a warning Changing the setting requires a full content sync and access control sync. ## Checklists #### Pre-Review Checklist - [x] this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check `config.yml.example`) - [x] this PR has a meaningful title - [x] this PR links to all relevant github issues that it fixes or partially addresses - [x] if there is no GH issue, please create it. Each PR should have a link to an issue - [x] this PR has a thorough description - [x] Covered the changes with automated tests - [x] Tested the changes locally - [ ] Added a label for each target release version (example: `v7.13.2`, `v7.14.0`, `v8.0.0`) - [ ] For bugfixes: backport safely to all minor branches still receiving patch releases - [ ] Considered corresponding documentation changes - [ ] Contributed any configuration settings changes to the configuration reference - [x] if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in [Kibana](https://github.com/elastic/kibana/blob/main/packages/kbn-search-connectors/types/native_connectors.ts) #### Changes Requiring Extra Attention - [x] Security-related changes (encryption, TLS, SSRF, etc) - [ ] New external service dependencies added. ## Related Pull Requests * #4392 — ServiceNow compact role DLS (same tracking issue) * elastic/kibana#288070 — Kibana RCF for `expand_site_group_members` ## Release Note SharePoint Online document-level security can store compact site-group tokens on documents instead of expanding every site group member onto each record, reducing memory use for large site groups. Disable **Expand site group members** to enable compact mode. The default preserves the previous behavior. Changing the setting requires a full content sync and access control sync. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
3 participants
Closes https://github.com/elastic/chat-program/issues/49
Part of https://github.com/elastic/chat-program/issues/47.
ServiceNow DLS expands every table read role into individual
user_id:/user:/email:entries on each content document (and attachment). For large tenants — especially when a table ACL includes thepublicrole — that can mean hundreds of thousands of ACL entries per document and OOM Elasticsearch during_reindex.This PR adds compact DLS, opt-in via
expand_role_members=false(defaulttruepreserves existing behavior). When compact mode is enabled:role_id:<sys_id>tokens instead of every role membersys_user_has_roleso DLS term overlap still workspublicread role omit_allow_access_control(world-readable via existing DLSmust_not exists)_allow_access_control(same as public tables; world-readable via existing DLSmust_not exists)Also fixes advanced sync rules to resolve ACL per table instead of stamping an empty ACL on every batch.
Changing the setting requires a full content sync and access control sync.
Checklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)Changes Requiring Extra Attention
Related Pull Requests
native_connectors.tsRCF forexpand_role_members— to be opened separatelyRelease Note
ServiceNow document-level security can store compact role tokens on content documents instead of expanding every role member onto each record, reducing memory use for large tenants. Disable Expand role members to enable compact mode. The default preserves the previous per-user expansion behavior. Changing the setting requires a full content sync and access control sync.