perf(oidc): disable push of user token meta-event by muhlemmer · Pull Request #8691 · zitadel/zitadel

muhlemmer · 2024-09-26T13:06:42Z

Which Problems Are Solved

When executing many concurrent authentication requests on a single machine user, there were performance issues. As the same aggregate is being searched and written to concurrently, we traced it down to a locking issue on the used index.
We already optimized the token endpoint by creating a separate OIDC aggregate.

At the time we decided to push a single event to the user aggregate, for the user audit log. See technical advisory 10010 for more details.

However, a recent security fix introduced an additional search query on the user aggregate, causing the locking issue we found.

How the Problems Are Solved

Add a feature flag which disables pushing of the user.token.v2.added. The event has no importance and was only added for informational purposes on the user objects. The oidc_session.access_token.added is the actual payload event and is pushed on the OIDC session aggregate and can still be used for audit trail.

Additional Changes

Fix an event mapper type for SystemOIDCSingleV1SessionTerminationEventType

Additional Context

Reported by support request
perf(oidc): optimize token creation #7822 changed the token aggregate
fix: correctly check user state #8631 introduced user state check

Load test trace graph with user.token.v2.added enabled. Query times are steadily increasing:

Load test trace graph with user.token.v2.added disabled. Query times constant:

This reverts commit d10d848.

vercel · 2024-09-26T13:06:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 26, 2024 1:55pm

github-actions · 2024-09-26T13:06:56Z

Thanks for your contribution @muhlemmer! 🎉

Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:

I am happy with the code
Documentations and examples are up-to-date
Logical behavior changes are tested automatically
No debug or dead code
My code has no repetitions
The PR title adheres to the conventional commit format
The example texts in the PR description are replaced.
If there are any open TODOs or follow-ups, they are described in issues and link to this PR
If there are deviations from a user stories acceptance criteria or design, they are agreed upon with the PO and documented.

codecov · 2024-09-26T13:20:52Z

Codecov Report

Attention: Patch coverage is 71.05263% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@7247f62). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/command/instance_features_model.go	40.00%	3 Missing ⚠️
internal/command/system_features_model.go	40.00%	3 Missing ⚠️
internal/query/instance_features_model.go	50.00%	2 Missing ⚠️
internal/query/system_features_model.go	33.33%	2 Missing ⚠️
internal/feature/key_enumer.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8691   +/-   ##
=======================================
  Coverage        ?   53.21%           
=======================================
  Files           ?     1512           
  Lines           ?   123307           
  Branches        ?        0           
=======================================
  Hits            ?    65620           
  Misses          ?    54313           
  Partials        ?     3374

Flag	Coverage Δ
core-integration-tests-postgres	`11.56% <26.31%> (?)`
core-unit-tests	`47.85% <55.26%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Which Problems Are Solved When executing many concurrent authentication requests on a single machine user, there were performance issues. As the same aggregate is being searched and written to concurrently, we traced it down to a locking issue on the used index. We already optimized the token endpoint by creating a separate OIDC aggregate. At the time we decided to push a single event to the user aggregate, for the user audit log. See [technical advisory 10010](https://zitadel.com/docs/support/advisory/a10010) for more details. However, a recent security fix introduced an additional search query on the user aggregate, causing the locking issue we found. # How the Problems Are Solved Add a feature flag which disables pushing of the `user.token.v2.added`. The event has no importance and was only added for informational purposes on the user objects. The `oidc_session.access_token.added` is the actual payload event and is pushed on the OIDC session aggregate and can still be used for audit trail. # Additional Changes - Fix an event mapper type for `SystemOIDCSingleV1SessionTerminationEventType` # Additional Context - Reported by support request - #7822 changed the token aggregate - #8631 introduced user state check Load test trace graph with `user.token.v2.added` **enabled**. Query times are steadily increasing: ![image](https://github.com/user-attachments/assets/4aa25055-8721-4e93-b695-625560979909) Load test trace graph with `user.token.v2.added` **disabled**. Query times constant: ![image](https://github.com/user-attachments/assets/a7657f6c-0c55-401b-8291-453da5d5caf9) --------- Co-authored-by: Livio Spring <livio.a@gmail.com> (cherry picked from commit 63d733b)

# Which Problems Are Solved When executing many concurrent authentication requests on a single machine user, there were performance issues. As the same aggregate is being searched and written to concurrently, we traced it down to a locking issue on the used index. We already optimized the token endpoint by creating a separate OIDC aggregate. At the time we decided to push a single event to the user aggregate, for the user audit log. See [technical advisory 10010](https://zitadel.com/docs/support/advisory/a10010) for more details. However, a recent security fix introduced an additional search query on the user aggregate, causing the locking issue we found. # How the Problems Are Solved Add a feature flag which disables pushing of the `user.token.v2.added`. The event has no importance and was only added for informational purposes on the user objects. The `oidc_session.access_token.added` is the actual payload event and is pushed on the OIDC session aggregate and can still be used for audit trail. # Additional Changes - Fix an event mapper type for `SystemOIDCSingleV1SessionTerminationEventType` # Additional Context - Reported by support request - #7822 changed the token aggregate - #8631 introduced user state check Load test trace graph with `user.token.v2.added` **enabled**. Query times are steadily increasing: ![image](https://github.com/user-attachments/assets/4aa25055-8721-4e93-b695-625560979909) Load test trace graph with `user.token.v2.added` **disabled**. Query times constant: ![image](https://github.com/user-attachments/assets/a7657f6c-0c55-401b-8291-453da5d5caf9) --------- Co-authored-by: Livio Spring <livio.a@gmail.com> (cherry picked from commit 63d733b) (cherry picked from commit f59d3ae)

# Which Problems Are Solved When executing many concurrent authentication requests on a single machine user, there were performance issues. As the same aggregate is being searched and written to concurrently, we traced it down to a locking issue on the used index. We already optimized the token endpoint by creating a separate OIDC aggregate. At the time we decided to push a single event to the user aggregate, for the user audit log. See [technical advisory 10010](https://zitadel.com/docs/support/advisory/a10010) for more details. However, a recent security fix introduced an additional search query on the user aggregate, causing the locking issue we found. # How the Problems Are Solved Add a feature flag which disables pushing of the `user.token.v2.added`. The event has no importance and was only added for informational purposes on the user objects. The `oidc_session.access_token.added` is the actual payload event and is pushed on the OIDC session aggregate and can still be used for audit trail. # Additional Changes - Fix an event mapper type for `SystemOIDCSingleV1SessionTerminationEventType` # Additional Context - Reported by support request - #7822 changed the token aggregate - #8631 introduced user state check Load test trace graph with `user.token.v2.added` **enabled**. Query times are steadily increasing: ![image](https://github.com/user-attachments/assets/4aa25055-8721-4e93-b695-625560979909) Load test trace graph with `user.token.v2.added` **disabled**. Query times constant: ![image](https://github.com/user-attachments/assets/a7657f6c-0c55-401b-8291-453da5d5caf9) --------- Co-authored-by: Livio Spring <livio.a@gmail.com> (cherry picked from commit 63d733b) (cherry picked from commit f59d3ae) (cherry picked from commit 555ecf0)

# Which Problems Are Solved When executing many concurrent authentication requests on a single machine user, there were performance issues. As the same aggregate is being searched and written to concurrently, we traced it down to a locking issue on the used index. We already optimized the token endpoint by creating a separate OIDC aggregate. At the time we decided to push a single event to the user aggregate, for the user audit log. See [technical advisory 10010](https://zitadel.com/docs/support/advisory/a10010) for more details. However, a recent security fix introduced an additional search query on the user aggregate, causing the locking issue we found. # How the Problems Are Solved Add a feature flag which disables pushing of the `user.token.v2.added`. The event has no importance and was only added for informational purposes on the user objects. The `oidc_session.access_token.added` is the actual payload event and is pushed on the OIDC session aggregate and can still be used for audit trail. # Additional Changes - Fix an event mapper type for `SystemOIDCSingleV1SessionTerminationEventType` # Additional Context - Reported by support request - zitadel#7822 changed the token aggregate - zitadel#8631 introduced user state check Load test trace graph with `user.token.v2.added` **enabled**. Query times are steadily increasing: ![image](https://github.com/user-attachments/assets/4aa25055-8721-4e93-b695-625560979909) Load test trace graph with `user.token.v2.added` **disabled**. Query times constant: ![image](https://github.com/user-attachments/assets/a7657f6c-0c55-401b-8291-453da5d5caf9) --------- Co-authored-by: Livio Spring <livio.a@gmail.com> (cherry picked from commit de9792e)

muhlemmer added 3 commits September 25, 2024 16:28

perf(oidc): do not push user token meta-event

d10d848

Revert "perf(oidc): do not push user token meta-event"

be53c3d

This reverts commit d10d848.

use feature flag for user token event

5327814

muhlemmer requested a review from livio-a September 26, 2024 13:16

livio-a previously approved these changes Sep 26, 2024

View reviewed changes

Merge branch 'main' into perf-user-token

c5547e6

muhlemmer enabled auto-merge (squash) September 26, 2024 13:30

fix unit test

c07d349

muhlemmer dismissed livio-a’s stale review via c07d349 September 26, 2024 13:39

livio-a approved these changes Sep 26, 2024

View reviewed changes

vercel Bot deployed to Preview September 26, 2024 13:55 View deployment

muhlemmer merged commit 63d733b into main Sep 26, 2024

muhlemmer deleted the perf-user-token branch September 26, 2024 13:55

This was referenced Dec 16, 2024

Restore user audit trail #9064

Closed

Stop pushing machine secret check events #9083

Closed

Navigation Menu

perf(oidc): disable push of user token meta-event - #8691

perf(oidc): disable push of user token meta-event#8691
muhlemmer merged 5 commits into
mainfrom
perf-user-token

muhlemmer commented Sep 26, 2024

Uh oh!

vercel Bot commented Sep 26, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 26, 2024

Uh oh!

codecov Bot commented Sep 26, 2024 •

edited

Loading

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

muhlemmer commented Sep 26, 2024

Which Problems Are Solved

How the Problems Are Solved

Additional Changes

Additional Context

Uh oh!

vercel Bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 26, 2024

Thanks for your contribution @muhlemmer! 🎉

Uh oh!

codecov Bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented Sep 26, 2024 •

edited

Loading

codecov Bot commented Sep 26, 2024 •

edited

Loading