Skip to content
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

Use RoleRetrievalResult for better caching #34197

Merged
merged 14 commits into from
Oct 15, 2018

Conversation

jaymode
Copy link
Member

@jaymode jaymode commented Oct 1, 2018

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
@jaymode jaymode added >enhancement v7.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.5.0 labels Oct 1, 2018
@jaymode jaymode requested review from bizybot and jkakavas October 1, 2018 18:29
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

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

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

allSuccessful = false;
}

private boolean hadFailures() {
Copy link
Contributor

Choose a reason for hiding this comment

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

RolesRetrievalResult#isFailure()

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

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

Small change else LGTM, Thank you for the iteration.

Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

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

private final Set<RoleDescriptor> descriptors;
private final Exception failure;

private RoleRetrievalResult(Set<RoleDescriptor> descriptors, Exception failure) {
Copy link
Member

Choose a reason for hiding this comment

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

Mark as @Nullable ?

failure = true;
}

private boolean isFailure() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

I think merging the two if will not hurt readability.

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

Choose a reason for hiding this comment

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

s/cacheSize/nlcCacheSize

@jaymode
Copy link
Member Author

jaymode commented Oct 5, 2018

@elasticmachine run packaging tests

@jaymode
Copy link
Member Author

jaymode commented Oct 10, 2018

run gradle build tests

@jaymode jaymode merged commit 0cd03d3 into elastic:master Oct 15, 2018
@jaymode jaymode deleted the role_retrieval_result branch October 15, 2018 19:52
jaymode added a commit that referenced this pull request Oct 15, 2018
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
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 15, 2018
* 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)
kcm pushed a commit that referenced this pull request Oct 30, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants