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

Move hashing on API key creation to crypto thread pool #74165

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jun 16, 2021

The changes in #74106 make API keys cached on creation time. It helps avoid the expensive hashing operation on initial authentication when a request using the key hits the same node that creates the key. Since the more expensive hashing on authentication time is handled by a dedicated "crypto" thread pool (#58090), it is expected that usage of the "crypto" thread pool to be reduced.

This PR moves the hashing on creation time to the "crypto" thread pool so that a similar (before #74106) usage level of "crypto" thread pool is maintained. It also has the benefit to avoid costly operations in the transport_worker thread, which is generally preferred.

Relates: #74106

@ywangd ywangd added >enhancement :Security/Security Security issues without another label v8.0.0 v7.14.0 labels Jun 16, 2021
@ywangd ywangd requested a review from tvernum June 16, 2021 08:20
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jun 16, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM.

@tvernum
Copy link
Contributor

tvernum commented Jun 18, 2021

This does mean that API key creation is potentially slower (due to intentionally limiting the number of concurrent threads that will do crypto), so I think it's worth making @ruflin and @scunningham aware.

Overall it should be better from a QoS point of view because it reduces the likelihood that API Key creation will steal CPU time from ingestion. But the effect could show up as a change in some benchmarks that focus on API Key setup.

@ruflin
Copy link
Contributor

ruflin commented Jun 18, 2021

Does the team have benchmarks in place that will show the diff if there is one?

@ywangd
Copy link
Member Author

ywangd commented Jun 18, 2021

Does the team have benchmarks in place that will show the diff if there is one?

No I am not aware of any benchmarks that are focused on creating API keys.

The crypto thread pool by default has half of the allocated processors while the transport_worker has all of the allocated processors. Just based on these numbers, it may seem that the performance for API key creation could drop. But transport_worker is a very busy thread pool since it handles every communication goes between nodes, e.g. ingestion, plus some additional computations that just ride along, e.g. authorization code. On the other hand, crypto thread pool is dedicated to only API key secret hashing for the time being and nothing else. So it is hard to say that performace would be worse after this change. Also, like Tim mentioned, from the QoS perspective, it is reasonably to consider this change would be beneficial because transport_worker has many other critical tasks, e.g. heartbeat check between nodes which could cause cluster instability in case of failures.

@ywangd ywangd merged commit a365bcc into elastic:master Jun 22, 2021
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 22, 2021
The changes in elastic#74106 make API keys cached on creation time. It helps avoid the
expensive hashing operation on initial authentication when a request using the
key hits the same node that creates the key. Since the more expensive hashing
on authentication time is handled by a dedicated "crypto" thread pool (elastic#58090),
it is expected that usage of the "crypto" thread pool to be reduced.

This PR moves the hashing on creation time to the "crypto" thread pool so that
a similar (before elastic#74106) usage level of "crypto" thread pool is maintained. It
also has the benefit to avoid costly operations in the transport_worker thread,
which is generally preferred.

Relates: elastic#74106
ywangd added a commit that referenced this pull request Jun 22, 2021
The changes in #74106 make API keys cached on creation time. It helps avoid the
expensive hashing operation on initial authentication when a request using the
key hits the same node that creates the key. Since the more expensive hashing
on authentication time is handled by a dedicated "crypto" thread pool (#58090),
it is expected that usage of the "crypto" thread pool to be reduced.

This PR moves the hashing on creation time to the "crypto" thread pool so that
a similar (before #74106) usage level of "crypto" thread pool is maintained. It
also has the benefit to avoid costly operations in the transport_worker thread,
which is generally preferred.

Relates: #74106
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 22, 2021
Since elastic#74165, API key creation has one additional async layer. Hence the
test assertions are now executed asynchronously and may not get called
in time before the test ends. This PR ensures the assertions are always
called by waiting for a flag.

Resolves: elastic#74427
ywangd added a commit that referenced this pull request Jun 23, 2021
Since #74165, API key creation has one additional async layer. Hence the
test assertions are now executed asynchronously and may not get called
in time before the test ends. This PR ensures the assertions are always
called by waiting for a flag.

Resolves: #74427
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 23, 2021
…#74431)

Since elastic#74165, API key creation has one additional async layer. Hence the
test assertions are now executed asynchronously and may not get called
in time before the test ends. This PR ensures the assertions are always
called by waiting for a flag.
ywangd added a commit that referenced this pull request Jun 24, 2021
…#74527)

Since #74165, API key creation has one additional async layer. Hence the
test assertions are now executed asynchronously and may not get called
in time before the test ends. This PR ensures the assertions are always
called by waiting for a flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants