Use RoleRetrievalResult for better caching by jaymode · Pull Request #34197 · elastic/elasticsearch

jaymode · 2018-10-01T18:29:10Z

Security caches the result of role lookups and negative lookups are
cached indefinitely. In the case of transient failures this leads to a
bad experience as the roles could truly exist. The CompositeRolesStore
needs to know if a failure occurred in one of the roles stores in order
to make the appropriate decision as it relates to caching. In order to
provide this information to the CompositeRolesStore, the return type of
methods to retrieve roles has changed to a new class,
RoleRetrievalResult. This class provides the ability to pass back an
exception to the roles store. This exception does not mean that a
request should be failed but instead serves as a signal to the roles
store that missing roles should not be cached and neither should the
combined role if there are missing roles.

As part of this, the negative lookup cache was also changed from an
unbounded cache to a cache with a configurable limit.

Relates #33205

Security caches the result of role lookups and negative lookups are cached indefinitely. In the case of transient failures this leads to a bad experience as the roles could truly exist. The CompositeRolesStore needs to know if a failure occurred in one of the roles stores in order to make the appropriate decision as it relates to caching. In order to provide this information to the CompositeRolesStore, the return type of methods to retrieve roles has changed to a new class, RoleRetrievalResult. This class provides the ability to pass back an exception to the roles store. This exception does not mean that a request should be failed but instead serves as a signal to the roles store that missing roles should not be cached and neither should the combined role if there are missing roles. As part of this, the negative lookup cache was also changed from an unbounded cache to a cache with a configurable limit. Relates elastic#33205

elasticmachine · 2018-10-01T18:29:12Z

Pinging @elastic/es-security

bizybot

I have left some comments around further refactoring, other changes look good to me.

bizybot · 2018-10-03T08:12:57Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

+            allSuccessful = false;
+        }
+
+        private boolean hadFailures() {


RolesRetrievalResult#isFailure()

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

bizybot

Small change else LGTM, Thank you for the iteration.

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

jkakavas

I took a first pass and this looks good to me @jaymode with a fix and some minor suggestions.

jkakavas · 2018-10-05T08:36:28Z

...ore/src/main/java/org/elasticsearch/xpack/core/security/authz/store/RoleRetrievalResult.java

+    private final Set<RoleDescriptor> descriptors;
+    private final Exception failure;
+
+    private RoleRetrievalResult(Set<RoleDescriptor> descriptors, Exception failure) {


Mark as @Nullable ?

jkakavas · 2018-10-05T09:01:42Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

+            failure = true;
+        }
+
+        private boolean isFailure() {


It feels more natural to me to have a success with a default true value and an isSuccessful() method. No concrete arguments for this, just personal sense of how consumable/readable that is, so take if with a grain of salt.

jkakavas · 2018-10-05T09:03:10Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

-                                     */
-                                    if (invalidationCounter == numInvalidation.get()) {
-                                        roleCache.computeIfAbsent(roleNames, (s) -> role);
+                                if (rolesRetrievalResult.isFailure() == false) {


I think merging the two if will not hurt readability.

jkakavas · 2018-10-05T09:06:21Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

-        this.customRolesProviders = Collections.unmodifiableList(rolesProviders);
+        CacheBuilder<String, Boolean> nlcBuilder = CacheBuilder.builder();
+        final int nlcCacheSize = NEGATIVE_LOOKUP_CACHE_SIZE_SETTING.get(settings);
+        if (cacheSize >= 0) {


s/cacheSize/nlcCacheSize

jaymode · 2018-10-05T20:25:33Z

@elasticmachine run packaging tests

jaymode · 2018-10-10T12:41:14Z

run gradle build tests

Security caches the result of role lookups and negative lookups are cached indefinitely. In the case of transient failures this leads to a bad experience as the roles could truly exist. The CompositeRolesStore needs to know if a failure occurred in one of the roles stores in order to make the appropriate decision as it relates to caching. In order to provide this information to the CompositeRolesStore, the return type of methods to retrieve roles has changed to a new class, RoleRetrievalResult. This class provides the ability to pass back an exception to the roles store. This exception does not mean that a request should be failed but instead serves as a signal to the roles store that missing roles should not be cached and neither should the combined role if there are missing roles. As part of this, the negative lookup cache was also changed from an unbounded cache to a cache with a configurable limit. Relates #33205

* master: Do not update number of replicas on no indices (elastic#34481) Core: Remove two methods from AbstractComponent (elastic#34336) Use RoleRetrievalResult for better caching (elastic#34197) Revert "Search: Fix spelling mistake in Javadoc (elastic#34480)" Search: Fix spelling mistake in Javadoc (elastic#34480) Docs: improve formatting of Query String Query doc page (elastic#34432) Tests: Handle epoch date formatters edge cases (elastic#34437) Test: Fix running with external cluster Fix handling of empty keyword in terms aggregation (elastic#34457) [DOCS] Fix tag in SecurityDocumentationIT [Monitoring] Add additional necessary mappings for apm-server (elastic#34392) SCRIPTING: Move Aggregation Script Context to its own class (elastic#33820) MINOR: Remove Deadcode in ExpressionTermSetQuery (elastic#34442) HLRC: Get SSL Certificates API (elastic#34135)

Security caches the result of role lookups and negative lookups are cached indefinitely. In the case of transient failures this leads to a bad experience as the roles could truly exist. The CompositeRolesStore needs to know if a failure occurred in one of the roles stores in order to make the appropriate decision as it relates to caching. In order to provide this information to the CompositeRolesStore, the return type of methods to retrieve roles has changed to a new class, RoleRetrievalResult. This class provides the ability to pass back an exception to the roles store. This exception does not mean that a request should be failed but instead serves as a signal to the roles store that missing roles should not be cached and neither should the combined role if there are missing roles. As part of this, the negative lookup cache was also changed from an unbounded cache to a cache with a configurable limit. Relates #33205

jaymode added >enhancement v7.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.5.0 labels Oct 1, 2018

jaymode requested review from bizybot and jkakavas October 1, 2018 18:29

bizybot reviewed Oct 4, 2018

View reviewed changes

Merge branch 'master' into role_retrieval_result

b91e631

jaymode added the >breaking-java label Oct 4, 2018

jaymode added 5 commits October 4, 2018 09:03

rename method/variables

7950ebb

simplify code using iterating listener for everything

7762e50

Merge branch 'master' into role_retrieval_result

073f71c

prebuild lists like we do for realms

1e7e602

Merge branch 'master' into role_retrieval_result

d898344

bizybot approved these changes Oct 5, 2018

View reviewed changes

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java Show resolved Hide resolved

jkakavas reviewed Oct 5, 2018

View reviewed changes

jaymode added 5 commits October 5, 2018 08:39

Merge branch 'master' into role_retrieval_result

6b9aa32

add nullable annotation

2984699

failure -> success

a0adc25

minor cleanups

6ba1a8d

fix negative lookup cache settings and add test

4d63622

Merge branch 'master' into role_retrieval_result

cc8c37f

Merge branch 'master' into role_retrieval_result

8bf9a7a

jaymode merged commit 0cd03d3 into elastic:master Oct 15, 2018

jaymode deleted the role_retrieval_result branch October 15, 2018 19:52

jaymode mentioned this pull request Jan 15, 2019

Reduce the need to reload roles from index #33205

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use RoleRetrievalResult for better caching#34197

Use RoleRetrievalResult for better caching#34197
jaymode merged 14 commits intoelastic:masterfrom
jaymode:role_retrieval_result

jaymode commented Oct 1, 2018

Uh oh!

elasticmachine commented Oct 1, 2018

Uh oh!

bizybot left a comment

Uh oh!

bizybot Oct 3, 2018

Uh oh!

Uh oh!

Uh oh!

bizybot left a comment

Uh oh!

Uh oh!

jkakavas left a comment

Uh oh!

jkakavas Oct 5, 2018

Uh oh!

jkakavas Oct 5, 2018

Uh oh!

jkakavas Oct 5, 2018

Uh oh!

jkakavas Oct 5, 2018

Uh oh!

jaymode commented Oct 5, 2018

Uh oh!

jaymode commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jaymode commented Oct 1, 2018

Uh oh!

elasticmachine commented Oct 1, 2018

Uh oh!

bizybot left a comment

Choose a reason for hiding this comment

Uh oh!

bizybot Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bizybot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

jkakavas Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

jkakavas Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

jkakavas Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

jkakavas Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

jaymode commented Oct 5, 2018

Uh oh!

jaymode commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants