Use separate BitSet cache in Doc Level Security #43669

tvernum · 2019-06-27T02:13:35Z

Document level security was depending on the shared
"BitsetFilterCache" which (by design) never expires its entries.

However, when using DLS queries - particularly templated ones - the
number (and memory usage) of generated bitsets can be significant.

This change introduces a new cache specifically for BitSets used in
DLS queries, that has memory usage constraints and access time expiry.

The whole cache is automatically cleared if the role cache is cleared.
Individual bitsets are cleared when the corresponding lucene index
reader is closed.

Closes: #30974

Document level security was depending on the shared "BitsetFilterCache" which (by design) never expires its entries. However, when using DLS queries - particularly templated ones - the number (and memory usage) of generated bitsets can be significant. This change introduces a new cache specifically for BitSets used in DLS queries, that has memory usage constraints and access time expiry. The whole cache is automatically cleared if the role cache is cleared. Individual bitsets are cleared when the corresponding lucene index reader is closed.

elasticmachine · 2019-06-27T02:13:56Z

Pinging @elastic/es-security

tvernum · 2019-06-27T02:43:39Z

@elasticmachine update branch

tvernum · 2019-06-28T07:35:12Z

I haven't written docs for these new cache config properties (ttl and max_bytes) but will do so on Monday.

I'm curious about other people's opinions on the default values for these.

I've set max_bytes to 50Mb, because it seems like it would be a problem to use more than that on a small (1Gb) node, but I don't really have the information at hand to know whether that's big enough to store typical workloads or whether users will have cache thrashing.
I set ttl to a week (7 * 24 hours) because I think we want to depend on the max_bytes setting to keep the cache under control, rather than aggresively expiring entries if they aren't used for a day or so.

I could easily be persuaded to a different set of values, but since the main purpose of this change it to reduce the amount of memory used by DLS bitsets, we don't want to make those numbers too big.

jpountz

We should try to find a better way to evict on closed segments. We used to have other caches that required linear scans for eviction and this caused issues, eg. when closing a node because then all its indices would close one after the other, and since close listeners are called synchonously from IndexReader#close, this would run in O(n^2) as a function of the number of segments on the node.

jpountz · 2019-06-28T14:45:53Z

...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

+
+    @Override
+    public void onClose(IndexReader.CacheKey ownerCoreCacheKey) {
+        bitsetCacheHelper.removeKeysIf(key -> key.matchesIndex(ownerCoreCacheKey));


We have had performance issues with using linear scans over cache keys in close listeners in the past. I'm not sure how to address it, some other caches have two levels of keys, ie. they look like a Map<IndexReader.CacheKey, Map<Query, Value>>. But maybe we could also separately maintain a mapping between indexreader cache keys and queries that have a cache entry on that particular segment.

I'll look at maintaining a secondary mapping. The problem with the two-level cache is that we don't have good ways to manage expiry.
The lower level caches could have access-time expiry, but it's hard to do memory-size expiry.
The upper level caches can do memory-size expiry, but will expire everything under a IndexReader.CacheKey rather than expiring the LRU queries.

jpountz · 2019-06-28T14:46:50Z

...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

+        this.bitsetCache = CacheBuilder.<BitsetCacheKey, BitSet>builder()
+            .setExpireAfterAccess(ttl)
+            .setMaximumWeight(size.getBytes())
+            .weigher((key, bitSet) -> bitSet.ramBytesUsed()).build();


maybe return 0 if bitSet == NULL_MARKER?

# Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapperUnitTests.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

jkakavas

The changes / logic looks good to me, I just left some wording nits and a suggestion about the naming of the related settings. I don't feel particularly confident with this part but thankfully @jpountz makes up for it

...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

...in/core/src/main/java/org/elasticsearch/xpack/core/security/support/CacheIteratorHelper.java

jpountz

I left some minor comments but otherwise it looks good to me, including the mapping from queries to cache keys.

...ava/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

- Rename "max_bytes" to "size" - Rename "dls_fls" to "dls" - Fix typos in Javadoc

tvernum · 2019-07-03T02:14:42Z

@elasticmachine update branch

Document level security was depending on the shared "BitsetFilterCache" which (by design) never expires its entries. However, when using DLS queries - particularly templated ones - the number (and memory usage) of generated bitsets can be significant. This change introduces a new cache specifically for BitSets used in DLS queries, that has memory usage constraints and access time expiry. The whole cache is automatically cleared if the role cache is cleared. Individual bitsets are cleared when the corresponding lucene index reader is closed. The cache defaults to 50MB, and entries expire if unused for 7 days. Backport of: elastic#43669

Document level security was depending on the shared "BitsetFilterCache" which (by design) never expires its entries. However, when using DLS queries - particularly templated ones - the number (and memory usage) of generated bitsets can be significant. This change introduces a new cache specifically for BitSets used in DLS queries, that has memory usage constraints and access time expiry. The whole cache is automatically cleared if the role cache is cleared. Individual bitsets are cleared when the corresponding lucene index reader is closed. The cache defaults to 50MB, and entries expire if unused for 7 days. Backport of: #43669

Two new settings were introduced in elastic#43669 (bb130f5) to control the behaviour of the Document Level Security BitSet cache. This change adds documentation for these 2 settings.

Two new settings were introduced in elastic#43669 (bb130f5) to control the behaviour of the Document Level Security BitSet cache. This change adds documentation for these 2 settings. Backport of: elastic#44100

Two new settings were introduced in #43669 to control the behaviour of the Document Level Security BitSet cache. This change adds documentation for these 2 settings. Backport of: #44100

The Document Level Security BitSet Cache (see elastic#43669) had a default configuration of "small size, long lifetime". However, this is not a very useful default as the cache is most valuable for BitSets that take a long time to construct, which is (generally speaking) the same ones that operate over a large number of documents and contain many bytes. This commit changes the cache to be "large size, short lifetime" so that it can hold bitsets representing billions of documents, but releases memory quickly. The new defaults are 10% of heap, and 2 hours. This also adds some logging when a single BitSet exceeds the size of the cache and when the cache is full.

The Document Level Security BitSet Cache (see #43669) had a default configuration of "small size, long lifetime". However, this is not a very useful default as the cache is most valuable for BitSets that take a long time to construct, which is (generally speaking) the same ones that operate over a large number of documents and contain many bytes. This commit changes the cache to be "large size, short lifetime" so that it can hold bitsets representing billions of documents, but releases memory quickly. The new defaults are 10% of heap, and 2 hours. This also adds some logging when a single BitSet exceeds the size of the cache and when the cache is full. Resolves: #49260

The Document Level Security BitSet Cache (see elastic#43669) had a default configuration of "small size, long lifetime". However, this is not a very useful default as the cache is most valuable for BitSets that take a long time to construct, which is (generally speaking) the same ones that operate over a large number of documents and contain many bytes. This commit changes the cache to be "large size, short lifetime" so that it can hold bitsets representing billions of documents, but releases memory quickly. The new defaults are 10% of heap, and 2 hours. This also adds some logging when a single BitSet exceeds the size of the cache and when the cache is full. Resolves: elastic#49260 Backport of: elastic#50535

The Document Level Security BitSet Cache (see #43669) had a default configuration of "small size, long lifetime". However, this is not a very useful default as the cache is most valuable for BitSets that take a long time to construct, which is (generally speaking) the same ones that operate over a large number of documents and contain many bytes. This commit changes the cache to be "large size, short lifetime" so that it can hold bitsets representing billions of documents, but releases memory quickly. The new defaults are 10% of heap, and 2 hours. This also adds some logging when a single BitSet exceeds the size of the cache and when the cache is full. Backport of: #50535

The Document Level Security BitSet Cache (see elastic#43669) had a default configuration of "small size, long lifetime". However, this is not a very useful default as the cache is most valuable for BitSets that take a long time to construct, which is (generally speaking) the same ones that operate over a large number of documents and contain many bytes. This commit changes the cache to be "large size, short lifetime" so that it can hold bitsets representing billions of documents, but releases memory quickly. The new defaults are 10% of heap, and 2 hours. This also adds some logging when a single BitSet exceeds the size of the cache and when the cache is full. Resolves: elastic#49260

tvernum added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v7.3.0 v8.0.0 labels Jun 27, 2019

elasticmachine and others added 4 commits June 26, 2019 19:43

Merge branch 'master' into dls-bitset-cache

0237401

Fix test

dc0a116

Merge branch 'master' into dls-bitset-cache

a1f7941

Change access time TTL to 1 week

9f19744

tvernum marked this pull request as ready for review June 28, 2019 07:30

tvernum requested review from jkakavas and jpountz June 28, 2019 07:30

jpountz requested changes Jun 28, 2019

View reviewed changes

tvernum added 3 commits July 1, 2019 15:19

Treat null bitset marker as 0 bytes

6a73aa9

Maintain map of index to cache-key

1c4d83b

tvernum requested a review from jpountz July 1, 2019 11:46

jkakavas approved these changes Jul 1, 2019

View reviewed changes

jpountz requested changes Jul 1, 2019

View reviewed changes

tvernum added 2 commits July 2, 2019 16:12

Switch to concurrent set

09f3579

Merge branch 'master' into dls-bitset-cache

cd6c60d

jpountz approved these changes Jul 2, 2019

View reviewed changes

Address feedback

0b2d8e7

- Rename "max_bytes" to "size" - Rename "dls_fls" to "dls" - Fix typos in Javadoc

Merge branch 'master' into dls-bitset-cache

6775c7b

tvernum added the >enhancement label Jul 3, 2019

tvernum merged commit bb130f5 into elastic:master Jul 3, 2019

tvernum added the backport pending label Jul 3, 2019

tvernum mentioned this pull request Jul 3, 2019

[Backport 7.x] Use separate BitSet cache in Doc Level Security #43899

Merged

tvernum removed the backport pending label Jul 3, 2019

tvernum mentioned this pull request Jul 9, 2019

Document xpack.security.dls.bitset.cache settings #44100

Merged

jpountz added the release highlight label Jul 10, 2019

tvernum mentioned this pull request Jul 16, 2019

[Backport 7.x] Document xpack.security.dls.bitset.cache settings #44400

Merged

tvernum mentioned this pull request Jul 16, 2019

[Backport 7.3] Document xpack.security.dls.bitset.cache settings #44401

Merged

tvernum mentioned this pull request Dec 31, 2019

Increase Size and lower TTL on DLS BitSet Cache #50535

Merged

tvernum mentioned this pull request Jan 14, 2020

[Backport 7.x] Increase Size and lower TTL on DLS BitSet Cache #50953

Merged

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate BitSet cache in Doc Level Security #43669

Use separate BitSet cache in Doc Level Security #43669

tvernum commented Jun 27, 2019

elasticmachine commented Jun 27, 2019

tvernum commented Jun 27, 2019

tvernum commented Jun 28, 2019

jpountz left a comment

jpountz Jun 28, 2019

tvernum Jul 1, 2019

jpountz Jun 28, 2019

jkakavas left a comment

jpountz left a comment

tvernum commented Jul 3, 2019

Use separate BitSet cache in Doc Level Security #43669

Use separate BitSet cache in Doc Level Security #43669

Conversation

tvernum commented Jun 27, 2019

elasticmachine commented Jun 27, 2019

tvernum commented Jun 27, 2019

tvernum commented Jun 28, 2019

jpountz left a comment

Choose a reason for hiding this comment

jpountz Jun 28, 2019

Choose a reason for hiding this comment

tvernum Jul 1, 2019

Choose a reason for hiding this comment

jpountz Jun 28, 2019

Choose a reason for hiding this comment

jkakavas left a comment

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

tvernum commented Jul 3, 2019