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

Lazy compute the index access control #88708

Merged

Conversation

slobodanadamovic
Copy link
Contributor

@slobodanadamovic slobodanadamovic commented Jul 22, 2022

The access control was always computed eagerly, even in cases when
it was not necessary (e.g. shard is not accessed on the node doing
the authorization or in cases when authorization is denied).
This commit defers the computation to when it's really needed and tries
to avoid that the actual work is done on the network worker thread.

This commit is separating authorization check from computation of
index access control. The change is simply a preparation for allowing
the access control to be computed lazily.
@slobodanadamovic slobodanadamovic added >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team labels Jul 22, 2022
@slobodanadamovic slobodanadamovic self-assigned this Jul 22, 2022
@slobodanadamovic
Copy link
Contributor Author

This PR is still a draft as it depends on changes made in #88662.

@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:04
github.com:slobodanadamovic/elasticsearch
into sa-lazy-compute-access-control

# Conflicts:
#	x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
…movic/elasticsearch into sa-lazy-compute-access-control
…movic/elasticsearch into sa-lazy-compute-access-control
…movic/elasticsearch into sa-lazy-compute-access-control
…movic/elasticsearch into sa-lazy-compute-access-control
@slobodanadamovic
Copy link
Contributor Author

The elasticsearch-ci/part-2 build failure is caused by the same issue as reported in [CI] CartesianShapeQueryTests testQueryRandomGeoCollection failing.

@slobodanadamovic
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-2

@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
…compute-access-control

# Conflicts:
#	x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM nice job!

Left a couple of small issues. Please look into those, though they don't require me to take another look.

@albertzaharovits
Copy link
Contributor

@ycombinator We discussed some time ago that I'll ping you when we get ready to merge this change so that you can validate on the cloud side that all is well.

This change makes DLS/FLS feature tracking not report usage on the coordinating-only nodes, in most cases (when the feature is licensed). It will continue to report usage on the data nodes, as usual.

@ycombinator
Copy link
Contributor

@ycombinator We discussed some time ago that I'll ping you when we get ready to merge this change so that you can validate on the cloud side that all is well.

This change makes DLS/FLS feature tracking not report usage on the coordinating-only nodes, in most cases (when the feature is licensed). It will continue to report usage on the data nodes, as usual.

Thanks for the ping, @albertzaharovits. I want to make sure I understand the impact of the change correctly. Before this change DLS/FLS feature tracking (if the feature was licensed) was being reported by all nodes? And after this change this will only be reported by data nodes?

@slobodanadamovic
Copy link
Contributor Author

@ycombinator

I want to make sure I understand the impact of the change correctly. Before this change DLS/FLS feature tracking (if the feature was licensed) was being reported by all nodes? And after this change this will only be reported by data nodes?

That is correct.

With this change, the DLS/FLS feature usage will be reported by data nodes (as it did before), with just a small exception that it might not be reported by coordinating only nodes in some cases. Hence, I think it's fine to assume that coordinating-only nodes will not report the feature usage anymore and only data nodes will report it.

@ycombinator
Copy link
Contributor

ycombinator commented Jan 13, 2023

With this change, the DLS/FLS feature usage will be reported by data nodes (as it did before), with just a small exception that it might not be reported by coordinating only nodes in some cases. Hence, I think it's fine to assume that coordinating-only nodes will not report the feature usage anymore and only data nodes will report it.

Thanks for clarifying, @slobodanadamovic. Just to confirm, this change will not impact the results of DLS/FLS feature usage for a cluster, right? Meaning, whatever DLS/FLS feature usage the cluster was reporting before, it will report after this change as well --- the only difference is what nodes may report that usage now, yes?

@slobodanadamovic
Copy link
Contributor Author

slobodanadamovic commented Jan 13, 2023

Thanks for clarifying, @slobodanadamovic. Just to confirm, this change will not impact the results of DLS/FLS feature usage for a cluster, right? Meaning, whatever DLS/FLS feature usage the cluster was reporting before, it will report after this change as well --- the only difference is what nodes may report that usage now, yes?

Correct.

@ycombinator
Copy link
Contributor

Thanks for clarifying, @slobodanadamovic. Just to confirm, this change will not impact the results of DLS/FLS feature usage for a cluster, right? Meaning, whatever DLS/FLS feature usage the cluster was reporting before, it will report after this change as well --- the only difference is what nodes may report that usage now, yes?

Correct.

Perfect, thanks @slobodanadamovic.

That means this change is a no-op from a Billing perspective, given that our process gets feature usage data from every ES node and then aggregates the results downstream. cc: @blfrantz as an FYI.

@slobodanadamovic
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-3-fips

1 similar comment
@slobodanadamovic
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-3-fips

@slobodanadamovic
Copy link
Contributor Author

@elasticmachine update branch

@slobodanadamovic slobodanadamovic merged commit 06552d0 into elastic:main Jan 17, 2023
@slobodanadamovic slobodanadamovic deleted the sa-lazy-compute-access-control branch January 17, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants