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

Fixes #38206 - make token endpoint work concurrently #11306

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

ianballou
Copy link
Member

What are the changes introduced in this pull request?

Adds retry and transaction logic to ensure that two concurrent clients won't trigger a database race condition.

Considerations taken when implementing this change?

Is the transaction necessary?

What are the testing steps for this pull request?

  1. Destroy all PersonalAccessToken records called 'registry'.
  2. Instruct puma to run with a single worker (-w 1).
  3. Add a debugger right before personal_token.save! on line 489.
  4. podman login on two separate terminals at the same time.
  5. See the two requests hit the breakpoint in the logs.
  6. Let both continue and see that neither trigger an error.

These reproducer steps simulate any number of hosts that might be trying to log in at the same time.

Copy link
Contributor

@qcjames53 qcjames53 left a comment

Choose a reason for hiding this comment

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

I was able to verify that this PR allows for any number of users to request OCI auth at the exact same time. Users are still rejected as normal when improper credentials are supplied. The source looks good to me and I don't think any unit test changes are needed. ACKing!

@ianballou ianballou merged commit 86ea51e into Katello:master Feb 13, 2025
19 checks passed
@ianballou ianballou deleted the 38206-osp-container-pull-fail branch February 13, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants